[ac-pplx]

Problem

LoadSnapshot checked the request's rootfs_rate_limit only after it had staged host state. A request with a malformed limit (a DriveRateLimit missing its bandwidth or operations bucket) was refused with InvalidArgument. By the time it was refused, node-manager had already done the following:

• Plaintext load (source_dir set): copied vmstate into the sandbox jail, re-owned to the jail uid the staged in-jail copy of vmstate (stage_open_file returns the in-jail file) and the source memory file, and bind-mounted memory into the jail.
• Encrypted load (snapshot_volume_mount_dir set): chowned the volume's vmstate, memory and root to the jail uid, and bind-mounted the volume into the jail.

The refusal does not undo these changes. The code comment only promised to validate "before the load".

Root cause

drive_rate_limiter (crates/libs/node/mgr/server/src/service/snapshots.rs:1204 on f48eb2b) ran inside finish_load_snapshot (:680, parse at :692). That is the shared tail of both load paths, and both paths call it only after their host staging:

• Plaintext: stage_plaintext_snapshot_inputs (copies vmstate into the jail) → reown(&vmstate) on the in-jail copy, reown(&memory) on the source file → bind_pinned_files (:647–:658).
• Encrypted: pin_encrypted_snapshot_bind (re-owns the volume) → bind_pinned_files (:602–:607).

Note: the fix commit's message says "re-owned the source vmstate and memory files"; the accurate wording is the one above (the in-jail copy of vmstate and the source memory file).

Fix

The limit is now parsed at the top of load_snapshot_inner, right after validate_sandbox_id. The parse now also precedes the lifecycle permit, the lease lookup and the path checks (validate_snapshot_volume / load_snapshot_paths). The parsed RateLimiter goes to finish_load_snapshot, which only applies it after the Firecracker load. finish_load_snapshot now takes the sandbox id and the parsed limiter instead of the raw request. It derives the API socket path from jail_root, as it already does for v.sock. Its argument count is unchanged, so no too_many_arguments allow is needed.

Design note

Semantics

The order in which LoadSnapshot checks things (the execution fence and VM lock in load_snapshot come first and are unchanged):

Step	Before	After
1	validate_sandbox_id	validate_sandbox_id
2	lifecycle permit (LoadSnapshot)	rootfs_rate_limit parse
3	lease lookup	lifecycle permit
4	path checks (validate_snapshot_volume / load_snapshot_paths)	lease lookup
5	host staging (stage/reown/bind)	path checks
6	rootfs_rate_limit parse	host staging
7	API socket wait → FC load → apply limiter → v.sock	unchanged

What a caller gets back:

Request	Before	After
Malformed limit, no staging inputs	InvalidArgument rootfs_rate_limit.<bucket> is required	same
Malformed limit + valid source_dir	same error, after vmstate copy, chown and memory bind	same error, before any host work
Malformed limit + encrypted volume	same error, after volume chown and bind	same error, before any host work
Malformed limit + no lease, or + a bad path	Unavailable / path error	InvalidArgument (rate limit)
Well-formed or absent limit	unchanged	unchanged

Why this is correct

• docs/rust/principles.md §1 (line 36, and the boundary rule at line 194): parse a wire value once at the boundary and pass the typed value inward. The limit is now parsed once into RateLimiter before any work. The tail gets the typed value, so it can no longer re-parse or reach into the raw request.
• The parse needs no host or node state. Nothing justified running it after host mutation, and the rollback machinery (rollback_loaded_snapshot) only covers failures after the load. The staging steps have no undo on a validation refusal.
• The guards that already ran first stay ahead of the hoisted parse: the execution fence (check_execution, FailedPrecondition) and the VM-cgroup lock in load_snapshot. No CAS or fence semantics change.

Alternatives considered

• Hoist every pure validator (volume ownership, artifact names) above the permit and lease lookup as well. Not done. These checks already run before any host staging, so they never had this bug. Moving them would change which refusal wins for requests that are invalid in two ways, and is out of scope. load_snapshot_paths also stats the checkpoint files, which reads host state, so it cannot move above the lease without splitting it (a refactor).
• Parse at the top but keep a second parse in the tail. Rejected: that duplicates the check and leaves two places that decide.
• Add #[allow(clippy::too_many_arguments)] and keep api_socket as a parameter. Rejected in favour of deriving it from jail_root inside the tail. Both callers computed it the same way.
• Roll back staged state on a late refusal. Rejected: validating first is simpler and leaves nothing to roll back.

