fix: drop response chunks after the response stream is destroyed (#5356) by cesarvspr · Pull Request #5357 · nodejs/undici · GitHub

This relates to...

Rationale

When a request is aborted mid-stream while its body is being consumed (e.g. body.json() with an AbortSignal timeout), a late chunk could crash the process with TypeError: Cannot read properties of null (reading 'push') instead of the consume promise simply rejecting with the AbortError.

Root cause: the abort-signal listener in lib/api/api-request.js destroys this.res in place but, unlike onResponseError, does not null it. So a chunk that arrives afterwards - for example flushed asynchronously by the decompress interceptor after the abort - passes the existing if (!this.res) guard in onResponseData (the reference is still set, just destroyed) and is pushed into the torn-down BodyReadable, reaching consumePush on a consume whose body has already been nulled.

Note: this supersedes the earlier revision of this PR, which guarded consumePush in readable.js. As @mcollina pointed out, the bug is elsewhere - that function should not be called after teardown. The fix now stops the late chunk at the handler, so consumePush is never reached.

Changes

lib/api/api-request.js: onResponseData now returns early when the response stream is already destroyed (!this.res || this.res.destroyed), dropping late chunks at the handler instead of pushing them into a torn-down stream.

Alternative considered

Nulling this.res in the abort-signal listener (to mirror onResponseError) also fixes it and removes the asymmetry directly. I chose the onResponseData guard because it is a smaller surface, uses the public Readable.destroyed, and matches the destroyed-aware request-body guard in api-pipeline.js. Happy to switch to nulling this.res if you prefer the symmetry - the regression test covers both.

Bug Fixes

A late chunk arriving after an aborted consume no longer crashes the process; the consume promise rejects with the AbortError as expected.

Breaking Changes and Deprecations

N/A

Status

I have read and agreed to the Developer's Certificate of Origin
Tested (new regression test test/issue-5356.js, RED without the fix, GREEN with it; readable/request/client-request/decompress/abort+signal suites pass; lint clean)
[S] Benchmarked (optional)
Documented (code comment; no public API change)
Review ready
In review
Merge ready

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.35%. Comparing base (dd1e50a) to head (99861a0).
⚠️ Report is 48 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5357      +/-   ##
==========================================
+ Coverage   93.23%   93.35%   +0.11%     
==========================================
  Files         110      110              
  Lines       36668    36999     +331     
==========================================
+ Hits        34189    34540     +351     
+ Misses       2479     2459      -20

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

I think the bug is elsewhere. That function should not be called.

When a request is aborted mid-stream while its body is being consumed (e.g. body.json()), the abort-signal listener destroys `this.res` in place but does not null it, unlike onResponseError. A chunk arriving afterwards - for example flushed asynchronously by the decompress interceptor - passed the `if (!this.res)` guard in onResponseData and was pushed into the torn-down stream, crashing with "Cannot read properties of null (reading 'push')" instead of the consume promise rejecting with the AbortError. Guard onResponseData against a destroyed response stream so the late chunk is dropped at the handler, before it can reach consumePush. Fixes: nodejs#5356 Signed-off-by: cesarvspr <vinicius_spr@hotmail.com>

thx for the heads up @mcollina

The abort listener destroys this.res but never nulls it like onResponseError does, so the late decompressor chunk slips past the if (!this.res) check and into consumePush. Moved the guard to onResponseData (drop the chunk if the stream's already destroyed), reverted the readable.js change, added a test.

Could also just null this.res in the abort listener to match onResponseError. Went with the guard since it touches less, but easy to switch if you prefer that.

thx for the heads up @mcollina

The abort listener destroys this.res but never nulls it like onResponseError does, so the late decompressor chunk slips past the if (!this.res) check and into consumePush. Moved the guard to onResponseData (drop the chunk if the stream's already destroyed), reverted the readable.js change, added a test.

Could also just null this.res in the abort listener to match onResponseError. Went with the guard since it touches less, but easy to switch if you prefer that.

Fixing the underlying bug is what should be done, not adding another if condition.

…ta (nodejs#5356) Addresses review: fix the underlying invariant violation rather than adding another guard condition. The codebase contract is that once the response stream is torn down, this.res is null, so the existing `if (!this.res) return` guard in onResponseData drops late chunks. onResponseError already obeys this. The abort-signal listener was the one producer that broke it: it destroyed this.res in place but left the reference set, so a chunk flushed after the abort (e.g. an async decompressor flush) slipped past the guard, reached BodyReadable.push -> consumePush, and crashed on the consume body that consumeFinish had already nulled. Restore the invariant at the producer: null this.res before destroying in the abort listener, mirroring onResponseError. The onResponseData guard reverts to the original bare `!this.res`; the earlier readable.js change is reverted. consumePush is now genuinely unreachable on this path, not guarded.

lgtm

metcoder95 approved these changes Jun 4, 2026

View reviewed changes

mcollina requested changes Jun 5, 2026

View reviewed changes

cesarvspr force-pushed the fix/consume-push-null-guard branch from 70e6d62 to 33bd0c8 Compare June 6, 2026 18:32

cesarvspr changed the title fix: guard against late chunk after consume teardown fix: drop response chunks after the response stream is destroyed (#5356) Jun 6, 2026

cesarvspr requested a review from mcollina June 6, 2026 20:45

mcollina approved these changes Jun 18, 2026

View reviewed changes

mcollina merged commit e71624e into nodejs:main Jun 18, 2026
38 checks passed

konrad-erli mentioned this pull request Jul 1, 2026

consumePush crashes when chunk arrives after body.json() teardown #5356

Closed

github-actions Bot mentioned this pull request Jul 2, 2026

[Release] v8.6.0 #5491

Merged

coderabbitai Bot mentioned this pull request Aug 22, 2026

ci: gate commits on custom node compatibility Comfy-Org/ComfyUI_frontend#15590

Merged

fix: drop response chunks after the response stream is destroyed (#5356) by cesarvspr · Pull Request #5357 · nodejs/undici · GitHub

Uh oh!

Uh oh!

fix: drop response chunks after the response stream is destroyed (#5356) - #5357

fix: drop response chunks after the response stream is destroyed (#5356)#5357
mcollina merged 2 commits into
nodejs:mainfrom
cesarvspr:fix/consume-push-null-guard

cesarvspr commented Jun 4, 2026 •
edited
Loading

Uh oh!

Uh oh!

codecov-commenter commented Jun 4, 2026 •
edited
Loading

Uh oh!

Uh oh!

mcollina left a comment

Uh oh!

cesarvspr commented Jun 6, 2026 •
edited
Loading

Uh oh!

Uh oh!

mcollina commented Jun 16, 2026

Uh oh!

mcollina left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fix: drop response chunks after the response stream is destroyed (#5356) by cesarvspr · Pull Request #5357 · nodejs/undici · GitHub

Uh oh!

Uh oh!

fix: drop response chunks after the response stream is destroyed (#5356) - #5357

fix: drop response chunks after the response stream is destroyed (#5356)#5357mcollina merged 2 commits intonodejs:mainnodejs/undici:mainfrom cesarvspr:fix/consume-push-null-guardcesarvspr/undici:fix/consume-push-null-guardCopy head branch name to clipboard

Conversation

cesarvspr commented Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

This relates to...

Rationale

Changes

Alternative considered

Bug Fixes

Breaking Changes and Deprecations

Status

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

cesarvspr commented Jun 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina commented Jun 16, 2026

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

fix: drop response chunks after the response stream is destroyed (#5356)#5357
mcollina merged 2 commits into
nodejs:mainfrom
cesarvspr:fix/consume-push-null-guard

cesarvspr commented Jun 4, 2026 •
edited
Loading

codecov-commenter commented Jun 4, 2026 •
edited
Loading

cesarvspr commented Jun 6, 2026 •
edited
Loading