Skip to content

fix(interrupt): per-agent interrupt event to prevent cross-session contamination - #6186

Closed
malaiwah wants to merge 1 commit into
NousResearch:mainfrom
malaiwah:fix/per-agent-interrupt
Closed

fix(interrupt): per-agent interrupt event to prevent cross-session contamination#6186
malaiwah wants to merge 1 commit into
NousResearch:mainfrom
malaiwah:fix/per-agent-interrupt

Conversation

@malaiwah

@malaiwah malaiwah commented Apr 8, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #6185. tools/interrupt.py exposed a single process-wide
_interrupt_event = threading.Event() shared by every concurrent
AIAgent. When two agents run simultaneously in the same process —
two /v1/chat/completions clients on the API server, or a gateway
runner with multiple active sessions — one agent's interrupt() set
the global event and every other concurrent agent's tools (terminal,
web_extract, browser, vision, all environment runners) returned
[interrupted] immediately, producing truncated mid-task completions
on unrelated requests.

The reverse race existed too: any other request whose
run_conversation() started would call clear_interrupt()
_set_interrupt(False), silently un-interrupting an agent that
another caller had legitimately stopped.

Per-instance self._interrupt_requested only guarded the LLM API-call
polling loop in _interruptible_api_call; the 11 long-running tools
that import is_interrupted from tools.interrupt all observed the
singleton, so the isolation broke at the tool boundary.

Issue #6185 has the full reproduction and a list of the affected
tools. PR #3427 (merged) made the SSE-disconnect path the most
reliable trigger.

Approach

  • Each AIAgent now owns its own threading.Event
    (self._interrupt_event).
  • tools/interrupt.py adds a contextvars.ContextVar that holds the
    currently active agent's event. is_interrupted() and
    set_interrupt() consult the bound event when present, falling
    back to the historical module-level singleton when none is bound
    (CLI single-agent usage and tests that import _interrupt_event
    directly continue to work unchanged).
  • AIAgent.run_conversation() becomes a thin wrapper that binds
    self._interrupt_event to the context variable, calls the renamed
    _run_conversation_locked() (the original body), and unbinds in a
    finally.
  • AIAgent.interrupt() and clear_interrupt() write directly to
    self._interrupt_event rather than going through set_interrupt,
    because they are typically invoked from a different thread (gateway
    message handler, SSE disconnect handler) where the contextvar may
    be unbound or bound to a different agent.
  • _execute_tool_calls_concurrent snapshots the current context with
    contextvars.copy_context() and hands each ThreadPoolExecutor
    worker a fresh .copy() to run() the tool in. Without this,
    worker threads would not inherit the binding, since neither
    ThreadPoolExecutor.submit nor asyncio.loop.run_in_executor
    propagates contextvars automatically.

The 11 tool modules that import is_interrupted need no edits — the
per-agent binding is fully transparent to them.

Test plan

New tests/tools/test_interrupt_isolation.py adds 8 regression tests
covering:

  • The bind / unbind API (per-agent event isolated from global, set/unbind restores fallback, set_interrupt routes to bound event, set_interrupt falls back to global when unbound)
  • Two-context independence simulating two concurrent API requests
  • ThreadPoolExecutor workers inheriting the correct event via copy_context().run
  • End-to-end AIAgent cross-contamination: interrupting agent A does not affect agent B; clearing one agent's interrupt does not clear the other's

Existing tests/run_agent/test_interrupt_propagation.py updated:

  • The previous test_child_clear_interrupt_at_start_clears_global
    asserted the bug-as-feature; it now asserts the new isolation
    contract under the name
    test_child_clear_interrupt_does_not_affect_global.
  • All bare-constructed mock agents in test_interrupt_propagation,
    test_real_interrupt_subagent, and test_cli_interrupt_subagent
    receive a _interrupt_event = threading.Event().

tests/run_agent/test_run_agent.py:

  • TestInterrupt and TestHydrateTodoStore drop the now-unused
    patch("run_agent._set_interrupt") context managers (the import
    alias is gone).
  • TestMemoryNudgeCounterPersistence::test_counters_not_reset_in_preamble
    and TestDeadRetryCode::test_no_unreachable_max_retries_after_backoff
    now inspect.getsource(AIAgent._run_conversation_locked) since
    run_conversation is the binding wrapper and no longer holds the
    preamble or the retry loop.

Test results

  • 42 interrupt-related tests pass on this branch (8 new isolation
    tests, plus all pre-existing tools/test_interrupt.py,
    run_agent/test_interrupt_propagation.py,
    run_agent/test_real_interrupt_subagent.py,
    run_agent/test_interactive_interrupt.py,
    run_agent/test_exit_cleanup_interrupt.py,
    cli/test_cli_interrupt_subagent.py,
    gateway/test_interrupt_key_match.py).
  • Full tests/run_agent/test_run_agent.py (228 tests) and
    tests/gateway/test_api_server*.py (141 tests) pass.
  • Wider sweep of tests/run_agent/, tests/tools/, tests/cli/,
    and tests/gateway/test_api_server*.py: 3558 pass, 19 pre-existing
    failures (verified by re-running on main HEAD ff6a86cb). The
    pre-existing failures are environment-dependent
    (test_docker_environment, test_file_tools_live,
    test_managed_media_gateways, test_transcription,
    test_vision_tools::test_check_requirements_accepts_codex_auth,
    test_quick_commands) and unrelated to interrupt handling.

