Skip to content

sbx-mgr: snapshot completion and rollback write only over the state they expect - #421

Merged
ac-pplx merged 1 commit into
ac/sbxmgr-sw-pr5c-backgroundfrom
ac/sbxmgr-sw-pr5a-snapshots
Oct 14, 2026
Merged

ac-pplx merged 1 commit into
ac/sbxmgr-sw-pr5c-backgroundfrom
ac/sbxmgr-sw-pr5a-snapshots

Conversation

@ac-pplx

@ac-pplx ac-pplx commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Stacked on #416 (on #413, #418, #438 and main). Base: ac/sbxmgr-sw-pr5c-background @ cf519ce. This PR is one commit on top of #416. Stack, bottom to top: #438 → #418 → #413 → #416 → #421 → #417 → #414. #372 and #388 have merged to main.

Every state named here is sbx-manager's node-local SQLite sandboxes.state, not the control-plane Postgres public.sandboxes.lifecycle.

This is plan §5 PR 5, the snapshot slice: findings 5 and 6, plus the scheduler's checkpoint admission. The other PRs in the stack convert the other update_state callers.

What merging does

Call sites converted

Call site Before After Edge
grpc_state_snapshot.rs create_template_base_snapshot admission require_state_write(update_state(Checkpointing)) begin_template_base_snapshot_state new TEMPLATE_SNAPSHOT_BEGIN (Request, {paused} → checkpointing)
same, completion require_state_write(update_state(Paused)) complete_template_base_snapshot_state CHECKPOINT_COMPLETE_PAUSED ({checkpointing} → paused)
same, failure path update_state_best_effort(Paused) rollback_template_base_snapshot_state CHECKPOINT_ROLLBACK_PAUSED ({checkpointing} → paused)
snapshot_scheduler.rs do_checkpoint admission require_state_write(update_state(Checkpointing)) begin_checkpoint_state CHECKPOINT_BEGIN ({started, paused} → checkpointing)
grpc_spaced_process_snapshots.rs full rollback restage require_state_write(update_state(Suspended)) restage_rolled_back_full_snapshot PROCESS_SNAPSHOT_ROLLBACK_RESTAGE ({stopped} → suspended)

Each new function binds its edge at one call site, so the tests drive the call site itself.

Of the six #438 items this PR now calls in production (require_state_write_from, StateWriteRejected, StateWriteGuard, state_write_result, StateWriteRejected::into_status and update_state_from_best_effort), #418, below it, already removed the expect(dead_code) from the first five; this PR removes update_state_from_best_effort's.

Locks and transactions. Caller mutex and lifecycle-lock scopes are unchanged; in autocommit update_state_from adds its own IMMEDIATE transaction around its reads and the UPDATE, where update_state ran bare statements, which is harmless on the single connection. Side columns are update_state's: failure_reason, the restore columns and non-matching receipts are cleared. The one exception is behavior change 5 below.

Races fixed

  • Finding 6: template base-snapshot completion over a Stop.
    • Before. The snapshot holds only the wake lock, and Stop and Start take only the cleanup fence. A Stop admitted during capture or hashing leaves stopped, or whatever a later operator Start or Pause moved the row to: starting, started or paused. The completion's update_state(Paused) overwrote any of those. So did the failure path's best-effort write, and that path is the likelier one, because Stop's kill makes the capture fail.
    • Now. Both refuse every row that is not checkpointing. The completion returns Aborted and stops; the failure path logs and returns its original error.
  • Finding 5: full process-snapshot rollback restage over a Stop's stopping.
    • Before. The rollback holds only the artifact lock while it rolls the disk back and copies the snapshot files. A Stop admitted in that window (stopped → stopping) was overwritten with suspended, and Stop's completion then no longer owned the row.
    • Now. The restage returns Aborted and stops before start_from_checkpoint. The row stays stopping, and Stop's completion settles it as stopped. sbx-mgr: add the persisted state-transition table and update_state_from (no callers) #388 notes that this Stop is reached only in Stop's runtime-gate-to-cleanup-fence window or with a leaked runtime.

