From 001fd08ea2ef6259ceae73eb1235ec804c913733 Mon Sep 17 00:00:00 2001 From: Jared Lunde Date: Wed, 7 Oct 2026 06:21:25 -0700 Subject: [PATCH] agent: the restore window never waits on a session that will not restore; release binaries scanned for seams Audit of #156: - A listed restoring session could hold the 503 window for its full 10 min if it failed to start, ended with an error, got no events hub (its MCP catalog now empty), or panicked on every start. `mcp_events::restored` is now called on each of those terminal paths: a failed boot pin (events session or runtime session), a session body that ends with an error, a session that starts with no hub, and every panic (a restart reserves its own tokens again). Tests: a listed session with no events hub, and one whose transcript no longer opens, each close the window, so a dead token is 410 within moments; both stay 503 without the fix. - No test seam ships, enforced on the artifact: `build (release)` now runs `mise check:release-seams`, which scans every release binary for the `BEYOND_AI_AGENT_TEST_` prefix and fails on any hit (a grep, seconds; no extra build). The source checker stays as the fast early warning. The scanner is pinned by a test: it fails on this crate's debug build (which carries the seams), on a `concat!`-assembled name, and passes a clean file. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01JimHGjsfk2Ktm5GxyZJKKk --- .github/workflows/ci.yml | 3 + crates/agent/ARCHITECTURE.md | 6 +- crates/agent/scripts/scan-release-seams.sh | 26 ++++++ crates/agent/src/serve.rs | 4 + crates/agent/src/serve_ws.rs | 30 +++++-- crates/agent/src/tools/mcp_events/mod.rs | 4 +- crates/agent/src/tools/mcp_events/webhook.rs | 7 +- crates/agent/tests/mcp_events_receiver.rs | 85 ++++++++++++++++++++ crates/agent/tests/release_seams.rs | 45 ++++++++++- mise.toml | 4 + 10 files changed, 199 insertions(+), 15 deletions(-) create mode 100755 crates/agent/scripts/scan-release-seams.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9238aac1..cee20f77 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -299,6 +299,9 @@ jobs: - uses: jdx/mise-action@v4 - uses: Swatinem/rust-cache@v2 - run: mise build:rs:release + # The authority on "no test seam ships": the release binaries themselves, scanned for the + # seam prefix (a grep over what was just built; seconds). + - run: mise check:release-seams # Mutation testing on what this PR changed: every line the diff touches in the gateway and the # catalog is mutated, and a mutant the tests don't catch fails the job. Cost scales with the diff # (each mutant is a rebuild plus a test run), so the mutants are split across 8 runners: a typical diff --git a/crates/agent/ARCHITECTURE.md b/crates/agent/ARCHITECTURE.md index 68307321..fd91368a 100644 --- a/crates/agent/ARCHITECTURE.md +++ b/crates/agent/ARCHITECTURE.md @@ -4312,7 +4312,7 @@ backend an operator points it at. | `BEYOND_AI_AGENT_TEST_SLOW_SESSION_OPEN_MS` | unset (no delay) | **Debug builds only** (`#[cfg(debug_assertions)]`), and only for tests: makes every `Persistence::open` sleep this many milliseconds, standing in for a slow network-backed session directory. `tests/serve_startup_stall.rs` uses it to prove session start-up is off the shared runtime thread. A release binary contains no latency-injection path at all. | | `AI_AGENT_OAUTH_CALLBACK_HOST` | unset (`127.0.0.1`) | The host the fixed-port OAuth login callback listener (Anthropic 53692, OpenAI Codex 1455) binds. **Loopback only** — an address in `127.0.0.0/8`, `::1`, or `localhost` — because the listener receives an authorization code; anything else (`0.0.0.0`, a routable address, a name) fails the login with a clear error before any bind (`oauth::callback_server::callback_host`). Tests give a login its own loopback address so concurrent runs do not collide on the fixed port. | | `BEYOND_AI_AGENT_TEST_SLOW_CHECKPOINT_MS` | unset (no delay) | **Debug builds only** (`#[cfg(debug_assertions)]`), and only for tests: makes every mid-run checkpoint write in `serve` take this many milliseconds longer, inside the write itself, standing in for a slow disk — so an acknowledgement sent before the write would be caught. `tests/serve_session_lifecycle.rs` uses it to prove a tool does not start before its `tool_use` is on disk. A release binary contains no latency-injection path at all. | -| `BEYOND_AI_AGENT_TEST_FAIL_CHECKPOINT` | unset (no delay) | **Debug builds only** (`#[cfg(debug_assertions)]`), and only for tests: fails `serve`'s checkpoint writes — `all` of them, or only the `n`th (a number), counted per session so a daemon's sessions do not shift each other's count (`serve_session_isolation`: `an_injected_checkpoint_failure_counts_each_sessions_writes`). `tests/serve_session_lifecycle.rs` uses it to prove a failed write before a tool fails closed (the tool never runs) and that a best-effort one is superseded. A release binary contains no fault-injection path and none of these names (`tests/release_seams.rs`). | +| `BEYOND_AI_AGENT_TEST_FAIL_CHECKPOINT` | unset (no delay) | **Debug builds only** (`#[cfg(debug_assertions)]`), and only for tests: fails `serve`'s checkpoint writes — `all` of them, or only the `n`th (a number), counted per session so a daemon's sessions do not shift each other's count (`serve_session_isolation`: `an_injected_checkpoint_failure_counts_each_sessions_writes`). `tests/serve_session_lifecycle.rs` uses it to prove a failed write before a tool fails closed (the tool never runs) and that a best-effort one is superseded. A release binary contains no fault-injection path and none of these names: CI's `build (release)` job scans every release binary for the `BEYOND_AI_AGENT_TEST_` prefix (`mise check:release-seams`, `scripts/scan-release-seams.sh`) and fails on any hit; `tests/release_seams.rs` is the fast source-level warning and pins the scanner. | | `BEYOND_AI_AGENT_TEST_SLOW_SESSION_END_MS` | unset (no delay) | **Debug builds only** (`#[cfg(debug_assertions)]`), and only for tests: delays a failed daemon session's `error` broadcast after its loop has ended. `tests/serve_service_session_end.rs` uses it to prove the reason still reaches the client before the close. A release binary contains no latency-injection path at all. | | `BEYOND_AI_AGENT_TEST_SLOW_ATTACH_MS` | unset (no delay) | **Debug builds only** (`#[cfg(debug_assertions)]`), and only for tests: delays a daemon connection's registration of its output. `tests/serve_service_session_end.rs` uses it to prove a connection registering after its session ended is still told why. A release binary contains no latency-injection path at all. | | `--idle-timeout-ms` / `AI_AGENT_IDLE_TIMEOUT_MS` (`run`-only) | `agent_core::client::READ_TIMEOUT` (~600s) | Task #19 (pi-parity feature): overrides the gateway HTTP client's idle-read timeout between response chunks (`GatewayClient::with_idle_timeout`, previously unused anywhere in this codebase) — matters most for a direct-routed/custom `models.json` `base_url` override, which bypasses the gateway's own upstream-timeout assumption entirely | @@ -4608,7 +4608,9 @@ mcp_events_subscribe (any session) ──► owned by that session the draft treats as "stop"). Before any state is read, a daemon with a callback URL answers every unknown token `503` from its start until each session it starts to restore callbacks (the events session, and every session holding runtime subscriptions) has read its state and held its tokens - (`webhook::restore_pending` / `restored`, also bounded at 10 min); after that an unknown token is + (`webhook::restore_pending` / `restored`, also bounded at 10 min) — or will not: a listed session + that fails to start, ends with an error, starts with no events hub, or panics is released from the + window at once; after that an unknown token is `410` at once. A runtime subscription the server ended is not restored, so its token is not held and its old callback answers `410`. No subscription makes a request — configured, restored after a restart or a panic, or resubscribing — until the session can take the server's questions (its diff --git a/crates/agent/scripts/scan-release-seams.sh b/crates/agent/scripts/scan-release-seams.sh new file mode 100755 index 00000000..51e8bd26 --- /dev/null +++ b/crates/agent/scripts/scan-release-seams.sh @@ -0,0 +1,26 @@ +#!/bin/sh +# Fail if any given build artifact carries a test seam's name. +# +# A test seam (`BEYOND_AI_AGENT_TEST_SLOW_CHECKPOINT_MS`, ...) changes how the binary behaves when an +# environment variable is set: fine in a debug test binary, never in a shipped one. Every seam is +# compiled only under `#[cfg(debug_assertions)]`, so a release binary must not contain the prefix at +# all. This scans the artifact itself: whatever the source looks like (a name assembled with +# `concat!`, one the source checker in `tests/release_seams.rs` misreads), a name the compiler kept +# is a contiguous string in the binary, and this finds it. CI runs it on every release binary the +# `build (release)` job produces (`mise check:release-seams`). +# +# Usage: scan-release-seams.sh ... +set -eu +if [ "$#" -eq 0 ]; then + echo "usage: $0 ..." >&2 + exit 2 +fi +status=0 +for artifact in "$@"; do + if grep -aq 'BEYOND_AI_AGENT_TEST_' "$artifact"; then + echo "$artifact carries test seam names (they must exist only in debug builds):" >&2 + grep -ao 'BEYOND_AI_AGENT_TEST_[A-Z0-9_]*' "$artifact" | sort -u | sed 's/^/ /' >&2 + status=1 + fi +done +exit "$status" diff --git a/crates/agent/src/serve.rs b/crates/agent/src/serve.rs index 7237921e..6a17166d 100644 --- a/crates/agent/src/serve.rs +++ b/crates/agent/src/serve.rs @@ -2931,6 +2931,10 @@ pub(crate) async fn serve_session( // server may raise a nested request during the very first `events/poll`, and asked before // then it would be declined as having no client. mcp_events = Some(hub); + } else { + // No events hub, so nothing of this session's will be restored: the daemon's restore + // window (`mcp_events::restore_pending`) must not wait on it. + crate::tools::mcp_events::restored(persistence.session_id()); } // SEP-2640 skills from every connected server, seen through this session's MCP gate. let mcp_skills = diff --git a/crates/agent/src/serve_ws.rs b/crates/agent/src/serve_ws.rs index bb54e75e..4aaf22aa 100644 --- a/crates/agent/src/serve_ws.rs +++ b/crates/agent/src/serve_ws.rs @@ -529,6 +529,9 @@ fn serve_session_body(base: ServeConfig) -> SessionBody { Box::pin(async move { if let Err(e) = serve_session(cfg, input_rx, out_conn, running, keep_alive).await { eprintln!("serve: session {id} ended: {e}"); + // Failed (to start, or later): whatever it would still have restored, the + // restore window does not wait on it. + crate::tools::mcp_events::restored(&id); simulated_slow_session_end().await; lock_ignoring_poison(&out_err).end(OutFrame::Value(json!({ "type": "error", @@ -1894,10 +1897,14 @@ pub async fn serve_ws( supervisor.unpin(&p.id, p.incarnation); eprintln!("serve: MCP Events session `{id}` owns the configured subscriptions"); } - Err(e) => eprintln!( - "serve: could not start the MCP Events session `{id}`: {}", - HttpError::from(e) - ), + Err(e) => { + eprintln!( + "serve: could not start the MCP Events session `{id}`: {}", + HttpError::from(e) + ); + // It will reserve nothing: the restore window must not wait on it. + crate::tools::mcp_events::restored(&id); + } } } @@ -1915,6 +1922,10 @@ pub async fn serve_ws( let mut backoff: std::collections::HashMap = std::collections::HashMap::new(); while let Some(id) = panicked_rx.recv().await { + // A panicked incarnation reserves nothing more, and one that panics on every start + // never will: the restore window does not wait on it. (Tokens it already reserved + // stay held; a restart reserves its own again.) + crate::tools::mcp_events::restored(&id); let holds_runtime = match &session_dir { Some(dir) => { let dir = dir.clone(); @@ -1974,10 +1985,13 @@ pub async fn serve_ws( supervisor.unpin(&p.id, p.incarnation); eprintln!("serve: restored session `{id}` for its MCP Events subscriptions"); } - Err(e) => eprintln!( - "serve: could not restore session `{id}` for its MCP Events subscriptions: {}", - HttpError::from(e) - ), + Err(e) => { + eprintln!( + "serve: could not restore session `{id}` for its MCP Events subscriptions: {}", + HttpError::from(e) + ); + crate::tools::mcp_events::restored(&id); + } } } } diff --git a/crates/agent/src/tools/mcp_events/mod.rs b/crates/agent/src/tools/mcp_events/mod.rs index 96a04799..0f34d73d 100644 --- a/crates/agent/src/tools/mcp_events/mod.rs +++ b/crates/agent/src/tools/mcp_events/mod.rs @@ -54,7 +54,9 @@ use crate::settings::{McpEventAction, McpEventDelivery, McpEventSubscription}; use crate::tools::mcp::McpCatalog; use state::{PendingEvent, PersistedSub, StateStore}; -pub use webhook::{MAX_WEBHOOK_BODY, WebhookReply, receive_webhook, restore_pending, route_exists}; +pub use webhook::{ + MAX_WEBHOOK_BODY, WebhookReply, receive_webhook, restore_pending, restored, route_exists, +}; pub use wire::NotificationRouter; use wire::{Conn, RpcError, StreamMsg}; diff --git a/crates/agent/src/tools/mcp_events/webhook.rs b/crates/agent/src/tools/mcp_events/webhook.rs index f3a5a2b3..94e9aa85 100644 --- a/crates/agent/src/tools/mcp_events/webhook.rs +++ b/crates/agent/src/tools/mcp_events/webhook.rs @@ -136,9 +136,10 @@ pub fn restore_pending(sessions: impl IntoIterator) { }); } -/// `session` has reserved every token it persisted. Once every session restore was waiting on has, -/// unknown tokens are `410` again. -pub(super) fn restored(session: &str) { +/// `session` has reserved every token it persisted — or will not: it failed to start, ended with no +/// events hub, or panicked. Once every session restore was waiting on is done either way, unknown +/// tokens are `410` again, rather than for the rest of [`RESERVE_FOR`]. +pub fn restored(session: &str) { let mut state = restoring_state(); if let Some(r) = state.as_mut() { r.sessions.remove(session); diff --git a/crates/agent/tests/mcp_events_receiver.rs b/crates/agent/tests/mcp_events_receiver.rs index 0eaa4c18..e8271e63 100644 --- a/crates/agent/tests/mcp_events_receiver.rs +++ b/crates/agent/tests/mcp_events_receiver.rs @@ -700,3 +700,88 @@ async fn a_session_with_runtime_subscriptions_is_restarted_after_a_panic() { ); assert_eq!(daemon_sessions(port).await.get("watcher"), Some(&true)); } + +/// Make a runtime webhook subscription in session `runtime-owner` on a first daemon, wait for its +/// spec and callback on disk, then kill that daemon: what a restart finds to restore. +async fn persist_a_runtime_subscription( + home: &std::path::Path, + base: &str, + held: &HeldPort, + mcp_url: &str, +) { + let mut servers = hooks(mcp_url, "notify"); + servers[0]["events"] = json!([]); + write_settings(home, servers); + let mut first = daemon(home, base, held, &[]); + let mut ws = ws_connect(held.port(), Some("runtime-owner")).await; + ws_send( + &mut ws, + json!({ "type": "mcp_events_subscribe", "id": "s", "server": "hooks", "name": "ticket.updated", + "delivery": "webhook", "action": "notify" }), + ) + .await; + let r = ws_next(&mut ws, Duration::from_secs(20), "the subscribe", |f| { + f["type"] == "response" && f["id"] == "s" + }) + .await; + assert_eq!(r["success"], true, "{r:#}"); + eventually( + Duration::from_secs(10), + "the runtime spec and its callback on disk", + || { + std::fs::read_dir(home.join("sessions")) + .ok()? + .map(|e| e.unwrap().path()) + .filter(|p| p.to_string_lossy().ends_with(".mcp-events.json")) + .any(|p| { + let text = std::fs::read_to_string(p).unwrap_or_default(); + text.contains("\"runtime\"") && text.contains("\"token\"") + }) + .then_some(()) + }, + ); + drop(ws); + first.kill().unwrap(); + let _ = first.wait(); +} + +/// The restore window must not wait on a session that will never reserve anything. Here the +/// session holding the runtime subscription gets no events hub on the restart (its server is no +/// longer configured): a token nothing holds is `410` again within moments, not after the window's +/// ten-minute bound. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_restoring_session_with_no_events_hub_closes_the_restore_window() { + let (_fx, mcp_url, _fixture) = spawn_http_fixture(&[("MCP_FIXTURE_ALLOW_HTTP_CALLBACK", "1")]); + let home = tempfile::tempdir().unwrap(); + let (base, _bodies) = spawn_model_server_routed(vec![], turn_text("noted")); + let held = HeldPort::bind(); + persist_a_runtime_subscription(home.path(), &base, &held, &mcp_url).await; + write_settings(home.path(), json!([])); + let _second = daemon(home.path(), &base, &held, &[]); + eventually(Duration::from_secs(10), "a dead token answered 410", || { + (dead_token_status(held.port()) == 410).then_some(()) + }); +} + +/// Likewise for a listed session that fails to start (here its transcript no longer opens). +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_restoring_session_that_fails_to_start_closes_the_restore_window() { + let (_fx, mcp_url, _fixture) = spawn_http_fixture(&[("MCP_FIXTURE_ALLOW_HTTP_CALLBACK", "1")]); + let home = tempfile::tempdir().unwrap(); + let (base, _bodies) = spawn_model_server_routed(vec![], turn_text("noted")); + let held = HeldPort::bind(); + persist_a_runtime_subscription(home.path(), &base, &held, &mcp_url).await; + let transcript = std::fs::read_dir(home.path().join("sessions")) + .unwrap() + .map(|e| e.unwrap().path()) + .find(|p| { + let name = p.file_name().unwrap().to_string_lossy().into_owned(); + name.ends_with("runtime-owner.jsonl") + }) + .expect("the session's transcript"); + std::fs::write(&transcript, b"not a session file\n").unwrap(); + let _second = daemon(home.path(), &base, &held, &[]); + eventually(Duration::from_secs(10), "a dead token answered 410", || { + (dead_token_status(held.port()) == 410).then_some(()) + }); +} diff --git a/crates/agent/tests/release_seams.rs b/crates/agent/tests/release_seams.rs index 2ef3c6f2..4072fa74 100644 --- a/crates/agent/tests/release_seams.rs +++ b/crates/agent/tests/release_seams.rs @@ -5,7 +5,13 @@ //! shipped one. Each lives in a function (or a match arm) under `#[cfg(debug_assertions)]`, with a //! no-op twin for release, so a release binary has no injection path and carries none of the names. //! A name written at a call site instead — `delay("BEYOND_AI_AGENT_TEST_…")` into a gated helper — -//! still compiles into release as a string the binary carries; this test fails on that. +//! still compiles into release as a string the binary carries. +//! +//! **The authority is the release binary itself**: CI's `build (release)` job runs +//! `scripts/scan-release-seams.sh` on every release binary it built (`mise check:release-seams`), +//! and fails on any `BEYOND_AI_AGENT_TEST_` string the compiler kept, however the source spelled it +//! (`concat!`, a `//` inside a string literal, anything a text heuristic misreads). The source check +//! here is only the fast early warning; the scanner's own behaviour is pinned below. #![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] /// Where a seam name may appear: in a function, or a match arm, marked `#[cfg(debug_assertions)]`. @@ -146,3 +152,40 @@ async fn body() { "; assert_eq!(ungated_seam_names("f.rs", gated_neighbour).len(), 1); } + +/// The artifact scanner CI trusts: it fails on a binary that carries seam names — this test's own +/// debug build of the agent does, by design — and passes one that does not. A name assembled at +/// compile time (`concat!`) is a contiguous string in the artifact, so it is caught the same way. +#[test] +fn the_release_artifact_scan_fails_on_any_seam_name() { + let script = + std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("scripts/scan-release-seams.sh"); + let scan = |artifact: &std::path::Path| { + std::process::Command::new("sh") + .arg(&script) + .arg(artifact) + .output() + .unwrap() + }; + let debug = scan(std::path::Path::new(env!("CARGO_BIN_EXE_beyond-ai-agent"))); + assert_eq!(debug.status.code(), Some(1), "{debug:?}"); + assert!( + String::from_utf8_lossy(&debug.stderr).contains("BEYOND_AI_AGENT_TEST_SLOW_CHECKPOINT_MS"), + "{debug:?}" + ); + let dir = tempfile::tempdir().unwrap(); + let clean = dir.path().join("clean"); + std::fs::write( + &clean, + b"\x7fELF BEYOND_AI_AGENT_MCP_IDLE_SECS nothing to see", + ) + .unwrap(); + assert_eq!(scan(&clean).status.code(), Some(0)); + let assembled = dir.path().join("assembled"); + std::fs::write( + &assembled, + concat!("\x7fELF ", "BEYOND_AI_AGENT_", "TEST_", "SLOW_X_MS").as_bytes(), + ) + .unwrap(); + assert_eq!(scan(&assembled).status.code(), Some(1)); +} diff --git a/mise.toml b/mise.toml index e28a8243..97da9d75 100644 --- a/mise.toml +++ b/mise.toml @@ -18,6 +18,10 @@ run = "cargo build" [tasks."build:rs:release"] run = "cargo build --release" +[tasks."check:release-seams"] +description = "Fail if a release binary carries a test seam's name (run after build:rs:release)." +run = "crates/agent/scripts/scan-release-seams.sh $(find target/release -maxdepth 1 -type f -perm -u+x)" + [tasks."ai:mint-dev-key"] description = "Mint a deterministic DEV-ONLY signing pubkey + bai_v1 token (fixed seed) for wiring up the dev config. Not for production." run = "cargo run -p beyond-ai --example mint_dev_key"