fix: swallow client-aborted download errors after headers sent#232
Open
colachg wants to merge 1 commit intofalcondev-oss:devfrom
Open
fix: swallow client-aborted download errors after headers sent#232colachg wants to merge 1 commit intofalcondev-oss:devfrom
colachg wants to merge 1 commit intofalcondev-oss:devfrom
Conversation
When a runner aborts a `/download/<id>` request mid-stream (cancelled job, parallel runner already finished, etc.), the response stream's error propagates up from `sendStream`. By that point response headers are already on the wire, so Nitro's default error handler crashes with `ERR_HTTP_HEADERS_SENT` while trying to write a 500 response, and the error surfaces as an unhandled request error in logs. Wrap the `sendStream` call in a try/catch and check `event.node.res.headersSent` — if the response has already started, log at debug and return; otherwise re-throw so legitimate pre-flush errors still produce a proper HTTP error response.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
GET /download/<cacheEntryId>proxies a cache entry through the cache-server when the entry is unmerged (orENABLE_DIRECT_DOWNLOADSis off / unsupported by the storage adapter). When the runner aborts mid-stream — cancelled job, parallel runner already finished, network hiccup — the response stream's error bubbles up out ofsendStream. Headers and some body bytes are already on the wire by then, so Nitro'sdefaultNitroErrorHandlercrashes trying to write a 500 response:It's not a process crash (
fatal: false), but every aborted download produces a noisy unhandled-error log on the server, and the runner-side gets a misleading 500 response code logged after a partial download. We saw this firing many times an hour on our deployment of v9.4.7 — all from runner aborts on legitimate caches.Repro recipe:
Storage.download()returns the merge-as-side-effect proxy stream (i.e. an unmerged entry, or any storage whereENABLE_DIRECT_DOWNLOADSfalls through todefaultUrlinlib/storage.ts)./download/<id>and abort the client connection (curl --max-time 1against a >100MB cache entry, or kill the runner).ERR_HTTP_HEADERS_SENTunhandled error in cache-server logs.Fix
Wrap the
sendStreamcall in a try/catch. If the response has already started flushing (event.node.res.headersSent), log at debug and return — there's nothing more we can do with the response. If headers aren't sent yet (a true pre-flush failure), re-throw so Nitro can build a proper HTTP error response as before.This is the smallest change that addresses the unhandled-error noise without affecting legitimate error paths.
Alternatives considered
Storage.download'spumpPartsToStreams.catch— also viable, but mixes HTTP-layer concerns into the storage layer. The route handler is the right place to decide HTTP-level error semantics.ERR_HTTP_HEADERS_SENTglobally in a Nitro error hook — too broad; would hide unrelated bugs.Testing
pnpm lintpasses locally.Cache file not found404) still propagate correctly becauseevent.node.res.headersSentisfalseuntilsendStreamwrites the first chunk.