Skip to content

sbx-mgr: characterize the state writers before the state-write helper (tests only) - #372

Merged
ac-pplx merged 1 commit into
mainfrom
ac/sbxmgr-state-write-pr0-tests
Oct 14, 2026
Merged

ac-pplx merged 1 commit into
mainfrom
ac/sbxmgr-state-write-pr0-tests

Conversation

@ac-pplx

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

Copy link
Copy Markdown
Contributor

Pins today's behavior of sbx-mgr's state writers, so that the later update_state_from helper 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 SQLite sandboxes table (state, SandboxState) and its sandbox_cleanup_episodes, sandbox_quarantine_*, local_artifacts, sandbox_restore_transfer_scopes and jobs rows. None of it touches the control-plane Postgres public.sandboxes.lifecycle.

No production code changes. The only production-file edit is one #[cfg(test)] mod stop_admission_tests; line in service/cleanup.rs. The six new DB modules are wired by mod lines in the existing test file db/tests.rs.

What is pinned

1. The update_state outcome matrix (db/tests/update_state_matrix.rs)

update_state is the generic node-local SQLite sandboxes.state writer, and it has no expected-state check. The matrix covers all 18×18 (from, to) pairs of SandboxState for each cleanup-ownership shape that changes its answer:

  • no intent and no episode;
  • a delete_row row intent;
  • an armed create episode (delete_row);
  • an armed cold-start episode (mark_stopped), which isolates the "episode does not delete the row" predicate;
  • a requested create episode.

Each pair runs twice:

  • once with a transition receipt for to itself (or a started receipt when to cannot carry one);
  • once with a receipt for another receipt-capable state, for example a started receipt with to = 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, Missing or Err(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 leaving restoring), and the transition receipt (cleared unless its target equals to). An applied deleting write also fails the row's active teardown-suspend jobs row. Any other outcome changes nothing.

Every seed carries something the bug would show:

  • a failure reason;
  • a receipt;
  • a remote restore's three columns when from is restoring;
  • backdated timestamps;
  • an active teardown-suspend job.

