test: add unexpected disconnect guards to more client test files by samayer12 · Pull Request #4844 · nodejs/undici

samayer12 · 2026-02-27T21:29:11Z

This relates to...

Relates to #251 and #4833.

Rationale

Adds disconnect guard assertions to unit tests that follow the pattern seen in #4833. This PR adds the guard to additional tests that do not already adopt the pattern, and documents it.

Changes

Disconnect guards

This PR structures tests along these lines:

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

Fix encoding test (Node 20 + macOS)

I noticed that there were CI issues with test/fetch/encoding.js on Node.js 20 + macOS. As part of this PR, I also changed the chain limit test server from server.listen(0, 'listening') to specify 127.0.0.1 which fixed the test failure's ECONNRESET error. I also added flushHeaders() and per-socket setNoDelay() as seen in #4496.

Features

N/A

Bug Fixes

N/A

Breaking Changes and Deprecations

None

Benchmarks

[bench:run] ┌─────────┬───────────────────────┬─────────┬────────────────────┬────────────┬─────────────────────────┐
[bench:run] │ (index) │ Tests                 │ Samples │ Result             │ Tolerance  │ Difference with slowest │
[bench:run] ├─────────┼───────────────────────┼─────────┼────────────────────┼────────────┼─────────────────────────┤
[bench:run] │ 0       │ 'http - no keepalive' │ 85      │ '4648.39 req/sec'  │ '± 2.91 %' │ '-'                     │
[bench:run] │ 1       │ 'node-fetch'          │ 20      │ '11330.13 req/sec' │ '± 2.88 %' │ '+ 143.74 %'            │
[bench:run] │ 2       │ 'axios'               │ 15      │ '11494.72 req/sec' │ '± 2.82 %' │ '+ 147.28 %'            │
[bench:run] │ 3       │ 'got'                 │ 20      │ '12015.76 req/sec' │ '± 2.92 %' │ '+ 158.49 %'            │
[bench:run] │ 4       │ 'undici - fetch'      │ 40      │ '14663.42 req/sec' │ '± 2.82 %' │ '+ 215.45 %'            │
[bench:run] │ 5       │ 'request'             │ 35      │ '14892.99 req/sec' │ '± 2.82 %' │ '+ 220.39 %'            │
[bench:run] │ 6       │ 'superagent'          │ 15      │ '18511.94 req/sec' │ '± 2.25 %' │ '+ 298.24 %'            │
[bench:run] │ 7       │ 'http - keepalive'    │ 15      │ '19108.18 req/sec' │ '± 2.74 %' │ '+ 311.07 %'            │
[bench:run] │ 8       │ 'undici - pipeline'   │ 55      │ '22599.61 req/sec' │ '± 2.99 %' │ '+ 386.18 %'            │
[bench:run] │ 9       │ 'undici - stream'     │ 30      │ '25741.88 req/sec' │ '± 2.96 %' │ '+ 453.78 %'            │
[bench:run] │ 10      │ 'undici - request'    │ 10      │ '28529.09 req/sec' │ '± 2.17 %' │ '+ 513.74 %'            │
[bench:run] │ 11      │ 'undici - dispatch'   │ 20      │ '29355.94 req/sec' │ '± 2.96 %' │ '+ 531.53 %'            │
[bench:run] └─────────┴───────────────────────┴─────────┴────────────────────┴────────────┴─────────────────────────┘

Status

docs/docs/best-practices/writing-tests.md

test/guard-disconnect.js

codecov-commenter · 2026-03-02T14:36:15Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.01%. Comparing base (b12081c) to head (ba79c89).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4844      +/-   ##
==========================================
- Coverage   93.02%   93.01%   -0.02%     
==========================================
  Files         112      112              
  Lines       35157    35157              
==========================================
- Hits        32705    32701       -4     
- Misses       2452     2456       +4

☔ View full report in Codecov by Sentry.
📢 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.

Signed-off-by: Sam Mayer <[email protected]> Signed-off-by: Sam Mayer <[email protected]>

Signed-off-by: Sam Mayer <[email protected]>

…est files Signed-off-by: Sam Mayer <[email protected]>

The first describe block used the global agent, leaving pooled connections alive after server.closeAllConnections() sent RST. On macOS, the async RST delivery could surface as ECONNRESET in the next describe block's fetch call. Give each block its own Client and await cleanup to eliminate the race. Signed-off-by: Sam Mayer <[email protected]>

This reverts commit 29ddaeb.

This reverts commit 78d583d.

…ervers The chain limit tests ran concurrently (node:test uses Promise.all within a Suite), so a shared server was unsafe: an aborted connection's async RST delivery on macOS could corrupt the server state for a sibling test. Replace the shared before/after server with a setupChainServer(t) helper that creates an isolated server+client pair per test and binds cleanup to t.after, which is scoped to each individual test context. Also removes the keepalive: false no-op (undici documents this flag as a noop outside of browser context).

metcoder95 · 2026-03-08T10:05:18Z

test/fetch/encoding.js


    await t.assert.rejects(
-      fetch(`http://localhost:${server.address().port}`, {
+      fetch(`http://127.0.0.1:${server.address().port}`, {


why the loopback change?

I noticed that Test with Node.js 20 on macos-latest was consistently failing in CI with the error:

[cause]: Error: read ECONNRESET at TCP.onStreamRead (node:internal/stream_base_commons:218:20) { errno: -54, code: 'ECONNRESET', syscall: 'read' }

I was unable to reproduce this locally on my mac, but setting an explicit localhost IP instead of resolving from localhost seemed to resolve the issue. Taking a look at some other open PRs (not using # to avoid a noisy cross-reference), I see that they're failing for what appears to be the same reason:

PR 4901's CI log

PR 4888's CI log

PR 4869's CI log

PR 4860's CI log

I have a hunch that the core issue is with how localhost resolves in the CI environment, but that particular change is unrelated to the focus of this PR. I could remove this change or present it as a separate PR, if you'd like.

mcollina

lgtm

samayer12 commented Feb 27, 2026

View reviewed changes

docs/docs/best-practices/writing-tests.md Outdated Show resolved Hide resolved

samayer12 force-pushed the 251-disconnect-checks branch 2 times, most recently from 5c94a36 to 8e69a9f Compare February 27, 2026 22:39

samayer12 marked this pull request as ready for review February 27, 2026 22:57

metcoder95 reviewed Mar 2, 2026

View reviewed changes

test/guard-disconnect.js Outdated Show resolved Hide resolved

samayer12 force-pushed the 251-disconnect-checks branch 3 times, most recently from a88b414 to 8e69a9f Compare March 2, 2026 15:39

samayer12 and others added 10 commits March 6, 2026 12:39

test: add unexpected disconnect guards to more client test files

5a11eb8

Signed-off-by: Sam Mayer <[email protected]> Signed-off-by: Sam Mayer <[email protected]>

test: add unexpected disconnect guards to remaining client test files

69f09b7

Signed-off-by: Sam Mayer <[email protected]>

Document use of guard-clause in tests

c4ee3db

test: add unexpected disconnect guards to content-length and header t…

ce9f027

…est files Signed-off-by: Sam Mayer <[email protected]>

create guardDisconnect() helper

167e631

Apply suggestion from @samayer12

dc05ff9

Revert "Apply suggestion from @samayer12"

480b136

This reverts commit 29ddaeb.

Revert "create guardDisconnect() helper"

0bd6b30

This reverts commit 78d583d.

samayer12 force-pushed the 251-disconnect-checks branch from 2737414 to 5cf144d Compare March 6, 2026 18:39

fix macos test failure by following pattern set in nodejs#4496

02ce816

samayer12 force-pushed the 251-disconnect-checks branch from 13e6c47 to 02ce816 Compare March 6, 2026 19:05

Specify host in CI

ba79c89

samayer12 requested a review from metcoder95 March 6, 2026 20:36

metcoder95 reviewed Mar 8, 2026

View reviewed changes

samayer12 requested a review from metcoder95 March 16, 2026 20:02

mcollina approved these changes Mar 17, 2026

View reviewed changes

metcoder95 approved these changes Mar 17, 2026

View reviewed changes

metcoder95 merged commit 1c5dc1a into nodejs:main Mar 17, 2026
37 checks passed

github-actions bot mentioned this pull request Mar 19, 2026

[Release] v7.24.5 #4915

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

test: add unexpected disconnect guards to more client test files#4844

test: add unexpected disconnect guards to more client test files#4844
metcoder95 merged 12 commits intonodejs:mainfrom
samayer12:251-disconnect-checks

samayer12 commented Feb 27, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Mar 2, 2026 •

edited

Loading

Uh oh!

metcoder95 Mar 8, 2026

Uh oh!

samayer12 Mar 16, 2026 •

edited

Loading

Uh oh!

mcollina left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

samayer12 commented Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

This relates to...

Rationale

Changes

Disconnect guards

Fix encoding test (Node 20 + macOS)

Features

Bug Fixes

Breaking Changes and Deprecations

Benchmarks

Status

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Mar 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

metcoder95 Mar 8, 2026

Choose a reason for hiding this comment

Uh oh!

samayer12 Mar 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

samayer12 commented Feb 27, 2026 •

edited

Loading

codecov-commenter commented Mar 2, 2026 •

edited

Loading

samayer12 Mar 16, 2026 •

edited

Loading