Skip to content

Session ids: allocate the test store prefix instead of hashing it, and panic on a duplicate #280

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Broccolito merged 1 commit into from
Sep 12, 2026
Merged

Session ids: allocate the test store prefix instead of hashing it, and panic on a duplicate #280

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 20 additions & 22 deletions crates/biorouter/src/agents/subagent_handler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
/// `<date>_<n>` (`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 `<today>_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 `<today>_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 `<today>_1` in the process-global `AgentManager` pin, and
/// `workspace_extension`'s `the_default_scope_sees_a_registered_child_…`
/// registers its own `<today>_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)]
Expand Down
35 changes: 0 additions & 35 deletions crates/biorouter/src/agents/subagent_tool.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 `<YYYYMMDD>_<n>` 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 `<today>_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) {
Expand Down Expand Up @@ -2577,7 +2546,6 @@ mod tests {
);
let provider: std::sync::Arc<dyn crate::providers::base::Provider> =
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(
Expand Down Expand Up @@ -2711,7 +2679,6 @@ mod tests {
);
let provider: std::sync::Arc<dyn crate::providers::base::Provider> =
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(
Expand Down Expand Up @@ -2824,7 +2791,6 @@ mod tests {
);
let provider: std::sync::Arc<dyn crate::providers::base::Provider> =
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(
Expand Down Expand Up @@ -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,
Expand Down
81 changes: 31 additions & 50 deletions crates/biorouter/src/agents/workspace_extension.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 `<date>_<n>`
/// (`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 `<today>_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 `<today>_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 `<today>_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 `<today>_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
Expand Down Expand Up @@ -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 `<today>_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
/// `<today>_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]
Expand Down
Loading
Loading