Skip to content
This repository was archived by the owner on Jul 4, 2026. It is now read-only.

fix(codex_app_server): make /stop actually stop the codex turn - #9

Merged
h4x3rotab merged 1 commit into
phalafrom
fix/codex-app-server-interrupt-propagation
May 24, 2026
Merged

fix(codex_app_server): make /stop actually stop the codex turn#9
h4x3rotab merged 1 commit into
phalafrom
fix/codex-app-server-interrupt-propagation

Conversation

@h4x3rotab

Copy link
Copy Markdown

Summary

Two related bugs in the codex_app_server interrupt path. Net effect today: `/stop` (and any gateway-side cancellation) does nothing on the codex_app_server runtime for up to 10 minutes, and the session lifecycle has races around the few interrupt paths that do work.

Bug 1 — `AIAgent.interrupt()` never propagated to the codex session

`CodexAppServerSession.request_interrupt()` is the public API for signalling the run-turn loop to bail. Tests call it; production never does. Grep:

```
$ grep -rn 'request_interrupt' --include='*.py' | grep -v test_ | grep -v pycache
agent/transports/codex_app_server_session.py:315: def request_interrupt(self) -> None:
```

Zero production callers. So when a user hits `/stop` (or the gateway cancels a Telegram conversation, etc.), `AIAgent.interrupt()` flips `self._interrupt_requested = True`, fans out signals to worker threads, walks child agents — but the codex subprocess never sees any of it. The turn runs until one of:

  • post-tool quiet watchdog (90 s after a tool completion)
  • outer deadline (600 s default)

That's the user-visible "stop doesn't stop" symptom on this runtime.

Fix: in `AIAgent.interrupt()`, after the existing fan-out, also call `self._codex_session.request_interrupt()` when a session is attached. The session's run-turn loop already checks `_interrupt_event` at the top of every iteration and issues `turn/interrupt` to codex when it sees it — we just had to wake it up.

Bug 2 — User interrupt + `<turn_aborted>` left `should_retire=False`

Three out of four "we're done early" paths set `result.should_retire = True`:

line trigger sets `should_retire`?
451 `_interrupt_event.is_set()` (user/gateway) no ← bug
481 post-tool quiet watchdog yes
567 / 510 `<turn_aborted>` marker no ← bug
605 outer deadline yes

The runtime caller in `agent/codex_runtime.py:98` only closes the session when `should_retire` is True. So a user-interrupted turn (or a `<turn_aborted>` one) leaves the codex subprocess alive while it's presumably still cleaning up the interrupted turn. The next `turn/start` races against that cleanup.

Fix: set `result.should_retire = True` on the user-interrupt path (1 line) and both `<turn_aborted>` paths (1 line each). Matches the existing post-tool and deadline behaviour.

Test plan

`tests/run_agent/test_interrupt_codex_session.py` (new, 4 tests):

  • propagation fires `request_interrupt()` exactly once when a session is attached
  • no-op when `_codex_session` attribute doesn't exist (default runtime / pre-first-turn)
  • a throwing `request_interrupt()` doesn't break the rest of the interrupt flow
  • `_codex_session = None` (post-retire) is handled cleanly

`tests/agent/transports/test_codex_app_server_session.py: TestSessionRetirement` (2 new tests):

  • user interrupt → `should_retire is True`
  • `<turn_aborted>` in agentMessage text → `should_retire is True`

`pytest tests/agent/transports/test_codex_app_server_session.py tests/run_agent/test_interrupt_codex_session.py tests/run_agent/test_concurrent_interrupt.py tests/run_agent/test_memory_sync_interrupted.py` → 81/81 pass (5 new + 76 pre-existing, no regressions).

Smaller follow-ups not in scope

These came up during the audit but aren't broken behaviour, just suboptimal:

  • The interrupt RPC blocks up to 5 s with codex wedged. Could lower the timeout or fire-and-forget.
  • The approval-drain loop doesn't check `_interrupt_event`. Sub-second window, low impact.
  • The drain-loop `<turn_aborted>` branch doesn't carry over `result.final_text` (the normal-loop sibling does). Cosmetic.

Happy to tackle these in a follow-up PR if you want.

🤖 Generated with Claude Code

Two related bugs in the codex_app_server interrupt path:

Bug 1: AIAgent.interrupt() never propagated to the codex session.
``CodexAppServerSession.request_interrupt()`` (the public API for
signalling the running turn loop to bail out) is called only from
tests. In production, ``/stop``, gateway-side cancellation, and
Ctrl+C all set ``self._interrupt_requested = True`` and fan out to
worker threads + child agents, but never reach the codex session.
The result: the codex subprocess keeps grinding on the active turn
until either the post-tool quiet watchdog (90 s) or the outer
deadline (600 s) fires — ``/stop`` effectively does nothing for up
to 10 minutes.

Bug 2: User interrupt + ``<turn_aborted>`` left ``should_retire=False``.
The other interrupt-out paths (post-tool watchdog, outer deadline,
turn/start auth failure) all set ``should_retire = True`` so the
caller in ``agent/codex_runtime.py`` retires the session and the
next turn respawns codex from scratch. The user-interrupt path and
the ``<turn_aborted>`` marker paths skipped this, leaving the
session alive while codex was presumably mid-cleanup of the
interrupted turn. The next ``turn/start`` then races against that
cleanup.

