Repository navigation
test(control): guard lite/pg schema drift, drop unused schema objects, align lite indexes - #368
Merged
Merged
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review. 📝 WalkthroughWalkthroughAdds test-only checks for SQLite and Postgres schema drift and inventories reserved or unused schema objects. Updates SQLite indexes for ready-job claims and latest-attempt lookups. Adds tests for query plans, canonical ID and timestamp storage, and timeline cleanup. ChangesSchema maintenance
Storage behavior coverage
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Other Merge Risk: ⚪ Minimal · up to The supplied evidence identifies no actionable issue with the schema checks or storage tests. The change appears mergeable after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…rom indexes `jobs_ready` carried `namespace_id` between the pool key and the priority, so SQLite sorted the last three ORDER BY terms of every claim poll (`USE TEMP B-TREE FOR LAST 3 TERMS OF ORDER BY`); Postgres already keyed its index exactly like the claim ORDER BY. The key is now `(pool_key, priority DESC, run_order, job_order)` on both backends (`SCAN j USING INDEX jobs_ready`, no sort). SQLite also gains `job_requests_attempts (run_id, job_id, request_id DESC)` — the index Postgres has — which serves the latest-attempt lookups (`WHERE run_id = ? AND job_id = ? ORDER BY request_id DESC LIMIT 1`, used by dispatch, settle, steps, the reaper and the test view) and the `jobs` -> `job_requests` cascade; the partial inflight index cannot serve settled attempts. Fresh databases get the aligned indexes; an existing database keeps working unchanged (a missing index is never an error), which is why this needs no schema-version bump and strands no data. Tests: a new `TestDb::query_plan` helper pins both plans (`jobs_ready_serves_the_claim_order_without_a_sort`, `attempt_lookups_use_the_attempts_index`), and the shared suite gains the canonical-id / microsecond-truncation coverage plus the lite shared-timeline trigger test that the schema-drift guard in the next commit documents.
…objects The two control schemas are hand-maintained copies of one design, and nothing caught the drift between them. `control/schema_drift.rs` parses both DDL files (tables, column names, inline UNIQUE, foreign keys, index names and ordered index columns) and asserts they match except for `DOCUMENTED_DIFFERENCES`, where every entry carries its reason. The test fails on an undocumented difference and on a stale entry, so aligning the backends shrinks the list instead of leaving it to rot. The 14 current entries cover the partition children, the xid8 outbox columns and their reader index (owned by the durable-outbox PR), `consumer_offsets.last_txid`, the GIN `runners_labels` index, the pg-only `job_messages.job_timeout_s` extraction (aligning it needs a migration, because the reaper must not read a column an existing database does not have), the shared `timeline_id` (with the reverse FK), and the `runner_sessions.runner_id` FK that the legacy session-before-registration path makes impossible on SQLite's `PRAGMA foreign_keys = ON`. The same module inventories what no code reads: `RESERVED_UNUSED` (`namespace_policies`, the three unread `namespace_limits` columns and the `artifacts` table — placeholders for planned features) and `UNUSED` (`namespaces.cell_generation`/`config_version`, `run_submissions.secret_refs`, `jobs.not_before`, `provision_requests.lease_owner`/`leased_until`, `runner_sessions.engine_node_id`, `log_files.byte_count`/`line_count`, `artifacts.upload_token_hash`/`finalized_at`). Both DDL files carry matching `-- RESERVED:`/`-- UNUSED:` markers at each site, and a second test greps the Rust sources: the day code starts using one of these, it fails with the object, the reason and the location. Dropping any of them is a migration-backed change, not this PR: the branch has no migration runner, and the backends refuse a database stamped with any other schema version. Also stops `docs/control-schema.sql` from claiming secrets and upload tokens are stored: the file is the pg schema mirrored for readers, and both copies now mark the RESERVED/UNUSED objects at each site.
…inventory Unreleased: the schema-drift guard and reserved/unused inventory, the canonical-id / microsecond-truncation and shared-timeline coverage, and the SQLite index alignment (`jobs_ready` key, new `job_requests_attempts`) with its before/after EXPLAIN QUERY PLAN.
Bnjoroge1
force-pushed
the
Bnjoroge/schema-drift
branch
from
October 7, 2026 04:35
4126f75 to
2084f22
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
… use `sql_mentions` treated `DELETE` as a non-verb, so `DELETE FROM provision_requests WHERE lease_owner = ?` — a real read of an inventoried column — was invisible to `reserved_and_unused_objects_stay_unreferenced`, and the inventory test would have stayed green while that column was in use. `DELETE` now counts when the needle is a column; a table-only `DELETE` stays ignored, so the delete-only `artifacts` cleanup is still not a use. A new unit test pins both halves.
The engine's run for 878f542 checked out the stale test merge 23e7361 ("Merge 2084f22 into 992e3b8") — the previous head's merge — so those checks, although green, never exercised the DELETE-predicate fix. No content change: the tree is identical, and this commit only gives GitHub a fresh merge to compute (its ^2 is this head) so the next run tests the right tree.
Collaborator
Author
|
@devin-ai-integration review |
Drop run_submissions.secret_refs, jobs.not_before, provision_requests.lease_owner/leased_until, runner_sessions.engine_node_id, log_files.byte_count/line_count, artifacts.upload_token_hash and pg's runners_labels GIN index: no code referenced any of them (the only labels SQL is jsonb_array_elements_text, which a GIN index cannot serve). Existing databases that still have the columns keep working. Namespace platform columns and namespace_policies stay, marked platform-owned. Remove the RESERVED/UNUSED inventory and its source-grep tests.
Bnjoroge1
changed the title
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Comment on lines
+147
to
+150
| /// `column -> table(columns)` for inline `REFERENCES`, `a,b -> table(x,y)` | ||
| /// for table-level `FOREIGN KEY` clauses. The `ON DELETE` action is not | ||
| /// part of the key (it is behavioral, not structural, and both schemas | ||
| /// pick the action to match their own cascade rules). |
There was a problem hiding this comment.
🟡 Foreign-key cascade drift goes undetected
If job_requests loses its ON DELETE CASCADE in either schema, foreign_keys still compares equal. Run archival can then fail while the drift guard passes.
Learn more
The guard compares the two control database schemas to catch accidental divergence. It records each foreign key as its source columns and referenced table and columns, but omits the delete action. Both schemas currently cascade deletion from jobs to job_requests (SQLite definition, Postgres definition). Changing either action leaves the parsed keys identical. Deleting a run then fails on the backend without the cascade because requests still reference jobs.
Example: Remove ON DELETE CASCADE from SQLite's job_requests foreign key while retaining its target. The guard passes; archiving a completed run with an attempt cannot delete the referenced job.
Recommended fix: Parse and compare ON DELETE actions for inline and table-level foreign keys, including actions on continuation lines. Add a guard regression test changing only the action.
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+339
to
+355
| fn parse_index(rest: &str) -> Option<(String, Vec<String>)> { | ||
| let (name, tail) = split_ident(rest); | ||
| let open = tail.find('(')?; | ||
| let close = tail[open..].find(')')? + open; | ||
| let columns = tail[open + 1..close] | ||
| .split(',') | ||
| .map(|column| { | ||
| let column = column.trim(); | ||
| let column = column | ||
| .strip_suffix("DESC") | ||
| .or_else(|| column.strip_suffix("ASC")) | ||
| .unwrap_or(column); | ||
| column.trim().to_ascii_lowercase() | ||
| }) | ||
| .filter(|column| !column.is_empty()) | ||
| .collect(); | ||
| Some((name, columns)) |
There was a problem hiding this comment.
🟡 Partial-index drift goes undetected
If jobs_ready changes its WHERE predicate on one backend, parse_index still reports matching columns. The guard passes despite losing the ready-queue index.
Learn more
The drift guard compares indexes between the two control backends. It records the index name and column list, but not the partial-index predicate or uniqueness. The claim path filters queue_state = 'ready' (claim_one), and the index is useful only when its predicate includes those ready rows. Changing its predicate on one backend leaves the guard green, although the backends now have different claim access paths.
Example: Replace SQLite's jobs_ready predicate with WHERE queue_state = 'claimed' and leave its name and columns unchanged. The drift guard passes, but ready jobs are absent from that index.
Recommended fix: Parse and compare the index's UNIQUE modifier and complete WHERE predicate as well as its ordered columns. Add a focused test changing only the predicate.
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+178
to
+179
| -- The request a run was created from. No secret values are ever stored: | ||
| -- secrets are resolved from the SecretProvider when a job is acquired. |
There was a problem hiding this comment.
🔍 Schema diagrams still show removed objects
The schema diagrams still list secret_refs, engine_node_id, upload_token_hash, and runners_labels. Regenerate the derived diagrams alongside the SQL schema changes.
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+5332
to
+5335
| assert_eq!( | ||
| loaded.created_at, | ||
| chrono::DateTime::from_timestamp_micros(loaded.created_at.timestamp_micros()).unwrap(), | ||
| "the stored instant must be its own microsecond truncation" |
There was a problem hiding this comment.
🔍 Precision test misses submitted timestamp
created_at is compared only with its own truncation. Postgres uses its database clock, and SQLite never checks the submitted instant against the stored one.
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+7696
to
+7705
| #[tokio::test] | ||
| async fn jobs_ready_serves_the_claim_order_without_a_sort() { | ||
| let backend = LiteBackend::in_memory().unwrap(); | ||
| let plan = backend | ||
| .test_db_mutate(|tx| { | ||
| tx.query_plan(&format!( | ||
| "SELECT j.run_id FROM jobs j \ | ||
| WHERE j.queue_state = 'ready' AND ({}) \ | ||
| ORDER BY j.pool_key, j.priority DESC, j.run_order, j.job_order \ | ||
| LIMIT 64 OFFSET 0", |
There was a problem hiding this comment.
🔍 Ready-queue speedup lacks benchmark evidence
The empty-database EXPLAIN test proves an access path, not a measured poll speedup. The review guide calls for before-and-after benchmark numbers.
Was this helpful? React with 👍 or 👎 to provide feedback.
Bnjoroge1
added a commit
that referenced
this pull request
Oct 8, 2026
…igns the lite indexes Rebase onto main's schema trim (#368): the baseline still carries the dropped objects and the old SQLite jobs_ready key, so forward-migrate them away. Harden the drift guard for SQLite's quoted identifiers and one-line column lists, and make the migration/schema parity test ignore DDL comments.
Bnjoroge1
added a commit
that referenced
this pull request
Oct 8, 2026
…igns the lite indexes Rebase onto main's schema trim (#368): the baseline still carries the dropped objects and the old SQLite jobs_ready key, so forward-migrate them away. Harden the drift guard for SQLite's quoted identifiers and one-line column lists, and make the migration/schema parity test ignore DDL comments.
Bnjoroge1
added a commit
that referenced
this pull request
Oct 8, 2026
…igns the lite indexes Rebase onto main's schema trim (#368): the baseline still carries the dropped objects and the old SQLite jobs_ready key, so forward-migrate them away. Harden the drift guard for SQLite's quoted identifiers and one-line column lists, and make the migration/schema parity test ignore DDL comments.
Bnjoroge1
added a commit
that referenced
this pull request
Oct 8, 2026
…e CLI (#372) * feat(control): versioned refinery migrations, boot ledger guard, store CLI Add forward-only migration sets (migrations/{sqlite,postgres}) applied by embedded refinery 0.10: frozen v1 baseline, fork-approval sweep index, rerun history attempt keys (lite rebuild / pg PK swap + run_attempt backfill), environment deployment columns and the durable review audit table. - control/migrations.rs: ledger-only boot guard (refinery_schema_history is the sole version authority; schema_meta keeps key_fingerprint), legacy/ foreign/unledgered classification, exact applied-set verification. - control/migrate_runner.rs: the one schema writer (init, upgrade, explicit --adopt-baseline after shape verification), plus SQLite/Postgres fresh-vs-upgraded structural fingerprints. - lite/pg open verify instead of creating; test-support initializes through the same migration SQL; preloop serve refuses uninitialized/older/newer/ divergent databases with the recovery command. - preloop store migrate|status (store_admin) with a VACUUM INTO pre-migration backup for SQLite; brand-new local run/init/installer paths initialize an absent store, never upgrade an existing one. - populated baseline fixtures + migration gate tests (fresh init, upgrade, rollback/refusal, adoption, divergence, failed-batch retry), wired into just test-ci and the control-plane PG16/17/18 matrix with a zero-PG guard. Docs: docs/control-migrations.md. Ledger name refinery_schema_history (parent decision; supersedes the preloop_control_migrations placeholder). No Cargo.toml/Cargo.lock changes (dependency PR #370 stacked). * fix(control): queue bucket alias, archive attempt keys, migration test/schema drift - queue_stats: GROUP BY bucket — the `kind` alias was captured by the jobs.kind column, collapsing every queue bucket into one arbitrary row - archive_finished_runs (lite+pg): job_history writes run_attempt, which migration 2026100503 made NOT NULL - lite schema.sql: history tables quoted and jobs columns appended exactly as the migration chain leaves them (parity test) - first test-compile fixes: GenericClient fingerprint, &mut adoption preflight, OptionalExtension import, unused muts - populated-baseline upgrade goes through probe -> adopt like the CLI * fix(control): baseline migration matches the current v5/v4 schema main moved the control schema in place (Postgres v5 / SQLite v4, #365): `check_run_updates` gained the outbox `version`/`lease_owner` columns and dropped its foreign key to `jobs`. The frozen baseline was copied from the pre-#365 tree, so a database created by the current runtime no longer matched it: `preloop store migrate --adopt-baseline` refused every released store, and the fresh-vs-upgraded parity checks drifted from `schema.sql`. The baseline is now the shape the pre-migration runtime actually creates, which is what adoption and the upgrade fixtures compare against. Header names the commit it was frozen from. * fix(control): upgrade a populated pre-ledger pg baseline through adopt The Postgres arm of the migration gate ran the embedded set directly against a populated pre-ledger store. That cannot work: the set starts at the frozen baseline, whose first table (`schema_meta`) already exists, so refinery aborted with `relation "schema_meta" already exists` — the gate was red and the flow it claims to cover was never actually exercised. The upgrade an operator runs is `preloop store migrate --adopt-baseline`: probe the pre-ledger shape, stamp the ledger at the verified version, apply the rest. The SQLite twin of this gate already runs exactly that probe → adopt → apply sequence; the Postgres arm now does the same, and the populated fixture rows are still checked on the upgraded database. * docs(control): name the pre-ledger versions the guard meets The current in-place schema moves are SQLite v4 / Postgres v5 (#365), but the ledger refusal and the `--adopt-baseline` help text still said v3/v4 — an operator reading the refusal would look for the wrong build. The foreign-store refusal also named a `preloop store import` command that does not exist; the command is `preloop store import-legacy`, as docs/control-migrations.md and the legacy-store refusal already say. * style(control): rustfmt the migration branch The rust-lint job runs `cargo fmt --all -- --check`; the store CLI, the lite/pg open guards and the migration gate tests landed unformatted. * fix(conformance): initialize the replay server's control store The control store is migration-ledgered: `preloop serve` refuses a database that has no ledger instead of creating one, and the conformance harness starts the replay server against a brand-new state dir — so the server exited with "control database .../preloop.db is not initialized" and every server-light run failed before the first scenario. A fresh store is initialized the way an install does: `preloop store migrate` (create-only on an empty database, embedded migrations + ledger) now runs once in the harness before the server starts. The harness also builds the CLI it uses for that. * fix(cli): gate the install-time store init on the platform that installs `prepare_control_store` is only called from the systemd installer path, so on any other host it is dead code and `cargo clippy --workspace --all-targets -- -D warnings` fails the lint job there (`function is never used`). Its Linux-only siblings next to it (the env-file helpers, the smolvm bootstraps) are gated the same way. * fix(store): prepare the control store as the service account, never as root A system install initialized the brand-new SQLite store as root *before* the recursive chown, inside the state tree a previous install had already handed to the `preloop` account. That account could plant `state/preloop.db` (or `state` itself) as a symlink and aim root's create, write and chmod at a target of its choosing — a dangling link read as "absent" and an empty target read as "empty" for the brand-new check. `install_systemd` now chowns first and runs the internal `store init-local` as the service account (`setpriv --reuid/--regid preloop --clear-groups`) with an emptied environment; a user-scope install runs the same command as the invoker, so the create only ever happens with the privileges the service itself has. The preparation now judges "brand-new" with `symlink_metadata`, refuses a symlinked database path instead of following it (a link to an existing store is still left untouched), refuses non-regular files, and — for a privileged (euid 0) caller — requires the parent directory to be exclusively the preparer's own. Zero-config first start is unchanged: the database is still created at install time and an existing store is still never touched. * test(control): keep the ambient store URL out of test-build AppState The store-URL precedence tests in tests/concurrency.rs mutate the process-wide PRELOOP_STORE_URL under GITHUB_ENV_LOCK, but every other test that builds an AppState resolves its store through the same variable inside Backend::open, so a victim can open a sibling test temp store (deleted: CANTOPEN panic; alive: foreign store). In test/test-support builds AppState::new_with_store now pins a missing store URL to the state-dir default; Backend::open keeps the production env fallback, and test_open_backend still exercises that contract. A regression test pins the hermetic AppState. Production selection is unchanged. * feat(control): migration 0505 drops the unused control objects and aligns the lite indexes Rebase onto main's schema trim (#368): the baseline still carries the dropped objects and the old SQLite jobs_ready key, so forward-migrate them away. Harden the drift guard for SQLite's quoted identifiers and one-line column lists, and make the migration/schema parity test ignore DDL comments. * fix(control): reconcile the environment columns with the migration column order --------- Co-authored-by: Bnjoroge1 <Bnjoroge1@users.noreply.github.com> Co-authored-by: Preloop Agent <agent@preloop.invalid>
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
1 participant
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.
Problem
control/lite/schema.sql(SQLite) andcontrol/pg/schema.sql(Postgres, mirrored intodocs/control-schema.sql) are two hand-maintained copies of one control-plane schema behindControlBackend. They had drifted, and nothing caught it. Some objects in them also had no reader or writer.What this does
Drift guard (
control/schema_drift.rs). Parses both DDL files (tables, column names, inlineUNIQUE, foreign keys, index names and ordered index columns) and asserts the two parse trees are equal except forDOCUMENTED_DIFFERENCES, where every entry carries a reason. It fails on an undocumented difference and on a stale entry, so aligning the backends shrinks the list. Parse-level only: types, defaults,CHECK, table-levelUNIQUE/PKand triggers are covered by behaviour tests instead.13 documented differences: 5 Postgres partition children, the xid8 outbox columns/index (
outbox_events.txid/origin,consumer_offsets.last_txid,outbox_events_read; owned by the durable-outbox work),job_messages.job_timeout_s(pg generated column, lite usesjson_extract), the sharedtimeline_id(pgUNIQUE+ reverse FK vs lite trigger), and therunner_sessions.runner_idFK (pg only).Drop objects nothing uses. No code referenced them:
run_submissions.secret_refs,jobs.not_before,provision_requests.lease_owner/leased_until,runner_sessions.engine_node_id,log_files.byte_count/line_count,artifacts.upload_token_hash, and Postgres'srunners_labelsGIN index (the only labels SQL isjsonb_array_elements_text, which a GIN index cannot serve; runner matching readslabelsin Rust). An existing database that still has the columns keeps working because nothing touches them. Kept and marked platform-owned:namespaces.cell_generation/config_version,namespace_limits.max_job_timeout_minutes/priority_tier/run_history_retention_days,namespace_policies.artifactsstays (catalog rows).SQLite index alignment.
jobs_readybecomes(pool_key, priority DESC, run_order, job_order), the key Postgres has and exactly the claimORDER BY, so the temp B-tree sort on every poll is gone. SQLite gainsjob_requests_attempts (run_id, job_id, request_id DESC). Two tests pin the plans viaTestDb::query_plan.Storage mapping tests.
ids_and_timestamps_are_stored_canonically(both backends) and a SQLite test for the shared timeline and its cascade trigger.Verification
cargo test -p preloop-runner-server --lib control::on SQLite + a local Postgres: 232 passed, 0 failed.cargo fmt --all --checkandcargo clippy -p preloop-runner-server --all-targets -- -D warnings: clean.lite.jobs_readyto the old key makes the drift test fail with the exact column diff.Not done here
runner_sessions.runner_idFK makes the legacy session-before-registration path impossible on Postgres. Parity gap, to decide separately.created_atprovenance differs: SQLite stores the submitted value, Postgres takesnow().