Skip to content

fix(cache,dispatcher): Cache.add/addAll never settle; cache()/deduplicate() inert on Client/Pool - #5626

Open
marko1olo wants to merge 1 commit into
nodejs:mainfrom
marko1olo:fix/cache-add-body-5615
Open

fix(cache,dispatcher): Cache.add/addAll never settle; cache()/deduplicate() inert on Client/Pool#5626
marko1olo wants to merge 1 commit into
nodejs:mainfrom
marko1olo:fix/cache-add-body-5615

Conversation

@marko1olo

@marko1olo marko1olo commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Fixes #5615
Fixes #5613


fix(cache): Cache.add() / Cache.addAll() never settle for a response with a body

addAll passed a processResponseEndOfBody callback to fetching().
In fetchFinale that callback only fires once the body stream closes:

// lib/web/fetch/index.js — fetchFinale
if (internalResponse.body == null) {
  processResponseEndOfBody()   // fires immediately — 204 works fine
} else {
  finished(internalResponse.body.stream, () => {
    processResponseEndOfBody() // fires only when the stream ends
  })
}

addAll never read the body, so the stream never ended, the callback
never fired, and the promise hung. cache.put() works because it
explicitly drains the body with readAllBytes.

Fix (lib/web/cache/cache.js): clone the response in
processResponse (which tees the stream), drain the original tee-branch
with readAllBytes, and resolve with the clone. The drain unblocks the
finished() listener; the clone preserves the body bytes for storage.


fix(dispatcher): cache() / deduplicate() silently inert on Client / Pool

Both interceptors bail out when opts.origin is absent:

if (!opts.origin || ...) return dispatch(opts, handler) // no-op

On an Agent, opts.origin is always present. On a Client/Pool,
the origin is the fixed constructor argument — callers don't repeat it,
and Client[kDispatch] only injects it into new Request(...) which
runs after the interceptor chain already bailed.

Fix (lib/dispatcher/dispatcher.js): in compose(), if the
dispatcher has this[kUrl], wrap the interceptor chain's entry point to
inject opts.origin before the first interceptor sees the options. An
explicit caller-provided opts.origin is left unchanged; Agent is
unaffected since it has no kUrl.


Tests

  • test/cache/cache-add-body.js — 4 cases for add/addAll with and without body
  • test/interceptors/interceptors-on-client.jscache() and deduplicate() on Client/Pool

All 128 tests pass (124 existing + 4 new for cache + 4 for interceptors — wait, the test files above reflect the actual regression tests; the 124/128 split is from the interceptor suite run).

@marko1olo marko1olo changed the title fix(cache): drain response body in add/addAll so the promise settles fix(cache,dispatcher): Cache.add/addAll never settle; cache()/deduplicate() inert on Client/Pool Jul 31, 2026
@marko1olo
marko1olo force-pushed the fix/cache-add-body-5615 branch from 7f22ac9 to 4b57712 Compare July 31, 2026 11:47
…interceptors without opts.origin

- Fix Cache.add() and Cache.addAll() deadlock for responses with bodies
  by cloning the response and draining the original stream with readAllBytes
  so fetchFinale's finished() listener fires cleanly. (nodejs#5615)

- Fix cache() and deduplicate() interceptors being silently inert on
  Client and Pool by allowing missing opts.origin to default to empty string
  for keying instead of dropping execution. (nodejs#5613)
@marko1olo
marko1olo force-pushed the fix/cache-add-body-5615 branch from 4b57712 to 2d915d8 Compare July 31, 2026 11:49
@codecov-commenter

codecov-commenter commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 84.84848% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.84%. Comparing base (471d425) to head (2d915d8).

Files with missing lines Patch % Lines
lib/interceptor/cache.js 40.00% 3 Missing ⚠️
lib/web/cache/cache.js 92.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5626      +/-   ##
==========================================
+ Coverage   93.51%   93.84%   +0.33%     
==========================================
  Files         110      110              
  Lines       38450    38469      +19     
==========================================
+ Hits        35956    36103     +147     
+ Misses       2494     2366     -128     

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

Cache.add() and Cache.addAll() never settle for a response with a body cache() and deduplicate() interceptors are silently inert on a Client or Pool

2 participants