Backwards compatibility

  • The is_interrupted() and set_interrupt() public API is
    unchanged.
  • The module-level _interrupt_event is preserved as a fallback so
    any external caller that imports it directly continues to work.
  • All existing CLI and single-agent gateway code paths behave
    identically: a single agent in a process binds its own event and
    observes its own signal — exactly the same outward behaviour as
    before, but isolated from other agents that might run alongside.

cc #4072 #3427

🤖 Generated with Claude Code

…ntamination

`tools/interrupt.py` exposed a single process-wide
`_interrupt_event = threading.Event()` shared by every concurrent
`AIAgent` in the process. When one agent's `interrupt()` was called
(e.g. an SSE client disconnecting on the API server), the global event
was set and every other concurrently-running agent's tools — terminal,
web_extract, browser, vision, all environment runners — observed
`is_interrupted() == True` and returned `[interrupted]` immediately,
producing truncated mid-task completions on unrelated requests.

The reverse race existed too: any other request whose
`run_conversation()` started would call `clear_interrupt()` →
`_set_interrupt(False)`, silently un-interrupting an agent that another
caller had legitimately stopped.

The session isolation at `self._interrupt_requested` only guarded the
LLM API-call polling loop; the 11 long-running tools that import
`is_interrupted` from `tools.interrupt` all observed the singleton.

Fix:

- Each `AIAgent` now owns its own `threading.Event`
  (`self._interrupt_event`).
- `tools/interrupt.py` adds a `contextvars.ContextVar` that holds the
  currently-active agent's event. `is_interrupted()` and
  `set_interrupt()` consult the bound event when present, falling back
  to the historical module-level singleton when none is bound (CLI
  single-agent usage and tests that import `_interrupt_event` directly
  keep working unchanged).
- `AIAgent.run_conversation()` becomes a thin wrapper that binds
  `self._interrupt_event` to the context variable, calls the renamed
  `_run_conversation_locked()` (the original body), and unbinds in a
  `finally`.
- `AIAgent.interrupt()` and `clear_interrupt()` write directly to
  `self._interrupt_event` rather than going through `set_interrupt`,
  because they are typically invoked from a different thread (gateway
  message handler, SSE disconnect handler) where the contextvar may be
  unbound or bound to a different agent.
- `_execute_tool_calls_concurrent` snapshots the current context with
  `contextvars.copy_context()` and hands each `ThreadPoolExecutor`
  worker a fresh `.copy()` to `run()` the tool in. Without this,
  worker threads would not inherit the binding, since neither
  `ThreadPoolExecutor.submit` nor `asyncio.loop.run_in_executor`
  propagates contextvars automatically.

Tests:

- New `tests/tools/test_interrupt_isolation.py` adds 8 regression
  tests covering the bind/unbind API, two-context independence,
  ThreadPoolExecutor inheritance, and end-to-end `AIAgent` cross-
  contamination scenarios.
- Existing `tests/run_agent/test_interrupt_propagation.py` updated:
  the previous `test_child_clear_interrupt_at_start_clears_global`
  asserted the bug-as-feature; it now asserts the new isolation
  contract under the name
  `test_child_clear_interrupt_does_not_affect_global`.
- All bare-constructed mock agents in `test_interrupt_propagation`,
  `test_real_interrupt_subagent`, and `test_cli_interrupt_subagent`
  receive a `_interrupt_event = threading.Event()`.
- `TestInterrupt` and `TestHydrateTodoStore` in `test_run_agent.py`
  drop the now-unused `patch("run_agent._set_interrupt")` context
  managers (the import alias is gone).
- `TestMemoryNudgeCounterPersistence::test_counters_not_reset_in_preamble`
  and `TestDeadRetryCode::test_no_unreachable_max_retries_after_backoff`
  now `inspect.getsource(AIAgent._run_conversation_locked)` since
  `run_conversation` is the binding wrapper and no longer holds the
  preamble or the retry loop.

Reported in NousResearch#4072 (thread safety) and NousResearch#3427 (the merged SSE-disconnect
PR that made this trigger reliable on multi-client API server
deployments).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the detailed write-up and the ContextVar approach — the per-agent interrupt isolation problem you identified in #6185 is real and the analysis is solid.

This is an automated hermes-sweeper review.

The fix landed on main three days after this PR was opened, via PR #7930 (commit dfc820345), which took a different but equivalent path:

  • tools/interrupt.py now uses _interrupted_threads: set[int] + a lock instead of a singleton threading.Event. is_interrupted() checks threading.current_thread().ident; set_interrupt(active, thread_id) targets a specific thread.
  • run_agent.py stores self._execution_thread_id at run_conversation() start and fans out set_interrupt(True/False, tid) to each concurrent tool worker via self._tool_worker_threads — covering the ThreadPoolExecutor propagation concern your PR addresses with copy_context().
  • TestPerThreadInterruptIsolation in tests/run_agent/test_interrupt_propagation.py (line 170) already covers the two-agent cross-contamination contract.

The ContextVar design has merit for a future async-first tool dispatch, but the functional gap is closed on main. Closing as implemented.

@teknium1 teknium1 closed this Apr 27, 2026
@alt-glitch alt-glitch added type/bug Something isn't working P1 High — major feature broken, no workaround comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/tools Tool registry, model_tools, toolsets duplicate This issue or pull request already exists labels Apr 30, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Likely duplicate of merged #7930 — same root cause (process-wide interrupt singleton). #7930 scoped interrupt per-thread; this PR uses contextvars instead.

@alt-glitch

Copy link
Copy Markdown
Collaborator

Likely duplicate of merged #7930.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/tools Tool registry, model_tools, toolsets duplicate This issue or pull request already exists P1 High — major feature broken, no workaround type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Process-wide interrupt singleton causes cross-session contamination on multi-client deployments

3 participants