The expected outcomes are written out as literal grids, not derived from can_transition_to. Rows and columns are indexed by a wildcard-free match over SandboxState, 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:

  • the failure reason;
  • the transition receipt;
  • the runtime with a valid spaced endpoint proof, and the route identity;
  • artifact pins and the suspend predecessor;
  • an active teardown-suspend job.

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_writers and start_arms_access_idle_only_after_guest_readiness cover arming. A creating row 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_rows cannot 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):

    • the five eviction claims, with all four pin/version statements of claim_suspended_sandbox_for_eviction;
    • begin_suspend and rollback_suspending_to_paused;
    • restore_checkpoint_state, recover_interrupted_pause_state and restore_resident_resume_state, each to started and to paused;
    • the three publish_restored_* writers;
    • publish_stopped_artifact from stopping, from cleanup_required and from an unpinned stopped row;
    • pin_suspended_artifact_for_cleanup.
  • Cleanup-episode bound (PR 8):

    • activation admission (begin_activation_with_route_evidence): cold start from created, stopped and suspended, and restore;
    • mark_start_succeeded and mark_restore_succeeded (to started and to paused);
    • complete_create as created and as suspended, with publish_activation_completion false 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, a created row gains an activation_completed receipt and a suspended row gains none: the schema allows activation-completed receipts only for created, started and paused;
    • complete_pool_clone, and complete_import as stopped and as suspended;
    • both activation rollbacks;
    • request_cleanup_episode from creating and from restoring;
    • record_cleanup_episode_failure from cleanup_required and from cleanup_failed;
    • quarantine_cleanup_episode_for_job for an activation episode and for a pool-clone disposal;
    • rearm_prepared_clone_disposal after a charged and failed attempt;
    • apply_cleanup_state through complete_cleanup_episode_for_job (mark_created, mark_stopped, mark_suspended, delete_row, delete_pool_clone);
    • apply_cleanup_state through complete_row_intent_cleanup_for_job, with its returned disposition: stop, suspend with an artifact, the startup quarantine intent and delete return Applied; suspend without an artifact returns Quarantined.
  • Quarantine (PR 9):

    • mark_quarantined_from_state from started and from restoring. Quarantine keeps the runtime, route identity and endpoint proof;
    • recover_pause_failed_quarantine (to started and to paused);
    • publish_orphan_quarantine_recovered_artifact, with and without an artifact, after a failed staging attempt recorded its reason;
    • the adopted recovery episode's completion.

    adopt_quarantine_cleanup_episode and settle_startup_resident are 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-checkpointing source states are refused; a missing row is refused.

  • update_state_superseding_activation_in_transaction, to both stopping and deleting:

    • an armed cold-start episode and a requested create episode are superseded: the episode is deleted and the intent cleared before the generic write;
    • a create quarantined while its episode was armed refuses Stop. The generic write is BlockedByQuarantine, and the episode deleted earlier in the same transaction comes back when the transaction is abandoned. Delete is accepted;
    • an adopted quarantine-recovery episode refuses Stop with QuarantineRecoveryInProgress and yields to Delete;
    • a missing row returns Missing.

    Per-source-state outcomes are the matrix's.

  • settle_startup_resident:

    • all 12 {started, paused, resuming, checkpointing} × {Started, Paused, Quarantined} settlements, as whole-row diffs;
    • all 14 other observed states are rejected;
    • 23 single-column changes to the observed row each supersede the observation, plus a missing row: version, state, failure reason, 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:

    • the adoption of a synthetic quarantined orphan, as a whole-row diff;
    • all 17 non-restoring inserts are rejected;
    • 42 single-predicate perturbations of the orphan each refuse adoption: 16 states, the three quarantine-marker predicates, and every column of the synthetic tuple.
  • adopt_quarantine_cleanup_episode:

    • the accepted shape, with the generation seeded and with it absent;
    • all 17 other source states, each with the rest of the shape intact and no restore columns, so the state guard itself refuses;
    • a different pin id, a disk pin, a stop pin 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_artifacts already 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_cleanup with mark_stopped) from all 18 source states, with the intent persisted (non-blocking Stop) and without (blocking Stop):

  • Admitted from 16 states: each leaves the row stopping with its intent and an active cleanup job.
  • Four of those 16 are admission-only, unreachable through the Stop RPC: creating, created, stopped and suspended. The RPC resolves its target with running_only, which selects nothing unless the sandbox's fc.pid resolves to a live process, and those states have no Firecracker process.
  • Refused from deleting and quarantined with Unavailable, 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_none already pins that a live runtime is admitted.

