PLA-1585: fix flaky undici telemetry tests + harden CI matrix#1473
Merged
dbutts29 merged 7 commits intoJun 12, 2026
Conversation
The server telemetry fetch tests mock the network with undici MockAgent (npm undici 6.23) + setGlobalDispatcher, but assert against Node's global fetch, which uses Node's *built-in* undici. When the built-in dispatcher diverges from the npm package's (newest Node ships undici 7.x), setGlobalDispatcher is a no-op for global fetch: the request reaches the real example.com and the test sees 404 instead of the mocked 200. This fails deterministically on the CI `latest` Node leg, and fail-fast then cancels the other legs. Route the global fetch through the same undici instance the mock patches (globalThis.fetch = undiciFetch) for the duration of these tests, restoring it afterEach. Deterministic on every Node version, and still faithfully exercises the instrumentation since Node's built-in fetch is undici. Verified: server tests pass (149) on Node 18/22/24 locally. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…stalls - fail-fast: false so one leg's failure no longer cancels the others (this cascade is what made the telemetry failure look like all legs were broken). - continue-on-error on the floating `latest` leg: keep it as a forward-compat canary (it caught the undici 7.x divergence) without letting a moving, non-LTS target block PRs. - timeout-minutes: 20 + retry/timeout around `playwright install chromium`, which can stall indefinitely on the browser download and hung the 24/latest legs for 40+ min. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dbutts29
changed the title
Contributor
Author
|
Folded in CI-reliability changes (same PLA-1585 goal of making rollbar.js CI trustworthy):
The telemetry fix is the substantive change; these make the result observable and stop a stalled browser download from hanging the matrix. |
`playwright install chromium` stalls indefinitely on Node 24/latest (a pre-existing issue that was masked by the telemetry failure until the server tests started passing). Browser tests launch chromium, so the harness Node version is irrelevant to browser coverage — gate the Playwright + Web Test Runner steps to 18/20/22 and let 24/latest run the server + validation suites only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
Looking at using package aliasing to install two versions of undici, so the tests continue to use the version of fetch for each Node version in the test matrix. https://docs.npmjs.com/cli/v8/using-npm/package-spec#aliases |
undici MockAgent already intercepts Node's built-in fetch on every blocking leg (18/20/22/24) — they share Symbol(undici.globalDispatcher.1). The swap only masked a failure on the non-blocking `latest` leg, where Node bundles undici 7.x (global-dispatcher symbol bumped to .2). With fail-fast off and `latest` on continue-on-error, that divergence now surfaces as a forward-compat canary instead of cascading into the other legs. Reverting the swap restores genuine per-version built-in fetch coverage for these fetch-compatibility tests (review feedback). Verified 149 passing on Node 22 and 24 with the reverted test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ch on all legs The telemetry fetch tests mock via npm undici's MockAgent but assert against Node's built-in global fetch. undici 7's setGlobalDispatcher writes both the legacy `.1` and the current `.2` global-dispatcher symbol slots, so the mock is seen by built-in fetch whether the running Node bundles undici 6 (reads `.1`) or 7 (reads `.2`) — including the `latest` leg, which was the only failure. This keeps the tests exercising each Node version's real built-in fetch (no globalThis.fetch swap) while turning the previously-red, non-blocking `latest` leg green. undici is a devDependency used only in test/server.telemetry.test.js (src/ instruments the global fetch and never imports undici), so there is zero runtime impact. Floor is ^7.27.2 because that is the verified version whose setGlobalDispatcher writes both symbol slots. Verified 149 server tests pass on Node 22 and 24. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…-in fetch The straight undici 7 bump fixed the `latest` leg but broke Node 18: undici 7 requires Node 20+ (it references the global `File`), so it can't even load on 18. Node 18 is a supported target (CLAUDE.md: "Minimum Node.js: 18+"), so keep it. Keep undici 6 as the default and add undici@7 via the `undici-v7` npm alias. The telemetry test picks one by Node major via dynamic import: <20 → undici 6 (symbol `.1`, the only major that runs on Node 18), >=20 → undici 7 (writes both `.1` and `.2`, so MockAgent intercepts the built-in fetch whether Node bundles undici 6 (reads `.1`) or 7 (reads `.2`, the `latest` leg)). Dynamic import is required so the unselected major never loads (a static undici-7 import would crash at load on Node 18). devDependency-only (src/ never imports undici), zero runtime impact. Verified 149 server tests pass on Node 18, 20, 22, and 24. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
waltjones
reviewed
Jun 12, 2026
| // (statically importing undici 7 on Node 18 would crash at load time). | ||
| const nodeMajor = parseInt(process.versions.node.split('.')[0], 10); | ||
| const { MockAgent, getGlobalDispatcher, setGlobalDispatcher } = | ||
| nodeMajor >= 20 ? await import('undici-v7') : await import('undici'); |
Contributor
There was a problem hiding this comment.
One small nit. Let's use the decorated name for the legacy version, and the undecorated name for the forward version. e.g. 'undici' and 'undici-v6'.
waltjones
approved these changes
Jun 12, 2026
waltjones
left a comment
waltjones
left a comment
Contributor
There was a problem hiding this comment.
One comment and then looks good.
… version Review nit: name the forward version undecorated and the legacy version decorated. `undici` now resolves to ^7.27.2 and `undici-v6` is the npm:undici@^6 alias (was the reverse). Test selection follows: Node >=20 imports `undici` (7, symbol .2 — and writes .1 too), Node <20 imports `undici-v6` (6, symbol .1, the only major that runs on Node 18). Verified 149 server tests pass on Node 18 and 20; lint clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
dbutts29
deleted the
darrenbutts/PLA-1585/fix-flaky-undici-telemetry-test
branch
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Description of the change
Fixes the flaky
with fetch network capture enabledtelemetry tests and hardens the CI matrix so a single floating-Node failure can no longer mask the whole suite. Test-infra only — no SDK/srcchanges, zero runtime impact (undiciis a devDependency used only intest/server.telemetry.test.js; the SDK instruments the globalfetchand never imports undici).Background
The two fetch-capture tests mock the network with undici
MockAgent+setGlobalDispatcherbut assert against Node's global built-infetch.MockAgentonly intercepts the built-infetchwhen its global-dispatcher symbol matches the running Node's bundled undici. undici 6 usesSymbol.for('undici.globalDispatcher.1'); undici 7 bumped it to.2. So with the pinned npmundici@6, the mock was bypassed on any Node leg that bundles undici 7: the request escaped to the realexample.com/api/usersand returned 404 instead of the mocked 200 (a 404 rather than aMockNotMatchedErroris the tell that it escaped the mock).This failed only on the
latestleg, but withfail-faston, that one failure cancelled the other four legs — so every PR looked broken. Master had not run green since 2026-03-11, so the regression went unnoticed.Telemetry test fix
Measured interception across the matrix:
MockAgentintercepts built-infetch?.1).2)The fix keeps the tests asserting against each Node version's real built-in
fetch(so the matrix still exercises per-version fetch behaviour) while makingMockAgentintercept on every leg:undicidevDep is the forward version (^7.27.2); the legacy version is theundici-v6alias (npm:undici@^6).<20→undici-v6(6),>=20→undici(7). undici 7'ssetGlobalDispatcherwrites both the.1and.2slots, so the mock is seen whether the running Node bundles undici 6 (.1) or 7 (.2).Two constraints shaped this:
Fileconstructor (added in Node 20), so it throwsReferenceError: File is not definedat load time on Node 18, a supported target (CLAUDE.md: "Minimum Node.js: 18+"). Node 18 must stay on undici 6.undici(7) import would crash at load on Node 18 even if unused.Alternatives considered and rejected:
globalThis.fetch = undiciFetch(route global fetch through the npm undici): works on every leg, but then all legs test the npm undici'sfetchinstead of each version's real built-infetch— which defeats the matrix for fetch-compatibility tests.latestbut breaks the Node 18 leg (undici 7 can't load there).CI matrix hardening
fail-fast: false— one leg's failure no longer cancels the others (the cascade that made alatest-only failure look universal).continue-on-erroron the floatinglatestleg — keep it as a forward-compat canary that doesn't block PRs.timeout-minutes: 20plus a retry/timeout wrapper aroundplaywright install chromium, which stalls indefinitely on Node 24/latest; gate the Playwright + Web Test Runner steps to stable Node (the harness Node version is irrelevant to browser coverage).Verification
npm run test:server: 149 passing locally on Node 18, 20, 22, and 24;npm run lintclean.cf9742d0: all five legs green, includinglatest— the undici-7-bundled environment that can't be reproduced locally, giving live confirmation the.2interception path works.Type of change
Related issues
Checklists
Development
Code review
🤖 Generated with Claude Code