You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
While doing a code correctness review of the interceptors, I noticed that interceptors.dump() checks the wrong "aborted" flag in onResponseEnd. As a result, an aborted request whose response body is smaller than maxSize never settles: the request() promise hangs forever (no resolve, no reject). The same abort with a body larger than maxSize correctly rejects with AbortError.
Since maxSize defaults to 1 MiB and most real-world responses are smaller than that, the hang is the common case for aborted requests under this interceptor, not the exception.
Root cause
DumpHandler.onRequestStart replaces the controller's abort method with its own (lib/interceptor/dump.js:31):
controller.abort=this.#abort.bind(this)
#abort only sets the handler-local this.aborted = true. The underlying RequestController's private #aborted field is only ever set inside its original abort() method (lib/core/request.js:76-86), which can no longer be reached once the instance property shadows it. So after the hijack, controller.aborted can never become true.
onResponseStart (line 48) and onResponseData (line 72) correctly check the handler-local this.aborted, but onResponseEnd (line 87) checks this.#controller.aborted:
onResponseEnd(controller,trailers){if(this.#dumped){return}if(this.#controller.aborted===true){// <-- always false, dead conditionsuper.onResponseError(controller,this.reason)return}super.onResponseEnd(controller,trailers)}
Failure chain for "abort before response, body < maxSize":
response arrives → onResponseStart swallows it (line 48-50, return true), so the downstream RequestHandler.onResponseStart is never invoked and the promise stays pending
body (100 B) < maxSize (512 B) → the correct abort branch in onResponseData (line 69-77) is never reached
onResponseEnd → dead condition is false → forwards a normal onResponseEnd to a downstream handler that never saw onResponseStart; this.res?.push(null) is a no-op because res is null
the callback is never invoked → the promise never settles
The existing test test/interceptors/dump-interceptor.js:141 ("Should dump on already aborted request") asserts that an aborted request must reject with AbortError — it only passes because its body (1024 B) happens to exceed maxSize (512 B), hitting the correct check in onResponseData instead.
Reproduction
Standalone reproduction script:
'use strict'const{ test }=require('node:test')const{ createServer }=require('node:http')const{ once }=require('node:events')const{ Client, interceptors }=require('undici')test('aborted request whose response body is smaller than maxSize should reject',{timeout: 10000},async(t)=>{t.plan(1)constserver=createServer((req,res)=>{res.writeHead(200,{'Content-Type': 'application/octet-stream'})res.end(Buffer.alloc(100))// 100 bytes < maxSize (512)})t.after(()=>server.close())server.listen(0)awaitonce(server,'listening')constclient=newClient(`http://localhost:${server.address().port}`).compose(interceptors.dump({maxSize: 512}))t.after(()=>client.destroy())constabc=newAbortController()abc.abort()awaitt.assert.rejects(client.request({method: 'GET',path: '/',signal: abc.signal}),{name: 'AbortError'})})
not ok 1 - aborted request whose response body is smaller than maxSize should reject
failureType: 'testTimeoutFailure'
error: 'test timed out after 10000ms'
Changing Buffer.alloc(100) to Buffer.alloc(1024) (larger than maxSize) makes the same test pass, confirming the inconsistency: the outcome of an abort depends on the response body size.
Expected Behavior
The aborted request rejects with AbortError, exactly like the body > maxSize case and like the existing "Should dump on already aborted request" test asserts.
Actual Behavior
The request() promise never settles. Callers without their own timeout wait forever.
Suggested minimal fix: in onResponseEnd, check the handler-local flag like the other callbacks do, i.e. replace this.#controller.aborted === true with this.aborted === true. I can send a PR with this one-line fix plus a regression test.
Related: fix(interceptor/dump): fix handler lifecycle violation #4634 restructures the dump handler lifecycle, but its diff keeps the this.#controller.aborted check in onResponseEnd, so it does not address this hang (and the broken abort semantics may be contributing to its CI failures).
This report was prepared with AI assistance; I reproduced the issue locally and reviewed every conclusion.
Bug Description
While doing a code correctness review of the interceptors, I noticed that
interceptors.dump()checks the wrong "aborted" flag inonResponseEnd. As a result, an aborted request whose response body is smaller thanmaxSizenever settles: therequest()promise hangs forever (no resolve, no reject). The same abort with a body larger thanmaxSizecorrectly rejects withAbortError.Since
maxSizedefaults to 1 MiB and most real-world responses are smaller than that, the hang is the common case for aborted requests under this interceptor, not the exception.Root cause
DumpHandler.onRequestStartreplaces the controller'sabortmethod with its own (lib/interceptor/dump.js:31):#abortonly sets the handler-localthis.aborted = true. The underlyingRequestController's private#abortedfield is only ever set inside its originalabort()method (lib/core/request.js:76-86), which can no longer be reached once the instance property shadows it. So after the hijack,controller.abortedcan never becometrue.onResponseStart(line 48) andonResponseData(line 72) correctly check the handler-localthis.aborted, butonResponseEnd(line 87) checksthis.#controller.aborted:Failure chain for "abort before response, body < maxSize":
RequestHandlercallscontroller.abort(reason)→ hijacked method → onlyDumpHandler.aborted = trueonResponseStartswallows it (line 48-50,return true), so the downstreamRequestHandler.onResponseStartis never invoked and the promise stays pendingmaxSize(512 B) → the correct abort branch inonResponseData(line 69-77) is never reachedonResponseEnd→ dead condition isfalse→ forwards a normalonResponseEndto a downstream handler that never sawonResponseStart;this.res?.push(null)is a no-op becauseresisnullThe existing test
test/interceptors/dump-interceptor.js:141("Should dump on already aborted request") asserts that an aborted request must reject withAbortError— it only passes because its body (1024 B) happens to exceedmaxSize(512 B), hitting the correct check inonResponseDatainstead.Reproduction
Standalone reproduction script:
Output on current
main(2afeee4):Changing
Buffer.alloc(100)toBuffer.alloc(1024)(larger thanmaxSize) makes the same test pass, confirming the inconsistency: the outcome of an abort depends on the response body size.Expected Behavior
The aborted request rejects with
AbortError, exactly like thebody > maxSizecase and like the existing "Should dump on already aborted request" test asserts.Actual Behavior
The
request()promise never settles. Callers without their own timeout wait forever.Logs & Screenshots
See reproduction output above.
Environment
Additional context
onResponseEnd, check the handler-local flag like the other callbacks do, i.e. replacethis.#controller.aborted === truewiththis.aborted === true. I can send a PR with this one-line fix plus a regression test.this.#controller.abortedcheck inonResponseEnd, so it does not address this hang (and the broken abort semantics may be contributing to its CI failures).This report was prepared with AI assistance; I reproduced the issue locally and reviewed every conclusion.