[ac-pplx]

What this does

This removes production-dead state writers and two unreachable branches from sbx-mgr's node-local SQLite sandboxes table (state / SandboxState). It does not touch the control-plane Postgres public.sandboxes table (lifecycle).

The sbx-mgr state-write inventory (reports/state-write-helper-plan.md on branch ac/state-write-helper-plan, finding 8) lists eight dead or test-only writers and two unreachable branches. Each one was re-checked against current main (2d9e1d3). No reachable production path changes.

Self-assessed severity: none (cleanup). The change is behavior-preserving, removes about 30 lines of dead or unreachable code and makes nine items test-only.

Writers: no production caller, now #[cfg(test)]

Every writer below is still used by tests, either as a fixture (seeding a quarantined, stopping or deleting row) or as a test entry point into a shared writer. mark_starting_quarantined and mark_restoring_quarantined reach mark_quarantined_from_state. mark_quarantined calls mark_quarantined_inner directly with no expected state. rollback_starting and rollback_restoring reach rollback_activation_state. The superseding writers reach update_state_superseding_activation_in_transaction. Deleting them would mean rewriting those fixtures in test files that open PRs edit, and widening private helpers' visibility. So they move under #[cfg(test)] and their bodies are unchanged.

Item	File	Non-test callers on main	Test callers
mark_quarantined	db/quarantine.rs	0	22 (db, service, recovery and timestamps tests)
mark_starting_quarantined	db/state_transitions.rs	0	2
mark_restoring_quarantined	db/state_transitions.rs	0	6
rollback_starting	db/state_transitions.rs	0	2
rollback_restoring	db/state_transitions.rs	0	2
mark_stopping_superseding_activation	db/state_transitions.rs	0	3
mark_deleting_superseding_activation	db/state_transitions.rs	0	2
clear_cleanup_intent (side-column writer)	db/cleanup_episodes.rs	0	5
update_state_superseding_activation (private helper of the two superseding writers; mechanical fallout)	db/state_transitions.rs	0	via the two above

Call-site inventory, before and after. I grepped the whole repository for each name: every hit outside its definition is in a tests.rs or tests/ module. The only crate that depends on pplx-space-sbx-mgr is crates/apps/sbx/mgr/svc, and it imports run, SERVICE_NAME, SOCKET_PATH and SbxMgrRunConfig only.

I also ran a throwaway compiler probe: make db crate-private (pub(crate) mod db) and cargo check -p pplx-space-sbx-mgr --lib (non-test). On main, rustc reports all nine items above as "never used". With this change those nine warnings disappear because the items no longer exist outside tests, and no new item becomes unused. Production code no longer reaches any of them.

Unreachable branch 1: request_cleanup_episode keeping the restore columns (inventory F-d)

request_cleanup_episode kept restore_origin, restore_phase and restore_storage_uri when the episode carried a quarantine-recovery fence (episode.quarantine_recovery.is_some()). Now it always clears them.

Proof the branch is unreachable. The UPDATE runs only for an episode whose requested_at is NULL, which means phase = 'armed' (schema CHECK). The function returns earlier for a requested episode. For the branch to run, an armed episode would need recovery fence columns:

• Producers of a fence. recovery_local_artifact_id and recovery_execution_generation are written by exactly one production statement. That is the INSERT in adopt_quarantine_cleanup_episode (db/quarantine_recovery.rs), and it inserts phase = 'requested' with requested_at set. insert_cleanup_episode, the only other INSERT, never sets fence columns. No UPDATE writes them.
• Transitions back to armed. Every UPDATE of sandbox_cleanup_episodes (charge_prepared_clone_disposal_attempt, rearm_prepared_clone_disposal, bind_prepared_clone_root, request_cleanup_episode, record_cleanup_episode_failure, the two operation_activation_attempt_id writers) either sets phase = 'requested' or leaves phase alone. None sets phase = 'armed' or requested_at = NULL. Nothing outside crates/libs/sbx/mgr/server/src writes the table, and the baseline is the only migration.
• Even if it were reached, the schema CHECK ties non-NULL restore columns to state = 'restoring'. Keeping them while writing cleanup_required either keeps NULLs, which is the same as clearing them, or fails the CHECK.

Probes (throwaway, not committed):

• I made the branch panic on main (assert!(!preserve_restore_operation)) and ran the full sbx-mgr suite: 1568/1568 passed, so no test reaches it.

• A throwaway test drove the only producer (adopt_quarantine_cleanup_episode) and then called request_cleanup_episode. It returned AlreadyRequested and the row was unchanged, on main and with this change.

