Skip to content

fix(test): fix race condition in next-pages HMR test#26311

Closed
robobun wants to merge 4 commits into
mainfrom
claude/fix-next-pages-hmr-race-condition
Closed

fix(test): fix race condition in next-pages HMR test#26311
robobun wants to merge 4 commits into
mainfrom
claude/fix-next-pages-hmr-race-condition

Conversation

@robobun

@robobun robobun commented Jan 20, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Fixed intermittent timeout in test/integration/next-pages/test/dev-server.test.ts
  • The HMR test was failing because the console message listener was being set up AFTER the file copy that triggers HMR
  • In fast environments, the HMR cycle could complete before the listener was attached, causing the test to miss the "counter b loaded" message

Test plan

  • Code compiles correctly
  • Test should pass consistently on CI (cannot test locally on linux arm64 due to puppeteer limitations)

Fixes #11255

🤖 Generated with Claude Code

@robobun

robobun commented Jan 20, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 8:12 PM PT - Jan 20th, 2026

❌ Your commit 9f3bb999 has 3 failures in Build #35465 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 26311

That installs a local version of the PR into your bun-26311 executable, so you can run:

bun-26311 --bun

@robobun robobun force-pushed the claude/fix-next-pages-hmr-race-condition branch from 4b90a70 to 6844778 Compare January 20, 2026 21:34
@coderabbitai

coderabbitai Bot commented Jan 20, 2026

Copy link
Copy Markdown
Contributor

Walkthrough

Added persistent console buffering and an index-aware waitForConsoleMessage to the dev-server Puppeteer test; navigations and reloads now use explicit load semantics and console-message index markers (initial load, reload, HMR) to avoid race conditions and sequence test steps. (50 words)

Changes

Cohort / File(s) Summary
Dev-server Puppeteer test
test/integration/next-pages/test/dev-server-puppeteer.ts
Added consoleMessages buffer and a persistent p.on("console", …) listener; implemented waitForConsoleMessage(p, regex, startIndex?) that checks buffered messages and returns the next search index; switched navigations (page.goto, page.reload) to waitUntil: "load" / networkidle0; introduced index markers (afterInitialLoad, afterReload) and updated HMR/file-swap waits to use index-aware synchronization and visibility checks.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: fixing a race condition in the next-pages HMR test.
Description check ✅ Passed The description covers both required template sections with clear details about the race condition root cause and test verification status.
Linked Issues check ✅ Passed The code changes implement the core fix for issue #11255 by attaching the console listener before the file copy and using buffered message tracking to prevent race conditions.
Out of Scope Changes check ✅ Passed All changes are focused on fixing the HMR test race condition in dev-server-puppeteer.ts and are directly related to the linked issue requirements.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@robobun robobun force-pushed the claude/fix-next-pages-hmr-race-condition branch from 6844778 to c598c5c Compare January 20, 2026 21:35

@coderabbitai coderabbitai Bot 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.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
test/integration/next-pages/test/dev-server-puppeteer.ts (1)

66-85: Reload wait can resolve before the reload happens.

Both initial_load_promise and reload_promise listen for /counter a/ and are attached at the same time (lines 82–83), so the first match resolves both. This makes await reload_promise at line 117 effectively a no-op, reintroducing the race condition the early attachment was meant to prevent. Wait for the second match (or otherwise distinguish the reload signal) while still attaching the listener early.

🛠️ Suggested fix (wait for Nth match)
-  function waitForConsoleMessage(page: Page, regex: RegExp) {
+  function waitForConsoleMessage(page: Page, regex: RegExp, matchIndex = 1) {
     const { resolve, promise } = Promise.withResolvers<void>();
+    let matches = 0;
     function onMessage(msg: ConsoleMessage) {
       const text = msg.text();
       if (regex.test(text)) {
-        page.off("console", onMessage);
-        resolve();
+        if (++matches >= matchIndex) {
+          page.off("console", onMessage);
+          resolve();
+        }
       }
     }
     page.on("console", onMessage);
     return promise;
   }
@@
-  const initial_load_promise = waitForConsoleMessage(p, /counter a/);
-  const reload_promise = waitForConsoleMessage(p, /counter a/);
+  const initial_load_promise = waitForConsoleMessage(p, /counter a/, 1);
+  const reload_promise = waitForConsoleMessage(p, /counter a/, 2);

@robobun robobun force-pushed the claude/fix-next-pages-hmr-race-condition branch 2 times, most recently from 4471dae to bca74c3 Compare January 20, 2026 23:23
@robobun

robobun commented Jan 21, 2026

Copy link
Copy Markdown
Collaborator Author

CI build failed due to infrastructure issue (Buildkite API returning malformed JSON in SetupBuildkite.cmake). Tests didn't run. Will need a rebuild.

The HMR test was failing intermittently due to multiple race conditions:

1. Console message listeners could miss messages or match wrong occurrence
   - Fixed by collecting ALL messages into an array and searching with startIndex

2. Page interactions happened before navigation completed
   - Fixed by awaiting p.goto() and p.reload() with waitUntil: "load"

The new approach:
- A persistent listener collects all console messages from page creation
- waitForConsoleMessage(page, regex, startIndex) searches the array
- If message exists, returns immediately; otherwise waits for new messages
- Each call returns the next index, allowing waiting for Nth occurrence

Fixes #11255

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@robobun robobun force-pushed the claude/fix-next-pages-hmr-race-condition branch from bca74c3 to 9317608 Compare January 21, 2026 00:37
alii and others added 2 commits January 20, 2026 18:00
The test was failing because after reload, the element was queried
immediately after the page load event but before React hydration
completed. The click wasn't taking effect because the element
wasn't fully interactive yet.

Fixes:
1. Use networkidle0 instead of load for reload to wait for all network
   requests to complete
2. Explicitly wait for #counter-fixture to be visible before interacting

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

@coderabbitai coderabbitai Bot 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.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@test/integration/next-pages/test/dev-server-puppeteer.ts`:
- Around line 74-93: The waitForConsoleMessage function has a race between
checking consoleMessages and attaching the temporary listener; fix it by first
attaching the onMessage listener to page, then re-scan the consoleMessages
buffer (consoleMessages) for a match and if found remove the listener and
resolve immediately, otherwise keep the listener active and return the promise
from Promise.withResolvers; update references in waitForConsoleMessage and
ensure the onMessage handler removes itself via page.off("console", onMessage)
when resolving so you don't leak listeners.

Comment thread test/integration/next-pages/test/dev-server-puppeteer.ts
Fixed race condition where a message could arrive between checking
the buffer and attaching the listener. Now we attach the listener
FIRST, then check the buffer. A `resolved` flag prevents double
resolution and ensures the listener is properly removed.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[windows] next-pages/test/dev-server crashes and timeouts in CI

3 participants