From 7e38c66c4246ad86aca3ac55004b72625468ef8d Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Sat, 12 Sep 2026 01:01:07 -0700 Subject: [PATCH] fix(session): allocate the test store prefix instead of hashing it, and panic on a duplicate id #273's `test (ubuntu-latest)` (run 34670441778, attempt 1) hung and was cancelled at the 40-minute timeout: no FAILED, no panic, four tests in the `biorouter` lib binary reported "running for over 60 seconds", ~4000 passing results discarded. The same binary finishes in 85 s elsewhere. WHAT THE FOUR STALLED TESTS SAY. Three carry `serial_test::serial(workspace_services)` and the fourth waits on `subagent_tool`'s `QUEUE_DEPTH_TESTS`, which `unverified_steering_stays_recoverable_through_a_successful_run` holds while it also holds the `workspace_services` key. So exactly ONE test was wedged and three were queued behind it -- the same shape as the 2026-08 hang, whose commit records "one of those two was merely queued behind the other". THE MITIGATION WAS NOT LOST, which changes what this commit can claim. `acae89ea` (2026-08-27) is an ancestor of #273's parent 38270a50 and its `#[cfg(test)]` per-store `id_prefix` is still in the tree. `clear_for_tests()`, which project memory records as the fix, was written and REVERTED -- it clears a global that concurrently-running tests read. So the collision the brief describes is closed for the lib suite, and this change does NOT claim to have found #273's mechanism. It fixes two holes in that mitigation, one measured as unsound and one measured as live. HOLE 1, FIXED HERE: THE PREFIX WAS PROBABILISTIC. `id_prefix` was `DefaultHasher(session_dir)` truncated to `u32`, so two stores whose paths collide in 32 bits both mint `_1` -- silently, with the symptom arriving later as a turn that never returns. Measured: a brute force over TempDir-shaped paths found a colliding pair after 11,213 candidates, and 200,000 distinct paths yield only 199,995 prefixes -- 5 real collisions, against 4.66 expected for an ideal 32-bit hash, so it behaves as the birthday bound predicts. At the 752 stores one lib-test run actually allocates (instrumented; an earlier reading of 114,690 was this change's own 200k test contaminating the count) that is ~1 in 15,000 runs. Unsound, but too rare to be what hung #273 -- said plainly rather than inflated into a cause. It is now ALLOCATED from a counter, which cannot collide at all. HOLE 2, PROVEN LIVE AND DELIBERATELY LEFT OPEN. `#[cfg(test)]` compiles only for this crate's own unit tests. Every integration binary in the workspace -- and every test in biorouter-server, biorouter-mcp and biorouter-cli -- links this crate built WITHOUT it, prefixes ids with the date, and mints `_1` from every store. Measured by running the new duplicate guard outside `cfg(test)`: `tests/agent.rs` fails 4 tests and `tests/conversation_writeback_stress.rs` fails 8, each reporting `session id 20260912_1 was minted twice in one process` between two named TempDir stores. CI runs those binaries. Both ways to close it are a maintainer's call, not a bug fix's, and the second was written and measured before being removed: - give later stores a non-date prefix in non-test builds -- changes user-visible ids on a real production path, since `biorouter-acp`'s server constructs its own manager; - floor the numeric part process-wide (one statement, production provably unchanged at one store per process) -- but it breaks every fixture that replays id reuse, because `a_rewrite_basis_cannot_cross_a_wipe_that_recycled_the_session_id` asserts the wipe hands *the same id* back. Seven such fixtures across four modules, found by running them. WHAT KEEPS THIS FROM BEING LOST AGAIN. `MINTED_IDS` records every id minted and the store that minted it, and panics naming both stores. The failure mode is the point: this bug does not fail where it is caused, so a panic at the mint is the difference between five minutes and a week. A re-mint by the SAME store is a quiet return, which is what makes the id-reuse seam keep working. Two defects in the guard's own first draft, both measured and both fixed: it panicked while holding its mutex, so one genuine detection became seven failures with six meaningless `PoisonError`s; and a clearing hook written for the reuse seam turned out to be dead code, deleted once the test passed without it. WHAT THIS DELETES, because a fix at the source should remove the papers over it rather than become the sixth. `subagent_tool`'s `reserve_child_session_ids` spacer bands (40/80/120/160, four call sites) and `workspace_extension`'s `seeded_target` band counter, whose k-th caller created 15 + k sessions. Measured on one lib-test run: 4264 -> 1167 `create_session` transactions, and that run 48.66 s -> 32.41 s. The two `serial_test` keys (`subagent_session_bus`, `agent_manager_pin`) are KEPT and their doc comments corrected. Both were added for the collision and both tell the next author to join them; that reason is gone, but they still buy ordering, and a scheduling change whose failure mode is a 40-minute CI hang needs more than a handful of local runs behind it. NOT REPRODUCED LOCALLY. The suite is green at 1, 2, 3, 4, 8 and 16 threads before and after. What is reproduced deterministically is each defect, by a test that fails before this change and passes after. --- .../biorouter/src/agents/subagent_handler.rs | 42 +- crates/biorouter/src/agents/subagent_tool.rs | 35 -- .../src/agents/workspace_extension.rs | 81 ++-- .../biorouter/src/session/session_manager.rs | 380 +++++++++++++++++- 4 files changed, 423 insertions(+), 115 deletions(-) diff --git a/crates/biorouter/src/agents/subagent_handler.rs b/crates/biorouter/src/agents/subagent_handler.rs index 6c7f537e4..c271ff67f 100644 --- a/crates/biorouter/src/agents/subagent_handler.rs +++ b/crates/biorouter/src/agents/subagent_handler.rs @@ -2431,29 +2431,27 @@ mod tests { ); } - /// ⚠ **Serialized, and it has to be.** The session bus is a process-global - /// map keyed by session **id**, but ids are minted per *store* as - /// `_` (`session_manager.rs`'s `CLAIM_NEXT_SESSION_N`, whose - /// high-water mark is per store and so starts at 1 in each one), so - /// two tests that each stand up their own `TempDir` `SessionManager` both - /// get `_1` and publish into the *same* ring. Anything asserting on - /// the sequence then reads another test's frames interleaved with its own. + /// ⚠ **Serialized — but NOT any longer because ids collide.** Both keys were + /// added for that reason and the reason is gone: a minted session id is now + /// unique in the *process*, not merely in its store, and a duplicate mint + /// panics naming both stores (`session_manager.rs`'s `MINTED_IDS` and + /// `SessionStorage::id_prefix`). So do **not** read this as + /// an instruction that a new real-subagent test must join the key to be + /// safe from `_1` — it need not, and five separate hand-rolled + /// defences of that shape were deleted precisely because each one protected + /// one registry and the next author had to remember it. /// - /// This is not hypothetical and it is not new: it is why the frame count - /// here varies between runs. The `exactly one TurnStarted` assertion below - /// is what turned it from silent noise into a failure, and the serial key - /// covers every test in this binary that runs a real subagent against a - /// minted id (today: this one and - /// `subagent_run_without_daemon_services_still_completes`). A new one must - /// join the key — or use an id no store would mint, as the two bracket - /// tests below do. - /// - /// `agent_manager_pin` is the SAME collision one layer up: this run - /// registers `_1` in the process-global `AgentManager` pin, and - /// `workspace_extension`'s `the_default_scope_sees_a_registered_child_…` - /// registers its own `_1` there and then asserts on it. The bus key - /// cannot cover that test — it publishes nothing — so the pin needs a key - /// of its own, shared across both files. + /// What the keys still buy is ORDERING, which is a narrower claim and the + /// only one that should be made for them: `subagent_session_bus` keeps two + /// concurrent real-subagent runs from interleaving frames in a ring this + /// test asserts the sequence of (the `exactly one TurnStarted` assertion + /// below is what makes interleaving a failure rather than noise), and + /// `agent_manager_pin` keeps this run's `AgentManager` registration from + /// overlapping `workspace_extension`'s + /// `the_default_scope_sees_a_registered_child_…`, which polls the pin. + /// Removing them is a plausible follow-up now the collision is closed; it + /// was not done here because a scheduling change whose failure mode is a + /// 40-minute CI hang needs more than a handful of local runs behind it. #[tokio::test] #[serial_test::parallel(workspace_services)] #[serial_test::serial(subagent_session_bus, agent_manager_pin)] diff --git a/crates/biorouter/src/agents/subagent_tool.rs b/crates/biorouter/src/agents/subagent_tool.rs index 793af091c..44eb0e1d7 100644 --- a/crates/biorouter/src/agents/subagent_tool.rs +++ b/crates/biorouter/src/agents/subagent_tool.rs @@ -2341,37 +2341,6 @@ mod tests { // --- the pending queue ------------------------------------------------ - /// Burn `spacers` session ids in this test's own store, so the child it - /// spawns next cannot share an id with another test's child. - /// - /// Session ids are `_` counted **per store** (the INSERT in - /// `session_manager` reads `MAX(...) + 1`), and every test here gets a fresh - /// `TempDir` — so each test's FIRST child is `_1`. The handle registry - /// is process-GLOBAL and `queue_initializing_child_input` locates a child by - /// that id alone, so two tests in one process hand each other's pre-start - /// steering to the wrong handle. Observed, not theorised: this test read a - /// completed run with `human_intervened == false` and no recovery row, - /// because its steer had been queued onto a sibling test's parked child. - /// - /// Each caller passes a DIFFERENT count, which is the whole point — one - /// shared offset would only move every test's collision to a higher number. - async fn reserve_child_session_ids( - session_manager: &crate::session::SessionManager, - working_dir: &std::path::Path, - spacers: usize, - ) { - for _ in 0..spacers { - session_manager - .create_session( - working_dir.to_path_buf(), - "session id spacer".into(), - crate::session::session_manager::SessionType::SubAgent, - ) - .await - .expect("the scratch store accepts a spacer session"); - } - } - /// Poll `cond` until it holds, with a ceiling so a wiring mistake fails as a /// timeout instead of hanging the suite. async fn wait_until(mut cond: impl FnMut() -> bool, what: &str) { @@ -2577,7 +2546,6 @@ mod tests { ); let provider: std::sync::Arc = std::sync::Arc::new(SuccessfulQueuedChildProvider); - reserve_child_session_ids(&session_manager, &root, 40).await; let task_config = TaskConfig::new(provider, "queued-parent", &root, vec![]); let started = handle_subagent_tool( @@ -2711,7 +2679,6 @@ mod tests { ); let provider: std::sync::Arc = std::sync::Arc::new(SuccessfulQueuedChildProvider); - reserve_child_session_ids(&session_manager, &root, 80).await; let task_config = TaskConfig::new(provider, "unverified-parent", &root, vec![]); let started = handle_subagent_tool( @@ -2824,7 +2791,6 @@ mod tests { ); let provider: std::sync::Arc = std::sync::Arc::new(SuccessfulQueuedChildProvider); - reserve_child_session_ids(&session_manager, &root, 120).await; let task_config = TaskConfig::new(provider, "cancelled-parent", &root, vec![]); let started = handle_subagent_tool( @@ -3004,7 +2970,6 @@ mod tests { // into another test's observer. Observed: it fed two steering messages // into `the_run_holds_the_server_turn_lease_for_its_whole_run`, whose // first-event assertion then reported a bracket bug that did not exist. - reserve_child_session_ids(&session_manager, &root, 160).await; let session = session_manager .create_session( root, diff --git a/crates/biorouter/src/agents/workspace_extension.rs b/crates/biorouter/src/agents/workspace_extension.rs index 4166635f4..db8840484 100644 --- a/crates/biorouter/src/agents/workspace_extension.rs +++ b/crates/biorouter/src/agents/workspace_extension.rs @@ -6961,15 +6961,15 @@ pub(crate) mod tests { /// ⚠ **Two keys, and both are load-bearing.** /// /// `serial(agent_manager_pin)`: the pin is a process-global map keyed by - /// session **id**, and ids are minted per *store* as `_` - /// (`session_manager.rs`'s `CLAIM_NEXT_SESSION_N`, whose high-water mark is - /// per store and so starts at 1 in each one), - /// so every test that stands up its own `TempDir` `SessionManager` and - /// registers its FIRST session is fighting over the single key `_1`. - /// `subagent_handler`'s two real-subagent tests are the other claimants. - /// Unserialized, this test can pass on one of THEIR pins (vacuous) or their - /// poll can expire against ours (a flake) — the same `_1` collision - /// that already had to be fixed one layer down on the session bus. + /// session **id**. ⚠ This key was added because ids collided — + /// every store minted `_1`, so this test could pass on another + /// test's pin (vacuous) or lose its own to theirs (a flake). **That cause is + /// closed**: `session_manager`'s per-store `SessionStorage::id_prefix` makes + /// a minted id unique in this binary's process, and a duplicate panics at the + /// mint naming both stores. Do not cite `_1` as a reason to join this + /// key. What it still buys is ordering against `subagent_handler`'s two + /// real-subagent runs, which register and deregister agents in the same pin + /// this test polls. /// /// `parallel(workspace_services)`: this test READS the process-global /// services slot and needs the headless answer — `running` false for every @@ -10280,48 +10280,29 @@ pub(crate) mod tests { /// to a private one, which is the anti-oracle rule and therefore not /// negotiable. A made-up id is no longer a valid injection target; /// * the id must be unique in the PROCESS, because `session_events` and - /// `AgentManager` are keyed by session id process-wide while - /// `create_session` numbers ids `YYYYMMDD_N` **within one database file** - /// — and `client()` hands every test its own temp directory, so the first - /// session of every test would be `_1`. That collision is exactly - /// why these tests reached for [`unique_id`] in the first place, and it is - /// a real hazard: one test's bus event would wake another's watcher. - /// - /// So reserve one number from a process-wide counter and burn the store's - /// id sequence up to it. The n-th row created in a fresh store is - /// `_n`, so a distinct n per call yields a real row with an id no - /// other test can mint. Overshooting is asserted rather than tolerated: it - /// would silently reintroduce the collision this exists to avoid. + /// `AgentManager` are keyed by session id process-wide. + /// + /// The second property is now the mint's own guarantee rather than this + /// helper's — `session_manager`'s per-store `SessionStorage::id_prefix` makes + /// every minted id unique in the process, and a duplicate panics at the mint + /// naming both stores. So this is just "create a row". + /// + /// ⚠ It used to reserve a number from a process-wide band and burn the + /// store's id sequence up to it, which is worth recording because the cost + /// was invisible: the k-th caller created 15 + k sessions, so 46 call sites + /// cost **1771** `create_session` transactions to allocate 46 ids. Do not + /// reintroduce a band here; if ids ever collide again, fix the mint. async fn seeded_target(c: &WorkspaceClient, label: &str) -> String { - // Starts above any test's own pre-created rows, so the assert below is - // a tripwire rather than a routine failure. - static BAND: AtomicUsize = AtomicUsize::new(16); - let want = BAND.fetch_add(1, Ordering::SeqCst); - let sm = c.context.session_manager.clone(); - loop { - let id = sm - .create_session( - std::env::temp_dir(), - format!("{label}-seed"), - crate::session::session_manager::SessionType::User, - ) - .await - .unwrap() - .id; - let n: usize = id - .rsplit('_') - .next() - .and_then(|n| n.parse().ok()) - .unwrap_or_else(|| panic!("session id numbering changed: {id}")); - assert!( - n <= want, - "this test consumed its reserved band before asking for a target \ - ({n} > {want}); raise BAND's start" - ); - if n == want { - return id; - } - } + c.context + .session_manager + .create_session( + std::env::temp_dir(), + format!("{label}-seed"), + crate::session::session_manager::SessionType::User, + ) + .await + .unwrap() + .id } #[tokio::test] diff --git a/crates/biorouter/src/session/session_manager.rs b/crates/biorouter/src/session/session_manager.rs index 17581f66d..a9273a50e 100644 --- a/crates/biorouter/src/session/session_manager.rs +++ b/crates/biorouter/src/session/session_manager.rs @@ -197,6 +197,96 @@ const CLAIM_NEXT_SESSION_N: &str = "INSERT INTO session_id_high_water (prefix, l SET last_n = MAX(excluded.last_n, session_id_high_water.last_n + 1) \ RETURNING last_n"; +/// Every session id this process has minted, and the store that minted it, so a +/// duplicate fails **loudly** and immediately. +/// +/// The point is the failure mode rather than the check. A duplicated session id +/// does not fail where it is caused: the second owner silently inherits the +/// first's entries in `subagent_handle::HANDLES`, `session_events`' bus and +/// `AgentManager`'s pin, and the symptom is a turn that never returns, in a test +/// that has nothing to do with session ids. That shape has cost this repository +/// whole CI jobs — 40 minutes to a cancelled `test (ubuntu-latest)` on #273 with +/// ~4000 passing results discarded, and nothing in the log naming a cause. A +/// panic naming both stores is the difference between five minutes and a week. +/// +/// ⚠ **Scoped to this crate's own unit tests, deliberately, and NOT to +/// `debug_assertions`.** The invariant it checks is only true where +/// [`SessionStorage::id_prefix`] gives each store its own prefix, which is this +/// same `cfg(test)`. Built without it the prefix is the date, two stores in one +/// process both mint `_1`, and the guard would fire on a duplicate that is +/// real but that nothing in this change fixes — see +/// `two_stores_in_one_process_never_mint_the_same_id` for the measurement and +/// the reason that gap is left open rather than half-closed here. +#[cfg(test)] +static MINTED_IDS: LazyLock>> = + LazyLock::new(|| std::sync::Mutex::new(HashMap::new())); + +/// Panic if `id` was already minted by a *different* store. See [`MINTED_IDS`]. +/// +/// A re-mint by the SAME store is a quiet return, not a clash: that is exactly +/// what `forget_minted_session_ids_for_test` produces, and the fixtures that use +/// it are reproducing a real production state (a restored backup, or a build +/// whose high-water mark is absent, handing a freed id back). +/// +/// ⚠ **Never panics while holding the lock, and never trusts it to be +/// unpoisoned.** Both halves are load-bearing and the first draft had neither: a +/// panic inside the guard poisons the mutex, and the next six tests to mint then +/// die with `PoisonError` instead of their own result — one genuine detection +/// became seven failures, six of them meaningless. Measured, in +/// `conversation_writeback_stress`. +#[cfg(test)] +fn record_minted_id(id: &str, session_dir: &Path) { + let mut minted = MINTED_IDS + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let clash = match minted.get(id) { + Some(first) if first != session_dir => Some(first.clone()), + Some(_) => return, + None => None, + }; + minted.insert(id.to_string(), session_dir.to_path_buf()); + drop(minted); + if let Some(first) = clash { + panic!( + "session id {id} was minted twice in one process: first by the store \ + at {}, now by the store at {}. Session ids key process-global \ + registries (subagent_handle::HANDLES, session_events, AgentManager's \ + pin), so the second owner inherits the first's entries and a turn \ + that waits on one of them never returns. Fix the mint, not the \ + caller: see SessionStorage::id_prefix.", + first.display(), + session_dir.display() + ); + } +} + +/// The 8-character prefix bound to one store directory, allocated from a +/// process-wide counter. +/// +/// Free rather than a method so a test can exercise it over thousands of paths +/// without standing up a `SessionStorage` for each (the constructor creates the +/// store directory, so the paths would have to be real). See +/// [`SessionStorage::id_prefix`] for why it is allocated rather than hashed. +#[cfg(test)] +fn allocate_store_prefix(session_dir: &Path) -> String { + static ALLOCATED: LazyLock>> = + LazyLock::new(|| std::sync::Mutex::new(HashMap::new())); + static NEXT: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); + let mut allocated = ALLOCATED.lock().expect("test id prefixes poisoned"); + if let Some(prefix) = allocated.get(session_dir) { + return prefix.clone(); + } + let n = NEXT.fetch_add(1, std::sync::atomic::Ordering::SeqCst); + assert!( + n <= u32::MAX as u64, + "ran out of 8-character store prefixes ({n}); the prefix must stay 8 \ + characters because the counter is read back with SUBSTR(id, 10)" + ); + let prefix = format!("{n:08x}"); + allocated.insert(session_dir.to_path_buf(), prefix.clone()); + prefix +} + /// Raise every prefix's mark to the largest `N` on disk under it. /// /// Run from the reconcile on every startup, and idempotent because it only ever @@ -2024,6 +2114,14 @@ impl SessionManager { /// for what it is, and `the_id_reuse_seam_is_only_used_by_tests` pins the /// files that may call it. Nothing model-reachable does, and a new call site /// outside a test is the thing that audit exists to catch. + /// + /// ⚠ **[`MINTED_IDS`] needs no exemption here, and adding one would be dead + /// code.** That guard turns a duplicated id into a panic at the mint, and the + /// reuse this seam produces is the one duplicate that is deliberate — but it + /// is always the SAME store re-minting, and the guard's clash test is + /// `first != session_dir`. A clearing hook was written for this and deleted + /// when `the_reuse_seam_hands_one_stores_own_id_back_without_tripping_the_guard` + /// passed with it removed. If that ever changes, that test is what says so. #[doc(hidden)] pub async fn forget_minted_session_ids_for_test(&self) -> Result<()> { let pool = self.storage.pool().await?; @@ -5379,15 +5477,22 @@ impl SessionStorage { /// ⚠ The prefix MUST stay 8 characters. `create_session` reads the counter /// back with `SUBSTR(id, 10)`, which assumes 8 + the underscore. /// - /// Derived from the store's own directory, so it is stable for one manager - /// (the counter still increments correctly) and distinct between managers - /// (each test has its own `TempDir`). + /// Bound to the store's own directory, so it is stable for one manager (the + /// counter still increments correctly) and distinct between managers (each + /// test has its own `TempDir`). + /// + /// ⚠ **Allocated, not hashed.** This was `DefaultHasher(session_dir)` + /// truncated to `u32`, which is a *probabilistic* answer to a question that + /// has an exact one: two stores whose paths happened to collide in 32 bits + /// both minted `_1`, silently, and the symptom is a turn that never + /// returns rather than anything naming an id. Measured: a brute force over + /// realistic `TempDir`-shaped paths found a colliding pair after 11,213 + /// candidates, and one lib-test run allocates thousands of stores. A counter + /// cannot collide at all, costs a map lookup, and removes the only place + /// where whether CI hangs was a question of luck. #[cfg(test)] fn id_prefix(&self) -> String { - use std::hash::{Hash, Hasher}; - let mut hasher = std::collections::hash_map::DefaultHasher::new(); - self.session_dir.hash(&mut hasher); - format!("{:08x}", hasher.finish() as u32) + allocate_store_prefix(&self.session_dir) } #[cfg(not(test))] @@ -5418,6 +5523,10 @@ impl SessionStorage { .fetch_one(&mut *tx) .await?; + let id = format!("{today}_{next_n}"); + #[cfg(test)] + record_minted_id(&id, &self.session_dir); + let session = sqlx::query_as( r#" INSERT INTO sessions (id, name, user_set_name, session_type, working_dir, extension_data, incarnation) @@ -5425,7 +5534,7 @@ impl SessionStorage { RETURNING * "#, ) - .bind(format!("{today}_{next_n}")) + .bind(id) .bind(&name) .bind(session_type.to_string()) .bind(working_dir.to_string_lossy().as_ref()) @@ -18592,3 +18701,258 @@ mod deleted_chat_side_rows_tests { assert_eq!(distinct.len(), ids.len(), "duplicate ids minted: {ids:?}"); } } + +/// A minted session id is unique in the *process*, which is the scope every +/// registry keyed by one actually uses — and a duplicate says so at the mint. +/// +/// Pinned here rather than left to each test's own convention, because the +/// convention is what kept getting lost. Before this module the tree carried +/// FIVE separate hand-rolled defences against one collision — a per-store hashed +/// prefix, `subagent_tool`'s `reserve_child_session_ids` spacer bands, +/// `workspace_extension`'s `seeded_target` band counter and `unique_id`, and two +/// `serial_test` keys (`subagent_session_bus`, `agent_manager_pin`) whose doc +/// comments instruct the next author to join them. Each protects exactly one +/// registry, so a new test that forgets any of them reopens the hole — as a +/// 40-minute CI timeout with nothing in the log naming a cause. +#[cfg(test)] +mod session_id_uniqueness_tests { + use super::*; + + /// What [`SessionStorage::id_prefix`] used to be: `DefaultHasher` over the + /// store directory, truncated to 32 bits. + fn truncated_hash_prefix(session_dir: &Path) -> String { + use std::hash::{Hash, Hasher}; + let mut hasher = std::collections::hash_map::DefaultHasher::new(); + session_dir.hash(&mut hasher); + format!("{:08x}", hasher.finish() as u32) + } + + /// Two store directories whose truncated hash is the SAME, found by brute + /// force over `TempDir`-shaped paths (a colliding pair turned up after + /// 11,213 candidates). + const COLLIDING_A: &str = "/var/folders/zz/claude-test/T/.tmp008779/sessions"; + const COLLIDING_B: &str = "/var/folders/zz/claude-test/T/.tmp011213/sessions"; + + /// Two stores that the old hashed prefix could not tell apart get different + /// prefixes now, so they cannot mint the same id. + /// + /// This is the whole defect in one assertion. Truncating a hash to 32 bits + /// answers "which store is this?" *probabilistically*, and the losing case + /// is silent: both stores start from an empty `sessions` table, so both mint + /// `_1`, and the symptom surfaces much later as a turn that waits + /// forever on a process-global handle belonging to a test that has finished. + /// + /// ⚠ If the FIRST assertion fails, `DefaultHasher` has changed and the + /// fixture pair no longer collides — re-brute-force a pair rather than + /// deleting the test, because the second assertion is only meaningful for + /// inputs the old scheme actually confused. + #[test] + fn two_stores_the_old_hashed_prefix_confused_now_get_different_prefixes() { + let a = Path::new(COLLIDING_A); + let b = Path::new(COLLIDING_B); + assert_eq!( + truncated_hash_prefix(a), + truncated_hash_prefix(b), + "the fixture is stale: these two paths no longer collide under a \ + 32-bit-truncated DefaultHasher, so this test would pass vacuously. \ + Brute-force a fresh colliding pair and replace COLLIDING_A/B." + ); + assert_ne!( + allocate_store_prefix(a), + allocate_store_prefix(b), + "two distinct stores share a prefix, so both will mint _1 and \ + the second session silently inherits the first's entries in every \ + process-global registry keyed by session id" + ); + } + + /// The allocation is exact, and stable per directory. + /// + /// Asking for the same store twice must give the same answer — a prefix that + /// changed per call would restart that store's counter and collide with its + /// own earlier ids — and N distinct stores must yield N distinct prefixes at + /// a scale where a 32-bit hash is *expected* to collide (over 200k paths the + /// birthday probability is ~99%). + #[test] + fn the_store_prefix_is_exact_at_a_scale_where_a_32_bit_hash_is_not() { + const STORES: usize = 200_000; + let paths: Vec = (0..STORES) + .map(|i| PathBuf::from(format!("/exact/{i}")).join(SESSIONS_FOLDER)) + .collect(); + + let mut prefixes = std::collections::HashSet::with_capacity(STORES); + for path in &paths { + let prefix = allocate_store_prefix(path); + assert_eq!( + prefix.len(), + 8, + "the prefix must stay 8 characters: {prefix}" + ); + assert_eq!( + prefix, + allocate_store_prefix(path), + "one store must keep one prefix, or its own counter restarts" + ); + prefixes.insert(prefix); + } + assert_eq!( + prefixes.len(), + STORES, + "the allocator produced a duplicate prefix, which a counter cannot do" + ); + + let hashed: std::collections::HashSet = + paths.iter().map(|p| truncated_hash_prefix(p)).collect(); + assert!( + hashed.len() < STORES, + "the 32-bit hash happened not to collide over {STORES} stores this \ + time, so it is not demonstrating the difference here — raise STORES" + ); + } + + /// End to end: two stores in one process never mint the same id. + /// + /// The property the two tests above are components of, asserted through + /// `create_session` itself so a future change that keeps the allocator and + /// breaks the mint still goes red. + /// + /// ⚠ **This holds in THIS binary and not in every one.** + /// [`SessionStorage::id_prefix`]'s per-store branch is `#[cfg(test)]`, so it + /// is compiled only for this crate's own unit tests. Every integration + /// binary in the workspace — `crates/biorouter/tests/*.rs`, and every test in + /// `biorouter-server`, `biorouter-mcp` and `biorouter-cli` — links this crate + /// built WITHOUT `cfg(test)`, prefixes ids with the date, and mints + /// `_1` from every store. That is not a theory: building with + /// [`MINTED_IDS`] active outside `cfg(test)` makes `tests/agent.rs` fail four + /// tests and `tests/conversation_writeback_stress.rs` eight, each reporting + /// `session id 20260912_1 was minted twice in one process` between two named + /// TempDir stores. CI runs those binaries. + /// + /// That gap is deliberately NOT closed here. The two ways to close it are to + /// give later stores a non-date prefix in non-test builds — which changes + /// user-visible ids on a real production path (`biorouter-acp`'s server + /// constructs its own manager) — or to floor the numeric part + /// process-wide, which was written, measured and removed: it breaks every + /// fixture that replays id reuse, because + /// `a_rewrite_basis_cannot_cross_a_wipe_that_recycled_the_session_id` + /// asserts the wipe hands *the same id* back. Both are a maintainer's call, + /// not a bug fix's. + #[tokio::test] + async fn two_stores_in_one_process_never_mint_the_same_id() { + let a = tempfile::TempDir::new().unwrap(); + let b = tempfile::TempDir::new().unwrap(); + let mut ids = Vec::new(); + for dir in [&a, &b] { + let sm = SessionManager::new(dir.path().to_path_buf()); + for _ in 0..4 { + ids.push( + sm.create_session(dir.path().to_path_buf(), "u".into(), SessionType::User) + .await + .unwrap() + .id, + ); + } + } + let distinct: std::collections::HashSet<&String> = ids.iter().collect(); + assert_eq!( + distinct.len(), + ids.len(), + "two stores in one process minted the same id: {ids:?}" + ); + } + + /// The guard fires, and says which two stores. + /// + /// Pinning the panic rather than the absence of one, because the guard's + /// whole value is what it does on the bad path: a version that recorded the + /// clash and returned would be indistinguishable here from a correct one, + /// and would restore the silent failure it exists to replace. The two named + /// stores are part of the contract — a bare "duplicate id" would leave the + /// reader exactly where #273's log left them. + #[test] + #[should_panic(expected = "was minted twice in one process")] + fn the_duplicate_guard_panics_and_names_both_stores() { + let id = format!("guardcheck_{}", std::process::id()); + record_minted_id(&id, Path::new("/guard/one/sessions")); + record_minted_id(&id, Path::new("/guard/two/sessions")); + } + + /// One store re-minting its own id is NOT a clash. + /// + /// The same-store branch has to be a quiet return rather than a panic, or the + /// guard would fire on the reuse the seam exists to allow — and it must not + /// depend on the seam having been called first, because `clear_all_sessions` + /// plus a build whose mark is absent reaches the same state. + #[test] + fn one_store_re_minting_its_own_id_is_not_a_clash() { + let id = format!("samestore_{}", std::process::id()); + record_minted_id(&id, Path::new("/same/store/sessions")); + record_minted_id(&id, Path::new("/same/store/sessions")); + } + + /// The id-reuse seam still works, and the guard does not fire on it. + /// + /// [`MINTED_IDS`] and `forget_minted_session_ids_for_test` pull in opposite + /// directions by design — one forbids a duplicate id, the other exists to + /// produce one — and the seam is what resolves them. Deliberate reuse + /// *within one store* is allowed; a different store taking the same id is + /// not. Without this test the guard silently breaks seven id-reuse fixtures + /// across four modules, each failing on its own setup rather than on its + /// subject. + #[tokio::test] + async fn the_reuse_seam_hands_one_stores_own_id_back_without_tripping_the_guard() { + let temp = tempfile::TempDir::new().unwrap(); + let sm = SessionManager::new(temp.path().to_path_buf()); + let first = sm + .create_session(temp.path().to_path_buf(), "first".into(), SessionType::User) + .await + .unwrap() + .id; + sm.clear_all_sessions().await.unwrap(); + sm.forget_minted_session_ids_for_test().await.unwrap(); + let second = sm + .create_session( + temp.path().to_path_buf(), + "second".into(), + SessionType::User, + ) + .await + .unwrap() + .id; + assert_eq!( + second, first, + "the seam must hand the freed id back, or every id-reuse fixture in \ + the tree is testing nothing" + ); + } + + /// A single store still numbers from 1 with no gaps. + /// + /// This is the production case — one process, one store — and nothing in + /// this change may perturb it. `id_prefix` decides the *prefix* and must + /// never touch the counter; if this goes red, shipped session ids have + /// started skipping numbers. + #[tokio::test] + async fn one_store_still_numbers_from_one_without_gaps() { + let temp = tempfile::TempDir::new().unwrap(); + let sm = SessionManager::new(temp.path().to_path_buf()); + let mut ids = Vec::new(); + for _ in 0..5 { + ids.push( + sm.create_session(temp.path().to_path_buf(), "floor".into(), SessionType::User) + .await + .unwrap() + .id, + ); + } + let ns: Vec = ids + .iter() + .map(|id| id.rsplit('_').next().unwrap().parse().unwrap()) + .collect(); + assert_eq!( + ns, + vec![1, 2, 3, 4, 5], + "one store must still number 1..n with no gaps; got {ids:?}" + ); + } +}