• The same test then forced two rows with raw SQL, which no production statement can do:

  • the episode back to phase = 'armed', requested_at = NULL;
  • the sandbox row back to state = 'restoring', restore_origin = 'local', restore_phase = 'activation'.

  That made the main panic probe fire, confirming the probe was live. Without the panic, main returned the restore-column CHECK constraint failed error. With this change it returns Requested with the restore columns cleared.

• Forcing only the episode back to armed, with the row left in cleanup_required and NULL restore columns, gives the same result on both: one row changed, NULL columns. The arm can matter only when restore columns are set, so any behavioral difference is limited to input no production path can produce.

Unreachable branch 2: cleanup admission's non-stop/delete else

admit_lifecycle_cleanup_fenced called update_state when target_state was neither Stopping nor Deleting. Now it always calls update_state_superseding_activation_in_transaction, whose own debug_assert! already requires one of those two states.

Proof the branch is unreachable. target_state comes only from CleanupAdmissionSpec. All four constructors in service/cleanup.rs produce Stopping or Deleting:

• admit_auto_lifecycle_delete → Deleting.
• admit_selected_cleanup → Stopping or Deleting.
• admit_lifecycle_cleanup_from_observed_state takes claim.target_state. Its only production caller (grpc_spaced_process_snapshots.rs) passes Stopping, and so does its direct test caller (service/tests.rs ~10880).
• admit_lifecycle_cleanup_for_observed_execution takes a parameter. Its only production caller (grpc_lifecycle/kill.rs) passes Stopping. Its direct test callers (service/tests.rs ~10636, 10676, 10697, 10807 and 10921) all pass Stopping. The test-only admit_lifecycle_cleanup helper (service/tests.rs ~57) also calls it, and the helper's nine callers pass Stopping or Deleting.

Probe. I made the else branch unreachable!() on main and ran the full sbx-mgr suite: 1568/1568 passed.

With whitespace ignored (git diff -w), this hunk is pure deletion: the if matches!(…) { header and the else { … } body.

Considered and left alone

• Other test-only items the compiler probe found. These are outside this inventory and left for a follow-up: begin_activation, complete_cleanup_episode, quarantine_cleanup_episode, restore_adopt_orphan_or_insert_sandbox (in db/sandboxes.rs, which sbx-mgr: keep quarantined rows through inventory and bound recovery that can never succeed #270 edits), claim_teardown_suspend_operation, delete_sandbox. Also the readers count_pool_sandboxes, current_quarantine_reason_wire_value, list_sandbox_id_states and list_by_state.

• Production-unreachable arms of mark_quarantined_inner (db/quarantine.rs ~805–1001). With mark_quarantined test-only, every production path passes a literal, non-quarantined, non-failed expected state:

  • Checkpointing, Stopping, Resuming ×2 and Pausing through mark_quarantined_best_effort;
  • Pausing in service/recovery.rs;
  • Suspending in mark_suspending_quarantined.

  So starts_new_episode is always true in production. The following arms are reachable only from tests:

  • the expected_state: None path;
  • UnknownCurrentState;
  • the Transition error for a failed row;
  • the whole reconfirmation branch (!starts_new_episode, ~864–1001).

  These tests pin them:

  • quarantine_recovery_reason_change_resets_episode_retry_state;
  • quarantine_recovery_source_change_resets_episode_retry_state;
  • quarantine_recovery_reason_change_preserves_a_live_claim;
  • mark_quarantined_reports_unknown_current_state;
  • the reconfirm half of mark_quarantined_persists_current_state_and_history;
  • malformed_expiry_cannot_clear_ownership_when_quarantine_is_reclassified (db/timestamps/tests.rs ~238).

  A follow-up must decide whether to delete the reconfirmation branch or name the production path meant to reconfirm. It is left here because sbx-mgr: keep quarantined rows through inventory and bound recovery that can never succeed #270 edits db/tests.rs, where most of those tests live.

• rollback_activation_state (db/state_transitions.rs ~1216–1234) has one production caller, restore_resident_resume_state, which passes expected = Resuming after checking for Started or Paused itself. So its restore_* = CASE WHEN ?4 = 'restoring' arms are dead in the same way as F-d, and its Quarantined | Failed guard duplicates the caller's check. Follow-up: retarget activation_entry_rollbacks_and_quarantines_do_not_overwrite_newer_state and generic_lifecycle_writers_reject_failed_targets to the _episode writers, then replace the CASE with = NULL or delete the wrappers.

• Typing cleanup admission's target state. With the else gone, the rule that the target is Stopping or Deleting is enforced only by the debug_assert! in update_state_superseding_activation_in_transaction (~310). A two-variant enum for CleanupAdmissionSpec.target_state, ObservedCleanupClaim.target_state and the admit_lifecycle_cleanup_for_observed_execution parameter would make the type system enforce it.

  • It is left for a follow-up because it isn't mechanical-small here. It also changes grpc_lifecycle/kill.rs and grpc_spaced_process_snapshots.rs (which sbx-mgr: inject the node-manager client transport and template-manager resolver #296 edits), and six direct call sites plus the test helper in service/tests.rs (where sbx-mgr: build every test service from explicit test options #322 has hunks around ~10754 and ~10875). Changing the helper's signature would also touch its nine callers in three test files.
  • No runtime fallback is reintroduced.

• Deferred for sbx-mgr: keep quarantined rows through inventory and bound recovery that can never succeed #270. None of the inventory's ten items is in sbx-mgr: keep quarantined rows through inventory and bound recovery that can never succeed #270's hunks, so nothing is deferred.

• No item turned out reachable.

Host safety

This adds no tests and changes no test bodies. The throwaway probes ran in a scratch copy, inside the CI image, as an unprivileged uid with --cap-drop=ALL and no host network. They used task-owned Postgres and Redis containers and touched no fixed host path, socket, firewall, netns or foreign process. The one privileged sbx-mgr test (transfer_directory::tests::distinct_service_uids_can_exchange_downloads) ran as root in a --privileged container, as CI's tests-privileged step does.

Overlap with open PRs

• sbx-mgr: keep quarantined rows through inventory and bound recovery that can never succeed #270 (ac/fix-sbxmgr-inventory-keeps-quarantined-rows, head cc062ce797): no shared files. Its diff applies cleanly on this branch.
• sbx-mgr: inject the node-manager client transport and template-manager resolver #296 (ac/sbxmgr-seam-node-manager-transport, rebased, head 5547397a26): both edit service/cleanup.rs, in disjoint hunks (sbx-mgr: inject the node-manager client transport and template-manager resolver #296: lines 563 and 2204–2788; this PR: 1226–1300). Its diff applies cleanly on this branch.
• sbx-mgr: build every test service from explicit test options #322 (ac/sbxmgr-explicit-test-service, stacked on sbx-mgr: inject the node-manager client transport and template-manager resolver #296, head 2b8049a7d0): its diff shows the same five service/cleanup.rs hunks as sbx-mgr: inject the node-manager client transport and template-manager resolver #296, carried from the stack, and is otherwise disjoint from this PR. It applies cleanly on this branch. sbx-mgr: inject the template-manager resolver #299 has merged into sbx-mgr: inject the node-manager client transport and template-manager resolver #296's branch.
• sbx-mgr: test create after template lookup, unpersisted cleanup and endpoint capture on the seams (tests only) #327 (ac/sbxmgr-deferred-tests-on-seams, stacked on sbx-mgr: build every test service from explicit test options #322, head f93fc01e3b): touches none of this PR's four files.
• node-mgr: validate LoadSnapshot requests before staging host state #269, sbx-mgr: characterize stop, delete and create core RPC paths (tests only) #271, api: characterize Kubernetes substrate suspend, pooled preparation and restore hydration (tests only) #273, docs: localized AGENTS.md for sbx-mgr, node-mgr and the api crate #275, api: release a Kubernetes create's claimed execution on every early exit #277 and tests: keep three tests off host state they don't own #278 are merged.

Validation

All Rust commands ran in the repository's CI image (ci/Dockerfile.ci, toolchain 1.98.0) against this branch's tree, as an unprivileged uid with --cap-drop=ALL. Postgres and Redis came from ci/scripts/ci-test-services.sh, restarted fresh for each run, with SPACE_CI_REQUIRE_TEST_DEPS=1.

Check	Result
cargo fmt --all -- --check	pass
cargo clippy --locked --workspace --all-targets --all-features -- -D warnings	pass
cargo clippy --locked -p pplx-space-sbx-mgr --all-targets --all-features -- -D warnings	pass
ci/scripts/run-rust-checks.sh fmt (env-contract check, Rust error policy, fmt)	pass
scripts/source_guards/env_test_hook_boundary.py, observability_vendor_boundary.py	pass
scripts/check_no_inline_rust_test_modules.py, scripts/source_guards/thin_service_mains.py	fail, pre-existing: they flag only crates/libs/observability and crates/apps/vol/mgr/svc/src/main.rs, which this change does not touch, and thin_service_mains.py prints identical output on main
pre-commit (gitleaks) over 2d9e1d3..HEAD	pass
cargo nextest run --locked -p pplx-space-sbx-mgr --retries 0 -E 'not group(privileged)', three consecutive runs (after one earlier identical green run)	1568/1568 passed each time; no SPACE-TEST-DEP-SKIP recorded. sbx-mgr's only ignored test is the privileged one below
cargo nextest run --locked -p pplx-space-sbx-mgr --retries 0 --run-ignored all -E 'group(privileged)', as root in a --privileged container (CI's tests-privileged shape), three runs	1/1 passed each time (transfer_directory::tests::distinct_service_uids_can_exchange_downloads)
Probes on main: both removed branches made to panic, then the full sbx-mgr suite	1568/1568 passed. The F-d panic does fire for the raw-SQL hypothetical, so the probe was live
Kind: fresh space-local cluster (none existed beforehand), bring-up through scripts/tests/smoke-kind-e2e.sh with Tilt and SPACE_KIND_TELEMETRY_MODE=otel	pass, 7/7 steps
Deployed binaries come from this build	the incremental build's source copy of crates/ and Cargo.lock is byte-identical to this branch. All four sandbox-manager pods (two workers, two builders) run space-sbxmgr-svc sha256 bbae248b…, identical to the artifact build. The binary lacks the removed SQL literal (CASE WHEN ?6 THEN restore_origin) and contains an unchanged control literal
Tilt quiet before the typed smoke	42 ok and 4 not applicable, nothing pending; still the same pod binary hashes after the run
SKIP_BOOTSTRAP=1 scripts/tests/smoke-kind-typed.sh all with SPACE_SMOKE_KIND_ADMIN_TOKEN_FILE and SPACE_SMOKE_KIND_COLD_RECEIPTS set as CI does	ECW: 77 passed, 0 failed, 0 inconclusive. Native: 23 passed, 0 failed, 0 inconclusive. native_contract.command_replay_credentials is declared ignored in the scenario inventory and was withheld. ecw.cold_create_environment and local.encrypted_resuspend_preserves_cow ran and passed

No reruns were needed, so no failure classification applies.

AI disclosure: an AI coding agent (Claude) wrote this change, ran the probes and validation, and drafted this description.

🤖 Generated with Claude Code

[the-gigi-pplx]

Review — 2 findings

Low

1. crates/libs/sbx/mgr/server/src/service/cleanup.rs:1229 — once the else is gone, the only guard on target_state is a debug_assert!, and release builds compile it out.
   update_state_superseding_activation_in_transaction (db/state_transitions.rs:305) deletes the sandbox's cleanup episode and NULLs cleanup_completion / cleanup_keep_filesystem before it calls update_state. The removed branch did a plain update_state and left the episode in place. So if a future constructor hands this path a third state, a release build would silently drop an armed episode instead of writing only the state. Its DB errors would also land in the generic Err(error) arm and come back as CleanupAdmission::unpersisted (unless require_atomic_admission), where the old branch returned Status::internal. None of this is reachable today: I traced all four CleanupAdmissionSpec constructors and both production callers (grpc_spaced_process_snapshots.rs:1530, grpc_lifecycle/kill.rs:64), and every one passes Stopping or Deleting. Until the two-variant enum you've deferred lands, a cheap stopgap is a release-mode check at the call site that returns Status::internal for any other target.
2. crates/libs/sbx/mgr/server/src/db/state_transitions.rs:286 — the doc comment that states the stop/delete supersede contract now sits on the #[cfg(test)] wrapper, and the production entry point update_state_superseding_activation_in_transaction has none.
   Since this function's debug_assert! is now the only place the invariant lives (see 1), moving the doc onto the pub(crate) function would keep the contract next to the code production actually calls.

I traced the two unreachability proofs myself and both hold. adopt_quarantine_cleanup_episode (db/quarantine_recovery.rs:1275) is the only writer of the recovery fence columns, and it inserts phase = 'requested'. request_cleanup_episode returns before the UPDATE for a requested episode (db/cleanup_episodes.rs:469). Outside tests, the only 'armed' write is insert_cleanup_episode, which sets no fence. The rollback_activation_state CASE arms and the mark_quarantined_inner reconfirmation branch are already listed in your follow-ups, so I haven't repeated them here.

Verified against 1a5f1017196d7e24ee8ff71fc70d437103c7e8ab. Reproduced locally: I traced callers and writers in an isolated checkout of the head and relied on CI (all checks green) for build and test. I didn't re-run the suite or the panic probes.

[the-gigi-pplx]

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