Repository navigation
fix(sqlite): preserve POSIX locks for verified readers - #4803
Conversation
Refs #4513 Keep Unix verified readers on SQLite-owned handles, retain pathname identity checks around HAS_MOVED, and fail closed without VFS proof. Carry the cross-process rollback/WAL lock regressions separately from the held application/decision-model integration PR.
|
|
Native macOS acceptance is not covered by ordinary PR CI: current |
ScriptedAlchemy
left a comment
ScriptedAlchemy
left a comment
There was a problem hiding this comment.
Refs #4513 — review of exact head 83253b18e2d73ba72e6ac005fc71b242feeb608e.
The Unix change removes the extra raw descriptor from verified readers and preserves the NOTFOUND refusal for their HAS_MOVED check. I have not established an additional production defect in this narrow patch in this review; the remaining actionable request is to strengthen behavioral qualification of the new lifecycle regression (inline), not to conflate this PR with #4805's separate initialization bugs.
Required evidence before treating this as qualified:
- Execute nonzero
bazel test //crates/tracedecay-rusqlite-runtime:unit_test //crates/tracedecay-sessions:unit_test --test_output=errorson the exact resulting head. The PR-triggered run https://github.com/ScriptedAlchemy/tracedecay/actions/runs/42434926345 was still in progress when checked; its existence is not a pass. - Run the existing native macOS workflow with
run_os=true,native_platform=macos, and those samenative_targets. Record the tested SHA and actual nonzero results. No native macOS result is established by this review, and a skipped platform job is not evidence. - For full #4513 acceptance, retain a separate isolated-daemon test using an ordinary external read-only
sqlite3client while writes/checkpoints and verified-reader teardown occur, followed by restart, committed-row verification and integrity checks. The subprocess Rust helper is useful lock evidence, but it is not itself the reported daemon/CLI journey. Do not make that clientimmutable=1to bypass normal locking.
No Rust/Bazel execution is claimed here. Keep this independently reviewable, use Refs #4513, and leave the issue open until full acceptance is on master. Ordinary merging remains with the existing maintenance owner; held draft #4776 and release #4763 remain excluded.
Exercise real WAL and rollback-mode snapshots through the verified reader path, with separate-process read-only checks before and after reader teardown and writer commit. Keep immutable semantics distinct. Refs #4513
|
Refs #4513 — pushed e9cf7d5 as an expected-old-head, nonforce fast-forward on this issue-only branch. The cross-process verified-reader regression now asserts the old committed value through a live read snapshot, separate-process |
Verify real committed-row visibility through both snapshot and immutable readers while a separate-process contender confirms SQLite writer locks. Checkpoint only the immutable WAL fixture's seed; leave ordinary WAL snapshot coverage uncheckpointed. Preserve teardown/commit assertions. Refs #4513
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Repair three hosted automation_runner_test failures by supplying the configured executable identity required by the production prefilter. Use the same identity for the prefilter and retained runner, preserving manual-schedule, exact-ledger, live-lock and lock-race assertions. Add a production-prefilter regression for unconfigured refusal, deduplication and same-occurrence recovery after binding, with zero backend calls and no effectful terminal. Do not relax backend admission, change timeouts, or alter the graph publication changes. Refs #4532
Scope — Refs #4513 (partial; do not close issue)
Extract the SQLite-only verified-reader lock fix from the held draft integration PR #4776 as three SQLite implementation and regression files based on master
91c72aba7750a398dd28e83d64bf0c6d6fef4172. No Kev, model scoring, decision-model, release, or infrastructure changes. Do not merge #4776 as a consequence.The GitHub comparison also lists the shared scheduler test-fixture repair that subsequently landed in master through #4821.
A verified foreign reader used to independently
File::open-pin the database on Unix; closing that raw descriptor can release other SQLite connections' process-wide POSIX record locks, permitting an external writer or journal-mode change while a live daemon transaction remains active.SQLITE_FCNTL_HAS_MOVEDand refuse when the VFS cannot prove it.Verification
At
f743b8275295df31c6c497f4ede57f7b3dafc177, four native macOS Bazel cases passed for verified-reader release, independent writer exclusion, rollback locks, and the WAL journal-mode guard. The full global-db unit suite passed all 498 tests in 83.2 seconds.The Linux fleet job passed all 433 rusqlite-runtime and 662 sessions tests, including the Linux-only descriptor lifetime regression. The global-db suite reached 402 of 498 tests before its 301-second timeout. The SQLite lock and isolation cases passed. The original native Windows check passed. The official Linux retry on the same head passed all 183 selected Bazel targets. Eighteen ran uncached, including the global-db suite in 195.8 seconds and rusqlite-runtime in 15.2 seconds. The downstream native Windows check on attempt 2 also passed.
The bundled SQLite Unix VFS chooses the regular main-database file methods before the pager reads the
immutable=1flag. Its close path defers the descriptor close while another handle holds inode locks. The four physical regression cases check the live writer still excludes the external process after verified-reader teardown, the reader observes committed value 1 and a fresh reader observes value 2 after commit, followed by an integrity check. Committed-value review · Immutable-reader reviewRemaining issue acceptance
This patch does not fix new-store sidecar namespace ownership: pre-existing
-wal,-shm, and-journalcan be overwritten even during successful initialization; failed-initializationdiscard_createdalso unlinks unproven sidecars and has a pathname replacement TOCTOU. Those require separately justified race-safe admission/teardown work and real tests, not a preflightexists()band-aid. Exact daemon/externalsqlite3reader endurance, fail/restart recovery, and final proven state onmasterremain open.Please review and run hosted Bazel gates before the ordinary TraceDecay PR-maintenance task considers a merge. Refs #4513 only.