nodejs / nodejs/undici

Cache.add() and Cache.addAll() never settle for a response with a body

Open
#5,615 2 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
JavaScript
Stars
7.7k
Forks
880
Avg merge
2d 16h
Merged PRs (30d)
68

Description

Bug Description

caches.open(...) then cache.add(request) or cache.addAll([request]) never settles when the response has a body. The promise stays pending forever — no resolution, no rejection, no timeout. A response with no body (204) settles fine, which is the discriminator.

Reproducible By
const { createServer } = require('node:http')
const { caches } = require('undici')

const server = createServer((req, res) => {
  if (req.url === '/empty') { res.writeHead(204); return res.end() }
  res.writeHead(200, { 'content-type': 'text/plain' })
  res.end('hello')
})

server.listen(0, '127.0.0.1', async () => {
  const base = `http://127.0.0.1:${server.address().port}`
  const cache = await caches.open('demo')

  const withTimeout = (p, label) => Promise.race([
    p.then(() => `${label}: settled`),
    new Promise(r => setTimeout(() => r(`${label}: STILL PENDING after 5000ms`), 5000))
  ])

  console.log(await withTimeout(cache.add(`${base}/body`), 'cache.add(200 with body)'))
  console.log(await withTimeout(cache.addAll([`${base}/body`]), 'cache.addAll([200 with body])'))
  console.log(await withTimeout(cache.add(`${base}/empty`), 'cache.add(204 no body)'))
  console.log('cache.keys() ->', (await cache.keys()).map(r => r.url))

  server.close()
})
Expected Behaviour

All three settle, and cache.keys() lists both URLs.

Actual Behaviour
cache.add(200 with body): STILL PENDING after 5000ms
cache.addAll([200 with body]): STILL PENDING after 5000ms
cache.add(204 no body): settled

cache.keys() -> [ 'http://127.0.0.1:56729/empty' ]

Only the bodyless response is ever stored. A plain fetch() of the same URL works, and cache.put(request, response) works — it is specific to add/addAll.

Where it comes from

Cache.addAll() (and Cache.add(), which delegates to it) hands fetching() a processResponseEndOfBody callback and awaits the promise it resolves — lib/web/cache/cache.js:188. It never reads the response body.

In fetchFinale, that callback only runs on one of two paths — lib/web/fetch/index.js:1129-1145:

if (internalResponse.body == null) {
  processResponseEndOfBody()
} else {
  // mcollina: all the following steps of the specs are skipped.
  // The internal transform stream is not needed.
  // See https://github.com/nodejs/undici/pull/3093#issuecomment-2050198541
  finished(internalResponse.body.stream, () => {
    processResponseEndOfBody()
  })
}

So with a body the callback waits for the body stream to finish, and nothing drains it, so it never does — addAll waits on a stream that is waiting on addAll. With body == null the callback fires immediately, which is exactly why the 204 works.

Per the spec, step 3 of that skipped section sets processResponseEndOfBody as the transform stream's flushAlgorithm, so it would fire as the body passed through rather than when a consumer finished it. The shortcut from #3093 is sound for the paths that read the body; add/addAll are the case that does not, because for them storing the response is the consumption.

I did not want to guess the right shape of the fix — draining the body inside addAll before awaiting, or restoring something equivalent to the flush hook for this path, are both plausible and the second has wider consequences.

Environment

Node v24.13.0, Windows, current main (10898087), and the same on 8.9.0. caches is exported from index.js:179.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by running the provided reproduction, then read lib/web/cache/cache.js around line 188 and lib/web/fetch/index.js around lines 1129-1145. Trace how Cache.add() and addAll() consume response bodies and how processResponseEndOfBody is resolved. Done means body-bearing responses settle and both URLs appear in cache.keys(), without regressing bodyless responses or fetch paths.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript, node.js
Domain
api, backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.