Repository navigation
fix(sqlite): preserve POSIX locks for verified readers - #4803
ScriptedAlchemy wants to merge 3 commits into
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.
| assert_eq!( | ||
| reader | ||
| .connection() | ||
| .query_row("SELECT 1", [], |row| row.get::<_, i64>(0)), |
There was a problem hiding this comment.
[P2] Also assert committed-row visibility across verified-reader teardown.
SELECT 1 proves the returned connection executes a constant, but not that this newly exercised reader returns the correct database contents. The snapshot constructor already reads sqlite_schema, so this is not a claim that it takes no read lock. The missing assertion is data visibility through this particular verified-reader/drop lifecycle while the independent writer is active.
For the snapshot arm, read sample.value and require the old committed value (1), then use the existing separate-process read:1 helper before and after dropping the verified reader; after the writer commits require a new reader to see 2 and pass integrity. The existing external_read_only_wal_reader_survives_live_writer_identity_checks regression covers identity checks, not this verified-reader destruction path.
Keep immutable semantics separate: the WAL fixture creates sample after entering WAL mode, so its schema/data need not yet exist in the main file. Do not mechanically replace the constant with a current-WAL-data expectation in the immutable arm, or claim that arm qualifies live external reading. Preserve the lock-exclusion test and add the appropriate real-row/lifecycle assertions with nonzero Linux and native macOS Bazel results.
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}
| #[cfg(not(unix))] | ||
| _opened_file: OpenedDatabaseFile, |
There was a problem hiding this comment.
🔴 Immutable reader close releases writer locks
When an immutable VerifiedReader closes during a same-process writer transaction, its descriptor releases the writer's POSIX locks. Immutable SQLite handles bypass inode-lock bookkeeping, so external writers can enter before the transaction ends.
Learn more
A POSIX record lock belongs to the process, not to the file descriptor that acquired it. Closing any descriptor for the database inode releases locks held by other SQLite connections in that process. Ordinary SQLite Unix connections can defer a close while their shared inode record has locks; an immutable=1 connection uses a no-lock path that does not participate in that bookkeeping. Removing the retained pin eliminates one unsafe close, but closing the immutable SQLite handle itself still releases the writer's lock. The immutable reader is opened by open_verified_immutable_reader, while open_immutable_reader specifies immutable=1.
Example: A daemon holds BEGIN IMMEDIATE on a database in DELETE mode. It opens and drops an immutable verified reader of that database. The reader closes its own database descriptor; a separate process can now acquire a write lock while the daemon transaction is still active.
Recommended fix: Ensure immutable SQLite descriptors cannot close while a same-inode SQLite writer holds POSIX locks. Use a VFS/reader lifecycle with effective shared-inode deferred-close coordination, then run the new cross-process DELETE and WAL probes against the exact implementation.
Was this helpful? React with 👍 or 👎 to provide feedback.
Scope — Refs #4513 (partial; do not close issue)
Extract the SQLite-only verified-reader lock fix from the held draft integration PR #4776 as an independently reviewable three-file change based on master
91c72aba7750a398dd28e83d64bf0c6d6fef4172. No Kev, model scoring, decision-model, release, or infrastructure changes. Do not merge #4776 as a consequence.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 / strict limits
//crates/tracedecay-rusqlite-runtime:unit_testand affected sessions test targets on this exact head. Native macOS cross-process regression must also be executed before declaring the platform qualified; do not weaken tests/CI.Remaining 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, native macOS, 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.