Skip to content

fix(esc): make ESC cancel Bash + streaming within ~50ms - #130

Merged
agentforce314 merged 1 commit into
mainfrom
fix/esc-cancellation-latency
May 14, 2026
Merged

fix(esc): make ESC cancel Bash + streaming within ~50ms#130
agentforce314 merged 1 commit into
mainfrom
fix/esc-cancellation-latency

Conversation

@agentforce314

@agentforce314 agentforce314 commented May 14, 2026

Copy link
Copy Markdown
Owner

Summary

ESC during a Bash command or model stream had to wait the full Bash timeout (10–60s) or the rest of a turn. Three stacked gaps:

  1. Event loop blocked: _call_tool ran sync tools inline (tool_execution.py:441). While Bash blocked, the ESC handler couldn't fire abort_controller.abort() at all.
  2. Unkillable Bash: _bash_call used subprocess.run(timeout=...) with no PID and no abort_signal reference (bash_tool.py:184). Even if abort fired, nothing killed the subprocess.
  3. Coarse streaming poll: _run_model_turn didn't take an abort signal; streaming_query polled only at the outer while-loop boundary (streaming.py:89). Long responses emitted every chunk before noticing the abort.

Fix

Mirrors typescript/src/utils/ShellCommand.ts (tree-kill on abort listener) + per-chunk signal checks from typescript/src/services/api/claude.ts:

File Change
src/services/tool_execution/tool_execution.py Wrap sync tools in asyncio.to_thread so the event loop stays responsive
src/tool_system/tools/bash/bash_tool.py Swap subprocess.run for Popen(start_new_session=True) + 50ms abort/timeout poll loop + os.killpg(SIGTERM) → 2s grace → SIGKILL
src/query/streaming.py Thread abort_signal into _run_model_turn; check .aborted per chunk from call_model; re-check after inner loop so aborted-mid-turn yields aborted not turn_complete

Sync entry to _bash_call is preserved so existing direct callers in skill.py and tests work unchanged.

Test plan

  • tests/test_tool_system_tools.py 70/70, tests/test_skills_shell_exec.py 13/13 (Bash sync entry intact)
  • tests/test_streaming_query_loop.py 10/10, tests/test_query_loop.py 5/5
  • tests/test_streaming_executor*.py + tests/test_stream_watchdog.py 29/29
  • tests/test_tool_execution_integration.py + tests/test_tool_orchestration.py + tests/test_orchestrator_concurrency.py + tests/parity/test_tool_execution_order.py 41/41
  • Bash smoke: sleep 30 aborted via abort_controller: 227ms end-to-end through _call_tool → to_thread → Popen → killpg (was ~30s)
  • Streaming smoke: 20×100ms chunks aborted at 250ms: 303ms to aborted event; no further text/turn_complete
  • Baseline: normal completion, timeout path, and non-zero exit-code paths preserved (57ms / 1018ms for 1s timeout / 59ms for exit 42)
  • In-terminal smoke during an actual REPL/TUI session

Out of scope

  • Streaming idle-timeout watchdog (TS has STREAM_IDLE_TIMEOUT_MS = 90_000). Separate concern, separate PR.
  • Threading abort_signal into call_model itself so we can tear down the HTTP stream — current per-chunk check is sufficient for the dominant case.

🤖 Generated with Claude Code

@agentforce314 @claude
fix(esc): make ESC cancel Bash + streaming within ~50ms
6d84c1d 
ESC presses had to wait the full Bash timeout (often 10–60s) or the
remainder of a model turn before having any effect. Three architectural
gaps were stacked:

1. ``_call_tool`` ran SYNCHRONOUS tools inline on the event loop, so
   while ``subprocess.run`` blocked, no other task — including the
   ESC key handler that fires ``abort_controller.abort()`` — could run.

2. ``_bash_call`` used ``subprocess.run(timeout=...)`` with no PID
   handle and no abort_signal reference. Even if abort were called,
   there was no path to kill the subprocess; the call returned only
   when the command finished or hit its timeout.

3. ``_run_model_turn`` did not take an abort_signal at all, and
   ``streaming_query`` only polled the signal at the OUTER while-loop
   boundary (between turns). A long model response would emit every
   chunk before the abort was noticed.

Fixes mirror the TS reference (``typescript/src/utils/ShellCommand.ts``
+ the ``shouldBypassPermissions`` / per-chunk abort checks in
``typescript/src/services/api/claude.ts``):

* ``_call_tool``: wrap sync tools in ``asyncio.to_thread`` so the event
  loop stays responsive while Bash runs.
* ``_bash_call``: replace ``subprocess.run`` with ``Popen(..., start_new_session=True)``
  + a 50ms poll loop that watches ``abort_signal.aborted`` and the
  timeout; on either, ``os.killpg(SIGTERM)`` → 2s grace → SIGKILL the
  whole process group. Sync entry preserved so ``skill.py``/tests keep
  working.
