You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
restores the trailing PTY flush after a queued control aborts before commitflaked on CI (run 30846092608, green on rerun): the manager was built withflushIntervalMs: 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 andassert.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 globalDate.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 previoussetTimeout/clearTimeout. Production behavior is unchanged.LiveShellRunBase.flushTimer: NodeJS.TimeoutbecomescancelFlush: () => void, which decouples the pending-flush slot from the platform handle type.flushIntervalMs: 60_000(so the pty branch never takes thedelay === 0path 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!snapshotbranch ofqueuePersistfails the test at the pending-flush assertion.Deeper cleanup deliberately not done here: the manager constructs
PtyProcessDriver/PipeProcessDriveritself, 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.queuePersistmakes the test fail atwaitUntil(() => flushes.pending()); restoring it passes.node --test "dist/**/*.test.js"inpackages/runtime: 3076 pass / 4 fail. The 4 failures are the/privatesymlink workspace-path tests and reproduce identically on unmodifiedmain.biome checkclean on the three touched files.