diff --git a/.openspec/adr/0047-in-process-inode-lock-registry.md b/.openspec/adr/0047-in-process-inode-lock-registry.md new file mode 100644 index 0000000..49ed330 --- /dev/null +++ b/.openspec/adr/0047-in-process-inode-lock-registry.md @@ -0,0 +1,147 @@ +# 0047 — A process-wide `(device, inode)` registry arbitrates in-process locks before `fcntl` + +**Status:** Accepted · **Date:** 2026-09-11 + +## Context + +Two `Connection`s (in this crate's current, pre-#705 form: two independent +`Pager`s built directly over `UnixVfs`) opened on the same file **in the +same process** did not lock against each other (#706). Measured: A takes +`BEGIN IMMEDIATE` and inserts a row; B inserts a second row and gets `Ok`; +A commits; B's row is gone, silently, and `PRAGMA integrity_check` still +reports `ok`. + +POSIX `fcntl(F_SETLK)` record locks are scoped to `(process, inode)`, not +to a file descriptor or a Rust value: two fds opened independently by the +same process on the same inode never conflict with each other at the OS +level — the kernel treats them as the same lock holder. `src/vfs/lock.rs` +already documented this (`FileLockState::file`'s doc comment, ~line 96) +and `check_reserved_lock`'s own doc comment said outright it "cannot see +another handle in the same process." What was missing is the layer stock +SQLite builds on top of that fact: `unixInodeInfo` (`os_unix.c`), a +process-wide registry keyed by `(device, inode)` that arbitrates between +a process's own handles *before* any `fcntl` call is made, so that two +handles on one inode serialize the way two processes already correctly +do. + +#491 asked the adjacent question for the three WAL `-shm` guards +(`WalWriteLock`/`WalCheckpointLock`/`WalReadLock`) and was closed +COMPLETED without an answer recorded. Investigating it here: `src/vfs/shm.rs` +already shares one `-shm` fd per path via `open_shm_shared`'s `Weak` +registry, which fixes the "closing any fd drops every lock this process +holds on that inode" hazard — but nothing stopped two in-process holders +from both winning the same non-blocking `F_WRLCK` byte lock, since that +hazard and *this* one (#706's) are different bugs that happen to share a +root cause. #491 is answered here, not just asserted: the gap was real +and reachable, and is now closed for `WAL_WRITE_LOCK`/`WAL_CKPT_LOCK` +(`WalReadLock`'s per-slot in-process race is a known, non-data-lossy +residual — see Consequences). + +## Decision + +`src/vfs/inode_registry.rs`: a process-wide `Mutex>>>`. `SharedInodeLock` wraps exactly one real, +fcntl-backed `FileLockState` per inode (shared, not one per `UnixVfsFile` +the way it was before), plus in-process bookkeeping — `shared_holders` +(a count) and `write_holder` (a bool) — that gates each ladder transition +*before* the real `fcntl` call: + +- `Unlocked -> Shared`: refused if any in-process handle already holds + `Pending`/`Exclusive` (mirrors the real ladder's own PENDING_BYTE + probe, just against this process's own handles too). +- `Shared -> Reserved`: refused if `write_holder` is already `true`. +- `Pending -> Exclusive`: refused if `shared_holders > 1` (some other + in-process handle still holds `Shared`). + +`claim(path, needs_write, open)` looks up the entry by `path`'s +`(device, inode)` (via `std::fs::metadata`, not by opening first — see +Consequences) and reuses it if found, only calling `open` when no entry +exists; the whole lookup-open-insert sequence runs under the registry's +one `Mutex`, matching stock SQLite's `unixEnterMutex`-guarded +`findInodeInfo`. `UnixVfsFile`/`UnixLockGuard` (`src/vfs/unix.rs`) now +hold an `Arc>` instead of each minting its own +`Rc>`. `ExclusiveGate`, a smaller sibling type +in the same module, gives `src/vfs/shm.rs`'s `WAL_WRITE_LOCK`/ +`WAL_CKPT_LOCK` the same in-process arbitration in miniature (an +at-most-one-in-process-holder set keyed by `-shm` path), reusing +`open_shm_shared`'s existing fd-sharing rather than replacing it. + +## Alternatives rejected + +- **Leave `UnixVfsFile` per-call, add a global "is this path open + elsewhere" flag only at `Pager::open` time.** Doesn't compose: the + actual conflict is per-ladder-*level*, not per-open — a `BEGIN + IMMEDIATE` on handle A must block handle B's escalation specifically, + while B's own `Shared` read alongside A's `Reserved` must still be + fine (RESERVED is held *alongside* SHARED). A single flag can't express + that; the ladder-aware `SharedInodeLock` can. +- **Key the registry by canonicalized path instead of `(device, inode)`.** + This is exactly the workaround the issue calls out the SQE consumer + already having to build on their own — hardlinks/bind mounts/`..` + segments make two different path strings resolve to one inode (or + vice versa across mount namespaces), which `(device, inode)` gets + right by construction and path canonicalization does not. +- **Block (real `F_SETLKW`) instead of refusing.** Every other lock + primitive in this crate is non-blocking `F_SETLK`, mapped to + `VfsError::Locked`/`SQLITE_BUSY`-style retry at the caller. Blocking + here would be the only exception, and would risk an in-process + deadlock (two handles on the same thread, one waiting on the other) + that a non-blocking refusal can't cause. +- **Reuse the cached fd unconditionally regardless of open mode.** Tried + first, and wrong: a registry entry created by a read-only opener (e.g. + `dump.rs`'s header peek before `Pager::open`) handed a later writer an + fd with no write access, and every `F_WRLCK` step on it failed `EBADF` + — caught by `tests/tiers/tier0.rs::t0_hot_journal_recovers_committed_state` + once real fd-sharing was wired in. Fixed by tracking `writable` and + reopening in place (`SharedInodeLock::upgrade_to_writable`) the one + time a write-needing caller finds a read-only entry — safe because a + read-only opener never calls `lock_shared`, so the entry it created is + always still `Unlocked` when this happens. + +## Consequences + +- `claim`'s lookup-open-insert runs under one global `Mutex` for the + *whole* operation, including the `open()` syscall — a deliberate + serialization of every `Vfs::open_*` call process-wide (matching + `unixEnterMutex`'s own scope), not a fast path. Opens are rare relative + to reads/writes, so this is judged acceptable; if profiling ever shows + otherwise, narrowing the critical section is a follow-up, not a + redesign. +- `WalReadLock`'s per-slot claim (`src/vfs/shm.rs::claim_wal_read_lock`) + is **not** routed through an in-process arbiter in this change, and + the residual is the *unsafe* direction, not merely a wasteful one. + `active_reader_marks` (`src/vfs/shm.rs`) probes slot occupancy with a + non-blocking `F_WRLCK`; POSIX record locks never conflict within one + process, so a reader mark held by *this* process is invisible to its + own probe — the exact bug class #706 exists to fix, left unfixed for + this one lock. With a single in-process `WalReadLock` held, + `active_reader_marks` returns an empty list, and + `src/pager/checkpoint.rs`'s `marks.into_iter().filter(|&mark| mark > + 0).min().unwrap_or(total_frames)` folds that emptiness into "no + constraint" — the checkpoint's safe bound becomes the *whole* WAL + rather than a conservative floor, so a checkpoint can backfill past a + live same-process reader's snapshot. (Two in-process readers racing + `claim_wal_read_lock` can also land on the same slot, the second + clobbering the first's published mark.) This is deliberately out of + scope here, not because it is harmless, but because actual harm is + not reachable yet. The checkpoint path itself + (`Pager::switch_wal_to_journal` -> `checkpoint::checkpoint_passive`, + wired to the public `Pager::set_journal_mode` / `PRAGMA + journal_mode`) is already reachable today with a single in-process + `Pager` — `PRAGMA journal_mode=WAL` followed by `PRAGMA + journal_mode=DELETE` checkpoints and removes `-wal`/`-shm` today — + but causing harm needs a *second* in-process reader racing it, and + nothing shipped can open two in-process `Pager`s (or `Connection`s) + at once yet: the CLI is one `Pager` per process, and the embedding + API that would let a consumer do this (`src/api.rs`, #705) is still + unmerged. Follow-up ticket to route + `claim_wal_read_lock`/`active_reader_marks` through the same + in-process arbiter once that surface lands. +- `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 a pool-of-N-handles progress check, all + against real `Pager`s/`BEGIN IMMEDIATE` — no mocked locks. + +## Related + +Refs: #706, #491, #412 diff --git a/.openspec/adr/index.md b/.openspec/adr/index.md index ab53a47..bbb28de 100644 --- a/.openspec/adr/index.md +++ b/.openspec/adr/index.md @@ -45,3 +45,4 @@ Specs record what the system must do; ADRs record **why it is shaped this way** | [0039](0039-value-payloads-are-arc-not-rc.md) | `Value`'s text and blob payloads are `Arc`, not `Rc` | 2026-09-04 | | [0040](0040-streaming-execution-with-batch-as-wrapper.md) | One streaming execution primitive, with the batch path as its wrapper | 2026-09-01 | | [0041](0041-embedding-api-owns-the-connection-driver-out-of-tree.md) | The embedding API owns the connection; the `sqlx` driver stays out of tree | 2026-08-28 | +| [0047](0047-in-process-inode-lock-registry.md) | A process-wide `(device, inode)` registry arbitrates in-process locks before `fcntl` | 2026-09-11 | diff --git a/CHANGELOG.md b/CHANGELOG.md index 7106870..e219976 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,25 @@ All notable changes to sqlite-rs. Format follows [Keep a Changelog](https://keep **Versioning policy:** one minor version per completed plan phase — the version number tells the plan's story, sub-steps stay inside a phase. V1 (READ CORE) = 0.1.0 through 0.4.0. *(History note: internal iterations briefly numbered 0.4.0–0.6.0 were renumbered into the phase scheme on 14 Aug 2026, before any tag or publication of those versions existed.)* +## [0.18.12] - 2026-09-11 + +### Fixed + +- Two `Connection`s (currently: two independent `Pager`s) opened on the + same file in one process did not lock against each other — a `BEGIN + IMMEDIATE` write on one handle could be silently discarded by another + handle's commit, with no error and `PRAGMA integrity_check` still + reporting `ok`. POSIX `fcntl` locks are scoped to `(process, inode)`, + not to a file descriptor, so two independently-opened fds on the same + inode never conflicted with each other. `src/vfs/inode_registry.rs` + adds the process-wide `(device, inode)` registry stock `sqlite3`'s + `unixInodeInfo` provides: one real fcntl-backed lock ladder per inode, + shared by every `Connection` on it, arbitrating in-process requests + before any `fcntl` call. `src/vfs/shm.rs`'s `WAL_WRITE_LOCK`/ + `WAL_CKPT_LOCK` guards gain the same in-process arbitration, answering + #491 for real rather than by assertion. See ADR-0047 (#706, #491, + #412). + ## [0.18.11] - 2026-09-11 ### Fixed diff --git a/Cargo.lock b/Cargo.lock index 06ee421..d216126 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -781,7 +781,7 @@ checksum = "8ed6a63f02c8539c91a8685a86f4099661ba3da017932f6ebbea6de3f0fa7c90" [[package]] name = "sqlite-rs" -version = "0.18.11" +version = "0.18.12" dependencies = [ "criterion", "md-5", diff --git a/Cargo.toml b/Cargo.toml index b3cbeda..edf34a5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "sqlite-rs" -version = "0.18.11" +version = "0.18.12" edition = "2021" publish = false license = "Apache-2.0" diff --git a/src/vfs.rs b/src/vfs.rs index ef36809..ed10745 100644 --- a/src/vfs.rs +++ b/src/vfs.rs @@ -14,6 +14,7 @@ //! access here goes through safe `nix`/`std` APIs, and the crate is //! `#![forbid(unsafe_code)]` with no local override anywhere. +pub(crate) mod inode_registry; pub(crate) mod lock; mod memory; mod page_source; diff --git a/src/vfs/inode_registry.rs b/src/vfs/inode_registry.rs new file mode 100644 index 0000000..d39682f --- /dev/null +++ b/src/vfs/inode_registry.rs @@ -0,0 +1,357 @@ +// Copyright 2026 Schuberg Philis +// SPDX-License-Identifier: Apache-2.0 +//! Process-wide registry keyed by a file's `(device, inode)`, mediating +//! same-process lock requests *before* any real `fcntl` call is made +//! (#706). Reference: stock `sqlite3`'s `unixInodeInfo` (`os_unix.c`). +//! +//! POSIX `fcntl(F_SETLK)` record locks are scoped to `(process, inode)`, +//! not to a file descriptor or a Rust value: two independently-opened +//! handles on the same file **in the same process** never conflict with +//! each other at the OS level — the kernel considers them the same lock +//! holder. `src/vfs/lock.rs`'s own doc comments already note this (`file()`, +//! line ~96); what was missing is the thing stock SQLite builds on top of +//! it, so two `Connection`s opened on one file in one process actually +//! serialize the way two processes already do, instead of one silently +//! discarding the other's write. +//! +//! [`SharedInodeLock`] is that layer: exactly one real, fcntl-backed +//! [`FileLockState`] per `(device, inode)`, process-wide — every +//! `UnixVfsFile` opened on the same underlying file shares it (via the +//! registry below) instead of each minting its own. On top of that single +//! real lock ladder, it tracks the in-process bookkeeping (`shared_holders`, +//! `write_holder`) that lets each *step* of the ladder refuse a transition +//! that would otherwise silently succeed against the OS but conflicts with +//! another handle in this same process — this is the layer stock SQLite's +//! `unixInodeInfo` provides and this crate didn't. +//! +//! [`ExclusiveGate`] is the same idea in miniature, for `src/vfs/shm.rs`'s +//! single-byte `WAL_WRITE_LOCK`/`WAL_CKPT_LOCK` (#491): those already share +//! one `-shm` fd per path (`open_shm_shared`), which fixes the "closing any +//! fd drops every lock" hazard, but nothing stopped two in-process holders +//! from both winning the same non-blocking `fcntl` byte lock — this gate is +//! that check. + +use std::collections::{HashMap, HashSet}; +use std::fs::File; +use std::hash::Hash; +use std::io; +use std::os::unix::fs::MetadataExt; +use std::path::Path; +use std::sync::{Arc, Mutex, OnceLock, PoisonError, Weak}; + +use super::lock::{FileLockState, LockLevel}; +use crate::sys::fcntl::EAGAIN; + +type InodeKey = (u64, u64); + +/// `Weak` entries so an inode with no more live handles is dropped +/// (releasing its real fcntl locks via `FileLockState`'s own `Drop`) +/// instead of pinned in this registry forever — the "entry outlives +/// handles, removed when the last one goes" lifecycle the ticket calls +/// for. Entries are pruned opportunistically in [`claim`], the only +/// place that touches the map. +static REGISTRY: OnceLock>>>> = OnceLock::new(); + +fn registry() -> &'static Mutex>>> { + REGISTRY.get_or_init(|| Mutex::new(HashMap::new())) +} + +/// The process-wide lock-arbitration state for one `(device, inode)`. +pub(crate) struct SharedInodeLock { + real: FileLockState, + /// Whether the fd currently behind `real` was opened with write + /// access. `fcntl(F_SETLK, F_WRLCK, ...)` — every step past `Shared` + /// on the ladder — fails `EBADF` on an fd opened read-only, so a + /// registry entry first created by a read-only opener (e.g. a header + /// peek) must be upgraded before a later writer shares it, not just + /// reused as-is (see [`claim`]'s own doc comment). + writable: bool, + /// Count of in-process handles currently holding at least `Shared` + /// (the write-holder, if any, is included in this count — RESERVED is + /// held *alongside* SHARED, per the ladder's own doc comments). + shared_holders: u32, + /// Whether some in-process handle already holds `Reserved` or + /// higher — at most one may, mirroring the real ladder's own + /// single-writer invariant, just enforced against this process's + /// other handles too, which raw `fcntl` never does. + write_holder: bool, +} + +impl SharedInodeLock { + fn new(file: File, writable: bool) -> Self { + SharedInodeLock { + real: FileLockState::new(file), + writable, + shared_holders: 0, + write_holder: false, + } + } + + /// Replaces the fd behind `real` with a freshly-opened, write-capable + /// one — used by [`claim`] when a write-needing caller finds an + /// existing entry that was only ever opened read-only. Only valid + /// while `real` is at `Unlocked` (a read-only opener never calls + /// `lock_shared`, so this holds in practice; `claim` doesn't call + /// this otherwise). + fn upgrade_to_writable(&mut self, file: File) { + self.real = FileLockState::new(file); + self.writable = true; + } + + /// Runs `f` with the shared, real fd behind this inode — never a + /// second, independently-opened fd to the same path (see this + /// module's doc comment for why that would be a correctness hazard, + /// not just a wasted syscall). Closure-based rather than returning a + /// borrowed reference: every `FileExt` method this crate calls + /// (`read_at`/`write_at`/`sync_data`/`set_len` via `Metadata`) takes + /// `&File`, so there's never a need to hold a borrow across more + /// than one call. + pub(crate) fn with_file(&self, f: impl FnOnce(&File) -> R) -> R { + f(self.real.file()) + } + + /// Whether some *other* in-process or cross-process holder has + /// RESERVED — `false` when queried by a handle that is itself the + /// current in-process write-holder (matching + /// `sqlite3OsCheckReservedLock`'s "not my own lock" semantics). + pub(crate) fn check_reserved(&self, caller_holds_write: bool) -> io::Result { + if caller_holds_write { + return Ok(false); + } + Ok(self.write_holder || self.real.check_reserved()?) + } + + /// Performs exactly one ladder step (`from` -> `to`, adjacent rungs + /// only — callers step one level at a time, same as + /// `FileLockState::set_level`'s own loop), applying in-process + /// arbitration on the rungs where a same-process `fcntl` call would + /// not itself detect a conflict. + fn step(&mut self, from: LockLevel, to: LockLevel) -> io::Result<()> { + use LockLevel::*; + match (from, to) { + (Unlocked, Shared) => { + // A new reader must not start once some in-process + // writer is mid-ladder (PENDING or EXCLUSIVE) — the same + // rule the real ladder already enforces against *other* + // processes (`step_up`'s own PENDING_BYTE probe). + if self.real.lock_state() >= Pending { + return Err(would_block()); + } + if self.shared_holders == 0 { + self.real.set_level(Shared)?; + } + self.shared_holders = self.shared_holders.saturating_add(1); + } + (Shared, Reserved) => { + if self.write_holder { + return Err(would_block()); + } + self.real.set_level(Reserved)?; + self.write_holder = true; + } + (Reserved, Pending) | (Exclusive, Pending) => { + self.real.set_level(Pending)?; + } + (Pending, Exclusive) => { + // EXCLUSIVE needs the whole SHARED range to itself — a + // same-process fcntl call would not see another + // in-process handle still holding SHARED, so that has to + // be checked here. + if self.shared_holders > 1 { + return Err(would_block()); + } + self.real.set_level(Exclusive)?; + } + (Pending, Reserved) => { + self.real.set_level(Reserved)?; + } + (Reserved, Shared) => { + self.real.set_level(Shared)?; + self.write_holder = false; + } + (Shared, Unlocked) => { + self.shared_holders = self.shared_holders.saturating_sub(1); + if self.shared_holders == 0 { + self.real.set_level(Unlocked)?; + } + } + (a, b) if a == b => {} + (from, to) => { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + format!( + "non-adjacent lock transition {from:?} -> {to:?}: callers must \ + step one ladder rung at a time" + ), + )); + } + } + Ok(()) + } +} + +fn would_block() -> io::Error { + io::Error::from_raw_os_error(EAGAIN) +} + +fn lock_registry_or_recover( + registry: &Mutex>>>, +) -> std::sync::MutexGuard<'_, HashMap>>> { + registry.lock().unwrap_or_else(PoisonError::into_inner) +} + +/// Returns the shared, process-wide [`SharedInodeLock`] for `path`'s +/// underlying `(device, inode)`, opening `path` via `open` only when no +/// existing entry already covers that inode. +/// +/// The whole lookup-open-insert sequence runs under the registry's single +/// `Mutex`, matching stock SQLite's own `unixOpen`/`findInodeInfo` (both +/// run under `unixEnterMutex`) — not for throughput, but for correctness: +/// `open` must never run for a path that already resolves to a tracked +/// inode. An extra, unused fd opened (and then dropped) after a hit would +/// itself close every lock this process holds on that inode the moment +/// it drops (the same POSIX scoping this whole module exists to work +/// around), so the redundant open has to be prevented outright, not +/// cleaned up after the fact. +pub(crate) fn claim( + path: &Path, + needs_write: bool, + open: impl FnOnce() -> io::Result, +) -> io::Result>> { + let mut map = lock_registry_or_recover(registry()); + map.retain(|_, weak| weak.strong_count() > 0); + + if let Ok(meta) = std::fs::metadata(path) { + let key = (meta.dev(), meta.ino()); + if let Some(existing) = map.get(&key).and_then(Weak::upgrade) { + if needs_write { + let mut inner = existing.lock().unwrap_or_else(PoisonError::into_inner); + if !inner.writable { + // A read-only opener (e.g. a header peek before the + // real `Pager::open`) claimed this inode first. A + // read-only fd never calls `lock_shared`, so this + // entry has taken no real lock yet — safe to reopen + // write-capable and swap in place, rather than + // handing this writer an fd whose every `F_WRLCK` + // step would fail `EBADF`. + inner.upgrade_to_writable(open()?); + } + drop(inner); + } + return Ok(existing); + } + } + + let file = open()?; + let meta = file.metadata()?; + let key = (meta.dev(), meta.ino()); + let entry = Arc::new(Mutex::new(SharedInodeLock::new(file, needs_write))); + map.insert(key, Arc::downgrade(&entry)); + Ok(entry) +} + +/// One in-process handle's view of its own place on the ladder, plus the +/// shared, process-wide state every handle on this inode steps through. +/// Mirrors [`FileLockState`]'s own `level` field, just resolved through +/// [`SharedInodeLock::step`] instead of calling `fcntl` directly. +pub(crate) struct InodeLockHandle { + shared: Arc>, + level: LockLevel, +} + +fn lock_shared_inode( + shared: &Arc>, +) -> std::sync::MutexGuard<'_, SharedInodeLock> { + shared.lock().unwrap_or_else(PoisonError::into_inner) +} + +impl InodeLockHandle { + pub(crate) fn new(shared: Arc>) -> Self { + InodeLockHandle { + shared, + level: LockLevel::Unlocked, + } + } + + pub(crate) fn check_reserved(&self) -> io::Result { + lock_shared_inode(&self.shared).check_reserved(self.level >= LockLevel::Reserved) + } + + pub(crate) fn set_level(&mut self, target: LockLevel) -> io::Result<()> { + while self.level < target { + let next = successor(self.level); + lock_shared_inode(&self.shared).step(self.level, next)?; + self.level = next; + } + while self.level > target { + let prev = predecessor(self.level); + lock_shared_inode(&self.shared).step(self.level, prev)?; + self.level = prev; + } + Ok(()) + } +} + +impl Drop for InodeLockHandle { + fn drop(&mut self) { + // Best-effort, matching `FileLockState`'s own `Drop`: nothing + // more can be done about a failure here. + self.set_level(LockLevel::Unlocked).ok(); + } +} + +fn successor(level: LockLevel) -> LockLevel { + use LockLevel::*; + match level { + Unlocked => Shared, + Shared => Reserved, + Reserved => Pending, + Pending | Exclusive => Exclusive, + } +} + +fn predecessor(level: LockLevel) -> LockLevel { + use LockLevel::*; + match level { + Exclusive => Pending, + Pending => Reserved, + Reserved => Shared, + Shared | Unlocked => Unlocked, + } +} + +/// A process-wide "at most one in-process holder at a time" gate keyed by +/// an arbitrary `Eq + Hash` identity — the same in-process arbitration +/// [`SharedInodeLock`] gives the main db file's multi-level ladder, +/// shrunk to the single-exclusive-holder case `src/vfs/shm.rs`'s +/// `WAL_WRITE_LOCK`/`WAL_CKPT_LOCK` need (#491): those already share one +/// `-shm` fd per path, but nothing stopped a second in-process handle +/// from also winning the same non-blocking `fcntl` byte lock. +pub(crate) struct ExclusiveGate { + held: Mutex>, +} + +impl ExclusiveGate { + pub(crate) fn new() -> Self { + ExclusiveGate { + held: Mutex::new(HashSet::new()), + } + } + + /// Claims `key` if no in-process holder already has it. Returns + /// `false` (refused) rather than blocking, matching every other lock + /// primitive in this crate (`F_SETLK`, never `F_SETLKW`). + pub(crate) fn acquire(&self, key: K) -> bool { + self.held + .lock() + .unwrap_or_else(PoisonError::into_inner) + .insert(key) + } + + pub(crate) fn release(&self, key: &K) { + self.held + .lock() + .unwrap_or_else(PoisonError::into_inner) + .remove(key); + } +} diff --git a/src/vfs/shm.rs b/src/vfs/shm.rs index a24c427..9ec293c 100644 --- a/src/vfs/shm.rs +++ b/src/vfs/shm.rs @@ -44,6 +44,7 @@ use std::sync::{Arc, Mutex, OnceLock, Weak}; use crate::sys::fcntl::{off_t, EACCES, EAGAIN, F_RDLCK, F_UNLCK, F_WRLCK, O_NOFOLLOW}; +use super::inode_registry::ExclusiveGate; use super::lock::fcntl_lock; use super::{SharedLockGuard, VfsError}; @@ -96,6 +97,29 @@ const WAL_WRITE_LOCK_BYTE: off_t = UNIX_SHM_BASE; /// read(0..4)). const WAL_CKPT_LOCK_BYTE: off_t = UNIX_SHM_BASE.saturating_add(1); +/// In-process arbitration for `WAL_WRITE_LOCK` (#491/#706): `open_shm_shared` +/// already shares one fd per `-shm` path, so the real `fcntl` byte lock +/// below no longer risks the "closing any fd drops every lock" hazard — +/// but a second in-process holder would still win the same non-blocking +/// `fcntl` call, since POSIX locks never conflict with the calling +/// process's own locks. Keyed by `-shm` path, same identity +/// `open_shm_shared` already uses. +static WAL_WRITE_HOLDERS: OnceLock> = OnceLock::new(); +/// Same idea as [`WAL_WRITE_HOLDERS`], for `WAL_CKPT_LOCK`. +static WAL_CKPT_HOLDERS: OnceLock> = OnceLock::new(); + +fn wal_write_holders() -> &'static ExclusiveGate { + WAL_WRITE_HOLDERS.get_or_init(ExclusiveGate::new) +} + +fn wal_ckpt_holders() -> &'static ExclusiveGate { + WAL_CKPT_HOLDERS.get_or_init(ExclusiveGate::new) +} + +fn would_block_error() -> io::Error { + io::Error::from_raw_os_error(EAGAIN) +} + /// A held `WAL_WRITE_LOCK`, releasing on drop — taken by a writer (#389) /// before appending frames/advancing `mxFrame`, so a second concurrent /// writer is refused rather than interleaving frames or racing the @@ -103,6 +127,7 @@ const WAL_CKPT_LOCK_BYTE: off_t = UNIX_SHM_BASE.saturating_add(1); #[derive(Debug)] pub struct WalWriteLock { file: Arc, + path: PathBuf, } impl SharedLockGuard for WalWriteLock {} @@ -110,14 +135,36 @@ impl SharedLockGuard for WalWriteLock {} impl Drop for WalWriteLock { fn drop(&mut self) { fcntl_lock(&self.file, F_UNLCK, WAL_WRITE_LOCK_BYTE, 1).ok(); + wal_write_holders().release(&self.path); } } pub(crate) fn claim_wal_write_lock(shm_path: &Path) -> io::Result { - let file = open_shm_shared(shm_path)?; - validate_shm_len(&file)?; - fcntl_lock(&file, F_WRLCK, WAL_WRITE_LOCK_BYTE, 1)?; - Ok(WalWriteLock { file }) + // #491/#706: `open_shm_shared` already gives every in-process caller + // the same fd for this path, so the real `fcntl` call below can't be + // relied on to refuse a second in-process holder — POSIX locks never + // conflict with the calling process's own locks, regardless of fd. + // `wal_write_holders` is the in-process arbitration layer that + // catches what the real lock can't. + if !wal_write_holders().acquire(shm_path.to_path_buf()) { + return Err(would_block_error()); + } + let result = (|| -> io::Result> { + let file = open_shm_shared(shm_path)?; + validate_shm_len(&file)?; + fcntl_lock(&file, F_WRLCK, WAL_WRITE_LOCK_BYTE, 1)?; + Ok(file) + })(); + match result { + Ok(file) => Ok(WalWriteLock { + file, + path: shm_path.to_path_buf(), + }), + Err(e) => { + wal_write_holders().release(&shm_path.to_path_buf()); + Err(e) + } + } } /// A persistent `-shm` fd for [`super::Vfs::open_wal_shm`] (#437): opened @@ -157,13 +204,28 @@ fn to_shm_lock_error(path: &Path, source: io::Error) -> VfsError { impl super::WalShm for UnixWalShm { fn claim_write_lock(&self) -> super::Result<()> { - fcntl_lock(&self.file, F_WRLCK, WAL_WRITE_LOCK_BYTE, 1) - .map_err(|source| to_shm_lock_error(&self.path, source)) + // Same `wal_write_holders` gate `claim_wal_write_lock` uses (#491/ + // #706) — this is the persistent-handle (#437) path to the same + // `WAL_WRITE_LOCK_BYTE`, so both routes have to arbitrate against + // each other through the one gate, not just against themselves. + if !wal_write_holders().acquire(self.path.clone()) { + return Err(VfsError::Locked { + path: self.path.display().to_string(), + }); + } + let result = fcntl_lock(&self.file, F_WRLCK, WAL_WRITE_LOCK_BYTE, 1) + .map_err(|source| to_shm_lock_error(&self.path, source)); + if result.is_err() { + wal_write_holders().release(&self.path); + } + result } fn release_write_lock(&self) -> super::Result<()> { - fcntl_lock(&self.file, F_UNLCK, WAL_WRITE_LOCK_BYTE, 1) - .map_err(|source| to_shm_vfs_error(&self.path, source)) + let result = fcntl_lock(&self.file, F_UNLCK, WAL_WRITE_LOCK_BYTE, 1) + .map_err(|source| to_shm_vfs_error(&self.path, source)); + wal_write_holders().release(&self.path); + result } fn publish_mx_frame(&self, mx_frame: u32) -> super::Result<()> { @@ -277,6 +339,7 @@ const N_BACKFILL_OFFSET: u64 = 96; #[derive(Debug)] pub struct WalCheckpointLock { file: Arc, + path: PathBuf, } impl SharedLockGuard for WalCheckpointLock {} @@ -284,14 +347,32 @@ impl SharedLockGuard for WalCheckpointLock {} impl Drop for WalCheckpointLock { fn drop(&mut self) { fcntl_lock(&self.file, F_UNLCK, WAL_CKPT_LOCK_BYTE, 1).ok(); + wal_ckpt_holders().release(&self.path); } } pub(crate) fn claim_wal_checkpoint_lock(shm_path: &Path) -> io::Result { - let file = open_shm_shared(shm_path)?; - validate_shm_len(&file)?; - fcntl_lock(&file, F_WRLCK, WAL_CKPT_LOCK_BYTE, 1)?; - Ok(WalCheckpointLock { file }) + // See `claim_wal_write_lock`'s doc comment: same in-process + // arbitration gap, same fix. + if !wal_ckpt_holders().acquire(shm_path.to_path_buf()) { + return Err(would_block_error()); + } + let result = (|| -> io::Result> { + let file = open_shm_shared(shm_path)?; + validate_shm_len(&file)?; + fcntl_lock(&file, F_WRLCK, WAL_CKPT_LOCK_BYTE, 1)?; + Ok(file) + })(); + match result { + Ok(file) => Ok(WalCheckpointLock { + file, + path: shm_path.to_path_buf(), + }), + Err(e) => { + wal_ckpt_holders().release(&shm_path.to_path_buf()); + Err(e) + } + } } /// The frame marks of readers currently pinned to this WAL generation — diff --git a/src/vfs/unix.rs b/src/vfs/unix.rs index d2670c9..953802d 100644 --- a/src/vfs/unix.rs +++ b/src/vfs/unix.rs @@ -2,14 +2,14 @@ // SPDX-License-Identifier: Apache-2.0 //! Unix `Vfs` implementation, backed by `std::fs`. -use std::cell::RefCell; use std::fs::File; use std::os::unix::fs::FileExt; use std::path::{Path, PathBuf}; -use std::rc::Rc; +use std::sync::{Arc, Mutex}; use crate::sys::fcntl::{EACCES, EAGAIN}; +use super::inode_registry::{self, InodeLockHandle}; use super::{companion_path, lock, shm, FileLock, Result, SharedLockGuard, Vfs, VfsError, VfsFile}; /// Reads database files directly from the local filesystem via `std::fs`. @@ -18,17 +18,18 @@ pub struct UnixVfs; impl Vfs for UnixVfs { fn open_read(&self, path: &Path) -> Result> { - let file = File::open(path).map_err(|source| to_vfs_error(path, source))?; - Ok(Box::new(UnixVfsFile::new(file, path))) + Ok(Box::new(UnixVfsFile::new(path, false, || { + File::open(path) + })?)) } fn open_write(&self, path: &Path) -> Result> { - let file = std::fs::OpenOptions::new() - .read(true) - .write(true) - .open(path) - .map_err(|source| to_vfs_error(path, source))?; - Ok(Box::new(UnixVfsFile::new(file, path))) + Ok(Box::new(UnixVfsFile::new(path, true, || { + std::fs::OpenOptions::new() + .read(true) + .write(true) + .open(path) + })?)) } fn exists(&self, path: &Path) -> Result { @@ -37,14 +38,14 @@ impl Vfs for UnixVfs { } fn create_or_open_write(&self, path: &Path) -> Result> { - let file = std::fs::OpenOptions::new() - .read(true) - .write(true) - .create(true) - .truncate(false) - .open(path) - .map_err(|source| to_vfs_error(path, source))?; - Ok(Box::new(UnixVfsFile::new(file, path))) + Ok(Box::new(UnixVfsFile::new(path, true, || { + std::fs::OpenOptions::new() + .read(true) + .write(true) + .create(true) + .truncate(false) + .open(path) + })?)) } fn delete(&self, path: &Path) -> Result<()> { @@ -130,69 +131,81 @@ impl Vfs for UnixVfs { } } -/// A single fd, shared (via `Rc`) between this file's I/O and any -/// [`FileLock`] `lock_shared` hands out — never a second, independently- -/// opened fd to the same path. `Pager::open`'s hot-journal recovery reads, -/// writes, and locks the main database file through this one handle end to -/// end, sidestepping the "`close()` drops all `fcntl` locks on the inode" -/// trap (POSIX `fcntl` locks are scoped to `(process, inode)`, not the open -/// file description — see [`lock::FileLockState::file`]). +/// A single fd, shared process-wide (via [`inode_registry`], keyed by +/// `(device, inode)` rather than just this one `UnixVfsFile`'s own `Rc` — +/// #706) between this file's I/O and any [`FileLock`] `lock_shared` hands +/// out, and with every *other* `UnixVfsFile`/`Connection` opened on the +/// same underlying file in this process. Never a second, independently- +/// opened fd to the same path: `Pager::open`'s hot-journal recovery reads, +/// writes, and locks the main database file through this one shared +/// handle end to end, sidestepping the "`close()` drops all `fcntl` locks +/// on the inode" trap (POSIX `fcntl` locks are scoped to `(process, +/// inode)`, not the open file description — see [`lock::FileLockState::file`]). struct UnixVfsFile { - lock: Rc>, + lock: Arc>, path: PathBuf, } impl UnixVfsFile { - fn new(file: File, path: &Path) -> Self { - UnixVfsFile { - lock: Rc::new(RefCell::new(lock::FileLockState::new(file))), + /// `open` is only called when [`inode_registry::claim`] finds no + /// existing entry for `path`'s inode — see its own doc comment for + /// why an unconditional open here would be a correctness bug, not + /// just a wasted syscall. + fn new( + path: &Path, + needs_write: bool, + open: impl FnOnce() -> std::io::Result, + ) -> Result { + let lock = inode_registry::claim(path, needs_write, open) + .map_err(|source| to_vfs_error(path, source))?; + Ok(UnixVfsFile { + lock, path: path.to_path_buf(), - } + }) } } impl VfsFile for UnixVfsFile { fn read_at(&self, buf: &mut [u8], offset: u64) -> Result { self.lock - .borrow() - .file() - .read_at(buf, offset) + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .with_file(|file| file.read_at(buf, offset)) .map_err(|source| to_vfs_error(&self.path, source)) } fn size(&self) -> Result { self.lock - .borrow() - .file() - .metadata() - .map(|m| m.len()) + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .with_file(|file| file.metadata().map(|m| m.len())) .map_err(|source| to_vfs_error(&self.path, source)) } fn lock_shared(&self) -> Result { - self.lock - .borrow_mut() + let mut handle = InodeLockHandle::new(Arc::clone(&self.lock)); + handle .set_level(lock::LockLevel::Shared) .map_err(|source| to_lock_error(&self.path, source))?; Ok(FileLock(Box::new(UnixLockGuard { - lock: Rc::clone(&self.lock), + handle, path: self.path.clone(), }))) } fn write_at(&self, buf: &[u8], offset: u64) -> Result<()> { self.lock - .borrow() - .file() - .write_all_at(buf, offset) + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .with_file(|file| file.write_all_at(buf, offset)) .map_err(|source| to_vfs_error(&self.path, source)) } fn truncate(&self, len: u64) -> Result<()> { self.lock - .borrow() - .file() - .set_len(len) + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .with_file(|file| file.set_len(len)) .map_err(|source| to_vfs_error(&self.path, source)) } @@ -209,71 +222,58 @@ impl VfsFile for UnixVfsFile { // through the vendored wrapper. #[cfg(target_os = "macos")] fn sync(&self) -> Result<()> { - crate::sys::fcntl::fsync(self.lock.borrow().file()) + self.lock + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .with_file(crate::sys::fcntl::fsync) .map_err(|source| to_vfs_error(&self.path, source)) } #[cfg(not(target_os = "macos"))] fn sync(&self) -> Result<()> { self.lock - .borrow() - .file() - .sync_data() + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .with_file(File::sync_data) .map_err(|source| to_vfs_error(&self.path, source)) } } -/// Returned by [`UnixVfsFile::lock_shared`]: holds the fd's shared lock -/// ladder at `Shared` (or, briefly, `Exclusive` for hot-journal recovery — +/// Returned by [`UnixVfsFile::lock_shared`]: holds this handle's own +/// place on [`inode_registry::SharedInodeLock`]'s process-wide ladder at +/// `Shared` (or, briefly, `Exclusive` for hot-journal recovery — /// [`FileLock::escalate_to_exclusive`]) until dropped. struct UnixLockGuard { - lock: Rc>, + handle: InodeLockHandle, path: PathBuf, } impl SharedLockGuard for UnixLockGuard { fn check_reserved(&self) -> Result { - self.lock - .borrow() + self.handle .check_reserved() .map_err(|source| to_vfs_error(&self.path, source)) } fn escalate_to_exclusive(&mut self) -> Result<()> { - self.lock - .borrow_mut() + self.handle .set_level(lock::LockLevel::Exclusive) .map_err(|source| to_lock_error(&self.path, source)) } fn de_escalate_to_shared(&mut self) -> Result<()> { - self.lock - .borrow_mut() + self.handle .set_level(lock::LockLevel::Shared) .map_err(|source| to_lock_error(&self.path, source)) } fn set_level(&mut self, level: lock::LockLevel) -> Result<()> { - self.lock - .borrow_mut() + self.handle .set_level(level) .map_err(|source| to_lock_error(&self.path, source)) } } -impl Drop for UnixLockGuard { - fn drop(&mut self) { - // Best-effort, matching `FileLockState`'s own `Drop`: a `drop` - // can't propagate failure, and there is nothing more to do about - // one anyway. The fd stays open via `UnixVfsFile`'s own `Rc` - // clone — only the lock level this guard represents is released. - self.lock - .borrow_mut() - .set_level(lock::LockLevel::Unlocked) - .ok(); - } -} - fn to_vfs_error(path: &Path, source: std::io::Error) -> VfsError { let path_str = path.display().to_string(); if source.kind() == std::io::ErrorKind::NotFound { diff --git a/tests/corpus/in_process_lock_registry_test.rs b/tests/corpus/in_process_lock_registry_test.rs new file mode 100644 index 0000000..c39ef9b --- /dev/null +++ b/tests/corpus/in_process_lock_registry_test.rs @@ -0,0 +1,289 @@ +// Copyright 2026 Schuberg Philis +// SPDX-License-Identifier: Apache-2.0 +//! #706: two `Connection`s (here, two independent `Pager`/`Vm` sessions +//! built directly over `UnixVfs`, without going through `src/api.rs` — +//! PR #705 isn't merged yet) opened on the same file **in the same +//! process** did not lock against each other. `BEGIN IMMEDIATE` on one +//! handle escalated only *its own* `FileLockState`'s fcntl lock; a second +//! handle's own, independently-opened fd on the same inode took the +//! conflicting fcntl lock too, since POSIX `fcntl` never conflicts with a +//! lock already held by the calling process — so the second handle's +//! write silently succeeded and the first handle's commit clobbered it on +//! disk. +//! +//! `src/vfs/inode_registry.rs` fixes this by sharing one process-wide +//! `SharedInodeLock` per `(device, inode)`, arbitrating in-process +//! requests before any real `fcntl` call. This file proves the fix at the +//! same altitude #491/#412 were investigating: real `Pager`s, real +//! `BEGIN IMMEDIATE`, no mocked locks. Schema setup goes through the +//! pinned oracle (like `begin_immediate_lock_interop_test.rs`), so every +//! test here is oracle-gated even though the reproduction itself is +//! entirely in-process. + +use std::cell::RefCell; +use std::path::{Path, PathBuf}; +use std::process::Command; +use std::rc::Rc; +use std::sync::atomic::{AtomicU64, Ordering}; + +use sqlite_rs::btree::TableCursor; +use sqlite_rs::codegen::compile_statement; +use sqlite_rs::header::DatabaseHeader; +use sqlite_rs::pager::Pager; +use sqlite_rs::schema::read_schema; +use sqlite_rs::vdbe::{execute_transaction_step, ExecError}; +use sqlite_rs::vfs::{PageSource, UnixVfs}; + +use crate::oracle::{assert_integrity_check_ok, oracle_list_output, pinned_oracle, skip_no_oracle}; + +fn scratch_db(label: &str) -> PathBuf { + static COUNTER: AtomicU64 = AtomicU64::new(0); + let n = COUNTER.fetch_add(1, Ordering::Relaxed); + let dir = std::env::temp_dir().join(format!( + "sqlite-rs-in-process-lock-registry-{label}-{}-{n}", + std::process::id() + )); + std::fs::create_dir_all(&dir).unwrap(); + dir.join("test.db") +} + +fn oracle_exec(oracle: &Path, db: &Path, sql: &str) { + let status = Command::new(oracle).arg(db).arg(sql).status().unwrap(); + assert!(status.success(), "oracle script failed: {sql}"); +} + +fn header_of(vfs: &UnixVfs, db: &Path, page_size: u32) -> DatabaseHeader { + let source = Pager::open(vfs, db, page_size).unwrap(); + let bytes = source.read_page(1).unwrap(); + let mut buf = [0u8; 100]; + buf.copy_from_slice(&bytes[..100]); + DatabaseHeader::parse(&buf).unwrap() +} + +/// A minimal in-process session: its own `Pager` opened independently +/// against the same path another session may already have open — the +/// exact "two `Connection`s, one file, one process" shape #706 named. +/// Schema is read fresh from disk at construction, same as +/// `begin_immediate_lock_interop_test.rs`'s `OurSession`; unlike that +/// type, `exec` here returns `Result` instead of unwrapping, so a test +/// can assert on the second handle's write being refused rather than +/// treat it as a panic-worthy bug. +struct Session { + pager: Rc>, + header: DatabaseHeader, + schemas: Vec, + autocommit: bool, +} + +impl Session { + fn open(vfs: &UnixVfs, db: &Path, page_size: u32) -> Self { + let header = header_of(vfs, db, page_size); + let pager = Rc::new(RefCell::new(Pager::open(vfs, db, page_size).unwrap())); + let schemas = { + let borrowed = pager.borrow(); + let mut schema_cursor = TableCursor::new(&*borrowed, &header, 1); + read_schema(&mut schema_cursor, header.text_encoding).unwrap() + }; + Session { + pager, + header, + schemas, + autocommit: true, + } + } + + fn exec(&mut self, stmt: &str) -> Result<(), ExecError> { + let program = compile_statement(stmt, &self.schemas, &[]).unwrap(); + let (_, autocommit) = execute_transaction_step( + &program, + Rc::clone(&self.pager), + self.header, + self.autocommit, + )?; + self.autocommit = autocommit; + Ok(()) + } +} + +/// Row count of table `t`, via `dump_database` (fresh `Pager::open` +/// each call, so it always reads current on-disk state rather than a +/// stale cached schema/header). +fn row_count(vfs: &UnixVfs, db: &Path) -> usize { + let result = sqlite_rs::dump::dump_database(vfs, db).unwrap(); + let table = result + .tables + .iter() + .find(|t| t.name == "t") + .expect("table t not found"); + table.rows.len() +} + +/// The issue's exact reproduction: A `BEGIN IMMEDIATE` + insert; B's +/// insert must be refused (blocked/busy), not silently accepted and then +/// discarded by A's commit. After A commits, the file holds both rows and +/// `integrity_check` passes. +#[test] +fn two_in_process_connections_on_one_file_serialize_a_write() { + let Some(oracle) = pinned_oracle() else { + skip_no_oracle("two_in_process_connections_on_one_file_serialize_a_write"); + return; + }; + + let db = scratch_db("two-connections"); + oracle_exec( + &oracle, + &db, + "create table t(a integer); insert into t values (1);", + ); + + let vfs = UnixVfs; + let mut session_a = Session::open(&vfs, &db, 4096); + session_a.exec("BEGIN IMMEDIATE").unwrap(); + session_a.exec("INSERT INTO t VALUES (2)").unwrap(); + + // Session B is a second, independent `Pager` on the very same path, + // in this very same process — the exact shape that used to lose + // data silently. + let mut session_b = Session::open(&vfs, &db, 4096); + let b_result = session_b.exec("INSERT INTO t VALUES (3)"); + assert!( + b_result.is_err(), + "a second in-process connection's write must not succeed while the \ + first holds BEGIN IMMEDIATE — got Ok, meaning it was silently \ + accepted and would be discarded on A's commit" + ); + + // B releases its handle (as a pool would return a failed borrow) + // before A commits — a still-open reader would legitimately block A's + // own EXCLUSIVE commit escalation in rollback-journal mode (real + // stock sqlite3 behavior, not something #706 changes); this test is + // about B's *write* being refused, not about reader/writer commit + // ordering. + drop(session_b); + session_a.exec("COMMIT").unwrap(); + drop(session_a); + + assert_integrity_check_ok(&oracle, &db); + let rows = oracle_list_output(&oracle, &db, "t", &["a".to_string()]); + assert_eq!(rows.trim(), "1\n2", "row 3 must never have been written"); + assert_eq!(row_count(&vfs, &db), 2); +} + +/// Regression guard: the same sequence, but B is a real second *process* +/// (stock `sqlite3`) rather than a second in-process handle — this must +/// keep behaving exactly as it did before #706 (cross-process locking was +/// already correct; the fix must not break it while closing the +/// in-process gap). +#[test] +fn cross_process_locking_still_works_after_the_in_process_fix() { + let Some(oracle) = pinned_oracle() else { + skip_no_oracle("cross_process_locking_still_works_after_the_in_process_fix"); + return; + }; + + let db = scratch_db("cross-process-regression"); + oracle_exec( + &oracle, + &db, + "create table t(a integer); insert into t values (1);", + ); + + let vfs = UnixVfs; + let mut session_a = Session::open(&vfs, &db, 4096); + session_a.exec("BEGIN IMMEDIATE").unwrap(); + session_a.exec("INSERT INTO t VALUES (2)").unwrap(); + + let output = Command::new(&oracle) + .arg(&db) + .arg("insert into t values (3);") + .output() + .unwrap(); + assert!( + !output.status.success(), + "a concurrent stock sqlite3 write must still be blocked by our BEGIN IMMEDIATE" + ); + + session_a.exec("COMMIT").unwrap(); + drop(session_a); + + let status = Command::new(&oracle) + .arg(&db) + .arg("insert into t values (3);") + .status() + .unwrap(); + assert!( + status.success(), + "sqlite3 write must succeed once our BEGIN IMMEDIATE's lock is released" + ); + + assert_integrity_check_ok(&oracle, &db); +} + +/// Lifecycle: closing every in-process handle on a file, then reopening +/// it, must not leave the registry wedged (stale lock state, or a +/// leaked-forever entry) — the fresh handle gets a fully-`Unlocked` +/// ladder and a plain write succeeds. +#[test] +fn registry_entry_does_not_survive_past_its_last_handle() { + let Some(oracle) = pinned_oracle() else { + skip_no_oracle("registry_entry_does_not_survive_past_its_last_handle"); + return; + }; + + let db = scratch_db("lifecycle"); + oracle_exec( + &oracle, + &db, + "create table t(a integer); insert into t values (1);", + ); + + let vfs = UnixVfs; + { + let mut session = Session::open(&vfs, &db, 4096); + session.exec("BEGIN IMMEDIATE").unwrap(); + session.exec("INSERT INTO t VALUES (2)").unwrap(); + session.exec("COMMIT").unwrap(); + // `session` drops here — every handle on this inode is now gone. + } + + // A brand-new handle must see a clean, unlocked ladder: BEGIN + // IMMEDIATE succeeds immediately rather than reporting stale + // contention from the handle that just closed. + let mut reopened = Session::open(&vfs, &db, 4096); + reopened.exec("BEGIN IMMEDIATE").unwrap(); + reopened.exec("INSERT INTO t VALUES (3)").unwrap(); + reopened.exec("COMMIT").unwrap(); + drop(reopened); + + assert_integrity_check_ok(&oracle, &db); + assert_eq!(row_count(&vfs, &db), 3); +} + +/// A pool of N in-process handles on one file must make progress: each +/// one opens, writes serially (waiting its turn is out of scope — this +/// crate's locks are non-blocking `F_SETLK`, so a caller retries rather +/// than the lock itself blocking), and none of them deadlocks or wedges +/// the registry for the next. +#[test] +fn a_pool_of_in_process_handles_makes_progress() { + let Some(oracle) = pinned_oracle() else { + skip_no_oracle("a_pool_of_in_process_handles_makes_progress"); + return; + }; + + let db = scratch_db("pool-progress"); + oracle_exec(&oracle, &db, "create table t(a integer);"); + + let vfs = UnixVfs; + for i in 0..5 { + let mut session = Session::open(&vfs, &db, 4096); + session.exec("BEGIN IMMEDIATE").unwrap(); + session + .exec(&format!("INSERT INTO t VALUES ({i})")) + .unwrap(); + session.exec("COMMIT").unwrap(); + } + + assert_integrity_check_ok(&oracle, &db); + assert_eq!(row_count(&vfs, &db), 5); +} diff --git a/tests/corpus/main.rs b/tests/corpus/main.rs index 6d39a9c..561c09d 100644 --- a/tests/corpus/main.rs +++ b/tests/corpus/main.rs @@ -35,6 +35,7 @@ mod families_test; mod group_by_projection_test; mod harness_test; mod hash_group_by_test; +mod in_process_lock_registry_test; mod index_maintenance_test; mod index_ordered_group_by_test; mod index_ordered_scan_test;