From 4174fef2633ea0c4d7c19025626332bca9e61009 Mon Sep 17 00:00:00 2001 From: Daniel Hoffman Date: Sat, 23 May 2026 16:53:26 +1000 Subject: [PATCH 1/4] feat(gain): release-boundary slicing for --weak-filters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `gain --weak-filters` reported lifetime stats by default — months of behaviour from pre-upgrade binaries blended with current binary's filter quality. Hard to read post-release whether a filter improved. Add a self-correcting boundary mechanism: 1. New table `release_boundaries(id, version, installed_at)` populated once per binary upgrade. On every `Tracker::new()`, compare `env!("CARGO_PKG_VERSION")` to the latest row; insert a new row on change. Cost: 1 SELECT per invocation, 1 INSERT once per upgrade. Best-effort — boundary write failures never poison tracker construction. 2. `get_weak_filters(project, since)` gains a `since` parameter that filters command rows lexicographically by ISO-8601 timestamp. `None` = no slice (lifetime). 3. `gain --weak-filters` defaults to slicing from `latest_boundary_timestamp()`. New `--all-time` flag bypasses the slice for cross-version analysis. Window banner makes the cutoff explicit so the reader knows what they're looking at. Live behaviour on v0.1.10 (post-install, 3 commands): Window: since latest release boundary (2026-05-23T06:52:43Z). 1. ls 1 run 2.6M in 1.6M leaked 39.0% 2. git show 1 run 1.5K in 790 leaked 47.7% 3. git status 1 run 138 in 86 leaked 37.7% vs --all-time (lifetime): 1. ls 1634 runs 29.8M 18.0M leaked 39.5% 2. proxy cargo 172 4.5M 4.5M leaked 0.0% … This makes the post-release audit honest: the user is identifying gaps in the CURRENT binary, not gaps that have been historically present and may already be fixed. Tests (+3, all green): - ensure_release_boundary_writes_first_row_on_fresh_db - ensure_release_boundary_is_idempotent_on_same_version - get_weak_filters_since_excludes_older_rows 2671/2671 pass. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/analytics/gain.rs | 32 ++++++- src/core/tracking.rs | 189 ++++++++++++++++++++++++++++++++++++++++-- src/main.rs | 6 ++ 3 files changed, 219 insertions(+), 8 deletions(-) diff --git a/src/analytics/gain.rs b/src/analytics/gain.rs index 650df93e56..8076728db7 100644 --- a/src/analytics/gain.rs +++ b/src/analytics/gain.rs @@ -40,6 +40,7 @@ pub fn run( reset: bool, yes: bool, _verbose: u8, + all_time: bool, // added: bypass release-boundary slicing for weak-filters ) -> Result<()> { let tracker = Tracker::new().context("Failed to initialize tracking database")?; let project_scope = resolve_project_scope(project)?; // added: resolve project path @@ -61,7 +62,7 @@ pub fn run( } if weak_filters { - return show_weak_filters(&tracker, project_scope.as_deref()); + return show_weak_filters(&tracker, project_scope.as_deref(), all_time); } // Handle export formats @@ -719,9 +720,24 @@ fn check_rtk_disabled_bypass() -> Option { /// Render `gain --weak-filters`: tools ranked by leaked tokens, so the /// reader can see where a better or new filter would recover the most. -fn show_weak_filters(tracker: &Tracker, project_scope: Option<&str>) -> Result<()> { +fn show_weak_filters( + tracker: &Tracker, + project_scope: Option<&str>, + all_time: bool, +) -> Result<()> { + // Default: slice from the latest release boundary so we measure the + // *current* binary's filter quality, not months of pre-upgrade history. + // `--all-time` bypasses the slice for cross-version analysis. + let boundary = if all_time { + None + } else { + tracker + .latest_boundary_timestamp() + .context("Failed to read latest release boundary")? + }; + let weak = tracker - .get_weak_filters(project_scope) + .get_weak_filters(project_scope, boundary.as_deref()) .context("Failed to load weak-filter data")?; println!( @@ -730,6 +746,16 @@ fn show_weak_filters(tracker: &Tracker, project_scope: Option<&str>) -> Result<( ); println!("{}", "═".repeat(64)); println!(); + if let Some(since) = boundary.as_deref() { + println!( + "Window: since latest release boundary ({}). Use `--all-time` for lifetime.", + since + ); + println!(); + } else if all_time { + println!("Window: lifetime (all-time)."); + println!(); + } if weak.is_empty() { println!("No command data recorded yet — run some commands first."); diff --git a/src/core/tracking.rs b/src/core/tracking.rs index 28a71eec67..57c92d5981 100644 --- a/src/core/tracking.rs +++ b/src/core/tracking.rs @@ -32,7 +32,7 @@ use anyhow::{Context, Result}; use chrono::{DateTime, Utc}; use regex::Regex; -use rusqlite::{params, Connection}; +use rusqlite::{params, Connection, OptionalExtension}; use serde::Serialize; use std::ffi::OsString; use std::path::PathBuf; @@ -523,6 +523,20 @@ impl Tracker { [], )?; + // Release boundaries: one row per `contextcrawler --version` change + // observed. Written by `ensure_release_boundary()` on first invocation + // after a binary upgrade. `gain --weak-filters` slices by the latest + // boundary timestamp so newly-released filter behaviour isn't masked + // by months of accumulated pre-upgrade leakage. + conn.execute( + "CREATE TABLE IF NOT EXISTS release_boundaries ( + id INTEGER PRIMARY KEY, + version TEXT NOT NULL, + installed_at TEXT NOT NULL + )", + [], + )?; + // One-time migration for legacy DBs: `auto_vacuum=INCREMENTAL` set // above is a no-op on a DB created in mode 0 (full/none). A single // full VACUUM rewrites the file and commits it to incremental mode, @@ -537,7 +551,50 @@ impl Tracker { let _ = conn.execute_batch("VACUUM;"); } - Ok(Self { conn }) + let tracker = Self { conn }; + // Best-effort: never fail tracker construction on a boundary insert + // problem (downstream record() still works without it). + let _ = tracker.ensure_release_boundary(); + Ok(tracker) + } + + /// Write a new release-boundary row if the installed binary version + /// differs from the most recent one recorded in the DB. Called once at + /// tracker construction; cost is one SELECT plus, on the upgrade tick + /// only, one INSERT. + fn ensure_release_boundary(&self) -> Result<()> { + const CURRENT_VERSION: &str = env!("CARGO_PKG_VERSION"); + let last: Option = self + .conn + .query_row( + "SELECT version FROM release_boundaries ORDER BY id DESC LIMIT 1", + [], + |row| row.get(0), + ) + .optional()?; + if last.as_deref() != Some(CURRENT_VERSION) { + let now = Utc::now().to_rfc3339(); + self.conn.execute( + "INSERT INTO release_boundaries (version, installed_at) VALUES (?1, ?2)", + params![CURRENT_VERSION, now], + )?; + } + Ok(()) + } + + /// Timestamp of the most recent release boundary, in RFC-3339 / ISO-8601 + /// form. `None` if no boundary has been recorded yet (fresh DB on a + /// pre-feature binary, or in-memory test tracker). Callers should treat + /// `None` as "no slice, fall back to lifetime". + pub fn latest_boundary_timestamp(&self) -> Result> { + Ok(self + .conn + .query_row( + "SELECT installed_at FROM release_boundaries ORDER BY id DESC LIMIT 1", + [], + |row| row.get(0), + ) + .optional()?) } /// Create an isolated in-memory tracker for tests. @@ -590,6 +647,16 @@ impl Tracker { "CREATE INDEX IF NOT EXISTS idx_pf_timestamp ON parse_failures(timestamp)", [], )?; + // Mirror production: release_boundaries table for version-aware + // weak-filter analytics. See block at L526 for details. + self.conn.execute( + "CREATE TABLE IF NOT EXISTS release_boundaries ( + id INTEGER PRIMARY KEY, + version TEXT NOT NULL, + installed_at TEXT NOT NULL + )", + [], + )?; Ok(()) } @@ -903,15 +970,24 @@ impl Tracker { /// model unfiltered, and sorts highest-leak first. Passthrough rows /// (0 input) contribute nothing and drop out — so a command that is /// always passthrough never appears. - pub fn get_weak_filters(&self, project_path: Option<&str>) -> Result> { + pub fn get_weak_filters( + &self, + project_path: Option<&str>, + since: Option<&str>, + ) -> Result> { let (project_exact, project_glob) = project_filter_params(project_path); + // `since` defaults to NULL (no slice) when caller passes None — same + // pattern as project_path. The boundary timestamp is RFC-3339 / ISO- + // 8601 and command timestamps are also ISO-8601, so lexicographic + // comparison is correct. let mut stmt = self.conn.prepare( "SELECT rtk_cmd, COUNT(*), SUM(input_tokens), SUM(saved_tokens) FROM commands WHERE (?1 IS NULL OR project_path = ?1 OR project_path GLOB ?2) + AND (?3 IS NULL OR timestamp >= ?3) GROUP BY rtk_cmd", )?; - let rows = stmt.query_map(params![project_exact, project_glob], |row| { + let rows = stmt.query_map(params![project_exact, project_glob, since], |row| { Ok(( row.get::<_, String>(0)?, row.get::<_, i64>(1)? as usize, @@ -2789,7 +2865,7 @@ mod tests { .expect("record passthrough"); let weak = tracker - .get_weak_filters(None) + .get_weak_filters(None, None) .expect("Failed to load weak filters"); // read leaks 3 × (1000 input − 100 saved) = 2700; cargo leaks 50. @@ -2808,4 +2884,107 @@ mod tests { "a passthrough-only tool (0 input) must be excluded" ); } + + // ─── Release-boundary slicing ────────────────────────────────────────── + + #[test] + fn ensure_release_boundary_writes_first_row_on_fresh_db() { + let tracker = Tracker::new_in_memory().expect("in-memory tracker"); + // new_in_memory() does not call ensure_release_boundary() (production + // path does, but the test constructor mirrors only the schema). Call + // it explicitly to validate the insert path. + tracker + .ensure_release_boundary() + .expect("boundary insert"); + let ts = tracker + .latest_boundary_timestamp() + .expect("read boundary") + .expect("boundary row must exist"); + assert!( + !ts.is_empty(), + "stored timestamp must be a non-empty RFC-3339 string" + ); + } + + #[test] + fn ensure_release_boundary_is_idempotent_on_same_version() { + let tracker = Tracker::new_in_memory().expect("in-memory tracker"); + tracker.ensure_release_boundary().expect("first insert"); + tracker.ensure_release_boundary().expect("no-op call"); + tracker.ensure_release_boundary().expect("no-op call"); + let n: i64 = tracker + .conn + .query_row( + "SELECT COUNT(*) FROM release_boundaries", + [], + |row| row.get(0), + ) + .expect("count rows"); + assert_eq!( + n, 1, + "same-version call must not append duplicate boundary rows" + ); + } + + #[test] + fn get_weak_filters_since_excludes_older_rows() { + let tracker = Tracker::new_in_memory().expect("in-memory tracker"); + // Two rows: one before, one after the cutoff. + tracker + .conn + .execute( + "INSERT INTO commands + (timestamp, original_cmd, rtk_cmd, input_tokens, output_tokens, + saved_tokens, savings_pct, exec_time_ms, project_path) + VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9)", + params![ + "2026-01-01T00:00:00Z", + "read /old", + "contextcrawler read /old", + 1000, + 100, + 900, + 90.0, + 0, + "" + ], + ) + .expect("insert old row"); + tracker + .conn + .execute( + "INSERT INTO commands + (timestamp, original_cmd, rtk_cmd, input_tokens, output_tokens, + saved_tokens, savings_pct, exec_time_ms, project_path) + VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9)", + params![ + "2026-06-01T00:00:00Z", + "read /new", + "contextcrawler read /new", + 2000, + 400, + 1600, + 80.0, + 0, + "" + ], + ) + .expect("insert new row"); + let all = tracker + .get_weak_filters(None, None) + .expect("all-time query"); + let read_all = all.iter().find(|w| w.tool == "read").expect("read entry"); + assert_eq!(read_all.runs, 2); + assert_eq!(read_all.input_tokens, 3000); + + let sliced = tracker + .get_weak_filters(None, Some("2026-03-01T00:00:00Z")) + .expect("since query"); + let read_sliced = sliced + .iter() + .find(|w| w.tool == "read") + .expect("read entry after slice"); + assert_eq!(read_sliced.runs, 1, "old row must be excluded"); + assert_eq!(read_sliced.input_tokens, 2000); + } } diff --git a/src/main.rs b/src/main.rs index d57ba1efe9..60ce11c1f9 100644 --- a/src/main.rs +++ b/src/main.rs @@ -457,6 +457,10 @@ enum Commands { /// Rank tools by leaked tokens — where a better filter would help most #[arg(short = 'W', long = "weak-filters")] weak_filters: bool, + /// For --weak-filters: include rows from before the latest release + /// boundary (default: slice from latest contextcrawler version install). + #[arg(long = "all-time")] + all_time: bool, /// Reset all token savings stats to zero #[arg(long)] reset: bool, @@ -3578,6 +3582,7 @@ fn run_cli() -> Result { format, failures, weak_filters, + all_time, reset, yes, } => { @@ -3597,6 +3602,7 @@ fn run_cli() -> Result { reset, yes, cli.verbose, + all_time, )?; 0 } From dabb9b19cbf04c5a956a82f57eab5d74cd2142fc Mon Sep 17 00:00:00 2001 From: Daniel Hoffman Date: Sat, 23 May 2026 18:44:33 +1000 Subject: [PATCH 2/4] harden(gain): close peer-review #150 findings (Codex + agy) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both reviewers returned YELLOW with the same top finding (TOCTOU race on ensure_release_boundary). Closing all medium-priority items in one follow-up. 1. ATOMIC INSERT (Codex + agy, the blocker): Previous version did SELECT-latest then INSERT, with no transaction. Two concurrent contextcrawler processes hitting a fresh-upgrade DB could both observe the old latest-version and both write a duplicate boundary row. Fix: collapse to single atomic INSERT ... SELECT ... WHERE NOT EXISTS. The WHERE clause evaluates against the LATEST row only (id = MAX(id)), so a downgrade-then-upgrade scenario still appends a new boundary row correctly (rejected only when version equals the most-recently-written one). 2. TEST TIMESTAMP FORMAT (agy): Production uses `Utc::now().to_rfc3339()` which yields `+00:00` suffix. Existing tests used `Z` suffix — lexicographic comparison safe within each fixture but would diverge against real DB data. Aligned test timestamps to `+00:00`. 3. UX HINT FOR FIRST-RUN-ON-UPGRADE (agy): On a pre-existing DB with command history, the first invocation of a new binary writes a boundary row that slices out ALL history by default. User sees an empty leaderboard with no indication that --all-time would show data. Fix: when the sliced query returns 0 rows AND the lifetime query has rows, print a hint pointing at --all-time with the historical run count. 4. THREE NEW TESTS (agy): - latest_boundary_timestamp_is_none_on_empty_table - ensure_release_boundary_appends_row_on_version_change - ensure_release_boundary_handles_concurrent_safely (tight-loop same-connection approximation of the original race) Verification: 2674/2674 pass (+3 from this commit). The silent-failure design at the `let _ = ensure_release_boundary()` call site stays as-is per agy's reasoning: contextcrawler is a transparent proxy and stderr warnings would pollute build/pipe consumers. Codex's "log a warning" was a soft recommendation, not a blocker. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/analytics/gain.rs | 20 +++++++ src/core/tracking.rs | 124 +++++++++++++++++++++++++++++++++++------- 2 files changed, 124 insertions(+), 20 deletions(-) diff --git a/src/analytics/gain.rs b/src/analytics/gain.rs index 8076728db7..e441cb3f8d 100644 --- a/src/analytics/gain.rs +++ b/src/analytics/gain.rs @@ -758,6 +758,26 @@ fn show_weak_filters( } if weak.is_empty() { + // Peer-review #150 (agy): on a pre-existing DB the first invocation + // of a new binary version writes a boundary row that excludes ALL + // prior history. The user sees an empty leaderboard despite + // months of data sitting one --all-time flag away. Surface that. + if boundary.is_some() { + let lifetime_runs = tracker + .get_weak_filters(project_scope, None) + .map(|w| w.iter().map(|f| f.runs).sum::()) + .unwrap_or(0); + if lifetime_runs > 0 { + println!( + "No commands recorded since the latest release boundary." + ); + println!( + "Run some commands or use `--all-time` to see the lifetime view ({} historical runs).", + lifetime_runs + ); + return Ok(()); + } + } println!("No command data recorded yet — run some commands first."); return Ok(()); } diff --git a/src/core/tracking.rs b/src/core/tracking.rs index 57c92d5981..b225c0ab77 100644 --- a/src/core/tracking.rs +++ b/src/core/tracking.rs @@ -560,25 +560,30 @@ impl Tracker { /// Write a new release-boundary row if the installed binary version /// differs from the most recent one recorded in the DB. Called once at - /// tracker construction; cost is one SELECT plus, on the upgrade tick - /// only, one INSERT. + /// tracker construction; cost is one INSERT-WHERE per invocation (no + /// follow-up SELECT in steady-state — the WHERE clause evaluates to + /// zero rows when the version already matches, so SQLite skips the + /// INSERT atomically). + /// + /// The INSERT ... SELECT ... WHERE pattern collapses the previous + /// read-then-write into a single atomic statement. Without this, two + /// concurrent contextcrawler processes hitting a fresh-upgrade DB + /// would both observe the old latest-version, both pass the Rust-side + /// check, and both insert a duplicate boundary row (peer-review #150 + /// finding, Codex + agy). fn ensure_release_boundary(&self) -> Result<()> { const CURRENT_VERSION: &str = env!("CARGO_PKG_VERSION"); - let last: Option = self - .conn - .query_row( - "SELECT version FROM release_boundaries ORDER BY id DESC LIMIT 1", - [], - |row| row.get(0), - ) - .optional()?; - if last.as_deref() != Some(CURRENT_VERSION) { - let now = Utc::now().to_rfc3339(); - self.conn.execute( - "INSERT INTO release_boundaries (version, installed_at) VALUES (?1, ?2)", - params![CURRENT_VERSION, now], - )?; - } + let now = Utc::now().to_rfc3339(); + self.conn.execute( + "INSERT INTO release_boundaries (version, installed_at) + SELECT ?1, ?2 + WHERE NOT EXISTS ( + SELECT 1 FROM release_boundaries + WHERE version = ?1 + AND id = (SELECT MAX(id) FROM release_boundaries) + )", + params![CURRENT_VERSION, now], + )?; Ok(()) } @@ -2928,6 +2933,9 @@ mod tests { #[test] fn get_weak_filters_since_excludes_older_rows() { + // Use `+00:00` suffix to match production `Utc::now().to_rfc3339()` + // output — lexicographic ISO-8601 comparison requires consistent + // suffix shape across all rows. Peer-review #150 (agy) finding. let tracker = Tracker::new_in_memory().expect("in-memory tracker"); // Two rows: one before, one after the cutoff. tracker @@ -2938,7 +2946,7 @@ mod tests { saved_tokens, savings_pct, exec_time_ms, project_path) VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9)", params![ - "2026-01-01T00:00:00Z", + "2026-01-01T00:00:00+00:00", "read /old", "contextcrawler read /old", 1000, @@ -2958,7 +2966,7 @@ mod tests { saved_tokens, savings_pct, exec_time_ms, project_path) VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9)", params![ - "2026-06-01T00:00:00Z", + "2026-06-01T00:00:00+00:00", "read /new", "contextcrawler read /new", 2000, @@ -2978,7 +2986,7 @@ mod tests { assert_eq!(read_all.input_tokens, 3000); let sliced = tracker - .get_weak_filters(None, Some("2026-03-01T00:00:00Z")) + .get_weak_filters(None, Some("2026-03-01T00:00:00+00:00")) .expect("since query"); let read_sliced = sliced .iter() @@ -2987,4 +2995,80 @@ mod tests { assert_eq!(read_sliced.runs, 1, "old row must be excluded"); assert_eq!(read_sliced.input_tokens, 2000); } + + // ─── Peer-review #150 follow-up coverage (agy) ───────────────────────── + + #[test] + fn latest_boundary_timestamp_is_none_on_empty_table() { + let tracker = Tracker::new_in_memory().expect("in-memory tracker"); + let ts = tracker + .latest_boundary_timestamp() + .expect("read boundary on empty table"); + assert!( + ts.is_none(), + "no boundary rows must yield None (callers fall back to lifetime)" + ); + } + + #[test] + fn ensure_release_boundary_appends_row_on_version_change() { + // Simulate the binary moving from a prior version to the current one: + // pre-seed a boundary row for some old version, then call ensure_* + // and confirm a new row is appended (not deduplicated against the + // OLD entry — only against the LATEST one). + let tracker = Tracker::new_in_memory().expect("in-memory tracker"); + tracker + .conn + .execute( + "INSERT INTO release_boundaries (version, installed_at) + VALUES (?1, ?2)", + params!["0.0.0-test-prior", "2026-01-01T00:00:00+00:00"], + ) + .expect("seed prior-version row"); + tracker + .ensure_release_boundary() + .expect("boundary on version change"); + let n: i64 = tracker + .conn + .query_row( + "SELECT COUNT(*) FROM release_boundaries", + [], + |row| row.get(0), + ) + .expect("count rows"); + assert_eq!(n, 2, "a version change must append a new boundary row"); + // The latest row must carry the CURRENT compile-time version, not + // the seeded prior one. + let latest_version: String = tracker + .conn + .query_row( + "SELECT version FROM release_boundaries ORDER BY id DESC LIMIT 1", + [], + |row| row.get(0), + ) + .expect("read latest version"); + assert_eq!(latest_version, env!("CARGO_PKG_VERSION")); + } + + #[test] + fn ensure_release_boundary_handles_concurrent_safely() { + // Approximation of the TOCTOU concern: simulate two back-to-back + // calls on the same connection (the original Rust-side check would + // race two SQLite connections; this exercises the atomic INSERT- + // WHERE-NOT-EXISTS path on a single connection to confirm it is + // idempotent even when called rapidly.). + let tracker = Tracker::new_in_memory().expect("in-memory tracker"); + for _ in 0..10 { + tracker.ensure_release_boundary().expect("repeated call"); + } + let n: i64 = tracker + .conn + .query_row( + "SELECT COUNT(*) FROM release_boundaries", + [], + |row| row.get(0), + ) + .expect("count rows"); + assert_eq!(n, 1, "tight-loop calls must not append duplicate rows"); + } } From b595e6a87ced31c5e10a7a2ffff1c80e4a50220b Mon Sep 17 00:00:00 2001 From: Daniel Hoffman Date: Sat, 23 May 2026 18:50:08 +1000 Subject: [PATCH 3/4] test(tracking): real multi-thread concurrency test for boundary insert MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-2 peer-review (Codex + agy) cleared the atomic INSERT race in production but both flagged that the regression test was a single- connection tight-loop — name suggested concurrency but the body did not exercise SQLite's write-locking behaviour across processes. Add a true multi-thread test: - Pin a private file-backed DB via the existing PinnedDb RAII helper. - Spawn 8 worker threads, each calling `Tracker::new()` independently. - Every Tracker::new() opens its own SQLite Connection AND runs `ensure_release_boundary()` — exactly the production race shape (two contextcrawler processes starting concurrently after a binary upgrade share the same DB path). - Assert exactly 1 row in release_boundaries after all workers join. This empirically pins the atomic INSERT...SELECT...WHERE NOT EXISTS guarantee against SQLite's actual lock arbitration, not just the Rust-side check that the previous test approximated. Also renamed the prior test from ensure_release_boundary_handles_concurrent_safely to ensure_release_boundary_is_idempotent_on_tight_loop because the old name was misleading — same-connection idempotency is a different invariant from cross-connection concurrency safety. Both are now pinned by distinct tests. Verification: 2675/2675 pass (+1 new test, total +5 in PR #150). Co-Authored-By: Claude Opus 4.7 (1M context) --- src/core/tracking.rs | 58 +++++++++++++++++++++++++++++++++++++++----- 1 file changed, 52 insertions(+), 6 deletions(-) diff --git a/src/core/tracking.rs b/src/core/tracking.rs index b225c0ab77..d9cdf6ff85 100644 --- a/src/core/tracking.rs +++ b/src/core/tracking.rs @@ -3051,12 +3051,11 @@ mod tests { } #[test] - fn ensure_release_boundary_handles_concurrent_safely() { - // Approximation of the TOCTOU concern: simulate two back-to-back - // calls on the same connection (the original Rust-side check would - // race two SQLite connections; this exercises the atomic INSERT- - // WHERE-NOT-EXISTS path on a single connection to confirm it is - // idempotent even when called rapidly.). + fn ensure_release_boundary_is_idempotent_on_tight_loop() { + // Same-connection idempotency: repeated calls on one Tracker must + // not append duplicate rows. This is the cheap baseline check + // that the WHERE-NOT-EXISTS guard fires correctly. The real + // multi-connection race is exercised by the test below. let tracker = Tracker::new_in_memory().expect("in-memory tracker"); for _ in 0..10 { tracker.ensure_release_boundary().expect("repeated call"); @@ -3071,4 +3070,51 @@ mod tests { .expect("count rows"); assert_eq!(n, 1, "tight-loop calls must not append duplicate rows"); } + + #[test] + fn ensure_release_boundary_no_duplicates_under_real_concurrency() { + // Round-2 peer-review (Codex + agy) asked for a real multi-thread + // race against a file-backed DB — the single-connection idempotency + // test above doesn't exercise SQLite's write-locking behaviour. + // + // Strategy: pin a private file-backed DB via the PinnedDb RAII + // helper, then spawn 8 threads that each call `Tracker::new()`. + // Every Tracker::new() runs `ensure_release_boundary()` on its own + // sqlite Connection — exactly the production race shape (two + // contextcrawler processes starting concurrently after a binary + // upgrade). The atomic INSERT...WHERE NOT EXISTS guarantees that + // only the first one to acquire the SQLITE write lock inserts; + // the others find the row already there and no-op. + // + // Holding ENV_LOCK serialises with other RTK_DB_PATH-touching + // tests in the same process, not with our own spawned threads — + // they share the same path env-var. + let _guard = ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let _db = PinnedDb::new("rtk_boundary_concurrency"); + + const N_THREADS: usize = 8; + let mut handles = Vec::with_capacity(N_THREADS); + for _ in 0..N_THREADS { + handles.push(std::thread::spawn(|| { + Tracker::new().expect("tracker construction in worker thread"); + })); + } + for h in handles { + h.join().expect("worker thread panicked"); + } + + let tracker = Tracker::new().expect("verifier tracker"); + let n: i64 = tracker + .conn + .query_row( + "SELECT COUNT(*) FROM release_boundaries", + [], + |row| row.get(0), + ) + .expect("count rows"); + assert_eq!( + n, 1, + "atomic INSERT...WHERE NOT EXISTS must dedupe across N concurrent connections" + ); + } } From 9dbc602886d7e4a7dee46e9b2e17adbe3b3cbb71 Mon Sep 17 00:00:00 2001 From: Daniel Hoffman Date: Sat, 23 May 2026 19:22:02 +1000 Subject: [PATCH 4/4] harden(tracking): pragma order + PinnedDb sidecar cleanup (agy round-3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two pre-existing tracking-module issues surfaced by the new multi- thread boundary test in PR #150. Neither is a regression introduced by this PR, but both directly motivated by what the new test now exercises — so closing them in-PR rather than as a separate follow-up. 1. PRAGMA ordering (medium): Tracker::new() was running `PRAGMA journal_mode=WAL` BEFORE `PRAGMA busy_timeout=5000`. The WAL switch needs an exclusive lock and returns SQLITE_BUSY immediately if a peer holds the DB; without busy_timeout already armed, the connection then runs in non-WAL mode with no retry budget — write contention is non-deterministically fatal. Swapped to: PRAGMA busy_timeout=5000; PRAGMA journal_mode=WAL; Now any peer-lock encountered during the WAL upgrade is waited out for up to 5s. Eliminates a flakiness vector that the new multi-thread test could trigger under high CPU contention. 2. PinnedDb sidecar cleanup (low): PinnedDb::drop removed the .db file but not the WAL `-wal` and `-shm` sidecars. In WAL mode these are created on first write and persist if a worker thread panics or the connection closes abnormally — they pollute $HOME across runs. Added cleanup for both suffixes. Deferred to a follow-up issue (agy's third low): Worker threads inside `_no_duplicates_under_real_concurrency` read RTK_DB_PATH via std::env::var() concurrently. Other tests in the suite that don't hold ENV_LOCK can mutate RTK_DB_PATH at the same time — a process-wide concern that needs a broader audit of which tests touch the env, not a localised fix. Verification: 2675/2675 pass, full suite. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/core/tracking.rs | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/src/core/tracking.rs b/src/core/tracking.rs index d9cdf6ff85..14c95aaf72 100644 --- a/src/core/tracking.rs +++ b/src/core/tracking.rs @@ -455,9 +455,15 @@ impl Tracker { let _ = conn.execute_batch("PRAGMA auto_vacuum=INCREMENTAL;"); // WAL mode + busy_timeout for concurrent access (multiple Claude Code instances). // Non-fatal: NFS/read-only filesystems may not support WAL. + // Order matters: busy_timeout MUST register before the WAL switch. + // `PRAGMA journal_mode=WAL` requires an exclusive lock and can return + // SQLITE_BUSY if a peer holds the DB; with busy_timeout already armed, + // SQLite waits out the peer instead of failing immediately. Peer- + // review #150 (agy) — the multi-thread boundary test surfaced this + // ordering as a flakiness vector under high CPU contention. let _ = conn.execute_batch( - "PRAGMA journal_mode=WAL; - PRAGMA busy_timeout=5000;", + "PRAGMA busy_timeout=5000; + PRAGMA journal_mode=WAL;", ); conn.execute( "CREATE TABLE IF NOT EXISTS commands ( @@ -2319,7 +2325,16 @@ mod tests { Some(v) => std::env::set_var("RTK_DB_PATH", v), None => std::env::remove_var("RTK_DB_PATH"), } + // Remove the .db file plus the WAL sidecars. Without `-wal`/`-shm` + // cleanup, WAL-mode connections leave them behind on abnormal exit + // (e.g. a worker thread panic) and pollute $HOME across runs. + // Peer-review #150 (agy) found. let _ = std::fs::remove_file(&self.path); + for suffix in ["-wal", "-shm"] { + let mut s = self.path.clone().into_os_string(); + s.push(suffix); + let _ = std::fs::remove_file(PathBuf::from(s)); + } } }