[ac-pplx]

Based on main: #296 (with #299) has merged. #327 is stacked on this PR. Tests only: the release space-sbxmgr-svc binary's machine code (.text) is identical to main's; the edits to production files are cfg(test) fields, a pub(crate) constant and the removal of a constructor production never calls (see Validation).

What merging does

Every sandbox-manager test service is built from one cfg(test) options builder instead of SandboxManagerServiceOptions::default().

Default is production wiring. It reads SPACE_NODE_ID or /space/etc/node-id, names the production host-control and credential-manager sockets, probes fc.pid under the fixed sandbox run directory, and dials the host's node-manager and template-manager. Until now, about two hundred test call sites built the service from it.

SandboxManagerServiceOptions::for_test_in(dir) (new service/test_options.rs, wired by #[cfg(test)] pub(crate) mod test_options;) resolves every host dependency under a test-owned directory:

Field	Production Default	for_test_in(dir)
local_node_id	SPACE_NODE_ID or /space/etc/node-id	fixed test_options::test_node_id()
sandbox_runtime_live	fc.pid under /space/run/firecracker/sandboxes	dir/<id>/fc.pid
cleanup_run_dir (new, cfg(test))	absent in production; stop/delete target selection reads fc.pid under the fixed run directory	dir, so target selection reads the same dir/<id>/fc.pid
credential_manager_socket_path	production credmgr socket (dialed)	dir/absent-credential-manager.sock, which nothing binds
host_control_socket_path	production host-control socket; sandbox-manager never dials it, only passes it through to node-manager's Launch	dir/absent-host-control.sock, which names no live service
node_manager	NodeManagerConnector::host()	node_manager_transport::fake::unbound(dir/absent-node-manager.sock)
template_resolver	HostTemplateManager	template_resolver::fake::unbound(dir/absent-template-manager.sock)
shutdown_token, task_tracker, node_role, node_managed_by, route bundle	fresh / None	same

for_test() uses a fresh directory under the temp dir that nothing creates. SandboxManagerService::for_test(conn) builds a service from it. Tests override the fields they exercise with struct update syntax (..SandboxManagerServiceOptions::for_test()).

Both unbound fakes dial their absent socket once and fail as the host clients do when the service is down, without the retry wait. node-manager's connect()/connect_once() return a ConnectError, and lazy clients fail at first use. A dialed template lookup fails with TemplateLookupError::Connect, which create reports as base's Internal "cannot connect to template-manager: …". A shared-channel lookup fails with Unavailable.

Both liveness probes

Stop and delete target selection reads liveness through a cfg(test) branch of its own, which defaulted to the fixed run directory. Eight tests redirected it by hand through the cleanup executor's run_dir hook. The builder now sets a cfg(test) cleanup_run_dir option, carried on a cfg(test) service field, so both probes read the test's directory. The hook and the eight hand-set copies are removed. cancelled_cleanup_rpc_tests, which marks a runtime live, builds its service with for_test_in on that directory.

Production wiring is unchanged

• impl Default for SandboxManagerServiceOptions, new_with_options, new_with_config and run.rs behave as before. The new options field and service field exist only in test builds.
• SandboxManagerService::new, which production never called, is removed, so a test cannot fall back to the host defaults through it. With Default temporarily hidden from test builds (#[cfg(not(test))], reverted), the only remaining user is run.rs:852, the production run() wiring, which no test calls.
• Release space-sbxmgr-svc, main (9214edf) vs this head: .text is identical with default features (a57348a2…) and with the Kind feature set (otel, datadog, test-hooks; 8022cd2f…), and the file sizes are equal. The remaining bytes (73 in .rodata; 240 in .data.rel.ro, 243 with the Kind set; the build id) are source line numbers: file.rs:line strings and panic Location line fields. One-edit-at-a-time variants of the base confirmed this: a #[cfg(not(test))] line, the removal of new and the cfg(test) fields each left .text unchanged. An earlier draft that reordered the executor's construction did change .text, and was replaced.

Call-site inventory (test files under crates/libs/sbx/mgr/server/src)

Construction	main	This PR
SandboxManagerService::new(conn)	182	0
..Default::default() in a SandboxManagerServiceOptions literal	14	0
new_with_config(conn, Default::default(), ..)	3	0
SandboxManagerServiceOptions::default()	2	0
options literal naming NodeManagerConnector::host() / HostTemplateManager	2 / 2	0 / 0
subsystem test contexts naming NodeManagerConnector::host(): GC, upload jobs, scheduler, watchdog and restore test files (10), plus the cfg(test) SandboxWatchdog::new_for_tests (2)	12	0
hand-set cleanup_executor.run_dir	8	0
SandboxManagerService::for_test(conn)	0	183 (the 182 former new calls, plus service_for_teardown_test)
..SandboxManagerServiceOptions::for_test() / for_test_in(dir)	0	24 (+1 in test_options.rs)

Helpers:

• service_for_teardown_test (118 calls) and service_with_runtime_probe keep their names and delegate to the builder.
• service_with_runtime_liveness (65 calls on main, three of them from sbx-mgr: keep quarantined rows through inventory and bound recovery that can never succeed #270) and runtime_liveness_options folded into service_with_runtime_probe. They used the host node-manager and template-manager.
• service_with_runtime_liveness_and_absent_sockets now uses for_test_in(sockets).
• grpc_core/tests.rs's isolated_service keeps its signature and becomes node_role over for_test_in(scratch).
• node_manager_transport_tests's isolated_service is folded into the builder. service_with_node_manager(conn, connector) remains as a one-field override, and create_template_lookup_tests sets its fakes directly.
• route_identity_tests no longer names the host node-manager and template-manager.

Host paths each test touches, main vs this PR

Every test of the package was run alone under strace -f -e trace=%file,connect. The table counts the tests that name each fixed /space path.

Fixed host path	main	This PR
/space/run/node-manager/node-manager.sock	25	0
/space/run/template-manager/template-manager.sock	1	0
/space/etc/node-id	416	13
credential-manager and host-control sockets	0	0

The unbound node-manager fake dials like a real client

fake::unbound used to return a lazy client from connect(), so a dial "succeeded" and the first RPC failed with Unavailable. It now fails at connect(), as a real dial does.

This is the reason three tests pass, on main through the host connector and now through the builder:

• checkpoint_retry_keeps_retirement_for_dangling_pin
• checkpoint_start_retires_existing_restoring_runtime_before_retry
• restore_routes_provisional_suspended_retirement_through_cleanup_before_consumption

Each asserts that a restore's runtime retirement leaves the row CleanupRequired. That holds because node-manager cannot be dialed, so the retirement's cleanup never starts. Each test now says so in a comment.

On main the default template resolver is HostTemplateManager, which dials the fixed template-manager socket. The test default dials an absent socket under the test directory instead, and fails to connect the same way. A file marker on both of its methods, over the full suite, showed one test reaching it: failed_paused_wake_does_not_adopt_the_next_activation_attempt, through the shared channel, which is Unavailable on main as well. It passes unchanged.

Follow-ups folded in

• grpc_core/tests.rs (from sbx-mgr: characterize stop, delete and create core RPC paths (tests only) #271's review):
  • The forwarded-create test drops a !sandbox_exists assert that held by construction, along with the same assert in create_template_lookup_tests. The create arm of core_rpcs_refuse_unauthenticated_and_unlisted_peers_without_mutation keeps its assert.
  • Where one status code comes from two production branches, the case also matches the message: the auto-lifecycle delete's two Internal guards, and the pool-clone delete's two FailedPrecondition guards.
  • node_state_sync::SKIP_NODE_PUBLISH_METADATA is pub(crate), and the skip-publish test uses it.
  • The completion dirty-ref wait sleeps 5 ms between checks inside its 30 s bound.
  • BARRIER_AUTHORITY's doc says that a lookup past the barrier stops at the test service's refusing template-manager fake.
• From tests: keep three tests off host state they don't own #278's review: the builder's doc says the host-control socket is passed through to node-manager's Launch, not dialed.
• From sbx-mgr: inject the template-manager resolver #299: the node_manager_transport_tests module doc lists the template resolver.
• Stale comments in start/tests.rs and grpc_core/tests.rs now say every test service's template-manager fails to connect.
• The sbx-mgr AGENTS.md test guidance points at the builder.

Mutation proofs

Each mutation was a scratch edit, reverted with git checkout after the run. Every run was under strace -f, and none wrote under /space or connected to a /space socket.

Mutation	Tests run	Result
Auto-lifecycle delete: a template filter falls into the canonical-id guard (same Internal code)	auto_lifecycle_delete_requires_one_canonical_id_and_the_expected_policy	fails on the message. main's test file passes it.
Pool-clone delete: a quarantined clone's template-manager/auto-lifecycle delete falls into the "publication still owns" guard (same FailedPrecondition code)	delete_of_a_pool_clone_disposal_requires_an_operator_after_quarantine	fails on the message. main's test file passes it.
Seam misconfigured: unbound connect() succeeds lazily (the old fake)	the three checkpoint-restore tests	all 3 fail (CleanupFailed ≠ CleanupRequired)
start.rs: skip routing a pending MarkSuspended retirement through cleanup	the three tests	restore_routes_provisional_… fails
start.rs: change the ambiguous-restoring retirement reason	the three tests	checkpoint_start_retires_… fails
Builder leaves cleanup_run_dir unset (target selection reads the fixed run directory)	grpc_core cleanup tests, cancelled_cleanup_rpc_tests, #327's unpersisted test	5 fail
Target selection ignores cleanup_run_dir	same	5 fail
Control: no edit	each set above	pass

Call sites left unrouted

These need production seams this PR does not add.

• activation_support.rs:201 sets identity.current_node_id = local_space_node_id() directly. It is a free function that re-reads the identity on each activation, and routing it through the service's local_node_id would change production to read it once at startup. These 13 tests still read /space/etc/node-id:
  • service::start::tests::restore_artifact_as_new_* (7);
  • service_tests::base_snapshot_create_wake_accepts_the_open_activation_identity;
  • service_tests::no_op_wake_leaves_a_locally_minted_provisional_attempt_inert;
  • service_tests::restore_artifact_as_new_start_reattaches_existing_started_runtime;
  • service_tests::runtime_activation_identity_does_not_promote_a_locally_minted_kubernetes_attempt;
  • service_tests::runtime_activation_identity_preserves_explicit_kubernetes_substrate_without_pod_uid;
  • service_tests::wake_start_requests_preserve_claim_ineligible_wire_identity.
• Fixed-socket dials left (all fail with ENOENT on fresh-UUID paths):
  • the guest's spaced-host socket, in activation_support::tests::restore_finalizer_retains_cleanup_until_state_persistence_succeeds, …restore_defers_guest_readiness_only_while_paused and …activation_env_failure_keeps_setup_context_for_callers_and_cleanup, and in service_tests::start_readiness_failure_does_not_overwrite_concurrent_delete;
  • the net-gateway control socket, in service_tests::resume_fails_closed_when_network_policy_cannot_confirm, service_tests::pre_resume_network_policy_apply_fails_closed_without_gateway and activation_support::tests::restore_completion_requires_gateway_readiness_only_when_starting.
• Other fixed run-directory probes (watchdog.rs pid files, grpc_state_snapshot.rs:821) and fixed CACHE_DIR stats of fresh-UUID paths.
• Service construction still reads process environment, such as the feature-flag provider settings. Tests that depend on it use enter_route_test_process.

Host safety

• Every test service now gets its node id, both liveness probes, sockets, node-manager and template-manager from test-owned values. Every subsystem test context gets an unbound node-manager, including the watchdog's cfg(test) constructor, whose loops tick immediately. The three snapshot-GC tests that sent DeleteCacheArtifact to the host node-manager's socket on main now dial an absent socket.

• Pre-existing, unchanged by this PR: seven tests reach restore staging, which calls create_dir_all on /space/cache/firecracker/sandboxes/.<id>.restore-<uuid> under the fixed cache root:

  • six service::recovery::tests orphan-restore tests;
  • checkpoint_retry_preserves_row_for_resumable_restore.

  None of them is in the privileged nextest group, so on main's CI they run in the unprivileged step, where create_dir_all fails with EACCES at /space. As root they would create those directories and then dial the fixed volume-manager socket (/space/run/volume-manager/volume-manager.sock, snapshot_restore.rs:2169, cleanup.rs:550). Unprivileged, checkpoint_retry_preserves_row_for_resumable_restore's asserted Unavailable comes from the failed create_dir_all; as root it would come from that volume-manager connect.

• Follow-up: a cache-root and volume-manager seam for those seven tests.

Overlap with open PRs and merge order

• sbx-mgr: keep quarantined rows through inventory and bound recovery that can never succeed #270 merged first, and this PR was rebased onto it. The rebase resolved one conflict, in service/tests.rs: sbx-mgr: keep quarantined rows through inventory and bound recovery that can never succeed #270's quarantine block in evict_cluster_deleted_sandbox_reclaims_retained_restoring_and_quarantined_rows sits beside the renamed service_with_runtime_liveness(conn, false). The rebase also converted sbx-mgr: keep quarantined rows through inventory and bound recovery that can never succeed #270's three new service_with_runtime_liveness calls to service_with_runtime_probe: two in cache_cleanup/periodic/tests.rs, and recovery/tests.rs's bound_test_service. Their assertions are unchanged. sbx-mgr: keep quarantined rows through inventory and bound recovery that can never succeed #270 as merged adds no SandboxManagerService::new call.
• Any PR that lands after this one converts its leftover calls: service_with_runtime_liveness → service_with_runtime_probe, SandboxManagerService::new → SandboxManagerService::for_test, and a hand-set cleanup_executor.run_dir → for_test_in(dir). With this PR, leftover calls fail to compile instead of silently using host defaults.
• [backup] refuse teardown-suspend backups from retired executions and release their runtimes #331 adds SandboxManagerService::new (2), service_with_runtime_liveness (1) and cleanup_executor.run_dir (1). It no longer applies to main either.
• internal-api: scope snapshot location reads to the calling node's placement #341 applies to this head as it does to main, and adds one SandboxManagerService::new call.
• Retry failed generation-suspend backups behind an admission gate #349 applies to main but not to this head: its snapshot_upload_jobs/tests.rs hunk overlaps this PR's edit there. It also adds one SandboxManagerService::new call.
• sbx-mgr: recover an encrypted suspend capture whose volume did not seal #364 (stacked on another branch) adds one service_with_runtime_liveness call.
• fix(sandbox): wait before retrying failed activations #3 adds one SandboxManagerService::new call. It already fails to apply on main.
• Every other open PR touching crates/libs/sbx/mgr/server applies to this head exactly as it does to main: Defer btrfs-send capture by default in sandbox-manager #287, Validate encrypted backup containers at admission #344, Observability wave 2a: correct sparse Kind counters, classify build failures, add bounded outcome labels #348 and Observability wave 2b: request-scoped traces, canonical sandbox_id, request-to-trace correlation #354 apply; sbx-mgr: resume a teardown suspend's unfinished publication through recovery #366, PoC: claim BYOK warm sandboxes by switching Firecracker drives #336, Approve bounded publication of validated retained suspends #283, [api] stop retrying restore pods whose snapshot the host CPU cannot load #221, Preserve authenticated scope in cached template lookups #217, Add an opt-in Firecracker balloon device per template #212, [sbx-mgr] record the capture host CPU on snapshots and publish it as a node label #203, sbx-mgr: keep teardown polls pending while a delete is in flight #201, [api] restore full snapshots only onto CPU-compatible hosts, behind a default-off gate #176 and Quarantine sandboxes after sustained guest-agent probe failure #110 fail on main as well.

Validation

Check	Result
cargo fmt --all --check	pass
cargo clippy --workspace --locked --all-targets --all-features -- -D warnings	pass
Gitleaks pre-commit hook, public-safety review of the staged diff	pass, no findings
cargo nextest run --locked -p pplx-space-sbx-mgr --retries 0, SPACE_CI_REQUIRE_TEST_DEPS=1, 3 consecutive runs	3/3: 1583 passed, 1 skipped (an existing #[ignore] test), 0 dependency skips. The Postgres setup script ran in each run.
Release space-sbxmgr-svc binary vs main	.text identical (default and Kind feature sets)
GitHub CI at dbc7b0b	13/13 checks pass (Pull CI run 38348874622, including Rust tests)
Kind	not run: the machine code of the deployed binary is unchanged

AI assistance: the code and this description were written with Claude Code.

🤖 Generated with Claude Code

[the-gigi-pplx]

Review — 5 findings

Medium

1. crates/libs/sbx/mgr/server/src/service/grpc_core/create_template_lookup_tests.rs:171, crates/libs/sbx/mgr/server/src/service/grpc_core/tests.rs:281 — dropping the !sandbox_exists asserts removes the only guard that a refused create persists nothing.
   They hold "by construction" only because create writes its row long after these refusals. I made create insert a creating row for the requested id right after validate_create_request, ahead of the auto-lifecycle and network-policy authority checks. On this head create_checks_auto_lifecycle_authority_before_template_lookup and forwarded_create_refuses_owner_and_network_policy_authority_before_template_lookup both still pass. With the two asserts restored, both fail. I'd keep them. They cost one query each, and they're what turns "refuses before template lookup" into "refuses without side effects".

Low

2. crates/libs/sbx/mgr/server/src/service/tests.rs:8606 — service_with_runtime_probe (and service_with_runtime_liveness_and_absent_sockets at :12352) override only sandbox_runtime_live, so with true the service's probe says live while stop/delete target selection (cleanup_targets.rs:207) reads an empty cleanup_run_dir and says not running.
   No caller depends on this today. I pointed target selection at the injected probe, and the whole package still passes, which also suggests the cfg(test) probe could just reuse sandbox_runtime_live. But the helper's doc ("its runtime probe") and for_test_in's "both probes read the same path" invite the opposite assumption. I'd say in the helper docs that only the service probe is overridden.
3. PR body, "Host safety" — "Every test service now gets its node id … from test-owned values" overclaims.
   start.rs:212, snapshot_restore.rs:1402 and node_state_sync.rs:233 read SPACE_NODE_ID from the process environment rather than local_node_id. They're env-only (no host file), so this isn't a host-safety gap, but they belong in "Call sites left unrouted" next to activation_support.rs:201.
4. crates/libs/sbx/mgr/server/src/service/tests.rs:13581 — the comment still says a stale cleanup would spend "the connect retry budget against a socket that is not there."
   This service now comes from for_test, whose fake::unbound node-manager dials once with no retry wait (node_manager_transport/fake.rs:134, reached through connect_node → connect() at tail_support.rs:195). The PR fixed the matching comments in snapshot_gc/tests.rs and start/tests.rs. I'd fix or drop this one too.
5. crates/libs/sbx/mgr/server/src/node_manager_transport/fake.rs:146 — unbound_in_absent_dir repeats the temp_dir()/space-sbx-mgr-test-<uuid> recipe from test_options.rs:33.
   One shared helper would keep the two "nothing creates this" directories from drifting. Separately, nothing stops a new test from reaching for SandboxManagerServiceOptions::default() again, so the zero-count inventory in the body will drift. A clippy.toml disallowed-methods entry or a small grep guard would hold it if you want it held.

Verified against 7e6b46a3e44701df938380bec1a278d62476a66c. Reproduced locally: the full pplx-space-sbx-mgr nextest on this PR's code (via PR #327's head, which contains it: 1575 passed, 1 skipped), and the create-persists-before-refusal mutation with and without the dropped asserts. I also ran the probe-swap mutation (full package passes), and fake::unbound connecting lazily, which fails the three checkpoint/restore tests with CleanupFailed ≠ CleanupRequired, so their new comments name the right mechanism. I checked by reading that the channel wrapping in both unbound fakes is the same function the real clients use. I didn't redo the .text comparison or the strace inventory.

[the-gigi-pplx]

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