Client-visible effects

• Same status code and message (InvalidArgument, rootfs_rate_limit.bandwidth is required / rootfs_rate_limit.operations is required).
• The refusal now comes back sooner. It no longer waits for a lifecycle permit or does blocking host work first, and it no longer leaves a staged jail, re-owned snapshot files or a bind mount behind.
• Precedence: a request that also lacks a lease, or also has a bad path, now gets the rate-limit InvalidArgument instead of Unavailable or the path error. sandbox-manager, the in-tree caller, always sends both buckets (drive_rate_limit_proto, crates/libs/sbx/mgr/server/src/service/start.rs), so it never hits this refusal.
• No new refusal or deferral path, so no new log or metric. This is the existing refusal, returned earlier.

Risks

• Low. Well-formed requests and requests with no limit go through the same steps in the same order. The only reordering is the parse relative to the permit, lease lookup and path checks, which matters only for requests that are already invalid.

Rolling-deploy safety

• Node-local change to node-manager only. No proto, wire, schema or persisted-format change. The LoadSnapshotRequest fields are unchanged.
• Old and new node-managers side by side: a node still on the old binary keeps the late refusal. A node on the new binary refuses early. Both return the same code and message, and both accept every well-formed request the same way, so sandbox-manager and the API replicas see no difference.
• Old sandbox-manager with new node-manager, or the reverse: the in-tree sandbox-manager sends well-formed limits, so the restore path is unaffected in every combination.

Tests

Rows added to the existing load_snapshot_refuses_invalid_requests_before_staging_the_jail table (service/snapshots/tests.rs), not a new function. The table runs through the LoadSnapshot RPC entry point against refuse_host_work: a pre-cancelled artifact-work owner, so the first blocking host step returns Cancelled.

• checkpoint load with a rate limit without bandwidth: plaintext with the sandbox's own source_dir. Expects InvalidArgument. This row pins "the parse precedes the path checks", not the staging barrier (see below).
• encrypted load with a rate limit without operations: the sandbox's own snapshot volume. Expects InvalidArgument. This is the row that proves the validation error comes before the barrier.
• valid encrypted load with a rate limit: the accepting control. A well-formed limit is accepted, and the load stops at the refused volume bind (Cancelled).
• The two existing rate-limit rows that stage nothing (rate limit without bandwidth / without operations) now run against the bound but unallocated sandbox. On the fixed code they still expect the rate-limit InvalidArgument. If the parse regresses, they stop at the lease lookup (Unavailable) instead of reaching the shared tail's poll of the fixed jail socket.
• The other existing rows still pass: fence, lease, paths, and the valid encrypted load control. Every row still checks that the lease is undisturbed and that no lifecycle permit leaked.

Limitation of the plaintext row. load_snapshot_paths → validate_load_snapshot_source stats vmstate/memory under the fixed cache root (SANDBOX_CACHE_DIR is a constant: /space/cache/firecracker/sandboxes/<id>/snapshot) before staging. That means a test cannot reach the plaintext staging barrier without creating files on a fixed host path, which is not allowed. So on main the plaintext row fails at that read-only stat of a fresh-UUID path (NotFound here; host-dependent elsewhere) rather than at the barrier. After the fix it is refused before the stat. If the files somehow existed, staging would still stop at the barrier, so the row fails closed either way. So this row pins "the parse precedes the path checks"; the encrypted row is the one that proves validation comes before the barrier. There is no "valid checkpoint load" control because that stat's outcome depends on the host. The table comment says so.

Red (commit 1 on unchanged code; the UUIDs are fresh test-generated ids):

thread 'service::snapshots::tests::load_snapshot_refuses_invalid_requests_before_staging_the_jail' (16616) panicked at crates/libs/node/mgr/server/src/service/snapshots/tests.rs:727:5:
[
    "checkpoint load with a rate limit without bandwidth: code: 'Some requested entity was not found', message: \"snapshot artifact /space/cache/firecracker/sandboxes/01a127a7-d012-7120-a4fc-fd1f139079ed/snapshot/vmstate not found: No such file or directory (os error 2)\"",
    "encrypted load with a rate limit without operations: code: 'The operation was cancelled', message: \"artifact work cancelled\"",
]