How the tests run:

  • The service is built from SandboxManagerServiceOptions::for_test_in(scratch), so the run directory is the test's own.
  • Node cleanup reaches only the executor's test dispatch, which holds until shutdown.
  • Each admission, each RPC and each shutdown is bounded by a labelled 30-second timeout.
  • Admission never consults the shutdown token, but the cleanup executor it would start does not start once the service is shut down.
    • Each admission case cancels the token, and waits (bounded) for the service's own background tasks to exit.
    • Only then does it snapshot the database and admit, so neither the executor nor its recovery scan runs. The refused cases' whole-database comparison therefore sees admission's writes alone.
    • Each case then asserts that the service's task tracker is still empty.
  • An admitted Stop RPC's watchdog removal stats the fixed sidecar pid path, so the admission sweep calls the admission function directly.

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), then set_cleanup_intent(tx, id, true, MarkStopped), then commit. That persisted intent is a non-blocking Stop's. "Stop completion" is complete_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.

    1. A started row; update_state(Pausing) returns Applied.
    2. Stop admission leaves stopping with intent mark_stopped.
    3. Stop completion leaves stopped: runtime and route cleared, execution generation 7 kept as teardown proof, intent cleared.
    4. The pause rollback's update_state(Started) returns Applied.

    Resulting row: state=started, pid=NULL, uid=NULL, route generation NULL, execution generation 7, no intent, no episode: a started row with no VM. The completion path's update_state(Paused) over the same stopped row is also Applied and leaves state=paused in the same shape.

  • Plan finding 2: resident resume over stopped.

    1. A paused row; resume's read sees paused.
    2. Stop admission and Stop completion leave stopped.
    3. The no-policy branch's update_state(Resuming) returns Applied.

    Resulting row: state=resuming, runtime cleared, execution generation 7, no intent.

  • Plan finding 4: create completion over stopping.

    1. insert_sandbox_with_cleanup_episode_and_route_evidence creates a creating row with an armed delete_row episode.
    2. Stop admission deletes the episode, clears its intent, and writes stopping with intent mark_stopped.
    3. Create's update_state(Created) returns Applied.
    4. complete_create(cleanup_id) returns false, so Create reports failure.
    5. Stop completion returns Applied but changes nothing, because created does not own a mark_stopped intent.

    Resulting row: state=created, intent mark_stopped/keep, no episode. In base-snapshot mode the following update_state(Suspended) is also Applied, and the row ends suspended with the same intent.

  • Plan finding 5: process-snapshot rollback over stopping.

    1. A stopped row; rollback's read sees stopped.
    2. Stop admission writes stopping with intent mark_stopped.
    3. The rollback's update_state(Suspended) returns Applied.
    4. Stop completion returns Applied and changes nothing.

    Resulting row: state=suspended, intent mark_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 creating or stopped row, because the runtime gate selects only a live fc.pid. Its service callers also send blocking: 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.

  1. CONFIRMED: a row with a stale mark_stopped intent cannot be activated. Races 4 and 5 above end in a created or suspended row that carries a mark_stopped row intent and no episode.

    • Activation refuses it: begin_activation_with_route_evidence admits only a NULL intent or the activation's own completion. Reproduced: begin_activation(created, ColdStart), begin_activation(suspended, Restore) and begin_activation(suspended, ColdStart) all return Ok(None).
    • Clients see a retryable Unavailable and loop.
    • Stop completion cannot clear the intent: a created or suspended row does not own a mark_stopped intent, and with no job id complete_row_intent_cleanup_for_job reports Applied for it (the plan's F-f).
    • Only Delete repairs the row. An auto-lifecycle Stop of a suspended row, 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).

  2. The create-born completions treat failure_reason differently, on an unreachable input.

    • complete_import would keep a failure_reason; its siblings complete_create and complete_pool_clone clear it.
    • A creating row never carries one in production, so the difference is unobservable. The seeds carry none, and that is why mutation E7 (complete_create keeps it) survives.
    • It is not pinned, so a later unification (D4) can change it without changing a test.
  3. rearm_prepared_clone_disposal leaves the sandbox's sandbox_quarantine_state row while the row moves from quarantined to cleanup_required. assert_quarantine_invariants documents only the adoption exception to "a quarantine row exists only for a quarantined or restoring sandbox". 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.

  4. 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.

  5. 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. Only adopt_quarantine_cleanup_episode creates either shape, and it always sets both. The test pins that production shape.

  6. 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 observation settle_startup_resident receives has a NULL intent, and on such an observation the two checks agree.

  7. Reachability limits, not pinned:

    • restore_adopt_orphan_or_insert_sandbox_in_transaction also adopts over a restoring row that still has the synthetic orphan tuple. I found no production path that leaves one, so only the quarantined adoption is pinned.
    • Supersession clears the row intent only when it matches the episode. I found no reachable row whose intent differs from its live episode (set_cleanup_intent refuses while an episode exists), so the mismatch case is not pinned.
    • Following the rubric, nothing pins the writers sbx-mgr: remove dead and test-only state writers #332 made test-only, or mark_quarantined_inner's reconfirmation branch.
  8. Confirmed and pinned as current behavior:

    • D2: the Stop admission function admits from 16 source states. Through the Stop RPC, four of them (creating, created, stopped and suspended) are gated off by the live-runtime check.
    • F-e: begin_suspend, rollback_suspending_to_paused, the eviction claims and the publications keep transition receipts.
    • F-e: publish_restored_suspended_artifact keeps failure_reason.
    • F-e: settle_startup_resident stamps state_changed_at even when the state is unchanged.
    • update_state from deleting supersedes every non-delete target even without a persisted intent, because the deleting state alone owns delete_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.

  • Every mutation was checked to match its target text the expected number of times. Three receipt mutations deliberately change all six CASE arms of a writer together. A half-kept receipt would violate the receipt CHECK, and a consistent one leaves the assertions as the oracle.
  • SQL mutations keep every bound ?N and :name parameter, so a failure is the assertion's, not rusqlite's parameter check.
  • A kill whose output carries CHECK constraint failed is labelled "killed by the schema CHECK".
  • After the run, git diff was clean, and git diff --stat origin/main shows only test files plus the one #[cfg(test)] mod line.