Red/green tests. All run at the DB level on db::test_conn() with no service, socket or /space path. Both RPCs write under the production CACHE_DIR before their state write, so a service-level test would touch the host. The tests use the real writers:

  • Stop admission is update_state_superseding_activation_in_transaction(Stopping) plus a mark_stopped intent in one IMMEDIATE transaction.
  • Stop completion is complete_row_intent_cleanup_for_job(MarkStopped).

The race tests:

Test Sequence and assertion
grpc_state_snapshot::tests::template_base_snapshot_writes_leave_a_stop_that_completed_during_capture_stopped paused row → begin_template_base_snapshot_state → Stop admission and completion → stopped. The completion must return Aborted with the exact message, and the failure rollback must return; either way every table must be unchanged.
grpc_spaced_process_snapshots::tests::full_rollback_restage_leaves_a_stop_admitted_after_its_own_stop started row → the rollback's own stop → stopped → a second Stop admission → stopping. The restage must return Aborted with the exact message and leave every table unchanged, and the second Stop's completion must then settle stopped with no intent.

The sweeps run over all 18 SandboxState values, each with a wildcard-free oracle and accepting controls. Each one checks either the exact Aborted message and every table unchanged, or the accepted write.

Sweep Accepts Refuses
template_base_snapshot_writes_return_only_a_checkpointing_row_to_paused (completion and failure rollback) checkpointing → paused. The rollback must enqueue the row's sandbox_inventory dirty ref; the completion must not, as before every other state
template_base_snapshot_admission_writes_only_over_a_paused_row paused → checkpointing every other state
snapshot_scheduler::tests::checkpoint_admission_writes_only_over_a_started_or_paused_row started, paused → checkpointing every other state, with today's texts
full_rollback_restage_writes_only_over_a_stopped_row stopped → suspended; suspended with every table unchanged every other state

In the template-writes and restage sweeps, rows carry a failure reason, a receipt targeting started and backdated timestamps (#388's state_write_fixtures::decorate, widened to pub(crate)). Any write is therefore visible in every column it touches.

Red on #438. The new functions do not exist there, so I restored #388's call-site code inside them (M1–M3, M8, M9 below). Every new test fails that way and passes at this head.

Behavior changes (client-visible)

  1. CreateTemplateBaseSnapshot over a row that left checkpointing.
    • Before: OK, and the row was rewritten to paused.
    • Now: Aborted with "complete template checkpoint: sandbox state write was superseded: sandbox is ".
    • Reachable from: stopped (a Stop completed during capture or hashing), and starting, started or paused (that Stop followed by an operator Start or Pause). Delete and in-flight Stop rows were already refused by the generic guard.
    • As with that guard refusal before, the published template snapshot directory stays on disk.
  2. CreateTemplateBaseSnapshot failure path. The best-effort write no longer rewrites a non-checkpointing row as paused. It logs "sandbox state write was superseded" and does not mark the sandbox dirty. The RPC still returns its original error.
  3. CreateTemplateBaseSnapshot admission. The text for a from-state refusal is new. That refusal is unreachable: the caller checks paused under the same guard.
  4. Full Rollback with auto_restart: the restage writes only over stopped.
    • Before: update_state restaged every state the generic guards admitted. That included stopping (finding 5, or a rollback that started while a Stop was in flight, since the rollback stops only started or paused rows) and, on paper, created, failed, cleanup_required, cleanup_failed and transient rows.
    • Now: those return Aborted with "complete process snapshot suspension: sandbox state write was superseded: sandbox is ", and start_from_checkpoint does not run.
    • As with the guard refusals before, the disk rollback and the snapshot file copy have already happened. Node-manager refuses the disk rollback while a Firecracker runtime is live, so live transient rows never got this far.
  5. Full Rollback of a row that is already suspended.
    • It is still accepted and still restores.
    • No edge may list its own target as a source, so the restage skips the write instead of rewriting suspended in place.
    • It therefore no longer clears failure_reason or non-matching transition receipts, and no longer bumps updated_at, on that row. The sweep pins this; mutation M4 shows the in-place rewrite would fail it.
    • start_from_checkpoint's activation write (begin_activation_with_route_evidence(Restore)) clears all of those columns anyway, so the difference is visible only if the restore fails before that write.
  6. do_checkpoint (the scheduler and full CreateSnapshot). No behavior change: the from-set is exactly the started/paused the caller reads under the same guard. The new from-state text is unreachable.

Message changes. Every refusal uses #438's canonical text:

  • from-state: Aborted "{context}: sandbox state write was superseded: sandbox is {found}" (new);
  • quarantine: "{context}: sandbox state write was not applied: blocked_by_quarantine";
  • cleanup ownership: "…: superseded_by_cleanup_episode";
  • missing row: "…: missing";
  • database error: Internal "{context}: {error}".

The last four equal today's require_state_write texts on these call sites. The scheduler keeps its context-free texts ("sandbox state write was not applied: …", "db error: …"), and gains "sandbox state write was superseded: sandbox is {found}".

Tests from #372, #388 and #438 whose expectations changed.

  • sbx-mgr: one refusal-to-status mapping for update_state_from #438's best_effort_database_error_is_none_and_changes_nothing used TEMPLATE_SNAPSHOT_COMPLETE_AFTER_STOP, which this PR deletes. It now uses the insert edge ORPHAN_SUSPENDED_INSERT, which update_state_from refuses with the same InvalidQuery and which no PR in the stack deletes.
  • Nothing else changed.

Edges and allowlist

  • Added: TEMPLATE_SNAPSHOT_BEGIN: Request, {paused} → checkpointing. CHECKPOINT_BEGIN's {started, paused} is wider than the template caller expects. The pair is allowed and already requested, so the allowlist is unchanged.
  • Deleted (KnownRace): PROCESS_SNAPSHOT_ROLLBACK_AFTER_STOP (stopping → suspended) and TEMPLATE_SNAPSHOT_COMPLETE_AFTER_STOP (stopped → paused).
  • Docs refreshed: CHECKPOINT_BEGIN, CHECKPOINT_COMPLETE_PAUSED, CHECKPOINT_ROLLBACK_PAUSED and PROCESS_SNAPSHOT_ROLLBACK_RESTAGE now name the new functions.
  • Allowlist.
    • (Stopping, Suspended) is deleted: sbx-mgr: create completion and warm start write only over the state they expect #418, below this PR, already deleted WARM_START_UNVALIDATED, so no edge writes the pair now. Its reason was "known race: process-snapshot rollback in the gate-to-fence window".
    • (Stopped, Paused) is still written by the pause known race, so only its reason changes: "known races: pause and template-snapshot completion after Stop" → "known race: pause completion after Stop".
    • I re-read every other mixed reason. (Stopped, Suspended) still holds, because both of its edges remain.
  • KnownRace kind. Two KnownRace edges remain on this branch (pause ×2). sbx-mgr: pause writes only over the state it expects #417, above this PR, deletes them and retires EdgeKind::KnownRace and its any KnownRace assertion.
  • architecture-baseline.toml: grpc_spaced_process_snapshots.rs 1640 → 1636 and grpc_state_snapshot.rs 1835 → 1831. Both shrank. snapshot_scheduler.rs is not in the baseline: 987 code lines by archcheck's count, under the 1000 limit.

Decisions made without an operator

  • A new template begin edge rather than CHECKPOINT_BEGIN, so the from-set is exactly what the caller expects. The caller's same-guard paused check makes the edge unreachable through the RPC, so the sweep calls the function directly (mutation A4).
  • The restage accepts suspended. A full rollback of a suspended sandbox reaches the restage with no stop of its own, and update_state accepted it as a same-state rewrite. Refusing it would fail the RPC after the disk rollback and file copy had already happened. The pass-through matches only a from-state rejection with found == edge.to(); guard refusals of a suspended row still answer as before.
  • The restage's from-set stays {stopped}. No writer targets failed, and a created row has never run a VM, so it has no full process snapshot of its own. Both refusals are listed in behavior change 4.
  • The scheduler maps outcomes locally. Its errors are strings, and sbx-mgr: one refusal-to-status mapping for update_state_from #438's helpers are service-private. Changing their visibility would change sbx-mgr: one refusal-to-status mapping for update_state_from #438's API. The local mapping is one function with one caller, and it keeps today's texts.
  • Fixture placement. The Stop admission and completion fixture is duplicated inline in the two test modules, as in the other PRs in the stack; PR 6 consolidates it.

Overlaps with the other PRs in the stack

Mutation proof

Run one at a time on this head from a single backup, each restored and touched before the next; git diff was clean afterwards. The targeted set was the new tests plus state_write_table.

# Mutation Failing tests
M1 Template completion restored to #388's update_state(Paused) both template-writes tests
M2 Template failure rollback restored to update_state_best_effort(Paused) both template-writes tests
M3 Restage restored to require_state_write(update_state(Suspended)) both rollback tests
M4 Restage: put back the in-place rewrite of a suspended row rollback sweep
M5 Restage: drop the already-suspended pass-through rollback sweep
M6 Restage: every from-state rejection answered Ok both rollback tests
M7 #438's update_state_from_best_effort: no dirty mark when applied template-writes sweep
M8 do_checkpoint admission restored to update_state(Checkpointing) checkpoint admission sweep
M9 Template admission restored to update_state(Checkpointing) template admission sweep
A1 CHECKPOINT_COMPLETE_PAUSED from-set → StateSet::ALL both template-writes tests (+ the 2 table tests)
A2 CHECKPOINT_ROLLBACK_PAUSED → ALL both template-writes tests (+ table)
A3 PROCESS_SNAPSHOT_ROLLBACK_RESTAGE → ALL both rollback tests (+ table)
A4 TEMPLATE_SNAPSHOT_BEGIN → ALL template admission sweep (+ table)
A5 CHECKPOINT_BEGIN → ALL checkpoint admission sweep (+ table)

Validation of the restacked stack

The stack was restacked linearly on main (b18051f) after #372 and #388 merged: #438 → #418 → #413 → #416 → #421 → #417 → #414. The per-PR validation further down ran on this PR's pre-restack head.

This PR's head (9f30aa4): pull CI is green. fmt, workspace clippy --all-targets --all-features -D warnings and cargo arch (0 required findings) all pass. SPACE_CI_REQUIRE_TEST_DEPS=1 cargo nextest run --locked -p pplx-space-sbx-mgr --retries 0 passed 1647/1647, with 0 dependency skips. Tested at e752f07; the final head differs from it only in architecture-baseline.toml, which only cargo arch reads, and cargo arch was rerun on the final head. Kind Pull CI ran on #417, which contains this PR: all 4 Firecracker smoke shards passed.

The top of the stack (#414 @ 9a0a251, which contains this PR):

Check Result
sbx-mgr nextest ×3 1650/1650 each, 0 dependency skips
Whole-workspace nextest 8985/9021 passed, and every API crate passed. The 36 failures are all outside sbx-mgr. 34 fail identically on main: spaced-guest and spaced-host-cli process tests (the test box can't switch identity) and one net-gateway test. The other 2 are common-grpc retry-deadline tests that pass on rerun on both heads.
The stack's 30 new or changed tests, ×30 900/900
cargo test -p pplx-space-sbx-mgr (shared process) 1650/1650
Mutation spot-check on the top head #417's quarantine-miss mutation and #418's warm-start admission mutation are both caught
Kind, fresh cluster; the deployed sbx-mgr binary is from this head smoke-kind-e2e.sh passed. smoke-kind-typed.sh all: native 23/23; ECW 76 passed, 2 infra errors. local.ecw_template_build_from_oci_boots passed, and so did the leader-election brownout.
The 2 ECW infra errors node_group_boundary_schedules and node_selector_extras_render_and_enforce. Both scenarios restart the API Deployment, and the same failure family appears on main in an A/B on the same cluster. In the failed runs the kill never reached sbx-mgr.
Lifecycle race soak on Kind 11 concurrent operation pairs × 40 iterations, on the stack and on main on the same fresh cluster, checking that sbx-mgr's SQLite sandboxes.state, control-plane Postgres lifecycle and the VM agree. No change-induced violation: across 880 sandboxes, no quarantine, panic, restart or stuck state. Every violation reproduces on main with the same signature: an ECW pause that loses to Stop answers 503 instead of 409; after Suspend vs Delete, the node row can outlive the control-plane row until the next eviction pass (pass timing, not the build); and a refused quarantine write after Pause vs Delete is still logged. The soak's 0–200 ms API-level offsets did not reproduce the interleavings this stack closes, so the unit-level race tests remain the direct evidence for those.

Validation on the pre-restack head

Head c45e93c (round 2 is wording only; nextest re-run once: 1636/1636, 0 dependency skips); the shared /home/dev devbox disk is the limiting resource throughout.

Check Result
cargo fmt --all -- --check, scripts/check-rust-error-policy.sh pass
cargo clippy --workspace --locked --all-targets --all-features -- -D warnings pass, 0 warnings
cargo arch pass, 0 required findings; both converted service files shrank and their caps were lowered; no file over its cap grew
pre-commit (Gitleaks) on f1d1c9b..HEAD pass
Public-safety review of the diff, commit message and this body no findings in the reviewed scope
SPACE_CI_REQUIRE_TEST_DEPS=1 cargo nextest run --locked -p pplx-space-sbx-mgr --retries 0 ×3 1636/1636 passed each run; 0 dependency skips (empty SPACE_TEST_DEP_SKIP_LOG ×3); 1 skipped = the #[ignore] root-only transfer_directory::tests::distinct_service_uids_can_exchange_downloads
Mutation proof (14 above) all 14 caught by the new tests; all restored
Kind on this head Blocked, ENVIRONMENTAL. A fresh bring-up (no cluster before) failed in Tilt with "Error loading image to KIND" (no space left on device, 17 hits) as the shared /home/dev (393G) fell from 3.3G to 45M free. The rerun, after re-triggering the failed resources, failed the same way at 44M free. Other agents' worktrees hold about 330G of that disk; the only authorized deletion, this worktree's target/, is already gone. Pointing SPACE_KIND_DATA_ROOT at the root overlay (52G free) fails earlier: the control plane does not start with containerd storage on overlayfs. Torn down with infra/space/kind/scripts/teardown.sh. So the deployed-binary proof, Tilt quiet, e2e and typed did not run on this head
local.ecw_template_build_from_oci_boots (the only scenario reaching CreateTemplateBaseSnapshot) Blocked, ENVIRONMENTAL. Its OCI storage admission needs 55834574848 bytes free on the builder's /var/lib/containerd (~/.kind-data/builder/containerd on /home/dev). The previous head's run found 3542638592, then 3530162176 on rerun. At this round /home/dev had 3.3G free before bring-up and 44M after; Docker's disk had 22G free after the two authorized prunes. Neither can reach about 56G without deleting data this task does not own
Kind on the previous head af8956c (same call-site logic before the #438 rebase; this round adds do_checkpoint, the shared helpers and tests) fresh cluster; deployed space-sbxmgr-svc sha256 equal to that build's artifact, and logs version: af8956c5efaa; Tilt quiet (46 ok); smoke-kind-e2e.sh pass; smoke-kind-typed.sh all: ECW 77 passed, 0 failed, 1 infra error (above), native 23/23; torn down

Kind coverage limit. No smoke scenario drives the process-snapshot Rollback RPC. The one that reaches template base snapshots is blocked above. The converted paths are proven by the DB-level red/green tests.

🤖 Generated with Claude Code

@ac-pplx
ac-pplx force-pushed the ac/sbxmgr-sw-pr5a-snapshots branch from af8956c to 3eaadbd Compare October 14, 2026 00:50
@ac-pplx
ac-pplx changed the base branch from ac/sbxmgr-state-write-pr1-helper to ac/sbxmgr-sw-pr1b-refusal-status October 14, 2026 00:50
@ac-pplx
ac-pplx force-pushed the ac/sbxmgr-sw-pr5a-snapshots branch 2 times, most recently from 22d88d6 to c45e93c Compare October 14, 2026 02:08
…hey expect

All states here are sbx-mgr's node-local SQLite `sandboxes.state`, not the
control-plane Postgres `public.sandboxes.lifecycle`.

Convert the snapshot writers from `update_state` to `update_state_from`:

- `create_template_base_snapshot` admission: a new TEMPLATE_SNAPSHOT_BEGIN
  edge ({paused} -> checkpointing), checked and written under one guard as
  before.
- Its completion: CHECKPOINT_COMPLETE_PAUSED ({checkpointing} -> paused).
- Its failure rollback, best effort: CHECKPOINT_ROLLBACK_PAUSED
  ({checkpointing} -> paused).
- `do_checkpoint` admission: CHECKPOINT_BEGIN ({started, paused} ->
  checkpointing), the states it reads under the same guard. No behavior
  change.
- The full process-snapshot rollback's restage:
  PROCESS_SNAPSHOT_ROLLBACK_RESTAGE ({stopped} -> suspended). A row that is
  already `suspended` is accepted without a write.

The template snapshot holds only the wake lock, and the rollback only the
artifact lock; Stop takes neither. A Stop that completed while the template
snapshot was captured or hashed left `stopped` (or whatever a later Start or
Pause moved it to), and both the completion and the failure rollback
rewrote it to `paused`. A Stop admitted while the rollback restaged its
snapshot left `stopping`, and the restage overwrote it with `suspended`, so
the Stop's completion no longer settled the row. Both writes now refuse a
row outside their from-set: the completion and the restage return `Aborted`
("...: sandbox state write was superseded: sandbox is <state>") and stop,
and the best-effort rollback logs and leaves the row. Generic guard refusals
keep their existing messages.

The service call sites use the shared `require_state_write_from`,
`state_write_result` and `update_state_from_best_effort`; the last loses its
`dead_code` expectation. The scheduler's errors are strings, so its
admission maps the outcome to today's text locally. Caller mutex and
lifecycle-lock scopes are unchanged; in autocommit `update_state_from` adds
its own IMMEDIATE transaction around its reads and the UPDATE, which is
harmless on the single connection.

Delete the PROCESS_SNAPSHOT_ROLLBACK_AFTER_STOP and
TEMPLATE_SNAPSHOT_COMPLETE_AFTER_STOP known-race edges, drop the
`stopping -> suspended` allowlist entry, which no edge writes now, refresh
the `stopped -> paused` reason, and point the shared best-effort
database-error test at an insert edge. The restage lives in a child module
because its parent is at its size cap; both converted service files shrank,
and their caps are lowered.

Each call site binds its edge in a named function, which the new tests drive
with real Stop admission and completion writers and over every lifecycle
state. The template-writes and restage sweeps seed rows with a failure
reason, a receipt and backdated timestamps; the two admission sweeps use
plain rows.

@nikhilgupta-pplx nikhilgupta-pplx left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve. Template completion/rollback {checkpointing} → paused can't overwrite another owner (only wake-lock holders enter checkpointing); scheduler and template admission match the state read under the same guard; rollback restage {stopped} + suspended no-write keeps retries working, and a Start that slips in after the disk rollback is now refused instead of overwritten. update_state_from_best_effort gains its first caller and drops its expectation as planned.

Non-blocking:

  • Rollback only stops started/paused rows (grpc_spaced_process_snapshots.rs:1522). A row already stopping now proceeds through the disk rollback and snapshot copy and only then gets Aborted at the restage. Refusing states outside {started, paused, stopped, suspended} with FailedPrecondition at entry would avoid the destructive steps on a request that is certain to fail.
  • snapshot_scheduler.rs rebuilds the legacy SandboxStateWriteOutcome just for its Display text, a second copy of the refusal-text mapping next to into_status. Fine to fold into the deferred enum consolidation.

@ac-pplx
ac-pplx added this pull request to the merge queue Oct 14, 2026
@the-gigi-pplx

Copy link
Copy Markdown
Collaborator

Review — 4 findings

Medium

  1. crates/libs/sbx/mgr/server/src/service/grpc_spaced_process_snapshots/rollback_state.rs:20 — the restage catches a Stop admitted during the copy only while that Stop is still stopping. Once the Stop completes, the restage overwrites it.
    The from-set is {stopped}, and a second Stop that's admitted and then completes during the reflink copy also leaves stopped. The restage can't tell that row from its own stop's, so it writes suspended, and start_from_checkpoint restarts a sandbox the user just stopped. That's finding 5's outcome, reached through the half of the window the new test doesn't drive. full_rollback_restage_leaves_a_stop_admitted_after_its_own_stop checks the restage before complete_stop. I ran a copy of that test with a second complete_stop before the restage call: the restage applies and the row ends suspended. The copy can fall back to a full memory-image copy, so this half of the window isn't the short one. What would close it is an ownership check rather than a state check: compare the cleanup episode, or the state_changed_at the rollback observed after its own stop, at the restage. If that's out of scope for this stack, narrow the description's "Now" for finding 5 to a Stop that hasn't completed yet.

Low

  1. crates/libs/sbx/mgr/server/src/service/grpc_spaced_process_snapshots/rollback_state.rs:11 — the doc comment says a refused row "was moved after the rollback checked it", but the rollback never checks the entry state.
    rollback stops a row only when it entered as started or paused (grpc_spaced_process_snapshots.rs:1522). Every other entry state goes straight to the disk rollback and the restage. So a rollback that began while a Stop was in flight, which behavior change 4 in the description lists, reaches the restage on a stopping row that no one moved after any check. The Aborted text then reads "superseded" for a race the rollback walked into, rather than lost. Either say "any other row is outside the restage's from-set: a Stop admitted after the rollback's own stop, or a row the rollback found in a state it doesn't stop" or, if the intent is to refuse those rows, check the entry state up front with FailedPrecondition before rollback_btrfs_snapshot, so the disk isn't rolled back for a restage that will refuse. The description already accepts the late refusal for these rows, so I'd take the comment fix alone.
  2. crates/libs/sbx/mgr/server/src/service/grpc_state_snapshot.rs:1900 — after the completion refuses, the template manager and the disk disagree, which is more than "the published directory stays on disk".
    publish_template_base_snapshot_dir has already moved the previous directory aside and deleted it (template_snapshot_support.rs:104-136) by the time complete_template_base_snapshot_state runs. The template manager records the new hash with upsert_snapshot and invalidates the pool sandboxes only on OK (template/mgr/src/service/snapshots.rs:222 and :230). After this Aborted, the on-disk snapshot is the new capture while the template manager's row keeps the previous hash and the pool keeps its existing members, until the next successful build. The same was true of the guard refusals before, so this isn't a regression. But the Stop race makes it newly reachable, and behavior change 1 would be more accurate if it said so. Writing the completion state before publish_template_base_snapshot_dir would close it. A publish failure after that write would then reach the failure path with a paused row, which its best-effort rollback already refuses harmlessly.
  3. crates/libs/sbx/mgr/server/src/snapshot_scheduler.rs:47 — begin_checkpoint_state copies the mapping that state_write_result and StateWriteRejected::into_status already own.
    The reason given is that this module returns strings. into_status(context).message(), or a Display on StateWriteRejected, would give it the same text from one place, with a visibility widening. As it stands, a new refusal variant or a wording change has to be made in both cleanup_support.rs and here.

What I checked and found sound:

  • The template admission, completion and rollback from-sets match their callers. Admission is checked under the same guard as the paused read (grpc_state_snapshot.rs:1773-1788).
  • do_checkpoint's from-state refusal is unreachable, because the caller checks started/paused under the same guard (snapshot_scheduler.rs:853-862).
  • Dropping update_state_from_best_effort's dead_code expectation is right: rollback_template_base_snapshot_state is a non-test caller.
  • A best-effort rollback refusal can't strand a row in checkpointing. It refuses only rows that are already out of checkpointing; a database error leaves it there, exactly as update_state_best_effort did.
  • The restage's found == edge.to() pass-through matches only a from-state rejection, so a quarantined suspended row still refuses.
  • The deleted KnownRace edges and the (Stopping, Suspended) allowlist row have no remaining writer.
  • The scheduler's local mapping reproduces SandboxStateWriteNotApplied's text for the guard and missing cases.
  • The PR body, the commit message and the added lines contain nothing private.

Verified against 9f30aa4f5591. Reproduced locally: cargo clippy -p pplx-space-sbx-mgr --all-targets --all-features -D warnings (clean) and cargo nextest run -p pplx-space-sbx-mgr -p pplx-space-db-test-fixtures --all-features (1658 passed, 0 failed) at this head; for finding 1, a copy of full_rollback_restage_leaves_a_stop_admitted_after_its_own_stop with a second complete_stop before the restage passes with the restage applied and the row suspended.

@the-gigi-pplx the-gigi-pplx left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Merged via the queue into main with commit 1250648 Oct 14, 2026
20 checks passed
@ac-pplx
ac-pplx deleted the ac/sbxmgr-sw-pr5a-snapshots branch October 14, 2026 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants