Repository navigation
sbx-mgr: callers use the store's typed reads - #721
Merged
Merged
Conversation
ac-pplx
force-pushed
the
ac/sbxmgr-store-read-sweep
branch
from
October 20, 2026 18:37
4487917 to
a47dc2e
Compare
ac-pplx
marked this pull request as ready for review
October 20, 2026 18:38
ac-pplx
force-pushed
the
ac/sbxmgr-store-read-sweep
branch
from
October 20, 2026 19:47
a47dc2e to
3765c14
Compare
ac-pplx
force-pushed
the
ac/sbxmgr-store-read-sweep
branch
from
October 20, 2026 21:11
3765c14 to
7f71fe7
Compare
ac-pplx
force-pushed
the
ac/sbxmgr-store-read-sweep
branch
from
October 20, 2026 22:16
7f71fe7 to
2abde92
Compare
ac-pplx
force-pushed
the
ac/sbxmgr-store-read-sweep
branch
from
October 21, 2026 04:38
2abde92 to
edcce53
Compare
nickdujay-pplx
approved these changes
Oct 21, 2026
nickdujay-pplx
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed against #715, including cleanup and teardown fencing, recovery cursors, State RPC publication, and the production visibility boundary. SQL and persisted formats are unchanged, and the migrated callers retain their locking and read strictness.
Validation: 384 store tests and 1,407 sbx-mgr tests passed; the production library also compiles without test-only store visibility. Approving this as the second PR in the stack, after #715.
ac-pplx
added this pull request to stack #780
October 21, 2026 14:03
github-merge-queue
Bot
removed this pull request from the merge queue due to a conflict with the base branch
Oct 21, 2026
Convert every production caller of the readers that `db::read` and `db::scan` consolidate, and make those readers non-public outside the store. Exact-equivalent conversions only: each call site now calls the accessor that wraps the reader it called before, so it runs the same statements with the same decoder and missing-row rule. - Strict call sites stay strict: every former `get_sandbox` caller reads `read::sandbox`, including the cleanup admission checks, teardown suspend, pool clone, network runtime, pause and wake lookups that fail closed on a corrupt row. - Narrow projections stay narrow: `node_execution_fence` keeps the previous execution (`sandbox_view(..).node_execution_fence()`), the eviction CAS keeps the raw `state_changed_at` witness (`read::state_changed_at_witness`), and the snapshot-upload reclaim keeps the one-statement suspended-with-pin `EXISTS`. Its two call sites now say why it is a fence: the reclaim holds the connection guard across the unlink, and the earlier due check is only a hint because that guard is released before node-manager is asked. - Receipt reads go through `typed_sandbox_view`, so the outbox is read under the caller's `SandboxId`, as before. The recovery claim check opens its view on the sandbox the claim names. - The State RPC reads its batch through `read::state_page` and rebuilds the same response: state-only rows first, then decoded rows, with the same status codes and messages. One difference: the state-only rows' metric and two warnings are skipped if any later read in the same batch fails (a later row's budget, ambiguity or raw-state read, or the publication, state-change or recovery reads). Batches that finished earlier in the RPC still emit; responses, row order, status codes and messages are unchanged. - The cleanup-obligation loop keeps a `CleanupObligationScan` and only advances it when a page is read, as it kept `recovery_cursor` before. - The test-only `recover_prelaunch_startup_states_in` reads like its production siblings, and `sandbox_eviction/tests.rs` imports the two store counters it calls instead of reaching them through its parent. 79 readers (74 free functions and 5 cursor methods) leave the public API outside the `test-support` feature: `db_private_outside_tests!`, now importable within `db`, declares each as `pub(in crate::db)` in production and `pub` for sbx-mgr's tests, which still compare against them unchanged. That is the narrowest visibility that compiles: the consolidated reads in `db::read` and `db::scan` call every one of them. String literals in every changed store file are byte-identical to the base; only the code around them is re-indented inside the wrappers (review with `git diff -w`). The store's AGENTS.md now says to extend `db::read`/`db::scan` before adding a public reader.
ac-pplx
force-pushed
the
ac/sbxmgr-store-read-sweep
branch
from
October 21, 2026 14:57
edcce53 to
4061f30
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follows #715, now on
main. Converts every production caller of the readers thatdb::readanddb::scanconsolidate, and makes those readers non-public outside the store. Exact-equivalent conversions only: each site now calls the accessor that wraps the reader it called before, so it runs the same statements with the same decoder and missing-row rule. Review the store half withgit diff -w: it is wrappers around unchanged functions.db_private_outside_tests!(db/handle.rs) is now importable withindb. Each replaced reader is declared through it:pub(in crate::db)in production,pubundertest-support. 79 readers left the public API (74 free functions, 5 cursor methods).pub(in crate::db)is the narrowest that compiles, since the reads indb::read/db::scancall each one.teardown_suspend_retry_{due,admissible}lose their productionpub usethe same way.#[cfg(test)]helperrecover_prelaunch_startup_states_in, which now readsscan::all_sandboxes(SkipUndecodableRows)like its production siblings), converted mechanically from compiler-reported locations by parsing each call's arguments, then reviewed by hand.get_sandboxcaller readsread::sandbox, including the fail-closed sites insvc/cleanup.rs(admission checks),gl/teardown_suspend.rs,svc/grpc_pool_wait_clone.rs,svc/network_runtime.rs,gl/pause_resume.rs, andsvc/activation.rs(vialookup_sandbox, which usesread::sandboxandscan::resolve_selector(RpcSelector)).sandbox_view(..).node_execution_fence()(with previous);read::state_changed_at_witness(raw CAS witness,svc/eviction.rs);sandbox_view(..).suspended_with_pinned_artifact(..)(oneEXISTS). Atsrv/snapshot_upload_jobs.rsthe two sites now say why:reclaim_streamis a fence because it holds the connection guard across the unlink;stream_reclaim_dueis only a hint, because the guard is released before node-manager is asked and the reclaim rechecks.pending,pending_matches_observed_execution) usetyped_sandbox_view(SandboxId), so the outbox is read under the caller'sSandboxIdexactly as before; #715 makes them unreachable from a text view. The recovery claim check opens its view onclaimed.candidate.sandbox_id, the sandbox the claim names.state_batchreads throughread::state_pageand rebuilds the same response (state-only rows first, then decoded rows), status codes and messages unchanged, including #620'scurrent_local_artifact_created_at, which now comes fromstate_page's pinned capture-time read.CleanupObligationScanand advances a copy, keeping the original when the read fails, as it keptrecovery_cursor.sandbox_eviction/tests.rsnow imports the two store counters it calls by bare name, which it used to get through the parent module.AGENTS.md: code outside the store reads throughdb::read/db::scan; extend those before adding a public reader.Behavior difference (deliberate, telemetry-only)
Equivalence evidence
"test-support"cfg strings aside). In the 42 server files the only literal change is a renamed format variable ({state_error}→{error}) in one identical message.cargo arch(advisory rules only; no cap raised)Code-line deltas vs #715's head in baseline-listed files, mostly from rustfmt splitting longer call chains:
service/grpc_state_snapshot.rs+38,service/cleanup.rs+27,grpc_lifecycle/teardown_suspend.rs+23,recovery_cross_node.rs+13,grpc_core.rs+11,recovery.rs+11,start.rs+5, smaller elsewhere,mod.rs−1,snapshot_upload_jobs.rs−4. In the store, the two-line wrappers addsandboxes.rs+40,cleanup_episodes.rs+16,auto_lifecycle_delete_receipts.rs+11 andquarantine_recovery.rs+6. Five files that were within their caps on #715 now exceed them:sandbox_eviction.rs,auth_lookup.rs,cache_cleanup.rs,cleanup_episodes.rs,sandboxes.rs. Every other over-cap entry, now includingactivation_support.rs(over since #543), was already over onmain. No file shrank below its cap, so no cap moves.Validation
Rebased onto main after #715 merged and #619; converted
grpc_lifecycle/artifact_backup.rscapture_execution_generation(#619's new teardown-generation lookup):latest_teardown_suspend_operation→read::sandbox_view(..).teardown().latest(), same connection guard; #619's other new reads of narrowed readers are in server tests, which keep them.Rebased onto main after #717 (and #543/#700); no conflicts, and the patch is unchanged (
git range-diffreports it identical). Every call #543 or #700 adds to one of the 79 readers is in a server test (start/tests.rs,service/tests.rs,recovery_cross_node/tests.rs), which keeps the old readers like the other server tests, so no new site needed converting. The production clippy passes confirm no non-test code reaches a narrowed reader.Rebased onto main after #620;
grpc_state_snapshot.rsandlocal_artifacts.rsconflicted. The State RPC takes #620's pinned capture time fromstate_page(added in #715) and maps its error to #620's status message, andlocal_artifacts.rskeeps #620's new publicpinned_local_artifact_created_atbeside the wrapped readers. #620's other production store calls are to its own new functions, so no new site needed converting. #620's State testssandbox_state_publishes_the_pinned_artifact_capture_timeandan_unmarked_pin_reports_no_capture_time_even_when_its_times_read_currentpin the field and pass unchanged.Rebased onto #715 after #714 and #718 (and #734/#701) reached main; no conflicts, and
git range-diffshows the patch unchanged apart from context. No new site needed converting: #718's rewrittendelete_named_snapshot,DeleteLease, DeleteSnapshots, retention and GC code, #714's network-policy and quarantine changes and #734's revert call none of the narrowed readers (the production clippy passes confirm it), so main's code there is untouched, and this PR's existing conversions in those files apply as before. No narrowed reader was deleted: #714's and #718's store removals are writers, andget_local_artifactandget_named_local_artifactstay narrowed. #714's newhas_live_network_policy_gatewayis not one of the consolidated readers and stays public. #714's and #718's tests pass unchanged.bash ci/scripts/run-rust-checks.sh fmtbash ci/scripts/run-rust-checks.sh clippy(workspace + sbx-mgr production checks withouttest-support)cargo archSPACE_CI_REQUIRE_TEST_DEPS=1 cargo nextest run --locked --all-features -p pplx-space-sbx-mgr -p pplx-space-sbx-mgr-store -p pplx-space-db-migrate -p pplx-space-db-migrator -p pplx-space-db-test-fixturestransfer_directory::tests::distinct_service_uids_can_exchange_downloads, ignored, asserts a root runner; untouchedRUSTDOCFLAGS=-Dwarnings cargo doc --locked --no-deps -p pplx-space-sbx-mgr-storepinned_local_artifact_created_atlinks the privateinsert_local_artifact; none from this PR🤖 Generated with Claude Code