Fixes:

- ``run_agent.py: AIAgent.interrupt()``: after the existing fan-out
  to worker threads + child agents, also call
  ``self._codex_session.request_interrupt()`` when an active session
  exists. Defensive ``getattr(...) `` so the default-runtime path and
  pre-first-turn state both work; ``Exception`` swallowed so a weird
  session state can't break the rest of the interrupt flow.

- ``agent/transports/codex_app_server_session.py``: set
  ``result.should_retire = True`` on the user-interrupt path
  (line ~451) and both ``<turn_aborted>`` marker paths (normal
  loop ~573 and approval-drain ~518). Matches the post-tool and
  deadline paths.

Tests:

``tests/run_agent/test_interrupt_codex_session.py`` (new):
- ``test_interrupt_propagates_to_codex_session``: stub agent with
  a mock ``_codex_session``, call ``interrupt()``, assert
  ``request_interrupt`` was called exactly once.
- ``test_interrupt_is_noop_when_no_codex_session``: same but with
  no ``_codex_session`` attribute (default-runtime / pre-first-turn).
- ``test_interrupt_swallows_codex_session_exception``: if
  ``request_interrupt`` raises, the interrupt flow still completes.
- ``test_interrupt_after_session_closed_uses_session_attr_as_is``:
  when ``_codex_session = None`` (after retire), interrupt skips.

``tests/agent/transports/test_codex_app_server_session.py``:
- ``TestSessionRetirement.test_user_interrupt_marks_session_for_retirement``:
  call ``request_interrupt`` then ``run_turn``, assert
  ``should_retire is True``.
- ``TestSessionRetirement.test_turn_aborted_marker_marks_session_for_retirement``:
  feed an agentMessage containing ``<turn_aborted>``, assert
  ``should_retire is True``.

81/81 pass (5 new + 76 pre-existing across the touched test files,
no regressions).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@h4x3rotab
h4x3rotab merged commit dd57fea into phala May 24, 2026
4 checks passed
@h4x3rotab
h4x3rotab deleted the fix/codex-app-server-interrupt-propagation branch May 24, 2026 23:47
kingsleydon pushed a commit that referenced this pull request Jun 3, 2026
Two related bugs in the codex_app_server interrupt path:

Bug 1: AIAgent.interrupt() never propagated to the codex session.
``CodexAppServerSession.request_interrupt()`` (the public API for
signalling the running turn loop to bail out) is called only from
tests. In production, ``/stop``, gateway-side cancellation, and
Ctrl+C all set ``self._interrupt_requested = True`` and fan out to
worker threads + child agents, but never reach the codex session.
The result: the codex subprocess keeps grinding on the active turn
until either the post-tool quiet watchdog (90 s) or the outer
deadline (600 s) fires — ``/stop`` effectively does nothing for up
to 10 minutes.

Bug 2: User interrupt + ``<turn_aborted>`` left ``should_retire=False``.
The other interrupt-out paths (post-tool watchdog, outer deadline,
turn/start auth failure) all set ``should_retire = True`` so the
caller in ``agent/codex_runtime.py`` retires the session and the
next turn respawns codex from scratch. The user-interrupt path and
the ``<turn_aborted>`` marker paths skipped this, leaving the
session alive while codex was presumably mid-cleanup of the
interrupted turn. The next ``turn/start`` then races against that
cleanup.

Fixes:

- ``run_agent.py: AIAgent.interrupt()``: after the existing fan-out
  to worker threads + child agents, also call
  ``self._codex_session.request_interrupt()`` when an active session
  exists. Defensive ``getattr(...) `` so the default-runtime path and
  pre-first-turn state both work; ``Exception`` swallowed so a weird
  session state can't break the rest of the interrupt flow.

- ``agent/transports/codex_app_server_session.py``: set
  ``result.should_retire = True`` on the user-interrupt path
  (line ~451) and both ``<turn_aborted>`` marker paths (normal
  loop ~573 and approval-drain ~518). Matches the post-tool and
  deadline paths.

Tests:

``tests/run_agent/test_interrupt_codex_session.py`` (new):
- ``test_interrupt_propagates_to_codex_session``: stub agent with
  a mock ``_codex_session``, call ``interrupt()``, assert
  ``request_interrupt`` was called exactly once.
- ``test_interrupt_is_noop_when_no_codex_session``: same but with
  no ``_codex_session`` attribute (default-runtime / pre-first-turn).
- ``test_interrupt_swallows_codex_session_exception``: if
  ``request_interrupt`` raises, the interrupt flow still completes.
- ``test_interrupt_after_session_closed_uses_session_attr_as_is``:
  when ``_codex_session = None`` (after retire), interrupt skips.

``tests/agent/transports/test_codex_app_server_session.py``:
- ``TestSessionRetirement.test_user_interrupt_marks_session_for_retirement``:
  call ``request_interrupt`` then ``run_turn``, assert
  ``should_retire is True``.
- ``TestSessionRetirement.test_turn_aborted_marker_marks_session_for_retirement``:
  feed an agentMessage containing ``<turn_aborted>``, assert
  ``should_retire is True``.

81/81 pass (5 new + 76 pre-existing across the touched test files,
no regressions).

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant