fix(cli-tools): forward abortSignal to execAsync so a fired timeout actually kills the command - #681
Open
canblmz1 wants to merge 1 commit into
Open
Conversation
…ctually kills the command
Root cause: codepilot_cli_tools_install and codepilot_cli_tools_update call
execAsync(command, { timeout: 300_000, env }) without a `signal` option.
native-timeout.ts already documents why this matters: "ai@7 merely passes
the abort signal INTO execute() and still awaits its promise... the
consumer loop must iterate via guardStream to escape a tool that ignores
the signal." guardStream lets agent-loop.ts's own consuming loop move on
once a budget fires, but neither execute() destructured the SDK's
abortSignal nor passed anything to child_process.exec, so the real shell
command was never actually told to stop. The process keeps running to
completion (or Node's own hardcoded 300s timeout) in the background, after
the run has already been reported as timed out / aborted to the user and
the model.
Confirmed with a minimal reproduction (not just theoretical): calling
execAsync() with a long-running command, aborting an AbortController 400ms
in, with vs without `signal` wired to exec's options —
without signal: process still running 3s+ after abort() fires
with signal: process rejected (ABORT_ERR) within ~50ms of abort() firing
Fix: destructure `abortSignal` from execute()'s second parameter
(AI SDK's ToolCallOptions) in both codepilot_cli_tools_install and
codepilot_cli_tools_update, and pass it as `signal` to their execAsync()
calls. Node's child_process.exec has supported an AbortSignal-based
`signal` option since Node 15/16 — this wires an already-existing signal
through, it does not add new cancellation infrastructure. When no timeout
budget is configured, native-timeout.ts's own contract is that the
controller arms no timers, so abortSignal is simply undefined and exec()
behaves exactly as before — zero behavior change for the default,
unconfigured case.
Scope: only the two execAsync call sites with a real, long-running,
side-effecting shell command (install, update) are touched. The shorter,
read-only exec/execFile calls elsewhere in this file (--version checks,
`which`, `brew/npm outdated` queries, all capped at 5-30s) are left as-is
to keep this patch minimal and focused on the class of bug it fixes: an
externally-visible "timed out" result while a real command is still
running unsupervised.
cli-tools-mcp.ts (the SDK Runtime's separate implementation, per its own
header comment "the pure handler functions extracted from cli-tools-mcp.ts"
notwithstanding — the two files currently maintain independent execAsync
call sites) has the same gap but is intentionally not touched here, same
scoping precedent as PR op7418#676's Native-Runtime-only scope.
This is unrelated to prefix-safe-json / PR op7418#676's deferred-execution-
integrity fix (whether execute() should run at all based on the
surrounding stream's terminal state) — this fixes a different question
(once execute() has legitimately started, does aborting actually stop the
real side effect, or does it just orphan it in the background).
Tests: src/__tests__/unit/cli-tools-abort-signal.test.ts, 2 new tests —
one proving exec() honors a wired signal (kills the process on abort),
one control case reproducing the exact pre-fix behavior (an aborted
AbortController with no `signal` passed to exec() has no effect on the
running command). Both pass: `node --test src/__tests__/unit/cli-tools-abort-signal.test.ts`.
No existing test imports builtin-tools/cli-tools.ts directly (confirmed by
searching src/__tests__/unit for references before writing this patch),
so there is no existing coverage of this file to regress.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
codepilot_cli_tools_installandcodepilot_cli_tools_updaterun their realshell command via
execAsync(command, { timeout: 300_000, env })— nosignaloption.native-timeout.tsalready documents why this is a gap, in its own words:guardStreamsolves the consumer loop getting stuck. It does nothing forthe real process:
execute()never destructured the SDK'sabortSignal,and nothing was ever passed to
child_process.exec. So once atool-execution(or any other) budget fires — or a run is aborted for anyother reason — the agent loop moves on and reports the run as timed out /
aborted, while the actual install or update command keeps running,
unsupervised, in the background.
Reproduction
Not just theoretical — minimal, deterministic repro:
Both cases are in the new test file, as the "aborted" case and an explicit
control case reproducing the current (pre-fix) behavior.
Fix
Destructure
abortSignalfromexecute()'s second parameter (the AI SDK'sToolCallOptions, confirmed fromnode_modules/ai's own.d.ts:abortSignal?: AbortSignalonToolExecutionOptions) and pass it through assignaltoexecAsync()in both tools.child_process.exechas supportedan
AbortSignal-basedsignaloption since Node 15/16 — this wires analready-existing signal through; it adds no new cancellation machinery.
When no timeout budget is configured,
native-timeout.ts's own documentedcontract is "the controller arms no timers" — so
abortSignalis simplyundefinedin that case andexecAsyncbehaves exactly as it does today.Zero behavior change for the default, unconfigured path.
Scope
Only the two
execAsynccall sites with a real, potentially long-running,side-effecting command (install, update) are touched — 4 lines, 2 files.
The shorter read-only
exec/execFilecalls elsewhere in this file(
--versionchecks,which,brew/npm outdated, all capped 5-30s) areintentionally left alone to keep this patch minimal and focused on the exact
bug class it fixes.
cli-tools-mcp.ts(the SDK Runtime's separate implementation — the twofiles currently maintain independent
execAsynccall sites, despite thisfile's own header comment describing itself as the "single source of
truth") has the same gap but is intentionally not touched here, same
scoping precedent #676 used for Native-Runtime-only changes.
This is unrelated to #676 (deferred-execution-integrity — whether
execute()should run at all based on the surrounding stream's terminalstate). This fixes a different question: once
execute()has legitimatelystarted, does aborting it actually stop the real side effect, or does it
just orphan it in the background. Both can land independently.
Testing
src/__tests__/unit/cli-tools-abort-signal.test.ts(new): 2/2 passing —node --test src/__tests__/unit/cli-tools-abort-signal.test.tsexec()with a wiredsignalactually kills the process on abortexec()without asignal(today's behavior) — anaborted
AbortControllerhas no effect on the running commandbuiltin-tools/cli-tools.tsdirectly (checkedbefore writing this patch), so there's no prior coverage of this file to
regress.
aipackage's.d.ts(
abortSignal?: AbortSignalonToolExecutionOptions) rather thanassumed.
npx tsc --noEmit(full project): clean, 0 errors. (This took a longfirst-run pass in my sandbox — took long enough that I initially posted
this PR before it finished and said so; it has since completed cleanly,
updating that note now rather than leaving a stale caveat in place.)
No unrelated changes.