fix: catch synchronous errors in request callbacks #4443

mcollina · 2025-08-21T19:03:34Z

Summary

Fix synchronous errors thrown in request callbacks being swallowed when underlying stream is destroyed
Add proper error handling in RequestHandler.onHeaders() to catch, cleanup, and re-throw errors
Ensure errors are propagated through existing error handling chain via Request.onHeaders()

Rationale

Previously, when a synchronous error was thrown in a request callback after the underlying stream was destroyed, the error would be swallowed and become an uncaught exception. This happened because the RequestHandler.onHeaders() method called user callbacks via runInAsyncScope() without wrapping it in a try-catch block.

Changes

Bug Fixes

Added try-catch block around callback invocation in RequestHandler.onHeaders()
Properly destroy response stream when callback throws synchronously
Re-throw error to be handled by existing error handling in Request.onHeaders()
Added comprehensive test coverage for synchronous error scenarios

Implementation Details

The fix ensures that:

Synchronous errors in callbacks are caught immediately
Response streams are properly cleaned up before propagating errors
Errors flow through the existing error handling chain (Request.onHeaders → this.abort(err))
No uncaught exceptions occur due to swallowed callback errors

Test Plan

Added tests for synchronous errors in successful request callbacks
Added tests for immediate errors thrown in callbacks
Added tests for error handling in failure scenarios
Verified existing tests continue to pass
Confirmed no regressions in error handling behavior

🤖 Generated with Claude Code

When a synchronous error is thrown in a request callback after the underlying stream is destroyed, the error was being swallowed and becoming an uncaught exception. This fix adds proper error handling in RequestHandler.onHeaders() to catch these errors, destroy the response stream cleanly, and re-throw the error to be handled by the existing error handling chain. Fixes the case where user code throws synchronously in request callbacks when streams are already destroyed, ensuring errors are properly propagated through the abort mechanism. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Matteo Collina <hello@matteocollina.com>

Uzlopak · 2025-08-21T19:35:42Z

@mcollina

Do we need to check all occurences of runInAsyncScope in regard of being wrapped in try catch blocks?

mcollina · 2025-08-21T20:01:37Z

That might be the case.

lib/api/api-request.js

metcoder95

Shall we backport it to v6?

Co-authored-by: Aras Abbasi <aras.abbasi@googlemail.com>

mcollina · 2025-08-22T08:53:14Z

I think this has a bug still, I need to look at it again.

lib/api/api-request.js

Uzlopak · 2025-08-22T16:03:48Z

Fixed the linting

…andler When a synchronous error is thrown in a request callback, it should propagate to the global uncaughtException handler rather than being caught and causing the process to abort. This change uses process.nextTick() to re-throw the error, allowing it to reach the uncaughtException handler as expected. - Modified error handling in RequestHandler.onHeaders() to use nextTick - Updated tests to verify errors reach uncaughtException handler - Fixed flaky stats test expectation 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Matteo Collina <hello@matteocollina.com>

lib/api/api-request.js

Co-authored-by: Aras Abbasi <aras.abbasi@googlemail.com>

Uzlopak · 2025-08-25T10:00:59Z

@mcollina
Oh you mean, it was totally "swallowed" no even throwing an unhandledPromiseRejection?

mcollina · 2025-08-25T10:53:26Z

Yes

Uzlopak

LGTM

Combine res.on('error', noop) call directly into util.destroy for cleaner code structure.

Uzlopak

LGTM

Ethan-Arrowood

tests look good. nice fix

mcollina requested review from Uzlopak and metcoder95 August 21, 2025 19:06

Uzlopak reviewed Aug 21, 2025

View reviewed changes