test(runtime): make the Stop/timeout lifecycle race deterministic

[Astro-Han]

Problem

packages/runtime/src/__tests__/shell-run-manager.test.ts intermittently fails in CI at keeps the first committed lifecycle cause across Stop and timeout races.

The observed failure expected cancelled but received timed_out. Rerunning the unchanged job passed, which makes the current test timing-dependent and leaves ambiguity between a product race and a nondeterministic test setup.

Evidence:

• CI run 31258873738 on PR feat(runtime): open linked subagents while agent_spawn is still running #2472.
• The first test_workspaces attempt failed in the Runtime suite; the failed-job rerun passed without a code change.
• PR fix(ci): resolve three intermittent test flakes across runtime-host, runtime, and cli #1776 previously fixed a related but different Stop/timeout race.

Expected outcome

• Establish whether the first committed lifecycle cause is violated or the test starts Stop too close to the timeout boundary.
• Replace wall-clock scheduling with a deterministic synchronization seam where practical.
• Preserve the intended lifecycle contract; do not weaken the assertion or add retries.
• Verify the focused race repeatedly and keep the Runtime workspace CI stable.

[chinawch007]

I'd like to claim #2514. Below is my root-cause analysis and the fix I intend to implement.

Root cause: the test bets on wall-clock for commit ordering

Both scenarios in keeps the first committed lifecycle cause across Stop and timeout races differ only in when Stop arrives relative to the timeout killing the process, but that ordering is driven by a real setTimeout(timeoutMs) in armTimeout:

// shell-run-manager.ts — armTimeout
live.timeoutTimer = setTimeout(() => {
  if (live.rootExited || live.finalizeOnce || live.termination) return;
  live.lifecycleCause ??= 'timeout';
  this.requestForcedTermination(live, 'timeout');
}, live.timeoutMs);

The key point is how stopBackgroundTask resolves — it does not re-arbitrate the cause once termination is underway:

// stopBackgroundTask
if (live.driverExit) {                                   // process already dead (L478)
  return ... { kind: 'stop', applied: false };
}
if (live.termination) {                                  // timeout already committed (L482)
  await this.waitForTerminationDecision(...);
  return ... { kind: 'stop', applied: false };
}
...
// beginStopTermination also short-circuits (L1331/L1335):
if (live.rootExited) { pending.settle('exit'); return false; }
if (live.termination) { return this.finishPendingStop(pending); }

So whichever side commits first "wins" the cause, and Stop just joins the outcome:

• Scenario 1 (cancelled) requires Stop to commit before the timeout fires. Stop is called "immediately", but under CI load event-loop scheduling can delay it past the 350 ms boundary. Once the timeout has fired and set live.termination (or killed the process, setting rootExited/driverExit), stopBackgroundTask returns at its first guard without re-arbitrating → the run is reported timed_out. This matches the reported failure (expected cancelled but received timed_out).

• Scenario 2 (timed_out) requires the timeout to fire and kill before Stop arrives. The current writeStdin-throws barrier only proves live.termination was set, not that the kill was applied, so it also leans on wall-clock.

Note on the cancel-override at requestTermination (L1315, added in #1776): that branch is not reachable from stopBackgroundTask — Stop never calls requestTermination; it either starts its own termination via beginStopTermination or joins an existing one. The override's real callers are the run abort paths (runBackgroundBash/runForegroundBash, L279/L303) and session/runtime shutdown (terminateLive, L1464). So #1776 fixed the related race on those paths (cancel arriving while the process is still alive); #2514 is the different race the issue author flagged — wall-clock deciding whether Stop arrives before or after the timeout commits, with stopBackgroundTask simply joining whichever outcome is already in flight.

This is the test's timing problem, not a broken contract. I confirmed empirically: locally the test passes 30/30 (light load), and a probe that forces a late Stop (wait until the timeout has finalized, then call Stop) deterministically reproduces timed_out, with Stop returning applied: false exactly as the early-return guards prescribe.

Fix: a scheduleTimeout seam (mirrors the existing scheduleFlush)

The runtime already injects a flush scheduler so tests can drive flush timing deterministically:

// shell-run-contract.ts
scheduleFlush?: (run: () => void, delayMs: number) => () => void;

I'll add the same seam for the timeout. No arbitration logic changes.

1. Contract — add an optional field:

/** Schedules the run timeout and returns its canceler; injected so tests can drive timeout timing. */
scheduleTimeout?: (run: () => void, delayMs: number) => () => void;

2. Manager — default to real setTimeout when unset (production behavior unchanged):

this.scheduleTimeout =
  input.scheduleTimeout ??
  ((run, delayMs) => { const t = setTimeout(run, delayMs); return () => clearTimeout(t); });

Switch the timeoutTimer field to a generic canceler (mirrors the existing cancelFlush field), and have armTimeout / clearLiveTimers use the seam. The unrelated rawPublishTimer and the killGraceMs latch are left untouched — they don't decide commit ownership.

3. Test — add a manualTimeoutScheduler() helper, a twin of manualFlushScheduler() with the same Set + iterate semantics so that fire() on an empty set is an explicit no-op:

function manualTimeoutScheduler(): {
  schedule: (run: () => void, delayMs: number) => () => void;
  fire: () => void;
} {
  const pending = new Set<() => void>();
  return {
    schedule: (run) => { pending.add(run); return () => pending.delete(run); },
    fire: () => { const due = [...pending]; pending.clear(); for (const run of due) run(); },
  };
}

Both original scenarios switch from timeoutMs: 350 (wall-clock) to a far-future timeoutMs plus timeouts.fire(), so the ordering is pinned rather than raced. The trap "" TERM + SIGKILL escalation stays real, but commit ownership is decided before the kill lands, so the kill timing does not affect the assertions. All original assertions are kept verbatim, including the operation checks — I'm not weakening any of them.

I'll cover three arrival orderings, each nailing a distinct stopBackgroundTask branch:

• Scenario 1 — Stop commits first → cancelled, applied: true (Stop owns the termination; timeouts.fire() afterward finds an empty set and no-ops).
• Scenario 2 — timeout kills, then Stop arrives → timed_out, applied: false (covers the L478 live.driverExit branch: timeouts.fire(), await terminal state via waitForTerminalShellRun, then call Stop — replacing the writeStdin-throws barrier).
• Scenario 3 (new) — timeout commits but process still alive, then Stop arrives → timed_out, applied: false (covers the L482 live.termination branch: timeouts.fire() sends SIGTERM but trap "" TERM keeps the process alive; the existing writeStdin-throws barrier detects live.termination being set without waiting for the kill — which is exactly the state we want here — then Stop joins via waitForTerminationDecision).

Scenarios 2 and 3 are not redundant: L478 (process gone) and L482 (termination committed, process alive) are different code paths in stopBackgroundTask with different join behavior, so pinning both gives independent regression coverage.

Why this satisfies the four goals

1. Establish cause — confirmed: the lifecycle contract is intact; the test starts Stop too close to the timeout boundary under CI load, and stopBackgroundTask does not re-arbitrate once termination is in flight.
2. Deterministic seam — scheduleTimeout replaces the wall-clock timeout exactly where it decides the race; trap "" TERM's SIGKILL escalation stays real but does not affect commit ownership.
3. Preserve contract — no change to requestTermination / runTermination / finalState / cancel-override; assertions (status and operation) are not weakened; no retries.
4. Verify repeatedly — the rewritten test will be looped locally (target 200× zero failures across all three scenarios), and the full shell-run-manager file re-run to confirm no regressions.

---

This plan was drafted with material assistance from GLM, DeepSeek, and reviewed by a human before publication.

I'll start once the issue is assigned.

[Astro-Han]

Closing as completed by #2611. The merged change established that the failure was caused by wall-clock-dependent test ordering rather than a broken lifecycle contract, introduced a deterministic timeout scheduler seam, and covered the relevant Stop/timeout arrival orderings without weakening the assertions.\n\nTriage and wording were prepared with OpenAI Codex and reviewed by the maintainer.