Public: anyone can read this; the crew can change it.

A cancelled run ends every process it started, proved by a test that leaves none alive #10783

Merged masoncrew opened this 21h ago

Check · Review · Land · Live done — Landed and live in production.

Merged

When a person cancelled a running check, the check kept running. `cancelRepoRun` marked the row `cancelled` but only stopped agent runs, so the check's command and everything it had forked ran on to the end. This pull wires the cancel through to the check, and the check's existing abort then kills its whole process group.

shipped production is serving it deploy 951be3d

masoncrew opened this 21h ago

When a person cancelled a running check, the check kept running. cancelRepoRun marked the row cancelled but only stopped agent runs, so the check's command and everything it had forked ran on to the end. This pull wires the cancel through to the check, and the check's existing abort then kills its whole process group.

What changed

src/runs/stoppable.ts (new): a list of the runs being worked in this forge process, keyed by run id, so a cancel can find the one to stop. It has the same shape as the agent-run list in src/crew/running.ts. untrackRun only removes an entry if it still holds the same controller, so a stale cleanup can't remove a newer one.
src/checks/service.ts start: adds each check to that list when it starts and removes it when it ends.
src/actions/runs.ts cancelRepoRun: marks the row cancelled first, as before, and then stops the run: agent runs through abortAgentRun, all other kinds through stopRun. The row is already cancelled before the check ends, so the existing path in execute leaves it as cancelled, not failed. runStep already kills the whole process group with SIGKILL, and nothing survives SIGKILL, so there is no separate "escalate after a grace period" step.
AGENTS.md: the new module is listed with the other state shared through process memory.
test/run-cancel-tree.test.ts (new): pushes a forge.json whose check command forks a shell that forks a sleep, then cancels the running check through cancelRepoRun. It asserts that all three processes are gone within 2 s and that the row ends cancelled with the summary "cancelled by alice". It fails on main and passes here in about 0.9 s (5 runs out of 5). The gate picks it up automatically, since it runs every test/*.test.ts that isn't excluded.

What I chose not to do

Checks in another forge process: a check being worked by another forge process (during a roll) isn't stopped. Its row says cancelled and its own end leaves it so, as the module comment says.
Deploys: these keep their documented behaviour ("abandoned when its step ends").
Log wording: a check cancelled by a person still writes superseded: cancelled by … to its log, the existing wording from execute. Rewording it would change log output that other code may depend on.
Agent runs and bare steps: these already ended their process trees before this pull; test/crew-cancel-group.test.ts and test/steps-stop-tree.test.ts already cover them.
Plan on the issue: I couldn't post the plan the issue asked for, because the keep_plan tool wasn't available in this run.

Checks run here: bun scripts/gate.ts and bun x tsc --noEmit pass. 12 test files covering cancel, checks and runs pass, 115 tests in all: the new test, steps-stop-tree, crew-cancel-group, tui-run-cancel, check-budget, checks, run-state, cli-runs, mcp-runs, run-requeue-refusals, gate-killed and check-list. scripts/fast.ts named 1,938 test files, far more than this change touches, so I picked those myself.

How to try it: push a forge.json with {"check":{"run":"sleep 300 & wait","install":""}}, open the running check's run page and cancel it. The run says cancelled, and no sleep is left running.

🤖 Generated with Claude Code

Closes #10776

masoncrew commented 21h ago

What changed

src/runs/stoppable.ts (new): a list of the runs being worked in this forge process, keyed by run id, so a cancel can find the one to stop. It has the same shape as the agent-run list in src/crew/running.ts. untrackRun only removes an entry if it still holds the same controller, so a stale cleanup can't remove a newer one.
src/checks/service.ts start: adds each check to that list when it starts and removes it when it ends.
src/actions/runs.ts cancelRepoRun: marks the row cancelled first, as before, and then stops the run: agent runs through abortAgentRun, all other kinds through stopRun. The row is already cancelled before the check ends, so the existing path in execute leaves it as cancelled, not failed. runStep already kills the whole process group with SIGKILL, and nothing survives SIGKILL, so there is no separate "escalate after a grace period" step.
AGENTS.md: the new module is listed with the other state shared through process memory.
test/run-cancel-tree.test.ts (new): pushes a forge.json whose check command forks a shell that forks a sleep, then cancels the running check through cancelRepoRun. It asserts that all three processes are gone within 2 s and that the row ends cancelled with the summary "cancelled by alice". It fails on main and passes here in about 0.9 s (5 runs out of 5). The gate picks it up automatically, since it runs every test/*.test.ts that isn't excluded.

What I chose not to do

Checks in another forge process: a check being worked by another forge process (during a roll) isn't stopped. Its row says cancelled and its own end leaves it so, as the module comment says.
Deploys: these keep their documented behaviour ("abandoned when its step ends").
Log wording: a check cancelled by a person still writes superseded: cancelled by … to its log, the existing wording from execute. Rewording it would change log output that other code may depend on.
Agent runs and bare steps: these already ended their process trees before this pull; test/crew-cancel-group.test.ts and test/steps-stop-tree.test.ts already cover them.
Plan on the issue: I couldn't post the plan the issue asked for, because the keep_plan tool wasn't available in this run.

🤖 Generated with Claude Code

sentinelcrew Approved 21h ago

Read every changed line. cancelRepoRun still checks the writer before it stops anything. The row is marked cancelled first, then stopRun aborts the check, and runStep kills the process group. Nothing is built from user text, and the test uses fixed commands. The new test starts a check that forks a child and a grandchild, cancels it, and polls /proc until all three are gone within 2 s. It then asserts the row says cancelled. The cancel already has a terminal door (tui-run-cancel), so no new screen or how-to is needed, and README is untouched. The new module is small, has one concept, and is listed in AGENTS.md. I did not fetch the preview or run the tests; this review rests on reading the diff.

Two non-blocking nits. runningRun is exported but only the test calls it. The afterAll SIGKILLs recorded pids, which could in theory hit a recycled pid.