[nickdujay-pplx]

Summary

A failed restore can be retried after its rollback. The node then snapshots the sandbox again. The next restore uses that new snapshot instead of the original one.

This PR makes the sandbox manager refuse that late retry. The successor restore then uses the original snapshot. No new snapshot or upload job is created.

Self-assessed severity: High. The bug silently replaces a sandbox's restore source with a snapshot taken from an attempt that the system already rolled back. Users keep their files in the cases we tested, but the node and the control plane disagree on which artifact is current.

Terms

• Generation N: one execution of a sandbox. Each restore normally gets a new generation.
• Activation attempt: one try at starting generation N. Kubernetes can retry the same attempt.
• Rollback: the node undoes a failed restore and records a rollback receipt for that attempt.
• Original artifact: the snapshot and local artifact written by the last good suspend.
• Eviction: the node deletes an idle, backed-up sandbox from local disk.

Problem

This sequence reproduces the bug:

1. A sandbox is suspended. This writes the original artifact.
2. A restore of generation 2 fails while it prepares. For example, the node rejects it for disk space.
3. The node rolls back generation 2. The sandbox is suspended again.
4. Kubernetes retries the same activation attempt about 15 seconds later. The restore now succeeds.
5. The next wake creates generation 3 and deletes the generation 2 pod.
6. Deleting that pod triggers a teardown suspend of generation 2. This writes a new snapshot and finalizes a new backup.
7. Generation 3 restores from the new snapshot, not the original.

There is a second path to the same bug. About two minutes after the rollback, eviction deletes the node's sandbox row. That also deletes the rollback receipt. The retry then rebuilds the row from the remote snapshot and activates generation 2 again.

What changed

All changes are in the sandbox manager: the server (crates/libs/sbx/mgr/server) and its SQLite store (crates/libs/sbx/mgr/store). Paths below are relative to those crates.

• Refuse a retry of a rolled-back attempt. begin_activation_with_route_evidence now returns RolledBackAttempt when the attempt matches the rollback receipt. The match is exact: same attempt, or same generation and attempt in the route evidence.
• Keep the fence after eviction. The delete receipt is now recorded before the row is deleted. It covers the larger of the route generation and the rolled-back generation. Activation of a generation covered by a delete receipt returns RetiredGeneration.
• Refuse before any restore work. Start now checks both guards before it prepares a restore or a cold start. It also checks them before it rebuilds an evicted row from the remote snapshot. The claim repeats the check inside its transaction, so a race is still caught.
• Return FAILED_PRECONDITION. One shared function, begin_activation_status, maps both errors to gRPC FAILED_PRECONDITION. Create and Start both use it. Other errors stay INTERNAL.
• Do not snapshot a rolled-back generation. Teardown of a suspended sandbox whose rollback covers the generation reuses the original artifact. This also covers a sandbox that ran before its suspend: the rollback keeps the earlier route generation and pod UID as a pair, and the teardown accepts that pair.
• Read the rollback receipt one way. Admission, the delete fence and the teardown use one helper, activation_rollback_receipt. A missing, malformed or other-kind receipt gives no authority to any of them. Admission and teardown also require the receipt to match the row's current state. The delete fence does not, because eviction moves the row to deleting before the delete.

Trade-off: after eviction, the node also refuses a new attempt for the retired generation. This matches existing behaviour before eviction. The route-activation fence already aborts a different attempt for a retained generation. The API starts a newer generation for each new wake, so this does not block recovery.

How it was validated

Unit tests

On e697abd (rebased on main at be16af3):

