Skip to content

test(runtime): drive the shell-run flush timer from the test instead of the wall clock - #2054

Merged
Astro-Han merged 2 commits into
mainfrom
fix/shell-run-flush-flake
Aug 3, 2026
Merged

test(runtime): drive the shell-run flush timer from the test instead of the wall clock#2054
Astro-Han merged 2 commits into
mainfrom
fix/shell-run-flush-flake

Conversation

@Astro-Han

Copy link
Copy Markdown
Contributor

Summary

restores the trailing PTY flush after a queued control aborts before commit flaked on CI (run 30846092608, green on rerun): the manager was built with flushIntervalMs: 1_000, and the test waited for the child's dirty-written file before asserting the PTY output had not been committed yet. When that wait crossed the 1s periodic flush, the durable revision advanced on its own and assert.equal(beforeAbort.revision, beforeDirty.revision) failed.

The assertion and the periodic flush are semantically in conflict: "not committed at abort time" can only hold if nothing else commits during the window, so as long as flush timing is owned by the wall clock and the test cannot control it, the test is probabilistic. Relaxing the assertion would hide that instead of resolving it.

Root cause is that the manager has two time sources — now() is injected, but the automatic flush uses the global Date.now() + setTimeout. This makes the flush timer injectable so the single test that depends on flush ordering owns it:

  • ShellRunProcessManagerInput.scheduleFlush?: (run, delayMs) => () => void, defaulting to the previous setTimeout/clearTimeout. Production behavior is unchanged.
  • LiveShellRunBase.flushTimer: NodeJS.Timeout becomes cancelFlush: () => void, which decouples the pending-flush slot from the platform handle type.
  • The test installs a manual scheduler and sets flushIntervalMs: 60_000 (so the pty branch never takes the delay === 0 path that flushes synchronously past the scheduler). No automatic flush can fire before the abort, so the revision and "durable output has no DIRTY" assertions are deterministic. After the abort it first asserts a flush was rescheduled, then runs it and checks the output lands.

The restore contract is now checked directly rather than inferred from a timeout: removing scheduleAutomaticFlush(live) from the !snapshot branch of queuePersist fails the test at the pending-flush assertion.

Deeper cleanup deliberately not done here: the manager constructs PtyProcessDriver/PipeProcessDriver itself, so every test in this file needs a real child process to verify what are internal orchestration rules. Making the driver injectable would remove wall-clock dependence from the timeout and kill-grace paths too, but that is a separate refactor and only this one test has actually flaked.

Verification

  • node --test dist/__tests__/shell-run-manager.test.js — 51 pass / 0 fail, 5 consecutive runs on macOS.
  • Mutation check: dropping the flush restore in queuePersist makes the test fail at waitUntil(() => flushes.pending()); restoring it passes.
  • Full node --test "dist/**/*.test.js" in packages/runtime: 3076 pass / 4 fail. The 4 failures are the /private symlink workspace-path tests and reproduce identically on unmodified main.
  • biome check clean on the three touched files.
  • Not run: the CI Linux job that produced the original flake.

@Astro-Han

Copy link
Copy Markdown
Contributor Author

Follow-up after external review (three passes: production seam design, correctness/regression, test determinism). Two residual wall-clock paths the first commit left open are now closed in 2878ca6:

  • scheduleAutomaticFlush no longer special-cases delay === 0 in the pty branch. That shortcut called queueAutomaticFlush synchronously, bypassing the seam entirely, so a machine that stalled longer than flushIntervalMs between the last persist and the abort could still commit inside the window the test pins. All time-driven flushes now go through scheduleFlush, which also removes a branch.
  • The test's child run timeout goes from 5s to 120s. A run timeout finalizes the record and advances the revision the assertions pin, so it was an implicit upper bound on the test's own duration. The child keeps itself alive with setInterval and is torn down in finally.

Deliberately not changed: elapsed in scheduleAutomaticFlush still uses Date.now() rather than the injected now(). Reviewers flagged the split time source, but now() stamps records and is a monotonic counter starting at 1000 under test, while elapsed measures real time to pace flushes. Routing it through now() would turn the flush interval into counter ticks and silently change flush batching for every test built by createManager (flushIntervalMs: 10). A comment now records that reasoning at the call site so the two clocks are not "unified" by accident.

Verification: 51/51 in shell-run-manager.test.js, 5 consecutive runs. Full packages/runtime suite 3076 pass / 4 fail, the same 4 /private symlink workspace-path failures that reproduce on main. Mutation check re-run against the new code: dropping the flush restore in queuePersist still fails the test. biome check clean.

@Astro-Han
Astro-Han marked this pull request as ready for review August 3, 2026 20:19
@Astro-Han
Astro-Han merged commit 982ac41 into main Aug 3, 2026
10 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.

1 participant