* ``_run_model_turn``: accept ``abort_signal``; check ``.aborted`` at the
  top of every event from ``call_model`` and return early.
  ``streaming_query`` re-checks after the inner loop so a turn that
  aborted mid-stream yields ``aborted`` instead of ``turn_complete``.

Smoke benchmarks:
* ``sleep 30`` aborted via context.abort_controller: **227ms** through
  the full orchestrator path (was ~30s).
* Streaming 100ms/chunk × 20 chunks aborted at 250ms: **303ms** until
  ``aborted`` event, no further ``text``/``turn_complete``.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@agentforce314
agentforce314 merged commit b02c8d7 into main May 14, 2026
@agentforce314 agentforce314 mentioned this pull request May 14, 2026
Merged
singlaamitesh pushed a commit to singlaamitesh/clawcodex that referenced this pull request Jul 7, 2026
@agentforce314
Merge pull request agentforce314#130 from agentforce314/fix/esc-cance…
2d21ea0 
…llation-latency

fix(esc): make ESC cancel Bash + streaming within ~50ms
singlaamitesh pushed a commit to singlaamitesh/clawcodex that referenced this pull request Jul 7, 2026
@agentforce314
docs(readme): add 2026-05-14 news entry for ESC cancellation fix
2ad5806 
Highlights PR agentforce314#130 (ESC cancels Bash + streaming within ~50ms) along
with the recent agentforce314#129 (diff color-bar full-width) and agentforce314#128
(bypass-permissions outside-paths) fixes. Appends current Python
codebase stats: 857 files, 167,034 lines.
singlaamitesh pushed a commit to singlaamitesh/clawcodex that referenced this pull request Jul 7, 2026
@ericleepi314 @claude
fix(bash): distinguish timeout from ESC-abort in tool_result
95cf6c1 
Before this change, the bash supervisor (`_run_bash_with_abort`) set
`interrupted=True` on BOTH ESC-abort and timeout. The two paths then
collapsed into one tool_result shape: content ending with
`<error>Command was aborted before completion</error>` and
`is_error=True`. The model treated timed-out commands as
user-cancelled and tended to retry them on resume.

The TS reference at `typescript/src/utils/ShellCommand.ts:135-141, 302,
323-328` and `typescript/src/tools/BashTool/BashTool.tsx:610-630` keeps
the two paths distinct via the result-label exit code:

* ESC abort: SIGKILL → exit code 137 → `interrupted=true` →
  `is_error=true` → `<error>Command was aborted before completion</error>`
  appended.
* Timeout: SIGKILL (kill is the same; only the label differs in TS at
  `#doKill(SIGTERM)`) → exit code label 143 → `interrupted=false` →
  `is_error=false` → stderr prepended with
  `Command timed out after <duration>`; no `<error>` tag.

Mirror this in Python:

* `_BashRunResult.interrupted` is now set only on ESC-abort; the
  timeout case sets only `timed_out=True`.
* Both paths SIGKILL the process group immediately (TS's `#doKill`
  always calls `treeKill(pid, 'SIGKILL')` regardless of the `code`
  label argument). The previous SIGTERM-with-grace-then-SIGKILL
  escalation is gone for parity and to keep ESC latency under the
  ~50ms target tracked by PR agentforce314#130.
* `_bash_call` produces two distinct payloads: the ESC branch keeps
  `interrupted=True, exit_code=-1, is_error=True`; the new timeout
  branch sets `timed_out=True, exit_code=143, is_error=False`,
  prepends the duration marker (formatted via
  `format_duration(timeout_s * 1000)` for TS-byte parity — `30s`,
  `2m 0s`, `1h 5m 12s`) to whatever stderr was captured with a
  single-space separator (mirrors TS `prependStderr` at
  `ShellCommand.ts:56-58`), and leaves stderr otherwise unmodified
  (no `.strip()`).
* `_KILL_GRACE_S` is renamed to `_KILL_REAP_TIMEOUT_S` to reflect its
  new role: a bound on the post-SIGKILL kernel-reap wait, not a
  SIGTERM→SIGKILL grace.
* `_bash_map_result_to_api` is unchanged. Its existing
  `interrupted = output.get("interrupted", False)` check naturally
  handles the split because timeout payloads no longer carry the
  `interrupted` key — only the ESC path appends the `<error>` tag and
  sets `is_error=True`.

Pinned by 7 tests in `tests/test_bash_timeout_vs_esc.py` covering the
supervisor return values, the `_bash_call` ToolResult shapes, and the
`_bash_map_result_to_api` API-block emissions for both paths.

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@agentforce314