diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b0f4aae54..6e41ba9cc 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -455,10 +455,11 @@ jobs: # Config-heavy async test frames (same root cause as the daemon-startup # stack overflow fixed in #5394); tests that build more than one Agent/ # ServeAgentDeps in the same frame (e.g. - # build_agent_factory_gates_trust_state_independently_per_session) can - # overflow the default per-test thread stack under instrumentation even - # though they pass in the non-instrumented `test` job. Give test threads - # a larger stack here rather than in every other job. + # build_agent_factory_gates_trust_state_independently_per_session, which as of + # #6699 spawns its own 32 MiB-stack thread and so no longer depends on this var + # itself) can overflow the default per-test thread stack under instrumentation + # even though they pass in the non-instrumented `test` job. Keep this set — it + # protects any other, unnamed multi-agent test that doesn't manage its own stack. RUST_MIN_STACK: "33554432" steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 diff --git a/CHANGELOG.md b/CHANGELOG.md index 16adf6825..7f350170f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -209,6 +209,24 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). - RC-4 (AutoSkill draft-name collisions with native tool IDs) is tracked separately in #6702 and intentionally out of scope here. +- `crates/zeph-core`/`src/serve/agent_factory.rs`: `cargo test --features full` could crash with + a genuine stack overflow (`SIGABRT`) on + `serve::agent_factory::tests::build_agent_factory_gates_trust_state_independently_per_session` + (issue #6699) — not the reporter's suspected `Config::default()` size growth, but + `VigilGate::try_new` (`crates/zeph-core/src/agent/vigil.rs`) recompiling the entire bundled + 14-pattern injection bank via `regex::Regex::new()` on every `Agent` construction, the only one + of 7 consumers of `RAW_INJECTION_PATTERNS` that didn't already cache the compile behind a + `LazyLock` (matching `zeph-sanitizer`, `zeph-mcp`, `zeph-skills`). One bundled pattern triggers + `regex_automata`'s SIMD Teddy-prefilter builder, whose frame overflowed the default 2 MiB test + thread stack when reached ~25 frames deep through this test's two-`Agent`, `--features + full`-inflated (`AnyProvider`'s unboxed `Candle`/`Gonka`/`Cocoon` variants) construction chain. + Cached the bundled-pattern compile behind a process-wide `static ...: LazyLock>` in + `vigil.rs` (a genuine perf fix independent of the crash — removes O(sessions) redundant + compilation) and gave the specific test a dedicated 32 MiB-stack thread (matching + `MAIN_THREAD_STACK_SIZE` in `src/main.rs`, #5394, and the `coverage` job's existing + `RUST_MIN_STACK: "33554432"` from commit `9a1efb89a`, which this test now also self-provides + rather than relying solely on the CI env var). + - `zeph-channels`: `MAX_RETRY_SECS` (the upper bound `send_with_retry` clamps a `Retry-After` delay to) had no compile-time invariant guard (issue #6517). #6516 (closing #6496) filtered the *parsed* `Retry-After` values so a negative/non-finite header or body field could no diff --git a/crates/zeph-core/src/agent/vigil.rs b/crates/zeph-core/src/agent/vigil.rs index 04931a772..881ca725d 100644 --- a/crates/zeph-core/src/agent/vigil.rs +++ b/crates/zeph-core/src/agent/vigil.rs @@ -24,6 +24,7 @@ //! - Retry-safe block semantics so a poisoned page does not trigger a fetch retry loop. use std::collections::HashSet; +use std::sync::LazyLock; use regex::Regex; use zeph_common::patterns::RAW_INJECTION_PATTERNS; @@ -37,6 +38,24 @@ struct CompiledPattern { regex: Regex, } +struct BundledPattern { + name: &'static str, + regex: Regex, +} + +/// Compiled bundled injection patterns, compiled once per process instead of once per +/// [`VigilGate::try_new`] call. `Regex::clone` is a cheap `Arc` bump, so every gate +/// construction reuses this compile. +static BUNDLED_PATTERNS: LazyLock> = LazyLock::new(|| { + RAW_INJECTION_PATTERNS + .iter() + .map(|(name, pat)| BundledPattern { + name, + regex: Regex::new(pat).expect("bundled patterns are valid"), + }) + .collect() +}); + #[non_exhaustive] /// Action to take when VIGIL flags a tool output. #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -92,11 +111,11 @@ impl VigilGate { pub fn try_new(config: VigilConfig) -> Result { config.validate()?; - let mut patterns: Vec = RAW_INJECTION_PATTERNS + let mut patterns: Vec = BUNDLED_PATTERNS .iter() - .map(|(name, pat)| CompiledPattern { - name: (*name).to_owned(), - regex: Regex::new(pat).expect("bundled patterns are valid"), + .map(|bp| CompiledPattern { + name: bp.name.to_owned(), + regex: bp.regex.clone(), }) .collect(); diff --git a/src/serve/agent_factory.rs b/src/serve/agent_factory.rs index c22b09c0b..c65cda774 100644 --- a/src/serve/agent_factory.rs +++ b/src/serve/agent_factory.rs @@ -1894,6 +1894,13 @@ mod tests { } } + /// Stack size for [`build_agent_factory_gates_trust_state_independently_per_session`]'s + /// dedicated test thread. Matches the `coverage` job's `RUST_MIN_STACK` in + /// `.github/workflows/ci.yml` (set for this same test under `-C instrument-coverage`, + /// commit `9a1efb89a`) — `Builder::stack_size` takes precedence over `RUST_MIN_STACK`, so + /// this must stay at least as large or it would silently shrink that protection back down. + const TEST_THREAD_STACK_SIZE: usize = 32 * 1024 * 1024; + /// R3 (SEC-H1 guardrail, #5973/#5977): `build_agent_factory` must give each session its /// OWN `TrustGateExecutor` trust-state instance — not share one gated executor (and its /// single mutable `effective_trust` atomic) across every `/sessions` agent built from the @@ -1903,8 +1910,38 @@ mod tests { /// A's), and asserts A's `bash` (`QUARANTINE_DENIED`) call is STILL Blocked. This test FAILS /// if `ServeAgentDeps::tool_executor`/`build_agent_factory` reverts to gating once, eagerly, /// in `assemble_serve_deps` instead of per session. - #[tokio::test] - async fn build_agent_factory_gates_trust_state_independently_per_session() { + /// + /// Runs the test body on a dedicated large-stack thread instead of directly under + /// `#[tokio::test]`. This test keeps TWO full `Agent`s alive simultaneously plus a large + /// `Config` (see #6699): in an unoptimized build, the self-by-value setter chain in + /// `agent_setup.rs`/`agent_factory.rs` (~120 calls) that constructs each `Agent`, combined + /// with `--features full`'s unboxed `AnyProvider` variants (`Candle`/`Gonka`/`Cocoon`), + /// consumes most of the default 2 MiB test-thread stack before a downstream `regex_automata` + /// SIMD prefilter build (`VigilGate::try_new`) is reached, overflowing it. Same pattern as + /// `MAIN_THREAD_STACK_SIZE` in `src/main.rs` (#5394) — a large/deep-frame condition, not + /// unbounded recursion. + #[test] + fn build_agent_factory_gates_trust_state_independently_per_session() { + let result = std::thread::Builder::new() + .name("agent-factory-trust-state-test".into()) + .stack_size(TEST_THREAD_STACK_SIZE) + .spawn(|| { + tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("failed to build test runtime") + .block_on( + build_agent_factory_gates_trust_state_independently_per_session_body(), + ); + }) + .expect("failed to spawn test thread") + .join(); + if let Err(e) = result { + std::panic::resume_unwind(e); + } + } + + async fn build_agent_factory_gates_trust_state_independently_per_session_body() { let memory = make_memory().await; let cid_a = memory.sqlite().create_conversation().await.unwrap(); let cid_b = memory.sqlite().create_conversation().await.unwrap();