diff --git a/agents/reference/testing-gates.md b/agents/reference/testing-gates.md index 00c95cc12..767bd7ea1 100644 --- a/agents/reference/testing-gates.md +++ b/agents/reference/testing-gates.md @@ -92,10 +92,19 @@ Keep the job count low enough that the build dirs fit: each *unscoped* job dir r - **7 were real gaps and are now closed** -- six operator mutations in `matcher.rs`'s `placeholder_spans` (its own grammar counter-examples were untested) and `lib.rs`'s `prepare_persist` reading `meta_current` as `compatible && lengths match`, where `&&` -> `||` survived. The tests added for them kill 14 mutants at those sites. - **7 are equivalent mutants, proved rather than argued.** `|` -> `^` over disjoint bit fields is always a no-op: `function_flags`' `SQLITE_UTF8|SQLITE_DETERMINISTIC` (bits `0x1` and `0x800`), `base64_encode`'s `(b0<<16)|(b1<<8)|b2` (exhaustive over all 16,777,216 inputs), and `base64_decode`'s `(triple<<6)|value_of(b)` (all 67,108,864). In `placeholder_spans`, `j += 1` -> `j *= 1` is re-consumed by the following `while`, and `i = j + 1` -> `i = j * 1` / `i = j - 1` land on the closing brace or a name byte, never a `{` -- zero differing results over all 335,923 strings up to length 7 on `{ } a 1 _ -`. **No exemptions were added**: the gate is diff-scoped, so these only surface if someone edits those lines, and the exemption count stays zero. -- **4 sit in effectful code no tier reaches** -- `RootedFs::hash` (two), `cli/run.rs`'s `load_state` guard and `wait_for_shutdown`. Tracked separately rather than re-litigating #233 here. +- **4 sat in effectful code no tier reached** -- `RootedFs::hash` (two), `cli/run.rs`'s `load_state` guard and `wait_for_shutdown`. Tracked as #1012 rather than re-litigating #233 there, and closed by it at the integration and e2e tiers (below). The headline: nothing merged behind the blind gate turned out to be a silent behavior regression. What it hid was **test debt** -- two pure functions whose own documented edge cases had no assertions. +**The four effectful survivors (#1012).** Three were real gaps at a tier that *can* reach them; the fourth is unreachable by construction. None of them needed #233 revisited, and none needed an exemption. + +- **`RootedFs::hash` -> `None` and `-> Some([0; 32])`** are both observable through the parsed path-table's racy window, which `packages/rust/tests/persist_parsed_path_table.rs` now drives on purpose. Pin a file's mtime *ahead* of the cache's `snapshot_ns` (`pin_inside_the_racy_window`) and `is_trusted` can no longer settle it on the stat tuple, so every reuse decision runs through the hash. Two tests fall out of that. A corpus pinned into the window and left alone must still parse nothing -- a hash that cannot be computed cannot confirm, so `None` re-parses all of it. And an in-place edit of the *same length* with the mtime restored leaves the stat tuple byte-identical (`fs::write` reuses the inode, and `FileStat`'s `ctime_ns` is `created()` -- a birth time no rewrite moves), so only a hash that varies with content re-parses it: that kills both constants, `Some([1; 32])` included. +- **`load_state`'s `delete !`** (the `--extension` override guard) is wrong in *both* directions, and each is a CLI behavior worth pinning. With the flag present the mutant drops the launcher's resolved extensions *and* stops suppressing the config's own; with the flag absent it suppresses a config's `[[dirsql.extension]]` entries that nothing overrode. `packages/rust/tests/extensions_e2e.rs` spawns the real binary over the `testext` fixture once per direction -- the builder tests in `extensions.rs` cannot see this decision, because it is the CLI's, not the builder's. +- **`wait_for_shutdown` at `cli/run.rs:651`**, the unix arm, was already caught -- but only by `wait_until_ready`'s timeout, since a server that returns from its own signal wait exits before any readiness poll can see it. That is a 10s never-became-ready panic standing in for a contract. `cli_e2e.rs`'s `the_server_runs_until_signalled_and_then_exits_zero` reads the startup banner instead (the mutant still prints it), waits a beat, and asserts the process is alive -- killing it in 0.6s against the contract `docs/reference/cli.md` states. +- **`wait_for_shutdown` at `cli/run.rs:664`** is the `#[cfg(not(unix))]` twin, and **no test on this gate can kill it**: the gate runs on Linux, where that item is never compiled. Proof rather than argument -- plant a `compile_error!` in its body and `cargo build --features cli` still succeeds. The test above would kill it on a Windows runner. No exemption was added, on the same reasoning as the seven equivalents: mutation is diff-scoped, so it only resurfaces if someone edits that line. + +**The three scanner timeouts were one deadlocked test (#1012).** `walk` (two) and `scan_directory` reported `Timeout` -- inconclusive rather than survivors -- and the mutants were telling the truth about the suite. `async_sdk.rs`'s `sync_backed_methods_before_ready_error` parked background init on a `std::sync::Barrier::new(2)` whose second arrival *is* the extract closure; a scan that matches no file never calls the closure, so the test's own release waited forever and wedged the whole test binary (every other binary in the tree failed fast under the same mutant -- `async_sdk` was the only hang). The barrier is now a `Mutex` + `Condvar` bounded on both sides: a starved scan now fails in 30s naming the cause, and the three mutants become ordinary kills. When a mutant times out, look for a wait with no deadline before assuming the engine is slow. + **Survivors.** The fix is almost always a **new assertion** that kills the mutant. Only a genuinely *equivalent* or intentionally-defensive mutant is lifted, via a `[[.exempt]]` entry whose `rules` includes `"mutation"` -- in the config that lane actually reads (`packages/python/testing-conventions.toml` / `packages/ts/testing-conventions.toml` since #933; the root file for the rust core) (with a real `path` and a `reason`) -- never weaken a test to make a survivor pass. There are none today. ## Coverage Floor diff --git a/packages/rust/tests/async_sdk.rs b/packages/rust/tests/async_sdk.rs index 4d925034b..8ff03177e 100644 --- a/packages/rust/tests/async_sdk.rs +++ b/packages/rust/tests/async_sdk.rs @@ -202,6 +202,19 @@ async fn start_watching_and_poll_events_forward() { ); } +/// Deadlock backstop for the parked-init handshake below. Generous: nothing +/// here is a timing assertion, it is the bound that turns a scan which never +/// reaches the extract closure into a failure instead of a wedged test binary. +const PARK_PATIENCE: Duration = Duration::from_secs(30); + +/// The parked-init handshake: `parked` is raised by the extract closure on +/// arrival, `released` by the test once its assertions are done. +#[derive(Default)] +struct Gate { + parked: bool, + released: bool, +} + #[tokio::test] async fn sync_backed_methods_before_ready_error() { let root = TempDir::new().unwrap(); @@ -209,16 +222,25 @@ async fn sync_backed_methods_before_ready_error() { // Background init runs eagerly, so merely "not awaiting ready()" races // it. Instead, park init deterministically: the extract closure blocks - // on a barrier released only after the assertions, so init cannot - // complete during the assertion window. - let gate = std::sync::Arc::new(std::sync::Barrier::new(2)); + // until the assertions release it, so init cannot complete during the + // assertion window. + let gate = std::sync::Arc::new(( + std::sync::Mutex::new(Gate::default()), + std::sync::Condvar::new(), + )); let gate_in_extract = gate.clone(); let gated_table = Table::new( "items", "CREATE TABLE items (name TEXT)", "**/*.txt", move |_| { - gate_in_extract.wait(); + let (lock, cvar) = &*gate_in_extract; + let mut state = lock.lock().unwrap(); + state.parked = true; + cvar.notify_all(); + let _ = cvar + .wait_timeout_while(state, PARK_PATIENCE, |s| !s.released) + .unwrap(); vec![HashMap::from([( "name".into(), Value::Text("apple".into()), @@ -260,9 +282,21 @@ async fn sync_backed_methods_before_ready_error() { "got: {watch_err}" ); - // Release init and let it finish so the background thread does not - // outlive the TempDir. - gate.wait(); + // Init parks in the extract closure, which is reached only if the scan + // matched `gate.txt`. Prove it arrived, then release it and let it finish + // so the background thread does not outlive the TempDir. + let (lock, cvar) = &*gate; + let (mut state, wait) = cvar + .wait_timeout_while(lock.lock().unwrap(), PARK_PATIENCE, |s| !s.parked) + .unwrap(); + assert!( + !wait.timed_out(), + "init never reached the extract closure: the scan matched no file, so \ + this test never exercised a parked init", + ); + state.released = true; + cvar.notify_all(); + drop(state); db.ready().await.unwrap(); } diff --git a/packages/rust/tests/cli_e2e.rs b/packages/rust/tests/cli_e2e.rs index 5649cb633..2949bc582 100644 --- a/packages/rust/tests/cli_e2e.rs +++ b/packages/rust/tests/cli_e2e.rs @@ -1541,3 +1541,75 @@ fn persist_flag_with_path_writes_the_cache_there() { "the default cache must not be written when a path is given" ); } + +/// Send `sig` to a live child. Mirrors [`kill_and_wait`]'s signalling, which is +/// the only way to ask a `dirsql server` to stop. +#[cfg(unix)] +fn signal_child(child: &Child, sig: i32) { + #[expect( + unsafe_code, + reason = "no safe std API sends a signal to another process" + )] + unsafe { + libc::kill(i32::try_from(child.id()).unwrap(), sig); + } +} + +/// The child's exit code, or `None` if it outlives `patience`. +#[cfg(unix)] +fn exit_code_within(child: &mut Child, patience: Duration) -> Option { + let deadline = Instant::now() + patience; + while Instant::now() < deadline { + if let Some(status) = child.try_wait().unwrap() { + return status.code(); + } + std::thread::sleep(Duration::from_millis(50)); + } + None +} + +/// `docs/reference/cli.md`: the server "runs until it receives `SIGINT` +/// (Ctrl-C) or `SIGTERM`", and that shutdown is exit code `0`. +/// +/// Both halves are load-bearing and neither is visible from a request. A +/// server that never waited would announce its port, serve whatever raced in, +/// and exit 0 — which reads as a clean run from the outside. +#[cfg(unix)] +#[test] +fn the_server_runs_until_signalled_and_then_exits_zero() { + let root = blog_fixture(); + let port = free_port(); + let mut child = spawn_dirsql_with_args(root.path(), port, &["-c", ".dirsql.toml"]); + + // Wait on the banner rather than on a request: it is printed the moment + // the listener is bound, so a server that leaves straight afterwards is + // still observed here instead of timing out as never-ready. + let stdout = child.stdout.take().expect("stdout piped"); + let mut line = String::new(); + BufReader::new(stdout) + .read_line(&mut line) + .expect("expected a startup line"); + assert!( + line.contains(&format!("localhost:{port}")), + "banner: {line:?}" + ); + + std::thread::sleep(Duration::from_millis(500)); + assert!( + child.try_wait().unwrap().is_none(), + "the server must stay up until it is signalled", + ); + + signal_child(&child, libc::SIGTERM); + let code = exit_code_within(&mut child, Duration::from_secs(10)); + if code.is_none() { + let _ = child.kill(); + let _ = child.wait(); + } + + assert_eq!( + code, + Some(0), + "a signalled server drains and exits 0, never 143", + ); +} diff --git a/packages/rust/tests/extensions_e2e.rs b/packages/rust/tests/extensions_e2e.rs new file mode 100644 index 000000000..33f7f8062 --- /dev/null +++ b/packages/rust/tests/extensions_e2e.rs @@ -0,0 +1,130 @@ +//! End-to-end tests for the CLI's two extension surfaces: a config's own +//! `[[dirsql.extension]]` entries and the launcher-facing `--extension` flag +//! that overrides them. +//! +//! Spawns the real compiled `dirsql` binary against a real temp directory, a +//! real `.dirsql.toml` and a real loadable extension built from +//! `tests/fixtures/testext`. Nothing is mocked (real process, real filesystem, +//! real SQLite, real `dlopen`). +//! +//! `packages/rust/tests/extensions.rs` covers the same two behaviors at the +//! builder surface. They are covered again here because the CLI has a +//! decision of its own to make -- which of the two sources of extensions to +//! hand the builder -- and the builder tests cannot see it. +//! +//! Gated behind `--features cli` (the `dirsql` bin needs it) and Unix (the +//! fixture config shells out to `cat`); the Rust CI test job runs on Linux. + +#![cfg(all(feature = "cli", unix))] + +mod common; + +use std::fs; +use std::path::Path; +use std::process::Output; + +use assert_cmd::prelude::*; +use common::build_fixture_extension; +use serde_json::Value; +use tempfile::TempDir; + +/// What `dirsql_testext_answer()` returns once the fixture extension is loaded +/// onto the connection. An unloaded extension is a `no such function` error, +/// not a wrong answer. +const ANSWER: i64 = 42; + +const SQL: &str = "SELECT dirsql_testext_answer() AS a"; + +/// A tempdir holding one parseable file and a `.dirsql.toml` that declares +/// `extension` (verbatim TOML, possibly empty) ahead of an ordinary table, so +/// the index has something to build either way. +fn fixture(extension: &str) -> TempDir { + let root = TempDir::new().unwrap(); + fs::write(root.path().join("a.json"), r#"[{"name": "a"}]"#).unwrap(); + fs::write( + root.path().join(".dirsql.toml"), + format!( + r#"{extension} +[[table]] +name = "files" +ddl = "CREATE TABLE files (name TEXT)" +glob = "*.json" +on-file = "cat {{path}}" +"# + ), + ) + .unwrap(); + root +} + +/// A `[[dirsql.extension]]` entry naming `path`. +fn extension_entry(path: &Path) -> String { + format!( + "[[dirsql.extension]]\npath = \"{}\"\nentrypoint = \"sqlite3_extension_init\"\n", + path.display(), + ) +} + +/// Run `dirsql query -c .dirsql.toml` in `root`, with `extra` appended. +fn query(root: &TempDir, extra: &[String]) -> Output { + std::process::Command::cargo_bin("dirsql") + .expect("`dirsql` binary must be built with --features cli") + .arg("query") + .arg(SQL) + .arg("--config") + .arg(root.path().join(".dirsql.toml")) + .args(extra) + .current_dir(root.path()) + .output() + .expect("spawning `dirsql query` failed") +} + +fn rows(out: &Output) -> Vec { + assert!( + out.status.success(), + "query must succeed, stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); + serde_json::from_slice(&out.stdout).expect("stdout must be JSON") +} + +#[test] +fn a_config_declared_extension_loads_when_no_flag_overrides_it() { + // The ordinary user path: no `--extension` anywhere, so the config's own + // entries are the only source of extensions and must be honored. + let ext = build_fixture_extension(); + let root = fixture(&extension_entry(&ext)); + + let out = query(&root, &[]); + + assert_eq!( + rows(&out)[0]["a"], + Value::from(ANSWER), + "the config's extension must be loaded onto the connection", + ); +} + +#[test] +fn an_extension_flag_replaces_the_config_entries_rather_than_adding_to_them() { + // The launcher path: it has already resolved the config's entries (package + // names and all) and hands the resolved paths back through `--extension`, + // so the config's own entries must not be loaded a second time. The config + // here names a path that does not exist, which would fail the build if it + // were honored -- a green run is the proof it was not. + let ext = build_fixture_extension(); + let root = fixture("[[dirsql.extension]]\npath = \"does/not/exist.so\"\n"); + + let out = query( + &root, + &[ + "--extension".to_string(), + format!("{}::sqlite3_extension_init", ext.display()), + ], + ); + + assert_eq!( + rows(&out)[0]["a"], + Value::from(ANSWER), + "the flag's extension must load and the config's must be suppressed", + ); +} diff --git a/packages/rust/tests/persist_parsed_path_table.rs b/packages/rust/tests/persist_parsed_path_table.rs index e015923ac..71d892d21 100644 --- a/packages/rust/tests/persist_parsed_path_table.rs +++ b/packages/rust/tests/persist_parsed_path_table.rs @@ -134,6 +134,16 @@ fn touch_into_the_future(path: &Path) { fs::File::open(path).unwrap().set_modified(future).unwrap(); } +/// Pin a file's mtime far enough ahead that every cache written during the +/// test predates it. That is the racy window: the cache write does not postdate +/// the file's mtime, so the stat tuple alone cannot settle whether the file +/// changed and the reuse decision falls through to the content hash. +fn pin_inside_the_racy_window(path: &Path) -> SystemTime { + let pinned = SystemTime::now() + Duration::from_secs(3600); + fs::File::open(path).unwrap().set_modified(pinned).unwrap(); + pinned +} + #[test] fn unchanged_second_run_reuses_the_cache_instead_of_reparsing() { let fx = Fixture::with_corpus(CORPUS); @@ -262,3 +272,85 @@ fn a_changed_parser_command_invalidates_the_cached_rows() { "the new parser's rows are served, not the cached ones: {rows:?}", ); } + +#[test] +fn files_inside_the_racy_window_are_hash_confirmed_rather_than_reparsed() { + // Every file is pinned into the window, so no reuse decision here can be + // settled by the stat tuple: each one is reused only if its content hash + // confirms it. A hash that cannot be computed cannot confirm, and the whole + // corpus goes back to the parser. + let fx = Fixture::with_corpus(8); + let parser = fx.parser("parse.sh", ECHO_FILE); + let pinned: Vec = (0..8) + .map(|i| pin_inside_the_racy_window(&fx.file(i))) + .collect(); + + let (cold_rows, _) = fx.run(&parser, true); + + assert_eq!(fx.parses(), 8, "the cold run parses every file once"); + let now = SystemTime::now(); + assert!( + pinned.iter().all(|mtime| *mtime > now), + "the cold run must finish before the pinned mtimes, or the corpus is \ + not in the racy window and this test proves nothing", + ); + fx.reset_counter(); + + let (warm_rows, _) = fx.run(&parser, true); + + assert_eq!( + fx.parses(), + 0, + "a hash-confirmed file must not reach the parser", + ); + assert_eq!(warm_rows, cold_rows, "the warm run returns the same rows"); +} + +#[test] +fn content_that_leaves_the_stat_tuple_untouched_is_caught_by_the_hash() { + // The one edit the stat tuple cannot see: same size, same mtime, same + // inode, different bytes. Inside the racy window the content hash is the + // only thing standing between the query and a stale cached payload. + let fx = Fixture::with_corpus(8); + let parser = fx.parser("parse.sh", ECHO_FILE); + let edited = fx.file(3); + let pinned = pin_inside_the_racy_window(&edited); + + let (cold_rows, _) = fx.run(&parser, true); + assert!(cold_rows.contains(&"Integer(3)|Text(\"v1\")".to_string())); + let before = fs::metadata(&edited).unwrap(); + fx.reset_counter(); + + // `payload` is the same length for either tag, so rewriting in place leaves + // size, inode and device alone; restoring the mtime leaves the rest. + fs::write(&edited, payload(3, "v2")).unwrap(); + fs::File::open(&edited) + .unwrap() + .set_modified(pinned) + .unwrap(); + + let after = fs::metadata(&edited).unwrap(); + assert_eq!( + after.len(), + before.len(), + "the edit must not change the size" + ); + assert_eq!( + after.modified().unwrap(), + before.modified().unwrap(), + "the edit must not change the mtime", + ); + assert_eq!( + after.created().ok(), + before.created().ok(), + "the edit must not change the creation time", + ); + + let (rows, _) = fx.run(&parser, true); + + assert_eq!(fx.parses(), 1, "the edited file, and only it, is re-parsed"); + assert!( + rows.contains(&"Integer(3)|Text(\"v2\")".to_string()), + "the edited file's new rows are returned, not its cached ones: {rows:?}", + ); +}