Skip to content

test(h2): stabilize http2-connection - #5801

Open
ViniciusDev26 wants to merge 1 commit into
nodejs:mainfrom
ViniciusDev26:test/stabilize-http2-connection
Open

test(h2): stabilize http2-connection#5801
ViniciusDev26 wants to merge 1 commit into
nodejs:mainfrom
ViniciusDev26:test/stabilize-http2-connection

Conversation

@ViniciusDev26

@ViniciusDev26 ViniciusDev26 commented Sep 10, 2026

Copy link
Copy Markdown

Fixes #5266

test/http2-connection.js is the last of the [flaky] test\http2-*.js batch filed in the same week that still had none of the hardening its siblings got (#5219123db79, #52129dfff73, #5195f33a6cb, #5216dd1e50a). This applies the same treatment.

The CI log referenced in the issue has expired (HTTP 410), so the exact assertion that failed is not recoverable. What follows is what the file can fail on, with the reproductions I could build.

1. The unexpected-disconnect guard fires on the client's own idle teardown

The guard added by #5533 is:

client.on('disconnect', () => {
  if (!client.closed && !client.destroyed) {
    t.fail('unexpected disconnect')
  }
})

When keepAliveTimeout (4s by default) elapses on an idle h2 session, the client destroys the socket itself — setHttp2IdleTimeout at lib/dispatcher/client-h2.js:514, onHttp2SessionIdleTimeout at :529 — and emits disconnect with closed and destroyed both false. The reconnect is transparent to the next request, but the guard reports it as a failure:

UNEXPECTED DISCONNECT -> socket idle timeout

Tests 2 and 4 in this file issue several sequential requests on one client, so a runner that deschedules the process for >4s between two of them trips this. It also arrives after await t.completed, where an extra tspl assertion throws an AssertionError out of the listener rather than just failing a plan.

The guard now lives in test/utils/h2-disconnect-guard.js, ignores socket idle timeout, still flags a disconnect the peer forced, and reports the error message instead of a bare string. test/h2-disconnect-guard.js covers all three cases; it uses keepAliveTimeout: 100 so the idle case runs in ~115ms instead of 4s.

Note this vector post-dates the issue (#5533 landed 2026-07-16) and would show up as a file duration over 4s, so it does not explain the 570ms run in the report — but it is a live flake today.

2. Per-test RSA-2048 keygen

The file called await pem.generate({ opts: { keySize: 2048 } }) five times. @metcoder95/https-pem delegates to selfsignedforge.pki.rsa.generateKeyPair, a pure-JS keygen measuring 34–83ms per call here (2.4x spread). This is the same call that produced a hard failure on a Windows job in #5195:

Error: error:068000DD:asn1 encoding routines::illegal padding
    at Http2SecureServer (node:internal/http2/core:3431:5)
    at createSecureServer (node:internal/http2/core:3660:10)
    at TestContext.<anonymous> (D:\a\undici\undici\test\http2-dispatcher.js:617:18)

I fuzzed 2400 generations across 6 processes on Linux without reproducing a bad PEM, so it is rare and/or specific to Windows/OpenSSL — but it was observed in CI, it fails fast, and it is consistent with the 570ms duration of the run in the report. Switching to the static pem, as dd1e50a did for the trailers test, removes the vector.

3. Teardown order

after(() => server.close()) was registered before after(() => client.close()), and Http2SecureServer.prototype.close calls closeAllSessions() — so the server sent a GOAWAY to a client that was still open. I measured 320 runs looking for the race and client.close() always won (the boundary between two hooks is only a microtask), so this was not the failure. It is still the wrong order: the client now closes first and both are awaited, via per-test t.after rather than the module-level after.

4. localhost127.0.0.1

Same as dd1e50a, to keep the IPv4/IPv6 resolution out of the picture on Windows.

5. A real assertion bug

test/http2-connection.js:290 validated the second response against the first response's buffer:

t.strictEqual(Buffer.concat(body).toString('utf8'), 'hello h2!')  // body2

Both responses were hello h2!, so the mistake was invisible. The server now numbers them, which makes it observable, and the assertion reads body2. This is not a flake — the second request's payload was simply never checked.

Verification

before after
file duration 375ms 99ms
per test 40–85ms 3–15ms
  • npm run test:h2:core — 112 tests, 0 failures
  • eslint clean
  • 360 runs of both files under 12-way parallel load, 0 failures

Suggested follow-up

The inline guard is still duplicated across 32 other test files (test/http2-*.js, test/h2-*.js) from #5533 and #5686, all carrying the same idle-timeout vector. Migrating them to test/utils/h2-disconnect-guard.js is mechanical and would close the whole class rather than this one file. I kept it out of this PR to keep the diff reviewable, and I am happy to do it here or in a follow-up — whichever the maintainers prefer.

- Ignore the client's own idle-socket teardown in the unexpected-disconnect
  guard. Once keepAliveTimeout elapses on an idle h2 session the client drops
  the socket itself and reconnects transparently on the next request, but the
  guard reported it as a failure. The guard now lives in
  test/utils/h2-disconnect-guard.js and has its own coverage.
- Use the static pem instead of generating a fresh RSA-2048 key per test. The
  node-forge keygen dominated the runtime and is the same call that produced
  ERR_OSSL_ASN1_ILLEGAL_PADDING on a Windows job in nodejs#5195.
- Close the client before the server and await both, so the client never has
  to react to a GOAWAY it did not ask for.
- Bind and connect on 127.0.0.1 instead of localhost.
- Assert the second response body against body2; it was checking body, so the
  second request's payload was never verified. The two responses are now
  numbered to keep that assertion honest.
@codecov-commenter

codecov-commenter commented Sep 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.52%. Comparing base (cb7373a) to head (d5aed82).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5801      +/-   ##
==========================================
- Coverage   93.52%   93.52%   -0.01%     
==========================================
  Files         110      110              
  Lines       39415    39415              
==========================================
- Hits        36864    36863       -1     
- Misses       2551     2552       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 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.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[flaky] test\http2-connection.js

3 participants