Skip to content

fix(runtime): make the Stop/timeout lifecycle race deterministic (#2514) - #2611

Merged
Astro-Han merged 1 commit into
apache:mainfrom
chinawch007:fix/runtime-stop-timeout-race-deterministic-2514
Aug 12, 2026
Merged

fix(runtime): make the Stop/timeout lifecycle race deterministic (#2514)#2611
Astro-Han merged 1 commit into
apache:mainfrom
chinawch007:fix/runtime-stop-timeout-race-deterministic-2514

Conversation

@chinawch007

Copy link
Copy Markdown
Contributor

Summary

Make the keeps the first committed lifecycle cause across Stop and timeout races test 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) in armTimeout. The decisive detail is how stopBackgroundTask resolves — it does not re-arbitrate the cause once termination is underway:

if (live.driverExit)  { return ... { kind: 'stop', applied: false }; }   // process already dead
if (live.termination) { await waitForTerminationDecision(...); return ... { applied: false }; }  // timeout committed
// beginStopTermination also short-circuits: rootExited / live.termination -> finishPendingStop

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), stopBackgroundTask returns at its first guard → the run is reported timed_out. This matches the reported failure (expected cancelled but received timed_out).

A clarification on the PR #1776 cancel-override (requestTermination L1315): 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) 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, with stopBackgroundTask simply 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 injectable scheduleTimeout seam, a twin of the existing scheduleFlush:

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

2. shell-run-manager.ts (+15 / −7) — wire the seam. The timeoutTimer?: NodeJS.Timeout field becomes a generic cancelTimeout?: () => void (mirrors the existing cancelFlush field). The constructor binds scheduleTimeout, defaulting to real setTimeout when unset (production behavior is unchanged). armTimeout / clearLiveTimers use the seam. No arbitration logic (requestTermination / runTermination / finalState / cancel-override) is touched.

3. shell-run-manager.test.ts (+101 / −48) — add a manualTimeoutScheduler() helper (twin of manualFlushScheduler(), same Set + iterate semantics so fire() 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 via timeouts.fire() instead of betting on timeoutMs: 350; a far-future timeoutMs: 60_000 keeps 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 distinct stopBackgroundTask branch:

Scenario Ordering Expected Branch
1 Stop commits first cancelled, applied: true Stop owns the termination
2 timeout kills, then Stop timed_out, applied: false live.driverExit (process gone)
3 timeout commits, process still alive, then Stop timed_out, applied: false live.termination (in-flight)

Scenarios 2 and 3 are not redundant: driverExit and live.termination are different code paths in stopBackgroundTask with 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

  1. Establish whether the contract is violated or Stop starts too close to the boundary. — The contract is intact. The test started Stop too close to the timeoutMs boundary under CI load; stopBackgroundTask does not re-arbitrate once termination is in flight. Confirmed both by reading the code paths and empirically (see Verification).
  2. Replace wall-clock scheduling with a deterministic synchronization seam where practical.scheduleTimeout replaces the wall-clock timeout exactly where it decides the race. The killGraceMs latch and rawPublishTimer are left real — they don't decide commit ownership.
  3. Preserve the lifecycle contract; do not weaken the assertion or add retries. — No change to arbitration logic; status and operation assertions kept unchanged; no retries.
  4. Verify the focused race repeatedly and keep the Runtime workspace CI stable. — See Verification.

Verification

Tooling gates (all clean):

  • npm run format:check — 1643 files, no changes needed
  • npm run lint — 2477 files, no issues
  • npm run typecheck — all workspaces, zero type errors
  • npm --workspace @maka/runtime run build — clean
  • npm --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:

run 25: all pass    run 100: all pass
run 50: all pass    run 125: all pass
run 75: all pass    + 75-run batch: 0 failures
                    === 200-run total: 0 failures ===

Before/after comparison (window elimination, per the issue's "verify" intent):

Before (original test) After (this PR)
Timeout trigger real setTimeout(350) — OS decides when timeouts.fire() — test decides when
Ordering control none; bets Stop beats 350 ms pinned per scenario via fire() placement
Local stability 30/30 pass (light load) — window still present 200/200 pass — window removed at the mechanism level

An 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's writeStdin-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 vs killGraceMs 500 ms) and is not a regression.

Refs #2514.

…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 Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Astro-Han
Astro-Han merged commit b1b3ba2 into apache:main Aug 12, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants