fix(response): route synchronous prepareResponse throws through the error pipeline by g1mn · Pull Request #1503 · h3js/h3

g1mn · 2026-07-22T16:46:49Z

Part of #1477 (1 of 2 — split per @pi0's request; companion Uint8Array fix in #1504).

Wraps the prepareResponse call in toResponse so a synchronous throw during response preparation — e.g. JSON.stringify on a circular return value, or circular error.data inside errorResponse — is routed through the normal error pipeline (toError → errorResponse) instead of escaping as a raw exception/rejection. onError / onResponse and logging now run, and a 500 is returned. A non-Error throw is handled the same way.

app.get("/circular", () => { const a: any = {}; a.self = a; return a; });
// before: app.fetch() rejects with a raw TypeError, hooks never run
// after:  toError() → errorResponse → 500, onError/onResponse fire

Tests (inside describeMatrix, via t.app / t.fetch per the coding guidelines): a circular return value and a thrown HTTPError with circular data both render as 500 without escaping, and the hook pipeline still fires. pnpm test (lint + format + typecheck + full vitest) is green.

Drafted with AI assistance; reviewed and verified against the test suite by me.

Summary by CodeRabbit

Bug Fixes
- Synchronous failures during response preparation (including circular/invalid handler return values) are now routed through the standard error pipeline, producing a consistent HTTP 500 response.
- Synchronous failures inside the error hook are handled safely, avoiding escaped exceptions or infinite recursion and returning the usual normalized error output.
- Request/error/response hooks continue to run once each even when response preparation fails.
Tests
- Added regression coverage for circular handler returns and circular error data.
- Added a regression case for synchronously throwing error hooks.
- Updated bundle size benchmark limits to allow slightly larger outputs.

coderabbitai · 2026-07-22T16:47:10Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 65e7c2e1-ce8c-4a86-b07b-da97c47c65d2

📥 Commits

Reviewing files that changed from the base of the PR and between 5089638 and a2a9bcd.

📒 Files selected for processing (1)

src/response.ts

💤 Files with no reviewable changes (1)

src/response.ts

📝 Walkthrough

Walkthrough

toResponse() now normalizes synchronous response-preparation failures, and synchronous onError throws enter the promise catch path. Tests cover circular values, circular error data, and hook execution. Bundle thresholds are updated.

Changes

Response preparation correctness

Layer / File(s)	Summary
Synchronous preparation error pipeline `src/response.ts`, `test/errors.test.ts`, `test/hooks.test.ts`	Synchronous preparation and `onError` failures are converted through `toError` into HTTP 500 responses, with coverage for circular values, circular error data, and exactly-once hook execution.
Bundle size benchmark thresholds `test/bench/bundle.test.ts`	Maximum byte and gzip-size thresholds are updated for the H3, H3Core, and defineHandler bundle benchmarks.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related issues

h3js/h3 issue 1477 — Covers synchronous prepareResponse failures addressed by this change.

Possibly related PRs

h3js/h3#1372 — Both update toResponse() handling of response-preparation failures.
h3js/h3#1485 — Both normalize response-preparation failures through toError.
h3js/h3#1352 — Both modify the error-response pipeline in src/response.ts.

Suggested reviewers: pi0

Poem

I caught the throw before it flew,
And sent it through the error queue.
The hooks danced once, the tests agreed,
While bundles gained the space they need.
Hop, hop—500s now land with care!

🚥 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 accurately summarizes the main change: synchronous prepareResponse errors are routed through the error pipeline.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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 unit tests (beta)

Create PR with unit tests

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

pi0 · 2026-07-22T16:48:44Z

Thanks. If you like to contribute, can you please split per issue to two prs?

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/errors.test.ts`:
- Around line 298-323: Update the “response preparation throws” tests to run
inside describeMatrix, creating a fresh H3 instance in beforeEach and issuing
requests through ctx.fetch. Preserve both circular return-value and circular
HTTPError assertions while ensuring they execute across the matrix modes.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b6dc03c6-7e21-4932-b8ae-ab2a7701e387

📥 Commits

Reviewing files that changed from the base of the PR and between 4a32c1b and 9aa7d52.

📒 Files selected for processing (5)

src/response.ts
test/bench/bundle.test.ts
test/errors.test.ts
test/hooks.test.ts
test/middleware.test.ts

…peline A synchronous throw while preparing the response (e.g. JSON.stringify on a circular return value, or on circular error.data in errorResponse) escaped toResponse()/~request() as a raw exception/rejection instead of going through onError/onResponse/logging. prepareResponse() is now called inside a try/catch; a synchronous throw is routed through toError() and re-entered into toResponse(), the same path already used for rejected-promise handlers. Split out of h3js#1503 per maintainer feedback (one PR per issue). Addresses the first bug in h3js#1477; the Uint8Array content-length bug is handled separately. Co-Authored-By: Claude Sonnet 5 <[email protected]>

g1mn · 2026-07-22T17:10:01Z

Thanks for the quick review! Split as requested:

fix(response): route synchronous prepareResponse throws through the error pipeline #1503 (this PR) — now scoped to just routing synchronous prepareResponse throws through the error pipeline.
fix(response): keep content-length header for Uint8Array responses #1504 — the Uint8Array content-length fix.

Also moved the new tests into describeMatrix with t.fetch per the coding guidelines (thanks @coderabbitai). 🙏

coderabbitai · 2026-07-22T17:10:17Z

Tip

For best results, initiate chat on the files or code changes.

@g1mn Thanks for splitting these cleanly. Keeping #1503 focused on synchronous response-preparation errors and moving the Uint8Array header fix to #1504 makes both changes easier to review and validate. The describeMatrix/t.fetch test update looks aligned with the project conventions. 🙏

ᕕ( ᐛ )ᕗ🐇

✏️ Learnings added

Learnt from: g1mn
URL: https://github.com/h3js/h3/pull/1503

Timestamp: 2026-07-22T17:10:16.682Z
Learning: In the h3 test suite, regression tests should use `describeMatrix` with the matrix-provided `t.fetch` helper, following the repository's coding guidelines.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/response.ts`:
- Around line 25-31: Update the synchronous error path in toResponse and the
onError invocation within prepareResponse so a throwing config.onError is
deferred through Promise.resolve().then(...). Ensure its rejection reaches the
existing normalization/error-response handling without recursively calling
toResponse or invoking onError again, while preserving the normalized 500
response behavior.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d371166f-b36d-4f26-9dff-b91fa9b7b169

📥 Commits

Reviewing files that changed from the base of the PR and between 9aa7d52 and 57b8054.

📒 Files selected for processing (4)

src/response.ts
test/bench/bundle.test.ts
test/errors.test.ts
test/hooks.test.ts

🚧 Files skipped from review as they are similar to previous changes (3)

test/errors.test.ts
test/hooks.test.ts
test/bench/bundle.test.ts

…nfinitely toResponse()'s try/catch (added for h3js#1477) turns a synchronous throw during response preparation into a call to toResponse(toError(error), ...), which re-enters prepareResponse() with `nested` reset to false. If `onError` itself throws synchronously, `Promise.resolve(onError(error, event))` evaluates that call before wrapping it, so the throw escapes prepareResponse() synchronously instead of becoming a promise rejection — triggering the same onError call again, forever (stack overflow instead of a normalized 500). Move the onError call inside a `.then()`, matching the pattern already used for the onResponse hook below: a synchronous throw there now becomes a rejection, caught by the existing `.catch((error) => error)`, which continues with `nested: true` and terminates the retry. Regression test in errors.test.ts cross-checked against pre-fix code (times out / stack-overflows there). CodeRabbit finding on h3js#1503. Co-Authored-By: Claude Sonnet 5 <[email protected]>

g1mn · 2026-07-22T17:26:56Z

Addressed the review:

🔴 Sync-throwing onError recursion — fixed in 5089638. Wrapped the hook as Promise.resolve().then(() => onError(error, event)) (matching the onResponse pattern), so a synchronous throw becomes a rejection handled by the existing .catch and the retry terminates after one attempt instead of re-entering toResponse. Added a regression test (a synchronously-throwing onError does not recurse infinitely) that stack-overflows / times out on the pre-fix code and returns a normalized 500 after.
describeMatrix note on test/errors.test.ts — these are already inside the describeMatrix("errors", …) block using t.app / t.fetch; that comment looks like it carried over from the pre-split revision.

Thanks @coderabbitai! 🙏

coderabbitai · 2026-07-22T17:27:10Z

Tip

For best results, initiate chat on the files or code changes.

@g1mn Thanks for the detailed follow-up. The Promise.resolve().then(...) wrapping correctly prevents a synchronous onError throw from re-entering toResponse, and the regression test covers the failure mode well.

You’re also right that the describeMatrix note was stale: those tests are already within the matrix and use t.app / t.fetch. Thanks for catching that.

ᕕ( ᐛ )ᕗ🐇

codecov · 2026-07-22T20:43:31Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

g1mn requested a review from pi0 as a code owner July 22, 2026 16:46

coderabbitai Bot reviewed Jul 22, 2026

View reviewed changes

Comment thread test/errors.test.ts

g1mn mentioned this pull request Jul 22, 2026

fix(response): keep content-length header for Uint8Array responses #1504

Merged

g1mn changed the title ~~fix(response): route sync prepare errors through hooks; keep content-length for Uint8Array~~ fix(response): route synchronous prepareResponse throws through the error pipeline Jul 22, 2026

g1mn force-pushed the fix/response-prepare-error-handling branch from 9aa7d52 to 57b8054 Compare July 22, 2026 17:09

coderabbitai Bot reviewed Jul 22, 2026

View reviewed changes

Comment thread src/response.ts

pi0 reviewed Jul 22, 2026

View reviewed changes

Comment thread src/response.ts Outdated

Apply suggestion from @pi0

a2a9bcd

pi0 approved these changes Jul 22, 2026

View reviewed changes

pi0 merged commit 283164c into h3js:main Jul 22, 2026
5 checks passed

g1mn mentioned this pull request Jul 22, 2026

fix(response): route synchronous prepareResponse throws through the error pipeline #1505

Closed

g1mn deleted the fix/response-prepare-error-handling branch July 22, 2026 22:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(response): route synchronous prepareResponse throws through the error pipeline#1503

fix(response): route synchronous prepareResponse throws through the error pipeline#1503
pi0 merged 3 commits into
h3js:mainfrom
g1mn:fix/response-prepare-error-handling

g1mn commented Jul 22, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jul 22, 2026 •

edited

Loading

Walkthrough

Changes

Uh oh!

pi0 commented Jul 22, 2026

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

g1mn commented Jul 22, 2026

Uh oh!

coderabbitai Bot commented Jul 22, 2026

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

g1mn commented Jul 22, 2026

Uh oh!

coderabbitai Bot commented Jul 22, 2026

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Jul 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

g1mn commented Jul 22, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

pi0 commented Jul 22, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

g1mn commented Jul 22, 2026

Uh oh!

coderabbitai Bot commented Jul 22, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

g1mn commented Jul 22, 2026

Uh oh!

coderabbitai Bot commented Jul 22, 2026

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Jul 22, 2026

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

g1mn commented Jul 22, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 22, 2026 •

edited

Loading