Repository navigation
sbx-mgr: extract the SQLite store into pplx-space-sbx-mgr-store - #578
Conversation
70444b9 to
6984b02
Compare
3e795d5 to
51364ca
Compare
Review — 3 findingsNo correctness findings in the moved store code. I checked that no Low
Verified against |
the-gigi-pplx
left a comment
There was a problem hiding this comment.
Approved. 3 non-blocking findings in the review comment above.
51364ca to
92f0dc5
Compare
sbx-mgr's node-local SQLite store (`sandboxes` and every other sbx-mgr table) moves out of the server crate into `crates/libs/sbx/mgr/store`, package `pplx-space-sbx-mgr-store`, as a pure move: `src/db` and the leaf type modules it decodes into (`state`, `auto_lifecycle`, `auto_lifecycle_delete_receipts`, `endpoint_proof`, `restore_operation`) keep their module paths inside the new crate, and the server re-exports them, so `crate::db::...` callers are unchanged. Only visibility changes, `cfg(test)` widening to the store's `test-support` feature, and path rewrites (`crate::proto` to the client crate's) touch the moved lines; the fixtures the server's tests share move into `db::fixture` and `db::test_fixtures`, and one store test assertion that called server code is replaced by a server test of that predicate. The boundary is now the compiler's: - The server has no rusqlite or sqlx dependency, normal or dev, and no longer asks for common-svc's `sqlite` feature. - Outside `test-support`, no public store item returns, exposes or re-exports a rusqlite connection, transaction, statement or row. The raw connection, error and statement methods are private to `db` unless the feature is on. What remains are trait impls: `From<rusqlite::Error>` into the store's errors, and `FromSql` for a timestamp column, whose `column_result` returns rusqlite's `FromSqlError` outbound, an error type rather than a database capability. - `--all-features` builds turn `test-support` on, so CI's clippy step now also lints sbx-mgr's production libraries and binaries in each feature configuration their images are built with (none, release, Kind), without dev targets, and asserts that `test-support` is not in those dependency graphs. - A new archcheck rule, `dependency_fence`, reads dependency records from `cargo metadata`, so a renamed, target-specific or dev dependency on rusqlite or sqlx (or sqlx's core and SQLite crates) in any sbx-mgr crate other than the store (and, for sqlx, the migrations crate) fails `cargo arch`. Every fence glob must match a workspace member. The database migrator takes sbx-mgr's database name, path and migrator from the store's `db::migration::target()`, which `db::migrate_at` uses too, so the `sbxmgr_sqlite_schema` baseline is empty. Both line ratchets now allow only the store crate, with every mode required.
92f0dc5 to
0cd506b
Compare
Summary
Milestone 3 of 3, stacked on #570. This concerns sbx-mgr's node-local SQLite database (
sandboxesand every other sbx-mgr table), not the control-plane Postgres. The store moves out of the server crate into its own crate,crates/libs/sbx/mgr/store(packagepplx-space-sbx-mgr-store), as a pure move. The guarantee that "only that crate touches the database" is now a compile-time boundary. Thecargo archline ratchets remain as tripwires.The crate
src/db.rs,src/db/**and the leaf type modulesdb/decodes into:state,auto_lifecycle,auto_lifecycle_delete_receipts,endpoint_proofandrestore_operation, with their tests. That is 72 files, plus a newlib.rsanddb/migration.rs.pplx_space_sbx_mgr_store::db,…::state, …), so the moved files' owncrate::db::…andcrate::state::…paths are unchanged. The server re-exports the six modules fromlib.rs, so none of itscrate::db::…callers changed.pplx-space-sbxmgr-migrationsand owns rusqlite. It also depends onpplx-space-observability, withdefault-features = falseas the server does, becausedb::cleanup_jobs::persistrecords the persisting caller's traceparent sincemain's Observability wave 7: warm-pool pass roots, JSON runtime-supervisor logs, sandbox events with their request trace #573. Feature unification gives it the server'sotel/datadogconfiguration in every image build, so the recorded value is unchanged.crate_namespasses:storeis not a banned token.ARCHITECTURE.tomlhas no crate-layering table, so none applies.rusqlite,sqlx,ref-castandpplx-space-sbxmgr-migrations, normal and dev, and itsfeatures = ["sqlite"]onpplx-space-common-svc. The server used none of them directly once the store left;sqlxand thesqlitefeature were already unused. The dev-dependency ispplx-space-sbx-mgr-storewithtest-support. No rusqlite dev-dependency is needed.test-supportfeature:Connection::raw();db::Connection;db::fixture(theparams!re-export and theinsert_test_sandbox{,_with_policy}seeds, moved fromservice/tests.rs);db::test_fixtures(state_write_fixtures,database_rowsand the quarantine-recovery seeds, moved out ofdb/tests.rs).Cargo.tomlregisters the member and workspace dependency.ci/rust-artifact-inputs.jsonis regenerated: the store joinsspace-db-migrate's and the sbx-mgr images' inputs, and so do its dependenciescommon/jobsandevents, which the migrator now builds through the store.AGENTS.mdstore rules move tostore/AGENTS.md, which has aCLAUDE.mdsymlink.The compile-time boundary (owner decisions)
test-support, the store's public API returns, exposes and re-exports no rusqlite value.Connection::raw(),Error::raw()and the statement methods (execute,prepare,prepare_cached,query_row) arepub(in crate::db).db_private_outside_tests!makes thempubonly withtest-support.SharedConnection::background_worker()BackgroundWorkerinside the store, from the privateArc<Mutex<rusqlite::Connection>>, and returns only the worker; theArcnever leavesdb/handle.rs. Kept as is.pplx_space_common_jobs::dbcall goes through the storedb::cleanup_jobs,db::teardown_*anddb::auto_lifecycle_delete_receipts. Server tests that inspect thejobstable passconn.raw()undertest-support.Connection::raw()behindtest-supportonly--all-featuresbuilds enabletest-support(workspace clippy and nextest), so a new CI step,run_sbxmgr_production_checks(called fromrun_clippy_checks), lints the production graph without it. For each feature configuration sbx-mgr's images are built with, it runscargo clippy --locked -p pplx-space-sbx-mgr-store -p pplx-space-sbx-mgr -p pplx-space-sbxmgr-svc -p pplx-space-db-migrate -p pplx-space-db-migrator [--features …] --lib --bins -- -D warnings. The configurations are: none; the release images'pplx-space-sbxmgr-svc/otel,datadog(docker/Dockerfile.rust-build-common); and the Kind images' set, which addstest-hooks(docker/Dockerfile.rust-build-kind). The migrator has no features. For each configuration it then asserts withcargo tree -e normal,featuresthatpplx-space-sbx-mgr-store feature "test-support"is not in the graph. Addingconn.execute("SELECT 1", [])to server production code fails that step with E0624 (probe removed).cargo-featuresmanifest key, so it ran once, and the exact command is given for reviewers.dependency_fence, not a regex. It reads each member's dependency records fromcargo metadata, which names the real package even underpackage = "…", and includeskindandtarget. Two required instances:sbxmgr_rusqlite(rusqlite,libsqlite3-sys; only the store may) andsbxmgr_sqlx(sqlx,sqlx-core,sqlx-sqlite; only the store and the migrations crate may). Everymembersandallowglob must match a workspace member, or archcheck exits 2. Both checkcrates/libs/sbx/mgr/**andcrates/apps/sbx/**, and normal, dev, build and target-specific dependencies are all rejected. On this tree, adding[target.'cfg(unix)'.dev-dependencies] sqlite = { package = "rusqlite", … }to the server failscargo archwith exit 1, reportingpplx-space-sbx-mgr depends on rusqlite (renamed sqlite, a dev-dependency, for target cfg(unix)).crates/libs/db/migratortakes sbx-mgr's database name, default path and service migrator frompplx_space_sbx_mgr_store::db::migration::target(), throughFrom<MigrationTarget> for SqliteDatabase.db::migrate_atuses the same target, so there is one definition. The shareddb/commonschema is not in the target: each runner applies it from its own embedded copy, as it does for every database with that schema. The migrator no longer names the migrations crate,SANDBOX_MANAGER_DBor asqlite_sandbox_managerhelper, and thesbxmgr_sqlite_schemabaseline is empty. The regression case for those patterns landed in #561 (sbxmgr_sqlite_schema_matches_the_migrators_sandbox_manager_database).Line ratchets:
sbxmgr_sqliteandsbxmgr_sqlite_schemanow allow onlycrates/libs/sbx/mgr/store/src/**. The schema rule still also allows the migrations crate and the path's definition in common-core.required, and both baselines are empty.ARCHITECTURE.tomlcomments call them tripwires on top of the crate boundary.API-surface audit
Run against the store's production API (no
test-support), with rusqlite as a private dependency:rustc's
exported_private_dependenciesreports every rusqlite type in the store's public interface. There are exactly three sites, all trait impls:db::Error's#[from] rusqlite::Error(the?conversion everywhere in the store): inbound only.From<rusqlite::Error> for db::inventory_read::InventoryReadError: inbound only.impl FromSql for db::timestamps::EpochSeconds, so rows decode into it. Itscolumn_resulttakes a rusqliteValueRefand returns rusqlite'sFromSqlResult. So a caller holding aValueRefwould get a rusqliteFromSqlErrorback: an outbound rusqlite error type, but no database capability. The server cannot obtain aValueRefwithout a rusqlite dependency.No public function, field, associated type or re-export returns a rusqlite connection, transaction, statement or row. Removing the three impls would mean replacing hundreds of
?conversions inside the store, which is not a move.What the boundary does not cover (milestone 4 closes the first item):
sql::sqlitemodule stays reachable from the server. The store enables common-svc'ssqlitefeature, and Cargo feature unification turns it on for every crate in the build that depends on common-svc, the server included, whether or not the server asks for it (it no longer does). That leaves these reachable:open_rusqlite, which returns a rawrusqlite::Connection;migrate_embeddedandmigrate;remove_database_filesandrestrict_database_permissions.The path-taking functions can act on sbx-mgr's database without the server naming any rusqlite type. The server calls none of them today, and their qualified path is an
sbxmgr_sqlitetripwire (sql::sqlite) that aliases escape.sbx-manager.db, as sbx-mgr: route every SQLite statement through db/ and fence the rest with cargo arch #561 lists. archcheck does not read them.Pure-move proof
The proof compares each moved file at #570's head (
bc51f2f, rebased ontomain825b786) with its new path under the store. After the rebase it has one more file,main's newdb/tests/stopped_artifact_pin.rs(#551), which moves with the store unchanged. The union of differences is exactly the one the original base gave.main's other changes underdb/moved with the store and contain no normalized difference: the retired-teardown claim, the job traceparent,row_to_local_artifact's visibility, the stopped-pin writers and fixtures, the eviction-claim doc and the quarantine-recovery backoff test. #551's new fixturebegin_cold_startispub, like the other moved fixtures. For every file it takes the multiset of non-blank code lines (archcheck's code lines: not starting with//), after these transformations on both sides:pub,pub(crate),pub(super)andpub(in crate::db). Items the server uses becamepub; fixture items becamepubundertest-support.cfgwidening:#[cfg(any(test, feature = "test-support"))]reads as#[cfg(test)], andnot(any(…))asnot(test). This covers the test fixtures, test-only constructors anddb_private_outside_tests!.state/tests.rs, which rustfmt splits in two):crate::proto→pplx_space_sbx_mgr_client::proto(instate.rs,auto_lifecycle.rsand their tests);crate::service::tests::service_tests::insert_test_sandbox→crate::db::fixture::insert_test_sandbox(seven db test files);crate::db::tests::→crate::db::test_fixtures::in server tests.db/tests/state_write_fixtures.rs→db/test_fixtures/state_write_fixtures.rs.Result:
pub(crate)shortens topub, plus the listed additions below.;/{/}. On those fragments 68 of 72 files are identical.service/tests.rs, the remaining differences are exactly:#[allow(clippy::unwrap_used | expect_used | indexing_slicing | panic)]attributes on fixtures, which now also compile outsidecfg(test)undertest-support.lib.rs(crate docs,forbid(unsafe_code), sixpub mods) anddb/migration.rs(MigrationTarget,target()), pluspub mod migration;andpub mod test_fixtures;indb.rs.db::migrate_at's body takes its name and service migrator frommigration::target()instead of naming them. It still namesCOMMON_SCHEMA_MIGRATOR, and callsmigrate_embeddedwith the same arguments.insert_test_sandbox{,_with_policy}fromservice/tests.rsintodb::fixture, with the fouruselines they need.service/tests.rsre-exports them, so the server's call sites are unchanged.db_module_files()indb/tests.rsalso skipstest_*directories, so the source-shape guards don't scan thetest-supportfixtures as production source.TestSandboxInsert,database_rowsand the four quarantine-recovery seeds fromdb/tests.rsintodb/test_fixtures.rs.db/tests.rsre-imports them (oneuseline), andtest_fixtures.rshasuse super::*;.db/tests.rs(mark_start_succeeded_requires_starting_state),assert!(crate::snapshot_encryption::capture_volume_is_fresh(&started), …). It called server code. It was the only test of that predicate'strueresult, so the server gainsservice_tests::a_successful_cold_start_captures_a_fresh_memory_volume, beside the existingfalse-case assertion. That test seeds a Starting row with a full/suspend artifact through the store'stest-supportfixtures, checks the predicate isfalsebefore, runsmark_start_succeededandget_sandbox, and asserts it istrue. An always-falsemutant ofcapture_volume_is_freshfails it, while the existing test still passes.db_private_outside_tests!.service/cleanup_executor.rs. Its inherentimpl CleanupOutcome { fn into_caller_result }cannot cross the crate boundary, so it becomes a localfn caller_result(outcome)with the same match.Hidden observable changes
tracingtargets followmodule_path!()): 26 events in 12 modules move frompplx_space_sbx_mgr::db::<m>topplx_space_sbx_mgr_store::db::<m>, for<m>inauto_lifecycle(2),cleanup_episodes(4),inventory(3),inventory_read(1),quarantine(1),quarantine_recovery(2),restore_transfer_scopes(1),sandbox_row(3),sandboxes(3),snapshot_gc(2),startup_recovery(2) andstate_write(2).EnvFilterdirective ofpplx_space_sbx_mgrstill matches, because targets match by prefix; one ofpplx_space_sbx_mgr::dbno longer does.sql!locators (/*module_path!():line!()*/in the SQL text, visible in SQLite traces): all 24 change prefix frompplx_space_sbx_mgr::db::topplx_space_sbx_mgr_store::db::.auto_lifecycle164→160, 184→180;cleanup_episodes1794→1791, 1812→1809, 1913→1910, 1926→1923;cleanup_jobs366→353;local_artifact_leases326→317;sandboxes1167→1161, 1265→1259, 1272→1266.file!()-derived values: panic locations ofexpect/unwrapin moved code now printcrates/libs/sbx/mgr/store/src/…instead ofcrates/libs/sbx/mgr/server/src/…. Nothing else in the moved code usesfile!(),line!()(beyondsql!) or#[track_caller].db::Error/db::OpenErrorand every store error enum keep theirDisplay.space-db-migratenow builds the store crate and its dependencies. Itsbuild.rsno longer watchesdb/sandbox-manager, since the migrations crate'sbuild.rsdoes and the migrator no longer embeds them itself.Counts
db/)rusqlitementions outside the store, sbx-mgr crates (production / tests)db/[dependencies]+[dev-dependencies][pattern_ratchet.sbxmgr_sqlite]baseline[pattern_ratchet.sbxmgr_sqlite_schema]baselinedependency_fencefindings#[test]/#[tokio::test]attributes)file_size:db/auto_lifecycle_delete_receipts.rs,db/cleanup_episodes.rs,db/quarantine_recovery.rs,db/sandboxes.rsanddb/state_transitions.rs.no_new_entries(advisory) reports the five renames for review.db/auto_lifecycle_delete_receipts.rs1149→1147,db/cleanup_episodes.rs1833→1830,db/sandboxes.rs1541→1535.Decisions
crate::rewrites inside moved files. It also leaves the server's ~730crate::db::…call sites alone.auto_lifecyclemoved whole.db/calls its runtime-state functions. Splitting off its coordinator (whichdb/does not use) would be a non-move refactor. It bringstonic(forStatusinextend_access_deadline) andpplx-space-sbx-mgr-client(forprotoconversions) into the store.db_private_outside_tests!declares each method twice, once per cfg, so the server's tests keep callingconn.execute(..). Undertest-supportthe statement methods expose nothing beyondraw(), which is public there anyway.[target.*]and every dependency kind exact, so there is nothing to harden.sqlxdependency was unused, so it is removed and fenced.migration::target()is aconst fnover the migrations crate's statics, so the migrator'sALL_MIGRATORStest table can use it.dependency_fencerule cases, which the task allows.main825b786. Conflicts:Cargo.lock(regenerated);db/tests.rs, wheremainaddedquarantine_recovery_no_progress_schedules_the_capped_doubling_backoffbeside the fixtures this PR moves todb::test_fixtures(the test stays in the store'sdb/tests.rs, the fixtures stay moved);db/tests/stopped_artifact_pin.rs, added bymainin the directory this PR moves, placed under the store; thefile_sizebaseline; and the archcheck README's exit-2 paragraph (archcheck: accept a renamed baseline entry instead of reporting it as new #596's--renameserrors kept).ci/rust-artifact-inputs.jsonregenerates to the same content.mainc4be884. The only conflict was inarchitecture-baseline.toml.main's Extract guest execution service and guest protocol crates #16 deleted thespaced/guest/serverfilesystem/router.rsandprocess.rscaps right next to where this PR adds the five store caps. Both sides are kept: the store caps are added, and the twospacedcaps stay deleted. No cap changes, becausemainchanged no file under sbx-mgr. This PR's additions toci/rust-artifact-inputs.jsonare the same ones regeneration adds on top ofmain.Review round 1
test-supportrun_clippy_checks: no-feature--lib --binsclippy over the store,pplx-space-sbx-mgr,pplx-space-sbxmgr-svc,pplx-space-db-migrateandpplx-space-db-migrator, plus acargo tree -e normal,featuresassertion that the store'stest-supportis not in that production graph. With dev edges included, the samecargo treedoes show the feature, so the assertion can fire. A probe callingConnection::executefrom server production code fails the step (E0624); the probe is removed.sqlitebut doesn't use itsql::sqlitesurface; milestone 4 closes it.true-result test ofcapture_volume_is_fresha_successful_cold_start_captures_a_fresh_memory_volume, beside the existingfalse-case assertion. An always-falsemutant fails it.db_module_files()test_*directories.dependency_fence[target.'cfg(unix)'.dev-dependencies]case. Everymembers/allowglob must now match a workspace member (exit 2 otherwise), tested for both keys; the repository's real globs all match.sbxmgr_sqlxsqlx-coreandsqlx-sqlite.target.commondb/commonfrom its own copy, as documented onMigrationTarget.migrate_atnamesCOMMON_SCHEMA_MIGRATORas before.sql!locators corrected (11 shifted, 13 unchanged). Also: thestate/tests.rsline split, the common-svc cause, theFromSql::column_resultoutboundFromSqlError(an error type, no database capability), test counts by attribute (1664 = 1310 + 354), and--all-featuresvs release/Kind builds.Rebased onto #570's round-2 head
20e4085, without conflicts.Rebased with the stack onto
main825b786; see Decisions.Review round 2 (final)
test-supportpplx-space-sbxmgr-svc/otel,pplx-space-sbxmgr-svc/datadog, and the Kind images' set withtest-hooks. Each gets clippy (--lib --bins, no dev targets) and thecargo treetest-supportassertion. Probe: a#[cfg(feature = "otel")]function in server production code callingconn.raw(). The no-feature configuration passes, because the probe is compiled out. The release configuration fails witherror[E0624]: method \raw` is privateatwatchdog.rs:3125`. Probe removed.ARCHITECTURE.toml, the store'sAGENTS.mdand the commit message now say it exactly. Outsidetest-support, no public item returns a rusqlite connection, transaction, statement or row. The remaining rusqlite types are trait impls:From<rusqlite::Error>(inbound), andFromSql, whosecolumn_resultreturns rusqlite'sFromSqlErroroutbound, an error type, not a database capability.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: the workspace--all-featurespass, the existing feature-isolation passes, and the new sbx-mgr production step with itscargo treeassertion)conn.execute("SELECT 1", [])in server production code (round 1), and#[cfg(feature = "otel")]+conn.raw()(round 2)cargo arch --base-baseline <main> --base-architecture <main>0cd506b). The advisory ones are pre-existingfile_sizedrift, plusno_new_entriesfor the five moved files' caps under their new paths.cargo test --locked -p pplx-space-archcheckdependency_fencewith the combined renamed and target-specific dev-dependency case and the unmatched-glob errors, and 5 repository testsSPACE_CI_REQUIRE_TEST_DEPS=1 cargo nextest run --locked -p pplx-space-sbx-mgr -p pplx-space-sbx-mgr-store -p pplx-space-db-migrate --retries 092f0dc5: 1785 passed (server 1366, store 366, migrator 53), 0 failed. Before the rebase, rounds 1 and 2: 1716 passed, 0 failed, each time (1715 plus the new server test); the original head ran it three times: 1715 passed, 0 failed, each run. The 1 skip is the root-only#[ignore]transfer_directory::tests::distinct_service_uids_can_exchange_downloads, not a dependency skip.exported_private_dependencies, above), re-run on the rebased headcargo archexit 1 withdependency_fence.sbxmgr_rusqlite.no_crossing(change reverted)After 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.Kind ran on #586's head, which contains this commit: on the final rebase (
1fbf246) and on the previous one (6ceee0c), each on a fresh cluster. Both runs passedsmoke-kind-e2e.shandsmoke-kind-typed.sh all(ECW 80 passed, native 25 passed, 0 failed, 0 inconclusive), and the deployed sbx-mgr binary was proven to be from that head. See #586 for the full table.🤖 Generated with Claude Code