D: abortable clone and storage preparation in process groups (#51 item 5)

[mchwang]

Item 5 of #51, the last item required before F1 merges.

What it adds

• runInProcessGroup (agents/process-group.ts) runs one command as the leader of a new process group.
  • It reports { pgid, startedAt } through onProcessGroup synchronously, in the same turn as the spawn, so F can save preparation_pgid and preparation_started_at before anything else runs.
  • On abort, at the deadline, or when output exceeds its limit, the group gets SIGTERM, then SIGKILL after 5 s.
  • It settles only after the leader has exited and every other member of its group is gone. A child left behind in the group is killed even after a normal exit.
  • It never rejects. The outcome has the same shape as runDocker's, so createOutcomeUnknown classifies it.
• createTaskCloneAsync (git/clone.ts) takes signal and onProcessGroup. Every Git call runs in its own group, and an abort removes the partial clone before the promise rejects.
• prepareTaskFilesystemsAsync (agents/container/storage.ts) works the same way for Docker calls.
  • An abort stops the running call.
  • The allocation then removes what it created. That cleanup is not cancelled, and its calls are reported through onProcessGroup too.
  • A create cut short by the abort counts as possibly created.

Design

The clone and the allocation are now step generators that yield each Git or Docker call. The existing synchronous helpers run the same steps with execFileSync or spawnSync, so their API, behaviour and error messages are unchanged for Ask and the existing tests. The async variants run each step through runInProcessGroup. The long file walks (the object audit and the link check) pause every 1,000 entries, so the async variants keep the event loop free, and an abort takes effect at the next pause.

Tests

• test/agent-process-group.test.ts covers the group being reported in the same turn as the spawn, killing leftovers after a normal exit, SIGKILL after the grace period for a group that ignores SIGTERM, abort, the output limit, and "not started" cases. A mutation that skips the leftover kill fails it.
• test/agent-clone-async.test.ts covers a normal clone, and an abort while a fake git clone ignores SIGTERM and runs a background child. The clone settles only after the SIGKILL, the event loop keeps ticking, nothing in the group survives, and the partial clone is removed.
• test/agent-storage-async.test.ts covers the same two cases for storage, against a fake Docker CLI, so they run without a daemon.
• test/agent-container.test.ts (real Docker) aborts an allocation while docker start ignores SIGTERM. It checks that nothing labelled with the allocation remains and that no group survives.

Non-Docker suite: 686 passed locally on the latest code change. CI's test job failed on 5b1d1cc and 4fb2c41 because the walk-pause test counted reads across an awaited Git call and hit 1,001 on Linux; d0cff55 fixed the test, which still fails at 2,506 on the pre-fix clone code. The exact-head CI result is the one to use.

Review rounds

Round	Reviewer	Findings	Fixed in
1	/engineering:code-review	1–5	357e149
2	/code-review	6–7	c913259
3	/engineering:code-review	8–10	4728f26
4	Copilot	11–13	5b1d1cc
5	/code-review	14–15	523992a
6	self-review, pass 2	16	ecb2fb3
7	self-review, pass 3	17	5fa6c18
8	independent review agent	18–19	2f45273
9	second independent review agent	20–21	bd956a6
10	third independent review agent	none	—

Nothing was declined. Every behaviour fix has a test that fails on the code before it (confirmed by running the new test against the previous version), except 8, 15 and 18, where the operating system would have to reuse a group ID within a sub-second window, and the extra I/O turn in 16, whose event-loop ordering a test could not reproduce (the attempted test passed on the old code, so it was dropped). The fail-closed part of 16 is tested.

Review-lesson audit

Rules cited from AGENTS.md: RR = "Required race regressions", RD = "Review readiness". The new rules are in this branch (4fb2c41).

#	Finding	Classification
1	The real-Docker test checked group liveness with kill -0 -<pgid>, which the Linux kill utility reads as an option, so the assertion always failed.	One-off: a test-tooling quirk. The fix uses process.kill(-pgid, 0), as the other tests already did.
2	A throwing onProcessGroup rejected the promise and left the child running, unwatched.	New rule (RR): a synchronous caller hook inside a lifecycle throws, blocks past the deadline, or aborts the signal.
3	signalGroup rethrew EPERM from timers and listeners, which could crash the runner or hang the drain.	New rule (RD): verify every behavioural claim the change makes (here, "never rejects") on every path, with a test that fails if it is false.
4	An abort during the allocation-ID check reported a generic message instead of "cancelled".	One-off: diagnostic wording. The behaviour was already fail-closed.
5	No tests for 2 and 3.	Covered (RR): "Every reproduced race requires a failing-before and passing-after regression."
6	An abort raised inside onProcessGroup came before the listener existed and was never seen.	New rule (RR): a signal already aborted when its listener is attached must still take effect.
7	A descendant that left the group but kept the output pipe stopped close from ever firing, so the call never settled.	New rule (RR): a child process exits, but a descendant outside its process group still holds its output pipes.
8	The SIGKILL grace timer was cleared only after the pipe wait, so it could fire at a group ID the group no longer held.	One-off: timer ordering in this helper; the default 5 s grace period made it unreachable outside tests.
9	The pipe-wait timer kept a finished process alive for up to 1 s.	One-off: timer hygiene.
10	The doc's "settles only after exit" did not state the EGROUPALIVE and 1 s pipe bounds.	New rule (RD): verify every behavioural claim in docs as well as code.
11	The storage link walk paused only per path taken off the queue, not per entry read, so one huge directory held the event loop.	New rules (RD): reread every changed function in full in each pass (the walk was unchanged since the first commit), and verify the "pauses every 1,000 entries" claim through nested loops.
12	The deadline was armed only after onProcessGroup returned, so a slow durable write extended the child's run.	New rule (RR): timers and listeners that bound a lifecycle must already be armed when a synchronous hook runs.
13	The same pause gap in the clone's object audit.	Same as 11.

| 14 | spawn throws synchronously for errors such as ENOMEM or E2BIG, which rejected runInProcessGroup and made the step drivers skip cleanup: an allocation kept its volumes, stayed live and kept its claim. | New rule (RD): verify every stated claim ("never rejects") on every path. |
| 15 | stop() could still run after the leader exited, sending signals to a drained, possibly reused group ID and re-arming the SIGKILL timer. | New rule (RD): the claim that nothing is signalled once the group is gone, checked through every late event. |
| 16 | A pipe held open past the 1 s wait (or a timer that fired before waiting output was read) returned the leader's exit status with output that may be incomplete. | New rule (RD): the claim "status is a number only when the command ran to completion" was false on this path. It now settles as ESTDIOHELD. |
| 17 | The lifecycle doc did not say that a held pipe settles as ESTDIOHELD, or that a failed spawn settles as not started. | New rule (RD): verify every behavioural claim in docs as well as code. |

| 18 | Output crossing the limit after the leader exited was dropped silently, and the call still reported success. | New rule (RD): the claim "status is a number only when the command ran to completion" was false on this path too. It now settles as ENOBUFS. |
| 19 | An abort landing between two calls made the next call refuse to start, so the clone or allocation rejected with a plain error rather than an AbortError. | New rule (RR): a signal already aborted when its listener is attached must still take effect, here across calls as well as within one. |

| 20 | A timeoutMs above 2³¹−1 ms passed validation, but Node timers treat it as 1 ms, so the async allocation timed out at once while the sync one worked. | New rule (RD): verify the claim that both variants "run these same steps" with the same behaviour, on every input. |
| 21 | The between-calls abort tests often aborted during the next call instead, so they did not reliably test the path they named. | New rule (RD): each claim needs a test that fails if the claim is false; the test must reach the path it names. |

Findings 11–13 came from Copilot after three author-run self-reviews had passed. That gap is the reason for the RD rule requiring an independent review (/codex review, a separate review agent, or /code-review at high effort or above) for concurrency, process and cleanup code.

🤖 Generated with Claude Code

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved cancellation, event-loop responsiveness, timeout, and cleanup coverage issues remain.

Review effort: Lite
Findings: 3

Open (3)

• Directory enumeration can block abort handling · New
• Timeout starts too late to cover process-group recording · New
• Large directory enumeration blocks async clone cancellation · New