Skip to content

test: fix flaky http2 maxOriginSetSize test#64407

Open
pimterry wants to merge 1 commit into
nodejs:mainfrom
pimterry:fix-h2-origin-flake
Open

test: fix flaky http2 maxOriginSetSize test#64407
pimterry wants to merge 1 commit into
nodejs:mainfrom
pimterry:fix-h2-origin-flake

Conversation

@pimterry

Copy link
Copy Markdown
Member

A fix for a flake from today's list: nodejs/reliability#1597.

Can't reproduce the flake locally directly, but I can with a small delay on the server stream response to simulate the very plausible IO delay on the response. Flake 100% reproduces with a setTimeout into the server stream handler at the top:

server.on('stream', (stream) => {
  stream.on('error', () => {}); // We need errors handlers just for the repro
  setTimeout(() => { // Delay longer than the setInterval below
    try {
      stream.respond();
      stream.end('ok');
    } catch {}
  }, 100);
});

This fails every time, as in the flake example in https://github.com/nodejs/reliability/blob/main/reports/2026-07-10.md:

Error [ERR_HTTP2_TOO_MANY_ORIGINS]: The server sent more ORIGIN frames than the allowed number of -0
    at Http2Session.onOrigin (node:internal/http2/core:783:23)
Emitted 'error' event on ClientHttp2Stream instance at:
    at emitErrorNT (node:internal/streams/destroy:170:8)
    at emitErrorCloseNT (node:internal/streams/destroy:129:3)
    at process.processTicksAndRejections (node:internal/process/task_queues:90:21) {
  code: 'ERR_HTTP2_TOO_MANY_ORIGINS'
}

I'll do a stress test as well and see if I can confirm the fix directly.

Flake explanation:

  • The server runs a 10ms interval on every session which sends 10 origins
  • The test tries different <10 max origin limits, which will all fail
  • The test also sends a request
  • When the origins are rejected, the session is destroyed
  • All open request streams will also be destroyed when the session is destroyed
  • This creates a race:
    • If the request completes before the interval then the test passes (request finishes clean, session destroyed as expected)
    • If the request is still open when the interval fires and destroys the session, the request fails and emits an unhandled error => flake.

This PR fixes that by dropping the request (and server handler) completely from all tests here (the immediate flake, as well as the others). They aren't necessary - I think they exist because this was copied from test-http2-origin, which does actually use the request flow.

These tests though operate purely on just the session directly (server session handler triggers the interval, client session emits the error). The requests were ignored and the server handler did nothing. Dropping the requests completely simplifies the test and kills the race.

Would be good to get confirmation of that from @mcollina, since this was added as a security fix (https://github.com/nodejs-private/node-private/pull/855) which I can't see but I'm fairly confident.

Signed-off-by: Tim Perry <pimterry@gmail.com>
@pimterry pimterry requested a review from mcollina July 10, 2026 11:59
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Jul 10, 2026
@codecov

codecov Bot commented Jul 10, 2026

Copy link
Copy Markdown

Codecov Report

βœ… All modified and coverable lines are covered by tests.
βœ… Project coverage is 90.23%. Comparing base (7036199) to head (e623a99).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #64407      +/-   ##
==========================================
- Coverage   90.25%   90.23%   -0.02%     
==========================================
  Files         741      741              
  Lines      241207   241216       +9     
  Branches    45424    45445      +21     
==========================================
- Hits       217696   217671      -25     
- Misses      15087    15123      +36     
+ Partials     8424     8422       -2     

see 35 files with indirect coverage changes

πŸš€ New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • πŸ“¦ JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pimterry

Copy link
Copy Markdown
Member Author

CC @nodejs/http2

@pimterry

Copy link
Copy Markdown
Member Author

Stress test on main & PR both pass 🀦. Can't verify that way then, but I'm still fairly confident this is the right fix.

@anonrig anonrig added the fast-track PRs that do not need to wait for 72 hours to land. label Jul 10, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Fast-track has been requested by @anonrig. Please πŸ‘ to approve.

@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 10, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 10, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fast-track PRs that do not need to wait for 72 hours to land. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants