Repository navigation
sbx-mgr: snapshot completion and rollback write only over the state they expect - #421
Conversation
af8956c to
3eaadbd
Compare
22d88d6 to
c45e93c
Compare
…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.
c45e93c to
9f30aa4
Compare
nikhilgupta-pplx
left a comment
There was a problem hiding this comment.
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/pausedrows (grpc_spaced_process_snapshots.rs:1522). A row alreadystoppingnow proceeds through the disk rollback and snapshot copy and only then getsAbortedat the restage. Refusing states outside{started, paused, stopped, suspended}withFailedPreconditionat entry would avoid the destructive steps on a request that is certain to fail. snapshot_scheduler.rsrebuilds the legacySandboxStateWriteOutcomejust for itsDisplaytext, a second copy of the refusal-text mapping next tointo_status. Fine to fold into the deferred enum consolidation.
Review — 4 findingsMedium
Low
What I checked and found sound:
Verified against |
the-gigi-pplx
left a comment
There was a problem hiding this comment.
Approved. 4 non-blocking findings in the review comment above.
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 Postgrespublic.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_statecallers.What merging does
Call sites converted
grpc_state_snapshot.rscreate_template_base_snapshotadmissionrequire_state_write(update_state(Checkpointing))begin_template_base_snapshot_stateTEMPLATE_SNAPSHOT_BEGIN(Request,{paused} → checkpointing)require_state_write(update_state(Paused))complete_template_base_snapshot_stateCHECKPOINT_COMPLETE_PAUSED({checkpointing} → paused)update_state_best_effort(Paused)rollback_template_base_snapshot_stateCHECKPOINT_ROLLBACK_PAUSED({checkpointing} → paused)snapshot_scheduler.rsdo_checkpointadmissionrequire_state_write(update_state(Checkpointing))begin_checkpoint_stateCHECKPOINT_BEGIN({started, paused} → checkpointing)grpc_spaced_process_snapshots.rsfull rollback restagerequire_state_write(update_state(Suspended))restage_rolled_back_full_snapshotPROCESS_SNAPSHOT_ROLLBACK_RESTAGE({stopped} → suspended)Each new function binds its edge at one call site, so the tests drive the call site itself.
service/template_snapshot_support.rs) use sbx-mgr: one refusal-to-status mapping for update_state_from #438'srequire_state_write_fromandupdate_state_from_best_effort.service/grpc_spaced_process_snapshots/rollback_state.rs, a child module because its parent is at itscargo archcap) uses sbx-mgr: one refusal-to-status mapping for update_state_from #438's typedstate_write_result. It answersStateWriteRejected::FromState { found: suspended }as already staged and passes every other rejection tointo_status.snapshot_scheduler.rsreturnsStringerrors, and sbx-mgr: one refusal-to-status mapping for update_state_from #438's helpers arepub(super)inservice, sobegin_checkpoint_statemaps the outcome locally. It keeps today's texts and replaces the scheduler's oldrequire_state_write, whose only caller this was.Of the six #438 items this PR now calls in production (
require_state_write_from,StateWriteRejected,StateWriteGuard,state_write_result,StateWriteRejected::into_statusandupdate_state_from_best_effort), #418, below it, already removed theexpect(dead_code)from the first five; this PR removesupdate_state_from_best_effort's.Locks and transactions. Caller mutex and lifecycle-lock scopes are unchanged; in autocommit
update_state_fromadds its ownIMMEDIATEtransaction around its reads and theUPDATE, whereupdate_stateran bare statements, which is harmless on the single connection. Side columns areupdate_state's:failure_reason, the restore columns and non-matching receipts are cleared. The one exception is behavior change 5 below.Races fixed
stopped, or whatever a later operator Start or Pause moved the row to:starting,startedorpaused. The completion'supdate_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.checkpointing. The completion returnsAbortedand stops; the failure path logs and returns its original error.stopping.stopped → stopping) was overwritten withsuspended, and Stop's completion then no longer owned the row.Abortedand stops beforestart_from_checkpoint. The row staysstopping, and Stop's completion settles it asstopped. 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/spacepath. Both RPCs write under the productionCACHE_DIRbefore their state write, so a service-level test would touch the host. The tests use the real writers:update_state_superseding_activation_in_transaction(Stopping)plus amark_stoppedintent in oneIMMEDIATEtransaction.complete_row_intent_cleanup_for_job(MarkStopped).The race tests:
grpc_state_snapshot::tests::template_base_snapshot_writes_leave_a_stop_that_completed_during_capture_stoppedpausedrow →begin_template_base_snapshot_state→ Stop admission and completion →stopped. The completion must returnAbortedwith 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_stopstartedrow → the rollback's own stop →stopped→ a second Stop admission →stopping. The restage must returnAbortedwith the exact message and leave every table unchanged, and the second Stop's completion must then settlestoppedwith no intent.The sweeps run over all 18
SandboxStatevalues, each with a wildcard-free oracle and accepting controls. Each one checks either the exactAbortedmessage and every table unchanged, or the accepted write.template_base_snapshot_writes_return_only_a_checkpointing_row_to_paused(completion and failure rollback)checkpointing→paused. The rollback must enqueue the row'ssandbox_inventorydirty ref; the completion must not, as beforetemplate_base_snapshot_admission_writes_only_over_a_paused_rowpaused→checkpointingsnapshot_scheduler::tests::checkpoint_admission_writes_only_over_a_started_or_paused_rowstarted,paused→checkpointingfull_rollback_restage_writes_only_over_a_stopped_rowstopped→suspended;suspendedwith every table unchangedIn the template-writes and restage sweeps, rows carry a failure reason, a receipt targeting
startedand backdated timestamps (#388'sstate_write_fixtures::decorate, widened topub(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)
CreateTemplateBaseSnapshotover a row that leftcheckpointing.OK, and the row was rewritten topaused.Abortedwith "complete template checkpoint: sandbox state write was superseded: sandbox is ".stopped(a Stop completed during capture or hashing), andstarting,startedorpaused(that Stop followed by an operator Start or Pause). Delete and in-flight Stop rows were already refused by the generic guard.CreateTemplateBaseSnapshotfailure path. The best-effort write no longer rewrites a non-checkpointingrow aspaused. It logs "sandbox state write was superseded" and does not mark the sandbox dirty. The RPC still returns its original error.CreateTemplateBaseSnapshotadmission. The text for a from-state refusal is new. That refusal is unreachable: the caller checkspausedunder the same guard.Rollbackwithauto_restart: the restage writes only overstopped.update_staterestaged every state the generic guards admitted. That includedstopping(finding 5, or a rollback that started while a Stop was in flight, since the rollback stops onlystartedorpausedrows) and, on paper,created,failed,cleanup_required,cleanup_failedand transient rows.Abortedwith "complete process snapshot suspension: sandbox state write was superseded: sandbox is ", andstart_from_checkpointdoes not run.Rollbackof a row that is alreadysuspended.suspendedin place.failure_reasonor non-matching transition receipts, and no longer bumpsupdated_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.do_checkpoint(the scheduler and fullCreateSnapshot). No behavior change: the from-set is exactly thestarted/pausedthe caller reads under the same guard. The new from-state text is unreachable.Message changes. Every refusal uses #438's canonical text:
Aborted"{context}: sandbox state write was superseded: sandbox is {found}" (new);Internal"{context}: {error}".The last four equal today's
require_state_writetexts 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.
best_effort_database_error_is_none_and_changes_nothingusedTEMPLATE_SNAPSHOT_COMPLETE_AFTER_STOP, which this PR deletes. It now uses the insert edgeORPHAN_SUSPENDED_INSERT, whichupdate_state_fromrefuses with the sameInvalidQueryand which no PR in the stack deletes.Edges and allowlist
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.PROCESS_SNAPSHOT_ROLLBACK_AFTER_STOP(stopping → suspended) andTEMPLATE_SNAPSHOT_COMPLETE_AFTER_STOP(stopped → paused).CHECKPOINT_BEGIN,CHECKPOINT_COMPLETE_PAUSED,CHECKPOINT_ROLLBACK_PAUSEDandPROCESS_SNAPSHOT_ROLLBACK_RESTAGEnow name the new functions.(Stopping, Suspended)is deleted: sbx-mgr: create completion and warm start write only over the state they expect #418, below this PR, already deletedWARM_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".(Stopped, Suspended)still holds, because both of its edges remain.KnownRaceedges remain on this branch (pause ×2). sbx-mgr: pause writes only over the state it expects #417, above this PR, deletes them and retiresEdgeKind::KnownRaceand itsany KnownRaceassertion.architecture-baseline.toml:grpc_spaced_process_snapshots.rs1640 → 1636 andgrpc_state_snapshot.rs1835 → 1831. Both shrank.snapshot_scheduler.rsis not in the baseline: 987 code lines by archcheck's count, under the 1000 limit.Decisions made without an operator
CHECKPOINT_BEGIN, so the from-set is exactly what the caller expects. The caller's same-guardpausedcheck makes the edge unreachable through the RPC, so the sweep calls the function directly (mutation A4).suspended. A full rollback of asuspendedsandbox reaches the restage with no stop of its own, andupdate_stateaccepted 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 withfound == edge.to(); guard refusals of asuspendedrow still answer as before.{stopped}. No writer targetsfailed, and acreatedrow has never run a VM, so it has no full process snapshot of its own. Both refusals are listed in behavior change 4.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.Overlaps with the other PRs in the stack
db/state_write.rs. I deleted only my two known-race entries, added one entry afterCHECKPOINT_BEGIN, and edited four of my edges' docs. No other PR in the stack convertsdo_checkpoint. sbx-mgr: create completion and warm start write only over the state they expect #418 and sbx-mgr: wake and resident resume write only over the state they expect #413, below this PR, deletedWARM_START_UNVALIDATEDandRESUME_BEGIN_AFTER_STOPfrom the same// Known races.block, and sbx-mgr: pause writes only over the state it expects #417, above it, deletesPAUSE_*_AFTER_STOPand the emptied block.WARM_START_UNVALIDATED, so this PR deletes(Stopping, Suspended). When sbx-mgr: pause writes only over the state it expects #417, above it, deletesPAUSE_COMPLETE_AFTER_STOP,(Stopped, Paused)loses its last writer and sbx-mgr: pause writes only over the state it expects #417 deletes the entry.service/cleanup_support.rs. sbx-mgr: create completion and warm start write only over the state they expect #418, below this PR, removed theexpect(dead_code)from the five helpers it adopts; this PR removes onlyupdate_state_from_best_effort's.service/cleanup_support/tests.rs. The insert-edge swap above.architecture-baseline.toml. My two cap lines are adjacent tosuspend_snapshot.rsandservice/mod.rs.db/tests/state_write_fixtures.rs.decoratewidened topub(crate).Mutation proof
Run one at a time on this head from a single backup, each restored and touched before the next;
git diffwas clean afterwards. The targeted set was the new tests plusstate_write_table.update_state(Paused)update_state_best_effort(Paused)require_state_write(update_state(Suspended))suspendedrowsuspendedpass-throughOkupdate_state_from_best_effort: no dirty mark when applieddo_checkpointadmission restored toupdate_state(Checkpointing)update_state(Checkpointing)CHECKPOINT_COMPLETE_PAUSEDfrom-set →StateSet::ALLCHECKPOINT_ROLLBACK_PAUSED→ALLPROCESS_SNAPSHOT_ROLLBACK_RESTAGE→ALLTEMPLATE_SNAPSHOT_BEGIN→ALLCHECKPOINT_BEGIN→ALLValidation 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 warningsandcargo arch(0 required findings) all pass.SPACE_CI_REQUIRE_TEST_DEPS=1 cargo nextest run --locked -p pplx-space-sbx-mgr --retries 0passed 1647/1647, with 0 dependency skips. Tested ate752f07; the final head differs from it only inarchitecture-baseline.toml, which onlycargo archreads, andcargo archwas 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):cargo test -p pplx-space-sbx-mgr(shared process)smoke-kind-e2e.shpassed.smoke-kind-typed.sh all: native 23/23; ECW 76 passed, 2 infra errors.local.ecw_template_build_from_oci_bootspassed, and so did the leader-election brownout.node_group_boundary_schedulesandnode_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.sandboxes.state, control-plane Postgreslifecycleand 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/devdevbox disk is the limiting resource throughout.cargo fmt --all -- --check,scripts/check-rust-error-policy.shcargo clippy --workspace --locked --all-targets --all-features -- -D warningscargo archf1d1c9b..HEADSPACE_CI_REQUIRE_TEST_DEPS=1 cargo nextest run --locked -p pplx-space-sbx-mgr --retries 0×3SPACE_TEST_DEP_SKIP_LOG×3); 1 skipped = the#[ignore]root-onlytransfer_directory::tests::distinct_service_uids_can_exchange_downloadsno 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'starget/, is already gone. PointingSPACE_KIND_DATA_ROOTat the root overlay (52G free) fails earlier: the control plane does not start with containerd storage on overlayfs. Torn down withinfra/space/kind/scripts/teardown.sh. So the deployed-binary proof, Tilt quiet, e2e and typed did not run on this headlocal.ecw_template_build_from_oci_boots(the only scenario reachingCreateTemplateBaseSnapshot)/var/lib/containerd(~/.kind-data/builder/containerdon/home/dev). The previous head's run found 3542638592, then 3530162176 on rerun. At this round/home/devhad 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 ownaf8956c(same call-site logic before the #438 rebase; this round addsdo_checkpoint, the shared helpers and tests)space-sbxmgr-svcsha256 equal to that build's artifact, and logsversion: af8956c5efaa; Tilt quiet (46 ok);smoke-kind-e2e.shpass;smoke-kind-typed.sh all: ECW 77 passed, 0 failed, 1 infra error (above), native 23/23; torn downKind coverage limit. No smoke scenario drives the process-snapshot
RollbackRPC. 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