fix: two Connections on one file in one process now lock against each other (#706) - #723
Open
dpsiderius wants to merge 2 commits into
Open
dpsiderius wants to merge 2 commits into
dpsiderius wants to merge 2 commits into
Conversation
… other (#706) Two Connections opened on the same file in one process shared no lock state: each UnixVfsFile minted its own Rc<RefCell<FileLockState>>, so a BEGIN IMMEDIATE escalation on one handle never conflicted with a second handle's own independently-opened fd on the same inode -- POSIX fcntl locks are scoped to (process, inode), never to the calling process's own prior lock. A second handle's write reported success and was silently discarded by the first handle's commit. src/vfs/inode_registry.rs adds the process-wide (device, inode) registry stock sqlite3's unixInodeInfo provides: one real fcntl-backed FileLockState per inode, shared by every Connection on it, with in-process bookkeeping (shared_holders, write_holder) that refuses a transition before any fcntl call would silently succeed against another in-process handle. UnixVfsFile/UnixLockGuard now go through this shared state instead of each owning an independent lock ladder. Found and fixed along the way: reusing a cached fd regardless of open mode handed a later writer a read-only fd, failing every F_WRLCK step with EBADF -- caught by tier0's hot-journal-recovery test once real fd-sharing was wired in. SharedInodeLock now tracks writability and reopens in place the one time a write-needing caller finds a read-only entry (safe: a read-only opener never locks, so the entry is always Unlocked at that point). src/vfs/shm.rs's WAL_WRITE_LOCK/WAL_CKPT_LOCK guards gain the same in-process arbitration via a smaller ExclusiveGate sibling, closing #491 for real (it was closed COMPLETED with no answer recorded) rather than by assertion -- WalReadLock's per-slot claim is a documented, non-data-lossy residual (see ADR-0042). Tests: tests/corpus/in_process_lock_registry_test.rs proves the issue's exact scenario, a cross-process regression guard, a handle-close/reopen lifecycle check, and an N-handle pool progress check, all against real Pagers/BEGIN IMMEDIATE. ADR-0042 records the design and alternatives rejected. Note: PR #705 (the embedding API, src/api.rs, and its #[ignore]d ratchet in_process_connections_lock_against_each_other) is not merged to main, so this reproduces the bug with two in-process Pager-backed sessions built directly over UnixVfs instead. That ratchet should be un-ignored once both #705 and this land. spend: within estimate (medium-large) -- most of it went to the in-process arbitration design and the read/write-mode fd-sharing bug it surfaced, not the WAL-lock follow-through.
7 tasks
7 tasks
5 tasks
…706) The Consequences section described the WalReadLock in-process residual as benign-in-the-safe-direction ("a checkpoint bounds itself more conservatively than necessary"). That's backwards: active_reader_marks probes occupancy with a non-blocking F_WRLCK, which POSIX never conflicts with a lock this same process already holds, so a same-process reader's mark is invisible to it; checkpoint.rs then folds the resulting empty mark list into .unwrap_or(total_frames), letting a checkpoint backfill the whole WAL rather than bounding conservatively. Also corrected the "dormant" framing: the checkpoint path (Pager::set_journal_mode -> switch_wal_to_journal -> checkpoint_passive) is reachable today through PRAGMA journal_mode round-tripping (verified against the built CLI); what's still missing for harm is a second in-process reader, which needs the unmerged embedding API (#705). No other file cites ADR-0047 yet (only CHANGELOG.md's own "See ADR-0047" note and the ADR index), so this stays an in-place edit per CLAUDE.md's uncited-ADR carve-out rather than a superseding ADR. spend: small, doc-only — a few file reads, a CLI build/repro to verify reachability, and the rewrite itself.
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.
Stacked on #710 (fix/710-change-counter) — merge that first; the reproduction
uses the persistent-oracle-session helper #710 adds.
What
src/vfs/inode_registry.rs— a process-wide registry keyed by(device, inode), mirroring stock SQLite'sunixInodeInfoinos_unix.c. EveryUnixVfsFile/lock guard on the same inode now sharesone real fcntl-backed lock ladder instead of each minting an independent
one, so a same-process lock conflict is refused before a
fcntlcallwould silently allow it (POSIX record locks are
(process, inode)scoped, not per-handle).
src/vfs/shm.rs'sWAL_WRITE_LOCK/WAL_CKPT_LOCKguards get the same arbitration via a smallerExclusiveGatesibling — this closes the question #491 left open.Design and alternatives rejected recorded in ADR-0047, referencing
#491/#412.
Bug found and fixed along the way
Reusing a cached fd regardless of open mode handed a later writer a
read-only fd, so every
F_WRLCKstep then failedEBADF. Caught bytests/tiers/tier0.rs::t0_hot_journal_recovers_committed_state. Fixedwith a
writableflag and in-place reopen.Known residual, out of scope, recorded in ADR-0047
WalReadLock's per-slot in-process race — two in-process readers couldstill claim the same reader-mark slot. Not data-lossy; flagged as a
follow-up rather than silently dropped.
Before/after (the issue's exact scenario)
Cross-process locking (verified against the pinned oracle) is unaffected
and stays correct.
Test plan
tests/corpus/in_process_lock_registry_test.rs— the issue's exactscenario, a cross-process regression guard, a handle close/reopen
lifecycle test, an N-handle pool progress test.
make test,make lint,make test-corpus,make check-mvl-limit,make check-mod-files,make assuranceall pass.Note for whoever lands #705 (embedding API): its
#[ignore]d ratchetin_process_connections_lock_against_each_othershould be un-ignored onceboth this and #705 are merged.
Refs: 013/Req-4, #491, #412, #705, ADR-0047
Closes #706
spend: within estimate (medium-large) -- most went to the arbitration
design and the read/write-mode fd bug it exposed