Repository navigation
sbx-mgr: characterize the state writers before the state-write helper (tests only) - #372
Conversation
c3538ee to
8f12bd6
Compare
Review — 8 findingsTests only, so nothing here can break the build or production. The two Mediums are places where the "complete whole-database diff" can't see a side effect, so a later conversion could drop it and stay green, which is the regression this PR exists to catch. Medium
Low
Verified against
I checked and dropped one candidate: the quarantine-episode matrix case seeds no restore columns. The schema Findings 3–8 come from reading the code; I didn't run them. I didn't re-run the description's 142-mutation table or the races it describes. |
the-gigi-pplx
left a comment
There was a problem hiding this comment.
Approved. 8 non-blocking findings in the review comment above.
… (tests only) Pin today's behavior of sbx-mgr's writers of the node-local SQLite `sandboxes.state` (and their cleanup-episode and `sandbox_quarantine_*` companion rows), so the planned `update_state_from` helper and its caller conversions can prove they are behavior-preserving. Nothing here touches the control-plane Postgres `public.sandboxes.lifecycle`. - `update_state` outcome matrix: every (from, to) pair of the 18 lifecycle states, for a plain row, a `delete_row` intent, an armed create episode, an armed cold-start episode and a requested episode. Each pair runs once with a transition receipt for the target and once with a receipt for another state. The matrix also covers a row the production quarantine writer quarantined and a missing row. Each case asserts the outcome and the whole-database diff. The expected outcomes are literal grids indexed by a wildcard-free match over `SandboxState`, not derived from `can_transition_to`. - Whole-database accepted-shape diffs for each specialized writer that still has a production caller, keyed by primary key and diffed column by column, so a later move onto a shared helper must stay column-identical. Seeds carry a failure reason where production can, a transition receipt, a runtime with its spaced endpoint proof, route identity, artifact pins and an active teardown-suspend job. - Direct tests, with the accepting case and each refusing predicate in its own case, for restore_checkpoint_state, update_state_superseding_activation_in_transaction, settle_startup_resident, restore_adopt_orphan_or_insert_sandbox, adopt_quarantine_cleanup_episode and pin_suspended_artifact_for_cleanup. - Stop's cleanup admission function from every source state, with and without a persisted intent: admitted from 16 states, refused from `deleting` and `quarantined`. `creating`, `created`, `stopped` and `suspended` are admission-only. The Stop RPC selects only a live runtime, and from every state it admits nothing without one. The new DB modules live under `db/tests/`. The Stop module is wired by one `#[cfg(test)] mod` line in `service/cleanup.rs` and builds its service with `SandboxManagerServiceOptions::for_test_in`. Known races in today's writers are documented in the pull request, not asserted. No production code changes.
8f12bd6 to
6d16ffb
Compare
Pins today's behavior of sbx-mgr's state writers, so that the later
update_state_fromhelper and the caller conversions can each prove they are behavior-preserving, or name exactly what they change. Every test here is about the node-local SQLitesandboxestable (state,SandboxState) and itssandbox_cleanup_episodes,sandbox_quarantine_*,local_artifacts,sandbox_restore_transfer_scopesandjobsrows. None of it touches the control-plane Postgrespublic.sandboxes.lifecycle.No production code changes. The only production-file edit is one
#[cfg(test)] mod stop_admission_tests;line inservice/cleanup.rs. The six new DB modules are wired bymodlines in the existing test filedb/tests.rs.What is pinned
1. The
update_stateoutcome matrix (db/tests/update_state_matrix.rs)update_stateis the generic node-local SQLitesandboxes.statewriter, and it has no expected-state check. The matrix covers all 18×18(from, to)pairs ofSandboxStatefor each cleanup-ownership shape that changes its answer:delete_rowrow intent;delete_row);mark_stopped), which isolates the "episode does not delete the row" predicate;Each pair runs twice:
toitself (or astartedreceipt whentocannot carry one);startedreceipt withto = stopped.On top of those 3,240 cases, it covers every target over a row the production quarantine writer quarantined (with its
sandbox_quarantine_*episode), and every target for a missing row.Each case asserts both the outcome (
Applied,BlockedByQuarantine,SupersededByCleanupEpisode,MissingorErr(InvalidQuery)) and the whole-database diff. An applied write changes exactly these columns:state,state_changed_at(only when the state changes),updated_at,failure_reason, the three restore columns (cleared when leavingrestoring), and the transition receipt (cleared unless its target equalsto). An applieddeletingwrite also fails the row's active teardown-suspendjobsrow. Any other outcome changes nothing.Every seed carries something the bug would show:
fromisrestoring;The expected outcomes are written out as literal grids, not derived from
can_transition_to. Rows and columns are indexed by a wildcard-freematchoverSandboxState, so a new state does not compile until every grid places it.2. Whole-row accepted shapes, one per specialized writer (
db/tests/accepted_shapes_*.rs)Each accepting case seeds the side columns the writer might touch:
Then it asserts the complete diff of every table: the sandbox row column by column, including the route-retention trigger's
last_observed_*columns, plus every inserted, updated or deleted companion row. Rows are keyed by primary key, so a delete-and-reinsert under a new id is not mistaken for an update. A later move onto a shared helper must therefore stay column-identical.Seeded rows carry a disabled auto-lifecycle policy, so arming after activation writes nothing here.
started_transition_arms_disarmed_lifecycle_for_rollback_writersandstart_arms_access_idle_only_after_guest_readinesscover arming. Acreatingrow never carries a failure reason in production, so the create, pool-clone and import completions are seeded without one.The tests use a primary-key-keyed variant of
database_rows' whole-database snapshot.database_rowscannot tell an update from a delete-and-reinsert, and it cannot name changed columns.These are the writers on the plan's PR 7–9 lists that still have production callers:
Single statement (PR 7):
claim_suspended_sandbox_for_eviction;begin_suspendandrollback_suspending_to_paused;restore_checkpoint_state,recover_interrupted_pause_stateandrestore_resident_resume_state, each tostartedand topaused;publish_restored_*writers;publish_stopped_artifactfromstopping, fromcleanup_requiredand from an unpinnedstoppedrow;pin_suspended_artifact_for_cleanup.Cleanup-episode bound (PR 8):
begin_activation_with_route_evidence): cold start fromcreated,stoppedandsuspended, and restore;mark_start_succeededandmark_restore_succeeded(tostartedand topaused);complete_createascreatedand assuspended, withpublish_activation_completionfalse and true. Every create episode carries a generation and an attempt id, as production create episodes do, so only the flag decides whether a receipt is written. With the flag false, no receipt is written. With it true, acreatedrow gains anactivation_completedreceipt and asuspendedrow gains none: the schema allows activation-completed receipts only forcreated,startedandpaused;complete_pool_clone, andcomplete_importasstoppedand assuspended;request_cleanup_episodefromcreatingand fromrestoring;record_cleanup_episode_failurefromcleanup_requiredand fromcleanup_failed;quarantine_cleanup_episode_for_jobfor an activation episode and for a pool-clone disposal;rearm_prepared_clone_disposalafter a charged and failed attempt;apply_cleanup_statethroughcomplete_cleanup_episode_for_job(mark_created,mark_stopped,mark_suspended,delete_row,delete_pool_clone);apply_cleanup_statethroughcomplete_row_intent_cleanup_for_job, with its returned disposition: stop, suspend with an artifact, the startup quarantine intent and delete returnApplied; suspend without an artifact returnsQuarantined.Quarantine (PR 9):
mark_quarantined_from_statefromstartedand fromrestoring. Quarantine keeps the runtime, route identity and endpoint proof;recover_pause_failed_quarantine(tostartedand topaused);publish_orphan_quarantine_recovered_artifact, with and without an artifact, after a failed staging attempt recorded its reason;adopt_quarantine_cleanup_episodeandsettle_startup_residentare in section 3.3. Direct tests for thinly covered writers (
db/tests/thin_writer_coverage.rs)Each writer gets its accepting case and each refusing predicate in its own case. Every refusal asserts that the whole database is unchanged.
restore_checkpoint_state: all 16 non-resident targets are rejected; all 17 non-checkpointingsource states are refused; a missing row is refused.update_state_superseding_activation_in_transaction, to bothstoppinganddeleting:BlockedByQuarantine, and the episode deleted earlier in the same transaction comes back when the transaction is abandoned. Delete is accepted;QuarantineRecoveryInProgressand yields to Delete;Missing.Per-source-state outcomes are the matrix's.
settle_startup_resident:{started, paused, resuming, checkpointing} × {Started, Paused, Quarantined}settlements, as whole-row diffs;created_at,uuid8, the six runtime ids, the two socket paths, artifact id, kind and origin, route identity, process environment, endpoint proof, cleanup completion, keep flag, a cleanup episode and a transfer scope.restore_adopt_orphan_or_insert_sandbox:restoringinserts are rejected;adopt_quarantine_cleanup_episode:diskpin, astoppin and another generation, each refused on its own.The claim's fence conjuncts are already swept by
every_claim_fenced_statement_pins_every_conjunct.pin_suspended_artifact_for_cleanup: reusing a staged identity keeps its backup-completion marker.suspended_cleanup_pin_requires_a_suspending_row_and_a_refusal_keeps_prior_artifactsalready sweeps the state predicate.4. Stop admission and the Stop RPC's runtime gate (
service/cleanup/stop_admission_tests.rs)#271 covers Stop only from
started. This adds the following.The admission function (
admit_selected_cleanupwithmark_stopped) from all 18 source states, with the intent persisted (non-blocking Stop) and without (blocking Stop):stoppingwith its intent and an active cleanup job.creating,created,stoppedandsuspended. The RPC resolves its target withrunning_only, which selects nothing unless the sandbox'sfc.pidresolves to a live process, and those states have no Firecracker process.deletingandquarantinedwithUnavailable, leaving every row unchanged.The Stop RPC from all 18 source states with no live
fc.pid, blocking and non-blocking: each returns OK with no stopped ids and changes nothing.stop_selects_only_a_live_runtime_and_both_treat_an_empty_template_filter_as_nonealready pins that a live runtime is admitted.How the tests run:
SandboxManagerServiceOptions::for_test_in(scratch), so the run directory is the test's own.Known races (fixed by later PRs)
These are the plan's §3 findings 1, 2, 4 and 5. Each was reproduced with a throwaway DB-level test, which was then deleted, on main at c565c8e; the writers involved are unchanged on this PR's base. No committed test pins the racy outcome. Each later fixing PR adds its own red/green regression test.
"Stop admission" below is the transaction cleanup admission runs:
update_state_superseding_activation_in_transaction(tx, id, Stopping), thenset_cleanup_intent(tx, id, true, MarkStopped), then commit. That persisted intent is a non-blocking Stop's. "Stop completion" iscomplete_row_intent_cleanup_for_job(id, MarkStopped, None, None, None). Each runtime row starts with a live runtime (pid, uid, route generation 3, execution generation 7).Plan finding 1: pause over Stop's
stopped.startedrow;update_state(Pausing)returnsApplied.stoppingwith intentmark_stopped.stopped: runtime and route cleared, execution generation 7 kept as teardown proof, intent cleared.update_state(Started)returnsApplied.Resulting row:
state=started,pid=NULL,uid=NULL, route generation NULL, execution generation 7, no intent, no episode: astartedrow with no VM. The completion path'supdate_state(Paused)over the samestoppedrow is alsoAppliedand leavesstate=pausedin the same shape.Plan finding 2: resident resume over
stopped.pausedrow; resume's read seespaused.stopped.update_state(Resuming)returnsApplied.Resulting row:
state=resuming, runtime cleared, execution generation 7, no intent.Plan finding 4: create completion over
stopping.insert_sandbox_with_cleanup_episode_and_route_evidencecreates acreatingrow with an armeddelete_rowepisode.stoppingwith intentmark_stopped.update_state(Created)returnsApplied.complete_create(cleanup_id)returnsfalse, so Create reports failure.Appliedbut changes nothing, becausecreateddoes not own amark_stoppedintent.Resulting row:
state=created, intentmark_stopped/keep, no episode. In base-snapshot mode the followingupdate_state(Suspended)is alsoApplied, and the row endssuspendedwith the same intent.Plan finding 5: process-snapshot rollback over
stopping.stoppedrow; rollback's read seesstopped.stoppingwith intentmark_stopped.update_state(Suspended)returnsApplied.Appliedand changes nothing.Resulting row:
state=suspended, intentmark_stopped/keep, no episode.Reachability of findings 4 and 5 through the Stop RPC. These two sequences are DB-level only. The Stop RPC never admits a
creatingorstoppedrow, because the runtime gate selects only a livefc.pid. Its service callers also sendblocking: true, which persists no intent: the API lifecycle, Kubernetes teardown and auto-lifecycle. The remaining way to reach the stale-intent end state is an operator CLI non-blocking Stop of a live sandbox racing a warm-snapshot Start (plan finding 3).Findings
These are not asserted.
CONFIRMED: a row with a stale
mark_stoppedintent cannot be activated. Races 4 and 5 above end in acreatedorsuspendedrow that carries amark_stoppedrow intent and no episode.begin_activation_with_route_evidenceadmits only a NULL intent or the activation's own completion. Reproduced:begin_activation(created, ColdStart),begin_activation(suspended, Restore)andbegin_activation(suspended, ColdStart)all returnOk(None).Unavailableand loop.createdorsuspendedrow does not own amark_stoppedintent, and with no job idcomplete_row_intent_cleanup_for_jobreportsAppliedfor it (the plan's F-f).suspendedrow, or eviction, can also clear it incidentally.Severity: high per occurrence, low likelihood. Through the Stop RPC, races 4 and 5 cannot produce this state; see "Reachability" above. The remaining path is an operator CLI non-blocking Stop of a live sandbox racing a warm-snapshot Start (plan finding 3).
The create-born completions treat
failure_reasondifferently, on an unreachable input.complete_importwould keep afailure_reason; its siblingscomplete_createandcomplete_pool_cloneclear it.creatingrow never carries one in production, so the difference is unobservable. The seeds carry none, and that is why mutation E7 (complete_createkeeps it) survives.rearm_prepared_clone_disposalleaves the sandbox'ssandbox_quarantine_staterow while the row moves fromquarantinedtocleanup_required.assert_quarantine_invariantsdocuments only the adoption exception to "a quarantine row exists only for aquarantinedorrestoringsandbox". The row is removed when the disposal deletes the sandbox, and replaced if the disposal is quarantined again. It is pinned as current behavior, with a comment saying so.update_state's SQL quarantine guard is unobservable (mutation U12 survives).AND (?1 = 'deleting' OR state != 'quarantined')repeats the Rust pre-check under the same mutex guard. With one connection, no test can tell the two apart. It is defense in depth if a second connection ever appears.Supersession's two Stop refusals are never independently reachable (T4 and T5 survive). Stop is refused when the episode was entered from
quarantined, or when it carries a quarantine-recovery fence. Onlyadopt_quarantine_cleanup_episodecreates either shape, and it always sets both. The test pins that production shape.The startup-resident witness's intent checks cannot be told apart (T23, T24, T32 and T33 survive). The witness compares the intent with the observed row and also requires the current intent to be NULL. A resident state (
started,paused,resuming,checkpointing) never owns a cleanup intent, so every observationsettle_startup_residentreceives has a NULL intent, and on such an observation the two checks agree.Reachability limits, not pinned:
restore_adopt_orphan_or_insert_sandbox_in_transactionalso adopts over arestoringrow that still has the synthetic orphan tuple. I found no production path that leaves one, so only thequarantinedadoption is pinned.set_cleanup_intentrefuses while an episode exists), so the mismatch case is not pinned.mark_quarantined_inner's reconfirmation branch.Confirmed and pinned as current behavior:
creating,created,stoppedandsuspended) are gated off by the live-runtime check.begin_suspend,rollback_suspending_to_paused, the eviction claims and the publications keep transition receipts.publish_restored_suspended_artifactkeepsfailure_reason.settle_startup_residentstampsstate_changed_ateven when the state is unchanged.update_statefromdeletingsupersedes every non-delete target even without a persisted intent, because thedeletingstate alone ownsdelete_row.Mutation proof
Each mutation broke one predicate or side-column write in production code, ran the new tests, and reverted. The runs were strictly sequential, one file backup at a time.
CASEarms of a writer together. A half-kept receipt would violate the receiptCHECK, and a consistent one leaves the assertions as the oracle.?Nand:nameparameter, so a failure is the assertion's, not rusqlite's parameter check.CHECK constraint failedis labelled "killed by the schemaCHECK".git diffwas clean, andgit diff --stat origin/mainshows only test files plus the one#[cfg(test)] modline.CHECKdb/state_transitions.rsdb/eviction.rsdb/local_artifacts.rsdb/transfer.rsdb/cleanup_episodes.rsdb/quarantine.rsdb/quarantine_recovery.rsdb/startup_recovery.rsdb/sandboxes.rsservice/cleanup.rsservice/cleanup_targets.rsSchema-enforced kill: U13,
update_statekeepingrestore_storage_uriwhen leavingrestoring. The schemaCHECKties the restore columns tostate = 'restoring', so no writer can keep them; the matrix catches it through theCHECKerror.Mutation N9 makes
complete_createignore its publication flag and always publish. It is killed by thepublish = falsecases, whose episodes now carry a generation and an attempt id.The quarantine-adoption state guard (A1) is now killed by an assertion: the state sweep keeps every other predicate satisfied and seeds no restore columns.
Survivors (findings 2 and 4–6):
update_state: drop the SQL quarantine guardcomplete_create: keepfailure_reasoncreating-born row never carries oneOverlaps with open pull requests
SandboxManagerServiceOptions::for_test_in. This change edits no file either one edits.modlines todb/tests.rs, as this does, so expect a trivial conflict in that list.db/eviction.rs. That claim is not covered here.db/cleanup_episodes.rs,db/quarantine.rs,db/startup_recovery.rs) and Retry failed generation-suspend backups behind an admission gate #349 (db/teardown_jobs.rs) change no column these tests assert, as far as their diffs show.Validation
cargo fmt --all -- --checkcargo clippy --workspace --locked --all-targets --all-features -- -D warningsSPACE_CI_REQUIRE_TEST_DEPS=1 cargo nextest run --locked -p pplx-space-sbx-mgr --retries 0, three consecutive runs#[ignore]transfer_directory::tests::distinct_service_uids_can_exchange_downloads; there were 0 dependency skips.--stress-count 200CHECK); the 8 survivors are explained above🤖 Generated with Claude Code