diff --git a/CHANGELOG.md b/CHANGELOG.md index 2a35d73..7106870 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,21 @@ 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.11] - 2026-09-11 + +### Fixed + +- The file change counter (header offset 24) and version-valid-for + (offset 92) were never incremented on commit — preserved rather than + bumped, so a long-lived reader (a live `sqlite3` process that already + cached page 1) had no signal to invalidate its cache and kept serving + stale rows after our write, with `PRAGMA integrity_check` reporting + `ok` throughout. `Pager::flush` now bumps both fields exactly once per + committed write transaction, before either the rollback-journal or WAL + commit path writes page 1, guarded against double-bumping on a + flush retried after lock contention. Wraps past `u32::MAX` rather than + erroring, matching stock `sqlite3` (#710). + ## [0.18.10] - 2026-08-31 ### Fixed diff --git a/Cargo.lock b/Cargo.lock index 494140f..06ee421 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -781,7 +781,7 @@ checksum = "8ed6a63f02c8539c91a8685a86f4099661ba3da017932f6ebbea6de3f0fa7c90" [[package]] name = "sqlite-rs" -version = "0.18.10" +version = "0.18.11" dependencies = [ "criterion", "md-5", diff --git a/Cargo.toml b/Cargo.toml index 9945743..b3cbeda 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "sqlite-rs" -version = "0.18.10" +version = "0.18.11" edition = "2021" publish = false license = "Apache-2.0" diff --git a/src/pager.rs b/src/pager.rs index 4fa3754..063e7f5 100644 --- a/src/pager.rs +++ b/src/pager.rs @@ -338,6 +338,14 @@ pub struct Pager { /// [`Pager::open`], same as stock SQLite; never read from or /// written to the database file. See [`SynchronousMode`]. synchronous: SynchronousMode, + /// #710: whether [`Pager::bump_change_counter`] has already run for + /// the transaction currently buffered in `dirty`. A retried `flush` + /// after a lock-contention failure (the dirty set survives a failed + /// attempt — see `flush`'s doc comment) must not bump the counter a + /// second time for what is still, on disk, a single committed + /// transaction. Reset to `false` everywhere `dirty` is cleared + /// (successful commit in either journal mode, and `rollback`). + change_counter_bumped: bool, } /// Byte offsets of the three header fields ([`crate::header::DatabaseHeader`]) @@ -349,6 +357,17 @@ const PAGE_COUNT_OFFSET: usize = 28; const FREELIST_TRUNK_PAGE_OFFSET: usize = 32; const FREELIST_PAGE_COUNT_OFFSET: usize = 36; +/// Bytes 24-27: the file change counter (#710). Stock `sqlite3` bumps this +/// once per committed write transaction so a reader with a cached page 1 +/// (page-1 change-counter check on acquiring SHARED) knows to invalidate its +/// cache. Wrapping past `u32::MAX` is expected, not an error. +const CHANGE_COUNTER_OFFSET: usize = 24; + +/// Bytes 92-95: "version-valid-for" — the change-counter value for which the +/// page-count field at [`PAGE_COUNT_OFFSET`] is valid. Kept equal to +/// [`CHANGE_COUNTER_OFFSET`] on every bump, mirroring stock `sqlite3`. +const VERSION_VALID_FOR_OFFSET: usize = 92; + fn read_be_u32(buf: &[u8], offset: usize) -> Result { let end = offset.saturating_add(4); let bytes: [u8; 4] = buf @@ -456,6 +475,7 @@ impl Pager { journal_path, journal_mode, synchronous: SynchronousMode::default(), + change_counter_bumped: false, }) } @@ -499,6 +519,19 @@ impl Pager { return Ok(()); } + // #710: bump the change counter (and version-valid-for) exactly + // once per committed transaction, before either commit path writes + // page 1 — this marks page 1 dirty if it wasn't already, so both + // the rollback-journal and WAL paths pick it up as just another + // dirty page. Guarded by `change_counter_bumped` so a retried + // `flush` (a prior attempt failed on lock contention and left + // `dirty` — including this already-bumped page 1 — intact) does + // not bump it a second time for one committed transaction. + if !self.change_counter_bumped { + self.bump_change_counter()?; + self.change_counter_bumped = true; + } + // WAL mode (#389) never touches the rollback journal or the main // file at commit time, and deliberately never escalates `self.lock` // (the main file's SHARED lock) to EXCLUSIVE either — that @@ -580,6 +613,19 @@ impl Pager { Ok(()) } + /// Increments the change counter at [`CHANGE_COUNTER_OFFSET`] and sets + /// version-valid-for ([`VERSION_VALID_FOR_OFFSET`]) to the same value, + /// wrapping on overflow (not an error — stock `sqlite3` wraps too). + /// Marks page 1 dirty via [`Pager::get_page_mut`] so the caller's commit + /// path journals/writes it like any other dirty page. + fn bump_change_counter(&mut self) -> Result<(), PagerError> { + let page1 = self.get_page_mut(1)?; + let counter = read_be_u32(page1, CHANGE_COUNTER_OFFSET)?.wrapping_add(1); + write_be_u32(page1, CHANGE_COUNTER_OFFSET, counter)?; + write_be_u32(page1, VERSION_VALID_FOR_OFFSET, counter)?; + Ok(()) + } + fn flush_locked(&mut self) -> Result<(), PagerError> { let mut page_nums: Vec = self.dirty.keys().copied().collect(); page_nums.sort_unstable(); @@ -633,6 +679,7 @@ impl Pager { self.vfs.delete(&self.journal_path)?; } self.dirty.clear(); + self.change_counter_bumped = false; Ok(()) } @@ -762,6 +809,7 @@ impl Pager { } } self.dirty.clear(); + self.change_counter_bumped = false; Ok(()) } @@ -777,6 +825,7 @@ impl Pager { /// transaction is ending here rather than at a later `flush`. pub fn rollback(&mut self) -> Result<(), PagerError> { self.dirty.clear(); + self.change_counter_bumped = false; self.release_tx_lock() } @@ -1601,11 +1650,20 @@ mod tests { pager.flush().unwrap(); + // #710: `flush` bumps the change counter (offset 24-27) and + // version-valid-for (offset 92-95) even though nothing else in + // this transaction touched page 1. + let mut expected_page1 = vec![1u8; 512]; + expected_page1[CHANGE_COUNTER_OFFSET..CHANGE_COUNTER_OFFSET + 4] + .copy_from_slice(&[1, 1, 1, 2]); + expected_page1[VERSION_VALID_FOR_OFFSET..VERSION_VALID_FOR_OFFSET + 4] + .copy_from_slice(&[1, 1, 1, 2]); + assert_eq!(pager.read_page(2).unwrap(), vec![9u8; 512].into()); let reopened = Pager::open(&vfs, Path::new("/test.db"), 512).unwrap(); assert_eq!(reopened.read_page(2).unwrap(), vec![9u8; 512].into()); - assert_eq!(reopened.read_page(1).unwrap(), vec![1u8; 512].into()); + assert_eq!(reopened.read_page(1).unwrap(), expected_page1.into()); } /// #320: a page cached by an earlier `read_page` must not survive a @@ -1624,7 +1682,75 @@ mod tests { pager.get_page_mut(1).unwrap().fill(9u8); pager.flush().unwrap(); - assert_eq!(pager.read_page(1).unwrap(), vec![9u8; 512].into()); + // #710: the change counter/version-valid-for bump lands on top of + // this transaction's own fill(9) of page 1. + let mut expected = vec![9u8; 512]; + expected[CHANGE_COUNTER_OFFSET..CHANGE_COUNTER_OFFSET + 4].copy_from_slice(&[9, 9, 9, 10]); + expected[VERSION_VALID_FOR_OFFSET..VERSION_VALID_FOR_OFFSET + 4] + .copy_from_slice(&[9, 9, 9, 10]); + assert_eq!(pager.read_page(1).unwrap(), expected.into()); + } + + /// #710: the change counter bumps exactly once per committed + /// transaction, not once per dirty page — two pages written in the + /// same `flush` must not double-bump it. + #[test] + fn change_counter_bumps_once_per_transaction_not_per_page() { + let mut vfs = MemoryVfs::new(); + let mut contents = vec![0u8; 512]; + contents.extend(vec![2u8; 512]); + vfs.insert("/test.db", contents); + + let mut pager = Pager::open(&vfs, Path::new("/test.db"), 512).unwrap(); + pager.get_page_mut(1).unwrap(); + pager.get_page_mut(2).unwrap().fill(5u8); + pager.flush().unwrap(); + + let page1 = pager.read_page(1).unwrap(); + assert_eq!( + &page1[CHANGE_COUNTER_OFFSET..CHANGE_COUNTER_OFFSET + 4], + &[0, 0, 0, 1], + "one transaction touching two pages bumps the counter exactly once" + ); + } + + /// #710 acceptance criterion: a rolled-back transaction must leave + /// the change counter and version-valid-for untouched — `rollback` + /// only clears the in-memory dirty set and never calls `flush`, so + /// the bump (which lives inside `flush`) never happens. + #[test] + fn rollback_leaves_change_counter_unchanged() { + let mut vfs = MemoryVfs::new(); + vfs.insert("/test.db", vec![7u8; 512]); + let mut pager = Pager::open(&vfs, Path::new("/test.db"), 512).unwrap(); + + pager.get_page_mut(1).unwrap().fill(9u8); + pager.rollback().unwrap(); + + let reopened = Pager::open(&vfs, Path::new("/test.db"), 512).unwrap(); + assert_eq!(reopened.read_page(1).unwrap(), vec![7u8; 512].into()); + } + + /// #710 acceptance criterion: the counter wraps past `u32::MAX` + /// rather than erroring. + #[test] + fn change_counter_wraps_past_u32_max() { + let mut vfs = MemoryVfs::new(); + let mut page1 = vec![0u8; 512]; + write_be_u32(&mut page1, CHANGE_COUNTER_OFFSET, u32::MAX).unwrap(); + page1.extend(vec![1u8; 512]); + vfs.insert("/test.db", page1); + + let mut pager = Pager::open(&vfs, Path::new("/test.db"), 512).unwrap(); + pager.get_page_mut(2).unwrap().fill(3u8); + pager.flush().unwrap(); + + let page1 = pager.read_page(1).unwrap(); + assert_eq!( + &page1[CHANGE_COUNTER_OFFSET..CHANGE_COUNTER_OFFSET + 4], + &0u32.to_be_bytes(), + "wrapping past u32::MAX must not error and must wrap to 0" + ); } /// #469: `Pager::read_page`'s cache-hit branch (`PageSource for diff --git a/tests/corpus/change_counter_test.rs b/tests/corpus/change_counter_test.rs new file mode 100644 index 0000000..4a20923 --- /dev/null +++ b/tests/corpus/change_counter_test.rs @@ -0,0 +1,162 @@ +// Copyright 2026 Schuberg Philis +// SPDX-License-Identifier: Apache-2.0 +//! #710: the file change counter (header offset 24) and version-valid-for +//! (offset 92) must be bumped on every committed write transaction, the +//! same way stock `sqlite3` does — otherwise a long-lived reader that has +//! already cached page 1 has no signal to invalidate that cache and keeps +//! serving stale rows after our write. +//! +//! `run_oracle` (`oracle.rs`) re-invokes `sqlite3` fresh for every call, +//! so it re-reads the header every time and can never observe this bug — +//! that's exactly why this file uses `oracle::OracleSession` instead: one +//! `sqlite3` process, held open across our write, asked to read the same +//! table before and after. + +use std::path::{Path, PathBuf}; +use std::process::{Command, Output}; +use std::sync::atomic::{AtomicU64, Ordering}; + +use crate::oracle::{assert_integrity_check_ok, pinned_oracle, skip_no_oracle, OracleSession}; + +const CLI: &str = env!("CARGO_BIN_EXE_sqlite-rs"); + +/// Header byte offset of the file change counter (#710). +const CHANGE_COUNTER_OFFSET: usize = 24; +/// Header byte offset of version-valid-for, kept equal to the change +/// counter on every bump. +const VERSION_VALID_FOR_OFFSET: usize = 92; + +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-change-counter-{label}-{}-{n}", + std::process::id() + )); + std::fs::remove_dir_all(&dir).ok(); + std::fs::create_dir_all(&dir).unwrap(); + dir.join("scratch.db") +} + +fn run_exec(db: &Path, sql: &str) -> Output { + Command::new(CLI) + .arg("exec") + .arg(db) + .arg(sql) + .output() + .unwrap_or_else(|e| panic!("running {CLI} exec {} {sql:?}: {e}", db.display())) +} + +fn header_field(db: &Path, offset: usize) -> [u8; 4] { + let bytes = std::fs::read(db).unwrap_or_else(|e| panic!("reading {}: {e}", db.display())); + bytes[offset..offset + 4] + .try_into() + .unwrap_or_else(|_| panic!("{} is shorter than {offset} + 4 bytes", db.display())) +} + +/// The headline acceptance criterion: a long-lived `sqlite3` session that +/// has already read (and cached) the table must see a row we wrote after +/// the fact, not the stale pre-write count — the change-counter bump is +/// exactly the signal that tells it to discard its page-1 cache and +/// re-read. Without #710's fix, this reproduces the bug: the session +/// keeps answering `1` forever. +#[test] +fn long_lived_oracle_session_sees_our_write_after_it_already_cached_the_table() { + let Some(oracle) = pinned_oracle() else { + skip_no_oracle( + "long_lived_oracle_session_sees_our_write_after_it_already_cached_the_table", + ); + return; + }; + + let db = scratch_db("long-lived-reader"); + let status = run_exec(&db, "create table t(a integer); insert into t values (1);").status; + assert!(status.success()); + + let mut session = OracleSession::spawn(&oracle, &db); + // Populates the session's page cache with the pre-write row count. + let before = session.exec("select count(*) from t;"); + assert_eq!(before.trim(), "1"); + + let status = run_exec(&db, "insert into t values (2);").status; + assert!(status.success()); + + let after = session.exec("select count(*) from t;"); + assert_eq!( + after.trim(), + "2", + "a long-lived reader that already cached the table must see our write \ + after it happens, not keep serving its stale cached count" + ); +} + +/// Offset 24 (change counter) and offset 92 (version-valid-for) must be +/// byte-identical between a database stock `sqlite3` writes and one our +/// engine writes, given the same write sequence starting from the same +/// state — both bump by exactly one per committed transaction. +#[test] +fn change_counter_and_version_valid_for_match_oracle_byte_for_byte() { + let Some(oracle) = pinned_oracle() else { + skip_no_oracle("change_counter_and_version_valid_for_match_oracle_byte_for_byte"); + return; + }; + + let ours = scratch_db("ours"); + let theirs = scratch_db("theirs"); + + let sql = "create table t(a integer); insert into t values (1); insert into t values (2);"; + assert!(run_exec(&ours, sql).status.success()); + assert!(Command::new(&oracle) + .arg(&theirs) + .arg(sql) + .status() + .unwrap() + .success()); + + assert_eq!( + header_field(&ours, CHANGE_COUNTER_OFFSET), + header_field(&theirs, CHANGE_COUNTER_OFFSET), + "change counter must match the oracle's after the same write sequence" + ); + assert_eq!( + header_field(&ours, VERSION_VALID_FOR_OFFSET), + header_field(&theirs, VERSION_VALID_FOR_OFFSET), + "version-valid-for must match the oracle's after the same write sequence" + ); + + assert_integrity_check_ok(&oracle, &ours); +} + +/// A file we create from scratch (one `CREATE TABLE` transaction, no +/// prior writes) must carry the same initial change-counter/ +/// version-valid-for values stock `sqlite3` gives a fresh database. +#[test] +fn fresh_database_has_the_same_initial_change_counter_as_the_oracle() { + let Some(oracle) = pinned_oracle() else { + skip_no_oracle("fresh_database_has_the_same_initial_change_counter_as_the_oracle"); + return; + }; + + let ours = scratch_db("ours-fresh"); + let theirs = scratch_db("theirs-fresh"); + + let sql = "create table t(a integer);"; + assert!(run_exec(&ours, sql).status.success()); + assert!(Command::new(&oracle) + .arg(&theirs) + .arg(sql) + .status() + .unwrap() + .success()); + + assert_eq!( + header_field(&ours, CHANGE_COUNTER_OFFSET), + header_field(&theirs, CHANGE_COUNTER_OFFSET), + "a freshly created database's change counter must match the oracle's" + ); + assert_eq!( + header_field(&ours, VERSION_VALID_FOR_OFFSET), + header_field(&theirs, VERSION_VALID_FOR_OFFSET), + "a freshly created database's version-valid-for must match the oracle's" + ); +} diff --git a/tests/corpus/main.rs b/tests/corpus/main.rs index e0a6086..6d39a9c 100644 --- a/tests/corpus/main.rs +++ b/tests/corpus/main.rs @@ -23,6 +23,7 @@ mod btree_delete_test; mod btree_index_insert_delete_test; mod btree_insert_test; mod btree_test; +mod change_counter_test; mod cli_e2e_test; mod cli_write_test; mod crash_torture_test; diff --git a/tests/corpus/oracle.rs b/tests/corpus/oracle.rs index 30b8c75..206df89 100644 --- a/tests/corpus/oracle.rs +++ b/tests/corpus/oracle.rs @@ -13,8 +13,9 @@ //! when no oracle is present. See `.openspec/specs/004-corpus/spec.md` //! Requirement 1. +use std::io::{BufRead, BufReader, Write}; use std::path::{Path, PathBuf}; -use std::process::Command; +use std::process::{Child, ChildStdout, Command, Stdio}; /// Must equal Cargo.toml's `[package.metadata.oracle] version` — a /// `const` cannot read it at run time, so `make version-pin` enforces @@ -195,3 +196,96 @@ pub fn oracle_csv_with_header_output( let sql = format!("select {select_list} from \"{table}\""); run_oracle(oracle, db, &["-csv", "-header"], &sql) } + +/// A long-lived `sqlite3 ` session (stdin/stdout piped), for proving +/// behaviour that a fresh-process-per-assertion oracle invocation cannot +/// see: [`run_oracle`] re-opens `sqlite3` for every call, so it re-reads +/// the header and never consults a page cache. #710 (the file change +/// counter) is invisible without a session that reads once, observes +/// someone else's write, and reads again to check whether it kept serving +/// its first read's cached pages. #706 (in-process locking) reuses this +/// same session type as the cross-process side of its lock interop check. +/// +/// Not `-readonly`: a session may itself be the writer in a locking test. +pub struct OracleSession { + child: Child, + stdout: BufReader, +} + +impl OracleSession { + /// Spawns `sqlite3 ` in `-list` mode with the same separator/null + /// rendering [`run_oracle`]'s callers already expect, so assertions + /// can share format expectations between the two. + pub fn spawn(oracle: &Path, db: &Path) -> Self { + let mut child = Command::new(oracle) + .arg("-list") + .arg("-separator") + .arg("|") + .arg("-nullvalue") + .arg("NULL") + .arg(db) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::null()) + .spawn() + .unwrap_or_else(|e| { + panic!( + "spawning persistent sqlite3 oracle session on {}: {e}", + db.display() + ) + }); + let stdout = BufReader::new( + child + .stdout + .take() + .expect("oracle session stdout was piped"), + ); + OracleSession { child, stdout } + } + + /// Runs `sql` in this session and returns everything it printed for + /// it. A sentinel marker query is appended after `sql` so the reader + /// knows where this call's share of the continuous stdout stream + /// ends — the session's stdout is not framed per command. + pub fn exec(&mut self, sql: &str) -> String { + const MARKER: &str = "__ORACLE_SESSION_DONE__"; + { + let stdin = self + .child + .stdin + .as_mut() + .expect("oracle session stdin was piped"); + writeln!(stdin, "{sql}").expect("writing to oracle session stdin"); + writeln!(stdin, "SELECT '{MARKER}';").expect("writing marker to oracle session stdin"); + stdin.flush().expect("flushing oracle session stdin"); + } + + let mut output = String::new(); + loop { + let mut line = String::new(); + let n = self + .stdout + .read_line(&mut line) + .unwrap_or_else(|e| panic!("reading oracle session stdout: {e}")); + assert!( + n > 0, + "oracle session stdout closed before the marker was seen \ + (the session process likely exited — check its SQL for errors)" + ); + if line.trim_end_matches(['\r', '\n']) == MARKER { + break; + } + output.push_str(&line); + } + output + } +} + +impl Drop for OracleSession { + fn drop(&mut self) { + if let Some(mut stdin) = self.child.stdin.take() { + writeln!(stdin, ".quit").ok(); + } + self.child.wait().ok(); + } +} diff --git a/tests/corpus/wal_write_interop_test.rs b/tests/corpus/wal_write_interop_test.rs index 311f2af..bc175f8 100644 --- a/tests/corpus/wal_write_interop_test.rs +++ b/tests/corpus/wal_write_interop_test.rs @@ -240,10 +240,14 @@ fn concurrent_writer_is_refused_the_wal_write_lock() { // Once the contending process releases the lock, the same writer // (its dirty page untouched by the failed attempt) commits cleanly. pager.flush().unwrap(); - assert_eq!( - pager.read_page(1).unwrap(), - vec![0xABu8; page_size as usize].into() - ); + // #710: `flush` bumps the change counter (offset 24-27) and + // version-valid-for (offset 92-95) exactly once for this one + // committed transaction — even though `flush` was called twice, the + // first (failed) attempt must not have double-bumped it. + let mut expected = vec![0xABu8; page_size as usize]; + expected[24..28].copy_from_slice(&[0xAB, 0xAB, 0xAB, 0xAC]); + expected[92..96].copy_from_slice(&[0xAB, 0xAB, 0xAB, 0xAC]); + assert_eq!(pager.read_page(1).unwrap(), expected.into()); } /// A `-wal` frame written by our own [`WalWriter`] must be readable by a