Production file Mutations Killed by an assertion Killed by the schema CHECK Survived
db/state_transitions.rs 36 31 1 4
db/eviction.rs 5 5 0 0
db/local_artifacts.rs 9 9 0 0
db/transfer.rs 1 1 0 0
db/cleanup_episodes.rs 17 17 0 0
db/quarantine.rs 5 5 0 0
db/quarantine_recovery.rs 9 9 0 0
db/startup_recovery.rs 3 3 0 0
db/sandboxes.rs 54 50 0 4
service/cleanup.rs 2 2 0 0
service/cleanup_targets.rs 1 1 0 0
Total 142 133 1 8

Schema-enforced kill: U13, update_state keeping restore_storage_uri when leaving restoring. The schema CHECK ties the restore columns to state = 'restoring', so no writer can keep them; the matrix catches it through the CHECK error.

Mutation N9 makes complete_create ignore its publication flag and always publish. It is killed by the publish = false cases, 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):

# Mutation Why it survives
U12 update_state: drop the SQL quarantine guard The Rust pre-check refuses first, under the same connection
E7 complete_create: keep failure_reason A creating-born row never carries one
T4 Supersession: drop the entered-from-quarantined disjunct It is never set without the recovery fence
T5 Supersession: drop the recovery-fence disjunct It is never set without entered-from-quarantined
T23, T24 Startup witness: ignore intent equality Resident states never own an intent
T32, T33 Startup witness: ignore "current intent is NULL" Resident states never own an intent

Overlaps with open pull requests

Validation

Check Result
cargo fmt --all -- --check pass
cargo clippy --workspace --locked --all-targets --all-features -- -D warnings pass, 0 warnings
SPACE_CI_REQUIRE_TEST_DEPS=1 cargo nextest run --locked -p pplx-space-sbx-mgr --retries 0, three consecutive runs 1613/1613 passed each run. The 1 skipped test is the existing root-only #[ignore] transfer_directory::tests::distinct_service_uids_can_exchange_downloads; there were 0 dependency skips.
Stop tests, --stress-count 200 200/200; also 100/100 under 48 busy CPU loops
Mutation proof 134/142 killed (133 by an assertion, 1 by the schema CHECK); the 8 survivors are explained above
Pre-commit (Gitleaks) pass
Public-content review of the diff and this text no findings
CI see the pull request checks
Kind not run: this change is tests only

🤖 Generated with Claude Code

@ac-pplx
ac-pplx force-pushed the ac/sbxmgr-state-write-pr0-tests branch 4 times, most recently from c3538ee to 8f12bd6 Compare October 13, 2026 10:03
@ac-pplx
ac-pplx marked this pull request as ready for review October 13, 2026 10:12
@ac-pplx
ac-pplx added this pull request to stack #401 October 13, 2026 21:14
@kumarpallav-pplx
kumarpallav-pplx requested review from nikhilgupta-pplx and the-gigi-pplx and removed request for shrikant-pplx and zibi-pplx October 13, 2026 21:18
@the-gigi-pplx

Copy link
Copy Markdown
Collaborator

Review — 8 findings

Tests 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

  1. crates/libs/sbx/mgr/server/src/db/tests/state_write_fixtures.rs:35 — no seed has a sandbox_open_route_activations row, so close_claim is invisible to every diff.
    route_activation_claims::close_claim deletes from that table, and the quarantine and cleanup writers call it: quarantine.rs:854, startup_recovery.rs:128, cleanup_episodes.rs:1033/1399/1502, local_artifacts.rs:458, state_transitions.rs:482/814/947. The table is empty in every seed, so the expected diff can't include its deletion. I commented out quarantine.rs:854 and then startup_recovery.rs:128, one at a time, and the whole sbx-mgr suite stayed 1613/1613 green both times. That covers the new tests and everything that was already there. A shared helper that loses close_claim would leave an open route activation behind a quarantined row, and nothing would fail. I'd seed an open claim in the quarantine and cleanup shapes and expect .deleted("sandbox_open_route_activations", …).
  2. crates/libs/sbx/mgr/server/src/db/tests/accepted_shapes_cleanup_episodes.rs:805 — the delete_row / delete_pool_clone completion shapes seed no route execution generation and no child rows. So the execution delete receipt and the child-row cascade before DELETE FROM sandboxes aren't observed. record_execution_delete_receipt returns early on None, and delete_sandbox_child_rows has nothing to delete. I commented out cleanup_episodes.rs:1017 (the receipt), then cleanup_episodes.rs:1018 (the cascade), one at a time, and the whole suite stayed 1613/1613 green both times. With the cascade gone, the sandbox row is deleted and its children are orphaned, with foreign keys off. I'd give these shapes an execution generation plus one row in each child table delete_sandbox_child_rows covers, and expect the receipt insert and the child deletions. The row-intent delete completion (cleanup_episodes.rs:1314-1320) has the same seed shape. I didn't mutate that path.

