From a429fde56ea3a93b71c425d611df823507d3d847 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 28 Sep 2026 21:54:37 +0000 Subject: [PATCH 01/20] feat(devmap-store): latest_symbols_for_file, one file's symbols with its generation GitPulse's code-intel panel (cmd_codeintel_symbols_for_file) reads the symbols a single file declares and compares the generation's head_sha with the commit it is showing. That reader was written against an unpushed local commit (c55d479) and vendored into GitPulse from it, so the vendored devmap-store carried FileSymbolsPage and Store::latest_symbols_for_file while DevCouncil did not, and GitPulse could not re-vendor without losing the feature. This lands the same code upstream. The rows and the head_sha are read from one pinned snapshot, like SearchPage: a second "latest" read could describe another generation. `None` means no generation; `Some` with no rows means the path has no indexed symbols. Corrupt spans go through checked_span, as every other symbol reader does. Tests in tests/symbols_for_file.rs cover the empty store, file scoping and source order, an unindexed path, the latest generation winning, and a corrupt span being refused. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01WQJD75wFPY52ckToQTu9FS --- rust/devmap-store/src/db.rs | 66 +++++++ rust/devmap-store/src/lib.rs | 10 +- rust/devmap-store/tests/symbols_for_file.rs | 190 ++++++++++++++++++++ 3 files changed, 261 insertions(+), 5 deletions(-) create mode 100644 rust/devmap-store/tests/symbols_for_file.rs diff --git a/rust/devmap-store/src/db.rs b/rust/devmap-store/src/db.rs index cc50d3f5..9eaf536b 100644 --- a/rust/devmap-store/src/db.rs +++ b/rust/devmap-store/src/db.rs @@ -1011,6 +1011,20 @@ pub struct FileEdges { pub analysis: Option, } +/// Symbols declared in one file in the latest generation, with that +/// generation's identity so a caller can compare `head_sha` to the one they +/// asked for. +/// +/// Travels as one page for the same reason [`SearchPage`] does: the rows and +/// the generation they came from must be one snapshot. A second "latest" +/// read for the head SHA could describe a different generation. +#[derive(Debug, Clone)] +pub struct FileSymbolsPage { + pub generation: u32, + pub head_sha: String, + pub rows: Vec, +} + #[derive(Debug, Clone, PartialEq)] pub struct StoredSymbol { pub name: String, @@ -6218,6 +6232,58 @@ impl Store { Ok(out) } + /// Symbols declared in `path` in the latest generation. + /// + /// `None` when the store holds no generation. An empty `rows` with `Some` + /// means the generation exists and this path has no indexed symbols (or + /// is not in the generation at all) — a completed read, not a missing map. + /// + /// Bounded to one file on purpose: callers that need the whole corpus use + /// [`Self::all_symbols`]. The generation's `head_sha` travels with the + /// rows so a mismatch against the SHA the caller asked for is visible. + pub fn latest_symbols_for_file(&self, path: &str) -> Result> { + let conn = lock_conn(&self.conn)?; + let Some((snapshot, generation)) = Self::latest_snapshot(&conn)? else { + return Ok(None); + }; + let head_sha: String = snapshot.query_row( + "SELECT head_sha FROM generations WHERE id = ?1", + params![generation], + |row| row.get(0), + )?; + let mut stmt = snapshot.prepare( + "SELECT n.name, n.qualified_name, n.kind, p.path, + n.span_start, n.span_end, n.is_exported, f.content_hash + FROM generation_nodes n + JOIN generation_files f ON f.generation_id = n.generation_id AND f.file_id = n.file_id + JOIN paths p ON p.id = n.file_id + WHERE n.generation_id = ?1 AND p.path = ?2 + ORDER BY n.span_start, n.ordinal", + )?; + let rows = stmt + .query_map(params![generation, path], |row| { + let name: String = row.get(0)?; + let path: String = row.get(3)?; + let (span_start, span_end) = checked_span(&path, &name, row.get(4)?, row.get(5)?)?; + Ok(StoredSymbol { + name, + qualified_name: row.get(1)?, + kind: row.get(2)?, + path, + span_start, + span_end, + is_exported: row.get::<_, i64>(6)? != 0, + content_hash: row.get::<_, i64>(7)? as u64, + }) + })? + .collect::>>()?; + Ok(Some(FileSymbolsPage { + generation, + head_sha, + rows, + })) + } + /// Every edge of the latest generation, rendered for comparison. /// Test-facing: proving incremental output equals cold output needs the /// whole edge set, not a count. diff --git a/rust/devmap-store/src/lib.rs b/rust/devmap-store/src/lib.rs index 1e0396dd..b05fd742 100644 --- a/rust/devmap-store/src/lib.rs +++ b/rust/devmap-store/src/lib.rs @@ -13,11 +13,11 @@ pub use coverage::{ }; pub use db::{ checked_min_confidence, current_git_head, BuildHistoryRow, CallersPage, DeadPage, FileEdges, - GenerationWriteOpts, PageSizeConversion, PendingClaim, PendingEnqueueReport, PendingReconcile, - PendingSupersede, PendingWatermark, QuerySourceFreshness, SearchPage, SourceTreeDelta, Store, - StoreStatus, StoredEdge, StoredFile, StoredSymbol, UnsupportedSchema, VacuumAction, - VacuumOutcome, WalCheckpointMode, WalCheckpointResult, WriteBreakdown, WriterLock, - MAX_PENDING_ATTEMPTS, + FileSymbolsPage, GenerationWriteOpts, PageSizeConversion, PendingClaim, PendingEnqueueReport, + PendingReconcile, PendingSupersede, PendingWatermark, QuerySourceFreshness, SearchPage, + SourceTreeDelta, Store, StoreStatus, StoredEdge, StoredFile, StoredSymbol, UnsupportedSchema, + VacuumAction, VacuumOutcome, WalCheckpointMode, WalCheckpointResult, WriteBreakdown, + WriterLock, MAX_PENDING_ATTEMPTS, }; pub use edge_index::{ edge_kind_from_stored, edge_resolution, resolution_kind_from_stored, resolution_kind_label, diff --git a/rust/devmap-store/tests/symbols_for_file.rs b/rust/devmap-store/tests/symbols_for_file.rs new file mode 100644 index 00000000..5bae815c --- /dev/null +++ b/rust/devmap-store/tests/symbols_for_file.rs @@ -0,0 +1,190 @@ +//! `Store::latest_symbols_for_file`: one file's symbols from one generation. +//! +//! GitPulse's code-intel panel lists the symbols of the file a diff touches and +//! compares the generation's `head_sha` with the commit it is showing. These +//! pin the contract that comparison relies on: the rows and the SHA come from +//! the same generation, an empty store is `None` rather than an empty page, an +//! unindexed path is an empty page rather than an error, and a corrupt span is +//! refused the way every other symbol reader refuses it. + +use std::fs; +use std::path::PathBuf; +use std::sync::atomic::{AtomicU64, Ordering}; +use std::time::{SystemTime, UNIX_EPOCH}; + +use devmap_analyze::analyze; +use devmap_extract::{extract_file, Extraction}; +use devmap_resolve::Resolver; +use devmap_store::{GenerationWriteOpts, Store}; + +fn tmp_dir(label: &str) -> PathBuf { + static COUNTER: AtomicU64 = AtomicU64::new(0); + let stamp = SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap() + .as_nanos(); + let seq = COUNTER.fetch_add(1, Ordering::Relaxed); + let dir = std::env::temp_dir().join(format!( + "devmap-symbols-for-file-{label}-{}-{stamp}-{seq}", + std::process::id() + )); + let _ = fs::remove_dir_all(&dir); + fs::create_dir_all(&dir).unwrap(); + dir +} + +fn save(store: &Store, extractions: &[Extraction], head_sha: &str) -> u32 { + let mut resolver = Resolver::new(); + resolver.index_extractions(extractions); + let resolution = resolver.resolve_all(extractions).unwrap(); + let analysis = analyze(extractions, &resolution); + store + .save_generation_with_metadata( + extractions, + &resolution, + &analysis, + GenerationWriteOpts::default(), + head_sha, + ) + .unwrap() +} + +fn corpus(lib_source: &str) -> Vec { + vec![ + extract_file("src/lib.py", lib_source), + extract_file("src/other.py", "def elsewhere():\n return 0\n"), + ] +} + +#[test] +fn an_empty_store_has_no_page_rather_than_an_empty_one() { + let store = Store::open_in_memory().unwrap(); + assert!( + store + .latest_symbols_for_file("src/lib.py") + .unwrap() + .is_none(), + "no generation means no map, which is not the same as a file with no symbols" + ); +} + +#[test] +fn rows_are_the_files_own_symbols_in_source_order_with_their_generation() { + let store = Store::open_in_memory().unwrap(); + let generation = save( + &store, + &corpus("def second():\n return 2\n\ndef first():\n return 1\n"), + "abc123", + ); + + let page = store + .latest_symbols_for_file("src/lib.py") + .unwrap() + .expect("a generation exists"); + assert_eq!(page.generation, generation); + assert_eq!(page.head_sha, "abc123"); + + let names: Vec<&str> = page.rows.iter().map(|row| row.name.as_str()).collect(); + assert!( + names.contains(&"second") && names.contains(&"first"), + "both declarations must be listed: {names:?}" + ); + assert!( + !names.contains(&"elsewhere"), + "another file's symbol leaked into the page: {names:?}" + ); + assert!( + page.rows.iter().all(|row| row.path == "src/lib.py"), + "every row must belong to the requested file: {:?}", + page.rows + ); + assert!( + page.rows + .windows(2) + .all(|pair| pair[0].span_start <= pair[1].span_start), + "rows must be in source order: {:?}", + page.rows + ); + assert!( + page.rows.iter().all(|row| row.span_end > row.span_start), + "spans must be real: {:?}", + page.rows + ); + + // The same rows every other symbol reader returns for this file. + let mut from_all: Vec<_> = store + .all_symbols() + .unwrap() + .into_iter() + .filter(|row| row.path == "src/lib.py") + .collect(); + let mut from_page = page.rows.clone(); + from_all.sort_by(|a, b| a.qualified_name.cmp(&b.qualified_name)); + from_page.sort_by(|a, b| a.qualified_name.cmp(&b.qualified_name)); + assert_eq!(from_page, from_all); +} + +#[test] +fn an_unindexed_path_is_an_empty_page_not_a_missing_map() { + let store = Store::open_in_memory().unwrap(); + let generation = save(&store, &corpus("def a():\n return 1\n"), "abc123"); + + let page = store + .latest_symbols_for_file("src/never_indexed.py") + .unwrap() + .expect("the generation exists even though this path is not in it"); + assert_eq!(page.generation, generation); + assert_eq!(page.head_sha, "abc123"); + assert!(page.rows.is_empty(), "{:?}", page.rows); +} + +#[test] +fn the_page_describes_the_latest_generation_only() { + let store = Store::open_in_memory().unwrap(); + save(&store, &corpus("def old_name():\n return 1\n"), "first"); + let latest = save(&store, &corpus("def new_name():\n return 1\n"), "second"); + + let page = store + .latest_symbols_for_file("src/lib.py") + .unwrap() + .expect("a generation exists"); + assert_eq!(page.generation, latest); + assert_eq!( + page.head_sha, "second", + "the SHA must name the generation the rows came from" + ); + let names: Vec<&str> = page.rows.iter().map(|row| row.name.as_str()).collect(); + assert!(names.contains(&"new_name"), "{names:?}"); + assert!( + !names.contains(&"old_name"), + "a superseded generation's symbol was returned: {names:?}" + ); +} + +/// Same policy as S-11 in `store_hardening.rs`: a negative stored span is +/// refused, never clamped to 0 and published as real. +#[test] +fn a_corrupt_span_is_refused_rather_than_published_as_zero() { + let dir = tmp_dir("corrupt-span"); + let db_path = dir.join("index.sqlite"); + let store = Store::open(&db_path).unwrap(); + save(&store, &corpus("def a():\n return 1\n"), "abc123"); + assert!(!store + .latest_symbols_for_file("src/lib.py") + .unwrap() + .expect("a generation exists") + .rows + .is_empty()); + + { + let conn = rusqlite::Connection::open(&db_path).unwrap(); + conn.execute("UPDATE generation_nodes SET span_start = -5", []) + .unwrap(); + } + + assert!( + store.latest_symbols_for_file("src/lib.py").is_err(), + "a corrupt span must fail the read" + ); + let _ = fs::remove_dir_all(&dir); +} From e63f184e030ffde3f77987305db011caebbf8c66 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 28 Sep 2026 22:02:05 +0000 Subject: [PATCH 02/20] fix(gusset-engine): record dc-glob 1.3.5 in the lockfile The v1.3.5 release bumped the workspace version, but rust/gusset-engine/Cargo.lock (outside the workspace) still named dc-glob 0.2.3, so every job that builds the umbrella staticlib with `cargo build --locked` failed before any test ran. Regenerated with `cargo update -p dc-glob --offline` against the pinned gusset 7a56c3c; gusset stays 0.0.1. The same change is on claude/zealous-faraday-56lcrv. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01WQJD75wFPY52ckToQTu9FS --- rust/gusset-engine/Cargo.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust/gusset-engine/Cargo.lock b/rust/gusset-engine/Cargo.lock index cf9deb99..9b893926 100644 --- a/rust/gusset-engine/Cargo.lock +++ b/rust/gusset-engine/Cargo.lock @@ -4,7 +4,7 @@ version = 4 [[package]] name = "dc-glob" -version = "0.2.3" +version = "1.3.5" [[package]] name = "devcouncil-gusset-engine" From a2c364ed42790e8daaef23ab0b4b5420e5d0ea61 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 28 Sep 2026 22:04:14 +0000 Subject: [PATCH 03/20] fix(deps): lock find-msvc-tools 0.1.14 so cc builds on Windows cc 1.2.67 does not compile against find-msvc-tools 0.1.13, which re-exports FILE_ATTRIBUTE_TEMPORARY as i32 where std wants u32; 0.1.14 restores u32. Every Windows leg that builds the workspace failed on it, main included. Same lockfile change as b6e9673 on claude/zealous-faraday-56lcrv, via `cargo update -p find-msvc-tools --precise 0.1.14`. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01WQJD75wFPY52ckToQTu9FS --- rust/Cargo.lock | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rust/Cargo.lock b/rust/Cargo.lock index 726c077f..09883343 100644 --- a/rust/Cargo.lock +++ b/rust/Cargo.lock @@ -573,9 +573,9 @@ dependencies = [ [[package]] name = "find-msvc-tools" -version = "0.1.13" +version = "0.1.14" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ef25905e51abafe4dcea6c15fec58c57b601cdbd0ee53d22ea1d3016c587d39b" +checksum = "aedcfb3409746eddb02b9e19ebda1c3394f759a152e48ee875a0844d1b955484" [[package]] name = "foldhash" From 92e5f8d75308da6e81ce3125845f01b1c976ab09 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 28 Sep 2026 22:13:18 +0000 Subject: [PATCH 04/20] fix(lsp): bound every wait on a language server (port of e3bcb6b) devmap-resolve's LSP client spawned its server with a raw Command::new, which devmap-extract's every_child_process_in_the_kernel_goes_through_the_runner guard rejects: a child outside the subprocess runner has no deadline and no output cap. That test failed on main and on every Rust leg of this PR. This is the lsp.rs and dc-proc part of e3bcb6b from claude/zealous-faraday-56lcrv, unchanged. Its rust.yml hunk (a static-musl step that builds on earlier workflow commits there) is left out. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01WQJD75wFPY52ckToQTu9FS --- rust/dc-proc/src/lib.rs | 96 ++++++++++ rust/devmap-resolve/src/lsp.rs | 315 +++++++++++++++++++++++++-------- 2 files changed, 338 insertions(+), 73 deletions(-) diff --git a/rust/dc-proc/src/lib.rs b/rust/dc-proc/src/lib.rs index 17a1f415..d8f16d07 100644 --- a/rust/dc-proc/src/lib.rs +++ b/rust/dc-proc/src/lib.rs @@ -320,6 +320,102 @@ pub fn run_bounded(command: &mut Command, bounds: Bounds) -> Result Option { + self.child.stdin.take() + } + + /// The child's stdout, once; `None` after the first call. + pub fn take_stdout(&mut self) -> Option { + self.child.stdout.take() + } + + /// The exit status if the child has already exited, without waiting. + pub fn try_status(&mut self) -> Option { + self.child.try_wait().ok().flatten() + } + + /// The program this session runs, for messages. + pub fn program(&self) -> &str { + &self.program + } + + /// Signal the child's process group and reap the child. Idempotent. + /// + /// The group is signalled even when the child has already exited: a + /// descendant it left behind is still in the group, and the group id + /// cannot be reused while any member lives. + pub fn kill(&mut self) { + if self.killed { + return; + } + self.killed = true; + kill_descendants(&mut self.child); + let _ = self.child.wait(); + } +} + +impl Drop for Session { + fn drop(&mut self) { + self.kill(); + } +} + +/// A command for [`spawn_session`], so a session's program is named through +/// this module like every other child the kernel starts. +pub fn session_command(program: impl AsRef) -> Command { + Command::new(program) +} + +/// Start `command` as a [`Session`]: stdin and stdout piped, stderr discarded +/// (a conversational child that fills an undrained stderr pipe blocks), and the +/// child leading its own process group. Spawning retries the same pre-exec +/// refusal [`run_bounded`] does, within `spawn_deadline`. +pub fn spawn_session(command: &mut Command, spawn_deadline: Duration) -> Result { + let program = command.get_program().to_string_lossy().into_owned(); + command + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::null()); + #[cfg(unix)] + { + use std::os::unix::process::CommandExt; + command.process_group(0); + } + let child = spawn_with_retry(|| command.spawn(), &program, Instant::now(), spawn_deadline)?; + Ok(Session { + child, + program, + killed: false, + }) +} + /// End the child and everything it started, then let the drain threads come /// home. /// diff --git a/rust/devmap-resolve/src/lsp.rs b/rust/devmap-resolve/src/lsp.rs index 338a089c..6e520116 100644 --- a/rust/devmap-resolve/src/lsp.rs +++ b/rust/devmap-resolve/src/lsp.rs @@ -10,12 +10,14 @@ use std::collections::{BTreeMap, BTreeSet}; use std::io::{BufRead, BufReader, Read, Write}; use std::path::{Path, PathBuf}; -use std::process::{Child, Command, Stdio}; use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::mpsc::{self, Receiver, RecvTimeoutError, SyncSender}; use std::sync::Arc; +use std::thread; use std::time::{Duration, Instant}; use devmap_extract::model::{EdgeKind, ExtractedSymbol, Extraction, Span}; +use devmap_extract::subprocess::{self, Session}; use serde_json::{json, Value}; use crate::model::{ @@ -39,6 +41,13 @@ pub const LSP_REQUEST_BUDGET: Duration = Duration::from_secs(8); /// rather than parsed into edges. pub const LSP_PAYLOAD_CAP: usize = 256 * 1024; +/// Longest header line accepted. A server streaming bytes with no newline would +/// otherwise grow one `String` without bound. +const LSP_HEADER_LINE_CAP: u64 = 8 * 1024; + +/// How long `shutdown` waits for a server to exit on its own before killing it. +const LSP_EXIT_GRACE: Duration = Duration::from_millis(500); + /// Servers this pass will start, in order, and only when the binary is on /// `PATH`. A missing binary is a status line that the pass did not run — never /// "zero new edges from a run". @@ -603,10 +612,25 @@ fn which_binary(name: &str) -> Option { None } +/// What the reader thread hands back: a decoded frame, or why it stopped. +enum Incoming { + Message(Value), + Failed(LspDidNotRun), + Eof, +} + +/// A language server session. +/// +/// Every wait here is bounded. Reads and writes happen on threads of their +/// own, and the caller only ever waits on a channel with `recv_timeout`: a +/// blocking `read_line` on the calling thread never returns from a server that +/// has gone quiet, which is how the per-request and per-pass budgets used to be +/// checked only between reads and never during one. On expiry the session is +/// killed, which closes the pipes and ends both threads. struct LspClient { - child: Child, - reader: BufReader, - writer: std::process::ChildStdin, + session: Session, + incoming: Receiver, + outgoing: Option>>, next_id: u64, server_version: Option, deadline: Instant, @@ -614,6 +638,57 @@ struct LspClient { repo_root: PathBuf, } +/// One frame from `reader`: headers, then a body of at most +/// [`LSP_PAYLOAD_CAP`] bytes. +fn read_frame(reader: &mut impl BufRead) -> Incoming { + let mut content_length: Option = None; + loop { + let mut line = Vec::new(); + let read = match reader + .by_ref() + .take(LSP_HEADER_LINE_CAP) + .read_until(b'\n', &mut line) + { + Ok(read) => read, + Err(error) => { + return Incoming::Failed(LspDidNotRun::Protocol(format!( + "read header failed: {error}" + ))) + } + }; + if read == 0 { + return Incoming::Eof; + } + if !line.ends_with(b"\n") { + return Incoming::Failed(LspDidNotRun::Protocol(format!( + "header line longer than {LSP_HEADER_LINE_CAP} bytes" + ))); + } + let text = String::from_utf8_lossy(&line); + let trimmed = text.trim_end(); + if trimmed.is_empty() { + break; + } + if let Some(value) = trimmed.strip_prefix("Content-Length:") { + content_length = value.trim().parse().ok(); + } + } + let Some(length) = content_length else { + return Incoming::Failed(LspDidNotRun::Protocol("missing Content-Length".into())); + }; + if length > LSP_PAYLOAD_CAP { + return Incoming::Failed(LspDidNotRun::OversizedPayload { bytes: length }); + } + let mut body = vec![0u8; length]; + if let Err(error) = reader.read_exact(&mut body) { + return Incoming::Failed(LspDidNotRun::Protocol(format!("read body failed: {error}"))); + } + match serde_json::from_slice(&body) { + Ok(value) => Incoming::Message(value), + Err(error) => Incoming::Failed(LspDidNotRun::Protocol(format!("json failed: {error}"))), + } +} + impl LspClient { fn spawn( spec: &LspServerSpec, @@ -621,28 +696,48 @@ impl LspClient { repo_root: &Path, deadline: Instant, ) -> Result { - let mut command = Command::new(binary); - command - .args(spec.args) - .stdin(Stdio::piped()) - .stdout(Stdio::piped()) - .stderr(Stdio::null()) - .current_dir(repo_root); - let mut child = command - .spawn() + let mut command = subprocess::session_command(binary); + command.args(spec.args).current_dir(repo_root); + let spawn_budget = deadline.saturating_duration_since(Instant::now()); + let mut session = subprocess::spawn_session(&mut command, spawn_budget) .map_err(|error| LspDidNotRun::Protocol(format!("spawn {}: {error}", spec.binary)))?; - let stdout = child - .stdout - .take() + let stdout = session + .take_stdout() .ok_or_else(|| LspDidNotRun::Protocol("missing stdout".into()))?; - let stdin = child - .stdin - .take() + let mut stdin = session + .take_stdin() .ok_or_else(|| LspDidNotRun::Protocol("missing stdin".into()))?; + + // Bounded to a few frames: a server that stops reading backs the + // writer up, and the caller's recv_timeout on the reply is what ends it. + let (incoming_tx, incoming) = mpsc::sync_channel(16); + thread::spawn(move || { + let mut reader = BufReader::new(stdout); + loop { + let frame = read_frame(&mut reader); + let last = !matches!(frame, Incoming::Message(_)); + if incoming_tx.send(frame).is_err() || last { + return; + } + } + }); + let (outgoing, outgoing_rx) = mpsc::sync_channel::>(16); + thread::spawn(move || { + for frame in outgoing_rx { + if stdin + .write_all(&frame) + .and_then(|()| stdin.flush()) + .is_err() + { + return; + } + } + }); + let mut client = Self { - child, - reader: BufReader::new(stdout), - writer: stdin, + session, + incoming, + outgoing: Some(outgoing), next_id: 1, server_version: None, deadline, @@ -715,7 +810,13 @@ impl LspClient { fn shutdown(&mut self) -> Result<(), LspDidNotRun> { let _ = self.request("shutdown", json!(null)); let _ = self.notify("exit", json!(null)); - let _ = self.child.wait(); + // A polite exit gets a grace period, never an unbounded wait: a server + // that ignores `exit` is killed with the rest of its process group. + let grace = Instant::now() + LSP_EXIT_GRACE; + while self.session.try_status().is_none() && Instant::now() < grace { + thread::sleep(Duration::from_millis(10)); + } + self.kill(); Ok(()) } @@ -769,67 +870,50 @@ impl LspClient { if body.len() > LSP_PAYLOAD_CAP { return Err(LspDidNotRun::OversizedPayload { bytes: body.len() }); } - write!(self.writer, "Content-Length: {}\r\n\r\n", body.len()) - .map_err(|error| LspDidNotRun::Protocol(format!("write header failed: {error}")))?; - self.writer - .write_all(&body) - .map_err(|error| LspDidNotRun::Protocol(format!("write body failed: {error}")))?; - self.writer - .flush() - .map_err(|error| LspDidNotRun::Protocol(format!("flush failed: {error}")))?; - Ok(()) + let mut frame = format!("Content-Length: {}\r\n\r\n", body.len()).into_bytes(); + frame.extend_from_slice(&body); + let Some(outgoing) = &self.outgoing else { + return Err(LspDidNotRun::Protocol("the session is closed".into())); + }; + // try_send, not send: a writer backed up behind a server that stopped + // reading must not block this thread past the deadline. + match outgoing.try_send(frame) { + Ok(()) => Ok(()), + Err(mpsc::TrySendError::Full(_)) => { + self.kill(); + Err(LspDidNotRun::TimedOut) + } + Err(mpsc::TrySendError::Disconnected(_)) => { + Err(LspDidNotRun::Protocol("write to server failed".into())) + } + } } fn read_message(&mut self, request_deadline: Instant) -> Result { - let mut content_length: Option = None; - loop { - if Instant::now() >= request_deadline { + let wait = request_deadline.saturating_duration_since(Instant::now()); + match self.incoming.recv_timeout(wait) { + Ok(Incoming::Message(value)) => Ok(value), + Ok(Incoming::Failed(reason)) => { self.kill(); - return Err(LspDidNotRun::TimedOut); - } - let mut line = String::new(); - let read = self - .reader - .read_line(&mut line) - .map_err(|error| LspDidNotRun::Protocol(format!("read header failed: {error}")))?; - if read == 0 { - let status = self.child.try_wait().ok().flatten(); - return Err(LspDidNotRun::NonZeroExit { - code: status.and_then(|status| status.code()), - }); + Err(reason) } - let trimmed = line.trim_end(); - if trimmed.is_empty() { - break; + Ok(Incoming::Eof) | Err(RecvTimeoutError::Disconnected) => { + let code = self.session.try_status().and_then(|status| status.code()); + self.kill(); + Err(LspDidNotRun::NonZeroExit { code }) } - if let Some(value) = trimmed.strip_prefix("Content-Length:") { - content_length = value.trim().parse().ok(); + Err(RecvTimeoutError::Timeout) => { + self.kill(); + Err(LspDidNotRun::TimedOut) } } - let length = content_length - .ok_or_else(|| LspDidNotRun::Protocol("missing Content-Length".into()))?; - if length > LSP_PAYLOAD_CAP { - self.kill(); - return Err(LspDidNotRun::OversizedPayload { bytes: length }); - } - let mut body = vec![0u8; length]; - self.reader - .read_exact(&mut body) - .map_err(|error| LspDidNotRun::Protocol(format!("read body failed: {error}")))?; - serde_json::from_slice(&body) - .map_err(|error| LspDidNotRun::Protocol(format!("json failed: {error}"))) } fn kill(&mut self) { - let _ = self.child.kill(); - let _ = self.child.wait(); - } -} - -impl Drop for LspClient { - fn drop(&mut self) { - let _ = self.child.kill(); - let _ = self.child.wait(); + // Dropping the sender ends the writer thread once it is unblocked; the + // kill closes the pipes, which unblocks both threads. + self.outgoing = None; + self.session.kill(); } } @@ -906,3 +990,88 @@ fn location_from_value(value: &Value, repo_root: &Path) -> Option { character, }) } + +#[cfg(all(test, unix))] +mod session_bounds { + use super::*; + + fn spec(script: String) -> LspServerSpec { + LspServerSpec { + binary: "sh", + args: Box::leak( + vec!["-c", Box::leak(script.into_boxed_str()) as &str].into_boxed_slice(), + ), + extensions: &[".rs"], + } + } + + fn alive(pid: i32) -> bool { + let mut probe = subprocess::session_command("kill"); + probe.args(["-0", &pid.to_string()]); + let bounds = subprocess::Bounds { + deadline: Duration::from_secs(5), + stdout_cap: 1024, + stderr_cap: 1024, + }; + subprocess::run_bounded(&mut probe, bounds).is_ok_and(|done| done.status.success()) + } + + /// A server that reads everything and never answers. The request budget + /// used to be checked only between blocking reads, so this hung the pass + /// forever; it must now end at the deadline, with the server's own child + /// killed along with it. + #[test] + fn a_silent_server_times_out_at_the_deadline_and_takes_its_children_with_it() { + let dir = std::env::temp_dir().join(format!("lsp-silent-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let pidfile = dir.join("child.pid"); + let script = format!( + "sleep 1000 & echo $! > {}; cat > /dev/null", + pidfile.display() + ); + let started = Instant::now(); + let deadline = started + Duration::from_secs(2); + let result = LspClient::spawn(&spec(script), Path::new("sh"), &dir, deadline); + let elapsed = started.elapsed(); + assert!( + matches!(result, Err(LspDidNotRun::TimedOut)), + "a silent server must time out, got {:?}", + result.err() + ); + assert!( + elapsed < Duration::from_secs(5), + "took {elapsed:?} against a 2s deadline" + ); + let pid: i32 = std::fs::read_to_string(&pidfile) + .unwrap() + .trim() + .parse() + .unwrap(); + let reaped_by = Instant::now() + Duration::from_secs(2); + while alive(pid) && Instant::now() < reaped_by { + thread::sleep(Duration::from_millis(20)); + } + assert!(!alive(pid), "the server's child {pid} outlived the kill"); + let _ = std::fs::remove_dir_all(&dir); + } + + /// Bytes with no newline used to grow one header String without bound. + #[test] + fn an_endless_header_line_is_refused_not_buffered() { + let script = "while :; do printf 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'; done".to_string(); + let started = Instant::now(); + let result = LspClient::spawn( + &spec(script), + Path::new("sh"), + &std::env::temp_dir(), + started + Duration::from_secs(20), + ); + match result { + Err(LspDidNotRun::Protocol(reason)) => { + assert!(reason.contains("header line longer"), "{reason}") + } + other => panic!("expected a header-cap refusal, got {:?}", other.err()), + } + assert!(started.elapsed() < Duration::from_secs(10)); + } +} From dd7695c24428cc8f1c406e08d3a893feb3429c7e Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 28 Sep 2026 22:13:18 +0000 Subject: [PATCH 05/20] style(policy): gofmt zfix_split_test.go The clients jobs run `gofmt -l .` and failed on this file, on main as well. Byte-identical to the file on claude/zealous-faraday-56lcrv (from f0ec3e2). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01WQJD75wFPY52ckToQTu9FS --- backend/go_orchestrator/policy/zfix_split_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/backend/go_orchestrator/policy/zfix_split_test.go b/backend/go_orchestrator/policy/zfix_split_test.go index 228049fb..cbcdf695 100644 --- a/backend/go_orchestrator/policy/zfix_split_test.go +++ b/backend/go_orchestrator/policy/zfix_split_test.go @@ -167,12 +167,12 @@ func diffCandidate(prefixIdx, sepIdx, payloadIdx int) string { func differentialGate() (CommandGate, *dc.Task) { return CommandGate{ - GlobalAllowedCommands: []string{"echo", "echo *", "cat *", "true", ":"}, - HardRules: true, - }, &dc.Task{ - ID: "TASK-001", - AllowedCommands: []string{"echo *", "cat *"}, - } + GlobalAllowedCommands: []string{"echo", "echo *", "cat *", "true", ":"}, + HardRules: true, + }, &dc.Task{ + ID: "TASK-001", + AllowedCommands: []string{"echo *", "cat *"}, + } } // runDifferentialCase executes one candidate under a real sh in its own From fba2df51019f359c9fdb0e8290d45ba0dcdf23dd Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 28 Sep 2026 22:16:51 +0000 Subject: [PATCH 06/20] test(devmap-store): declare symbols_for_file's parse requirement devmap-cli's every_integration_test_is_classified_against_the_parse_feature requires each integration test to be either feature-off safe or gated on `parse`. symbols_for_file builds its generations with extract_file and the resolver, so it is gated like retention_churn. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01WQJD75wFPY52ckToQTu9FS --- rust/devmap-store/Cargo.toml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/rust/devmap-store/Cargo.toml b/rust/devmap-store/Cargo.toml index d8c1ac57..e1e42df8 100644 --- a/rust/devmap-store/Cargo.toml +++ b/rust/devmap-store/Cargo.toml @@ -34,6 +34,11 @@ parse = ["devmap-extract/parse", "devmap-resolve/parse", "devmap-analyze/parse"] name = "retention_churn" required-features = ["parse"] +# Builds its generations from extracted source. +[[test]] +name = "symbols_for_file" +required-features = ["parse"] + # The carry-forward guards these tests exercise only exist when an identity can # be computed: with `parse` off, `current_payload_identity` returns `None`, no # payload is ever carried, and the partial-extraction write is refused outright From fea3992f1c85e99595db6ddbde04bcfd71dbc990 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 28 Sep 2026 02:09:53 +0000 Subject: [PATCH 07/20] feat(gusset): harden and deepen the dc-glob engine on Gusset The umbrella and gussetfn were a startup health check with gaps at the boundary. This closes the ones found by auditing it end to end: - Stale links: Go's build and test caches key a cgo package on its flag text, not on the archive bytes, so after a Rust rebuild `go build` linked the old archive and `go test` could replay a cached "ok" (demonstrated with a marker string: invisible without -a). rust/gusset-engine/cgo-env.sh always runs the incremental cargo build and folds the archive's sha256 into CGO_CFLAGS. verify.sh (which built only when the archive was missing), ci-local.mjs and the setup-gusset action now use it. - Poison: the shared handle lived in a sync.Once, so one poisoned handle failed every later Match for the life of the process. A poisoned handle is now retired and the next call opens a new one; a failed Open is retried instead of latched. Close() shuts the engine for servers. - Allocator blindness: no archive declared a global allocator, so gusset.Stats() read zero and AdviseMemoryLimit subtracted nothing. The umbrella installs gusset's Counting allocator; a test pins that a Rust buffer is counted once. - MatchAny made one crossing per pattern. Opcode 1 carries a whole list, split into frames under the 4 KiB inline limit, validating every pattern before matching so the answer cannot depend on order. - The firewall was only ever proven against gusset's diagnostic engine. SelfTest panics inside this archive (opcode 0x7fff0001, header-only) on a throwaway handle and requires ErrPanic, ErrPoisoned, and a clean match on the shared handle. Check stays panic-free: Manvi runs it in serve. - DrainLogs forwards gusset's Rust log ring, which nothing read. - The lockfile named gusset 0.0.1 / dc-glob 0.2.3, so `cargo build --locked` failed against current gusset; the setup-gusset pin moves from 7a56c3c to b6f5200 (gusset main, 0.0.2) to match it. Tests: the CPython fixture through the boundary, MatchAny against Go across frame splits, poison recovery under 64-way concurrency, 200 micro-deadlines without poison, and FuzzMatchParity. Against a checkout of gusset at the pin (b6f5200), with the archive built by cgo-env.sh: race detector with GOGC=1 -count=15 clean, a 90 s fuzz clean, gusset-check ok with non-zero Rust stats. An earlier 4 min fuzz (1.94M execs) against the pre-allocator archive also found no divergence. Protects I2 (panic firewall), I3 (cancellation), I4 (bounded handles) and I6 (the archive under test is the archive linked). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01FUNo5WZs78gWyCSXf2nRhm (cherry picked from commit 3197897490bff4fe2607d885cfe369ecc53363fb) --- .github/actions/setup-gusset/action.yml | 7 +- .../cmd/devcouncil/gusset_check.go | 13 +- .../go_orchestrator/gussetfn/boundary_test.go | 363 ++++++++++++++++++ backend/go_orchestrator/gussetfn/match.go | 273 +++++++++++-- rust/gusset-engine/Cargo.lock | 2 +- rust/gusset-engine/cgo-env.sh | 57 +++ rust/gusset-engine/src/lib.rs | 154 +++++++- rust/verify.sh | 17 +- scripts/ci-local.mjs | 9 + 9 files changed, 852 insertions(+), 43 deletions(-) create mode 100644 backend/go_orchestrator/gussetfn/boundary_test.go create mode 100755 rust/gusset-engine/cgo-env.sh diff --git a/.github/actions/setup-gusset/action.yml b/.github/actions/setup-gusset/action.yml index 8df1f023..7f5ad2ff 100644 --- a/.github/actions/setup-gusset/action.yml +++ b/.github/actions/setup-gusset/action.yml @@ -25,7 +25,7 @@ runs: # Pinned to the sibling revision this tree was linked against. # Bump deliberately; tracking main would let a Gusset push fail a # DevCouncil release without a DevCouncil commit. - ref: 7a56c3c98d026b4e445f687140c90d70755e8590 + ref: b6f5200bfff4730e4e5268bead7e743d684cb67e path: third_party/gusset persist-credentials: false @@ -102,3 +102,8 @@ runs: cargo build --release --locked fi test -f target/release/libgusset.a + # Key Go's build and test caches on the archive (see cgo-env.sh): + # setup-go restores GOCACHE, and without this a cached link or a + # cached "ok" from an older archive is replayed. + # The -L is already in gussetfn/engine.go; only the key is needed here. + ./cgo-env.sh --no-build | grep -v "^CGO_LDFLAGS=" >> "$GITHUB_ENV" diff --git a/backend/go_orchestrator/cmd/devcouncil/gusset_check.go b/backend/go_orchestrator/cmd/devcouncil/gusset_check.go index 0e6ee85d..a4b2e1f5 100644 --- a/backend/go_orchestrator/cmd/devcouncil/gusset_check.go +++ b/backend/go_orchestrator/cmd/devcouncil/gusset_check.go @@ -6,15 +6,24 @@ import ( "github.com/bharathvbcr/DevCouncil/backend/go_orchestrator/console" "github.com/bharathvbcr/DevCouncil/backend/go_orchestrator/gussetfn" + "github.com/bharathvbcr/gusset" ) func runGussetCheck() int { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() - if err := gussetfn.Check(ctx); err != nil { + err := gussetfn.SelfTest(ctx) + // The check panics on purpose; what Rust logged about it, and anything + // else, belongs on stderr rather than in a ring nobody reads. + if _, drainErr := gussetfn.DrainLogs(console.Stderr()); drainErr != nil { + console.Errorf("gusset-check: draining Rust logs: %v\n", drainErr) + } + if err != nil { console.Errorf("gusset-check: %v\n", err) return 1 } - console.Println("gusset-check: ok") + st := gusset.Stats() + console.Printf("gusset-check: ok (parity, match-any, panic firewall; rust live=%dB peak=%dB allocs=%d; go threads=%d)\n", + st.LiveBytes, st.PeakBytes, st.AllocCount, gusset.Threads()) return 0 } diff --git a/backend/go_orchestrator/gussetfn/boundary_test.go b/backend/go_orchestrator/gussetfn/boundary_test.go new file mode 100644 index 00000000..48d791bd --- /dev/null +++ b/backend/go_orchestrator/gussetfn/boundary_test.go @@ -0,0 +1,363 @@ +package gussetfn + +import ( + "bytes" + "context" + "errors" + "os" + "path/filepath" + "runtime" + "strings" + "sync" + "testing" + "time" + "unicode/utf8" + + "github.com/bharathvbcr/DevCouncil/backend/go_orchestrator/fnmatch" + "github.com/bharathvbcr/gusset" +) + +// parityRows loads the CPython fnmatch table dc-glob and fnmatch both pin. +func parityRows(t testing.TB) [][3]string { + t.Helper() + _, file, _, ok := runtime.Caller(0) + if !ok { + t.Fatal("cannot locate boundary_test.go") + } + path := filepath.Join(filepath.Dir(file), "..", "..", "..", "testdata", "fnmatch-parity.tsv") + raw, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read parity fixture: %v", err) + } + var rows [][3]string + for _, line := range strings.Split(string(raw), "\n") { + if strings.HasPrefix(line, "#") || strings.TrimSpace(line) == "" { + continue + } + cols := strings.Split(line, "\t") + if len(cols) != 3 { + t.Fatalf("malformed fixture row %q", line) + } + rows = append(rows, [3]string{cols[0], cols[1], cols[2]}) + } + if len(rows) < 800 { + t.Fatalf("only %d fixture rows loaded", len(rows)) + } + return rows +} + +// The CPython table, through the boundary. dc-glob's own test reads it +// in-process; this reads it through cgo, the ring and the frame codec, which +// is the path a host actually takes. +func TestParityFixtureAcrossTheBoundary(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + for _, row := range parityRows(t) { + pattern, name, want := row[0], row[1], row[2] == "true" + got, err := Match(ctx, pattern, name) + if err != nil { + t.Fatalf("Match(%q, %q): %v", pattern, name, err) + } + if got != want { + t.Errorf("Match(%q, %q) = %v across the boundary, CPython says %v", pattern, name, got, want) + } + if goGot := fnmatch.Match(pattern, name); goGot != got { + t.Errorf("gusset %v and Go fnmatch %v disagree on (%q, %q)", got, goGot, pattern, name) + } + } +} + +// MatchAny over the fixture's patterns agrees with the Go reference for each +// name, including lists that must be split over several frames. +func TestMatchAnyAgreesWithGoAcrossFrames(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + rows := parityRows(t) + patterns := make([]string, 0, len(rows)) + for _, r := range rows { + patterns = append(patterns, r[0]) + } + for _, list := range [][]string{patterns[:1], patterns[:40], patterns} { + if _, n := encodeAny("x", list); len(list) == len(patterns) && n >= len(list) { + t.Fatal("the full list fit one frame; the split path is not exercised") + } + for _, r := range rows[:200] { + got, err := MatchAny(ctx, list, r[1]) + if err != nil { + t.Fatalf("MatchAny(%d patterns, %q): %v", len(list), r[1], err) + } + if want := fnmatch.MatchAny(list, r[1]); got != want { + t.Errorf("MatchAny(%d patterns, %q) = %v, Go says %v", len(list), r[1], got, want) + } + } + } +} + +func TestEncodeAnyStaysUnderTheInlineLimit(t *testing.T) { + long := strings.Repeat("a", maxField) + list := []string{long, long, long, long, long} + name := strings.Repeat("b", maxField) + for rest := list; len(rest) > 0; { + frame, n := encodeAny(name, rest) + if n == 0 { + t.Fatal("encodeAny made no progress") + } + if len(frame) > maxFrame { + t.Fatalf("frame of %d bytes exceeds %d", len(frame), maxFrame) + } + rest = rest[n:] + } + many := make([]string, maxPatterns+10) + if _, n := encodeAny("", many); n > maxPatterns { + t.Fatalf("frame carries %d patterns, cap is %d", n, maxPatterns) + } +} + +func TestMatchAnyRefusesBeforeTheCall(t *testing.T) { + ctx := context.Background() + long := strings.Repeat("a", maxField+1) + if _, err := MatchAny(ctx, []string{"*", long}, "a"); err == nil { + t.Fatal("an overlong pattern late in the list was accepted") + } + if _, err := MatchAny(ctx, []string{"*"}, long); err == nil { + t.Fatal("an overlong name was accepted") + } + if _, err := MatchAny(nil, []string{"*"}, "a"); err == nil { + t.Fatal("a nil context was accepted") + } + cctx, cancel := context.WithCancel(ctx) + cancel() + if _, err := MatchAny(cctx, nil, "a"); !errors.Is(err, context.Canceled) { + t.Fatalf("empty list on a cancelled context = %v", err) + } +} + +// A malformed pattern after a matching one is still an error: the engine +// validates the frame before matching, so the answer cannot depend on order. +func TestMatchAnyInvalidUTF8IsAnErrorNotAPoison(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + _, err := MatchAny(ctx, []string{"*.py", "\xff"}, "a.py") + if err == nil || !strings.Contains(err.Error(), "UTF-8") { + t.Fatalf("got %v, want a UTF-8 error", err) + } + if poisoned(err) { + t.Fatalf("invalid UTF-8 poisoned the handle: %v", err) + } + if got, err := MatchAny(ctx, []string{"*.py"}, "a.py"); err != nil || !got { + t.Fatalf("next MatchAny = %v, %v", got, err) + } +} + +// A poisoned shared handle is retired and the next call opens a new one. +// Before, the handle was held by a sync.Once and every later call failed. +func TestPoisonedHandleIsReplaced(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + if _, err := engine(); err != nil { + t.Fatal(err) + } + bad, err := gusset.Open(gusset.WithPoolSize(1), gusset.WithOpcode(opcodeSelfTestPanic)) + if err != nil { + t.Fatal(err) + } + if _, err := bad.Call(ctx, nil); !errors.Is(err, gusset.ErrPanic) { + t.Fatalf("self-test panic = %v", err) + } + prev := current.Swap(bad) + + if _, err := Match(ctx, "*.py", "a.py"); !errors.Is(err, gusset.ErrPoisoned) { + t.Fatalf("Match on the poisoned handle = %v, want ErrPoisoned", err) + } + if current.Load() == bad { + t.Fatal("the poisoned handle is still the shared handle") + } + got, err := Match(ctx, "*.py", "a.py") + if err != nil || !got { + t.Fatalf("Match after replacement = %v, %v", got, err) + } + if prev != nil { + _ = prev.Close() + } +} + +// Many goroutines hit a handle that becomes poisoned under them. Every call +// returns (no hang), every error is a Gusset error, and exactly one new +// handle serves afterwards. +func TestPoisonUnderConcurrencyRecovers(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) + defer cancel() + for round := 0; round < 5; round++ { + if _, err := engine(); err != nil { + t.Fatal(err) + } + bad, err := gusset.Open(gusset.WithPoolSize(1), gusset.WithOpcode(opcodeSelfTestPanic)) + if err != nil { + t.Fatal(err) + } + _, _ = bad.Call(ctx, nil) + if prev := current.Swap(bad); prev != nil { + _ = prev.Close() + } + var wg sync.WaitGroup + errs := make(chan error, 64) + for g := 0; g < 64; g++ { + wg.Add(1) + go func() { + defer wg.Done() + for i := 0; i < 10; i++ { + got, err := Match(ctx, "*.py", "a.py") + if err == nil && !got { + errs <- errors.New("match returned false") + return + } + if err != nil && ctx.Err() != nil { + errs <- err + return + } + } + }() + } + wg.Wait() + close(errs) + for err := range errs { + t.Error(err) + } + if got, err := Match(ctx, "*.py", "a.py"); err != nil || !got { + t.Fatalf("round %d: settled Match = %v, %v", round, got, err) + } + } +} + +func TestCancellationDoesNotPoison(t *testing.T) { + for i := 0; i < 200; i++ { + ctx, cancel := context.WithTimeout(context.Background(), time.Duration(i%7)*time.Microsecond) + _, err := MatchAny(ctx, []string{"*.rs", "*.py"}, "a.py") + cancel() + if err != nil && poisoned(err) { + t.Fatalf("iteration %d: a deadline poisoned the handle: %v", i, err) + } + } + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + if got, err := Match(ctx, "*.py", "a.py"); err != nil || !got { + t.Fatalf("Match after cancellations = %v, %v", got, err) + } +} + +func TestDrainLogsWritesWhatRustLogged(t *testing.T) { + var buf bytes.Buffer + if _, err := DrainLogs(&buf); err != nil { + t.Fatal(err) + } + // A second drain of an idle ring is empty; the call must not block. + buf.Reset() + n, err := DrainLogs(&buf) + if err != nil || n != buf.Len() { + t.Fatalf("DrainLogs = %d, %v with %d bytes written", n, err, buf.Len()) + } +} + +func TestSelfTestProvesTheFirewall(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + if err := SelfTest(ctx); err != nil { + t.Fatal(err) + } +} + +// FuzzMatchParity is the differential check across the boundary: for valid +// UTF-8 the Rust engine and the Go matcher must agree, and anything else is +// an engine error that leaves the handle usable. Run with +// go test -fuzz=FuzzMatchParity ./gussetfn. +func FuzzMatchParity(f *testing.F) { + for _, seed := range [][2]string{ + {"*.py", "src/foo.py"}, {"[!a-c]*", "d"}, {"[c-a-e]", "e"}, {"[]]", "]"}, + {"[!]", "!"}, {"**?*", "ab"}, {"\xff", "a"}, {"[\\]", "\\"}, {"a*b*c*d*e", "aXbXcXdXe"}, + } { + f.Add(seed[0], seed[1]) + } + ctx := context.Background() + f.Fuzz(func(t *testing.T, pattern, name string) { + if len(pattern) > maxField || len(name) > maxField { + if _, err := Match(ctx, pattern, name); err == nil { + t.Fatal("an overlong field was accepted") + } + return + } + got, err := Match(ctx, pattern, name) + if !utf8.ValidString(pattern) || !utf8.ValidString(name) { + if err == nil { + t.Fatalf("invalid UTF-8 (%q, %q) matched instead of failing", pattern, name) + } + if poisoned(err) { + t.Fatalf("invalid UTF-8 poisoned the handle: %v", err) + } + return + } + if err != nil { + t.Fatalf("Match(%q, %q): %v", pattern, name, err) + } + if want := fnmatch.Match(pattern, name); got != want { + t.Fatalf("Match(%q, %q): gusset %v, Go %v", pattern, name, got, want) + } + any, err := MatchAny(ctx, []string{pattern, pattern + "x"}, name) + if err != nil { + t.Fatalf("MatchAny: %v", err) + } + if want := fnmatch.MatchAny([]string{pattern, pattern + "x"}, name); any != want { + t.Fatalf("MatchAny(%q, %q): gusset %v, Go %v", pattern, name, any, want) + } + }) +} + +// Close is last: it shuts the process-wide engine. reopen restores it so a +// -count>1 run still has an engine. +func TestZZCloseRefusesLaterCalls(t *testing.T) { + defer func() { closed.Store(false) }() + if _, err := engine(); err != nil { + t.Fatal(err) + } + if err := Close(); err != nil { + t.Fatalf("Close: %v", err) + } + if _, err := Match(context.Background(), "*", "a"); !errors.Is(err, ErrClosed) { + t.Fatalf("Match after Close = %v, want ErrClosed", err) + } + if err := Close(); err != nil { + t.Fatalf("second Close: %v", err) + } +} + +// The umbrella installs gusset's Counting allocator. A Rust buffer is then +// counted once — by Gusset's own buffer accounting or by the global +// allocator, not both — and it is counted at all: before, Stats read zero. +func TestStatsCountBuffersOnce(t *testing.T) { + if _, err := engine(); err != nil { + t.Fatal(err) + } + h, err := gusset.Open(gusset.WithPoolSize(1)) + if err != nil { + t.Fatal(err) + } + defer h.Close() + const n = 8 << 20 + before := gusset.Stats().LiveBytes + buf, err := h.NewBuffer(n) + if err != nil { + t.Fatal(err) + } + during := gusset.Stats().LiveBytes + if err := buf.Free(); err != nil { + t.Fatal(err) + } + after := gusset.Stats().LiveBytes + delta := int64(during) - int64(before) + // Other goroutines' Rust allocations are small next to 8 MiB. + if delta < n || delta > n+n/2 { + t.Fatalf("an %d-byte buffer moved LiveBytes by %d; want about %d once", n, delta, n) + } + if int64(after)-int64(before) > n/8 { + t.Fatalf("LiveBytes %d after Free, %d before", after, before) + } +} diff --git a/backend/go_orchestrator/gussetfn/match.go b/backend/go_orchestrator/gussetfn/match.go index 8bd8f3ca..1fad9cc3 100644 --- a/backend/go_orchestrator/gussetfn/match.go +++ b/backend/go_orchestrator/gussetfn/match.go @@ -4,8 +4,9 @@ // Gusset failure has nowhere to go except "no match" or "match" — one of // those opens a path the gate meant to refuse, the other denies a write the // gate meant to allow, and a poisoned handle would do it for every later -// check. Callers that can return an error use Match. The devcouncil and -// manvi gusset-check commands are the production callers. +// check. Callers that can return an error use Match. The gusset-check +// commands of devcouncil, manvi, jarvis and GitPulse run SelfTest; Manvi's +// serve policy plane runs Check (never a deliberate panic) before answering. package gussetfn import ( @@ -13,7 +14,9 @@ import ( "encoding/binary" "errors" "fmt" + "io" "sync" + "sync/atomic" "unicode/utf8" "github.com/bharathvbcr/DevCouncil/backend/go_orchestrator/fnmatch" @@ -25,24 +28,136 @@ import ( // copy on the cgo thread. const maxField = 1024 +// Opcodes the umbrella registers (rust/gusset-engine). Opcode 0 is Match. +const ( + opcodeMatchAny uint32 = 1 + opcodeSelfTestPanic uint32 = 0x7fff_0001 +) + +// maxFrame is Gusset's inline-copy limit. A MatchAny list is split into +// frames under it; a bigger []byte would be refused by Call. +const maxFrame = 4096 + +// maxPatterns matches MAX_PATTERNS in the umbrella. +const maxPatterns = 1024 + +// poolSize is the shared handle's worker count. +const poolSize = 4 + +// ErrClosed is returned after Close: the engine is shut for this process. +var ErrClosed = errors.New("gusset: engine is closed") + var ( - engineOnce sync.Once - engineH *gusset.Handle - engineErr error + registerOnce sync.Once + // engineMu serialises opening; the hot path reads current without it. + engineMu sync.Mutex + current atomic.Pointer[gusset.Handle] + closed atomic.Bool ) +// engine returns the shared handle, opening one if there is none. +// +// This used to be a sync.Once. A poisoned handle then stayed the only handle +// for the life of the process, and every later Match answered ErrPoisoned — +// the "close it and open a new handle" Gusset asks for had nowhere to happen. +// A failed Open is also retried by the next call instead of being latched. func engine() (*gusset.Handle, error) { - engineOnce.Do(func() { - register() - engineH, engineErr = gusset.Open(gusset.WithPoolSize(4)) - }) - if engineErr != nil { - return nil, engineErr + if h := current.Load(); h != nil { + return h, nil + } + engineMu.Lock() + defer engineMu.Unlock() + if closed.Load() { + return nil, ErrClosed + } + if h := current.Load(); h != nil { + return h, nil + } + registerOnce.Do(register) + h, err := gusset.Open(gusset.WithPoolSize(poolSize)) + if err != nil { + return nil, err + } + current.Store(h) + return h, nil +} + +// retire drops h after it reported poison, so the next call opens a fresh +// handle. Only the first caller to see a given handle's poison closes it. +// +// Close joins h's workers. dc-glob is bounded, so that is short, but it runs +// off the caller's goroutine anyway: the caller already has its answer and +// its deadline is not Close's business. +func retire(h *gusset.Handle) { + if current.CompareAndSwap(h, nil) { + go func() { _ = h.Close() }() + } +} + +// poisoned reports an error after which h refuses all further work. +func poisoned(err error) bool { + return errors.Is(err, gusset.ErrPanic) || errors.Is(err, gusset.ErrPoisoned) +} + +// Close shuts the shared handle and refuses every later call with ErrClosed. +// A long-running host calls it on shutdown, after gusset.Shutdown if it wants +// the drain bounded. +func Close() error { + engineMu.Lock() + defer engineMu.Unlock() + closed.Store(true) + h := current.Swap(nil) + if h == nil { + return nil + } + return h.Close() +} + +// DrainLogs writes whatever Gusset's Rust side logged (worker respawns, +// sigaltstack failures, caught panics) to w and reports how many bytes it +// wrote. Nothing reads that ring otherwise; it is bounded and evicts the +// oldest line, so a host that never drains it loses the evidence. +func DrainLogs(w io.Writer) (int, error) { + buf := make([]byte, 64<<10) + total := 0 + for { + n := gusset.DrainLogs(buf) + if n <= 0 { + return total, nil + } + if n > len(buf) { + n = len(buf) + } + m, err := w.Write(buf[:n]) + total += m + if err != nil { + return total, err + } + if n < len(buf) { + return total, nil + } } - if engineH == nil { - return nil, errors.New("gusset: engine handle is nil") +} + +// call runs one frame on the shared handle and retires it on poison. +func call(ctx context.Context, payload []byte) ([]byte, error) { + h, err := engine() + if err != nil { + return nil, err } - return engineH, nil + out, err := h.Call(ctx, payload) + if err != nil && poisoned(err) { + retire(h) + } + return out, err +} + +// decodeBool reads the engine's one-byte answer. +func decodeBool(out []byte) (bool, error) { + if len(out) != 1 || (out[0] != 0 && out[0] != 1) { + return false, fmt.Errorf("gusset: engine returned %q, want a single 0 or 1", out) + } + return out[0] == 1, nil } func encode(pattern, name string) ([]byte, error) { @@ -64,47 +179,95 @@ func encode(pattern, name string) ([]byte, error) { // // A Gusset or engine error is returned, never converted into a boolean. // The handle is not poisoned by a bad frame: the next well-formed call -// still runs. +// still runs. A handle that is poisoned anyway is retired, and the next call +// opens a new one. func Match(ctx context.Context, pattern, name string) (bool, error) { if ctx == nil { return false, errors.New("gusset: nil context") } - h, err := engine() - if err != nil { - return false, err - } payload, err := encode(pattern, name) if err != nil { return false, err } - out, err := h.Call(ctx, payload) + out, err := call(ctx, payload) if err != nil { return false, err } - if len(out) != 1 || (out[0] != 0 && out[0] != 1) { - return false, fmt.Errorf("gusset: engine returned %q, want a single 0 or 1", out) - } - return out[0] == 1, nil + return decodeBool(out) } // MatchAny reports whether name matches any pattern in patterns. -// If any Match call returns an error, matching stops and returns that error. +// +// The list crosses the boundary in as few frames as fit Gusset's 4 KiB +// inline copy, one call each, stopping at the first frame that matches. It +// used to be one call per pattern. Every pattern in a frame is validated by +// the engine before any is matched, so a malformed pattern is an error even +// when an earlier one would have matched. func MatchAny(ctx context.Context, patterns []string, name string) (bool, error) { + if ctx == nil { + return false, errors.New("gusset: nil context") + } + if len(name) > maxField { + return false, fmt.Errorf("gusset: name exceeds %d bytes", maxField) + } for _, p := range patterns { - matched, err := Match(ctx, p, name) + if len(p) > maxField { + return false, fmt.Errorf("gusset: pattern exceeds %d bytes", maxField) + } + } + if len(patterns) == 0 { + // Still a real answer, and still refused on a dead context. + if err := ctx.Err(); err != nil { + return false, err + } + return false, nil + } + actx := gusset.ContextWithOpcode(ctx, opcodeMatchAny) + for rest := patterns; len(rest) > 0; { + frame, used := encodeAny(name, rest) + out, err := call(actx, frame) if err != nil { return false, err } - if matched { - return true, nil + matched, err := decodeBool(out) + if err != nil || matched { + return matched, err } + rest = rest[used:] } return false, nil } -// Check runs the parity vectors the adoption measured, then a frame the -// diagnostic engine would treat as a panic. The second call must still match. +// encodeAny packs name and as many leading patterns as fit one frame, and +// reports how many it took. Fields are length-checked by the caller, so one +// pattern always fits: 2+1024+2+2+1024 is far below maxFrame. +func encodeAny(name string, patterns []string) ([]byte, int) { + size := 2 + len(name) + 2 + n := 0 + for n < len(patterns) && n < maxPatterns && size+2+len(patterns[n]) <= maxFrame { + size += 2 + len(patterns[n]) + n++ + } + buf := make([]byte, 0, size) + buf = binary.LittleEndian.AppendUint16(buf, uint16(len(name))) + buf = append(buf, name...) + buf = binary.LittleEndian.AppendUint16(buf, uint16(n)) + for _, p := range patterns[:n] { + buf = binary.LittleEndian.AppendUint16(buf, uint16(len(p))) + buf = append(buf, p...) + } + return buf, n +} + +// Check runs the parity vectors the adoption measured, the batched MatchAny +// path, and a frame the diagnostic engine would treat as a panic. It never +// panics anything: Manvi runs it on the first policy check of a live server, +// where a deliberate "panicked at" on stderr reads as a crash. SelfTest adds +// the panic proof for the gusset-check commands. func Check(ctx context.Context) error { + if ctx == nil { + return errors.New("gusset: nil context") + } vectors := []struct { pattern, name string want bool @@ -145,5 +308,55 @@ func Check(ctx context.Context) error { if !utf8.ValidString("\x01") { return errors.New("internal: 0x01 was expected to be valid UTF-8") } + + anyPatterns := []string{"*.rs", "*.go", "src/*.py"} + if got, err := MatchAny(ctx, anyPatterns, "src/foo.py"); err != nil || !got { + return fmt.Errorf("MatchAny(%q, src/foo.py) = %v, %v; want true", anyPatterns, got, err) + } + if got, err := MatchAny(ctx, anyPatterns, "src/foo.js"); err != nil || got { + return fmt.Errorf("MatchAny(%q, src/foo.js) = %v, %v; want false", anyPatterns, got, err) + } + + return nil +} + +// SelfTest is Check plus a real panic inside this archive on a throwaway +// handle (I2), after which the shared handle must still match. Rust's panic +// hook prints the induced panic to stderr; that line is the proof running, +// not a failure. For the gusset-check commands and tests, not for a server. +func SelfTest(ctx context.Context) error { + if err := Check(ctx); err != nil { + return err + } + if err := checkPanicFirewall(ctx); err != nil { + return err + } + got, err := Match(ctx, "*.py", "src/foo.py") + if err != nil || !got { + return fmt.Errorf("shared handle after the self-test panic: Match = %v, %v", got, err) + } + return nil +} + +// checkPanicFirewall panics inside the umbrella on a handle of its own and +// requires the panic to come back as ErrPanic, the handle to refuse the next +// call as ErrPoisoned, and the process to still be here. The diagnostic +// engine proves this for Gusset; this proves it for the archive this binary +// actually links, built with this crate's panic strategy. +func checkPanicFirewall(ctx context.Context) error { + if _, err := engine(); err != nil { // registers the umbrella's engines + return err + } + h, err := gusset.Open(gusset.WithPoolSize(1), gusset.WithOpcode(opcodeSelfTestPanic)) + if err != nil { + return fmt.Errorf("self-test handle: %w", err) + } + defer h.Close() + if _, err := h.Call(ctx, nil); !errors.Is(err, gusset.ErrPanic) { + return fmt.Errorf("self-test panic returned %v, want ErrPanic", err) + } + if _, err := h.Call(ctx, nil); !errors.Is(err, gusset.ErrPoisoned) { + return fmt.Errorf("call after a panic returned %v, want ErrPoisoned", err) + } return nil } diff --git a/rust/gusset-engine/Cargo.lock b/rust/gusset-engine/Cargo.lock index 9b893926..91ea4b45 100644 --- a/rust/gusset-engine/Cargo.lock +++ b/rust/gusset-engine/Cargo.lock @@ -16,7 +16,7 @@ dependencies = [ [[package]] name = "gusset" -version = "0.0.1" +version = "0.0.2" dependencies = [ "libc", "static_assertions", diff --git a/rust/gusset-engine/cgo-env.sh b/rust/gusset-engine/cgo-env.sh new file mode 100755 index 00000000..f34dd21d --- /dev/null +++ b/rust/gusset-engine/cgo-env.sh @@ -0,0 +1,57 @@ +#!/usr/bin/env bash +# Build the umbrella archive and print the cgo environment that links it. +# +# eval "$(rust/gusset-engine/cgo-env.sh --export)" # a shell +# rust/gusset-engine/cgo-env.sh >> "$GITHUB_ENV" # a CI job +# rust/gusset-engine/cgo-env.sh --no-build ... # archive already built +# +# Why CGO_CFLAGS carries the archive's hash: Go's build and test caches key a +# cgo package on the text of its flags, not on the bytes of the libraries it +# links. Rebuild libgusset.a and a plain `go build` relinks the old Rust from +# the cache, and `go test` can replay a cached "ok" measured against it. CI +# restores GOCACHE, so the same holds there. A -D naming the hash changes the +# flags whenever the archive changes, which is the only thing Go looks at. +# +# Always runs the incremental cargo build: building only when the archive is +# missing linked whatever an older checkout left behind. +set -euo pipefail + +here="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +mode=env +build=1 +for arg in "$@"; do + case "$arg" in + --export) mode=export ;; + --no-build) build=0 ;; + *) echo "cgo-env.sh: unknown argument $arg" >&2; exit 2 ;; + esac +done + +if [[ ! -f "$here/../../../gusset/crates/gusset/Cargo.toml" ]]; then + echo "cgo-env.sh: gusset is not checked out next to DevCouncil ($here/../../../gusset)" >&2 + exit 1 +fi +if [[ "$build" -eq 1 ]]; then + cargo build --release --locked --manifest-path "$here/Cargo.toml" >&2 +fi +archive="$here/target/release/libgusset.a" +if [[ ! -f "$archive" ]]; then + echo "cgo-env.sh: $archive is missing" >&2 + exit 1 +fi +if command -v sha256sum >/dev/null 2>&1; then + sum="$(sha256sum "$archive" | cut -c1-32)" +else + sum="$(shasum -a 256 "$archive" | cut -c1-32)" +fi + +ldflags="-L$here/target/release" +# Replace, never stack: runtime/cgo builds with -Werror, and a second -D of +# the same macro from re-sourcing this in one shell is a redefinition error. +base="$(printf '%s' "${CGO_CFLAGS:--O2 -g}" | sed -E 's/(^| )-DDEVCOUNCIL_GUSSET_ENGINE_SHA256=[^ ]*//g; s/^ +//')" +cflags="${base:+$base }-DDEVCOUNCIL_GUSSET_ENGINE_SHA256=$sum" +if [[ "$mode" == export ]]; then + printf 'export CGO_ENABLED=1\nexport CGO_LDFLAGS=%q\nexport CGO_CFLAGS=%q\n' "$ldflags" "$cflags" +else + printf 'CGO_ENABLED=1\nCGO_LDFLAGS=%s\nCGO_CFLAGS=%s\n' "$ldflags" "$cflags" +fi diff --git a/rust/gusset-engine/src/lib.rs b/rust/gusset-engine/src/lib.rs index b0e272f8..00f575ab 100644 --- a/rust/gusset-engine/src/lib.rs +++ b/rust/gusset-engine/src/lib.rs @@ -4,7 +4,17 @@ //! not the package name). It is the only Rust staticlib a Go binary in this //! suite may link. A second archive defines `rust_eh_personality` twice. -use gusset_core::{set_engine_handler, JobContext}; +use gusset_core::{register_engine, set_engine_handler, Counting, JobContext}; + +/// Counts every Rust allocation in the host process. +/// +/// Gusset declares no global allocator, so without this `gusset.Stats()` +/// reads zero and `AdviseMemoryLimit` subtracts nothing: the Go side of every +/// binary that links this archive was budgeting as if Rust used no memory. +/// The umbrella is the one archive per binary (R14), so this is the one place +/// it can be declared. +#[global_allocator] +static ALLOCATOR: Counting = Counting::new(std::alloc::System); /// Largest pattern or name accepted across the boundary. /// @@ -12,6 +22,26 @@ use gusset_core::{set_engine_handler, JobContext}; /// never memcpy's a path on the cgo thread. pub const MAX_FIELD: usize = 1024; +/// Opcode for [`match_any_frame`]: one crossing for a whole pattern list. +/// +/// Opcode 0 is the global handler ([`match_frame`]). Gusset resolves a +/// registered opcode before the global handler, so this cannot shadow it. +pub const OPCODE_MATCH_ANY: u32 = 1; + +/// Largest pattern count in one match-any frame. +/// +/// The 4 KiB inline limit already bounds a frame; this bounds the loop over +/// zero-length patterns, which cost two bytes each. +pub const MAX_PATTERNS: usize = 1024; + +/// Opcode that panics on purpose, so the host can prove the panic firewall +/// (I2) against this archive rather than against Gusset's diagnostic engine. +/// +/// Reachable only through the call header, which Go sets; no payload byte +/// selects it. `gussetfn.Check` sends it on a throwaway handle and requires +/// ErrPanic, then ErrPoisoned, then a clean match on the shared handle. +pub const OPCODE_SELF_TEST_PANIC: u32 = 0x7fff_0001; + /// Registers the dc-glob engine on Gusset's global handler. /// /// Call once, before the first submission. Registration is not a Gusset export; @@ -30,9 +60,70 @@ pub unsafe extern "C" fn devcouncil_gusset_init() -> i32 { match_frame(input) }, ); + register_engine( + OPCODE_MATCH_ANY, + |ctx: &JobContext, input: &[u8]| -> Result, String> { + ctx.check() + .map_err(|reason| format!("cancelled: {:?}", reason))?; + match_any_frame(input) + }, + ); + register_engine( + OPCODE_SELF_TEST_PANIC, + |_: &JobContext, _: &[u8]| -> Result, String> { + panic!("devcouncil gusset self-test panic"); + }, + ); 0 } +/// Reads one `u16le len | bytes` field at `at`, returning it and the next offset. +fn field<'a>(input: &'a [u8], at: usize, what: &str) -> Result<(&'a str, usize), String> { + let len_end = at + .checked_add(2) + .filter(|&end| end <= input.len()) + .ok_or_else(|| format!("truncated {what} length"))?; + let len = u16::from_le_bytes([input[at], input[at + 1]]) as usize; + if len > MAX_FIELD { + return Err(format!("{what} length {len} exceeds {MAX_FIELD} bytes")); + } + let end = len_end + .checked_add(len) + .filter(|&end| end <= input.len()) + .ok_or_else(|| format!("truncated {what}"))?; + let text = std::str::from_utf8(&input[len_end..end]) + .map_err(|err| format!("{what} is not valid UTF-8: {err}"))?; + Ok((text, end)) +} + +/// Decodes a match-any frame and returns a single byte, 1 when any pattern matches. +/// +/// Frame: `u16le name_len | name | u16le count | (u16le pattern_len | pattern){count}`, +/// nothing else. Every pattern is validated before any is matched, so a bad +/// field late in the list is an error rather than a result that depends on +/// whether an earlier pattern happened to match first. +pub fn match_any_frame(input: &[u8]) -> Result, String> { + let (name, mut at) = field(input, 0, "name")?; + if at + 2 > input.len() { + return Err("truncated pattern count".to_string()); + } + let count = u16::from_le_bytes([input[at], input[at + 1]]) as usize; + if count > MAX_PATTERNS { + return Err(format!("pattern count {count} exceeds {MAX_PATTERNS}")); + } + at += 2; + let mut patterns = Vec::with_capacity(count); + for _ in 0..count { + let (pattern, next) = field(input, at, "pattern")?; + patterns.push(pattern); + at = next; + } + if at != input.len() { + return Err("match-any frame length does not equal its headers".to_string()); + } + Ok(vec![u8::from(dc_glob::matches_any(&patterns, name))]) +} + /// Decodes a length-prefixed frame and returns a single byte, 1 for match. /// /// Frame: `u16le pattern_len | pattern | u16le name_len | name`, nothing else. @@ -80,6 +171,65 @@ mod tests { out } + fn any_frame(name: &[u8], patterns: &[&[u8]]) -> Vec { + let mut out = Vec::new(); + out.extend_from_slice(&(name.len() as u16).to_le_bytes()); + out.extend_from_slice(name); + out.extend_from_slice(&(patterns.len() as u16).to_le_bytes()); + for p in patterns { + out.extend_from_slice(&(p.len() as u16).to_le_bytes()); + out.extend_from_slice(p); + } + out + } + + #[test] + fn match_any_matches_one_of_the_list() { + let got = match_any_frame(&any_frame(b"src/foo.py", &[b"*.rs", b"*.py"])).unwrap(); + assert_eq!(got, vec![1]); + let got = match_any_frame(&any_frame(b"src/foo.js", &[b"*.rs", b"*.py"])).unwrap(); + assert_eq!(got, vec![0]); + } + + #[test] + fn match_any_of_nothing_is_no_match() { + assert_eq!(match_any_frame(&any_frame(b"a", &[])).unwrap(), vec![0]); + } + + #[test] + fn match_any_validates_every_pattern_before_matching() { + // The first pattern matches; the second is bad UTF-8. A short-circuit + // would answer 1 and hide the malformed field. + let err = match_any_frame(&any_frame(b"a.py", &[b"*.py", b"\xff"])).unwrap_err(); + assert!(err.contains("UTF-8"), "{err}"); + } + + #[test] + fn match_any_refuses_truncation_and_trailing_bytes() { + let framed = any_frame(b"a", &[b"a", b"b"]); + for cut in 0..framed.len() { + assert!(match_any_frame(&framed[..cut]).is_err(), "prefix of {cut} bytes"); + } + let mut long = framed.clone(); + long.push(0); + assert!(match_any_frame(&long).unwrap_err().contains("does not equal")); + } + + #[test] + fn match_any_refuses_a_count_past_the_cap() { + let mut framed = vec![1, 0, b'a']; + framed.extend_from_slice(&((MAX_PATTERNS + 1) as u16).to_le_bytes()); + assert!(match_any_frame(&framed).unwrap_err().contains("exceeds")); + } + + #[test] + fn no_prefix_of_a_match_frame_panics() { + let framed = frame(b"src/*.py", b"src/foo.py"); + for cut in 0..framed.len() { + assert!(match_frame(&framed[..cut]).is_err(), "prefix of {cut} bytes"); + } + } + #[test] fn star_crosses_a_separator() { let got = match_frame(&frame(b"*.py", b"src/foo.py")).unwrap(); @@ -113,7 +263,7 @@ mod tests { #[test] fn a_field_past_the_cap_is_refused_without_parsing_it() { let mut framed = vec![0x01, 0x04]; // 1025 - framed.extend(std::iter::repeat(b'a').take(4)); + framed.extend(std::iter::repeat_n(b'a', 4)); let err = match_frame(&framed).unwrap_err(); assert!(err.contains("exceeds"), "{err}"); } diff --git a/rust/verify.sh b/rust/verify.sh index ac1472ed..2be5ee08 100755 --- a/rust/verify.sh +++ b/rust/verify.sh @@ -69,15 +69,18 @@ else echo "Check out https://github.com/bharathvbcr/gusset there. CI does this in .github/actions/setup-gusset." >&2 exit 1 fi - if [[ ! -f "$archive" ]]; then - echo "building gusset-engine release staticlib (libgusset.a is not in git)" - cargo build --release --locked --manifest-path gusset-engine/Cargo.toml - fi - if [[ ! -f "$archive" ]]; then - echo "GATE FAIL: $archive is missing after cargo build --release --locked" >&2 + # Always the incremental build, never "only when missing": that linked + # whatever archive an older checkout left behind. cgo-env.sh also keys Go's + # caches on the archive's hash, without which a rebuilt archive is not + # relinked and a cached test result is replayed against the old one. + echo "building gusset-engine release staticlib (libgusset.a is not in git)" + eval "$(gusset-engine/cgo-env.sh --export)" || { + echo "GATE FAIL: gusset-engine/cgo-env.sh could not build $archive" >&2 exit 1 - fi + } go -C ../backend/go_orchestrator build -o bin/ ./cmd/devcouncil + go -C ../backend/go_orchestrator test -count=1 ./gussetfn + ../backend/go_orchestrator/bin/devcouncil gusset-check fi step "3/9 tests" diff --git a/scripts/ci-local.mjs b/scripts/ci-local.mjs index 5c44b051..b7a138ba 100644 --- a/scripts/ci-local.mjs +++ b/scripts/ci-local.mjs @@ -5,6 +5,7 @@ * as a pass. */ import { spawnSync } from "node:child_process"; +import { createHash } from "node:crypto"; import { existsSync, readFileSync } from "node:fs"; import path from "node:path"; import { fileURLToPath } from "node:url"; @@ -63,6 +64,14 @@ function ensureGussetEngine() { console.error(`FAIL: ${archive} is missing after cargo build`); process.exit(1); } + // Key Go's caches on the archive, as rust/gusset-engine/cgo-env.sh does: + // Go hashes cgo flags, not the libraries they link, so a rebuilt archive + // is otherwise not relinked and a cached "ok" is replayed against the old one. + const sum = createHash("sha256").update(readFileSync(archive)).digest("hex").slice(0, 32); + const base = (process.env.CGO_CFLAGS ?? "-O2 -g") + .replace(/(^| )-DDEVCOUNCIL_GUSSET_ENGINE_SHA256=\S*/g, "") + .trim(); + process.env.CGO_CFLAGS = `${base ? `${base} ` : ""}-DDEVCOUNCIL_GUSSET_ENGINE_SHA256=${sum}`; } function gofmtClean() { From 62331b32114dde276f65ac36f658dde7d422f6ae Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 28 Sep 2026 03:05:37 +0000 Subject: [PATCH 08/20] fix(gusset): close the gaps a second audit found in the engine integration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An adversarial review of 3197897 found these; each fix has a test that fails without it (reverted and run): - Match obeyed an opcode already on the caller's context: opcode 1 decoded a match frame as a match-any frame, and the self-test opcode panicked the shared handle. Match now pins opcode 0. (TestMatchIgnoresAnOpcodeOnTheContext) - A call landing on a handle a sibling had just poisoned or retired got ErrPoisoned or an unclassified "handle is closed" while a healthy handle was one call away (457 + 64 such errors in the review's probe). call() retries once on the replacement; a frame's own ErrPanic is never retried. After Close, the retry reaches engine() and answers ErrClosed, as Close documents. (TestPoisonUnderConcurrencyRecovers now requires every call to succeed and exactly one replacement per poisoning; TestZZCloseRacingCallsSeeErrClosed; TestOwnPanicIsNotRetried) - Close joins workers without a bound, so wiring it into a server's exit would hang on a stuck engine. Shutdown(drain) runs gusset.Shutdown first and returns its error without joining when the drain expires. (TestShutdownIsBoundedAndRefusesLaterCalls, in a child process) - MatchAny validated UTF-8 per frame, so a bad pattern in frame 2 went unreported when frame 1 matched. Every field is checked up front. (TestMatchAnyValidatesAcrossFrames) - TestStatsCountBuffersOnce passed with the global allocator removed: gusset counts buffers by hand regardless. TestCountingIsTheGlobalAllocator asserts AllocCount grows across Matches (0 -> 0 without Counting). - verify.sh: `eval "$(cgo-env.sh)" || fail` could never fail — eval of empty output succeeds — so a failed engine build linked a stale archive. The output is captured first. - cgo-env.sh and ci-local hashed target/release while an inherited CARGO_TARGET_DIR sent the fresh archive elsewhere; both now pass --target-dir. Base CFLAGS come from `go env` (process env, `go env -w`, or default) instead of the process env alone. A path with a space is refused by name. - check-release now requires rust/gusset-engine/Cargo.lock's dc-glob to match the product version; the next bump would otherwise break `cargo build --locked` of the umbrella exactly as 0.2.3-at-1.3.5 did. - Doc: the self-test opcode is sent by SelfTest, not Check. Race detector with GOGC=1 -count=5 clean; gusset-check ok. Protects I2 (panic firewall), I3 (bounded shutdown), I4 (bounded handles) and I6 (the archive under test is the one linked). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01FUNo5WZs78gWyCSXf2nRhm (cherry picked from commit 876ace2913e62233c24b0c32da333744f4b4c88b) --- .../go_orchestrator/gussetfn/boundary_test.go | 205 ++++++++++++++---- backend/go_orchestrator/gussetfn/match.go | 92 ++++++-- rust/gusset-engine/cgo-env.sh | 17 +- rust/gusset-engine/src/lib.rs | 6 +- rust/verify.sh | 5 +- scripts/check-release.mjs | 14 ++ scripts/check-release.test.mjs | 12 + scripts/ci-local.mjs | 9 +- 8 files changed, 301 insertions(+), 59 deletions(-) diff --git a/backend/go_orchestrator/gussetfn/boundary_test.go b/backend/go_orchestrator/gussetfn/boundary_test.go index 48d791bd..fd97297f 100644 --- a/backend/go_orchestrator/gussetfn/boundary_test.go +++ b/backend/go_orchestrator/gussetfn/boundary_test.go @@ -4,7 +4,9 @@ import ( "bytes" "context" "errors" + "fmt" "os" + "os/exec" "path/filepath" "runtime" "strings" @@ -165,25 +167,42 @@ func TestPoisonedHandleIsReplaced(t *testing.T) { t.Fatalf("self-test panic = %v", err) } prev := current.Swap(bad) - - if _, err := Match(ctx, "*.py", "a.py"); !errors.Is(err, gusset.ErrPoisoned) { - t.Fatalf("Match on the poisoned handle = %v, want ErrPoisoned", err) - } - if current.Load() == bad { - t.Fatal("the poisoned handle is still the shared handle") + if prev != nil { + defer prev.Close() } + + // The poison was a sibling's, so the call retires the handle and answers + // from the replacement instead of returning ErrPoisoned. got, err := Match(ctx, "*.py", "a.py") if err != nil || !got { - t.Fatalf("Match after replacement = %v, %v", got, err) + t.Fatalf("Match on the poisoned handle = %v, %v; want a retried match", got, err) } - if prev != nil { - _ = prev.Close() + if h := current.Load(); h == bad || h == nil { + t.Fatal("the poisoned handle was not replaced") + } +} + +// This frame's own panic is not retried: the retry would poison the +// replacement with the same input. +func TestOwnPanicIsNotRetried(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + before := opens.Load() + _, err := call(gusset.ContextWithOpcode(ctx, opcodeSelfTestPanic), nil) + if !errors.Is(err, gusset.ErrPanic) { + t.Fatalf("call = %v, want ErrPanic", err) + } + if n := opens.Load() - before; n > 1 { + t.Fatalf("%d handles opened for one panicking frame", n) + } + if got, err := Match(ctx, "*.py", "a.py"); err != nil || !got { + t.Fatalf("Match after a panicking frame = %v, %v", got, err) } } // Many goroutines hit a handle that becomes poisoned under them. Every call -// returns (no hang), every error is a Gusset error, and exactly one new -// handle serves afterwards. +// must still succeed — the poison was another call's, and call retries once +// on the replacement — and exactly one replacement is opened per poisoning. func TestPoisonUnderConcurrencyRecovers(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) defer cancel() @@ -199,21 +218,21 @@ func TestPoisonUnderConcurrencyRecovers(t *testing.T) { if prev := current.Swap(bad); prev != nil { _ = prev.Close() } + before := opens.Load() var wg sync.WaitGroup - errs := make(chan error, 64) + errs := make(chan error, 64*10) for g := 0; g < 64; g++ { wg.Add(1) go func() { defer wg.Done() for i := 0; i < 10; i++ { got, err := Match(ctx, "*.py", "a.py") - if err == nil && !got { - errs <- errors.New("match returned false") - return - } - if err != nil && ctx.Err() != nil { + if err != nil { errs <- err - return + continue + } + if !got { + errs <- errors.New("match returned false") } } }() @@ -221,14 +240,51 @@ func TestPoisonUnderConcurrencyRecovers(t *testing.T) { wg.Wait() close(errs) for err := range errs { - t.Error(err) + t.Errorf("round %d: %v", round, err) } - if got, err := Match(ctx, "*.py", "a.py"); err != nil || !got { - t.Fatalf("round %d: settled Match = %v, %v", round, got, err) + if n := opens.Load() - before; n != 1 { + t.Fatalf("round %d: %d handles opened to replace one poisoned handle", round, n) } } } +// An opcode already on the caller's context must not reach Match's frame: +// opcode 1 decoded it as a match-any frame, the self-test opcode panicked +// the shared handle. +func TestMatchIgnoresAnOpcodeOnTheContext(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + for _, op := range []uint32{opcodeMatchAny, opcodeSelfTestPanic} { + got, err := Match(gusset.ContextWithOpcode(ctx, op), "*.py", "a.py") + if err != nil || !got { + t.Fatalf("Match under opcode %#x = %v, %v", op, got, err) + } + } + if h := current.Load(); h == nil { + t.Fatal("no shared handle") + } + if got, err := Match(ctx, "*.py", "a.py"); err != nil || !got { + t.Fatalf("shared handle after opcode contexts: %v, %v", got, err) + } +} + +// A pattern in a later frame with bad UTF-8 is an error even when the first +// frame matches: validation is over the whole list, not per frame. +func TestMatchAnyValidatesAcrossFrames(t *testing.T) { + long := strings.Repeat("a", maxField) + list := []string{"*"} + for i := 0; i < 8; i++ { + list = append(list, long) + } + list = append(list, "\xff") + if _, n := encodeAny("x", list); n >= len(list) { + t.Fatal("the list fit one frame; the case is not exercised") + } + if _, err := MatchAny(context.Background(), list, "x"); err == nil || !strings.Contains(err.Error(), "UTF-8") { + t.Fatalf("got %v, want a UTF-8 error", err) + } +} + func TestCancellationDoesNotPoison(t *testing.T) { for i := 0; i < 200; i++ { ctx, cancel := context.WithTimeout(context.Background(), time.Duration(i%7)*time.Microsecond) @@ -245,12 +301,15 @@ func TestCancellationDoesNotPoison(t *testing.T) { } } -func TestDrainLogsWritesWhatRustLogged(t *testing.T) { +// DrainLogs terminates on an idle ring and reports the bytes it wrote. It +// does not prove anything was logged: gusset's ring receives no line for a +// caught panic (that goes to FfiStatus), so there is nothing reliable to +// drain here. +func TestDrainLogsTerminatesAndCounts(t *testing.T) { var buf bytes.Buffer if _, err := DrainLogs(&buf); err != nil { t.Fatal(err) } - // A second drain of an idle ring is empty; the call must not block. buf.Reset() n, err := DrainLogs(&buf) if err != nil || n != buf.Len() { @@ -311,27 +370,29 @@ func FuzzMatchParity(f *testing.F) { }) } -// Close is last: it shuts the process-wide engine. reopen restores it so a -// -count>1 run still has an engine. -func TestZZCloseRefusesLaterCalls(t *testing.T) { - defer func() { closed.Store(false) }() - if _, err := engine(); err != nil { +// The umbrella installs gusset's Counting allocator. Gusset counts its own +// buffers by hand whether or not Counting is installed, so buffer bytes prove +// nothing about the allocator; AllocCount across plain Matches does — each +// frame decode allocates a Vec that only a global Counting sees. +func TestCountingIsTheGlobalAllocator(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + if _, err := Match(ctx, "*", "a"); err != nil { t.Fatal(err) } - if err := Close(); err != nil { - t.Fatalf("Close: %v", err) - } - if _, err := Match(context.Background(), "*", "a"); !errors.Is(err, ErrClosed) { - t.Fatalf("Match after Close = %v, want ErrClosed", err) + before := gusset.Stats().AllocCount + for i := 0; i < 50; i++ { + if _, err := Match(ctx, "*.py", "src/foo.py"); err != nil { + t.Fatal(err) + } } - if err := Close(); err != nil { - t.Fatalf("second Close: %v", err) + if after := gusset.Stats().AllocCount; after < before+50 { + t.Fatalf("AllocCount %d -> %d over 50 Matches: Counting is not the global allocator", before, after) } } -// The umbrella installs gusset's Counting allocator. A Rust buffer is then -// counted once — by Gusset's own buffer accounting or by the global -// allocator, not both — and it is counted at all: before, Stats read zero. +// A Rust buffer is counted once, not by both gusset's buffer accounting and +// the global allocator. func TestStatsCountBuffersOnce(t *testing.T) { if _, err := engine(); err != nil { t.Fatal(err) @@ -353,7 +414,6 @@ func TestStatsCountBuffersOnce(t *testing.T) { } after := gusset.Stats().LiveBytes delta := int64(during) - int64(before) - // Other goroutines' Rust allocations are small next to 8 MiB. if delta < n || delta > n+n/2 { t.Fatalf("an %d-byte buffer moved LiveBytes by %d; want about %d once", n, delta, n) } @@ -361,3 +421,70 @@ func TestStatsCountBuffersOnce(t *testing.T) { t.Fatalf("LiveBytes %d after Free, %d before", after, before) } } + +// Calls racing Close return a result or ErrClosed, never an unclassified +// "handle is closed". Named ZZ and reset by defer because Close is +// process-wide; Go runs tests in source order, so this stays after the rest +// of this file (match_test.go runs later and relies on the reset). +func TestZZCloseRacingCallsSeeErrClosed(t *testing.T) { + defer func() { closed.Store(false) }() + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + if _, err := engine(); err != nil { + t.Fatal(err) + } + var wg sync.WaitGroup + errs := make(chan error, 32*50) + for g := 0; g < 32; g++ { + wg.Add(1) + go func() { + defer wg.Done() + for i := 0; i < 50; i++ { + if _, err := Match(ctx, "*.py", "a.py"); err != nil && !errors.Is(err, ErrClosed) { + errs <- err + } + } + }() + } + time.Sleep(2 * time.Millisecond) + if err := Close(); err != nil { + t.Errorf("Close: %v", err) + } + wg.Wait() + close(errs) + for err := range errs { + t.Errorf("call racing Close: %v", err) + } + if _, err := Match(context.Background(), "*", "a"); !errors.Is(err, ErrClosed) { + t.Fatalf("Match after Close = %v, want ErrClosed", err) + } + if err := Close(); err != nil { + t.Fatalf("second Close: %v", err) + } +} + +// Shutdown is process-wide and one-way, so it runs in a child process. +func TestShutdownIsBoundedAndRefusesLaterCalls(t *testing.T) { + if os.Getenv("GUSSETFN_SHUTDOWN_CHILD") == "1" { + ctx := context.Background() + if _, err := Match(ctx, "*", "a"); err != nil { + fmt.Println("pre-shutdown match:", err) + os.Exit(3) + } + if err := Shutdown(time.Second); err != nil { + fmt.Println("shutdown:", err) + os.Exit(4) + } + if _, err := Match(ctx, "*", "a"); !errors.Is(err, ErrClosed) { + fmt.Println("post-shutdown match:", err) + os.Exit(5) + } + os.Exit(0) + } + cmd := exec.Command(os.Args[0], "-test.run=^TestShutdownIsBoundedAndRefusesLaterCalls$") + cmd.Env = append(os.Environ(), "GUSSETFN_SHUTDOWN_CHILD=1") + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("child: %v\n%s", err, out) + } +} diff --git a/backend/go_orchestrator/gussetfn/match.go b/backend/go_orchestrator/gussetfn/match.go index 1fad9cc3..a432eb29 100644 --- a/backend/go_orchestrator/gussetfn/match.go +++ b/backend/go_orchestrator/gussetfn/match.go @@ -15,8 +15,10 @@ import ( "errors" "fmt" "io" + "strings" "sync" "sync/atomic" + "time" "unicode/utf8" "github.com/bharathvbcr/DevCouncil/backend/go_orchestrator/fnmatch" @@ -53,6 +55,9 @@ var ( engineMu sync.Mutex current atomic.Pointer[gusset.Handle] closed atomic.Bool + // opens counts handles opened, so tests can tell one replacement from a + // stampede of them. + opens atomic.Int64 ) // engine returns the shared handle, opening one if there is none. @@ -78,6 +83,7 @@ func engine() (*gusset.Handle, error) { if err != nil { return nil, err } + opens.Add(1) current.Store(h) return h, nil } @@ -100,8 +106,9 @@ func poisoned(err error) bool { } // Close shuts the shared handle and refuses every later call with ErrClosed. -// A long-running host calls it on shutdown, after gusset.Shutdown if it wants -// the drain bounded. +// +// It joins the handle's workers without a time limit, so a host that must +// not hang on a stuck engine calls Shutdown instead. func Close() error { engineMu.Lock() defer engineMu.Unlock() @@ -113,6 +120,24 @@ func Close() error { return h.Close() } +// Shutdown is Close with a bound: it runs gusset.Shutdown(drain) first, which +// refuses new work process-wide and cancels every job, then joins the handle. +// When the drain expires with work still running (an engine that ignores its +// JobContext) it returns that error without joining, because the join would +// wait for exactly that work: the process is exiting and the OS reclaims the +// threads. Process-wide and one-way, like gusset.Shutdown: for process exit +// only. +func Shutdown(drain time.Duration) error { + engineMu.Lock() + closed.Store(true) + engineMu.Unlock() + if err := gusset.Shutdown(drain); err != nil { + current.Store(nil) + return err + } + return Close() +} + // DrainLogs writes whatever Gusset's Rust side logged (worker respawns, // sigaltstack failures, caught panics) to w and reports how many bytes it // wrote. Nothing reads that ring otherwise; it is bounded and evicts the @@ -139,17 +164,45 @@ func DrainLogs(w io.Writer) (int, error) { } } +// handleGone reports an error that says h was closed under the call: +// retired after another call's panic, or shut by Close. Gusset has no +// sentinel for it; these are its two spellings. +func handleGone(err error) bool { + msg := err.Error() + return strings.Contains(msg, "gusset: handle is closed") || strings.Contains(msg, "gusset: handle closed") +} + // call runs one frame on the shared handle and retires it on poison. +// +// A call that lands on a handle another call just poisoned or retired +// retries once on the replacement: a match is idempotent, and the failure +// was a sibling's, not this frame's. Before, those callers got ErrPoisoned or +// an unclassified "handle is closed" while a healthy handle was one call +// away. ErrPanic is never retried — this frame is what panicked, and a retry +// would poison the replacement too. After Close the retry reaches engine(), +// so a call racing Close answers ErrClosed, as Close promises. func call(ctx context.Context, payload []byte) ([]byte, error) { - h, err := engine() - if err != nil { - return nil, err - } - out, err := h.Call(ctx, payload) - if err != nil && poisoned(err) { - retire(h) + for attempt := 0; ; attempt++ { + h, err := engine() + if err != nil { + return nil, err + } + out, err := h.Call(ctx, payload) + if err == nil { + return out, nil + } + if poisoned(err) { + retire(h) + } + sibling := errors.Is(err, gusset.ErrPoisoned) || handleGone(err) + if !sibling { + return nil, err + } + // After Close the retry reaches engine(), which answers ErrClosed. + if attempt > 0 || ctx.Err() != nil { + return nil, err + } } - return out, err } // decodeBool reads the engine's one-byte answer. @@ -189,7 +242,11 @@ func Match(ctx context.Context, pattern, name string) (bool, error) { if err != nil { return false, err } - out, err := call(ctx, payload) + // Opcode 0, whatever the caller's context carries. gusset reads the + // opcode from ctx, so a ctx that had passed through MatchAny's opcode — + // or the self-test's — decoded this frame as a match-any frame, or + // panicked the shared handle. + out, err := call(gusset.ContextWithOpcode(ctx, 0), payload) if err != nil { return false, err } @@ -200,9 +257,10 @@ func Match(ctx context.Context, pattern, name string) (bool, error) { // // The list crosses the boundary in as few frames as fit Gusset's 4 KiB // inline copy, one call each, stopping at the first frame that matches. It -// used to be one call per pattern. Every pattern in a frame is validated by -// the engine before any is matched, so a malformed pattern is an error even -// when an earlier one would have matched. +// used to be one call per pattern. Every field is length- and UTF-8-checked +// here before the first frame is sent, so a malformed pattern is an error +// even when an earlier one, in this frame or an earlier frame, would have +// matched; the engine re-validates each frame on its own side. func MatchAny(ctx context.Context, patterns []string, name string) (bool, error) { if ctx == nil { return false, errors.New("gusset: nil context") @@ -210,10 +268,16 @@ func MatchAny(ctx context.Context, patterns []string, name string) (bool, error) if len(name) > maxField { return false, fmt.Errorf("gusset: name exceeds %d bytes", maxField) } + if !utf8.ValidString(name) { + return false, errors.New("gusset: name is not valid UTF-8") + } for _, p := range patterns { if len(p) > maxField { return false, fmt.Errorf("gusset: pattern exceeds %d bytes", maxField) } + if !utf8.ValidString(p) { + return false, fmt.Errorf("gusset: pattern %q is not valid UTF-8", p) + } } if len(patterns) == 0 { // Still a real answer, and still refused on a dead context. diff --git a/rust/gusset-engine/cgo-env.sh b/rust/gusset-engine/cgo-env.sh index f34dd21d..90854c9c 100755 --- a/rust/gusset-engine/cgo-env.sh +++ b/rust/gusset-engine/cgo-env.sh @@ -31,8 +31,18 @@ if [[ ! -f "$here/../../../gusset/crates/gusset/Cargo.toml" ]]; then echo "cgo-env.sh: gusset is not checked out next to DevCouncil ($here/../../../gusset)" >&2 exit 1 fi +# Go splits CGO_LDFLAGS on spaces and gussetfn's #cgo -L uses ${SRCDIR}, which +# cgo refuses to expand to a path with spaces; fail here, by name. +if [[ "$here" == *" "* ]]; then + echo "cgo-env.sh: $here contains a space, which cgo cannot link from" >&2 + exit 1 +fi if [[ "$build" -eq 1 ]]; then - cargo build --release --locked --manifest-path "$here/Cargo.toml" >&2 + # --target-dir pins the output where gussetfn's #cgo -L and the hash below + # look. Without it an inherited CARGO_TARGET_DIR (two lanes on one + # checkout) sent the fresh archive elsewhere and this script keyed Go on + # the stale one it then linked. + cargo build --release --locked --manifest-path "$here/Cargo.toml" --target-dir "$here/target" >&2 fi archive="$here/target/release/libgusset.a" if [[ ! -f "$archive" ]]; then @@ -48,7 +58,10 @@ fi ldflags="-L$here/target/release" # Replace, never stack: runtime/cgo builds with -Werror, and a second -D of # the same macro from re-sourcing this in one shell is a redefinition error. -base="$(printf '%s' "${CGO_CFLAGS:--O2 -g}" | sed -E 's/(^| )-DDEVCOUNCIL_GUSSET_ENGINE_SHA256=[^ ]*//g; s/^ +//')" +# Seed from `go env`, which reports the process env, a `go env -w` setting or +# Go's default -O2 -g, in that order; the process env alone missed -w. +current="$(go env CGO_CFLAGS 2>/dev/null || printf '%s' "${CGO_CFLAGS:--O2 -g}")" +base="$(printf '%s' "$current" | sed -E 's/(^| )-DDEVCOUNCIL_GUSSET_ENGINE_SHA256=[^ ]*//g; s/^ +//')" cflags="${base:+$base }-DDEVCOUNCIL_GUSSET_ENGINE_SHA256=$sum" if [[ "$mode" == export ]]; then printf 'export CGO_ENABLED=1\nexport CGO_LDFLAGS=%q\nexport CGO_CFLAGS=%q\n' "$ldflags" "$cflags" diff --git a/rust/gusset-engine/src/lib.rs b/rust/gusset-engine/src/lib.rs index 00f575ab..ad7fbf9a 100644 --- a/rust/gusset-engine/src/lib.rs +++ b/rust/gusset-engine/src/lib.rs @@ -38,8 +38,10 @@ pub const MAX_PATTERNS: usize = 1024; /// (I2) against this archive rather than against Gusset's diagnostic engine. /// /// Reachable only through the call header, which Go sets; no payload byte -/// selects it. `gussetfn.Check` sends it on a throwaway handle and requires -/// ErrPanic, then ErrPoisoned, then a clean match on the shared handle. +/// selects it. `gussetfn.SelfTest` sends it on a throwaway handle and requires +/// ErrPanic, then ErrPoisoned, then a clean match on the shared handle; +/// `gussetfn.Match` pins opcode 0 so a context carrying this one cannot +/// reach the shared handle. `Check` never sends it. pub const OPCODE_SELF_TEST_PANIC: u32 = 0x7fff_0001; /// Registers the dc-glob engine on Gusset's global handler. diff --git a/rust/verify.sh b/rust/verify.sh index 2be5ee08..89acd086 100755 --- a/rust/verify.sh +++ b/rust/verify.sh @@ -74,10 +74,13 @@ else # caches on the archive's hash, without which a rebuilt archive is not # relinked and a cached test result is replayed against the old one. echo "building gusset-engine release staticlib (libgusset.a is not in git)" - eval "$(gusset-engine/cgo-env.sh --export)" || { + # Captured before eval: `eval "$(failing)" || …` evaluates the empty output, + # succeeds, and the build then links whatever archive is lying around. + engine_exports="$(gusset-engine/cgo-env.sh --export)" || { echo "GATE FAIL: gusset-engine/cgo-env.sh could not build $archive" >&2 exit 1 } + eval "$engine_exports" go -C ../backend/go_orchestrator build -o bin/ ./cmd/devcouncil go -C ../backend/go_orchestrator test -count=1 ./gussetfn ../backend/go_orchestrator/bin/devcouncil gusset-check diff --git a/scripts/check-release.mjs b/scripts/check-release.mjs index bc4f362a..da58ba2c 100644 --- a/scripts/check-release.mjs +++ b/scripts/check-release.mjs @@ -367,6 +367,11 @@ export function defaultSources(root) { packageLockPath: path.join(root, "package-lock.json"), cargoTomlPath: path.join(root, "rust", "Cargo.toml"), cargoLockPath: path.join(root, "rust", "Cargo.lock"), + // The Gusset umbrella is its own workspace with its own lock, and dc-glob + // inherits the product version, so a version bump that skips this lock + // breaks every `cargo build --locked` of the umbrella (setup-gusset, + // verify.sh, cgo-env.sh) — which is how 0.2.3 sat in it at 1.3.5. + umbrellaLockPath: path.join(root, "rust", "gusset-engine", "Cargo.lock"), // `devcouncil/version`, not `cmd/devcouncil`: the constant moved into a // library so `devcouncil/mcp` could import it, and this gate must read // the owner rather than the alias that `package main` keeps for printing. @@ -399,6 +404,12 @@ export function inspectRelease(sources, opts = {}) { const cargoToml = read(sources.cargoTomlPath, "rust/Cargo.toml"); const cargoLock = read(sources.cargoLockPath, "rust/Cargo.lock"); const goSrc = read(sources.goVersionPath, "version.go"); + // Optional so fixtures without an umbrella still inspect; the umbrella's + // own --locked build refuses a missing lock. + const umbrellaLock = + sources.umbrellaLockPath && existsSync(sources.umbrellaLockPath) + ? readFileSync(sources.umbrellaLockPath, "utf8") + : null; const versions = { "package.json": pkg ? parseNpmVersion(pkg) : null, @@ -411,6 +422,9 @@ export function inspectRelease(sources, opts = {}) { : null, "version.go": goSrc ? parseGoVersion(goSrc) : null, }; + if (umbrellaLock != null) { + versions["rust/gusset-engine/Cargo.lock dc-glob"] = parseCargoLockVersion(umbrellaLock, "dc-glob"); + } const present = Object.values(versions).filter((v) => typeof v === "string" && v !== ""); const identity = present[0] ?? null; diff --git a/scripts/check-release.test.mjs b/scripts/check-release.test.mjs index 6d3930c0..89297e5b 100644 --- a/scripts/check-release.test.mjs +++ b/scripts/check-release.test.mjs @@ -241,6 +241,18 @@ describe("inspectRelease", () => { assert.ok(result.errors.some((e) => e.includes("0.4.2") && e.includes("0.2.0"))); }); + it("fails when the Gusset umbrella's lock names another dc-glob version", () => { + const root = scratchTree("umbrella"); + const dir = path.join(root, "rust", "gusset-engine"); + mkdirSync(dir, { recursive: true }); + const lock = (v) => `version = 4\n\n[[package]]\nname = "dc-glob"\nversion = "${v}"\n`; + writeFileSync(path.join(dir, "Cargo.lock"), lock("0.2.0")); + assert.deepEqual(inspectRelease(defaultSources(root)).errors, []); + writeFileSync(path.join(dir, "Cargo.lock"), lock("0.1.9")); + const result = inspectRelease(defaultSources(root)); + assert.ok(result.errors.some((e) => e.includes("gusset-engine/Cargo.lock") && e.includes("0.1.9"))); + }); + it("fails when the notes file for this version is missing", () => { const root = scratchTree("notes", { notes: false }); const result = inspectRelease(defaultSources(root)); diff --git a/scripts/ci-local.mjs b/scripts/ci-local.mjs index b7a138ba..617bf4a3 100644 --- a/scripts/ci-local.mjs +++ b/scripts/ci-local.mjs @@ -58,6 +58,10 @@ function ensureGussetEngine() { "--locked", "--manifest-path", "rust/gusset-engine/Cargo.toml", + // Where gussetfn's #cgo -L and the hash below look, whatever + // CARGO_TARGET_DIR says. + "--target-dir", + "rust/gusset-engine/target", ]); const archive = path.join(REPO_ROOT, "rust", "gusset-engine", "target", "release", "libgusset.a"); if (!existsSync(archive)) { @@ -68,7 +72,10 @@ function ensureGussetEngine() { // Go hashes cgo flags, not the libraries they link, so a rebuilt archive // is otherwise not relinked and a cached "ok" is replayed against the old one. const sum = createHash("sha256").update(readFileSync(archive)).digest("hex").slice(0, 32); - const base = (process.env.CGO_CFLAGS ?? "-O2 -g") + // `go env` reports the process env, a `go env -w` setting, or Go's default. + const goEnv = spawnSync("go", ["env", "CGO_CFLAGS"], { encoding: "utf8", shell: false }); + const current = goEnv.status === 0 ? goEnv.stdout.trim() : (process.env.CGO_CFLAGS || "-O2 -g"); + const base = current .replace(/(^| )-DDEVCOUNCIL_GUSSET_ENGINE_SHA256=\S*/g, "") .trim(); process.env.CGO_CFLAGS = `${base ? `${base} ` : ""}-DDEVCOUNCIL_GUSSET_ENGINE_SHA256=${sum}`; From e88cdb3607b44393a5d8c3a12549392feb908f4f Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 28 Sep 2026 03:27:41 +0000 Subject: [PATCH 09/20] fix(gussetfn): drain gusset's log ring until it returns 0 gussetfn.DrainLogs stopped on a short read, treating it as "ring empty". gusset's second-pass audit (80c6468) makes gusset_drain_logs hand over whole lines and never split a UTF-8 character, so a short read can leave lines behind; only 0 means empty. Looping until 0 is correct under both the pinned gusset and the new one. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01FUNo5WZs78gWyCSXf2nRhm (cherry picked from commit f8866c74fb6dc296e14263a9b6262c85cc494f3f) --- backend/go_orchestrator/gussetfn/match.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/backend/go_orchestrator/gussetfn/match.go b/backend/go_orchestrator/gussetfn/match.go index a432eb29..d2115155 100644 --- a/backend/go_orchestrator/gussetfn/match.go +++ b/backend/go_orchestrator/gussetfn/match.go @@ -158,9 +158,9 @@ func DrainLogs(w io.Writer) (int, error) { if err != nil { return total, err } - if n < len(buf) { - return total, nil - } + // Not "a short read means empty": gusset drains whole lines and never + // splits a UTF-8 character, so a short read can leave more behind. + // Only 0 means the ring is empty. } } From 0b389c409818ef6f95f6c420225a995f108fff0d Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 28 Sep 2026 03:31:55 +0000 Subject: [PATCH 10/20] ci(gusset): pin the fixed gusset 709c2b7 b6f5200 -> 709c2b7: gusset's second audit fixed a use-after-free where a waiter returned freed Rust memory as its result, a double-Wait park, drain exit admitting unreadable work, a queue lost-wakeup, phantom ring overflow, failure codes lost with a NULL status, unbounded engine error text and split log drains, and added a chaos hammer. Verified against it before pinning: umbrella cargo test, gussetfn race suite, gusset-check. Protects I1, I2 and I4. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01FUNo5WZs78gWyCSXf2nRhm (cherry picked from commit 663b998d7265ca6a14ff72492ed17064df19fe05) --- .github/actions/setup-gusset/action.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/actions/setup-gusset/action.yml b/.github/actions/setup-gusset/action.yml index 7f5ad2ff..d06743f8 100644 --- a/.github/actions/setup-gusset/action.yml +++ b/.github/actions/setup-gusset/action.yml @@ -25,7 +25,7 @@ runs: # Pinned to the sibling revision this tree was linked against. # Bump deliberately; tracking main would let a Gusset push fail a # DevCouncil release without a DevCouncil commit. - ref: b6f5200bfff4730e4e5268bead7e743d684cb67e + ref: 709c2b7b85be0de7c263471d933f395bb3b1b27f path: third_party/gusset persist-credentials: false From d5ef7eb033c9327ed6b7f5269c8d1546b15254ce Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 28 Sep 2026 04:06:25 +0000 Subject: [PATCH 11/20] fix(ci): let the gusset gates run, and build the host on Windows A third audit found the gusset gates in DevCouncil CI could not run: - analysis-plane's clients job stopped at its gofmt step on policy/zfix_split_test.go (already on main), so Set up Gusset, vet, go test (which includes gussetfn) and -race never ran. gofmt applied. - A pin-only bump triggered nothing: neither workflow's path filter named .github/actions/setup-gusset/**, and rust.yml also omitted rust/gusset-engine/** and gussetfn. Added. - The Windows components leg could never build: gusset refuses non-unix targets with compile_error!, yet the action built it for x86_64-pc-windows-gnu through MinGW, and gussetfn plus gusset-check imported gusset unconditionally, so the host (which dc-verify's json contract needs) could not build there either. gussetfn and gusset-check are now build-tagged unix; elsewhere gussetfn answers ErrUnsupported and `devcouncil gusset-check` exits 2 ("could not run", never a pass). The action still checks out and links the sibling on Windows (go.mod's replace must resolve) and skips the Rust toolchain and archive build; the MinGW and gnu-target steps are gone. Verified: GOOS=windows CGO_ENABLED=0 go build and go vet of the host and gussetfn succeed. - TestStatsCountBuffersOnce had no downward slack on a process-wide counter; a concurrent free from an earlier test's cleanup could fail it. It allows n-n/16; double-counting still fails near 2n. actionlint clean; check-workflows tests pass; gussetfn -race and policy tests pass; gusset-check ok on linux. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01FUNo5WZs78gWyCSXf2nRhm (cherry picked from commit f0ec3e24284fee9db63a9f5fdae8f0631b07f5fd) --- .github/actions/setup-gusset/action.yml | 39 ++++--------------- .github/workflows/analysis-plane.yml | 2 + .github/workflows/rust.yml | 6 +++ .../cmd/devcouncil/gusset_check.go | 2 + .../cmd/devcouncil/gusset_check_other.go | 15 +++++++ .../go_orchestrator/gussetfn/boundary_test.go | 6 ++- backend/go_orchestrator/gussetfn/engine.go | 2 + backend/go_orchestrator/gussetfn/errors.go | 12 ++++++ backend/go_orchestrator/gussetfn/match.go | 5 +-- .../go_orchestrator/gussetfn/match_test.go | 2 + .../go_orchestrator/gussetfn/unsupported.go | 32 +++++++++++++++ 11 files changed, 87 insertions(+), 36 deletions(-) create mode 100644 backend/go_orchestrator/cmd/devcouncil/gusset_check_other.go create mode 100644 backend/go_orchestrator/gussetfn/errors.go create mode 100644 backend/go_orchestrator/gussetfn/unsupported.go diff --git a/.github/actions/setup-gusset/action.yml b/.github/actions/setup-gusset/action.yml index d06743f8..960329f8 100644 --- a/.github/actions/setup-gusset/action.yml +++ b/.github/actions/setup-gusset/action.yml @@ -60,47 +60,22 @@ runs: test -f "$dest/go.mod" test -f "$dest/crates/gusset/Cargo.toml" - # Gusset's cgo line is -lgusset -lpthread -lm -ldl. That is a GNU link. - # windows-latest's Rust host is MSVC, which emits gusset.lib and has no - # libdl. The gnu target plus MinGW produces libgusset.a, which is what - # the #cgo -L path in gussetfn/engine.go already names. - - name: Install MinGW - if: ${{ runner.os == 'Windows' && inputs.build-engine == 'true' }} - uses: msys2/setup-msys2@v2 - with: - msystem: MINGW64 - update: false - install: mingw-w64-x86_64-gcc mingw-w64-x86_64-dlfcn - path-type: inherit - + # gusset is unix-only: its crate refuses other targets with compile_error!, + # so no Windows toolchain can build the archive. On Windows the sibling is + # still checked out (go.mod's replace must resolve) and the host builds + # without it — gussetfn is build-tagged unix and answers ErrUnsupported + # elsewhere. The MinGW/gnu-target path this replaced could never succeed. - name: Rust toolchain if: ${{ runner.os != 'Windows' && inputs.build-engine == 'true' }} uses: dtolnay/rust-toolchain@stable - - name: Rust toolchain (Windows gnu target) - if: ${{ runner.os == 'Windows' && inputs.build-engine == 'true' }} - uses: dtolnay/rust-toolchain@stable - with: - targets: x86_64-pc-windows-gnu - - name: Build the umbrella staticlib - if: ${{ inputs.build-engine == 'true' }} + if: ${{ runner.os != 'Windows' && inputs.build-engine == 'true' }} shell: bash working-directory: rust/gusset-engine run: | set -euo pipefail - if [[ "${RUNNER_OS}" == "Windows" ]]; then - export CARGO_TARGET_X86_64_PC_WINDOWS_GNU_LINKER=gcc - cargo build --release --locked --target x86_64-pc-windows-gnu - mkdir -p target/release - cp -f target/x86_64-pc-windows-gnu/release/libgusset.a target/release/libgusset.a - # Later `go build` steps in this job must use the same gcc. cgo - # will not use the MSVC linker for Gusset's -lpthread/-ldl line. - echo "CC=gcc" >> "$GITHUB_ENV" - echo "CARGO_TARGET_X86_64_PC_WINDOWS_GNU_LINKER=gcc" >> "$GITHUB_ENV" - else - cargo build --release --locked - fi + cargo build --release --locked test -f target/release/libgusset.a # Key Go's build and test caches on the archive (see cgo-env.sh): # setup-go restores GOCACHE, and without this a cached link or a diff --git a/.github/workflows/analysis-plane.yml b/.github/workflows/analysis-plane.yml index 7d1f383c..a4af9158 100644 --- a/.github/workflows/analysis-plane.yml +++ b/.github/workflows/analysis-plane.yml @@ -24,11 +24,13 @@ on: - "rust/**" - "backend/go_orchestrator/**" - ".github/workflows/analysis-plane.yml" + - ".github/actions/setup-gusset/**" pull_request: paths: - "rust/**" - "backend/go_orchestrator/**" - ".github/workflows/analysis-plane.yml" + - ".github/actions/setup-gusset/**" workflow_dispatch: permissions: diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 13edd060..daf98630 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -20,6 +20,9 @@ on: - "rust/Cargo.lock" - "rust/.gitignore" - ".github/workflows/rust.yml" + - ".github/actions/setup-gusset/**" + - "rust/gusset-engine/**" + - "backend/go_orchestrator/gussetfn/**" pull_request: paths: - "rust/devmap-*/**" @@ -31,6 +34,9 @@ on: - "rust/Cargo.lock" - "rust/.gitignore" - ".github/workflows/rust.yml" + - ".github/actions/setup-gusset/**" + - "rust/gusset-engine/**" + - "backend/go_orchestrator/gussetfn/**" workflow_dispatch: permissions: diff --git a/backend/go_orchestrator/cmd/devcouncil/gusset_check.go b/backend/go_orchestrator/cmd/devcouncil/gusset_check.go index a4b2e1f5..79b79b9e 100644 --- a/backend/go_orchestrator/cmd/devcouncil/gusset_check.go +++ b/backend/go_orchestrator/cmd/devcouncil/gusset_check.go @@ -1,3 +1,5 @@ +//go:build unix + package main import ( diff --git a/backend/go_orchestrator/cmd/devcouncil/gusset_check_other.go b/backend/go_orchestrator/cmd/devcouncil/gusset_check_other.go new file mode 100644 index 00000000..22a7b5aa --- /dev/null +++ b/backend/go_orchestrator/cmd/devcouncil/gusset_check_other.go @@ -0,0 +1,15 @@ +//go:build !unix + +package main + +import ( + "github.com/bharathvbcr/DevCouncil/backend/go_orchestrator/console" + "github.com/bharathvbcr/DevCouncil/backend/go_orchestrator/gussetfn" +) + +// runGussetCheck says the check could not run: gusset is unix-only. A check +// that could not run exits non-zero, never as a pass. +func runGussetCheck() int { + console.Errorf("gusset-check: %v\n", gussetfn.ErrUnsupported) + return 2 +} diff --git a/backend/go_orchestrator/gussetfn/boundary_test.go b/backend/go_orchestrator/gussetfn/boundary_test.go index fd97297f..507fd25f 100644 --- a/backend/go_orchestrator/gussetfn/boundary_test.go +++ b/backend/go_orchestrator/gussetfn/boundary_test.go @@ -1,3 +1,5 @@ +//go:build unix + package gussetfn import ( @@ -414,7 +416,9 @@ func TestStatsCountBuffersOnce(t *testing.T) { } after := gusset.Stats().LiveBytes delta := int64(during) - int64(before) - if delta < n || delta > n+n/2 { + // Process-wide, so a concurrent free from an earlier test's cleanup can + // pull it slightly under n; double-counting would put it near 2n. + if delta < n-n/16 || delta > n+n/2 { t.Fatalf("an %d-byte buffer moved LiveBytes by %d; want about %d once", n, delta, n) } if int64(after)-int64(before) > n/8 { diff --git a/backend/go_orchestrator/gussetfn/engine.go b/backend/go_orchestrator/gussetfn/engine.go index a81ed978..42545981 100644 --- a/backend/go_orchestrator/gussetfn/engine.go +++ b/backend/go_orchestrator/gussetfn/engine.go @@ -1,3 +1,5 @@ +//go:build unix + package gussetfn /* diff --git a/backend/go_orchestrator/gussetfn/errors.go b/backend/go_orchestrator/gussetfn/errors.go new file mode 100644 index 00000000..3df79df5 --- /dev/null +++ b/backend/go_orchestrator/gussetfn/errors.go @@ -0,0 +1,12 @@ +package gussetfn + +import "errors" + +// ErrClosed is returned after Close: the engine is shut for this process. +var ErrClosed = errors.New("gusset: engine is closed") + +// ErrUnsupported is every answer on a platform gusset does not support. +// gusset is unix-only (its Rust crate refuses other targets with +// compile_error!), so a Windows build has no engine to link; it still has to +// build, because the host is built there for dc-verify's json contract. +var ErrUnsupported = errors.New("gusset: the engine is unix-only and is not built on this platform") diff --git a/backend/go_orchestrator/gussetfn/match.go b/backend/go_orchestrator/gussetfn/match.go index d2115155..49ecb0d9 100644 --- a/backend/go_orchestrator/gussetfn/match.go +++ b/backend/go_orchestrator/gussetfn/match.go @@ -1,3 +1,5 @@ +//go:build unix + // Package gussetfn matches paths through dc-glob on the Gusset runtime. // // The write gate does not call this. fnmatch.Match returns a bool, and a @@ -46,9 +48,6 @@ const maxPatterns = 1024 // poolSize is the shared handle's worker count. const poolSize = 4 -// ErrClosed is returned after Close: the engine is shut for this process. -var ErrClosed = errors.New("gusset: engine is closed") - var ( registerOnce sync.Once // engineMu serialises opening; the hot path reads current without it. diff --git a/backend/go_orchestrator/gussetfn/match_test.go b/backend/go_orchestrator/gussetfn/match_test.go index 99eb3082..922b9449 100644 --- a/backend/go_orchestrator/gussetfn/match_test.go +++ b/backend/go_orchestrator/gussetfn/match_test.go @@ -1,3 +1,5 @@ +//go:build unix + package gussetfn import ( diff --git a/backend/go_orchestrator/gussetfn/unsupported.go b/backend/go_orchestrator/gussetfn/unsupported.go new file mode 100644 index 00000000..53b81680 --- /dev/null +++ b/backend/go_orchestrator/gussetfn/unsupported.go @@ -0,0 +1,32 @@ +//go:build !unix + +package gussetfn + +import ( + "context" + "io" + "time" +) + +// Match reports ErrUnsupported: there is no engine on this platform. +func Match(ctx context.Context, pattern, name string) (bool, error) { return false, ErrUnsupported } + +// MatchAny reports ErrUnsupported: there is no engine on this platform. +func MatchAny(ctx context.Context, patterns []string, name string) (bool, error) { + return false, ErrUnsupported +} + +// Check reports ErrUnsupported: there is no engine on this platform. +func Check(ctx context.Context) error { return ErrUnsupported } + +// SelfTest reports ErrUnsupported: there is no engine on this platform. +func SelfTest(ctx context.Context) error { return ErrUnsupported } + +// Close has nothing to release on this platform. +func Close() error { return nil } + +// Shutdown has nothing to release on this platform. +func Shutdown(time.Duration) error { return nil } + +// DrainLogs has nothing to drain on this platform. +func DrainLogs(io.Writer) (int, error) { return 0, nil } From c3c5f4d2973c3827b08edbb8202c8cca6b9a295f Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 28 Sep 2026 22:27:52 +0000 Subject: [PATCH 12/20] fix(serve): time the reply write, not the queue for the writer (port of b6e9673) serve_stress's ten_thousand_pipelined_stdio_requests_stay_inside_the_ceiling failed on this PR with "MCP response write exceeded 5s": the 5 s RESPONSE_WRITE_TIMEOUT covered the wait for the writer mutex as well as the write, so on a loaded runner a reply queued behind other in-flight replies to a peer that was reading fine ran out of time. The bound now covers write+flush once the writer is held. This is the devmap-serve half of b6e9673 on claude/zealous-faraday-56lcrv, unchanged; its find-msvc-tools lockfile half is already here as a2c364e. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01WQJD75wFPY52ckToQTu9FS --- rust/devmap-serve/src/mcp.rs | 170 ++++++++++++++++++++++++++++++++--- 1 file changed, 156 insertions(+), 14 deletions(-) diff --git a/rust/devmap-serve/src/mcp.rs b/rust/devmap-serve/src/mcp.rs index 2a19601c..8a73b060 100644 --- a/rust/devmap-serve/src/mcp.rs +++ b/rust/devmap-serve/src/mcp.rs @@ -620,8 +620,18 @@ pub const MAX_FRAME_BYTES: usize = 1024 * 1024; /// only kill. const CALL_TIMEOUT: Duration = Duration::from_secs(30); -/// A response must acquire its writer and reach the peer within this bound. -/// A stopped reader must not retain every admission permit indefinitely. +/// A response's write and flush must reach the peer within this bound once it +/// holds the writer. A stopped reader must not retain every admission permit +/// indefinitely: the write that meets it fails here and marks the stream +/// failed, and every response queued behind it then fails at once. +/// +/// It does not cover the wait for the writer. It used to, and on a loaded +/// machine a reply queued behind the other in-flight replies to a peer that +/// was reading perfectly well ran out of its 5s before it was written, which +/// ended the whole session (serve_stress's ten-thousand-request flood on +/// macOS). Queueing behind a peer that is making progress is backpressure, +/// already bounded by the admission ceiling; only a peer that has stopped +/// reading is a failure. const RESPONSE_WRITE_TIMEOUT: Duration = Duration::from_secs(5); /// Requests one connection may have outstanding at once. @@ -3526,6 +3536,17 @@ async fn write_frame( writer: &tokio::sync::Mutex>, frame: &Value, ) -> anyhow::Result<()> +where + W: AsyncWrite + Unpin, +{ + write_frame_within(writer, frame, RESPONSE_WRITE_TIMEOUT).await +} + +async fn write_frame_within( + writer: &tokio::sync::Mutex>, + frame: &Value, + bound: Duration, +) -> anyhow::Result<()> where W: AsyncWrite + Unpin, { @@ -3533,27 +3554,148 @@ where payload.push(b'\n'); // The lock spans the write and the flush, so two concurrent responses cannot // interleave their bytes into one unparseable line. - tokio::time::timeout(RESPONSE_WRITE_TIMEOUT, async { - let mut writer = writer.lock().await; - anyhow::ensure!( - !writer.failed, - "MCP response stream failed on an earlier frame" - ); - // Leave this set if writing fails or this future is cancelled. No - // concurrent handler may append another reply to a partial JSON frame. - writer.failed = true; + let mut writer = writer.lock().await; + anyhow::ensure!( + !writer.failed, + "MCP response stream failed on an earlier frame" + ); + // Leave this set if writing fails or times out. No concurrent handler may + // append another reply to a partial JSON frame. + writer.failed = true; + tokio::time::timeout(bound, async { writer.stream.write_all(&payload).await?; // Buffered output can deadlock a request/response protocol, so flushing // shares the write deadline and mutex rather than extending either. writer.stream.flush().await?; - writer.failed = false; - Ok::<(), anyhow::Error>(()) + Ok::<(), std::io::Error>(()) }) .await - .map_err(|_| anyhow::anyhow!("MCP response write exceeded {RESPONSE_WRITE_TIMEOUT:?}"))??; + .map_err(|_| anyhow::anyhow!("MCP response write exceeded {bound:?}"))??; + writer.failed = false; Ok(()) } +#[cfg(test)] +mod response_write_bounds { + use super::{write_frame_within, ResponseWriter}; + use serde_json::json; + use std::pin::Pin; + use std::sync::Arc; + use std::task::{Context, Poll}; + use std::time::{Duration, Instant}; + use tokio::io::AsyncWrite; + + /// A peer that reads every byte, a little slowly: each write takes `delay`. + struct SlowSink { + delay: Duration, + sleep: Option>>, + } + + impl AsyncWrite for SlowSink { + fn poll_write( + mut self: Pin<&mut Self>, + cx: &mut Context<'_>, + buf: &[u8], + ) -> Poll> { + let delay = self.delay; + let sleep = self + .sleep + .get_or_insert_with(|| Box::pin(tokio::time::sleep(delay))); + match sleep.as_mut().poll(cx) { + Poll::Ready(()) => { + self.sleep = None; + Poll::Ready(Ok(buf.len())) + } + Poll::Pending => Poll::Pending, + } + } + fn poll_flush(self: Pin<&mut Self>, _: &mut Context<'_>) -> Poll> { + Poll::Ready(Ok(())) + } + fn poll_shutdown(self: Pin<&mut Self>, _: &mut Context<'_>) -> Poll> { + Poll::Ready(Ok(())) + } + } + + /// A peer that has stopped reading. + struct StoppedSink; + + impl AsyncWrite for StoppedSink { + fn poll_write( + self: Pin<&mut Self>, + _: &mut Context<'_>, + _: &[u8], + ) -> Poll> { + Poll::Pending + } + fn poll_flush(self: Pin<&mut Self>, _: &mut Context<'_>) -> Poll> { + Poll::Pending + } + fn poll_shutdown(self: Pin<&mut Self>, _: &mut Context<'_>) -> Poll> { + Poll::Ready(Ok(())) + } + } + + use std::future::Future; + + /// Forty replies queued behind each other to a peer that reads every one + /// within the bound. The queue as a whole takes eight bounds; no reply may + /// fail for having waited its turn. + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn a_reading_peer_is_not_failed_for_the_length_of_the_queue() { + let bound = Duration::from_millis(200); + let writer = Arc::new(tokio::sync::Mutex::new(ResponseWriter { + stream: SlowSink { + delay: Duration::from_millis(40), + sleep: None, + }, + failed: false, + })); + let replies: Vec<_> = (0..40) + .map(|id| { + let writer = Arc::clone(&writer); + tokio::spawn(async move { + write_frame_within(&writer, &json!({ "id": id }), bound).await + }) + }) + .collect(); + for reply in replies { + reply + .await + .expect("task") + .expect("a reading peer must get every reply"); + } + } + + /// A stopped peer still releases everyone promptly: the write that meets + /// it fails at the bound, and the rest fail at once behind it. + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn a_stopped_peer_fails_every_queued_reply_within_about_one_bound() { + let bound = Duration::from_millis(200); + let writer = Arc::new(tokio::sync::Mutex::new(ResponseWriter { + stream: StoppedSink, + failed: false, + })); + let started = Instant::now(); + let replies: Vec<_> = (0..32) + .map(|id| { + let writer = Arc::clone(&writer); + tokio::spawn(async move { + write_frame_within(&writer, &json!({ "id": id }), bound).await + }) + }) + .collect(); + for reply in replies { + assert!(reply.await.expect("task").is_err()); + } + assert!( + started.elapsed() < bound * 4, + "queued replies to a stopped peer took {:?}", + started.elapsed() + ); + } +} + #[cfg(test)] mod transport_audit_tests { use super::{read_frame, serve_streams, Frame, StoreSlot, MAX_FRAME_BYTES}; From b30b4773db82c2c86a48e10001037fac3912a5c9 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 28 Sep 2026 16:51:33 +0000 Subject: [PATCH 13/20] fix(safe_fs): check file identity on Windows too; test(repomap): give the scan detector room - safe_fs: check_unchanged and check_appended_only compared the held handle with the file now at the name by dev/ino under cfg(unix) only. On Windows a managed file swapped for another between open and use went unnoticed, and the name's metadata was computed and dropped, which is the unused-variable clippy error that failed the Windows leg once cc compiled again. same_identity compares volume serial and file index from the handles on Windows (windows_information, already used here), dev/ino on unix. Clippy is clean on linux; the Windows build is checked by CI (the vendored C grammars cannot be cross-built here). - repomap: TestAnAreaIsFoundWithoutWalkingEveryIndexedFile failed on macOS runners at 3.4x and 7.4x against a bound of 3x for an 8x larger map, though AreaForPath never touches the file table. The map is now 32x larger and the bound 8x: the lookup passes 5/5 here, and putting the old per-level scan back reads 39.7x and fails. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01FUNo5WZs78gWyCSXf2nRhm (cherry picked from commit c7563cfbf2b035917c3b952a613a08c25bcf9301) --- .../go_orchestrator/repomap/boundary_test.go | 13 +++-- rust/devmap-extract/src/safe_fs.rs | 57 +++++++++++++------ 2 files changed, 48 insertions(+), 22 deletions(-) diff --git a/backend/go_orchestrator/repomap/boundary_test.go b/backend/go_orchestrator/repomap/boundary_test.go index b6301bce..5ea8027f 100644 --- a/backend/go_orchestrator/repomap/boundary_test.go +++ b/backend/go_orchestrator/repomap/boundary_test.go @@ -451,11 +451,16 @@ func TestAnAreaIsFoundWithoutWalkingEveryIndexedFile(t *testing.T) { // be work rather than memory: the miss is timed against a map eight times // larger holding the same areas. A scan of the path table grows with it; a // set lookup does not. - small, large := build(256), build(2048) + // + // The gap is 32x the files against a bound of 8x. It was 8x against 3x, + // and shared macOS runners measured 3.4x and 7.4x for a lookup that never + // touches the file table: scheduler noise alone crossed a bound that left + // no room for it. A scan still reads about 32x and fails by a wide margin. + small, large := build(256), build(256*32) const probe = "elsewhere/entirely/deep/enough/x.go" ratio := pairedLookupRatio(small, large, probe) - if ratio > 3 { - t.Fatalf("resolving one path took %.1fx longer against 8x the files; the lookup is "+ + if ratio > 8 { + t.Fatalf("resolving one path took %.1fx longer against 32x the files; the lookup is "+ "walking the path table rather than consulting the set of areas", ratio) } } @@ -467,7 +472,7 @@ func TestAnAreaIsFoundWithoutWalkingEveryIndexedFile(t *testing.T) { // pause looks like the large map scanning the path table. Pairing them // measures both under the same load, and alternating which runs first // cancels a drift across the pair. A real scan of 8x the files is still -// about 8x on every pair, so the median stays over the bound. +// about 32x on every pair, so the median stays over the bound. func pairedLookupRatio(small, large *Map, probe string) float64 { const rounds = 9 ratios := make([]float64, 0, rounds) diff --git a/rust/devmap-extract/src/safe_fs.rs b/rust/devmap-extract/src/safe_fs.rs index 30559636..ece2feea 100644 --- a/rust/devmap-extract/src/safe_fs.rs +++ b/rust/devmap-extract/src/safe_fs.rs @@ -394,15 +394,11 @@ impl SafeFile { let current = check_regular(&self.file, self.access != Access::Read)?; let named = open_regular_child(&self._parent, &self.name, Access::Read, Creation::Never)?; let named_metadata = check_regular(&named, self.access != Access::Read)?; - #[cfg(unix)] - { - use std::os::unix::fs::MetadataExt; - if current.dev() != named_metadata.dev() || current.ino() != named_metadata.ino() { - return Err(io::Error::new( - io::ErrorKind::Interrupted, - "managed file was replaced while being examined", - )); - } + if !same_identity(&self.file, ¤t, &named, &named_metadata)? { + return Err(io::Error::new( + io::ErrorKind::Interrupted, + "managed file was replaced while being examined", + )); } if current.len() < self.initial.len() { return Err(refused("managed file was truncated while being examined")); @@ -429,15 +425,11 @@ impl SafeFile { let current = check_regular(&self.file, self.access != Access::Read)?; let named = open_regular_child(&self._parent, &self.name, Access::Read, Creation::Never)?; let named_metadata = check_regular(&named, self.access != Access::Read)?; - #[cfg(unix)] - { - use std::os::unix::fs::MetadataExt; - if current.dev() != named_metadata.dev() || current.ino() != named_metadata.ino() { - return Err(io::Error::new( - io::ErrorKind::Interrupted, - "managed file was replaced while being examined", - )); - } + if !same_identity(&self.file, ¤t, &named, &named_metadata)? { + return Err(io::Error::new( + io::ErrorKind::Interrupted, + "managed file was replaced while being examined", + )); } if named_metadata.len() != current.len() || named_metadata.modified()? != current.modified()? @@ -667,6 +659,35 @@ fn open_regular_child( } } +/// Whether the handle held and the file now at the name are one file. +/// +/// unix compares device and inode. Windows compares volume serial and file +/// index, from the handles: before this the check was unix-only, so on +/// Windows a managed file swapped for another between open and use was not +/// noticed, and the name's metadata was computed and then dropped unused. +#[cfg(unix)] +fn same_identity( + _held: &File, + held: &std::fs::Metadata, + _named: &File, + named: &std::fs::Metadata, +) -> io::Result { + use std::os::unix::fs::MetadataExt; + Ok(held.dev() == named.dev() && held.ino() == named.ino()) +} + +#[cfg(windows)] +fn same_identity( + held: &File, + _held: &std::fs::Metadata, + named: &File, + _named: &std::fs::Metadata, +) -> io::Result { + let a = windows_information(held)?; + let b = windows_information(named)?; + Ok(a.volume == b.volume && a.index_high == b.index_high && a.index_low == b.index_low) +} + #[cfg(windows)] fn windows_options() -> std::fs::OpenOptions { use std::os::windows::fs::OpenOptionsExt; From b0314917f9ba86cec71e6c3367233040af3ae33a Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 28 Sep 2026 19:28:42 +0000 Subject: [PATCH 14/20] fix(clippy,windows): the last two Windows lint errors Checked locally with the windows-gnu target and mingw-w64, the same workspace set rust.yml's Windows Clippy step lints; it is clean now: - same_identity (added in c7563cf) named a File and a Metadata `held` and `_held`; clippy::similar_names. Renamed to held_file / named_file. - dc-regress owners_and_test_signal: two tests drive a /bin/sh stub standing in for git and returned early on non-unix, which passed without testing anything and is unreachable_code under -D warnings. They are #[cfg(unix)] now, since the mechanism cannot exist on Windows. The other three tests in the file still run everywhere. Linux clippy and the file's tests (5/5) pass. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01FUNo5WZs78gWyCSXf2nRhm (cherry picked from commit a21ee78956a704f4f0d2609f35f0070975daf866) --- .../dc-regress/tests/owners_and_test_signal.rs | 18 ++++++++---------- rust/devmap-extract/src/safe_fs.rs | 12 ++++++------ 2 files changed, 14 insertions(+), 16 deletions(-) diff --git a/rust/dc-regress/tests/owners_and_test_signal.rs b/rust/dc-regress/tests/owners_and_test_signal.rs index 5d3b010d..e2dd1e0b 100644 --- a/rust/dc-regress/tests/owners_and_test_signal.rs +++ b/rust/dc-regress/tests/owners_and_test_signal.rs @@ -108,6 +108,10 @@ fn a_repo_with_no_commits_marks_owners_unavailable() { /// An empty author is not invented into a placeholder owner. It is named as /// unavailable so completeness stays honest. +// Drives a /bin/sh stub standing in for git; there is no shell to run it on +// Windows. It was an early `return` there, which passed without testing +// anything and tripped `unreachable_code` under -D warnings. +#[cfg(unix)] #[test] fn an_empty_author_is_unavailable_not_invented() { let root = temp_root("empty-author"); @@ -150,7 +154,6 @@ fn an_empty_author_is_unavailable_not_invented() { // Drive the collector against a stub that emits one empty author line. let stub = root.join("empty-author-git"); - #[cfg(unix)] { use std::os::unix::fs::PermissionsExt; std::fs::write( @@ -162,10 +165,6 @@ fn an_empty_author_is_unavailable_not_invented() { perms.set_mode(0o755); std::fs::set_permissions(&stub, perms).unwrap(); } - #[cfg(not(unix))] - { - return; - } let (owners, gaps) = owners_for_paths_with_program( stub.as_os_str(), @@ -191,6 +190,10 @@ fn an_empty_author_is_unavailable_not_invented() { /// A timeout after the first of many paths keeps the owners already found and /// records unavailable — it does not discard them or pretend the pass finished. +// Drives a /bin/sh stub standing in for git; there is no shell to run it on +// Windows. It was an early `return` there, which passed without testing +// anything and tripped `unreachable_code` under -D warnings. +#[cfg(unix)] #[test] fn a_timeout_after_one_path_keeps_partial_owners_and_is_incomplete() { let root = temp_root("timeout"); @@ -205,7 +208,6 @@ fn a_timeout_after_one_path_keeps_partial_owners_and_is_incomplete() { let counter = root.join("log-count"); let stub = root.join("slow-second-git"); - #[cfg(unix)] { use std::os::unix::fs::PermissionsExt; let script = format!( @@ -217,10 +219,6 @@ fn a_timeout_after_one_path_keeps_partial_owners_and_is_incomplete() { perms.set_mode(0o755); std::fs::set_permissions(&stub, perms).unwrap(); } - #[cfg(not(unix))] - { - return; - } let (owners, gaps) = owners_for_paths_with_program( stub.as_os_str(), diff --git a/rust/devmap-extract/src/safe_fs.rs b/rust/devmap-extract/src/safe_fs.rs index ece2feea..1ad4b82a 100644 --- a/rust/devmap-extract/src/safe_fs.rs +++ b/rust/devmap-extract/src/safe_fs.rs @@ -667,9 +667,9 @@ fn open_regular_child( /// noticed, and the name's metadata was computed and then dropped unused. #[cfg(unix)] fn same_identity( - _held: &File, + _held_file: &File, held: &std::fs::Metadata, - _named: &File, + _named_file: &File, named: &std::fs::Metadata, ) -> io::Result { use std::os::unix::fs::MetadataExt; @@ -678,13 +678,13 @@ fn same_identity( #[cfg(windows)] fn same_identity( - held: &File, + held_file: &File, _held: &std::fs::Metadata, - named: &File, + named_file: &File, _named: &std::fs::Metadata, ) -> io::Result { - let a = windows_information(held)?; - let b = windows_information(named)?; + let a = windows_information(held_file)?; + let b = windows_information(named_file)?; Ok(a.volume == b.volume && a.index_high == b.index_high && a.index_low == b.index_low) } From 9a1d8d2dd195440d6e745b022989aa3b5817814b Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 28 Sep 2026 19:46:12 +0000 Subject: [PATCH 15/20] fix(windows): native path validation, a shell-free detached build, sharing-violation retries The Windows leg compiled for the first time in b6e9673 and showed 28 failing tests across 12 targets. Taken by cause: Production - repo_path / MCP roots (devmap-serve root_resolve): the unix rule ran on every host, so a Windows server refused every native path ("this server only opens unix filesystem paths"). Validation is now the host's own: unix still refuses C:\... and /C:/..., Windows refuses a drive-less /... instead of resolving it against the current drive, and takes only file:///:/... URIs. Neither side reinterprets the other's spelling. Unit tests for both directions; the unix URI test is unix-only. - Hook background builds (devmap-cli hook): they ran under `sh -c`, which Windows does not guarantee, and the lock-owner liveness check answered "alive" for every pid on non-unix, so a dead build's lock stood for the full 30-minute ceiling. On Windows the build is now spawned directly as a detached process, removes its own lock on exit (OwnedLock, only when the lock's pid file names it, so the variable cannot direct any other removal), and OpenProcess/GetExitCodeProcess answers liveness. The hook's own std handles are made non-inheritable first; otherwise the build held the host's pipe and the host waited on the hook until the build ended. unix behaviour is unchanged. - write_atomic (devmap-query): the snapshot comparison did not retry os error 32 while another writer's rename was replacing the target, which failed concurrent artifact and `devmap claude` bundle writes. It now waits within a 5 s deadline, and the rename retry drops its 100-attempt cap (about 0.5 s) for the same deadline. - Store::open (devmap-store): SQLITE_PROTOCOL, a WAL-index lock race lost after SQLite's own retries, is retried from the top for up to 20 s with backoff; open is re-entrant because migrate re-reads the version under the write lock. All eight concurrent openers of a legacy store failed on it. - paths: state paths are joined per component, so doctor no longer prints `.\.devmap\codeintel/devmap.sqlite`. Tests (the code was right, the fixture was not) - Canonical paths on Windows are `\\?\` verbatim with long names where the temp dir used 8.3 (`RUNNER~1`). Assertions that a message names a path accept it as written or canonical without the prefix (`names`); the `paths` db_path comparison ignores the prefix on both sides. - Root URIs were `format!("file://{}", path)`, which is `file://C:\...` on Windows; `file_uri` builds the platform's form. - Host configs were JSON built by format!, so a Windows path was an invalid escape and the config registered nothing; now serde_json. - A repository name containing `"` cannot exist on Windows; there the fixture keeps the other hostile characters. - make_undetachable set the read-only attribute, which Windows ignores on directories; it now denies writes with icacls. - A mid-session store deletion cannot happen on Windows while the server holds the file; the test asserts the refused delete and that the store still serves. - fanout.sh runs only on unix, where its consumers (verify.sh, the memory probe) run; a bare `bash` on a Windows runner is the WSL launcher. The rusqlite half of the test still runs everywhere. Verified: clippy -D warnings clean for x86_64-pc-windows-gnu (mingw-w64) and linux across the rust.yml workspace set; linux tests for every touched binary pass. Three tests that rely on permission denials fail identically before and after this change here because the container runs as root (claude_integration an_unwritable_output_directory..., kernel_defects k1_..., hook an_unremovable_stale_lock..., pinned_root_disables_discovery); CI runs them as a normal user. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01FUNo5WZs78gWyCSXf2nRhm (cherry picked from commit 38c4d29d905630b3a8fc34ce6fd9e5e355aaaf2f) --- rust/devmap-cli/src/hook.rs | 207 +++++++++++++++--- rust/devmap-cli/src/main.rs | 3 + rust/devmap-cli/tests/doctor_gaps.rs | 21 +- rust/devmap-cli/tests/hook_conformance.rs | 50 ++++- rust/devmap-cli/tests/repo_scope_hardening.rs | 27 ++- .../tests/state_discovery_contract.rs | 25 ++- rust/devmap-cli/tests/test_fanout_metric.rs | 46 ++-- rust/devmap-extract/src/paths.rs | 23 +- rust/devmap-query/src/artifacts.rs | 46 +++- rust/devmap-serve/src/root_resolve.rs | 123 +++++++++-- rust/devmap-serve/tests/mcp_repo_scope.rs | 44 +++- .../tests/phase1_freshness_reach.rs | 48 +++- rust/devmap-store/src/db.rs | 39 +++- rust/devmap-store/tests/kernel_defects.rs | 17 +- 14 files changed, 589 insertions(+), 130 deletions(-) diff --git a/rust/devmap-cli/src/hook.rs b/rust/devmap-cli/src/hook.rs index 8a535dbf..d49435cd 100644 --- a/rust/devmap-cli/src/hook.rs +++ b/rust/devmap-cli/src/hook.rs @@ -4,6 +4,7 @@ //! Exit 2 is treated as "block the agent" on those hosts, so this path never //! exits 2: success and no-ops are 0, failures are 1. +use std::ffi::OsStr; use std::fs; use std::io::{self, Read}; use std::path::{Path, PathBuf}; @@ -1847,10 +1848,7 @@ fn detach_build(executable: &Path, root: &Path) -> anyhow::Result<()> { } spawn_detached_with_cleanup( executable, - &format!( - "--root {} build", - shell_single_quote(&root.display().to_string()) - ), + &[OsStr::new("--root"), root.as_os_str(), OsStr::new("build")], &lock_dir, ) } @@ -1862,10 +1860,11 @@ fn detach_session_report(executable: &Path, root: &Path) -> anyhow::Result<()> { } spawn_detached_with_cleanup( executable, - &format!( - "--root {} session-report", - shell_single_quote(&root.display().to_string()) - ), + &[ + OsStr::new("--root"), + root.as_os_str(), + OsStr::new("session-report"), + ], &lock_dir, ) } @@ -1982,44 +1981,165 @@ fn lock_dir_is_stale(lock_dir: &Path) -> bool { let rc = unsafe { libc::kill(pid, 0) }; rc != 0 } - #[cfg(not(unix))] + #[cfg(windows)] + { + !windows_process_is_running(pid as u32) + } + #[cfg(not(any(unix, windows)))] { let _ = pid; false } } +/// Keep this process's own stdin/stdout/stderr out of every child it spawns. +/// +/// Windows hands a child every inheritable handle of its parent, whatever the +/// child's own stdio is set to, and the pipes the host gave this hook are +/// inheritable. A detached build would hold the host's pipe open, and the host, +/// waiting for the hook's output to end, would wait for the build. unix never +/// had this: std marks every descriptor but 0-2 close-on-exec, and 0-2 are +/// redirected. Clearing the flag changes only what children receive; this +/// process's own reads and writes are unaffected. +#[cfg(windows)] +fn stop_standard_handles_from_being_inherited() { + const STD_INPUT_HANDLE: u32 = -10i32 as u32; + const STD_OUTPUT_HANDLE: u32 = -11i32 as u32; + const STD_ERROR_HANDLE: u32 = -12i32 as u32; + const HANDLE_FLAG_INHERIT: u32 = 0x1; + #[link(name = "kernel32")] + extern "system" { + fn GetStdHandle(which: u32) -> *mut std::ffi::c_void; + fn SetHandleInformation(handle: *mut std::ffi::c_void, mask: u32, flags: u32) -> i32; + } + for which in [STD_INPUT_HANDLE, STD_OUTPUT_HANDLE, STD_ERROR_HANDLE] { + // SAFETY: GetStdHandle returns this process's handle or null/invalid, + // and SetHandleInformation on either only fails. + unsafe { + let handle = GetStdHandle(which); + if !handle.is_null() && handle as isize != -1 { + SetHandleInformation(handle, HANDLE_FLAG_INHERIT, 0); + } + } + } +} + +/// Whether `pid` names a running process. It was not asked at all on Windows: +/// every lock read as live until [`LOCK_MAX_AGE`], so a build that died without +/// removing its lock stopped that repository's rebuilds for half an hour. +#[cfg(windows)] +fn windows_process_is_running(pid: u32) -> bool { + const PROCESS_QUERY_LIMITED_INFORMATION: u32 = 0x1000; + const STILL_ACTIVE: u32 = 259; + const ERROR_ACCESS_DENIED: u32 = 5; + #[link(name = "kernel32")] + extern "system" { + fn OpenProcess(access: u32, inherit: i32, pid: u32) -> *mut std::ffi::c_void; + fn GetExitCodeProcess(process: *mut std::ffi::c_void, code: *mut u32) -> i32; + fn CloseHandle(handle: *mut std::ffi::c_void) -> i32; + fn GetLastError() -> u32; + } + // SAFETY: plain Win32 calls on a handle this function opens and closes. + unsafe { + let handle = OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION, 0, pid); + if handle.is_null() { + // A process this user may not query still exists; anything else + // (invalid parameter) is no process with that pid. + return GetLastError() == ERROR_ACCESS_DENIED; + } + let mut code = 0u32; + let ok = GetExitCodeProcess(handle, &mut code) != 0; + CloseHandle(handle); + // Unknown is held, like every other unknown owner here. + !ok || code == STILL_ACTIVE + } +} + +/// The coalescing lock this process was started to hold, released on drop. +/// +/// Only the Windows spawn names one ([`OWNED_LOCK_ENV`]). It is removed only if +/// its `pid` file names this process, so the variable cannot direct the removal +/// of any other directory: a stale or hostile value is ignored. +pub struct OwnedLock(Option); + +impl OwnedLock { + /// Take the lock named in the environment, and unset the variable so no + /// grandchild inherits the claim. + pub fn from_env() -> Self { + let Some(dir) = std::env::var_os(OWNED_LOCK_ENV) else { + return Self(None); + }; + // Only the Windows spawn sets it, and Windows serialises environment + // access in the OS, so the runtime threads already started are safe. + std::env::remove_var(OWNED_LOCK_ENV); + Self(Some(PathBuf::from(dir))) + } +} + +impl Drop for OwnedLock { + fn drop(&mut self) { + let Some(dir) = self.0.take() else { + return; + }; + // The spawning hook writes our pid a moment after the spawn; a child + // that finishes first leaves the lock for the stale check, which now + // sees this pid dead. + let owner = devmap_query::stat_memo::read_bounded(&dir.join("pid"), MAX_PID_BYTES); + if owner.as_deref().map(str::trim) == Some(std::process::id().to_string().as_str()) { + let _ = fs::remove_dir_all(&dir); + } + } +} + +#[cfg(unix)] fn shell_single_quote(text: &str) -> String { format!("'{}'", text.replace('\'', "'\\''")) } +/// Environment variable naming the coalescing lock a detached child owns and +/// must remove when it exits. Set only on Windows, where there is no shell to +/// do it; see [`OwnedLock`]. +pub const OWNED_LOCK_ENV: &str = "DEVMAP_HOOK_OWNED_LOCK"; + +/// Spawn `executable args…` detached, holding `lock_dir` for its lifetime. +/// +/// unix runs it under `sh -c` in a new session, and the shell removes the lock +/// when the child exits. Windows has no shell to rely on — this path required +/// `sh` there, and with none on PATH no hook ever started a build — so the +/// child is started directly as a detached process and removes the lock itself +/// on exit ([`OwnedLock`]); a child that dies without doing so leaves a lock +/// [`lock_dir_is_stale`] now recognises by its dead pid. fn spawn_detached_with_cleanup( executable: &Path, - args_shell: &str, + args: &[&OsStr], lock_dir: &Path, ) -> anyhow::Result<()> { - let exe = executable - .to_str() - .ok_or_else(|| anyhow!("executable path is not UTF-8"))?; - let lock = lock_dir - .to_str() - .ok_or_else(|| anyhow!("lock path is not UTF-8"))?; - // The lock directory is held for the child's lifetime so concurrent hooks - // coalesce onto one build. The shell removes it when the child exits. - let script = format!( - "{} {}; status=$?; rm -rf {}; exit $status", - shell_single_quote(exe), - args_shell, - shell_single_quote(lock) - ); - let mut cmd = Command::new("sh"); - cmd.arg("-c") - .arg(script) - .stdin(Stdio::null()) - .stdout(Stdio::null()) - .stderr(Stdio::null()); #[cfg(unix)] - { + let mut cmd = { + let exe = executable + .to_str() + .ok_or_else(|| anyhow!("executable path is not UTF-8"))?; + let lock = lock_dir + .to_str() + .ok_or_else(|| anyhow!("lock path is not UTF-8"))?; + let mut quoted = Vec::with_capacity(args.len()); + for arg in args { + let arg = arg + .to_str() + .ok_or_else(|| anyhow!("argument is not UTF-8"))?; + quoted.push(shell_single_quote(arg)); + } + // The lock directory is held for the child's lifetime so concurrent + // hooks coalesce onto one build. The shell removes it when the child + // exits. + let script = format!( + "{} {}; status=$?; rm -rf {}; exit $status", + shell_single_quote(exe), + quoted.join(" "), + shell_single_quote(lock) + ); + let mut cmd = Command::new("sh"); + cmd.arg("-c").arg(script); use std::os::unix::process::CommandExt; unsafe { cmd.pre_exec(|| { @@ -2029,7 +2149,28 @@ fn spawn_detached_with_cleanup( Ok(()) }); } - } + cmd + }; + #[cfg(windows)] + let mut cmd = { + use std::os::windows::process::CommandExt; + stop_standard_handles_from_being_inherited(); + const DETACHED_PROCESS: u32 = 0x0000_0008; + const CREATE_NEW_PROCESS_GROUP: u32 = 0x0000_0200; + let mut cmd = Command::new(executable); + cmd.args(args) + .env(OWNED_LOCK_ENV, lock_dir) + .creation_flags(DETACHED_PROCESS | CREATE_NEW_PROCESS_GROUP); + cmd + }; + #[cfg(not(any(unix, windows)))] + let mut cmd: Command = { + let _ = (executable, args); + anyhow::bail!("detached builds are not supported on this platform"); + }; + cmd.stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()); let child = cmd .spawn() .with_context(|| format!("spawn detached {}", executable.display()))?; @@ -3684,7 +3825,7 @@ mod lock_tests { let lock = root.join("lock"); assert!(try_acquire_lock_dir(&lock).unwrap()); - spawn_detached_with_cleanup(Path::new("/bin/sleep"), "5", &lock).unwrap(); + spawn_detached_with_cleanup(Path::new("/bin/sleep"), &[OsStr::new("5")], &lock).unwrap(); std::thread::sleep(Duration::from_millis(200)); let recorded: i32 = fs::read_to_string(lock.join("pid")) diff --git a/rust/devmap-cli/src/main.rs b/rust/devmap-cli/src/main.rs index e4f1c2f1..55a73bd6 100644 --- a/rust/devmap-cli/src/main.rs +++ b/rust/devmap-cli/src/main.rs @@ -4988,6 +4988,9 @@ fn on_command_stack(work: impl FnOnce() -> T + Send + 'static #[tokio::main] async fn main() -> std::process::ExitCode { + // Held to the end of `main`: a detached hook build on Windows removes its + // coalescing lock when it finishes (see `hook::spawn_detached_with_cleanup`). + let _owned_lock = hook::OwnedLock::from_env(); let started = Instant::now(); let (cli, matches) = on_command_stack(|| { let matches = Cli::command().get_matches(); diff --git a/rust/devmap-cli/tests/doctor_gaps.rs b/rust/devmap-cli/tests/doctor_gaps.rs index edf929bd..47f2ca08 100644 --- a/rust/devmap-cli/tests/doctor_gaps.rs +++ b/rust/devmap-cli/tests/doctor_gaps.rs @@ -50,20 +50,23 @@ fn pinned_registrations_are_not_counted_as_global_duplicates() { // One global (no --root) and one pinned project entry. fs::write( home.join(".cursor/mcp.json"), - format!( - r#"{{"mcpServers":{{"devmap":{{"type":"stdio","command":"{}","args":["mcp"]}}}}}}"#, - devmap_bin().display() - ), + // Built, not formatted: a Windows path in a JSON literal is an + // invalid escape, and an unparseable config registers nothing. + serde_json::json!({"mcpServers": {"devmap": { + "type": "stdio", "command": devmap_bin(), "args": ["mcp"] + }}}) + .to_string(), ) .unwrap(); fs::create_dir_all(cwd.join(".cursor")).unwrap(); fs::write( cwd.join(".cursor/mcp.json"), - format!( - r#"{{"mcpServers":{{"devmap":{{"type":"stdio","command":"{}","args":["--root","{}","mcp"]}}}}}}"#, - devmap_bin().display(), - cwd.display() - ), + serde_json::json!({"mcpServers": {"devmap": { + "type": "stdio", + "command": devmap_bin(), + "args": ["--root", cwd, "mcp"] + }}}) + .to_string(), ) .unwrap(); let payload = doctor_with_home(&home, &cwd); diff --git a/rust/devmap-cli/tests/hook_conformance.rs b/rust/devmap-cli/tests/hook_conformance.rs index a71f379e..b9ccdbd8 100644 --- a/rust/devmap-cli/tests/hook_conformance.rs +++ b/rust/devmap-cli/tests/hook_conformance.rs @@ -520,17 +520,47 @@ fn unsafe_home_without_store_is_skipped() { /// and this file already relies on it for the sibling-union regression. fn make_undetachable(root: &Path) { let codeintel = root.join(".devcouncil/codeintel"); - let mut perms = std::fs::metadata(&codeintel).unwrap().permissions(); - perms.set_readonly(true); - std::fs::set_permissions(&codeintel, perms).unwrap(); + #[cfg(windows)] + { + // Windows ignores the read-only attribute on a directory, so the hook + // still created its lock there and this helper disabled nothing. A + // deny ACE for Everyone (S-1-1-0) on writing is what refuses the + // lock's `mkdir`, administrators included. + icacls(&codeintel, &["/deny", "*S-1-1-0:(W)"]); + } + #[cfg(not(windows))] + { + let mut perms = std::fs::metadata(&codeintel).unwrap().permissions(); + perms.set_readonly(true); + std::fs::set_permissions(&codeintel, perms).unwrap(); + } } fn make_detachable(root: &Path) { let codeintel = root.join(".devcouncil/codeintel"); - let mut perms = std::fs::metadata(&codeintel).unwrap().permissions(); - #[allow(clippy::permissions_set_readonly_false)] - perms.set_readonly(false); - std::fs::set_permissions(&codeintel, perms).unwrap(); + #[cfg(windows)] + icacls(&codeintel, &["/remove:d", "*S-1-1-0"]); + #[cfg(not(windows))] + { + let mut perms = std::fs::metadata(&codeintel).unwrap().permissions(); + #[allow(clippy::permissions_set_readonly_false)] + perms.set_readonly(false); + std::fs::set_permissions(&codeintel, perms).unwrap(); + } +} + +#[cfg(windows)] +fn icacls(path: &Path, args: &[&str]) { + let out = Command::new("icacls") + .arg(path) + .args(args) + .output() + .expect("icacls runs"); + assert!( + out.status.success(), + "icacls {args:?} failed: {}", + String::from_utf8_lossy(&out.stdout) + ); } /// `--root` must pin the repository, and the payload must not move it. @@ -686,7 +716,13 @@ fn cursor_session_start_stdout_survives_hostile_briefing_characters() { let root = scratch("cursor-quotes"); seed_store(&root); // The repository *name* is interpolated into the briefing. + // `"` cannot appear in a Windows file name, so no Windows repository can + // carry it; there the name keeps every other character that is hostile to + // a hand-built JSON string. Creating it failed with os error 123. + #[cfg(not(windows))] let named = root.join("repo \"quotes\" and 日本語"); + #[cfg(windows)] + let named = root.join("repo 'quotes' {braces} and 日本語"); std::fs::create_dir_all(&named).unwrap(); seed_store(&named); let payload = json!({ diff --git a/rust/devmap-cli/tests/repo_scope_hardening.rs b/rust/devmap-cli/tests/repo_scope_hardening.rs index dd70e5f0..3e7ed36b 100644 --- a/rust/devmap-cli/tests/repo_scope_hardening.rs +++ b/rust/devmap-cli/tests/repo_scope_hardening.rs @@ -6,6 +6,21 @@ use std::time::{SystemTime, UNIX_EPOCH}; use serde_json::Value; +/// Whether `text` names `path` in a spelling the platform gives it: as +/// written, or canonical. Windows canonicalises to a `\\?\` verbatim path +/// with long names where the temp dir used 8.3 short ones (`RUNNER~1`), and +/// macOS to `/private/var` for `/var`; every one is the same directory. +fn names(text: &str, path: &std::path::Path) -> bool { + if text.contains(&path.display().to_string()) { + return true; + } + let Ok(canonical) = path.canonicalize() else { + return false; + }; + let shown = canonical.display().to_string(); + text.contains(shown.strip_prefix(r"\\?\").unwrap_or(&shown)) +} + fn scratch(name: &str) -> PathBuf { let root = std::env::temp_dir().join(format!( "devmap-cli-scope-{name}-{}-{}", @@ -56,9 +71,11 @@ fn write_mcp(path: &Path, command: &str) { } std::fs::write( path, - format!( - r#"{{"mcpServers":{{"devmap":{{"type":"stdio","command":"{command}","args":["mcp"]}}}}}}"# - ), + // Built, not formatted: a Windows path spliced into a JSON literal is + // `"C:\Users\…"`, an invalid escape, and the host config it writes + // fails to parse — doctor then correctly reports nothing registered. + serde_json::json!({"mcpServers": {"devmap": {"type": "stdio", "command": command, "args": ["mcp"]}}}) + .to_string(), ) .unwrap(); } @@ -176,7 +193,7 @@ fn build_from_home_refuses_instead_of_indexing() { String::from_utf8_lossy(&out.stderr) ); assert!( - combined.contains(&home.display().to_string()), + names(&combined, &home), "refusal must name the resolved root: {combined}" ); let store = home @@ -212,7 +229,7 @@ fn doctor_warns_when_a_host_config_names_a_missing_binary() { "a host config pointing at a path that is not a file must not look like a clean doctor: {payload}" ); assert!( - warning.contains(&missing.display().to_string()) || warning.contains("no-such-devmap"), + names(warning, &missing) || warning.contains("no-such-devmap"), "warning must name the missing path: {warning}" ); let skew = payload.get("binary_skew_warning"); diff --git a/rust/devmap-cli/tests/state_discovery_contract.rs b/rust/devmap-cli/tests/state_discovery_contract.rs index 60a0e6f3..ae209e72 100644 --- a/rust/devmap-cli/tests/state_discovery_contract.rs +++ b/rust/devmap-cli/tests/state_discovery_contract.rs @@ -5,6 +5,13 @@ use std::process::{Command, Stdio}; use std::sync::atomic::{AtomicU64, Ordering}; use std::time::{Duration, Instant, SystemTime, UNIX_EPOCH}; +/// A path's text without a Windows `\\?\` verbatim prefix. `paths` reports +/// `db_path` in plain form while the fixture root is canonical (verbatim on +/// Windows); both name the same file, and elsewhere this is the identity. +fn plain(path: Option<&str>) -> Option<&str> { + path.map(|p| p.strip_prefix(r"\\?\").unwrap_or(p)) +} + struct Fixture(PathBuf); impl Fixture { @@ -108,7 +115,7 @@ fn relative_repository_paths_do_not_duplicate_the_repository_directory() { fixture.run(&fixture.0, &["build", "repo", "--manifest"], None); let report = fixture.run(&fixture.0, &["paths", "repo"], None); let expected = fixture.repo().join(".devmap/codeintel/devmap.sqlite"); - assert_eq!(report["db_path"].as_str(), expected.to_str()); + assert_eq!(plain(report["db_path"].as_str()), plain(expected.to_str())); assert_eq!(report["store_exists"], true); } @@ -126,8 +133,8 @@ fn explicit_relative_database_paths_stay_relative_to_the_invoking_directory() { None, ); assert_eq!( - report["db_path"].as_str(), - fixture.0.join("custom.sqlite").to_str() + plain(report["db_path"].as_str()), + plain(fixture.0.join("custom.sqlite").to_str()) ); assert_eq!(report["store_exists"], true); } @@ -174,11 +181,13 @@ fn relative_home_override_is_repository_relative_only_once() { fixture.run(&fixture.0, &["build", "repo", "--manifest"], Some(home)); let report = fixture.run(&fixture.0, &["paths", "repo"], Some(home)); assert_eq!( - report["db_path"].as_str(), - fixture - .repo() - .join("state/codeintel/devmap.sqlite") - .to_str() + plain(report["db_path"].as_str()), + plain( + fixture + .repo() + .join("state/codeintel/devmap.sqlite") + .to_str() + ) ); assert_eq!(report["store_exists"], true); } diff --git a/rust/devmap-cli/tests/test_fanout_metric.rs b/rust/devmap-cli/tests/test_fanout_metric.rs index 9e37a362..b7ccc3e1 100644 --- a/rust/devmap-cli/tests/test_fanout_metric.rs +++ b/rust/devmap-cli/tests/test_fanout_metric.rs @@ -154,25 +154,33 @@ fn the_fanout_metric_matches_a_hand_counted_corpus() { String::from_utf8_lossy(&out.stderr) ); - // The shell gates do not use rusqlite. They run `tools/fanout.sh`, which - // is whatever `sqlite3` is on PATH. Apple's CLI `-readonly` cannot open - // this store; the script must still return the same row. - let script = Path::new(env!("CARGO_MANIFEST_DIR")).join("../tools/fanout.sh"); - let shell = Command::new("bash") - .arg(&script) - .arg(&db) - .output() - .expect("fanout.sh"); - assert!( - shell.status.success(), - "fanout.sh failed: {}", - String::from_utf8_lossy(&shell.stderr) - ); - let shell_line = String::from_utf8(shell.stdout).expect("fanout.sh stdout is utf-8"); - assert!( - shell_line.contains(&format!("fanout_edges={EXPECTED_EDGES}")), - "fanout.sh row did not match the hand count: {shell_line}" - ); + // The shell half runs where its consumers do: `verify.sh` and + // `memory_model_probe.sh` are unix shell gates. On a Windows runner a bare + // `bash` is the WSL launcher, which exits non-zero with nothing on stderr + // when no distribution is installed. Everything below this block checks + // the same numbers through rusqlite and runs everywhere. + #[cfg(unix)] + { + // The shell gates do not use rusqlite. They run `tools/fanout.sh`, which + // is whatever `sqlite3` is on PATH. Apple's CLI `-readonly` cannot open + // this store; the script must still return the same row. + let script = Path::new(env!("CARGO_MANIFEST_DIR")).join("../tools/fanout.sh"); + let shell = Command::new("bash") + .arg(&script) + .arg(&db) + .output() + .expect("fanout.sh"); + assert!( + shell.status.success(), + "fanout.sh failed: {}", + String::from_utf8_lossy(&shell.stderr) + ); + let shell_line = String::from_utf8(shell.stdout).expect("fanout.sh stdout is utf-8"); + assert!( + shell_line.contains(&format!("fanout_edges={EXPECTED_EDGES}")), + "fanout.sh row did not match the hand count: {shell_line}" + ); + } let f = fanout(&db); diff --git a/rust/devmap-extract/src/paths.rs b/rust/devmap-extract/src/paths.rs index 41e7ee14..8348e100 100644 --- a/rust/devmap-extract/src/paths.rs +++ b/rust/devmap-extract/src/paths.rs @@ -144,44 +144,51 @@ pub fn resolve_state_dir( standalone } +/// `base` joined with a `/`-separated relative path, one component at a time, +/// so the result uses the platform's separator throughout. A plain join kept +/// the `/` on Windows (`.\.devmap\codeintel/devmap.sqlite` in `doctor`). +fn join_rel(base: PathBuf, rel: &str) -> PathBuf { + rel.split('/').fold(base, |path, part| path.join(part)) +} + /// Absolute-or-relative path to the store for `root`. pub fn store_path(root: impl AsRef) -> PathBuf { - state_dir(root).join(STORE_RELPATH) + join_rel(state_dir(root), STORE_RELPATH) } /// Path to `repo_map.json` for `root`. pub fn repo_map_path(root: impl AsRef) -> PathBuf { - state_dir(root).join(REPO_MAP_RELPATH) + join_rel(state_dir(root), REPO_MAP_RELPATH) } /// Path to `code_graph.json` for `root`. pub fn code_graph_path(root: impl AsRef) -> PathBuf { - state_dir(root).join(CODE_GRAPH_RELPATH) + join_rel(state_dir(root), CODE_GRAPH_RELPATH) } /// Path to the interned `code_graph.compact.json` for `root`. pub fn compact_code_graph_path(root: impl AsRef) -> PathBuf { - state_dir(root).join(CODE_GRAPH_COMPACT_RELPATH) + join_rel(state_dir(root), CODE_GRAPH_COMPACT_RELPATH) } /// Path to the content-hash memo for `root`. pub fn content_cache_path(root: impl AsRef) -> PathBuf { - state_dir(root).join(CONTENT_CACHE_RELPATH) + join_rel(state_dir(root), CONTENT_CACHE_RELPATH) } /// Path to the `devmap`-binary digest memo for `root`. pub fn binary_digest_cache_path(root: impl AsRef) -> PathBuf { - state_dir(root).join(BINARY_DIGEST_CACHE_RELPATH) + join_rel(state_dir(root), BINARY_DIGEST_CACHE_RELPATH) } /// Path to the workspace registry for `root`. pub fn workspace_path(root: impl AsRef) -> PathBuf { - state_dir(root).join(WORKSPACE_RELPATH) + join_rel(state_dir(root), WORKSPACE_RELPATH) } /// Path to the emitted Claude Code plugin bundle for `root`. pub fn plugin_dir(root: impl AsRef) -> PathBuf { - state_dir(root).join(PLUGIN_RELPATH) + join_rel(state_dir(root), PLUGIN_RELPATH) } /// The repository root a store path belongs to — the inverse of [`store_path`]. diff --git a/rust/devmap-query/src/artifacts.rs b/rust/devmap-query/src/artifacts.rs index 94f0feac..dbdc1bf1 100644 --- a/rust/devmap-query/src/artifacts.rs +++ b/rust/devmap-query/src/artifacts.rs @@ -21,6 +21,23 @@ pub struct ArtifactFingerprint { /// Distinguishes concurrent temp files written by one process. static WRITE_SEQUENCE: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); +/// How long a Windows writer waits out another process's hold on the target. +/// +/// Windows refuses to open or replace a file another handle holds without +/// `FILE_SHARE_DELETE` (os error 32, or 5 for a pending delete), which every +/// concurrent writer here does for a moment while it compares or renames. On +/// unix the same moment is invisible. Bounded, so a file held open for good +/// still fails. +#[cfg(windows)] +const WINDOWS_SHARING_DEADLINE: std::time::Duration = std::time::Duration::from_secs(5); + +/// An error that means "another handle holds this file right now". +#[cfg(windows)] +fn is_sharing_violation(error: &std::io::Error) -> bool { + error.kind() == std::io::ErrorKind::PermissionDenied + || matches!(error.raw_os_error(), Some(32 | 33)) +} + pub fn write_atomic(path: &Path, content: &[u8]) -> std::io::Result { use devmap_extract::safe_fs::{Access, Creation, PinnedDir}; devmap_extract::safe_fs::preflight_write(path)?; @@ -32,7 +49,11 @@ pub fn write_atomic(path: &Path, content: &[u8]) -> std::io::Result { // A concurrent atomic publisher can unlink the snapshot between open // and comparison. Re-read that snapshot within a finite retry budget; // an unsafe live file still refuses publication. - for attempt in 0..16 { + #[cfg(windows)] + let deadline = std::time::Instant::now() + WINDOWS_SHARING_DEADLINE; + let mut attempt = 0; + loop { + attempt += 1; let comparison = (|| -> std::io::Result { let mut existing = parent.open_file(target, Access::Read, Creation::Never)?; existing.require_owned()?; @@ -42,9 +63,19 @@ pub fn write_atomic(path: &Path, content: &[u8]) -> std::io::Result { Ok(true) => return Ok(true), Ok(false) => break, Err(error) if error.kind() == std::io::ErrorKind::NotFound => break, - Err(error) if error.kind() == std::io::ErrorKind::Interrupted && attempt < 15 => { + Err(error) if error.kind() == std::io::ErrorKind::Interrupted && attempt < 16 => { continue } + // Another writer is mid-rename onto this name. It failed the + // whole emission (concurrent_writers_to_one_artifact_all_succeed + // and concurrent `devmap claude` on Windows): wait it out. + #[cfg(windows)] + Err(error) + if is_sharing_violation(&error) && std::time::Instant::now() < deadline => + { + std::thread::sleep(std::time::Duration::from_millis(5)); + continue; + } Err(error) => return Err(error), } } @@ -83,18 +114,15 @@ pub fn write_atomic(path: &Path, content: &[u8]) -> std::io::Result { } #[cfg(windows)] { - let deadline = std::time::Instant::now() + std::time::Duration::from_secs(2); - let mut attempts = 0; + // One deadline, not an attempt count as well: under sixteen-way + // contention a hundred 5 ms attempts ran out long before the time. + let deadline = std::time::Instant::now() + WINDOWS_SHARING_DEADLINE; loop { match parent.rename_child(&temporary, target) { Ok(()) => break, Err(error) - if (error.kind() == std::io::ErrorKind::PermissionDenied - || matches!(error.raw_os_error(), Some(32 | 33))) - && attempts < 100 - && std::time::Instant::now() < deadline => + if is_sharing_violation(&error) && std::time::Instant::now() < deadline => { - attempts += 1; std::thread::sleep(std::time::Duration::from_millis(5)); } Err(error) => return Err(error), diff --git a/rust/devmap-serve/src/root_resolve.rs b/rust/devmap-serve/src/root_resolve.rs index 69109140..bd936887 100644 --- a/rust/devmap-serve/src/root_resolve.rs +++ b/rust/devmap-serve/src/root_resolve.rs @@ -177,11 +177,8 @@ pub fn validate_repo_path(raw: &str) -> Result { raw.len() )); } - if looks_like_windows_path(raw) { - return Err( - "repo_path looks like a Windows path; this server only opens unix filesystem paths" - .into(), - ); + if let Some(refusal) = foreign_path_refusal(raw) { + return Err(refusal.into()); } let path = Path::new(raw); if !path.is_absolute() { @@ -202,11 +199,58 @@ pub fn validate_repo_path(raw: &str) -> Result { }) } -/// MCP `roots/list` URI → a unix filesystem path, or `None` to skip. +/// Why `raw` is a path from the other platform, if it is. +/// +/// A path is opened only in its host's own spelling, never reinterpreted: a +/// unix server refuses `C:\…` and `/C:/…` rather than reading them as +/// relative names or a `/C:` directory, and a Windows server refuses `/Users/…` +/// rather than resolving it against the current drive. This used to be the +/// unix rule on every host, so on Windows every native path was refused with a +/// message saying the server "only opens unix filesystem paths". +#[cfg(not(windows))] +fn foreign_path_refusal(raw: &str) -> Option<&'static str> { + looks_like_windows_path(raw).then_some( + "repo_path looks like a Windows path; this server only opens unix filesystem paths", + ) +} + +#[cfg(windows)] +fn foreign_path_refusal(raw: &str) -> Option<&'static str> { + // `\\server\share`, `//server/share` and `\\?\C:\…` are native; a single + // leading slash is a unix path with no drive. + let unix_style = raw.starts_with('/') && !raw.starts_with("//"); + unix_style.then_some( + "repo_path looks like a unix path; this server only opens Windows paths with a drive or UNC share", + ) +} + +/// MCP `roots/list` URI → a path on this host, or `None` to skip. /// -/// Windows-style (`file:///C:/…`) and non-file URIs are skipped rather than -/// reinterpreted. Percent-decoding refuses invalid UTF-8 rather than using -/// lossy replacement. +/// On unix, Windows-style (`file:///C:/…`) and non-file URIs are skipped +/// rather than reinterpreted. On Windows, only `file:///:/…` is taken, +/// and a drive-less URI (`file:///Users/…`) is skipped. Percent-decoding +/// refuses invalid UTF-8 rather than using lossy replacement. +#[cfg(windows)] +pub fn file_uri_to_path(uri: &str) -> Option { + let rest = uri.strip_prefix("file://")?; + // `file://localhost/C:/…` names the same file as `file:///C:/…`; any other + // authority is a remote host. + let rest = rest.strip_prefix("localhost").unwrap_or(rest); + let path = rest.strip_prefix('/')?; + let decoded = percent_decode_strict(path)?; + let bytes = decoded.as_bytes(); + let drive_absolute = bytes.len() >= 3 + && bytes[0].is_ascii_alphabetic() + && bytes[1] == b':' + && (bytes[2] == b'/' || bytes[2] == b'\\'); + if !drive_absolute { + return None; + } + Some(PathBuf::from(decoded.replace('/', "\\"))) +} + +/// See the Windows variant above. +#[cfg(not(windows))] pub fn file_uri_to_path(uri: &str) -> Option { let path = if let Some(rest) = uri.strip_prefix("file://") { if rest.starts_with('/') { @@ -404,6 +448,21 @@ mod tests { use std::fs; use std::time::{SystemTime, UNIX_EPOCH}; + /// Whether `text` names `path` in a spelling the platform gives it: as + /// written, or canonical. Windows canonicalises to a `\\?\` verbatim path + /// with long names where the temp dir used 8.3 short ones (`RUNNER~1`), and + /// macOS to `/private/var` for `/var`; every one is the same directory. + fn names(text: &str, path: &std::path::Path) -> bool { + if text.contains(&path.display().to_string()) { + return true; + } + let Ok(canonical) = path.canonicalize() else { + return false; + }; + let shown = canonical.display().to_string(); + text.contains(shown.strip_prefix(r"\\?\").unwrap_or(&shown)) + } + fn scratch(name: &str) -> PathBuf { let root = std::env::temp_dir().join(format!( "devmap-root-resolve-{name}-{}-{}", @@ -443,11 +502,11 @@ mod tests { .resolve() .expect_err("cwd store and MCP-root store must not first-wins"); assert!( - err.contains(&via_root.display().to_string()), + names(&err, &via_root), "error must name the MCP root: {err}" ); assert!( - err.contains(&via_cwd.display().to_string()), + names(&err, &via_cwd), "error must name the cwd/--root repository: {err}" ); assert!( @@ -637,12 +696,9 @@ mod tests { } .resolve() .expect_err("two stores must not first-wins"); + assert!(names(&err, &first), "error must name the first root: {err}"); assert!( - err.contains(&first.display().to_string()), - "error must name the first root: {err}" - ); - assert!( - err.contains(&second.display().to_string()), + names(&err, &second), "error must name the second root: {err}" ); assert!( @@ -653,6 +709,7 @@ mod tests { ); } + #[cfg(not(windows))] #[test] fn windows_file_uri_is_not_reinterpreted_as_a_unix_path() { assert!(file_uri_to_path("file:///C:/Users/nobody/project").is_none()); @@ -677,4 +734,38 @@ mod tests { "a single-slash file: URI is not a unix path" ); } + + #[cfg(windows)] + #[test] + fn a_unix_file_uri_is_not_reinterpreted_on_windows() { + assert_eq!( + file_uri_to_path("file:///C:/Users/example/project").unwrap(), + PathBuf::from(r"C:\Users\example\project") + ); + assert_eq!( + file_uri_to_path("file://localhost/D:/src/a%20b").unwrap(), + PathBuf::from(r"D:\src\a b") + ); + assert!( + file_uri_to_path("file:///Users/example/project").is_none(), + "a drive-less URI must not resolve against the current drive" + ); + assert!(file_uri_to_path("file://example.com/C:/x").is_none()); + assert!(file_uri_to_path("file:///C:/tmp/foo%00bar").is_none()); + assert!(file_uri_to_path("https://example.com/repo").is_none()); + } + + #[test] + fn repo_path_is_refused_in_the_other_platform_s_spelling() { + #[cfg(not(windows))] + let (foreign, marker) = (r"C:\Users\nobody\project", "Windows path"); + #[cfg(windows)] + let (foreign, marker) = ("/Users/nobody/project", "unix path"); + let err = validate_repo_path(foreign).expect_err("a foreign path must be refused"); + assert!(err.contains(marker), "{err}"); + // And a real directory in this host's own spelling opens. + let here = scratch("native-spelling"); + let opened = validate_repo_path(here.to_str().unwrap()).expect("native path opens"); + assert_eq!(opened, here.canonicalize().unwrap()); + } } diff --git a/rust/devmap-serve/tests/mcp_repo_scope.rs b/rust/devmap-serve/tests/mcp_repo_scope.rs index 63a4cda1..e8ad4af0 100644 --- a/rust/devmap-serve/tests/mcp_repo_scope.rs +++ b/rust/devmap-serve/tests/mcp_repo_scope.rs @@ -15,6 +15,34 @@ use devmap_serve::mcp::{ use devmap_store::Store; use serde_json::{json, Value}; +/// Whether `text` names `path` in a spelling the platform gives it: as +/// written, or canonical. Windows canonicalises to a `\\?\` verbatim path +/// with long names where the temp dir used 8.3 short ones (`RUNNER~1`), and +/// macOS to `/private/var` for `/var`; every one is the same directory. +fn names(text: &str, path: &std::path::Path) -> bool { + if text.contains(&path.display().to_string()) { + return true; + } + let Ok(canonical) = path.canonicalize() else { + return false; + }; + let shown = canonical.display().to_string(); + text.contains(shown.strip_prefix(r"\\?\").unwrap_or(&shown)) +} + +/// `path` as a `file://` URI in this platform's form: `file:///C:/…` on +/// Windows. `format!("file://{}", path.display())` gives `file://C:\…` there, +/// which is not a file URI at all. +fn file_uri(path: &std::path::Path) -> String { + let shown = path.display().to_string(); + if cfg!(windows) { + let bare = shown.strip_prefix(r"\\?\").unwrap_or(&shown); + format!("file:///{}", bare.replace('\\', "/")) + } else { + format!("file://{shown}") + } +} + fn scratch(name: &str) -> PathBuf { let root = std::env::temp_dir().join(format!( "devmap-mcp-scope-{name}-{}-{}", @@ -333,8 +361,7 @@ async fn cwd_store_and_different_mcp_root_without_repo_path_is_refused() { let response = call(&slot, "devmap_status", json!({})).await; let text = tool_error_text(&response); assert!( - text.contains(&via_roots.display().to_string()) - && text.contains(&via_cwd.display().to_string()), + names(&text, &via_roots) && names(&text, &via_cwd), "must name both the MCP root and the cwd/--root repository: {text}" ); @@ -447,7 +474,7 @@ async fn preview_without_repo_path_and_two_candidate_roots_is_refused() { .await; let text = tool_error_text(&response); assert!( - text.contains(&a.display().to_string()) && text.contains(&b.display().to_string()), + names(&text, &a) && names(&text, &b), "preview must name both candidate roots, not judge the first: {text}" ); } @@ -659,8 +686,7 @@ async fn mixed_calls_cannot_first_wins_across_cwd_and_mcp_root() { 0 => { let text = tool_error_text(&call(&slot, "devmap_status", json!({})).await); assert!( - text.contains(&via_roots.display().to_string()) - && text.contains(&via_cwd.display().to_string()), + names(&text, &via_roots) && names(&text, &via_cwd), "iteration {i}: empty args must stay ambiguous: {text}" ); } @@ -809,12 +835,12 @@ async fn repo_path_against_a_foreign_root_pin_is_a_tool_error_naming_both() { .await; let text = tool_error_text(&response); assert!( - text.contains(&pinned.display().to_string()) + names(&text, &pinned) || text.contains(&pinned.canonicalize().unwrap().display().to_string()), "error must name the --root pin: {text}" ); assert!( - text.contains(&foreign.display().to_string()) + names(&text, &foreign) || text.contains(&foreign.canonicalize().unwrap().display().to_string()), "error must name the refused repo_path: {text}" ); @@ -832,7 +858,7 @@ async fn late_devmap_roots_reply_is_ignored_after_a_newer_seq() { "jsonrpc": "2.0", "id": "devmap-roots-2", "result": { - "roots": [{"uri": format!("file://{}", newer.display())}] + "roots": [{"uri": file_uri(&newer)}] } }); assert!( @@ -844,7 +870,7 @@ async fn late_devmap_roots_reply_is_ignored_after_a_newer_seq() { "jsonrpc": "2.0", "id": "devmap-roots-1", "result": { - "roots": [{"uri": format!("file://{}", older.display())}] + "roots": [{"uri": file_uri(&older)}] } }); assert!(handle_line(&slot, &apply_older.to_string()).await.is_none()); diff --git a/rust/devmap-serve/tests/phase1_freshness_reach.rs b/rust/devmap-serve/tests/phase1_freshness_reach.rs index 51e2ceaa..3928b443 100644 --- a/rust/devmap-serve/tests/phase1_freshness_reach.rs +++ b/rust/devmap-serve/tests/phase1_freshness_reach.rs @@ -12,6 +12,34 @@ use devmap_serve::RootResolveInput; use devmap_store::Store; use serde_json::json; +/// Whether `text` names `path` in a spelling the platform gives it: as +/// written, or canonical. Windows canonicalises to a `\\?\` verbatim path +/// with long names where the temp dir used 8.3 short ones (`RUNNER~1`), and +/// macOS to `/private/var` for `/var`; every one is the same directory. +fn names(text: &str, path: &std::path::Path) -> bool { + if text.contains(&path.display().to_string()) { + return true; + } + let Ok(canonical) = path.canonicalize() else { + return false; + }; + let shown = canonical.display().to_string(); + text.contains(shown.strip_prefix(r"\\?\").unwrap_or(&shown)) +} + +/// `path` as a `file://` URI in this platform's form: `file:///C:/…` on +/// Windows. `format!("file://{}", path.display())` gives `file://C:\…` there, +/// which is not a file URI at all. +fn file_uri(path: &std::path::Path) -> String { + let shown = path.display().to_string(); + if cfg!(windows) { + let bare = shown.strip_prefix(r"\\?\").unwrap_or(&shown); + format!("file:///{}", bare.replace('\\', "/")) + } else { + format!("file://{shown}") + } +} + fn scratch(name: &str) -> PathBuf { let root = std::env::temp_dir().join(format!( "devmap-p1-{name}-{}-{}", @@ -99,7 +127,7 @@ async fn mcp_roots_list_does_not_silently_replace_a_cwd_store() { "jsonrpc": "2.0", "id": ROOTS_LIST_REQUEST_ID, "result": { - "roots": [{"uri": format!("file://{}", repo_b.display())}] + "roots": [{"uri": file_uri(&repo_b)}] } }); assert!(handle_line(&slot, &response.to_string()).await.is_none()); @@ -108,7 +136,7 @@ async fn mcp_roots_list_does_not_silently_replace_a_cwd_store() { Err(err) => err, }; assert!( - err.contains(&repo_a.display().to_string()) && err.contains(&repo_b.display().to_string()), + names(&err, &repo_a) && names(&err, &repo_b), "must name both repositories: {err}" ); } @@ -124,7 +152,7 @@ async fn mcp_roots_list_opens_when_cwd_has_no_store() { "jsonrpc": "2.0", "id": ROOTS_LIST_REQUEST_ID, "result": { - "roots": [{"uri": format!("file://{}", repo.display())}] + "roots": [{"uri": file_uri(&repo)}] } }); assert!(handle_line(&slot, &response.to_string()).await.is_none()); @@ -192,7 +220,19 @@ async fn concurrent_status_while_store_deleted_mid_session_fails_closed() { let store_path = plant_store(&root); let slot = Arc::new(StoreSlot::resolving(None, root.clone(), None)); let _ = slot.get().expect("open"); - std::fs::remove_file(&store_path).unwrap(); + if let Err(error) = std::fs::remove_file(&store_path) { + // Windows refuses to delete a file an open handle holds without + // FILE_SHARE_DELETE (os error 32), so the mid-session deletion this + // test exercises cannot happen there: the store stays whole and keeps + // serving. Anything else is a real failure. + assert!( + cfg!(windows) && error.raw_os_error() == Some(32), + "removing the store failed: {error}" + ); + assert!(store_path.is_file(), "a refused delete leaves the store"); + slot.get().expect("the store the delete could not touch still serves"); + return; + } let err = match slot.get() { Ok(_) => panic!("deleted store must not reopen"), Err(err) => err, diff --git a/rust/devmap-store/src/db.rs b/rust/devmap-store/src/db.rs index 9eaf536b..432f24dc 100644 --- a/rust/devmap-store/src/db.rs +++ b/rust/devmap-store/src/db.rs @@ -3224,8 +3224,45 @@ impl Store { }) } + /// How long [`Store::open`] keeps retrying `SQLITE_PROTOCOL`. + /// + /// SQLite returns it when a WAL-index lock race outlasts its own internal + /// retries, which concurrent openers of one store can provoke; it is a lost + /// race, not a damaged file, and the docs' remedy is to try again. On + /// Windows eight threads opening one legacy store all lost it + /// (`concurrent_openers_of_a_legacy_store_all_reach_the_current_schema`). + /// Opening is safe to repeat from the top: `migrate` re-reads the schema + /// version under the write lock, so a step another opener finished is not + /// run twice. + const PROTOCOL_RETRY_DEADLINE: std::time::Duration = std::time::Duration::from_secs(20); + + fn lost_a_locking_race(error: &rusqlite::Error) -> bool { + matches!( + error, + rusqlite::Error::SqliteFailure(failure, _) + if failure.code == rusqlite::ErrorCode::FileLockingProtocolFailed + ) + } + pub fn open>(db_path: P) -> Result { - let resolved = devmap_extract::safe_fs::resolve_file_alias(db_path.as_ref()) + let deadline = std::time::Instant::now() + Self::PROTOCOL_RETRY_DEADLINE; + let mut pause = std::time::Duration::from_millis(10); + loop { + match Self::open_once(db_path.as_ref()) { + Err(error) + if Self::lost_a_locking_race(&error) + && std::time::Instant::now() + pause < deadline => + { + std::thread::sleep(pause); + pause = (pause * 2).min(std::time::Duration::from_millis(500)); + } + outcome => return outcome, + } + } + } + + fn open_once(db_path: &Path) -> Result { + let resolved = devmap_extract::safe_fs::resolve_file_alias(db_path) .map_err(|error| refusal(error.to_string()))?; let path = resolved.as_path(); Self::validate_database_file(path)?; diff --git a/rust/devmap-store/tests/kernel_defects.rs b/rust/devmap-store/tests/kernel_defects.rs index 7ba41771..b1c59d04 100644 --- a/rust/devmap-store/tests/kernel_defects.rs +++ b/rust/devmap-store/tests/kernel_defects.rs @@ -27,6 +27,19 @@ fn tmp_dir(label: &str) -> PathBuf { dir } +/// Whether `text` names `path` as written or canonical (a `\\?\` verbatim +/// path with long names on Windows, `/private/var` on macOS). +fn names(text: &str, path: &std::path::Path) -> bool { + if text.contains(&path.display().to_string()) { + return true; + } + let Ok(canonical) = path.canonicalize() else { + return false; + }; + let shown = canonical.display().to_string(); + text.contains(shown.strip_prefix(r"\\?\").unwrap_or(&shown)) +} + /// K3: a store this binary cannot open must say what to do about it. /// /// The refusal was `unsupported future schema version 99` — no store path, no @@ -48,7 +61,7 @@ fn k3_future_schema_refusal_names_the_store_the_versions_and_the_remedy() { .to_string(); assert!( - error.contains(&db_path.display().to_string()), + names(&error, &db_path), "the refusal must name the store it refused: {error}" ); assert!( @@ -90,7 +103,7 @@ fn k3_outdated_schema_refusal_names_the_store_the_versions_and_the_remedy() { .to_string(); assert!( - error.contains(&db_path.display().to_string()), + names(&error, &db_path), "the refusal must name the store it refused: {error}" ); assert!( From 58760dcb11248e7e338693bd7d8e67a08c6f38f4 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 28 Sep 2026 21:43:51 +0000 Subject: [PATCH 16/20] fix(windows,fmt): the last three Windows tests; format phase1_freshness_reach 38c4d29 took the Windows leg from 28 failures to 3, all in its own test changes, and one file it edited after formatting failed Format on every OS (verify.sh gates too, which runs the same check): - pinned_root_disables_discovery: the refusal names the pinned root canonically (`\\?\C:\...`), and the assertion compared the raw spelling; it uses `names`, like the other path assertions. - parse_roots_list_caps_at_max_and_reports_overflow built drive-less `file:///tmp/...` URIs, which a Windows server now correctly refuses, so there was nothing to cap; it builds them in the host's spelling. - windows_file_uri_is_rejected_not_reinterpreted asserts the unix rule and is unix-only; its Windows twin asserts that `file:///Users/...` does not become a root there. cargo fmt --all --check clean; clippy -D warnings clean for linux and x86_64-pc-windows-gnu; mcp_repo_scope 24/24 and phase1_freshness_reach 9/9 on linux (hook_conformance's pinned-root test needs a non-root user, as before). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01FUNo5WZs78gWyCSXf2nRhm (cherry picked from commit 241b018e744c5d587ff465d39ca09ab958657adc) --- rust/devmap-cli/tests/hook_conformance.rs | 15 +++++++++- rust/devmap-serve/tests/mcp_repo_scope.rs | 30 ++++++++++++++++++- .../tests/phase1_freshness_reach.rs | 3 +- 3 files changed, 45 insertions(+), 3 deletions(-) diff --git a/rust/devmap-cli/tests/hook_conformance.rs b/rust/devmap-cli/tests/hook_conformance.rs index b9ccdbd8..e699972c 100644 --- a/rust/devmap-cli/tests/hook_conformance.rs +++ b/rust/devmap-cli/tests/hook_conformance.rs @@ -11,6 +11,19 @@ use serde_json::json; const DEVMAP: &str = env!("CARGO_BIN_EXE_devmap"); +/// Whether `text` names `path` as written or canonical (a `\\?\` verbatim +/// path with long names on Windows, `/private/var` on macOS). +fn names(text: &str, path: &Path) -> bool { + if text.contains(&path.display().to_string()) { + return true; + } + let Ok(canonical) = path.canonicalize() else { + return false; + }; + let shown = canonical.display().to_string(); + text.contains(shown.strip_prefix(r"\\?\").unwrap_or(&shown)) +} + fn scratch(tag: &str) -> PathBuf { let dir = std::env::temp_dir().join(format!( "devmap-hook-conf-{tag}-{}-{}", @@ -619,7 +632,7 @@ fn pinned_root_disables_discovery() { "the pinned repository was never acted on, so the pin proves nothing" ); assert!( - run.stderr.contains(pinned.to_str().unwrap()), + names(&run.stderr, &pinned), "the failure must name the pinned root, not another repository: {}", run.stderr ); diff --git a/rust/devmap-serve/tests/mcp_repo_scope.rs b/rust/devmap-serve/tests/mcp_repo_scope.rs index e8ad4af0..3056423a 100644 --- a/rust/devmap-serve/tests/mcp_repo_scope.rs +++ b/rust/devmap-serve/tests/mcp_repo_scope.rs @@ -619,6 +619,7 @@ async fn two_spellings_of_one_case_insensitive_path_open_one_canonical_root() { ); } +#[cfg(not(windows))] #[tokio::test] async fn windows_file_uri_is_rejected_not_reinterpreted() { let unix = scratch("uri-unix"); @@ -919,7 +920,9 @@ async fn roots_list_error_keeps_the_previous_good_list() { #[test] fn parse_roots_list_caps_at_max_and_reports_overflow() { let roots: Vec = (0..200) - .map(|i| json!({"uri": format!("file:///tmp/devmap-overflow-{i}")})) + // In the host's own spelling: a Windows server refuses a drive-less + // `file:///tmp/...` as a unix path, which left nothing to cap. + .map(|i| json!({"uri": file_uri(&std::env::temp_dir().join(format!("devmap-overflow-{i}")))})) .collect(); let (paths, _skipped, overflow) = devmap_serve::mcp::parse_roots_list_result(&json!({"roots": roots})); @@ -930,3 +933,28 @@ fn parse_roots_list_caps_at_max_and_reports_overflow() { ); assert_eq!(overflow, 200 - devmap_serve::root_resolve::MAX_ROOTS); } + +/// The same rule from the other side: a Windows server does not resolve a +/// drive-less unix path against its current drive. +#[cfg(windows)] +#[tokio::test] +async fn unix_file_uri_is_rejected_not_reinterpreted_on_windows() { + let native = scratch("uri-native"); + plant_store(&native); + let slot = Arc::new(StoreSlot::resolving(None, native.clone(), None)); + let response = json!({ + "jsonrpc": "2.0", + "id": ROOTS_LIST_REQUEST_ID, + "result": { + "roots": [{"uri": "file:///Users/nobody/project"}] + } + }); + assert!(handle_line(&slot, &response.to_string()).await.is_none()); + assert!( + !slot + .candidate_roots() + .iter() + .any(|root| root.to_string_lossy().contains("nobody")), + "a unix URI must not become a root on Windows" + ); +} diff --git a/rust/devmap-serve/tests/phase1_freshness_reach.rs b/rust/devmap-serve/tests/phase1_freshness_reach.rs index 3928b443..eb5e79d0 100644 --- a/rust/devmap-serve/tests/phase1_freshness_reach.rs +++ b/rust/devmap-serve/tests/phase1_freshness_reach.rs @@ -230,7 +230,8 @@ async fn concurrent_status_while_store_deleted_mid_session_fails_closed() { "removing the store failed: {error}" ); assert!(store_path.is_file(), "a refused delete leaves the store"); - slot.get().expect("the store the delete could not touch still serves"); + slot.get() + .expect("the store the delete could not touch still serves"); return; } let err = match slot.get() { From 485a2a6ab4cbd5dfc71433f82afa72095543fdf8 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 28 Sep 2026 22:40:06 +0000 Subject: [PATCH 17/20] test(gate): wait for backgrounded writes before snapshotting the tree runInThrowawayTree ran `sh -c ` and snapshotted the tree as soon as sh exited. For a corpus entry that backgrounds its write (`echo hi > .env &`), sh exits first: the write landed after the snapshot, so the test recorded `would have written []` instead of `.env`, and on macOS it raced t.TempDir's RemoveAll, failing TestCommandVerdictIsNeverLooserThanTheWritesItPerforms with "directory not empty". Appending `wait` on its own line makes sh reap the job first. main never reached this on macOS: its clients job stopped at gofmt. Passes 30/30 with -count=30; the backgrounded entry now records [.env]. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01WQJD75wFPY52ckToQTu9FS --- backend/go_orchestrator/gate/redirect_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/backend/go_orchestrator/gate/redirect_test.go b/backend/go_orchestrator/gate/redirect_test.go index 6b62f4bc..7ad9d78e 100644 --- a/backend/go_orchestrator/gate/redirect_test.go +++ b/backend/go_orchestrator/gate/redirect_test.go @@ -232,7 +232,10 @@ func runInThrowawayTree(t *testing.T, command string) []string { t.Fatal(err) } before := treeSnapshot(root) - cmd := exec.Command("sh", "-c", command) + // `wait` on its own line: an entry that backgrounds its write (`... &`) + // otherwise lets sh exit first, so the write lands after the snapshot + // (unobserved) and races t.TempDir's cleanup ("directory not empty"). + cmd := exec.Command("sh", "-c", command+"\nwait") cmd.Dir = root // A corpus entry that hangs would wedge the suite; nothing here reads // stdin, and closing it turns any that starts to into an immediate EOF. From f9083a7c40b19292972352b09f911328c8c086db Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 28 Sep 2026 22:30:17 +0000 Subject: [PATCH 18/20] fix(windows): retry lost SQLite races on read-only opens too, and whole atomic writes; test(gate): wait for backgrounded writes The last two Windows failures at 3b58dd7, and a macOS flake at 241b018: - concurrent_readers_cannot_enqueue_writer_work: eight threads opening one store read-only lost the WAL-index race (SQLITE_PROTOCOL) as the migrators had; only Store::open retried it. Both opens now go through one retrying_lost_races helper. - concurrent_emission_leaves_one_valid_bundle: os error 32 still escaped write_atomic, from a step outside the comparison and the rename (a different bundle file each run). Every step of write_atomic is safe to repeat from the top, so on Windows the whole write retries a sharing violation within the same 5 s deadline. - gate: runInThrowawayTree ran `sh -c command` and read the tree when sh exited, so `echo hi > .env &` was recorded as writing nothing and its write landed during t.TempDir cleanup ("directory not empty", macOS). The shell now waits for its background jobs. If the gate had allowed that command, the old harness would have missed its write. TestThrowawayTreeSeesABackgroundedWrite fails without the wait (run 4: saw [], want [.env]). fmt --check clean; clippy -D warnings clean for linux and windows-gnu; migration_ladder 12/12, artifacts 7/7, gate passes. embedded_reader's a_filesystem_read_only_open... fails here identically without this change (root ignores the read-only fixture); CI runs it as a normal user. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01FUNo5WZs78gWyCSXf2nRhm (cherry picked from commit ebd2a8f1ceb08922ef1aec6ee1122ef3bc6fd934) --- backend/go_orchestrator/gate/redirect_test.go | 19 +++++++++++--- rust/devmap-query/src/artifacts.rs | 26 +++++++++++++++++++ rust/devmap-store/src/db.rs | 18 ++++++++++--- 3 files changed, 57 insertions(+), 6 deletions(-) diff --git a/backend/go_orchestrator/gate/redirect_test.go b/backend/go_orchestrator/gate/redirect_test.go index 7ad9d78e..7749615d 100644 --- a/backend/go_orchestrator/gate/redirect_test.go +++ b/backend/go_orchestrator/gate/redirect_test.go @@ -232,9 +232,11 @@ func runInThrowawayTree(t *testing.T, command string) []string { t.Fatal(err) } before := treeSnapshot(root) - // `wait` on its own line: an entry that backgrounds its write (`... &`) - // otherwise lets sh exit first, so the write lands after the snapshot - // (unobserved) and races t.TempDir's cleanup ("directory not empty"). + // `wait` so a backgrounded write (`echo hi > .env &`) lands before the + // tree is read. Without it sh exited first, the write was reported as + // nothing, and it landed during t.TempDir's cleanup, which then failed + // with "directory not empty" (macOS, 241b018). Every corpus entry is a + // complete command, so a trailing line cannot change how it parses. cmd := exec.Command("sh", "-c", command+"\nwait") cmd.Dir = root // A corpus entry that hangs would wedge the suite; nothing here reads @@ -253,6 +255,17 @@ func runInThrowawayTree(t *testing.T, command string) []string { return changed } +// A backgrounded write is a write. The corpus holds `echo hi > .env &`, and +// the invariant above only means something for it if its write is seen. +func TestThrowawayTreeSeesABackgroundedWrite(t *testing.T) { + for i := 0; i < 20; i++ { + written := runInThrowawayTree(t, "sleep 0.05; echo hi > .env &") + if len(written) != 1 || written[0] != ".env" { + t.Fatalf("run %d: saw %v, want [.env]", i, written) + } + } +} + func treeSnapshot(root string) map[string]string { out := map[string]string{} _ = filepath.Walk(root, func(p string, info os.FileInfo, err error) error { diff --git a/rust/devmap-query/src/artifacts.rs b/rust/devmap-query/src/artifacts.rs index dbdc1bf1..8c6db863 100644 --- a/rust/devmap-query/src/artifacts.rs +++ b/rust/devmap-query/src/artifacts.rs @@ -39,6 +39,32 @@ fn is_sharing_violation(error: &std::io::Error) -> bool { } pub fn write_atomic(path: &Path, content: &[u8]) -> std::io::Result { + // Every step below is safe to repeat from the top: the comparison reads, + // the temporary name is new each time, and the rename is the only write. + // On Windows a sharing violation can surface from any of them while + // another process holds the same file or directory for a moment, and + // retrying only the rename and the comparison still let concurrent + // `devmap claude` runs fail with os error 32; the whole write waits it + // out instead, within the same deadline. + #[cfg(windows)] + { + let deadline = std::time::Instant::now() + WINDOWS_SHARING_DEADLINE; + loop { + match write_atomic_once(path, content) { + Err(error) + if is_sharing_violation(&error) && std::time::Instant::now() < deadline => + { + std::thread::sleep(std::time::Duration::from_millis(10)); + } + outcome => return outcome, + } + } + } + #[cfg(not(windows))] + write_atomic_once(path, content) +} + +fn write_atomic_once(path: &Path, content: &[u8]) -> std::io::Result { use devmap_extract::safe_fs::{Access, Creation, PinnedDir}; devmap_extract::safe_fs::preflight_write(path)?; let parent = PinnedDir::open(path.parent().unwrap_or(Path::new(".")), true)?; diff --git a/rust/devmap-store/src/db.rs b/rust/devmap-store/src/db.rs index 432f24dc..8be60031 100644 --- a/rust/devmap-store/src/db.rs +++ b/rust/devmap-store/src/db.rs @@ -3174,7 +3174,11 @@ impl Store { /// even when the application has write access to the file. A writer must /// upgrade an older store explicitly before an advisory reader can use it. pub fn open_read_only>(db_path: P) -> Result { - let resolved = devmap_extract::safe_fs::resolve_file_alias(db_path.as_ref()) + Self::retrying_lost_races(|| Self::open_read_only_once(db_path.as_ref())) + } + + fn open_read_only_once(db_path: &Path) -> Result { + let resolved = devmap_extract::safe_fs::resolve_file_alias(db_path) .map_err(|error| refusal(error.to_string()))?; let path = resolved.as_path(); Self::validate_database_file(path)?; @@ -3244,11 +3248,15 @@ impl Store { ) } - pub fn open>(db_path: P) -> Result { + /// Run an open, retrying `SQLITE_PROTOCOL` within + /// [`Store::PROTOCOL_RETRY_DEADLINE`]. Shared by every entry point that + /// opens a connection: read-only opens lost the same race on Windows + /// (`concurrent_readers_cannot_enqueue_writer_work`). + fn retrying_lost_races(open: impl Fn() -> Result) -> Result { let deadline = std::time::Instant::now() + Self::PROTOCOL_RETRY_DEADLINE; let mut pause = std::time::Duration::from_millis(10); loop { - match Self::open_once(db_path.as_ref()) { + match open() { Err(error) if Self::lost_a_locking_race(&error) && std::time::Instant::now() + pause < deadline => @@ -3261,6 +3269,10 @@ impl Store { } } + pub fn open>(db_path: P) -> Result { + Self::retrying_lost_races(|| Self::open_once(db_path.as_ref())) + } + fn open_once(db_path: &Path) -> Result { let resolved = devmap_extract::safe_fs::resolve_file_alias(db_path) .map_err(|error| refusal(error.to_string()))?; From 975a79b40c4bd0cd7025641442323e976ffdc610 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 28 Sep 2026 23:28:16 +0000 Subject: [PATCH 19/20] fix(store,windows): serialise the setup of read-only connections within a process concurrent_readers_cannot_enqueue_writer_work still failed on Windows at ebd2a8f: eight threads opening one store read-only all got SQLITE_PROTOCOL for the whole 20 s retry deadline. Retrying cannot help. Reproduced locally under Wine with a windows-gnu build: a single reader opens fine and creates the -wal/-shm pair, the storm is intermittent and leaves the file healthy, and even plain opens with no further reads fail. It is contention between read-only connections in one process setting up the WAL index together, with no read-write connection there to finish it. A read-only connection now opens and makes its first read, the moment it maps the WAL index, under a process-wide lock (READ_ONLY_SETUP); reads after that run concurrently. Under Wine the test fails 3/3 without the lock (21-33 s) and passes 4/4 with it (about 2.7 s); the whole embedded_reader suite passes there, 11/11. Linux: migration_ladder 12/12, embedded_reader unchanged (its read-only-fixture test needs a non-root user). fmt and clippy clean for linux and windows-gnu. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01FUNo5WZs78gWyCSXf2nRhm (cherry picked from commit 9248cc6327f1af394f0b8c8ccdde37a48deeb2c0) --- rust/devmap-store/src/db.rs | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/rust/devmap-store/src/db.rs b/rust/devmap-store/src/db.rs index 8be60031..4ebf9b77 100644 --- a/rust/devmap-store/src/db.rs +++ b/rust/devmap-store/src/db.rs @@ -32,6 +32,15 @@ impl std::fmt::Display for StoreRefusal { impl std::error::Error for StoreRefusal {} +/// Held while a read-only connection opens and makes its first read, the +/// moment it maps the WAL index. Concurrent read-only connections in one +/// process setting that up together livelocked on Windows: every one of them +/// got SQLITE_PROTOCOL for as long as they kept retrying, and no read-write +/// connection was there to finish the index for them +/// (`concurrent_readers_cannot_enqueue_writer_work`). Serialised, the first +/// sets it up and the rest find it ready. Reads after that run concurrently. +static READ_ONLY_SETUP: std::sync::Mutex<()> = std::sync::Mutex::new(()); + fn refusal(message: impl Into) -> rusqlite::Error { rusqlite::Error::ToSqlConversionFailure(Box::new(StoreRefusal(message.into()))) } @@ -3195,12 +3204,17 @@ impl Store { ))); } let _sidecars = Self::checked_sidecars(path)?; + let setup = READ_ONLY_SETUP + .lock() + .unwrap_or_else(|poison| poison.into_inner()); let mut conn = Connection::open_with_flags( path, rusqlite::OpenFlags::SQLITE_OPEN_READ_ONLY | rusqlite::OpenFlags::SQLITE_OPEN_NO_MUTEX, )?; conn.busy_timeout(Self::BUSY_TIMEOUT)?; - let stamped: i32 = match conn.query_row("PRAGMA user_version", [], |row| row.get(0)) { + let first_read = conn.query_row("PRAGMA user_version", [], |row| row.get(0)); + drop(setup); + let stamped: i32 = match first_read { Ok(version) => version, Err(error) if path.is_file() && Self::directory_refused_the_wal(&error) => { conn = Self::open_immutable(path)?; From 4efeecfdbc05c556027c8f284b1055c9baa24bda Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 29 Sep 2026 00:09:06 +0000 Subject: [PATCH 20/20] fix(store,windows): serialise read-write opens within a process too At 975a79b the Windows leg failed concurrent_v19_openers_keep_pending_work_and_one_durable_identity: sixteen threads calling Store::open on one v19 store all got SQLITE_PROTOCOL for the whole 20 s retry deadline (the test ran 19.5 s). It is the livelock 9248cc6 fixed for read-only connections, between read-write ones: connections in one process setting the WAL index up together, each losing to the others, with retrying unable to help. The process-wide lock (renamed READ_ONLY_SETUP -> CONNECTION_SETUP, since it now covers both) is also held across open_once, from Connection::open through enable_wal and migrate. Migration was already serialised by SQLite's write lock, so this costs in-process openers nothing they were not already waiting for. Other processes still coordinate through SQLite, and everything after the open runs concurrently. Nothing inside an open opens another store, so the lock cannot be re-entered. Linux: fmt and clippy -D warnings clean; every devmap-store test binary passes as an unprivileged user (migration_ladder 12/12, embedded_reader 14/14, store_hardening 53/53, ...); the two concurrent-open tests pass 10/10 repeated; all 144 devmap-serve, devmap-query and devmap-cli test binaries pass, so the wider lock deadlocks nothing there. Not reproduced on Windows here; the next Windows leg is the check. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01WQJD75wFPY52ckToQTu9FS --- rust/devmap-store/src/db.rs | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/rust/devmap-store/src/db.rs b/rust/devmap-store/src/db.rs index 4ebf9b77..a6feb8c3 100644 --- a/rust/devmap-store/src/db.rs +++ b/rust/devmap-store/src/db.rs @@ -32,14 +32,18 @@ impl std::fmt::Display for StoreRefusal { impl std::error::Error for StoreRefusal {} -/// Held while a read-only connection opens and makes its first read, the -/// moment it maps the WAL index. Concurrent read-only connections in one -/// process setting that up together livelocked on Windows: every one of them -/// got SQLITE_PROTOCOL for as long as they kept retrying, and no read-write -/// connection was there to finish the index for them -/// (`concurrent_readers_cannot_enqueue_writer_work`). Serialised, the first -/// sets it up and the rest find it ready. Reads after that run concurrently. -static READ_ONLY_SETUP: std::sync::Mutex<()> = std::sync::Mutex::new(()); +/// Held while a connection opens and sets itself up: for a read-only +/// connection, its open and first read, the moment it maps the WAL index; for +/// a read-write one, the whole of `open_once`, through `enable_wal` and +/// `migrate`. Concurrent connections in one process setting the WAL index up +/// together livelocked on Windows: every one of them got SQLITE_PROTOCOL for +/// as long as they kept retrying (`concurrent_readers_cannot_enqueue_writer_work` +/// for readers, `concurrent_v19_openers_keep_pending_work_and_one_durable_identity` +/// for sixteen writers, which exhausted the 20 s retry deadline). Serialised, +/// the first sets it up and the rest find it ready. Other processes still +/// coordinate through SQLite's own locks, and everything after the open runs +/// concurrently. +static CONNECTION_SETUP: std::sync::Mutex<()> = std::sync::Mutex::new(()); fn refusal(message: impl Into) -> rusqlite::Error { rusqlite::Error::ToSqlConversionFailure(Box::new(StoreRefusal(message.into()))) @@ -3204,7 +3208,7 @@ impl Store { ))); } let _sidecars = Self::checked_sidecars(path)?; - let setup = READ_ONLY_SETUP + let setup = CONNECTION_SETUP .lock() .unwrap_or_else(|poison| poison.into_inner()); let mut conn = Connection::open_with_flags( @@ -3297,6 +3301,10 @@ impl Store { // repair after `Connection::open` is a repair the connection never // sees. See `repair_sidecar_modes`. Self::repair_sidecar_modes(path)?; + // Held to the end of this open; see `CONNECTION_SETUP`. + let _setup = CONNECTION_SETUP + .lock() + .unwrap_or_else(|poison| poison.into_inner()); let mut conn = Connection::open(path)?; let store = path.display().to_string();