Conversation
Node.js 24.20 (nodejs/node#64847) calls ClientRequest#end callbacks with the error that prevented the flush. got@11's _final forwards it as a stream error while a retry is scheduled, so the promise rejects early with ERR_SOCKET_CLOSED_BEFORE_CONNECTION, the retry attaches onCancel to a settled promise, and the retried request's error goes uncaught. Add a `restoreEndCallback` beforeRequest hook that wraps http/https.request so end callbacks keep their previous contract. fetch appends it to the user hooks, and it is exported for other got@11 instances. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VZiY7VMgg1nEMkCezwMXEC
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change adds request callback wrappers for Node.js HTTP and HTTPS requests. It appends the wrapper as a got ChangesClientRequest callback restoration
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant fetch
participant got
participant restoreEndCallback
participant ClientRequest
fetch->>got: create request with beforeRequest hooks
got->>restoreEndCallback: run hook
restoreEndCallback->>ClientRequest: wrap end callback
got->>ClientRequest: perform request
ClientRequest-->>got: report eligible callback result
Merge Risk: ⚪ Minimal · up to The compatibility wrapper preserves retry handling while retaining custom request and HTTP/2 paths. No actionable merge-blocking risk is identified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f0392a5. Configure here.
| if (options.request || options.http2) return | ||
| const { request } = options.url.protocol === 'https:' ? https : http | ||
| options.request = (...args) => wrapEnd(request(...args)) | ||
| } |
There was a problem hiding this comment.
Redirects can use the wrong protocol
High Severity
restoreEndCallback binds options.request to http.request or https.request from the first options.url protocol, then returns early whenever options.request is already set. got reuses that function on later beforeRequest runs, so an http: to https: redirect keeps calling http.request and never does TLS. Same-protocol redirects still work; mixed-protocol ones fail in fetch.
Reviewed by Cursor Bugbot for commit f0392a5. Configure here.


Fixes the
test/index.jsuncaughtENOTFOUNDin https://github.com/microlinkhq/html-get/actions/runs/34828445442/job/103935300740. Replaces #284 and keeps retries on.Root cause
The
content-typebump in #283 has nothing to do with it. CI moved from Node.js 24.19.0 (last green run) to 24.20.0.Node.js 24.20 includes nodejs/node#64847 (
0ad55e5618):ClientRequest#end(cb)now callscbwith the error that stopped the flush. Before,cbwas a'finish'listener and never ran on failure.That breaks got@11 when a network error is retried:
ENOTFOUND,ECONNREFUSED) and got starts the retry backoff without destroying the request.ERR_SOCKET_CLOSED_BEFORE_CONNECTION. Node 24.20 passes it to got's_final→end()callback, and the request emits'error'.as-promise'sonce('error')rejects the promise early with that error.makeRequest()callsonCancelon the settled promise, which throws before the new request gets an error listener. Its error is uncaught.Hooks traced on 24.20:
beforeRetryruns about 1s after the early rejection, so it can't be the fix.Fix
restoreEndCallback, a gotbeforeRequesthook insrc/got-hooks.js:options.requesttohttp.request/https.requestwithend()wrapped, so the callback keeps its old contract: it runs on finish, a flush error is dropped, andERR_STREAM_ALREADY_FINISHEDis still passed through. The socket error still reaches got through the request's'error'event, so got reports the original error.options.requestandhttp2requests alone.fetchappends it after anygotOpts.hooks.beforeRequest.getHTML.restoreEndCallbackso other got@11 instances (e.g. the microlink API's sharedgotOpts) can use it. Documented in the README.Tests
test/got-hooks.js:wrapEnd(finish, flush error, already-finished, no callback) andrestoreEndCallback(protocol, userrequest, http2). These catch a regression on any Node version.fetchretryECONNREFUSEDonce (checked viabeforeRetry), reject or resolve with the original error, and nothing is uncaught. Before the hook was wired in, thefetchtest crashed the file on 24.20 with the CI error.beforeRequesthooks still run.Results:
standardis clean.rejected ENOTFOUND,retryCount 1.Not covered here: an end-to-end run through the API's got-scraping proxy agents. They are plain
http/httpsagents passed tohttp.requestunchanged; that belongs in the follow-up API PR.🤖 Generated with Claude Code
https://claude.ai/code/session_01VZiY7VMgg1nEMkCezwMXEC
Note
Medium Risk
Patches low-level HTTP request creation for every fetch and exported got hook usage; behavior is narrow but sits on the critical network path.
Overview
Node.js 24.20 changed
ClientRequest#endcallbacks so flush failures reach got v11 while a retry is pending, causing early promise rejection and uncaught errors on the retried request. This PR adds arestoreEndCallbackgotbeforeRequesthook that wrapshttp/httpsrequests so end callbacks behave like pre-24.20 (drop flush errors likeERR_SOCKET_CLOSED_BEFORE_CONNECTION, still passERR_STREAM_ALREADY_FINISHED).The hook is always appended to
gotOpts.hooks.beforeRequestin the internalfetchpath and is exported asgetHTML.restoreEndCallbackfor other got v11 clients. UserbeforeRequesthooks, customoptions.request, and http2 are left unchanged.README documents the behavior;
test/got-hooks.jsadds unit and integration coverage for retries on refused connections without uncaught exceptions.Reviewed by Cursor Bugbot for commit f0392a5. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Bug Fixes
Documentation