fix(runtime): make the Stop/timeout lifecycle race deterministic (#2514) - #2611
Conversation
…che#2514) The `keeps the first committed lifecycle cause across Stop and timeout races` test intermittently reported `timed_out` where `cancelled` was expected (CI run 31258873738). Root cause is not a broken contract — it is the test betting on wall-clock for commit ordering. In PTY mode, `stopBackgroundTask` runs `beginStopTermination` inside `collector.mutateAtCut()`, which serializes against the PTY flush. Under CI load, Stop can be delayed past the `timeoutMs` boundary; once the timeout has fired and set `live.termination` (or killed the process), `stopBackgroundTask` returns at its first guard without re-arbitrating (`if (live.driverExit)` / `if (live.termination)`), so the run is reported `timed_out`. Note the PR apache#1776 cancel-override at `requestTermination` is not reachable from `stopBackgroundTask` — Stop never calls `requestTermination`; it either starts its own termination or joins an existing one. That override only serves the run abort and session/runtime shutdown paths. Fix: add a `scheduleTimeout` seam (twin of the existing `scheduleFlush`) so tests can drive timeout timing deterministically. No arbitration logic changes; production behavior is unchanged (defaults to real `setTimeout`). The flaky two-scenario test is rewritten into three deterministic scenarios, each pinning one arrival ordering and covering a distinct `stopBackgroundTask` branch: 1. Stop commits first -> cancelled (applied: true) 2. timeout kills, then Stop -> timed_out (driverExit branch) 3. timeout commits, process live -> timed_out (termination branch) All original assertions (status and operation) are kept verbatim. Verification: - format:check / lint / typecheck / build: clean - shell-run-manager.test.js: 58/58 pass - rewritten scenarios looped 200x locally: 0 failures
Astro-Han
left a comment
There was a problem hiding this comment.
Approved — I found no reproducible P0–P3 issues at d8a0d792.
The root-cause conclusion is sound: the lifecycle contract already preserves the first committed cause, while the old test allowed a real 350 ms timer to decide which operation committed first. Injecting scheduleTimeout at the existing scheduler boundary removes that wall-clock bet without changing arbitration or production behavior.
The seam is proportional and follows the existing scheduleFlush design. The Stop-first and post-exit scenarios now pin distinct orderings and retain the meaningful status and operation assertions. I do not see production code or complete test scenarios that should be removed.
One optional simplification: in the third scenario, timeouts.fire() installs live.termination synchronously, so calling stopBackgroundTask immediately afterward would lock the intended in-flight-termination branch and allow the waitUntil(writeStdin) barrier to be deleted. The current test still validates the externally observable lifecycle result, so this is a test-precision improvement rather than a merge blocker.
CI is green, and GitHub’s merge result against current main is clean.
中文对照
批准——在当前 head d8a0d792 上没有发现可复现的 P0–P3 问题。
根因判断成立:生命周期契约本身已经保留第一个完成提交的原因,旧测试的问题是让真实的 350ms timer 决定 Stop 和 timeout 谁先提交。在现有 scheduler 边界注入 scheduleTimeout,可以删除这个墙钟赌注,同时不改变 arbitration 或生产行为。
这个 seam 的复杂度与问题匹配,也遵循了已有的 scheduleFlush 设计。Stop-first 和进程已经退出两个场景现在能够固定不同顺序,并保留有意义的状态和 operation 断言。我没有看到应删除的生产代码或完整测试场景。
一个可选的简化建议:第三个场景中,timeouts.fire() 会同步安装 live.termination,因此可以紧接着调用 stopBackgroundTask,锁定目标 in-flight termination 分支,并删除 waitUntil(writeStdin) barrier。当前测试仍然验证了对外可观察的生命周期结果,所以这只是测试精度改进,不是合并阻塞项。
CI 全绿,GitHub 基于当前 main 的合并结果也没有冲突。
Disclosure: Codex assisted by inspecting the issue evidence, lifecycle arbitration paths, scheduler seam, all three race scenarios, CI, and GitHub’s merge result against current main. Astro-Han reviewed the resulting evidence, determined that no finding crossed the P0–P3 threshold, and made the final approval decision.
Summary
Make the
keeps the first committed lifecycle cause across Stop and timeout racestest deterministic so the Runtime workspace stops flaking on CI (#2514).Root cause: a wall-clock bet, not a broken contract
Both scenarios differ only in when Stop arrives relative to the timeout committing, but that ordering was driven by a real
setTimeout(timeoutMs)inarmTimeout. The decisive detail is howstopBackgroundTaskresolves — it does not re-arbitrate the cause once termination is underway:So whichever side commits first "wins" the cause, and Stop just joins. Scenario 1 requires Stop to commit before the 350 ms timeout fires. Under CI load, event-loop scheduling can delay Stop past that boundary; once the timeout has set
live.termination(or killed the process),stopBackgroundTaskreturns at its first guard → the run is reportedtimed_out. This matches the reported failure (expected cancelled but received timed_out).A clarification on the PR #1776 cancel-override (
requestTerminationL1315): that branch is not reachable fromstopBackgroundTask— Stop never callsrequestTermination; it either starts its own termination viabeginStopTerminationor joins an existing one. The override's real callers are the run abort paths (runBackgroundBash/runForegroundBash) and session/runtime shutdown. So #1776 fixed the related race on those paths; #2514 is the different race the issue author flagged — wall-clock deciding whether Stop arrives before or after the timeout commits, withstopBackgroundTasksimply joining whichever outcome is in flight. The lifecycle contract itself is intact; this is a test timing problem.Changes (3 files, +124 / −55)
1.
shell-run-contract.ts(+2) — add an injectablescheduleTimeoutseam, a twin of the existingscheduleFlush:2.
shell-run-manager.ts(+15 / −7) — wire the seam. ThetimeoutTimer?: NodeJS.Timeoutfield becomes a genericcancelTimeout?: () => void(mirrors the existingcancelFlushfield). The constructor bindsscheduleTimeout, defaulting to realsetTimeoutwhen unset (production behavior is unchanged).armTimeout/clearLiveTimersuse the seam. No arbitration logic (requestTermination/runTermination/finalState/ cancel-override) is touched.3.
shell-run-manager.test.ts(+101 / −48) — add amanualTimeoutScheduler()helper (twin ofmanualFlushScheduler(), sameSet+ iterate semantics sofire()on an empty set is an explicit no-op) and rewrite the flaky two-scenario test into three deterministic scenarios. Each pins one arrival ordering viatimeouts.fire()instead of betting ontimeoutMs: 350; a far-futuretimeoutMs: 60_000keeps the real timeout out of the way.trap "" TERM+ SIGKILL escalation stays real, but commit ownership is decided before the kill lands, so kill timing cannot flip the cause. Each scenario covers a distinctstopBackgroundTaskbranch:cancelled,applied: truetimed_out,applied: falselive.driverExit(process gone)timed_out,applied: falselive.termination(in-flight)Scenarios 2 and 3 are not redundant:
driverExitandlive.terminationare different code paths instopBackgroundTaskwith different join behavior, so pinning both gives independent regression coverage. All original assertions (status and operation) are kept verbatim.How this satisfies the issue's four goals
timeoutMsboundary under CI load;stopBackgroundTaskdoes not re-arbitrate once termination is in flight. Confirmed both by reading the code paths and empirically (see Verification).scheduleTimeoutreplaces the wall-clock timeout exactly where it decides the race. ThekillGraceMslatch andrawPublishTimerare left real — they don't decide commit ownership.Verification
Tooling gates (all clean):
npm run format:check— 1643 files, no changes needednpm run lint— 2477 files, no issuesnpm run typecheck— all workspaces, zero type errorsnpm --workspace @maka/runtime run build— cleannpm --workspace @maka/runtime run test:dist: 3390pass, 9 skipped.Focused-race stability (the issue's core ask) — 200× loop, 0 failures:
The rewritten scenarios were looped 200 times locally (125 + 75 batches), each run executing all three scenarios:
Before/after comparison (window elimination, per the issue's "verify" intent):
setTimeout(350)— OS decides whentimeouts.fire()— test decides whenfire()placementAn honest note on evidence strength: the 200× result is statistical support (the window is very likely gone), while the seam rewrite provides mechanism-level support (the wall-clock dependency that created the window is removed —
fire()decides ordering explicitly). Together they support the conclusion that the focused race is now deterministic. This does not mathematically prove zero residual timing surface; e.g. scenario 3'swriteStdin-throws barrier shares the same narrow shape as the original scenario 2's barrier (an in-flight-termination probe), but with ~25× timing margin (first poll ~20 ms vskillGraceMs500 ms) and is not a regression.Refs #2514.