Mutation proof. In the fix commit, I replaced the hoisted .map(drive_rate_limiter).transpose()? with .and_then(|limit| drive_rate_limiter(limit).ok()), so a malformed limit is silently dropped. Four rows fail, and the test finishes in about 10 ms:

thread 'service::snapshots::tests::load_snapshot_refuses_invalid_requests_before_staging_the_jail' (165654) panicked at crates/libs/node/mgr/server/src/service/snapshots/tests.rs:730:5:
[
    "rate limit without bandwidth: code: 'The service is currently unavailable', message: \"snapshot lease is no longer active\"",
    "rate limit without operations: code: 'The service is currently unavailable', message: \"snapshot lease is no longer active\"",
    "checkpoint load with a rate limit without bandwidth: code: 'Some requested entity was not found', message: \"snapshot artifact /space/cache/firecracker/sandboxes/01a127ef-6cf8-74a6-a2f7-d801354d7320/snapshot/vmstate not found: No such file or directory (os error 2)\"",
    "encrypted load with a rate limit without operations: code: 'The operation was cancelled', message: \"artifact work cancelled\"",
]

Host safety of that regression path: the unstaged rows stop at the lease lookup. The checkpoint row reaches the read-only stat of a fresh-UUID path under the fixed cache root. The encrypted row stops at the refused bind. No row polls or connects to a Firecracker socket, and nothing is written, chowned or bound. On the fixed code none of these steps is reached.

Validation

Every result below is for head c0f146ff87684568379863784f7cb250d258c4cc on base d29873e3efb82ab2d4d126dc3138d6f37b777a80. The Kind harness is the post-#154 one: SPACE Kind node image space-kind-node:15d575fd832acf75 on the sandbox workers and builders, and one DaemonSet per node service. The cluster was created fresh with teardown.sh, then the smoke-kind-e2e.sh bring-up.

Check	Result
ci/scripts/run-rust-checks.sh fmt (env contract, Rust error policy, cargo fmt --check)	pass
cargo clippy --workspace --locked --all-targets --all-features -- -D warnings, and the same scoped to -p pplx-space-node-mgr	pass
pre-commit (gitleaks)	pass
cargo nextest run -p pplx-space-node-mgr --retries 0 ×5, with SPACE_CI_REQUIRE_TEST_DEPS=1 and a skip log	448 passed ×5. 0 dependency-skip markers. The 9 #[ignore] root/btrfs-fixture tests were not selected; they are unrelated to this path.
Red (commit 1) / parse-suppression mutation (fix)	fails as shown above
Kind provenance	Tilt runs from this worktree at the head above with a clean tree. All 212 tracked files under crates/libs/node/{mgr,runtime} in the synced build input are byte-identical to the head, including the head-only comment Validate before either path stages the jail, which is absent on the base. The release build of that input, space-nodemgr-svc sha256 ec2fb94e…3d11749, is what runs on all four node-manager pods (worker and builder DaemonSets). Release binaries are stripped and this change adds no string literal, so no meaningful string in the binary is unique to the head. Instead, the same build with only this PR's two files reverted to the base recompiles just pplx-space-node-mgr/-svc and yields cdbbbcc6…f3354c1, which differs from the deployed binary.
Kind: Tilt quiet (no pending builds, no ReplicaSet or pod < 180 s), then 120 s settle re-checked every 5 s	reached before the scenario runs
Kind individual, ECW surface: ecw.pause_connect, ecw.pause_timeout_requires_connect, ecw.access_idle_reconnect_preserves_workspace, local.encrypted_resuspend_preserves_cow (BYOK encrypted load), local.suspend_restore_cross_node_from_object_storage, local.suspend_snapshot_rows_are_per_attempt, network_policy.v1_rules_survive_restore	7/7 PASS
Kind individual, native surface: ecw.pause_connect	PASS. Six of the seven scenarios above are ECW-only (registry default api_surfaces=("ecw",)). native.resident_pause also passed but never calls LoadSnapshot, so it is not evidence for this change. The other native evidence is the native typed suite below.
smoke-kind-typed.sh all (SPACE_SMOKE_KIND_COLD_RECEIPTS set)	ECW 75 passed / 0 failed / 0 inconclusive; native 23 passed / 0 failed / 0 inconclusive
smoke-kind-e2e.sh (bring-up baseline, then again after the typed suite)	PASS / PASS

