Skip to content

fix: keep got retries working on Node.js 24.20 - #285

Closed
Kikobeats wants to merge 1 commit into
masterfrom
Kikobeats/got-end-callback-f8fa318b
Closed

Kikobeats wants to merge 1 commit into
masterfrom
Kikobeats/got-end-callback-f8fa318b

Conversation

@Kikobeats

@Kikobeats Kikobeats commented Sep 14, 2026 •

Copy link
Copy Markdown
Member

Fixes the test/index.js uncaught ENOTFOUND in https://github.com/microlinkhq/html-get/actions/runs/34828445442/job/103935300740. Replaces #284 and keeps retries on.

Root cause

The content-type bump 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 calls cb with the error that stopped the flush. Before, cb was a 'finish' listener and never ran on failure.

That breaks got@11 when a network error is retried:

  1. The request fails (ENOTFOUND, ECONNREFUSED) and got starts the retry backoff without destroying the request.
  2. The pending headers write fails with ERR_SOCKET_CLOSED_BEFORE_CONNECTION. Node 24.20 passes it to got's _final → end() callback, and the request emits 'error'.
  3. as-promise's once('error') rejects the promise early with that error.
  4. The backoff timer fires and makeRequest() calls onCancel on the settled promise, which throws before the new request gets an error listener. Its error is uncaught.

Hooks traced on 24.20: beforeRetry runs about 1s after the early rejection, so it can't be the fix.

Fix

restoreEndCallback, a got beforeRequest hook in src/got-hooks.js:

  • Sets options.request to http.request / https.request with end() wrapped, so the callback keeps its old contract: it runs on finish, a flush error is dropped, and ERR_STREAM_ALREADY_FINISHED is still passed through. The socket error still reaches got through the request's 'error' event, so got reports the original error.
  • Leaves a user-provided options.request and http2 requests alone.
  • fetch appends it after any gotOpts.hooks.beforeRequest.
  • Exported as getHTML.restoreEndCallback so other got@11 instances (e.g. the microlink API's shared gotOpts) can use it. Documented in the README.

Tests

test/got-hooks.js:

  • Unit tests for wrapEnd (finish, flush error, already-finished, no callback) and restoreEndCallback (protocol, user request, http2). These catch a regression on any Node version.
  • Integration: got and html-get fetch retry ECONNREFUSED once (checked via beforeRetry), reject or resolve with the original error, and nothing is uncaught. Before the hook was wired in, the fetch test crashed the file on 24.20 with the CI error.
  • User beforeRequest hooks still run.

Results:

  • Full suite on Node 24.16.0, 24.19.0 and 24.20.0: 151 passed, 54 skipped, no uncaught errors. standard is clean.
  • The README example on 24.20 and 24.19: rejected ENOTFOUND, retryCount 1.

Not covered here: an end-to-end run through the API's got-scraping proxy agents. They are plain http/https agents passed to http.request unchanged; 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#end callbacks 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 a restoreEndCallback got beforeRequest hook that wraps http/https requests so end callbacks behave like pre-24.20 (drop flush errors like ERR_SOCKET_CLOSED_BEFORE_CONNECTION, still pass ERR_STREAM_ALREADY_FINISHED).

The hook is always appended to gotOpts.hooks.beforeRequest in the internal fetch path and is exported as getHTML.restoreEndCallback for other got v11 clients. User beforeRequest hooks, custom options.request, and http2 are left unchanged.

README documents the behavior; test/got-hooks.js adds 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

    • Improved request handling for compatibility with Node.js 24.20.
    • Preserved retry behavior and ensured original network errors are reported correctly.
    • Prevented uncaught exceptions during HTML retrieval.
    • Maintained user-provided request hooks and custom request functions.
  • Documentation

    • Documented the request callback compatibility behavior and restoration hook.
    • Clarified how the restoration hook interacts with existing request hooks.

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
@coderabbitai

coderabbitai Bot commented Sep 14, 2026 •

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 581d729c-423d-4101-ade5-c4d82ed64891

📥 Commits

Reviewing files that changed from the base of the PR and between 4be23b7 and f0392a5.

📒 Files selected for processing (4)
  • README.md
  • src/got-hooks.js
  • src/index.js
  • test/got-hooks.js

Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The change adds request callback wrappers for Node.js HTTP and HTTPS requests. It appends the wrapper as a got beforeRequest hook, exports it, documents its use, and adds tests for callback handling, retries, request options, and existing hooks.

Changes

ClientRequest callback restoration

Layer / File(s) Summary
Request end callback wrapper
src/got-hooks.js
Adds wrapEnd and restoreEndCallback. The wrapper suppresses flush-preventing errors and preserves ERR_STREAM_ALREADY_FINISHED.
got hook integration
src/index.js, README.md
Appends restoreEndCallback to got beforeRequest hooks, exports it, and documents the hook and gotOpts behavior.
Wrapper and retry validation
test/got-hooks.js
Tests callback handling, HTTP and HTTPS wrapping, request option exceptions, retry behavior, and preservation of user hooks.

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
Loading

Merge Risk: ⚪ Minimal · up to f0392

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)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: preserving got retry behavior on Node.js 24.20.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 3…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch Kikobeats/got-end-callback-f8fa318b

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

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

Comment thread src/got-hooks.js
if (options.request || options.http2) return
const { request } = options.url.protocol === 'https:' ? https : http
options.request = (...args) => wrapEnd(request(...args))
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit f0392a5. Configure here.

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.

1 participant