fix: report stale ISR background revalidation errors via onRequestError

[Divkix]

Fixes #775

Summary

• ISR background revalidation errors are now reported via onRequestError instrumentation hook (Sentry, OpenTelemetry, etc.) instead of being silently swallowed
• Extended triggerBackgroundRegeneration() in isr-cache.ts with optional error context parameter
• Updated App Router and Pages Router entry templates to pass route context to background regen callbacks
• Error reports include revalidateReason: "stale", matching Next.js 16 behavior (Ensure app-page reports stale ISR revalidation errors via onRequestError vercel/next.js#92282)

Changes

• server/isr-cache.ts: Added ISRRegenErrorContext type and error reporting in background regen catch
• entries/app-rsc-entry.ts: Updated inline __triggerBackgroundRegeneration to call _reportRequestError
• entries/pages-server-entry.ts: Forward error context through wrapper function
• server/dev-server.ts: Pass Pages Router route context to regen calls
• server/pages-page-data.ts: Pass route context for Pages Router page data regen
• Updated test snapshots and assertions

Test plan

• All existing ISR cache tests pass (42 tests)
• Entry template tests pass with updated snapshots
• App Router integration tests pass (383 tests)
• Lint, format, and typecheck pass

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/vinext@947

commit: bebbe22

[james-elicx]

/bigbonk review

[ask-bonk]

Review Summary

Good feature — surfacing ISR background revalidation errors to onRequestError instrumentation hooks is the right thing to do, and the approach is sound. The implementation correctly covers all four paths (App Router route handlers, App Router pages, Pages Router dev, Pages Router prod via pages-page-data.ts). Test assertions are updated appropriately.

A few items to address:

Issues

1. Missing void on _reportRequestError in the App Router generated entry — see inline comment.

2. JSDoc removed from triggerBackgroundRegeneration — see inline comment.

3. No unit test for the new error-reporting behavior. The existing isr-cache.test.ts test "handles render function errors gracefully" (line 222) verifies that errors are caught and console.error is called, but there's no test verifying that reportRequestError is called when errorContext is provided. This is the core new behavior and should have dedicated test cases:

   • When errorContext is provided and renderFn throws, reportRequestError is called with the correct args (revalidateReason: "stale", correct routerKind, etc.)
   • When errorContext is omitted and renderFn throws, reportRequestError is NOT called
   • When the error is not an Error instance (e.g., a string), it gets wrapped in new Error(String(err))

   These can mock by setting globalThis.__VINEXT_onRequestErrorHandler__ directly in the test.

4. PR body mentions ISRRegenErrorContext type but no such named type exists in the code. The error context is defined inline. Minor doc inconsistency.

Nits

• The dev-server.ts diff is +115/-103 but the actual logic change is just adding the third argument to triggerBackgroundRegeneration. The reformatting noise makes the diff harder to review.

[ask-bonk]

The _reportRequestError promise is not void-ed here, unlike the equivalent call in isr-cache.ts (line 94) which uses void reportRequestError(...). For consistency and to signal fire-and-forget intent:

Suggested change

	_reportRequestError(
	void _reportRequestError(

[ask-bonk]

The JSDoc that was here before explained the dedup semantics, waitUntil registration, and the no-op-on-duplicate-key behavior. That's useful context for callers. Please restore and update it to document the new errorContext parameter. Something like:

/**
 * Trigger a background regeneration for a cache key.
 *
 * If a regeneration for this key is already in progress, this is a no-op.
 * The renderFn should produce the new cache value and call isrSet internally.
 *
 * On Cloudflare Workers the regeneration promise is registered with
 * `ctx.waitUntil()` via the ALS-backed ExecutionContext, keeping the isolate
 * alive until the regeneration completes even after the Response is returned.
 *
 * When `errorContext` is provided and the render function fails, the error
 * is reported via `reportRequestError` (instrumentation hook) with
 * `revalidateReason: "stale"`.
 */

[ask-bonk]

Review posted successfully. Here's a summary of the review:

Review for PR #947 — Report stale ISR background revalidation errors via onRequestError

The approach is sound — all four ISR background regen paths (App Router routes, App Router pages, Pages Router dev, Pages Router prod) now forward errors to the onRequestError instrumentation hook. Three items to address:

1. Missing void on _reportRequestError in the App Router generated entry (app-rsc-entry.ts:543). The shared isr-cache.ts correctly uses void reportRequestError(...) but the inline copy doesn't. Posted a suggestion comment with the fix.

2. JSDoc removed from triggerBackgroundRegeneration in isr-cache.ts. The old function had a useful doc block explaining dedup, waitUntil, and no-op semantics. It should be restored and updated to document the new errorContext parameter.

3. No unit test for the core new behavior. The existing error-handling test in isr-cache.test.ts only checks console.error — it doesn't verify that reportRequestError is actually called with the correct args when errorContext is provided. Test cases for the happy path, the no-context path, and non-Error wrapping are needed.

github run

[james-elicx]

/bigbonk review

[ask-bonk]

Review Summary

This is a clean, well-structured change that addresses a real gap — ISR background revalidation errors were silently swallowed (only console.error), now they're reported through the onRequestError instrumentation hook so Sentry/OTel/etc. can pick them up.

What's good

• All four ISR regen paths are covered: App Router route handlers, App Router pages, Pages Router dev (dev-server.ts), Pages Router prod (pages-page-data.ts → pages-server-entry.ts). No path left behind.
• The shared isr-cache.ts implementation is the source of truth for the typed runtime. The generated App Router entry duplicates the logic inline, but that's justified by the RSC environment import constraints (documented at app-rsc-entry.ts:510-513).
• Tests cover the three key cases: error reported with context, no report without context, non-Error values wrapped. These test the actual reportRequestError pathway via globalThis.__VINEXT_onRequestErrorHandler__.
• request.path uses the cache key rather than the original URL — this is the right call since the original request context is gone by the time background regen runs. The cache key contains the pathname, which is the most useful identifier.
• Previous review feedback was already addressed: void on _reportRequestError is present, JSDoc was preserved and updated with the new errorContext parameter docs.

Minor notes

• The dev-server.ts diff is +115/-103 but the logic change is just adding the third argument to triggerBackgroundRegeneration. The rest is reformatting from the function call restructuring. I verified the logic is identical.
• The pages-page-data.ts type narrowing (routerKind: "Pages Router") is appropriately specific for that module.

Looks ready to merge.

[ask-bonk]

Nit: The { path: key, method: "GET", headers: {} } synthetic request object is the right call here (the original request is long gone by the time background regen runs). Worth knowing that path will be something like app:buildid:/blog/hello:html rather than /blog/hello — this is fine for instrumentation since it uniquely identifies what failed, but if users report confusion with these cache-key-style paths in Sentry, a future improvement could extract just the pathname portion.

[ask-bonk]

Review posted successfully. Here's a summary:

Approved PR #947 — Report stale ISR background revalidation errors via onRequestError.

The change is clean and complete. All four ISR background regeneration paths (App Router routes, App Router pages, Pages Router dev, Pages Router prod) now forward errors to the onRequestError instrumentation hook. The previous review's concerns have all been addressed. Tests cover the three key cases (with context, without context, non-Error wrapping). The only note I left was a minor observation that request.path in error reports will contain cache-key-format strings rather than raw pathnames — not a blocker, just something to be aware of if users report confusion in their Sentry dashboards.

github run