From 8b42c28bbaddcb039aae9c6f8a63c625eff1833e Mon Sep 17 00:00:00 2001 From: Zack Jackson <25274700+ScriptedAlchemy@users.noreply.github.com> Date: Tue, 3 Nov 2026 00:46:11 -0800 Subject: [PATCH 1/5] fix(storage): preserve SQLite locks during pathname identity checks Use pathname metadata on Unix instead of opening and closing a raw file descriptor, which releases process-owned SQLite record locks. Keep the pinned identity and both HAS_MOVED-adjacent checks intact. Add cross-process rollback and WAL lock regressions, normal-release and integrity checks, and missing/replaced/alias identity coverage. Refs #4513 --- .../src/connection/mod.rs | 21 +- .../src/connection/path_identity_tests.rs | 194 ++++++++++++++++++ 2 files changed, 213 insertions(+), 2 deletions(-) create mode 100644 crates/tracedecay-rusqlite-runtime/src/connection/path_identity_tests.rs diff --git a/crates/tracedecay-rusqlite-runtime/src/connection/mod.rs b/crates/tracedecay-rusqlite-runtime/src/connection/mod.rs index 0f4af77d0b..a179973b5e 100644 --- a/crates/tracedecay-rusqlite-runtime/src/connection/mod.rs +++ b/crates/tracedecay-rusqlite-runtime/src/connection/mod.rs @@ -201,8 +201,22 @@ impl OpenedDatabaseFile { } pub(crate) fn verify_current_path(&self, path: &Path) -> Result<(), OpenedDatabaseFileError> { - let current = File::open(path).map_err(|_| OpenedDatabaseFileError::Open)?; - if opened_file_identity(¤t)? != self.identity { + // POSIX record locks belong to the process: closing any descriptor + // for this inode releases SQLite's locks, even on another connection. + // Inspect the pathname without opening a temporary descriptor. Keep + // using the pinned descriptor's dev/inode identity, and retain both + // checks around SQLITE_FCNTL_HAS_MOVED in verify_connection. + #[cfg(unix)] + let current_identity = { + let current = std::fs::metadata(path).map_err(|_| OpenedDatabaseFileError::Open)?; + metadata_identity(¤t) + }; + #[cfg(not(unix))] + let current_identity = { + let current = File::open(path).map_err(|_| OpenedDatabaseFileError::Open)?; + opened_file_identity(¤t)? + }; + if current_identity != self.identity { return Err(OpenedDatabaseFileError::Replaced); } Ok(()) @@ -1269,5 +1283,8 @@ fn policy(stage: &'static str, source: rusqlite::Error) -> ConnectionPolicyError ConnectionPolicyError { stage, source } } +#[cfg(all(test, unix))] +mod path_identity_tests; + #[cfg(test)] mod tests; diff --git a/crates/tracedecay-rusqlite-runtime/src/connection/path_identity_tests.rs b/crates/tracedecay-rusqlite-runtime/src/connection/path_identity_tests.rs new file mode 100644 index 0000000000..02a3866088 --- /dev/null +++ b/crates/tracedecay-rusqlite-runtime/src/connection/path_identity_tests.rs @@ -0,0 +1,194 @@ +use std::{env, fs, path::Path, process::Command, time::Duration}; + +use rusqlite::{Connection, Error, ErrorCode, OpenFlags}; +use tempfile::TempDir; + +use super::{OpenedDatabaseFile, OpenedDatabaseFileError}; + +const PROBE_PATH: &str = "TRACEDECAY_PATH_IDENTITY_PROBE_PATH"; +const PROBE_OPERATION: &str = "TRACEDECAY_PATH_IDENTITY_PROBE_OPERATION"; +const PROBE_EXPECTATION: &str = "TRACEDECAY_PATH_IDENTITY_PROBE_EXPECTATION"; +const PROBE_COMPLETE: &str = "tracedecay-path-identity-probe-complete"; + +// A second connection in this process is not a lock probe: SQLite's Unix VFS +// shares inode/lock bookkeeping within a process and can hide a lost OS lock. +// Execute only the child test in a fresh process, with a marker to reject a +// successful test harness invocation that accidentally selected zero tests. +fn probe_external_connection(path: &Path, operation: &str, blocked: bool) { + let (_, module) = module_path!().split_once("::").expect("test module path"); + let output = Command::new(env::current_exe().expect("test executable")) + .args([ + "--exact", + &format!("{module}::external_connection_probe"), + "--nocapture", + ]) + .env(PROBE_PATH, path) + .env(PROBE_OPERATION, operation) + .env(PROBE_EXPECTATION, if blocked { "blocked" } else { "allowed" }) + .output() + .expect("run external SQLite connection"); + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + output.status.success() && stdout.contains(PROBE_COMPLETE), + "external {operation} probe failed: {}\n{stdout}\n{stderr}", + output.status, + ); +} + +#[test] +fn external_connection_probe() { + let Some(path) = env::var_os(PROBE_PATH) else { + return; + }; + let operation = env::var(PROBE_OPERATION).expect("probe operation"); + let expectation = env::var(PROBE_EXPECTATION).expect("probe expectation"); + let connection = Connection::open_with_flags( + Path::new(&path), + OpenFlags::SQLITE_OPEN_READ_WRITE | OpenFlags::SQLITE_OPEN_NO_MUTEX, + ) + .expect("open existing database in another process"); + connection.busy_timeout(Duration::ZERO).expect("no lock wait"); + let result = match operation.as_str() { + "write" => connection.execute_batch("BEGIN IMMEDIATE; ROLLBACK"), + "journal" => connection + .query_row("PRAGMA journal_mode = DELETE", [], |row| { + row.get::<_, String>(0) + }) + .map(|mode| assert_eq!(mode, "delete")), + _ => panic!("unknown probe operation: {operation}"), + }; + match expectation.as_str() { + "blocked" => assert!( + matches!( + &result, + Err(Error::SqliteFailure(error, _)) if error.code == ErrorCode::DatabaseBusy + ), + "expected SQLITE_BUSY from external {operation}, got {result:?}", + ), + "allowed" => result.expect("external operation after lock release"), + _ => panic!("unknown probe expectation: {expectation}"), + } + println!("{PROBE_COMPLETE}"); +} + +fn assert_committed_data(path: &Path) { + let connection = Connection::open_with_flags(path, OpenFlags::SQLITE_OPEN_READ_ONLY) + .expect("reopen committed database"); + let value: i64 = connection + .query_row("SELECT value FROM sample", [], |row| row.get(0)) + .expect("committed value"); + assert_eq!(value, 2); + let integrity: String = connection + .query_row("PRAGMA integrity_check", [], |row| row.get(0)) + .expect("integrity check"); + assert_eq!(integrity, "ok"); +} + +#[test] +fn path_checks_preserve_rollback_writer_lock() { + let temp = TempDir::new().expect("tempdir"); + let path = temp.path().join("rollback.db"); + // Pin before SQLite takes locks, and keep the pin until after it closes. + // This isolates pathname verification from the separate pin-drop lifetime. + let pinned = OpenedDatabaseFile::create_new(&path).expect("pin database"); + let connection = Connection::open(&path).expect("open database"); + connection + .execute_batch( + "PRAGMA journal_mode = DELETE; + CREATE TABLE sample(value INTEGER); + INSERT INTO sample VALUES (1); + BEGIN IMMEDIATE; + UPDATE sample SET value = 2;", + ) + .expect("hold write transaction"); + probe_external_connection(&path, "write", true); + + pinned.verify_current_path(&path).expect("same inode"); + probe_external_connection(&path, "write", true); + pinned + .verify_connection(&connection, &path) + .expect("same SQLite connection"); + probe_external_connection(&path, "write", true); + + connection.execute_batch("COMMIT").expect("commit"); + probe_external_connection(&path, "write", false); + drop(connection); + drop(pinned); + assert_committed_data(&path); +} + +#[test] +fn path_checks_preserve_live_wal_journal_mode_guard() { + let temp = TempDir::new().expect("tempdir"); + let path = temp.path().join("wal.db"); + let pinned = OpenedDatabaseFile::create_new(&path).expect("pin database"); + let connection = Connection::open(&path).expect("open database"); + connection + .execute_batch( + "PRAGMA journal_mode = WAL; + CREATE TABLE sample(value INTEGER); + INSERT INTO sample VALUES (1); + BEGIN IMMEDIATE; + UPDATE sample SET value = 2;", + ) + .expect("hold WAL write transaction"); + probe_external_connection(&path, "journal", true); + + pinned.verify_current_path(&path).expect("same inode"); + probe_external_connection(&path, "journal", true); + pinned + .verify_connection(&connection, &path) + .expect("same SQLite connection"); + probe_external_connection(&path, "journal", true); + + connection.execute_batch("COMMIT").expect("commit"); + // A live WAL connection still prevents another process switching modes, + // even between transactions. Only closing the connection releases it. + probe_external_connection(&path, "journal", true); + drop(connection); + probe_external_connection(&path, "journal", false); + drop(pinned); + assert_committed_data(&path); +} + +#[test] +fn path_checks_keep_pinned_identity_and_reject_missing_or_replaced_files() { + let temp = TempDir::new().expect("tempdir"); + let path = temp.path().join("identity.db"); + let alias = temp.path().join("same-inode.db"); + let symlink = temp.path().join("symlink.db"); + let pinned = OpenedDatabaseFile::create_new(&path).expect("pin database"); + fs::hard_link(&path, &alias).expect("hard link"); + std::os::unix::fs::symlink(&path, &symlink).expect("symlink"); + pinned.verify_current_path(&path).expect("original inode"); + pinned.verify_current_path(&alias).expect("hard-link inode"); + pinned.verify_current_path(&symlink).expect("symlink target"); + + fs::remove_file(&path).expect("unlink original path"); + assert_eq!( + pinned.verify_current_path(&path), + Err(OpenedDatabaseFileError::Open), + ); + assert_eq!( + pinned.verify_current_path(&symlink), + Err(OpenedDatabaseFileError::Open), + ); + fs::write(&path, b"").expect("create different inode"); + assert_eq!( + pinned.verify_current_path(&path), + Err(OpenedDatabaseFileError::Replaced), + ); + assert_eq!( + pinned.verify_current_path(&symlink), + Err(OpenedDatabaseFileError::Replaced), + ); + pinned.verify_current_path(&alias).expect("retained inode"); + + fs::remove_file(&path).expect("remove replacement"); + fs::create_dir(&path).expect("directory at database path"); + assert_eq!( + pinned.verify_current_path(&path), + Err(OpenedDatabaseFileError::Replaced), + ); +} From 0ad23db0bcd658fc5288c72f99e1d8edfc3fe1bf Mon Sep 17 00:00:00 2001 From: Zack Jackson <25274700+ScriptedAlchemy@users.noreply.github.com> Date: Tue, 3 Nov 2026 02:19:50 -0800 Subject: [PATCH 2/5] style(storage): format pathname identity regressions --- .../src/connection/path_identity_tests.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/crates/tracedecay-rusqlite-runtime/src/connection/path_identity_tests.rs b/crates/tracedecay-rusqlite-runtime/src/connection/path_identity_tests.rs index 02a3866088..0ecac08281 100644 --- a/crates/tracedecay-rusqlite-runtime/src/connection/path_identity_tests.rs +++ b/crates/tracedecay-rusqlite-runtime/src/connection/path_identity_tests.rs @@ -48,7 +48,9 @@ fn external_connection_probe() { OpenFlags::SQLITE_OPEN_READ_WRITE | OpenFlags::SQLITE_OPEN_NO_MUTEX, ) .expect("open existing database in another process"); - connection.busy_timeout(Duration::ZERO).expect("no lock wait"); + connection + .busy_timeout(Duration::ZERO) + .expect("no lock wait"); let result = match operation.as_str() { "write" => connection.execute_batch("BEGIN IMMEDIATE; ROLLBACK"), "journal" => connection @@ -163,7 +165,9 @@ fn path_checks_keep_pinned_identity_and_reject_missing_or_replaced_files() { std::os::unix::fs::symlink(&path, &symlink).expect("symlink"); pinned.verify_current_path(&path).expect("original inode"); pinned.verify_current_path(&alias).expect("hard-link inode"); - pinned.verify_current_path(&symlink).expect("symlink target"); + pinned + .verify_current_path(&symlink) + .expect("symlink target"); fs::remove_file(&path).expect("unlink original path"); assert_eq!( From 77278fa3f04fa049cdd4b40b17d12b538501cd17 Mon Sep 17 00:00:00 2001 From: Zack Jackson <25274700+ScriptedAlchemy@users.noreply.github.com> Date: Tue, 3 Nov 2026 04:30:20 -0800 Subject: [PATCH 3/5] style: rustfmt the SQLite path identity tests Co-Authored-By: Claude Opus 5.5 --- .../src/connection/path_identity_tests.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/crates/tracedecay-rusqlite-runtime/src/connection/path_identity_tests.rs b/crates/tracedecay-rusqlite-runtime/src/connection/path_identity_tests.rs index 0ecac08281..83bc8ac803 100644 --- a/crates/tracedecay-rusqlite-runtime/src/connection/path_identity_tests.rs +++ b/crates/tracedecay-rusqlite-runtime/src/connection/path_identity_tests.rs @@ -24,7 +24,10 @@ fn probe_external_connection(path: &Path, operation: &str, blocked: bool) { ]) .env(PROBE_PATH, path) .env(PROBE_OPERATION, operation) - .env(PROBE_EXPECTATION, if blocked { "blocked" } else { "allowed" }) + .env( + PROBE_EXPECTATION, + if blocked { "blocked" } else { "allowed" }, + ) .output() .expect("run external SQLite connection"); let stdout = String::from_utf8_lossy(&output.stdout); From d18cbd5f7d88ddce7c71df5bcbe0e5dc3008c6e6 Mon Sep 17 00:00:00 2001 From: Zack Jackson <25274700+ScriptedAlchemy@users.noreply.github.com> Date: Tue, 3 Nov 2026 11:11:48 -0800 Subject: [PATCH 4/5] fix(sqlite): inspect live worker identity without closing inode FDs Replace ephemeral raw file opens in both SQLite reader and writer startup with a metadata-only identity check on Unix. Closing those descriptors while SQLite is live releases process-wide POSIX locks without VFS bookkeeping. Keep existing non-Unix identity handling and typed refusal paths. Add a subprocess regression proving an independent SQLite writer stays excluded across repeated worker identity checks, then commits intact. Refs #4513. --- .../src/connection/mod.rs | 19 ++++++++++ .../src/connection/path_identity_tests.rs | 37 ++++++++++++++++++- .../src/reader/worker.rs | 6 +-- .../src/writer/worker/mod.rs | 4 +- 4 files changed, 60 insertions(+), 6 deletions(-) diff --git a/crates/tracedecay-rusqlite-runtime/src/connection/mod.rs b/crates/tracedecay-rusqlite-runtime/src/connection/mod.rs index a179973b5e..0ca75d6616 100644 --- a/crates/tracedecay-rusqlite-runtime/src/connection/mod.rs +++ b/crates/tracedecay-rusqlite-runtime/src/connection/mod.rs @@ -290,6 +290,25 @@ impl OpenedDatabaseFile { } } +/// Check a live SQLite worker's physical file without opening and closing a +/// second descriptor for the same inode on Unix. Closing that descriptor can +/// release process-wide SQLite locks owned by other connections, even though +/// SQLite's VFS did not perform the close. +pub(crate) fn live_path_identity(path: &Path) -> Result { + #[cfg(unix)] + { + let metadata = std::fs::metadata(path).map_err(|_| OpenedDatabaseFileError::Open)?; + if !metadata.is_file() { + return Err(OpenedDatabaseFileError::NotFile); + } + Ok(metadata_identity(&metadata)) + } + #[cfg(not(unix))] + { + OpenedDatabaseFile::pin(path).map(|opened| opened.identity()) + } +} + #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub enum OpenedDatabaseFileError { Create, diff --git a/crates/tracedecay-rusqlite-runtime/src/connection/path_identity_tests.rs b/crates/tracedecay-rusqlite-runtime/src/connection/path_identity_tests.rs index 83bc8ac803..4b61e3dc69 100644 --- a/crates/tracedecay-rusqlite-runtime/src/connection/path_identity_tests.rs +++ b/crates/tracedecay-rusqlite-runtime/src/connection/path_identity_tests.rs @@ -3,7 +3,7 @@ use std::{env, fs, path::Path, process::Command, time::Duration}; use rusqlite::{Connection, Error, ErrorCode, OpenFlags}; use tempfile::TempDir; -use super::{OpenedDatabaseFile, OpenedDatabaseFileError}; +use super::{OpenedDatabaseFile, OpenedDatabaseFileError, live_path_identity}; const PROBE_PATH: &str = "TRACEDECAY_PATH_IDENTITY_PROBE_PATH"; const PROBE_OPERATION: &str = "TRACEDECAY_PATH_IDENTITY_PROBE_OPERATION"; @@ -90,6 +90,41 @@ fn assert_committed_data(path: &Path) { assert_eq!(integrity, "ok"); } +#[test] +fn worker_identity_checks_keep_cross_process_writer_exclusion() { + let temp = TempDir::new().expect("tempdir"); + let path = temp.path().join("worker.db"); + let connection = Connection::open(&path).expect("open database"); + connection + .execute_batch( + "PRAGMA journal_mode = DELETE; + CREATE TABLE sample(value INTEGER); + INSERT INTO sample VALUES (1); + BEGIN IMMEDIATE; + UPDATE sample SET value = 2;", + ) + .expect("hold live SQLite writer lock"); + // The old worker check opened another raw descriptor and dropped it + // immediately. On POSIX that released this transaction's advisory lock. + probe_external_connection(&path, "write", true); + let identity = live_path_identity(&path).expect("inspect live worker identity"); + assert_ne!(identity, 0); + probe_external_connection(&path, "write", true); + assert_eq!(live_path_identity(&path).unwrap(), identity); + probe_external_connection(&path, "write", true); + + connection.execute_batch("COMMIT").expect("commit"); + probe_external_connection(&path, "write", false); + drop(connection); + assert_committed_data(&path); + + let absent = temp.path().join("absent.db"); + assert_eq!(live_path_identity(&absent), Err(OpenedDatabaseFileError::Open)); + let directory = temp.path().join("not-a-database"); + fs::create_dir(&directory).expect("directory"); + assert_eq!(live_path_identity(&directory), Err(OpenedDatabaseFileError::NotFile)); +} + #[test] fn path_checks_preserve_rollback_writer_lock() { let temp = TempDir::new().expect("tempdir"); diff --git a/crates/tracedecay-rusqlite-runtime/src/reader/worker.rs b/crates/tracedecay-rusqlite-runtime/src/reader/worker.rs index 34bbd20ab0..9edee1472e 100644 --- a/crates/tracedecay-rusqlite-runtime/src/reader/worker.rs +++ b/crates/tracedecay-rusqlite-runtime/src/reader/worker.rs @@ -18,7 +18,7 @@ use tracedecay_store::{ StorageRuntimeErrorV1, UnavailableReasonV1, }; -use crate::connection::{self, ConnectionMode, OpenedDatabaseFile}; +use crate::connection::{self, ConnectionMode}; use crate::exact_sql::{ ExactSqlError, ExactSqlRows, ExactSqlStatement, ExactSqlStatementBudget, execute_query, read_across_wal_recovery, @@ -516,8 +516,8 @@ pub(crate) fn spawn( let _ = started.send(Err(error)); return; } - let opened_file_identity = match OpenedDatabaseFile::pin(&worker_open_path) { - Ok(opened) => opened.identity(), + let opened_file_identity = match connection::live_path_identity(&worker_open_path) { + Ok(identity) => identity, Err(error) => { let _ = started.send(Err(ReaderStartError::OpenedDatabaseIdentity(error))); return; diff --git a/crates/tracedecay-rusqlite-runtime/src/writer/worker/mod.rs b/crates/tracedecay-rusqlite-runtime/src/writer/worker/mod.rs index 92ba85b5c1..1e6ea25489 100644 --- a/crates/tracedecay-rusqlite-runtime/src/writer/worker/mod.rs +++ b/crates/tracedecay-rusqlite-runtime/src/writer/worker/mod.rs @@ -434,8 +434,8 @@ impl Worker { } let opened_file_identity = match self.expected_file_identity { Some(expected) => { - let actual = match OpenedDatabaseFile::pin(&self.path) { - Ok(opened) => opened.identity(), + let actual = match connection::live_path_identity(&self.path) { + Ok(identity) => identity, Err(error) => { return self.fail_start(WriterStartError::OpenedDatabaseIdentity(error)); } From 7a4dabdd0eb521ca2e7c7f03cbe6f6fbb2b466a7 Mon Sep 17 00:00:00 2001 From: Zack Jackson <25274700+ScriptedAlchemy@users.noreply.github.com> Date: Tue, 3 Nov 2026 12:16:53 -0800 Subject: [PATCH 5/5] style(storage): format SQLite path identity regressions Apply the exact Bazel rustfmt_check_2024 diff reported in repository gates run 41701101003. Preserve the cross-process SQLite exclusion and pathname assertions unchanged. --- .../src/connection/path_identity_tests.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/crates/tracedecay-rusqlite-runtime/src/connection/path_identity_tests.rs b/crates/tracedecay-rusqlite-runtime/src/connection/path_identity_tests.rs index 4b61e3dc69..b024e5a454 100644 --- a/crates/tracedecay-rusqlite-runtime/src/connection/path_identity_tests.rs +++ b/crates/tracedecay-rusqlite-runtime/src/connection/path_identity_tests.rs @@ -119,10 +119,16 @@ fn worker_identity_checks_keep_cross_process_writer_exclusion() { assert_committed_data(&path); let absent = temp.path().join("absent.db"); - assert_eq!(live_path_identity(&absent), Err(OpenedDatabaseFileError::Open)); + assert_eq!( + live_path_identity(&absent), + Err(OpenedDatabaseFileError::Open) + ); let directory = temp.path().join("not-a-database"); fs::create_dir(&directory).expect("directory"); - assert_eq!(live_path_identity(&directory), Err(OpenedDatabaseFileError::NotFile)); + assert_eq!( + live_path_identity(&directory), + Err(OpenedDatabaseFileError::NotFile) + ); } #[test]