Repository navigation
sbx-mgr: suspend writes only over the state it expects - #414
Conversation
0599037 to
4d190bc
Compare
Suspend's auto-pause writes sbx-mgr's node-local SQLite `sandboxes.state`
three times while it holds the cleanup fence and the wake and artifact
locks: `pausing` after it reads the row as `started`, `started` to roll
back a failed guest preparation, and `paused` once node-manager confirms
the pause. Each was a generic `update_state`, which overwrites any state
its cleanup and quarantine guards admit. This is not the control-plane
Postgres `public.sandboxes.lifecycle`.
Those locks exclude every writer of these states except warm-snapshot
Start, which takes no lock. Before its conversion (`WARM_START_RESTAGE`,
below this commit), a `from_warm` Start on a running row (in practice an
operator CLI Start) could land between suspend's read and any of the three
writes, which then wrote `pausing`, `started` or `paused` over its
`suspended` row. The converted warm start refuses a row outside `created`
and `stopped`, which closes that interleaving.
The three writes now go through `update_state_from` with the existing
edges `PAUSE_BEGIN` ({started} -> pausing), `PAUSE_ROLLBACK`
({pausing} -> started) and `PAUSE_COMPLETE` ({pausing} -> paused), and
their outcomes through the shared `require_state_write_from`. A row
outside the from-set is refused with every row unchanged. This is a
defensive refusal: no known writer reaches these writes. Were one to, the
refused row would keep that writer's state over a running or paused VM, an
end state no better than an overwrite.
Begin and completion answer a refusal `Aborted`: "<context>: sandbox
state write was superseded: sandbox is <state>", or `update_state`'s
guard message when a generic guard answered first. The rollback still
logs the refusal and answers the guest-preparation `Internal`. Side
columns, the caller's mutex and the lifecycle-lock scopes are unchanged;
`update_state_from` adds its own IMMEDIATE transaction around its reads
and the UPDATE in autocommit, which is harmless on the single connection.
Suspend's guest preparation now goes through pause's
`prepare_guest_for_pause`, so the test-only guest-preparation seam covers
the auto-pause too. `PAUSE_BEGIN`'s doc comment says suspend reads
`started` rather than calling `transition`. No edge is added or deleted
and the parity allowlist is unchanged. The auto-pause was the last
production caller of the legacy `require_state_write`, which only the
`update_state` parity tests still call, so it now expects `dead_code`
outside tests.
The new test drives the Suspend RPC and sweeps every `SandboxState` at
each write: between the read and the begin (holding node-manager connect),
and during guest preparation before the rollback and the completion. The
accepting controls are `started` at the begin and `pausing` after it. A
trigger on the `suspending` write, installed in every case, stops the
completion control at the next write, before snapshot work, and fails any
other case that gets that far. On the previous writes every non-guarded
state was overwritten at each of the three.
4d190bc to
9a0a251
Compare
nikhilgupta-pplx
left a comment
nikhilgupta-pplx
left a comment
There was a problem hiding this comment.
Approve. Suspend holds the cleanup fence (try-acquired, so Stop/Delete win) plus wake and artifact locks, so no known writer reaches these writes; edges match the gating, and the auto-pause fails closed on anything but a confirmed paused.
Non-blocking:
- The auto-pause refusal (
suspend.rs~441) and the guest-prep rollback refusal (~371-395) still answerInternal, while begin/completion and #417's Pause answerAborted. Unreachable under current locks; a one-line comment on why (or aligning toAborted) would keep suspend and Pause consistent. - At the stack top,
require_state_writesurvives only for parity tests viaexpect(dead_code); consider moving it to#[cfg(test)]or callingrequire_applied()directly.
Stack-level: no code blockers. The lifecycle race soak is a merge gate for you to close out, separate from this review.
Review — 2 findingsLow
What I checked and found sound:
I deliberately left out the refused-rollback answer ( Verified against |
the-gigi-pplx
left a comment
the-gigi-pplx
left a comment
There was a problem hiding this comment.
Approved. 2 non-blocking findings in the review comment above.
Stacked on #417 (on #421, #416, #413, #418, #438 and main). Base:
ac/sbxmgr-sw-pr2-pauseat51bee12. This PR's diff is one commit on top of it. Stack, bottom to top: #438 → #418 → #413 → #416 → #421 → #417 → #414. #372 and #388 have merged to main.All state here is sbx-manager's node-local SQLite
sandboxes.state, not the control-plane Postgrespublic.sandboxes.lifecycle.What merging does
Suspend's auto-pause (
prepare_suspend_snapshot_publication_after_locksinservice/grpc_lifecycle/suspend.rs) runs for theSuspendRPC, auto-lifecycle suspend (reconcile.rs) and teardown suspend. It writes the row three times. After this PR each of those writes names the state it expects, and a row outside that from-set is refused with every row unchanged.startedupdate_state(Pausing)update_state_from(&edges::PAUSE_BEGIN)Request, {started} → pausingupdate_state(Started)update_state_from(&edges::PAUSE_ROLLBACK)Compensation, {pausing} → startedupdate_state(Paused)update_state_from(&edges::PAUSE_COMPLETE)Completion, {pausing} → pausedAll three outcomes go through #438's shared
require_state_write_from; this PR adds no mapping helper of its own. The rollback keeps a non-best-effort write:update_state_from_best_effortwould also mark the sandbox dirty, which this site never did.Suspend's guest preparation now goes through #417's
prepare_guest_for_pause, which this PR makespub(super)along with its test-onlyGUEST_PREPARATIONseam. In production that function is exactly the previous call (prepare_guest_for_vm_snapshot_via_spaced_host), so this changes nothing outside tests.Side columns, the caller's mutex and the lifecycle-lock scopes are unchanged.
update_state_fromadds its ownIMMEDIATEtransaction around its reads and theUPDATEin autocommit, whichupdate_statedid not; on the single connection that is harmless.#417's fail-closed adapter on
pause_with_reconciliation(suspend continues only onOk(Paused)) is kept as #417 wrote it.Who can move the row
All three writes run under the cleanup fence (try-locked), the wake lock and the artifact lock. Those exclude:
Stop's
stopping/stoppedtherefore cannot land during the auto-pause.The one writer that could move a
startedorpausingrow under these locks was warm-snapshot Start's unvalidatedupdate_state(Suspended)(try_start_from_warm_snapshot; theWARM_START_UNVALIDATEDknown race). #418, below this PR, converted it: a warm Start now refuses a row outsidecreated,stoppedandsuspendedbefore staging, and its restage write applies only overcreatedorstopped. No known writer can now move the row during the auto-pause. Productionfrom_warmStarts come from create's own autostart and the direct substrate's Start right after create, both on a just-created row; afrom_warmStart on a running row was in practice an operator CLI Start.What the refusal buys
This is a defensive refusal, not a race fix. Should a writer move the row anyway, the refused row keeps that writer's state over a VM that is running (begin, rollback) or that node-manager has just paused (completion). That end state is no better than #388's, where the auto-pause overwrote it. #418 (PR 4,
WARM_START_RESTAGEfrom {created, stopped}) closes the warm-start interleaving itself, and sits below this PR, so it lands first.On this stack, together with #417 (whose
PAUSE_ROLLBACKbest-effort rollbacks andpausing-only quarantine compare-and-set coverpause_with_reconciliation), no write in the suspend auto-pause lands over a state other than the one it expects. Before #417,pause_with_reconciliation's best-effortstartedcould still writesuspended → started.Behavior changes
startedbefore thepausingwrite is refused. The RPC returnsAborted, and the row stays as that writer left it. Before, the row was overwritten withpausingand the suspend went on.pausingis not writtenstarted. The refusal is logged (failed to roll back checkpoint pause state), and the client still gets the guest-preparationInternal, unchanged. This differs from sbx-mgr: pause writes only over the state it expects #417's Pause RPC, which answers a superseded rollbackAborted. I kept suspend's answer because the guest-preparation failure is the error the caller acts on.pausingis not writtenpaused. The RPC returnsAbortedand does not begin the suspend; node-manager has already paused the VM. Before, the row was rewritten topausedand the suspend continued over the warm start's restaged snapshot.Client-visible messages, compared with #388
Aborted:begin checkpoint pause: sandbox state write was superseded: sandbox is <state>/complete checkpoint pause: …(new; #438's canonical text)Aborted:<context>: sandbox state write was not applied: blocked_by_quarantine/superseded_by_cleanup_episode/missingInternal:<context>: <error>guest snapshot preparation before suspend failed: …)Test
service/grpc_lifecycle/suspend/tests.rs::suspend_auto_pause_writes_only_over_the_state_it_expectsdrives theSuspendRPC on astartedrow. It sweeps everySandboxStateat each of the three writes:NodeManagerTransportholds suspend inside its node-managerconnect, between thestartedread and thepausingwrite, while the test moves the row. Guest preparation then fails, so the accepting control (started) writespausingand rolls back tostarted.pausing: it rolls back tostarted. Every other state keeps every row as moved, and the client still gets the guest-preparationInternal.FakeNodeManagerconfirmsPauseSandbox. The accepting control ispausing: it writespaused, and a temporary trigger then rejectsbegin_suspend'ssuspendingwrite, so the control stops before snapshot work (Internal,db error: <tripwire>).Expected answers come from a wildcard-free per-state guard table written from #372's
PLAINgrid, not derived from the writer. Refusals assert the exactAbortedmessage and that every row of every table is unchanged since the interleaving (assert_unchanged). The tripwire trigger is installed at all three exits, so a begin or rollback case that wrongly reachedbegin_suspendwould fail on it rather than run snapshot work; it fires on no refused case. The test also asserts guest-preparation and node-manager call counts, and that the cleanup fence is released afterwards.Host safety: in-memory SQLite,
for_test_in(tempdir), the guest-preparation seam (no spaced-host socket is dialed) and a test-served node-manager. Every wait is bounded (30 s timeouts on the gate, the RPC and service shutdown), with no sleeps.Edges and allowlist
KnownRaceedge described these writes.PAUSE_BEGIN's doc comment now says the suspend auto-pause writes after readingstarted; it never callstransition(Pausing). ThePAUSE_COMPLETEandPAUSE_ROLLBACKcomments from sbx-mgr: pause writes only over the state it expects #417 are accurate on this head and unchanged.Pins
All green and unchanged: the suspend and teardown-suspend tests, #372's matrix, accepted-shape and thin-writer tests, #388's table, parity and edge-binding tests, and #417's pause supersession tests.
Overlaps
PAUSE_*edges;pause_with_reconciliation's writes are sbx-mgr: pause writes only over the state it expects #417's.require_state_write. In this order the auto-pause was its last production caller, so it now carries#[cfg_attr(not(test), expect(dead_code, …))]; only theupdate_stateparity tests still call it.Findings
suspended → pausing,suspended → startedorsuspended → paused, although the warm-start interleaving reached all three through the auto-pause. On this stack (with sbx-mgr: pause writes only over the state it expects #417) they are unreachable, so no edge is needed.Mutation proof
Each mutation ran alone, from one backup, against the final head, and was restored before the next.
update_state(+require_state_write), with guest preparation still on the seamBegin, Creating)update_state(Pausing)update_state(Started)update_state(Paused)PAUSE_BEGIN's from-set set toStateSet::ALLPAUSE_ROLLBACK's from-set set toStateSet::ALLPAUSE_COMPLETE's from-set set toStateSet::ALLThe literal #417
suspend.rsis not used as the red baseline: its guest preparation would dial the host's spaced-host socket.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 (
9a0a251): 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 1650/1650 three times on this exact head, with 0 dependency skips. Kind Pull CI ran on #417, the PR below this one (all 4 shards passed). This PR's own Kind evidence is the Kind run on the top head below.This PR is the top of the stack (
9a0a251):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
cargo fmt --all -- --check,scripts/check-rust-error-policy.shcargo clippy --workspace --locked --all-targets --all-features -- -D warningscargo archpre-commit(Gitleaks) on the changed filesSPACE_CI_REQUIRE_TEST_DEPS=1 cargo nextest run --locked -p pplx-space-sbx-mgr --retries 0× 3, final headSPACE_TEST_DEP_SKIP_LOGnever written); 1 skipped = the#[ignore]privilegedtransfer_directory::tests::distinct_service_uids_can_exchange_downloads, unrelatedsmoke-kind-e2e.sh's bootstrap from this checkoutsuspend.rs,pause_resume.rsandstate_write.rsis identical to this head. Thespace-sbxmgr-svcartifact built during the bring-up has sha25668c308ec…9aff, matching/opt/space/bin/space-sbxmgr-svcin bothspace-sandbox-managerand both builder sandbox-manager pods. The stack's service logs (node-ctl's, for one) report version4d190bc08ed8, this headok, 4not_applicable, none building or queuedsmoke-kind-e2e.sh(DATA_PLANE_URLset as CI does)smoke-kind-typed.sh all(ECW + native;SPACE_SMOKE_KIND_COLD_RECEIPTSand admin token file set as CI does)local.node_group_boundary_schedules,local.rate_limiters_enforce_and_fail_openspace-apiwith new env, and the client's create got no response within the scenario's timeout (status 0). Sandbox-manager's log shows the timed-out create admitted and its guest ready within about 2 s. Both passed in round 0 on this harnesslocal.ecw_template_build_from_oci_bootsINFRA_ERROR(build-manager storage admission needs 55834574848 bytes, finds about 15.3 GB). ENVIRONMENTAL: builder storage on this devbox, as in round 0; no sandbox-manager pathinfra/space/kind/scripts/teardown.sh: pass; no clusters, containers or Tilt watcher leftDecisions made without an operator
Internal(see Behavior changes).SuspendFailedevent orcreate_failedmetric on a refused begin or completion. Guard refusals at these sites have never emitted one. Adding a new failure-reason label is a telemetry-vocabulary change better made for every refusal arm at once, so I left it out.prepare_guest_for_pause,GuestPreparationandGUEST_PREPARATIONbecomepub(super)withingrpc_lifecyclerather than moving, to keep sbx-mgr: pause writes only over the state it expects #417's file stable.docker builder prune -afanddocker image prune -f, as authorized. No containers were running; volumes were untouched.install-prereqs.sh(INSTALL_DIR=~/.local/bin).🤖 Generated with Claude Code