Low

  1. crates/libs/sbx/mgr/server/src/db/tests/state_write_fixtures.rs:763 — quarantine_state_entered and quarantine_history_entered (line 789) both take quarantine_id as Cell::Fresh. Nothing ties the history row's id to the state row's, so a writer that stamps two different fresh ids would pass every quarantine-entering case.
  2. crates/libs/sbx/mgr/server/src/service/cleanup/stop_admission_tests.rs:87 — the Admitted vs AdmittedOnlyDirectly split is never asserted. The test branches only on Refused (line 130), and the RPC sweep expects "selects nothing" for every state. Moving a state between the two variants changes no outcome, so the classification is documentation that looks like a check. Either assert it, for example with a live-runtime RPC case for an Admitted state, or fold the two variants and keep the reason as a comment.
  3. crates/libs/sbx/mgr/server/src/db/tests/accepted_shapes_single_statement.rs:4 — the module doc says each accepting case seeds runtime, endpoint proof, route identity and an active teardown-suspend job. Only the disk-as-new and stopped-publication cases call seed_runtime, and only the eviction claims call seed_teardown_job. decorate seeds the failure reason, the receipt and the timestamps only. The other cases can't show a helper that wrongly clears or keeps runtime columns, or fails a teardown job. Either seed them or narrow the doc to what is seeded.
  4. crates/libs/sbx/mgr/server/src/db/tests/accepted_shapes_single_statement.rs:304 — publish_restored_suspended_artifact keeps the seeded failure_reason, but its sibling publish_restored_suspended_without_artifact clears it. The description lists this as F-e, but the test pins it with no comment. Whoever converts these writers will meet a failing cell with no hint that the divergence is known. A one-line comment at the cell list would do.
  5. crates/libs/sbx/mgr/server/src/db/tests/thin_writer_coverage.rs:268 — every startup-resident seed carries a started receipt. So the keep-matching-receipt arm is only exercised for the Started settlement, never for Paused. A paused receipt on the Paused settlement cases would cover it.
  6. crates/libs/sbx/mgr/server/src/db/tests/update_state_matrix.rs:276 — Receipt::target returns the same state for ForTarget and ForAnotherState whenever to can't carry a receipt. So for the receipt-incapable targets the two variants are the same seed run twice. The 3,240 cases in the description are runs, not distinct cases. Skipping the second variant for those targets would cut suite time without losing coverage.

Verified against 8f12bd6569ecbac8467443dc7978b4e9ca62208a. Reproduced locally with SPACE_CI_REQUIRE_TEST_DEPS=1:

  • The full sbx-mgr nextest suite passed at this head: 1613/1613, with only the existing #[ignore] skip.
  • I made four single-line production mutations in a scratch tree, each run separately against the full suite: quarantine.rs:854, startup_recovery.rs:128, cleanup_episodes.rs:1017 and cleanup_episodes.rs:1018. I confirmed each one recompiled the crate. All four survived. That's the evidence for findings 1 and 2.

I checked and dropped one candidate: the quarantine-episode matrix case seeds no restore columns. The schema CHECK ties those columns to restoring, so a quarantined row can't carry them.

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 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. 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.
@ac-pplx
ac-pplx force-pushed the ac/sbxmgr-state-write-pr0-tests branch from 8f12bd6 to 6d16ffb Compare October 14, 2026 03:11
@ac-pplx
ac-pplx added this pull request to the merge queue Oct 14, 2026
Merged via the queue into main with commit a7e700b Oct 14, 2026
20 checks passed
@ac-pplx
ac-pplx deleted the ac/sbxmgr-state-write-pr0-tests branch October 14, 2026 03:30
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.

2 participants