fix(agent-computer): honour a Stop that already landed - #733
Merged
davidmckayv merged 2 commits intoOct 5, 2026
Merged
davidmckayv merged 2 commits into
davidmckayv merged 2 commits into
Conversation
run() attached an abort listener and stopped the process group when it
fired, which is the whole of the person's Stop reaching the command:
const onAbort = stop;
input.signal?.addEventListener("abort", onAbort, { once: true });
An abort listener added to an already-aborted signal never fires, so this
only honoured a Stop arriving after that line. The abort can beat the spawn
above it: the surface aborts, the server aborts the request it made to this
computer, and Bun aborts this one in turn, which happens whenever the person
was quick. The command then ran to its own limit, the reply said nothing
about a Stop, and the person had no answer until it finished.
Reading the flag after subscribing is what makes a Stop mean the same thing
whenever it landed. The server already does this on its side before it
fetches, for the same reason.
Both cases are covered: a Stop that landed first, and one that lands
mid-command, which already worked and is pinned so the refactor keeps it.
aniruddhaadak80
requested review from
MikeRyanDev,
davidmckayv,
guidovizoso,
mxmzb and
tylerslaton
as code owners
October 4, 2026 09:04
# Conflicts: # CHANGELOG.md
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 free
to 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.
What this changes
shell.runinagent-computer/src/shell.tsis how a person's Stop reaches the command itself, not just the HTTP request that started it. It did that with an abort listener:An abort listener added to an already-aborted signal never fires. So this only ever honoured a Stop that arrived after that line.
The abort can beat the spawn above it.
POST /execpassesrequest.signalstraight through (agent-computer/src/index.ts:1168), and the file already documents the whole chain atindex.ts:1266-1267:Whenever the person was quick enough, that chain completed before
runreached the spawn. The command then ran to its owntimeoutMs,timedOutstayedfalse, and the reply carried no indication that anyone had pressed Stop. The person got no answer until the command finished on its own.This change reads the flag after subscribing, so a Stop means the same thing whenever it landed. The server already takes exactly this precaution on its side before it fetches (
server/src/computer/client.ts:192), for the same reason — an existing precedent rather than a new idea.Two tests, because only one of the two cases was broken:
main: the command runs its full 30 s and the assertion on elapsed time fails.Where it runs
stop()and the timer already existed; this only decides whenstop()is called. No state is added, and nothing is retained between calls.agent-computerprocess, holding no cross-request state. A Bot's computer is one process per Bot, so there is no second replica to disagree with here — and nothing new that a replica would have to share.Boundary and audit
Every acting call still goes through the gateway: resolve, decide, audit, then act.
Not an acting call. This is a Stop, which is the absence of an action — it kills the process group earlier than before. It cannot make a command act that would not have acted.
New refusals and new failures each write a row.
No new refusal or failure. A stopped command still resolves through the same
{ command, exitCode, stdout, stderr, truncated, timedOut }return, soPOST /execreports it exactly as it did for a mid-flight Stop.Nothing new is trusted from the client that the server can resolve itself.
Nothing new is read. The only value consulted is the same
AbortSignalthe server's own request already carries.Changelog
A line in
CHANGELOG.mdunderUnreleased.Added. The deployment-visible difference is that a Stop now takes effect immediately rather than at the end of the command.
Proof
Please read this part before judging the tests. This suite is POSIX-only and I could not execute it meaningfully on the machine I work on (Windows, no
bash/sh, and the file hardcodesHOME=/root,sleep,/dev/zero). 11 of the 25 tests inshell.test.tsalready fail onmainlocally for that reason, before any change of mine. So I am not claiming a green local run of this file, and my two new tests are not meaningful evidence on Windows — the spawned command fails instantly there regardless of the fix.What I did run:
bun test agent-computer/tests/shell.test.ts— 16 pass, 11 fail, against amainbaseline of 14 pass, 11 fail for the same file. The 11 failures are identical and pre-existing; my two additions account for the 14 → 16. Nothing regressed, but as above this is not the verification that matters.bun run typecheckinagent-computer/— exit 0, no diagnostics.bunx biome lint --error-on-warningsandbunx biome formaton both changed files — clean.The premise I was able to verify platform-independently, which is the part that makes this a bug rather than a guess — an abort listener on an already-aborted signal never fires, while one added before the abort does:
So the mid-flight path works today and the pre-abort path cannot. That is the whole defect, and it is a property of
AbortSignalrather than of this platform.The two new tests follow the wall-clock pattern the neighbouring tests in this file already use (
a command that backgrounds a process is still stoppedassertsDate.now() - startedis under 10 s), so they are written to be meaningful on theubuntu-latestrunner that CI'stestjob uses. I would rather say plainly that I expect those two tests to go red onmainand green here on CI than claim I watched them do it. If they do not, the fix is wrong and the test is telling the truth about it.