diff --git a/crates/tracedecay-rusqlite-runtime/src/connection/mod.rs b/crates/tracedecay-rusqlite-runtime/src/connection/mod.rs index fd4dae1b6ff..74df7011a33 100644 --- a/crates/tracedecay-rusqlite-runtime/src/connection/mod.rs +++ b/crates/tracedecay-rusqlite-runtime/src/connection/mod.rs @@ -262,7 +262,7 @@ impl OpenedDatabaseFile { // HAS_MOVED below still observes the SQLite handle's different inode. self.verify_current_path(canonical_path)?; #[cfg(unix)] - if sqlite_connection_has_moved(_connection)? { + if sqlite_connection_has_moved(_connection)?.unwrap_or(false) { return Err(OpenedDatabaseFileError::Replaced); } #[cfg(unix)] @@ -416,7 +416,9 @@ fn metadata_identity(metadata: &std::fs::Metadata) -> u64 { } #[cfg(unix)] -fn sqlite_connection_has_moved(connection: &Connection) -> Result { +fn sqlite_connection_has_moved( + connection: &Connection, +) -> Result, OpenedDatabaseFileError> { let database_name = CString::new("main").expect("static SQLite database name"); let mut moved = 0_i32; // SAFETY: `connection` owns a live SQLite handle, `database_name` is a @@ -431,10 +433,10 @@ fn sqlite_connection_has_moved(connection: &Connection) -> Result Ok(moved != 0), - // VFS implementations predating HAS_MOVED report NOTFOUND. The - // pinned dev/inode check remains authoritative in that case. - rusqlite::ffi::SQLITE_NOTFOUND => Ok(false), + rusqlite::ffi::SQLITE_OK => Ok(Some(moved != 0)), + // Workers with a retained raw pin may use the pinned identity when + // HAS_MOVED is unsupported. Unpinned verified readers must refuse it. + rusqlite::ffi::SQLITE_NOTFOUND => Ok(None), _ => Err(OpenedDatabaseFileError::Inspect), } } @@ -606,12 +608,13 @@ impl std::error::Error for VerifiedReaderError { /// Foreign-database reader bound to the physical file actually /// opened by SQLite rather than a later pathname observation. pub struct VerifiedReader { - // Keep the connection before the raw pin in declaration order: closing - // any raw descriptor on Unix can release another SQLite connection's - // process-wide record locks. SQLite must close its handle first. connection: Connection, canonical_path: PathBuf, file_identity: u64, + // On Unix a raw pin's eventual close would release POSIX record locks + // belonging to unrelated SQLite connections on the same inode. The + // SQLite handle itself retains the file; Windows keeps its pin. + #[cfg(not(unix))] _opened_file: OpenedDatabaseFile, } @@ -632,8 +635,8 @@ impl VerifiedReader { self.file_identity } - /// The verified canonical pathname. It remains bound to this reader's - /// retained physical file until the SQLite connection is closed. + /// The verified canonical pathname, bound to the SQLite connection's + /// physical file until the connection is closed. pub fn canonical_path(&self) -> &Path { &self.canonical_path } @@ -862,20 +865,54 @@ fn open_verified_reader( after_open: impl FnOnce(), ) -> Result { let canonical_path = path.canonicalize().map_err(VerifiedReaderError::Resolve)?; + // Opening and later closing even a read-only raw descriptor on Unix + // releases POSIX record locks of other live SQLite connections to this + // inode. Observe the pathname without opening it and let SQLite own its + // only descriptor. Check identity on both sides of HAS_MOVED to reject + // a concurrent replacement, including a swap restored after open. + #[cfg(unix)] + let file_identity = + live_path_identity(&canonical_path).map_err(VerifiedReaderError::Identity)?; + #[cfg(unix)] + let open_path = canonical_path.clone(); + + #[cfg(not(unix))] let pinned = OpenedDatabaseFile::pin(&canonical_path).map_err(VerifiedReaderError::Identity)?; + #[cfg(not(unix))] let open_path = pinned .reader_open_path(&canonical_path) .map_err(VerifiedReaderError::Identity)?; + #[cfg(not(unix))] + let file_identity = pinned.identity(); + after_pin(); let connection = open_reader(&open_path).map_err(VerifiedReaderError::Policy)?; after_open(); + + #[cfg(unix)] + { + let before = live_path_identity(&canonical_path).map_err(VerifiedReaderError::Identity)?; + let moved = sqlite_connection_has_moved(&connection) + .map_err(VerifiedReaderError::Identity)? + .ok_or(VerifiedReaderError::Identity( + OpenedDatabaseFileError::Inspect, + ))?; + let after = live_path_identity(&canonical_path).map_err(VerifiedReaderError::Identity)?; + if before != file_identity || moved || after != file_identity { + return Err(VerifiedReaderError::Identity( + OpenedDatabaseFileError::Replaced, + )); + } + } + #[cfg(not(unix))] pinned .verify_connection(&connection, &canonical_path) .map_err(VerifiedReaderError::Identity)?; Ok(VerifiedReader { connection, canonical_path, - file_identity: pinned.identity(), + file_identity, + #[cfg(not(unix))] _opened_file: pinned, }) } 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 a8c0ce5fcf0..0f5b3a5eb91 100644 --- a/crates/tracedecay-rusqlite-runtime/src/connection/path_identity_tests.rs +++ b/crates/tracedecay-rusqlite-runtime/src/connection/path_identity_tests.rs @@ -377,3 +377,71 @@ fn path_checks_keep_pinned_identity_and_reject_missing_or_replaced_files() { Err(OpenedDatabaseFileError::Replaced), ); } + +/// Closing a verified read snapshot must not release another SQLite +/// connection's process-wide POSIX locks. A second connection in this process +/// cannot prove this: run the lock/journal contender in a separate process. +#[test] +fn verified_reader_release_preserves_independent_writer_exclusion() { + for journal in ["DELETE", "WAL"] { + for immutable in [false, true] { + let temp = TempDir::new().expect("tempdir"); + let path = temp.path().join("sessions.db"); + let writer = Connection::open(&path).expect("writer"); + writer + .execute_batch(&format!( + "PRAGMA journal_mode = {journal}; + CREATE TABLE sample(value INTEGER); + INSERT INTO sample VALUES (1);" + )) + .expect("seed committed SQLite row"); + if journal == "WAL" && immutable { + // Immutable readers cannot rely on uncheckpointed WAL content. + // Give this variant a proven, durable committed baseline; + // the ordinary WAL snapshot variant stays uncheckpointed. + writer + .execute_batch("PRAGMA wal_checkpoint(TRUNCATE)") + .expect("checkpoint immutable reader baseline"); + } + writer + .execute_batch("BEGIN IMMEDIATE; UPDATE sample SET value = 2;") + .expect("hold SQLite writer lock"); + let operation = if journal == "WAL" { "journal" } else { "write" }; + probe_external_connection(&path, operation, true); + + let reader = if immutable { + super::open_verified_immutable_reader(&path) + } else { + super::open_verified_read_snapshot(&path) + } + .expect("verified read-only connection"); + assert_eq!(reader.file_identity(), live_path_identity(&path).unwrap()); + assert_eq!( + reader + .connection() + .query_row("SELECT value FROM sample", [], |row| row.get::<_, i64>(0)), + Ok(1), + "both verified reader policies must see committed rows, never the pending update" + ); + probe_external_connection_expectation(&path, "read", "read:1"); + drop(reader); + probe_external_connection(&path, operation, true); + // Reader destruction must not lose the writer's lock or + // prevent another process from reading the committed snapshot. + probe_external_connection_expectation(&path, "read", "read:1"); + + writer.execute_batch("COMMIT").expect("commit writer"); + probe_external_connection_expectation(&path, "read", "read:2"); + if journal == "WAL" { + probe_external_connection(&path, "journal", true); + } else { + probe_external_connection(&path, "write", false); + } + drop(writer); + if journal == "WAL" { + probe_external_connection(&path, "journal", false); + } + assert_committed_data(&path); + } + } +} diff --git a/crates/tracedecay-sessions/src/runtime/host_sqlite.rs b/crates/tracedecay-sessions/src/runtime/host_sqlite.rs index b48ccd3cf04..ad1405e428d 100644 --- a/crates/tracedecay-sessions/src/runtime/host_sqlite.rs +++ b/crates/tracedecay-sessions/src/runtime/host_sqlite.rs @@ -1,8 +1,8 @@ use std::sync::{Arc, Mutex, PoisonError}; -/// A plain SQLite read or a verified foreign read. The verified case retains -/// its inode pin for the whole connection lifetime instead of unpacking and -/// dropping it immediately after admission. +/// A plain SQLite read or a verified foreign read. The verified case keeps +/// the SQLite connection alive through the last cloned handle. Unix must not +/// retain an additional raw inode pin: closing it releases unrelated locks. enum SqliteReadHandle { Plain(rusqlite::Connection), Verified(tracedecay_rusqlite_runtime::VerifiedReader), @@ -72,7 +72,7 @@ mod tests { } #[test] - fn verified_foreign_reader_retains_its_pin_until_the_last_handle_closes() { + fn verified_foreign_reader_keeps_its_sqlite_handle_until_last_clone_closes() { let dir = tempfile::tempdir().expect("test directory"); let path = dir.path().join("foreign.sqlite3"); { @@ -87,9 +87,9 @@ mod tests { let verified = tracedecay_rusqlite_runtime::open_verified_immutable_reader(&path) .expect("open verified foreign database"); let pinned_descriptors = descriptors_for(&path); - assert!( - pinned_descriptors >= 2, - "SQLite and its verified identity pin each retain an inode handle" + assert_eq!( + pinned_descriptors, 1, + "verified Unix reads must not open a second raw descriptor to the SQLite inode" ); let reader = SqliteReadConn::new_verified(verified); diff --git a/crates/tracedecay/tests/automation_runner_test/jobs.rs b/crates/tracedecay/tests/automation_runner_test/jobs.rs index 740283d56e8..7342786381b 100644 --- a/crates/tracedecay/tests/automation_runner_test/jobs.rs +++ b/crates/tracedecay/tests/automation_runner_test/jobs.rs @@ -86,6 +86,7 @@ fn enabled_job_config() -> AutomationSettingsV1 { struct ContentBackend { calls: AtomicUsize, content: &'static str, + executable: Option, } impl ContentBackend { @@ -93,9 +94,15 @@ impl ContentBackend { Self { calls: AtomicUsize::new(0), content, + executable: None, } } + fn with_executable(mut self, executable: &Path) -> Self { + self.executable = Some(executable.to_path_buf()); + self + } + fn calls(&self) -> usize { self.calls.load(Ordering::SeqCst) } @@ -126,7 +133,7 @@ impl AgentTaskBackend for ContentBackend { } fn executable(&self) -> Option<&std::path::Path> { - None + self.executable.as_deref() } } @@ -988,6 +995,10 @@ async fn scheduler_trigger_skips_repeat_and_respects_lock_discipline() { let mut job = sample_job("sched-job"); job.schedule = Some("manual".to_string()); let config = enabled_job_config(); + // Reach the schedule gate with a bound identity, not the earlier + // backend-unconfigured refusal. This gate never invokes the backend. + let executable = tempfile::NamedTempFile::new().unwrap(); + let backend = ContentBackend::new("unused").with_executable(executable.path()); // Manual schedule: every scheduler tick of the same occurrence reaches // the prefilter, which records the skip once. @@ -996,7 +1007,7 @@ async fn scheduler_trigger_skips_repeat_and_respects_lock_discipline() { let run = evaluate_and_record_scheduler_skip( &dashboard_root, &config, - None, + backend.executable(), &job, "sched-occurrence", None, @@ -1019,7 +1030,7 @@ async fn scheduler_trigger_skips_repeat_and_respects_lock_discipline() { let repeated = evaluate_and_record_scheduler_skip( &dashboard_root, &config, - None, + backend.executable(), &job, "sched-occurrence", None, @@ -1031,6 +1042,76 @@ async fn scheduler_trigger_skips_repeat_and_respects_lock_discipline() { repeated.ledger_record, records[0], "an immediate repeat skip must return the exact durable prior row" ); + assert_eq!(backend.calls(), 0); +} + +#[tokio::test] +async fn scheduler_prefilter_rejects_unconfigured_backend_and_recovers_without_effect() { + let temp = tempdir().unwrap(); + let dashboard_root = temp.path().join("dashboard"); + let job = sample_job("backend-recovery"); + let config = enabled_job_config(); + let occurrence = "backend-recovery-occurrence"; + let backend = ContentBackend::new("unused"); + + let first = evaluate_and_record_scheduler_skip( + &dashboard_root, + &config, + backend.executable(), + &job, + occurrence, + None, + ) + .await + .unwrap() + .expect("a due job still requires a configured backend"); + assert_eq!(first.report["reason"], json!("backend_unconfigured")); + assert_eq!(first.ledger_record.status, AutomationRunStatus::Skipped); + assert_ne!(first.run_id, occurrence); + let repeated = evaluate_and_record_scheduler_skip( + &dashboard_root, + &config, + backend.executable(), + &job, + occurrence, + None, + ) + .await + .unwrap() + .unwrap(); + assert_eq!(repeated.ledger_record, first.ledger_record); + + let executable = tempfile::NamedTempFile::new().unwrap(); + let backend = backend.with_executable(executable.path()); + assert!( + evaluate_and_record_scheduler_skip( + &dashboard_root, + &config, + backend.executable(), + &job, + occurrence, + None, + ) + .await + .unwrap() + .is_none(), + "binding the backend must admit the same due occurrence without cooldown" + ); + assert_eq!(backend.calls(), 0); + assert_eq!( + load_run_records(&dashboard_root, 10).await.unwrap(), + vec![first.ledger_record], + "advisory admission must not append an effectful terminal" + ); + assert!( + tracedecay_automation_runtime::automation::run_ledger::find_run_record_exact_bounded( + &dashboard_root, + occurrence, + ) + .await + .unwrap() + .is_none() + ); } #[tokio::test] @@ -1206,15 +1287,22 @@ async fn scheduler_prefilter_live_lock_wins_over_not_due_summary() { let config = enabled_job_config(); let occurrence = "locked-prefilter-occurrence"; - let prefilter = - evaluate_and_record_scheduler_skip(&dashboard_root, &config, None, &job, occurrence, None) - .await - .unwrap() - .expect("the live job lock must produce a diagnostic"); + let executable = tempfile::NamedTempFile::new().unwrap(); + let backend = ContentBackend::new("unused").with_executable(executable.path()); + let prefilter = evaluate_and_record_scheduler_skip( + &dashboard_root, + &config, + backend.executable(), + &job, + occurrence, + None, + ) + .await + .unwrap() + .expect("the live job lock must produce a diagnostic"); assert_eq!(prefilter.report["reason"], json!("scheduler_lock_active")); assert_ne!(prefilter.run_id, occurrence); - let backend = ContentBackend::new("unused"); let admitted_job = tracedecay_automation_runtime::automation::jobs::admit_job(&job).unwrap(); let retained = run_user_job_with_backend_for_retained_settlement( &dashboard_root, @@ -1262,12 +1350,21 @@ async fn retained_scheduler_runner_reacquires_after_due_prefilter() { job.schedule = Some("every 1h".to_owned()); let config = enabled_job_config(); let occurrence = "prefilter-gap-occurrence"; + let executable = tempfile::NamedTempFile::new().unwrap(); + let backend = ContentBackend::new("unused").with_executable(executable.path()); assert!( - evaluate_and_record_scheduler_skip(&dashboard_root, &config, None, &job, occurrence, None) - .await - .unwrap() - .is_none(), + evaluate_and_record_scheduler_skip( + &dashboard_root, + &config, + backend.executable(), + &job, + occurrence, + None, + ) + .await + .unwrap() + .is_none(), "a new interval job is advisory-due before effect admission" ); let _raced_in = @@ -1276,7 +1373,6 @@ async fn retained_scheduler_runner_reacquires_after_due_prefilter() { .unwrap() .expect("the advisory prefilter must release its reservation lock"); - let backend = ContentBackend::new("unused"); let admitted_job = tracedecay_automation_runtime::automation::jobs::admit_job(&job).unwrap(); let retained = run_user_job_with_backend_for_retained_settlement( &dashboard_root,