Repository navigation
sbx-mgr: route every SQLite statement through db/ and fence the rest with cargo arch - #561
Merged
Merged
Conversation
ac-pplx
force-pushed
the
ac/sbxmgr-store-1-sql-into-db
branch
from
October 16, 2026 05:59
1b0f941 to
08c09fe
Compare
ac-pplx
force-pushed
the
ac/sbxmgr-store-1-sql-into-db
branch
2 times, most recently
from
October 16, 2026 14:43
f3165c8 to
6e5769c
Compare
ac-pplx
added this pull request to stack #576
October 16, 2026 14:58
ac-pplx
marked this pull request as ready for review
October 16, 2026 19:18
mabdelraouf-pplx
approved these changes
Oct 19, 2026
Collaborator
Review — 3 findingsNo correctness findings. I compared the moved SQL text and the error, Low
Verified against |
the-gigi-pplx
approved these changes
Oct 19, 2026
the-gigi-pplx
left a comment
Collaborator
There was a problem hiding this comment.
Approved. 3 non-blocking findings in the review comment above.
ac-pplx
force-pushed
the
ac/sbxmgr-store-1-sql-into-db
branch
from
October 19, 2026 21:28
6e5769c to
d887c6a
Compare
…with cargo arch sbx-mgr's node-local SQLite database (`sandboxes` and every other sbx-mgr table) was queried from seven modules outside `src/db`, and `db/` had two-way dependencies with `auto_lifecycle_delete_receipts` and `route_activation_claims`. Every statement now lives in a named `db/` function, and new `cargo arch` ratchets count what is left and trip on the common ways of adding a crossing. They are line-based tripwires, not a guarantee: aliases, wrapped imports and calls through names imported earlier escape them. The compile-time boundary is the store crate that follows. Store moves, with callers keeping their transactions, locks and error mapping: - `auto_lifecycle_delete_receipts/db.rs` moves to `db/auto_lifecycle_delete_receipts.rs`, with its tests. The parent module keeps only the receipt types, so it is a leaf `db/` depends on. - The claim type and statements of `route_activation_claims` move to `db/route_activation_claims.rs`; the admission check that reads them stays outside and depends on `db/` one way. - `local_artifacts.rs`'s statements and row types move to `db/local_artifact_leases.rs`; `LocalArtifacts` and its leases keep their transactions, mutex and poisoning behaviour, and re-export the types. - `sandbox_eviction`'s scans and evidence reads move to `db/eviction_scan.rs`, the watchdog's to `db/guest_health_scan.rs`, and the snapshot-upload jobs' to `db/snapshot_upload_jobs.rs`. The marker backfill visits rows through the open cursor and samples its timestamp after binding the read, as before, so its writes interleave with the read in the same order. The watchdog's per-id state lookup gains a test. - Cross-node recovery reuses `db::current_state_label`. `db/` now depends only on the leaf type modules `state`, `auto_lifecycle`, `auto_lifecycle_delete_receipts`, `endpoint_proof` and `restore_operation`. archcheck's `pattern_ratchet` gains an optional, non-empty `paths` list that limits an instance to the files it matches, since other services' crates use rusqlite for their own databases. `[rules.pattern_ratchet.sbxmgr_sqlite]` covers `crates/libs/sbx/mgr/**` and `crates/apps/sbx/**` and allows only `src/db`. It matches rusqlite paths and imports, connection and transaction constructors, the shared SQLite open helpers by their qualified path, the shared job store's SQL functions, parameter macros, statement and transaction calls, and SQL statement literals. Its baseline is seeded with the 34 files that still name rusqlite types, open transactions or import the job store (203 lines); no SQL statement remains outside `db/`. `[rules.pattern_ratchet.sbxmgr_sqlite_schema]` matches the sbx-mgr migrations crate, the database's path, file name, migrator and migrations directory names, and the database migrator's default-path helper, in any crate outside the store, the migrations crate and the path's definition. Its baseline lists `run.rs` and the database migrator (8 lines).
ac-pplx
force-pushed
the
ac/sbxmgr-store-1-sql-into-db
branch
from
October 19, 2026 22:14
d887c6a to
16016bb
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
Milestone 1 of 3 toward a single owner for sbx-mgr's node-local SQLite database:
sandboxesand every other sbx-mgr table, not the control-plane Postgres. After this PR:crates/libs/sbx/mgr/server/src/db;cargo archratchets count what remains and trip on the common ways of adding a new crossing. They are lexical tripwires; the compile-time guarantee is milestone 3's.What moved
Callers keep their transactions, locks and error mapping; the statements and their row decoding move into named
db/functions.auto_lifecycle_delete_receipts/db.rs(whole file) and its testsdb/auto_lifecycle_delete_receipts.rs,db/auto_lifecycle_delete_receipts/tests.rsOpenRouteActivationClaimand its statementsdb/route_activation_claims.rsvalidate_cluster_fenced_activationand its error stay inroute_activation_claims.rsand depend ondb/one way.local_artifacts.rsstatements,PendingBackup,ArtifactState,NamedArtifactPublicationdb/local_artifact_leases.rsLocalArtifacts, both leases, their IMMEDIATE transactions and mutex poisoning stay put. The types are re-exported, so callers are unchanged. Identical statements (pin check,deletingrollback/delete) now share one function each; see "Hidden observable changes" for the pin check.sandbox_evictionscans, counts and evidence readsdb/eviction_scan.rssandbox_evictionkeep their names and map rows to the eviction types, so its tests still drive them.db/guest_health_scan.rsstate_labels_follow_the_ids_and_report_missing_rows_as_nonecovers it. Dropping.optional()(as.map(Some)) fails it withQueryReturnedNoRows.jobsstatements and the marker backfill'sdb/snapshot_upload_jobs.rsSELECT statedb::current_state_labelSNAPSHOT_UPLOAD_JOB_KIND(the constant)db/snapshot_upload_jobs.rsmain's retired-teardown release claim indb/eviction.rs(#331) cancels the refused capture's upload job by this kind.snapshot_upload_jobsre-exports it, so its callers are unchanged anddb/still depends only on leaf modules.Afterwards,
db/production code depends only on the leaf type modulesstate,auto_lifecycle,auto_lifecycle_delete_receipts,endpoint_proofandrestore_operation. Nothing it imports importsdb/.The fence: lexical tripwires and migration counters
These two
cargo archinstances are lexical tripwires and migration counters, not a guarantee. They match single lines of text, so they count the known crossings this stack removes and catch the common ways of adding one. They cannot see aliases, wrapped imports or calls through names imported earlier. The guarantee is milestone 3's compile-time boundary, where the store becomes its own crate:archcheck's
pattern_ratchetgains an optionalpaths = [...](globs of the same form asallow) that limits an instance to the files it matches.pathsis rejected (exit 2); an emptyallowstays legal.pathsis stale.pattern_ratchet_pathsand documented in the archcheck README.mainhas since gained node-manager: prevent signal-driven btrfs send restarts #591's optionalinclude, which also narrows an instance's files. Its edges differ: an emptyincludemeasures nothing (tested), and a baseline entry outside it gets the ordinary stale wording. Both keys are kept unchanged, so neither side's tested behaviour moves; an instance that gives both checks only the files matching both. Thesbxmgr_*instances usepaths. Folding the two keys into one would change one of them, so it is left for a follow-up.[rules.pattern_ratchet.sbxmgr_sqlite]checkscrates/libs/sbx/mgr/**andcrates/apps/sbx/**. The CLI and svc crates match nothing today. It allows onlysrc/db.rsandsrc/db/**, and matches lines containing:rusqlite::,use rusqlite(includinguse ::rusqliteanduse rusqlite as x), andextern crate rusqlite;Connection::open*,Connection::new*andTransaction::new*;sql::sqlite(the shared SQLite open and migrate helpers);pplx_space_common_jobs::db, plain or inside a{…}group on the same line;params!andnamed_params!;.execute(,.execute_batch(,.prepare(,.prepare_cached(,.query_row(,.query_row_and_then(,.query_one(,.query_map(,.query_and_then(,.transaction(,.unchecked_transaction(,.transaction_with_behavior(,.savepoint(,.pragma_update(,.pragma_query(and.pragma_query_value(;immediate_transaction(;.query(or.exists(with an argument on the same line, sopath.exists()anduri.query()don't match;SELECT,INSERT … INTO;UPDATE <table> SET(orUPDATE <table>ending the line),DELETE FROM;PRAGMA, a CTE, or a transaction-control statement.[rules.pattern_ratchet.sbxmgr_sqlite_schema]checks every crate. It allows the store, the migrations crate, andcommon/core/src/host/linux.rs, where the path is defined. It matches:pplx_space_sbxmgr_migrationsandSANDBOX_MANAGER_(DB|MIGRATOR);sqlite_sandbox_manager;sbx-manager.db;db/sandbox-manager.This instance is how the fence counts the database migrator. Putting
crates/libs/db/migrator/**into the first instance'spathsinstead would match 37 unrelated migrator lines (bootstrap_roles.rs25,lib.rs7,user_password.rs5): its Postgres SQL and other services' SQLite databases.Known escape classes. Each of these reaches the database, or a raw handle to it, on lines that neither instance matches:
use pplx_space_common_jobs::{withdbon the next line), and groupedsql::{sqlite}imports;use pplx_space_common_svc::sql as s;);open_rusqlite(path, options)), and constructors imported under another name (use rusqlite::Connection as Db;is counted, butDb::open(path)is not);BackgroundWorker::new(Arc::clone(&self.db), …), which hands the raw connection to common-jobs;pragma,pragma_update_and_check,savepoint_with_name,raw_execute,raw_queryand others;.query(and.exists(.Per the owner's decision these regexes are not hardened further; milestone 3's crate boundary closes these escapes for the store's own API.
Pattern checks:
main, the SQL patterns match all 76 statement lines that sat outsidedb/.tests/repository.rsloads the repository's own rules:sbxmgr_sqlite: 22 sample lines, each a crossing. They cover the rusqlite imports, the unqualified constructors, a qualifiedsql::sqlitecall, two job-store imports, both macros,.execute(,.query_one(,.query_row_and_then(,.query(/.exists(with an argument,.unchecked_transaction(,immediate_transaction(, and three SQL literal forms. Eight look-alikes are clean. Not every method in the alternation has its own sample line.Connectiongrows its count, and so does a child module usinguse super::Connection. Both tests fail against the patterns this PR first shipped.sbxmgr_sqlite_schema, the migrator case: six crossings and five look-alikes in a migrator source file. The crossings are the migrations import,SANDBOX_MANAGER_DB, thesqlite_sandbox_manager()call and definition, thesbx-manager.dbpath and thedb/sandbox-managerdirectory. The look-alikes are the template-manager equivalents and theSandboxManagersubcommand variant. The case fails without this round's three patterns.Modes:
no_crossing,no_raised_caps,listed_no_growthandstale_entriesare required.no_new_entriesis advisory for this seeding PR, as inrootfs_copy, so CI reports the seeded entries for review.Not counted:
crates/libs/db/migratoropens and migratesSANDBOX_MANAGER_DBitself today. It is counted as ansbxmgr_sqlite_schemabaseline entry, 5 lines; milestone 3 moves it onto the store crate's migration entry point.scripts/disk-usage/reclaim-backed-btrfs-send-streams.pyandscripts/space-sbx-db-migrate.shnamesbx-manager.db. They are non-Rust operator tools that archcheck does not read; this PR leaves them unchanged as known exceptions.BackgroundWorkerruns SQL on sbx-mgr'sjobstable through the connection sbx-mgr hands it. That library is outside sbx-mgr's crates, and the hand-off is one of the escape classes above.Counts (production code outside
db/)mainrusqlite::/use rusqlitelinessbxmgr_sqlitepattern lines[pattern_ratchet.sbxmgr_sqlite]entries[pattern_ratchet.sbxmgr_sqlite_schema]entriesrun.rs3, migratorlib.rs5)Round 1 widened the patterns, which left the
sbxmgr_sqlitetotal at 203 lines:cleanup_executor.rs6 → 7, for its job-store import;run.rs7 → 6, because its migrations import is now counted bysbxmgr_sqlite_schema.No other file's count changed.
Round 2 added the three migrator patterns to
sbxmgr_sqlite_schema. That raised the migrator'slib.rsfrom 3 to 5 lines (thesqlite_sandbox_managerdefinition and its call); the baseline is recomputed.What remains:
Arc<Mutex<rusqlite::Connection>>) andrusqlite::Result/Errortypes and matches;rusqlite::Transaction::new_uncheckedinlocal_artifacts.rs;run.rs's open and migration.Milestone 2 replaces them with the store's own handle, transaction and error types. It routes the cleanup executor's job-store calls through
db::cleanup_jobsand empties thesbxmgr_sqlitebaseline.file_sizebaseline:--renameslist CI passes (archcheck: accept a renamed baseline entry instead of reporting it as new #596),no_new_entriesaccepts the moved entry; without it, it reports the rename for review.sandbox_eviction.rs1273→1253;db/state_transitions.rs1018→1016;db/sandboxes.rs1553→1547.snapshot_upload_jobs.rs,recovery_cross_node.rsanddb/cleanup_episodes.rsalso shrink, butmainhad already grown them past their caps (1318 > 1210, 1954 > 1946 and 1845 > 1833), so their caps stay.Hidden observable changes
tracingevent moved: the leaseDropwarnings, the backfill warning and the eviction and watchdog logs stay in their modules.sql!locators (module_path!():line!()in the SQL text, visible in SQLite profile traces):gc_staging_is_unownednow carrypplx_space_sbx_mgr::db::local_artifact_leases:<line>;pplx_space_sbx_mgr::db::guest_health_scan:<line>;current_state_label, which has no locator.checkout_delete_in_transaction's copy wassql!-wrapped;recover_stuck_states' copy had different whitespace and nosql!.sql!-wrapped statement, so the recovery path's SQL text changes in whitespace and gains a locator.sql!statements did not move, but shorterdb::route_activation_claimspaths removed lines above them, so theirline!()shifted by −6:db/cleanup_episodes.rs1784, 1806, 1824, 1925, 1938;db/sandboxes.rs1177, 1275, 1282.file!()-derived values are involved.Decisions
LocalArtifacts,sandbox_evictionandsnapshot_upload_jobskeep their entry points and transaction ownership. Only the receipts'db.rsmoves wholesale, since it was already a persistence module.OpenRouteActivationClaim, the artifact state, and the watchdog's guest-health row; otherwisedb/would import its callers.pathswould flag its unrelated Postgres and SQLite code.main825b786(66 commits past the original base).main's new sbx-mgr SQL all landed insidedb/(db/eviction.rs,db/cleanup_jobs.rs,db/local_artifacts.rs,db/cleanup_episodes.rs,db/state_write.rsand their tests), so no statement had to move. The counts above are recounted on the new base and are unchanged. The one new non-leaf dependency insidedb/,crate::snapshot_upload_jobs::SNAPSHOT_UPLOAD_JOB_KINDindb/eviction.rs, is resolved by moving the constant (table above). Conflicts: the archcheckinclude/pathsmerge above; thenode_manager_commandsrule and baseline table, kept beside the two new ones; the README's exit-2 paragraph, which keeps archcheck: accept a renamed baseline entry instead of reporting it as new #596's--renameserrors and adds thepathserror; and a watchdog import.mainc4be884, without conflicts. The five newmaincommits touch no sbx-mgr, migrator, common or archcheck code, so the counts and caps above are unchanged.grpc_state_snapshot.rs11) included test modules; in production code the SQL sat in 7 files.Validation
bash ci/scripts/run-rust-checks.sh fmt(includes the Rust error policy)bash ci/scripts/run-rust-checks.sh clippy(CI's clippy mode, including the workspace--all-targets --all-featurespass)cargo arch --base-baseline <main> --base-architecture <main>file_sizedrift, plus the expectedno_new_entriesfor the two seeded tables and the renamed receipts file.SPACE_CI_REQUIRE_TEST_DEPS=1 cargo nextest run --locked -p pplx-space-sbx-mgr --retries 0#[ignore]transfer_directoryUID test, not a dependency skip.cargo test --locked -p pplx-space-archcheckpattern_ratchet_pathsandmain'sincludescope test, and 5 repository tests including the twosbxmgr_sqlitepattern tests, the migrator case andmain'snode_manager_commandscaseAfter the
c4be884rebase, fmt, clippy,cargo archagainstmainand the archcheck tests re-ran on every head of the stack and passed. Nextest ran once, on #586's headd9f6b99:SPACE_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-sqlite -p pplx-space-db-test-fixtures --retries 0gave 1808 passed, 0 failed (server 1367, store 366, migrator 53, sqlite 12, test fixtures 10). The 1 skip is the root-only#[ignore]transfer_directoryUID test. Kind was not re-run, because the rebase changed no code.Stacked follow-ups:
pplx-space-sbx-mgr-storecrate, which takes over the migrator's open.🤖 Generated with Claude Code