From a40500eea9bed913f2da7abae2cd2b2ccee27da0 Mon Sep 17 00:00:00 2001 From: dave Date: Sun, 17 May 2026 00:28:48 +0000 Subject: [PATCH] huskies: merge 1111 bug Test isolation: `init_for_test()` and `ensure_content_store()` are once-per-thread, not once-per-test, polluting CRDT state across tests --- server/src/crdt_state/state/mod.rs | 76 ++++++++++--------- server/src/db/content_store.rs | 45 ++++++++++- server/src/db/ops.rs | 6 ++ server/src/http/mcp/story_tools/bug.rs | 1 + server/src/http/mcp/story_tools/refactor.rs | 2 + server/src/http/mcp/story_tools/spike.rs | 4 + .../src/http/mcp/story_tools/story/create.rs | 4 + .../src/http/mcp/story_tools/story/query.rs | 1 + server/src/http/workflow/bug_ops/tests.rs | 8 ++ 9 files changed, 111 insertions(+), 36 deletions(-) diff --git a/server/src/crdt_state/state/mod.rs b/server/src/crdt_state/state/mod.rs index 2de501e6..b00f89eb 100644 --- a/server/src/crdt_state/state/mod.rs +++ b/server/src/crdt_state/state/mod.rs @@ -122,49 +122,57 @@ pub(super) fn get_crdt() -> Option<&'static Mutex> { /// This avoids the async SQLite setup from `init()`. Ops are sent to a /// channel whose receiver is leaked (so nothing is persisted, but the channel /// stays open and `apply_and_persist` succeeds silently). -/// Safe to call multiple times — subsequent calls are no-ops (thread-local). +/// Always resets all thread-local state so each call produces a clean slate — +/// no cross-test pollution when two tests share the same thread. #[cfg(test)] pub fn init_for_test() { - // Initialise thread-local CRDT for test isolation. - // Only creates a new CRDT if one isn't set yet on this thread; - // subsequent calls are no-ops (matching the old OnceLock semantics - // while keeping each thread isolated). + let keypair = make_keypair(); + let crdt = BaseCrdt::::new(&keypair); + let (persist_tx, rx) = mpsc::unbounded_channel(); + // Leak the receiver so the channel stays open: apply_and_persist + // can then send without error, preventing [crdt_persist] WARNs + // from racing with other tests that watch the global log buffer. + std::mem::forget(rx); + let fresh = CrdtState { + crdt, + keypair, + index: HashMap::new(), + node_index: HashMap::new(), + token_index: HashMap::new(), + merge_job_index: HashMap::new(), + active_agent_index: HashMap::new(), + test_job_index: HashMap::new(), + agent_throttle_index: HashMap::new(), + gateway_project_index: HashMap::new(), + persist_tx, + lamport_floor: 0, + tombstones: HashSet::new(), + }; CRDT_STATE_TL.with(|lock| { - if lock.get().is_none() { - let keypair = make_keypair(); - let crdt = BaseCrdt::::new(&keypair); - let (persist_tx, rx) = mpsc::unbounded_channel(); - // Leak the receiver so the channel stays open: apply_and_persist - // can then send without error, preventing [crdt_persist] WARNs - // from racing with other tests that watch the global log buffer. - std::mem::forget(rx); - let state = CrdtState { - crdt, - keypair, - index: HashMap::new(), - node_index: HashMap::new(), - token_index: HashMap::new(), - merge_job_index: HashMap::new(), - active_agent_index: HashMap::new(), - test_job_index: HashMap::new(), - agent_throttle_index: HashMap::new(), - gateway_project_index: HashMap::new(), - persist_tx, - lamport_floor: 0, - tombstones: HashSet::new(), - }; - let _ = lock.set(Mutex::new(state)); + if let Some(mutex) = lock.get() { + // Already set on this thread — replace contents so the second + // (and subsequent) test on the same thread starts clean. + *mutex.lock().unwrap() = fresh; + } else { + let _ = lock.set(Mutex::new(fresh)); } }); let _ = statics::CRDT_EVENT_TX.get_or_init(|| broadcast::channel::(256).0); let _ = statics::SYNC_TX.get_or_init(|| broadcast::channel::(1024).0); - // Per-thread op journal + vector clock — keeps parallel tests' writes - // from corrupting each other's view of ALL_OPS (notably, one thread's - // `apply_compaction` could otherwise prune another thread's ops). + // Per-thread op journal + vector clock — always cleared so a second test + // on the same thread cannot see ops written by the first. statics::ALL_OPS_TL.with(|lock| { - let _ = lock.set(Mutex::new(Vec::new())); + if let Some(mutex) = lock.get() { + mutex.lock().unwrap().clear(); + } else { + let _ = lock.set(Mutex::new(Vec::new())); + } }); statics::VECTOR_CLOCK_TL.with(|lock| { - let _ = lock.set(Mutex::new(VectorClock::new())); + if let Some(mutex) = lock.get() { + mutex.lock().unwrap().clear(); + } else { + let _ = lock.set(Mutex::new(VectorClock::new())); + } }); } diff --git a/server/src/db/content_store.rs b/server/src/db/content_store.rs index 3fd53d31..8709f4f7 100644 --- a/server/src/db/content_store.rs +++ b/server/src/db/content_store.rs @@ -165,7 +165,9 @@ pub fn delete_content(key: ContentKey<'_>) { /// Ensure the in-memory content store is initialised. /// -/// Safe to call multiple times — the `OnceLock` is set at most once. +/// In non-test builds: init-once via `OnceLock` (safe to call multiple times). +/// In test builds: always resets `CONTENT_STORE_TL` to an empty `HashMap` so +/// each test on the same thread starts with a clean store. pub fn ensure_content_store() { #[cfg(not(test))] { @@ -175,7 +177,11 @@ pub fn ensure_content_store() { #[cfg(test)] { CONTENT_STORE_TL.with(|lock| { - if lock.get().is_none() { + if let Some(mutex) = lock.get() { + // Already initialised on this thread — reset to empty so the + // next test does not see content written by a previous test. + mutex.lock().unwrap().clear(); + } else { let _ = lock.set(Mutex::new(HashMap::new())); } }); @@ -203,6 +209,41 @@ pub(super) fn init_content_store(map: HashMap) { mod tests { use super::*; + /// Regression: two sequential `ensure_content_store()` + write + read cycles + /// in the same test body must not see each other's content. Before the fix, + /// `ensure_content_store()` was a no-op on the second call (OnceLock gating), + /// so the second cycle could read items written in the first cycle. + #[test] + fn sequential_ensure_content_store_resets_state() { + // ── Cycle 1 ────────────────────────────────────────────────────────── + ensure_content_store(); + write_content(ContentKey::Story("1111_cycle1"), "cycle-one body"); + assert_eq!( + read_content(ContentKey::Story("1111_cycle1")).as_deref(), + Some("cycle-one body"), + "cycle 1: item must be readable after write" + ); + + // ── Cycle 2: reset, write a different item ──────────────────────────── + ensure_content_store(); + // Cycle-1 item must no longer be visible. + assert!( + read_content(ContentKey::Story("1111_cycle1")).is_none(), + "cycle 2: store must be empty; cycle-1 content must not bleed through" + ); + write_content(ContentKey::Story("1111_cycle2"), "cycle-two body"); + assert_eq!( + read_content(ContentKey::Story("1111_cycle2")).as_deref(), + Some("cycle-two body"), + "cycle 2: own item must be readable" + ); + // And cycle-1 key must still be absent. + assert!( + read_content(ContentKey::Story("1111_cycle1")).is_none(), + "cycle 2: cycle-1 content must remain absent after cycle-2 write" + ); + } + /// AC 2 regression: writing under `ContentKey::Story` is not visible under /// `ContentKey::GateOutput` (and vice versa). The typed key namespace, not /// runtime substring matching, enforces the separation. diff --git a/server/src/db/ops.rs b/server/src/db/ops.rs index 05713ee5..f0ec6ff1 100644 --- a/server/src/db/ops.rs +++ b/server/src/db/ops.rs @@ -72,6 +72,12 @@ pub fn write_item_with_content(story_id: &str, stage: &str, content: &str, meta: .and_then(|d| serde_json::to_string(d).ok()); // Update in-memory content store. + // In test builds, the caller (test setup) is responsible for calling + // ensure_content_store() once before writing — calling it here would + // reset the store on every write, losing items from prior writes in the + // same test. In production, the lazy-init call is safe because nothing + // resets the store between writes. + #[cfg(not(test))] ensure_content_store(); write_content(ContentKey::Story(story_id), content); diff --git a/server/src/http/mcp/story_tools/bug.rs b/server/src/http/mcp/story_tools/bug.rs index 3117e017..0676bff8 100644 --- a/server/src/http/mcp/story_tools/bug.rs +++ b/server/src/http/mcp/story_tools/bug.rs @@ -86,6 +86,7 @@ mod tests { use crate::http::test_helpers::test_ctx; fn setup_git_repo_in(dir: &std::path::Path) { + crate::db::ensure_content_store(); std::process::Command::new("git") .args(["init"]) .current_dir(dir) diff --git a/server/src/http/mcp/story_tools/refactor.rs b/server/src/http/mcp/story_tools/refactor.rs index 17debcb1..c51163fd 100644 --- a/server/src/http/mcp/story_tools/refactor.rs +++ b/server/src/http/mcp/story_tools/refactor.rs @@ -115,6 +115,7 @@ mod tests { #[test] fn tool_create_refactor_accepts_single_criterion() { + crate::db::ensure_content_store(); let tmp = tempfile::tempdir().unwrap(); let ctx = test_ctx(tmp.path()); let result = tool_create_refactor( @@ -146,6 +147,7 @@ mod tests { #[test] fn tool_create_refactor_accepts_mixed_junk_and_real_acceptance_criteria() { + crate::db::ensure_content_store(); let tmp = tempfile::tempdir().unwrap(); let ctx = test_ctx(tmp.path()); let result = tool_create_refactor( diff --git a/server/src/http/mcp/story_tools/spike.rs b/server/src/http/mcp/story_tools/spike.rs index b84414ef..153da381 100644 --- a/server/src/http/mcp/story_tools/spike.rs +++ b/server/src/http/mcp/story_tools/spike.rs @@ -118,6 +118,7 @@ mod tests { #[test] fn tool_create_spike_creates_file() { + crate::db::ensure_content_store(); let tmp = tempfile::tempdir().unwrap(); let ctx = test_ctx(tmp.path()); @@ -147,6 +148,7 @@ mod tests { #[test] fn tool_create_spike_creates_file_without_description() { + crate::db::ensure_content_store(); let tmp = tempfile::tempdir().unwrap(); let ctx = test_ctx(tmp.path()); @@ -202,6 +204,7 @@ mod tests { #[test] fn tool_create_spike_accepts_single_criterion() { + crate::db::ensure_content_store(); let tmp = tempfile::tempdir().unwrap(); let ctx = test_ctx(tmp.path()); let result = tool_create_spike( @@ -233,6 +236,7 @@ mod tests { #[test] fn tool_create_spike_accepts_mixed_junk_and_real_acceptance_criteria() { + crate::db::ensure_content_store(); let tmp = tempfile::tempdir().unwrap(); let ctx = test_ctx(tmp.path()); let result = tool_create_spike( diff --git a/server/src/http/mcp/story_tools/story/create.rs b/server/src/http/mcp/story_tools/story/create.rs index 3fa255d2..7ad807c8 100644 --- a/server/src/http/mcp/story_tools/story/create.rs +++ b/server/src/http/mcp/story_tools/story/create.rs @@ -256,6 +256,7 @@ mod tests { #[test] fn tool_create_story_accepts_single_criterion() { + crate::db::ensure_content_store(); let tmp = tempfile::tempdir().unwrap(); let ctx = test_ctx(tmp.path()); let result = tool_create_story( @@ -283,6 +284,7 @@ mod tests { #[test] fn tool_create_story_accepts_mixed_junk_and_real_acceptance_criteria() { + crate::db::ensure_content_store(); let tmp = tempfile::tempdir().unwrap(); let ctx = test_ctx(tmp.path()); let result = tool_create_story( @@ -299,6 +301,7 @@ mod tests { #[test] fn tool_create_story_description_is_written_to_file() { + crate::db::ensure_content_store(); let tmp = tempfile::tempdir().unwrap(); let ctx = test_ctx(tmp.path()); @@ -368,6 +371,7 @@ mod tests { #[test] fn tool_create_story_html_sanitised_in_name() { + crate::db::ensure_content_store(); let tmp = tempfile::tempdir().unwrap(); let ctx = test_ctx(tmp.path()); // HTML in name is sanitised (not rejected) diff --git a/server/src/http/mcp/story_tools/story/query.rs b/server/src/http/mcp/story_tools/story/query.rs index c2f36d10..f11a7b74 100644 --- a/server/src/http/mcp/story_tools/story/query.rs +++ b/server/src/http/mcp/story_tools/story/query.rs @@ -124,6 +124,7 @@ mod tests { #[test] fn tool_create_story_and_list_upcoming() { + crate::db::ensure_content_store(); let tmp = tempfile::tempdir().unwrap(); // No git repo needed: spike 61 — create_story just writes the file; // the filesystem watcher handles the commit asynchronously. diff --git a/server/src/http/workflow/bug_ops/tests.rs b/server/src/http/workflow/bug_ops/tests.rs index 6955b2ec..17c227d3 100644 --- a/server/src/http/workflow/bug_ops/tests.rs +++ b/server/src/http/workflow/bug_ops/tests.rs @@ -6,6 +6,7 @@ use super::spike::create_spike_file; use std::fs; fn setup_git_repo(root: &std::path::Path) { + crate::db::ensure_content_store(); std::process::Command::new("git") .args(["init"]) .current_dir(root) @@ -166,6 +167,7 @@ fn extract_bug_name_from_content_parses_heading() { #[test] fn create_bug_file_writes_correct_content() { + crate::db::ensure_content_store(); let tmp = tempfile::tempdir().unwrap(); setup_git_repo(tmp.path()); @@ -257,6 +259,7 @@ fn create_bug_file_rejects_empty_acceptance_criteria() { #[test] fn create_spike_file_writes_correct_content() { + crate::db::ensure_content_store(); let tmp = tempfile::tempdir().unwrap(); let spike_id = create_spike_file( @@ -294,6 +297,7 @@ fn create_spike_file_writes_correct_content() { #[test] fn create_spike_file_uses_description_when_provided() { + crate::db::ensure_content_store(); let tmp = tempfile::tempdir().unwrap(); let description = "What is the best approach for watching filesystem events?"; @@ -319,6 +323,7 @@ fn create_spike_file_uses_description_when_provided() { #[test] fn create_spike_file_uses_placeholder_when_no_description() { + crate::db::ensure_content_store(); let tmp = tempfile::tempdir().unwrap(); let spike_id = create_spike_file( tmp.path(), @@ -350,6 +355,7 @@ fn create_spike_file_rejects_empty_name() { #[test] fn create_spike_file_with_special_chars_in_name_produces_valid_yaml() { + crate::db::ensure_content_store(); let tmp = tempfile::tempdir().unwrap(); let name = "Spike: compare \"fast\" vs slow encoders"; let result = create_spike_file( @@ -423,6 +429,7 @@ fn create_bug_file_with_depends_on_persists_to_crdt() { #[test] fn create_bug_file_without_depends_on_omits_field() { + crate::db::ensure_content_store(); let tmp = tempfile::tempdir().unwrap(); setup_git_repo(tmp.path()); @@ -474,6 +481,7 @@ fn create_refactor_file_with_depends_on_persists_to_crdt() { #[test] fn create_refactor_file_without_depends_on_omits_field() { + crate::db::ensure_content_store(); let tmp = tempfile::tempdir().unwrap(); setup_git_repo(tmp.path());