Skip to content

fix(express): skip the fallback 404 once the response has started - #2024

Open
dchaudhari7177 wants to merge 1 commit into
nestjs:masterfrom
dchaudhari7177:fix/spa-fallback-headers-sent
Open

fix(express): skip the fallback 404 once the response has started#2024
dchaudhari7177 wants to merge 1 commit into
nestjs:masterfrom
dchaudhari7177:fix/spa-fallback-headers-sent

Conversation

@dchaudhari7177

Copy link
Copy Markdown

PR Checklist

  • The commit message follows our guidelines
  • Tests for the changes have been added
  • Docs have been added / updated (not applicable — no API change)

PR Type

  • Bugfix

What is the current behavior?

Issue Number: #2023

The SPA fallback answers every res.sendFile error with a 404:

res.sendFile(indexFilePath, null, (err: Error) => {
  if (err) {
    const error = new NotFoundException(err.message);
    res.status(error.getStatus()).send(error.getResponse());
  }
});

sendFile also reports a client abort through that callback, and by then the headers and part of the body are already on the wire. res.status().send() then throws ERR_HTTP_HEADERS_SENT. Because Express invokes the callback from a setImmediate, the throw lands outside the middleware chain and outside any exception filter — so a page reload or a flaky mobile connection during the index stream takes down the whole process, not just the request.

What is the new behavior?

The 404 is skipped once res.headersSent is true, since there is no longer a response to write. Errors raised before anything is sent — a missing or unreadable index file — still produce the 404 exactly as before.

I confirmed the mechanism in isolation:

UNGUARDED -> ERR_HTTP_HEADERS_SENT | headersSent = true
GUARDED   -> skipped, no throw

Tests

Added an e2e case that drives the real fallback callback with the response already started, standing in for the abort so the test is deterministic (a genuine mid-stream disconnect is a race). It captures anything the callback throws and asserts nothing was thrown.

Two things I had to get right for the test to be meaningful, both verified rather than assumed:

  • It requests an unknown path (/some/spa/route). My first attempt used /, which express.static serves straight from disk — sendFile was never called, so the test passed with and without the fix. I instrumented it to check, and sendFile called = false confirmed the test was vacuous.
  • It asserts on the captured error, not just the status code. The response is already 200 either way, so status alone cannot distinguish the two.

With both corrected, reverting only lib/ fails it precisely:

× should not try to respond again once the headers are sent
  "code": "ERR_HTTP_HEADERS_SENT"

Verification

  • npm run test:e2e23 passed (22 before, +1 new)
  • npm run build → clean
  • oxlint lib/ → clean

I left the fastify loader alone: its 404 paths run before streaming begins (inside the fs.stat callback, ahead of createReadStream), so the headers are not yet sent there.

prettier --check flags both touched files, but that is pre-existing on a clean tree — verified by stashing — so I did not mix a reformat into this diff. The lint-staged hook did format the staged changes themselves.

Does this PR introduce a breaking change?

  • No

The SPA fallback answers every error from res.sendFile with a 404. That
callback also reports a client abort, and by then the headers and part of
the body are already on the wire, so res.status().send() throws
ERR_HTTP_HEADERS_SENT.

Express invokes the callback from a setImmediate, so the throw lands
outside the middleware chain and outside any exception filter. An aborted
page load therefore takes down the whole process instead of failing the
one request.

Bail out once res.headersSent is true, since there is no longer a response
to write. Errors raised before anything is sent, such as a missing index
file, still produce the 404.

Closes nestjs#2023
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.

2 participants