No failures, so nothing to classify. The known pre-existing flakes did not occur.

Follow-ups

• The pure parts of load_snapshot_paths (artifact names) and validate_snapshot_volume could move ahead of the lifecycle permit, so invalid requests never queue for one. This would change which refusal wins for requests that are invalid in two ways, so it is left out of this PR.
• An injectable cache root would allow a plaintext row that reaches the staging barrier. It would also allow a test that the parsed limiter actually reaches update_drive_rate_limiter. That is a pre-existing gap: replacing rootfs_limiter.as_ref() with None at either finish_load_snapshot call site currently passes every test.

🤖 Generated with Claude Code

[the-gigi-pplx]

Review — 4 findings

Low

1. crates/libs/node/mgr/server/src/service/snapshots.rs:690 — nothing checks that the parsed limiter actually reaches update_drive_rate_limiter.
   I replaced both rootfs_limiter.as_ref() with None and cargo test -p pplx-space-node-mgr --lib snapshots stayed 19/19 green. The PR body lists this under Follow-ups as a pre-existing gap, but this PR rewires exactly that plumbing, so it'd be a good place to close it with an injectable cache root or a Firecracker API fake.
2. crates/libs/node/mgr/server/src/service/snapshots.rs:578 — the parse moves to the top of load_snapshot_inner, not into load_snapshot.
   A malformed request still waits on run_owned_sandbox_work, the cgroup gate and family locks, and check_execution before it gets InvalidArgument. Under contention it can surface as "timed out waiting for sandbox cleanup or reset" instead. None of that is host state, so the PR's purpose holds. Optionally, parse next to the outer validate_sandbox_id at :552.
3. crates/libs/node/mgr/server/src/service/snapshots/tests.rs:744 — the per-row "leaked lifecycle permit" check can't fire for the four rate-limit rows.
   The parse (snapshots.rs:578) now runs before lifecycle_limiter.acquire (:584), so those rows never take a permit. No row claims otherwise; it's only worth knowing that the check is vacuous there.
4. PR body, Root cause — the paragraph about the fix commit's message sits between the Plaintext and Encrypted bullets and breaks the list. It reads as review narration rather than a description of the change. Its content is right (stage_plaintext_snapshot_inputs at snapshots.rs:164-176 returns the in-jail copy), so it could move to a note under the list or go.

I listed every host-side effect on the path in order: authorize, id validation, owned-work claim, gate and family lock, check_execution, the new parse, the lifecycle permit, the lease lookup, then the branch work. Every host mutation (chown, copy, reown, bind) is inside off_runtime, after the parse. There's no other caller of finish_load_snapshot or load_snapshot_inner. The set of rejected requests doesn't change. For a request that was already invalid, only the error that wins changes.

Verified against c0f146ff87684568379863784f7cb250d258c4cc. Reproduced: the new load_snapshot_refuses_invalid_requests_before_staging_the_jail passes at head. With snapshots.rs reverted to base and the head tests kept, it fails on four rows: both unallocated rows with Unavailable, the checkpoint row with NotFound, and the encrypted row with Cancelled. That matches the PR body. The limiter→None mutation above stayed green.

[the-gigi-pplx]

Approved. 4 non-blocking findings in the review comment above.

[ac-pplx]

Thanks. On the four findings:

1. Limiter reaching update_drive_rate_limiter. Agreed, nothing pins that today. Closing it needs a Firecracker API fake or an injectable cache root. That's the node-mgr seam work now underway (node-mgr: let tests inject the runtime supervisor and host-reading defaults #298 is the first part), so I'm tracking it there rather than growing this PR.
2. Parse placement. Leaving it at the top of load_snapshot_inner, since nothing on that earlier path is host state. Moving it next to the outer validate_sandbox_id is noted as a follow-up.
3. Leaked-permit check on the rate-limit rows. Noted. It's vacuous there by construction, because the parse now runs before the permit.
4. Body. Fixed: the commit-message note now sits under the list.