• cargo test -p pplx-space-sbx-mgr --lib: 1374 passed, 1 ignored.
• cargo test -p pplx-space-sbx-mgr-store: 366 passed. (The store's tests moved to this crate on main.)
• cargo clippy --tests -- -D warnings (with and without test-hooks) and cargo fmt --check pass for both crates.

New tests in service/tests.rs:

• teardown_after_late_activation_of_rolled_back_attempt_reuses_original_artifact (with and without a prior route generation and pod UID)
• a_malformed_rollback_receipt_carries_no_authority_anywhere (no refusal at admission, and the delete fence does not widen)
• a_rollback_receipt_refuses_only_activations_from_its_own_state
• new_activation_attempt_for_rolled_back_generation_is_admitted (the database guard only refuses the exact rolled-back attempt)
• eviction_after_rollback_keeps_the_rolled_back_generation_fenced
• inventory_stale_row_delete_after_rollback_keeps_the_rolled_back_generation_fenced (the inventory delete path also keeps the fence; a later late activation returns RetiredGeneration)
• start_rpc_refuses_late_restore_of_rolled_back_attempt (through start_sandbox, with and without a remote backup: FAILED_PRECONDITION; the row, rollback receipt and artifact pin are unchanged; no new artifact or job; the later teardown publishes no artifact and binds no driver)
• start_rpc_refuses_restore_of_generation_retired_by_eviction (no row is rebuilt, and no artifact or job is created)
• start_rpc_refuses_late_cold_start_of_rolled_back_attempt

Mutation check

We broke each guard on purpose, one at a time, and ran the tests again. A mutant is "killed" when at least one test fails.

Mutant	Before this commit	After
M1 attempt refusal disabled	Killed	Killed
M2 retired-generation refusal disabled	Killed	Killed
M3 teardown suspended-row branch disabled	Killed	Killed
M4 delete receipt written after the inventory row delete	Survived	Killed
M5 rolled-back generation dropped from the delete receipt	Killed	Killed
M6 both refusals mapped to INTERNAL	Survived	Killed
M7–M9 pre-check before restore work removed or ignored	Not applicable	Killed
M10 restore claim in create.rs maps to INTERNAL	Not applicable	Survived
M11 cold-start claim in start.rs maps to INTERNAL	Not applicable	Survived
M12 inventory delete records no receipt	Not applicable	Killed
M13 teardown suspended branch requires no route pod UID again	Not applicable	Killed
M14 admission ignores whether the receipt matches the row state	Not applicable	Killed
M15 delete fence requires the receipt to match the row state	Not applicable	Killed

M10 and M11 survive because the pre-check now refuses first. The claim refuses only if a receipt lands between the pre-check and the claim. Unit tests cannot reach the claim: it needs a node-manager, snapshot files under the fixed cache directory, or a template-manager. Both sites call the shared mapping, so M6 covers the mapping itself.

Live Kind runs

These runs used the PR before the review fixes and the rebase. The guard in #594 reruns this flow on Kind.

We ran SPACE on a local Kind cluster with real Firecracker microVMs. We used the public ECW/E2B SDK: create, write a file, pause (suspend), then connect (restore). A temporary local-only failpoint made one restore prepare fail with a disk-admission error. The failpoint is not part of this PR.

IDs below are synthetic labels. "New" means a snapshot that did not exist after the original suspend.

Before the fix (main)

Scenario	Late retry of generation 2	Snapshots	Teardown / upload jobs	Generation 3 restores from	File and exec after
Same attempt retries	Accepted; generation 2 started	2 (1 new)	2 / 2	New artifact	OK
Pod deleted, no retry	None	1	1 / 1	Original	OK
Normal suspend and restore	Not applicable	1	1 / 1	Original (generation 2)	OK

In the same-attempt case, the control plane still listed the original artifact. The node had moved to the new one.

After the fix (this PR)

Scenario	Node response to late retry	Snapshots	Teardown / upload jobs	Generation 3 restores from	File and exec after
Same attempt retries	8 × FAILED_PRECONDITION "attempt … was rolled back"; after eviction, 1 × "generation 2 was retired on this node"	1	1 / 1	Original	OK
Same attempt, second wake sent during the first	Same as above	1	1 / 1	Original	OK
New attempt for generation 2	8 × ABORTED from the existing route fence; after eviction, 1 × "retired on this node"	1	1 / 1	Original	OK
Pod deleted, no retry	None	1	1 / 1	Original	OK
Normal suspend and restore	Not applicable	1	1 / 1	Original (generation 2)	OK

"File and exec after" means the file written before the original suspend was still there, and a new command ran.

The first version of this PR fixed only the receipt check. The live run showed the eviction path: generation 2 was activated again, and a new snapshot was written. The second commit fixes that. The "after" table is from the second commit.

What the API does after FAILED_PRECONDITION

• The API does not start a new attempt right away. The first wake waits about 120 seconds and returns HTTP 502 "Sandbox is not running".
• A wake sent during that wait joins the same wait. It returns when the first one fails.
• The next wake deletes the generation 2 pod and starts generation 3 with a new attempt. That restore succeeds from the original artifact.
• In every run, the control plane kept the original artifact and snapshot ID.

We did not see a Superseded teardown outcome live. With this fix, generation 2 never starts, so its pod deletion does not call teardown. The unit test covers that outcome.

Open issues (not in this PR)

• The API waits about 120 seconds on a refused attempt. It could treat FAILED_PRECONDITION from the node as final and start a newer generation sooner.
• No end-to-end CI test covers this yet. The Kind smoke suite has no way to fail exactly one restore prepare. The smallest fix we propose: a test-hooks-only sandbox-manager hook that fails one prepare for a named sandbox, a matching smoke fault operation, and a scenario that checks the successor restores the original snapshot. This would be a separate PR.
• The race-only claim sites (M10, M11) need a fake node-manager and an injectable cache root to test directly.

Review guide

• store/src/db/state_transitions.rs: the two new guards in begin_activation_with_route_evidence.
• store/src/db.rs and store/src/db/sandboxes.rs: the delete receipt now records the rolled-back generation and is written before the row delete.
• store/src/db/teardown_suspend.rs: the suspended-row branch, including a retained prior route identity.
• store/src/db/state_transitions.rs: activation_rollback_receipt, the one reader of the rollback receipt.
• server/src/service/start.rs: the pre-check (refuse_retired_activation) and the shared error mapping (begin_activation_status).
• server/src/service/create.rs: uses the shared mapping.

AI disclosure

An AI agent (Perplexity Computer) wrote this description. It also wrote the code changes, the unit tests, the Kind simulations (including the local failpoint and test scripts), and the mutation checks. @nickdujay-pplx directed the work and reviews it.

[the-gigi-pplx]

Review — 3 findings

Medium

1. crates/libs/sbx/mgr/server/src/db/teardown_suspend.rs:487 — the new suspended arm of completed_activation_rollback_covers can't match a row from a sandbox that actually ran.
   The arm only fires through s.space_api_route_execution_generation IS NULL OR … < ?2. But apply_cleanup_state (db/cleanup_episodes.rs:789-813) keeps space_api_route_execution_generation and space_api_route_pod_uid under the same CASE condition, and the query still requires s.space_api_route_pod_uid IS NULL. So the case the comment at teardown_suspend.rs:444 describes ("may retain the predecessor's generation") never matches. A retained generation always comes with a retained pod UID. The fixture suspended_sandbox_after_restore_rollback starts from insert_test_sandbox, whose route pod UID is NULL, and that's why the test passes.
   I reproduced this by adding one UPDATE sandboxes SET space_api_route_execution_generation = 1, space_api_route_pod_uid = 'pod-gen-1' after insert_test_sandbox in the fixture, the shape a paused sandbox that ran has. The rollback keeps (Some(1), Some("pod-gen-1")), and teardown_after_late_activation_of_rolled_back_attempt_reuses_original_artifact fails at backed=true/late_activation=true with Pending instead of Superseded. The admission guard in state_transitions.rs still refuses the late retry, so the main fix holds. But the teardown arm (the "skip teardown snapshot" half of the title) is the same as main for real rows.
   Suggestion: drop space_api_route_pod_uid IS NULL for the suspended branch, or require it to equal the retained predecessor's pod. Then seed the fixture with a route generation and pod UID so the test covers the production shape.

Low

2. crates/libs/sbx/mgr/server/src/service/tests.rs:11409 — the doc comment written for suspended_sandbox_after_restore_rollback sits above job_snapshot. Also, in the teardown test, the if let Ok(Some(episode)) = late { … } "pre-fix behavior" block and the late_admitted assertion can't run, because the assert!(matches!(late, Err(RolledBackAttempt …))) just before them panics on any Ok. Delete them, or the test reads as if it exercises a reactivate-then-teardown path that it doesn't.
3. crates/libs/sbx/mgr/server/src/db.rs:394 — there are now three readers of the activation-rollback receipt with different rules. record_execution_delete_receipt matches the raw transition_receipt_kind string and doesn't decode. rolled_back_activation_attempt decodes, and treats a receipt that fails to decode as carrying no authority. completed_activation_rollback_covers decodes, with == for suspended and >= otherwise. So a receipt that fails to decode is ignored at admission but still widens the delete fence for the 7-day retention window. A shared activation_rollback_receipt(tx, id) -> Option<SandboxTransitionReceipt> helper would keep the three in step.

Verified against b19ab7355656dc2606ce973ae28914b33c264323. Reproduced locally: cargo test -p pplx-space-sbx-mgr --lib -- rolled_back with and without the one-line fixture change above (all 4 pass unmodified; the teardown test fails with it). Read but didn't run: the delete-receipt reorder in db/sandboxes.rs (the receipt is dropped with the transaction when the witness is lost, as the comment says) and the FAILED_PRECONDITION mapping in create.rs / start.rs. I didn't rerun the Kind scenarios.

[the-gigi-pplx]

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

[nickdujay-pplx]

Thanks, @the-gigi-pplx. All three are fixed in e697abd. I also rebased the PR on main at be16af3, so it now builds on the extracted store crate.

1. Medium: the suspended teardown arm never matched a sandbox that ran

You were right. I reproduced your failure on the rebased code: backed=true/late_activation=true returned Pending, not Superseded.

• Fix (store/src/db/teardown_suspend.rs): the suspended branch now accepts the retained pair, an older route generation plus its pod UID. apply_cleanup_state keeps the two together. A row with no route generation must still have no pod UID. The created/stopped branch keeps its old rule: no generation and no pod UID.
• Test: suspended_sandbox_after_restore_rollback takes a prior_route flag. When set, it seeds generation 1 and pod UID pod-gen-1, the production shape, and asserts that the rollback keeps that pair. The teardown test runs with and without it. Every other caller uses the production shape.
• Mutant: M13 puts space_api_route_pod_uid IS NULL back on the suspended branch. The new test kills it.

2. Low: misplaced comment and unreachable test code

• The fixture's doc comment now sits on suspended_sandbox_after_restore_rollback, and job_snapshot has its own comment.
• I deleted the if let Ok(Some(episode)) = late { … } block and the late_admitted assertion. The test now reads as what it is: the late retry is refused, then the teardown runs.

3. Low: three receipt readers with different rules

They now share one helper, activation_rollback_receipt (store/src/db/state_transitions.rs). It decodes the receipt against the state the receipt names. A missing, malformed or other-kind receipt carries no authority for any of the three readers. So a malformed receipt no longer widens the delete fence. The new test a_malformed_rollback_receipt_carries_no_authority_anywhere checks this for the delete fence and for admission.

One rule still differs, on purpose:

• Admission and the teardown arm also require the receipt to be current for the row's state (is_current_for).
• The delete fence does not. Eviction moves the row to deleting before the delete, so a receipt decoded against the row's current state would not decode, and the fence would be lost. My first version did exactly that, and eviction_after_rollback_keeps_the_rolled_back_generation_fenced caught it.

The generation rules are also per reader, as before. The teardown suspended arm uses ==, because the row may keep an older generation's identity. The other readers use >= or match the exact retired generation. a_rollback_receipt_refuses_only_activations_from_its_own_state covers the currency check.

Validation on e697abd

• cargo test -p pplx-space-sbx-mgr --lib: 1374 passed, 1 ignored. cargo test -p pplx-space-sbx-mgr-store: 366 passed.
• cargo clippy -D warnings, with and without test-hooks, and cargo fmt --check: clean.
• Mutation testing: 14 of 15 mutants killed. New ones: M13 (this Medium), M14 (admission ignores currency) and M15 (the delete fence requires currency). M10 and M11 still survive. They are the race-only claim call sites, which unit tests cannot reach.

Behaviour is unchanged otherwise. A confirmed rollback still takes no teardown snapshot or upload, and the fence still holds after eviction.

Since the Medium fix changes the teardown arm, could you re-review on the current head?

[the-gigi-pplx]

Re-reviewed at e697abd. All three findings from my previous review are fixed:

1. The suspended teardown arm now accepts the retained (older generation, pod UID) pair. The fixture seeds the production shape via prior_route, and M13 covers it.
2. The misplaced doc comment is moved and the unreachable test code is removed.
3. All three readers now go through activation_rollback_receipt. The delete fence intentionally skips is_current_for because the row is deleting at that point. That is correct, and eviction_after_rollback_keeps_the_rolled_back_generation_fenced covers it.

Also checked:

• record_execution_delete_receipt runs before the row delete on all three delete paths (inventory stale-row, cleanup-episode completion, row-intent cleanup). On WitnessChanged, the inventory path drops the tx without committing, so no stray receipt is left.
• Caller impact of the new FAILED_PRECONDITION: the Direct substrate treats it the same as INTERNAL (should_try_next_restore_candidate) and reserves a new route identity per candidate, so the post-eviction RetiredGeneration fence cannot block a fresh attempt. The shim passes the code through unchanged.

Non-blocking nit: the doc comment on retired_activation (store/src/db/state_transitions.rs) has two opening summaries. "Returns the refusal for an activation that repeats..." is left over above "Whether activation of this generation/attempt is retired on this node." One should go.

CI is green (20/20).