Skip to content

http: preserve socket errors as response error causes - #66061

Closed
XadillaX wants to merge 1 commit into
nodejs:mainfrom
XadillaX:fix-http-response-socket-error
Closed

XadillaX wants to merge 1 commit into
nodejs:mainfrom
XadillaX:fix-http-response-socket-error

Conversation

@XadillaX

@XadillaX XadillaX commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

When a socket error interrupts an HTTP response, the request receives the original error, but the response and its downstream pipeline receive only ECONNRESET: aborted. The underlying cause is lost.

Preserve that original error as cause on the response error, keeping the existing ECONNRESET code, aborted message, and event order. This applies to TLS errors and actual TCP resets alike. Premature closure without an underlying error leaves cause absent.

This came out of investigating #66001. I could not reproduce or identify the root cause of the reported TLS decryption failure on an intact stream, but confirmed the separate loss of error context described above. The original failure remains unresolved; this PR is limited to preserving diagnostic context. The TLS regression test deliberately sends an invalid record after response headers have been received to test error propagation.

Validation on macOS arm64:

  • Release build succeeded.
  • All 721 selected HTTP, HTTPS, TLS, error, and TCP reset tests passed.
  • Regression coverage includes TLS 1.2/1.3, real TCP resets, explicit destruction, closure without an error, cause identity and property attributes, and event order.
  • The TLS regression test fails on unmodified Node v24.21.0 because cause is missing.
  • JavaScript lint, Markdown lint, and git diff --check passed.

Refs: #66001

AI assistance: GPT-6 assisted with investigation, implementation, tests, and this description. The compatibility approach was selected through discussion with the contributor; the validation above was run by the assistant.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added errors Issues and PRs related to JavaScript errors originating in Node.js core. http Issues and PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Sep 16, 2026
Keep the original socket error as the cause of ECONNRESET errors emitted
when an HTTP response closes before completion. Preserve the existing
aborted message, error code, and event order. Leave cause absent when
there is no underlying error.

Allow ConnResetException to accept Error options, document the behavior,
and cover TLS record errors, TCP resets, explicit destruction, and
premature closure without a socket error.

This follows investigation of nodejs#66001 and addresses lost error context.
The original TLS decryption failure remains unresolved.

Refs: nodejs#66001
Signed-off-by: XadillaX <i@2333.moe>
@XadillaX
XadillaX force-pushed the fix-http-response-socket-error branch from dceab63 to cd91d44 Compare September 16, 2026 12:21
@codecov

codecov Bot commented Sep 16, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.26%. Comparing base (565f69f) to head (cd91d44).
⚠️ Report is 280 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #66061      +/-   ##
==========================================
+ Coverage   89.99%   90.26%   +0.26%     
==========================================
  Files         784      789       +5     
  Lines      268410   271477    +3067     
  Branches    51124    51811     +687     
==========================================
+ Hits       241562   245048    +3486     
+ Misses      17385    16919     -466     
- Partials     9463     9510      +47     
Files with missing lines Coverage Δ
lib/_http_client.js 97.66% <100.00%> (+0.02%) ⬆️
lib/internal/errors.js 98.81% <100.00%> (-0.01%) ⬇️

... and 117 files with indirect coverage changes

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

@yx64

yx64 commented Sep 20, 2026 •

Copy link
Copy Markdown

Edit — the attribution above is wrong. The root cause is not in Node at all: the proxy stack injects an unauthenticatable TLS record at end of stream. See #66061 (comment) for the correction. The conditions listed below (TLS 1.3, async downstream, and so on) are not defect conditions — they only determine whether the client reads far enough to encounter that record. The assessment under "On this PR specifically" still stands.

Thanks for picking this up, and for the honest note that the original failure did not
reproduce for you.

That is very likely the report's fault, not your attempt's. A second round of controlled
runs has just overturned three of the conditions I originally listed, and added two I had
missed entirely:

Original report said Actually
a node:zlib stream is required not required — any stage that defers its push to a later event-loop turn does it
the consumer must lag not required — reproduces with no artificial lag at all
per-chunk async deferral was ruled out wrong — that control's lag was aligned per-unit-of-data instead of per total duration, so it was never an equal-lag comparison
— TLS 1.3 is required — forcing maxVersion: 'TLSv1.2' passes 4/4, same-minute 1.3 control fails 2/2
— the stage immediately downstream of the response must be asynchronous — synchronous 3/3 pass vs asynchronous 2/2 fail, everything else held constant

So an environment built from the original condition list could easily be one that never
triggers: it would have the decompressor and the slow consumer, and might well be on TLS 1.2,
or have only synchronous stages between the response and the sink.

There is also a trigger-rate caveat I had not characterised: the minimal configuration fired
5/5 in one window and passed 5/5 about forty minutes later with nothing changed, while the
heavier configuration still failed 3/3 at that same moment. A single non-reproducing run
proves nothing in either direction unless it is paired with a same-minute positive control.

Corrected conditions, measurements, controls and a reduced repro script are in the issue
update: #66001 (comment)

On this PR specifically

It fixes something I had to work around by hand, and I think that is worth stating
independently of whether the TLS issue is ever root-caused.

While narrowing the conditions above, the probe had to be modified to attach its own listener
to the TLSSocket and print the first TLS-layer error on every result line, passing runs
included. Reasons, both of which this PR would have removed:

  • From the HTTP layer alone, a run that fails with bad record mac and a run that fails for
    an unrelated reason are indistinguishable — both surface as ECONNRESET: aborted with
    res.complete === false. Two earlier rounds of this investigation mistook bugs in my own
    test harness for reproductions on exactly that basis, and were only caught by separately
    instrumenting event order.
  • The converse also mattered: I needed to confirm that runs which passed were genuinely
    clean, rather than having hit a decryption failure that happened not to lose any data. That
    check is only possible with the underlying error in hand. (It turned out to matter — with
    Connection: close, a synchronous downstream delivers every declared byte with a correct
    hash and still raises the same bad record mac on the close_notify record.)

With cause preserved, neither workaround is necessary: the diagnostic is available to any
consumer of the response error, which is precisely the situation the original report
described as "rewrites the root cause into a network error and is why applications downstream
blame the proxy or the network".

One note on the regression test, not an objection: sending a deliberately invalid record after
the response headers is the right shape for testing cause propagation, and it is worth being
explicit that it does not reproduce the underlying issue in #66001 — there the stream is
intact and decryption fails anyway. The two are independent, and I read this PR as scoped to
error-context preservation only, which is how you have described it.


AI assistance: the experiments and write-up behind this comment were done with Claude
(Opus 5). All runs were executed on my own host against the real path; the numbers above are
measured, not estimated.

@yx64

yx64 commented Sep 20, 2026

Copy link
Copy Markdown

Follow-up, and a correction to my earlier comment here: I had the root cause wrong.

The defect is not in Node at all. Capturing the same connection at both ends showed the
client's TLS layer receiving exactly 24 bytes more than the origin sent — 5 bytes of TLS
record header plus a 19-byte body, i.e. one complete record. The proxy stack injects a TLS
record at end of stream that cannot be authenticated, and Node correctly rejects it. Byte
accounting and controls are here: #66001 (comment)

I've closed #66001 as invalid.

None of this affects this PR. Preserving the underlying socket error as cause stands on
its own merits — it actually cost me real time during this investigation, since without it a
failing run and a passing run are indistinguishable from the HTTP layer, and I had to patch my
probe to attach its own listener to the TLSSocket just to tell them apart. Two earlier
rounds of this investigation mistook harness bugs for reproductions on exactly that basis.

Sorry for the noise, and thanks again for looking into it.


AI assistance: the investigation and write-up behind this comment were done with Claude
(Opus 5). All measurements were taken on my own host against the real path.

@panva panva added author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Sep 25, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 25, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@panva panva added the resume-ci Add this label to resume the latest eligible Jenkins CI run on a PR with an approving review. label Sep 25, 2026
@github-actions github-actions Bot removed the resume-ci Add this label to resume the latest eligible Jenkins CI run on a PR with an approving review. label Sep 25, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

panva pushed a commit that referenced this pull request Sep 25, 2026
Keep the original socket error as the cause of ECONNRESET errors emitted
when an HTTP response closes before completion. Preserve the existing
aborted message, error code, and event order. Leave cause absent when
there is no underlying error.

Allow ConnResetException to accept Error options, document the behavior,
and cover TLS record errors, TCP resets, explicit destruction, and
premature closure without a socket error.

This follows investigation of #66001 and addresses lost error context.
The original TLS decryption failure remains unresolved.

Refs: #66001
Signed-off-by: XadillaX <i@2333.moe>
PR-URL: #66061
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Tim Perry <pimterry@gmail.com>
@panva

panva commented Sep 25, 2026

Copy link
Copy Markdown
Member

Landed in 5ebf690

@panva panva closed this Sep 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. errors Issues and PRs related to JavaScript errors originating in Node.js core. http Issues and PRs related to the http subsystem. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants