fix(next): trace compiled response lifecycles - #9708
Conversation
Overall package sizeSelf size: 7.88 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 3.3.3 | 125.43 kB | 441.68 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | dc-polyfill | 0.1.11 | 25.74 kB | 25.74 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
|
BenchmarksBenchmark execution time: 2026-08-06 00:09:53 Comparing candidate commit 7a3d98f in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 2312 metrics, 46 unstable metrics.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc57c1585f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9708 +/- ##
==========================================
+ Coverage 97.93% 98.47% +0.54%
==========================================
Files 964 964
Lines 136442 136610 +168
Branches 11952 11774 -178
==========================================
+ Hits 133619 134531 +912
+ Misses 2823 2079 -744 Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Compiled App Route and App Page request spans started inside response generators, so cache hits had no span, stale background revalidations created request spans, and App Route hooks received a synthetic response. Starting at the shared RouteModule response boundary covers the foreground request once and passes the Node response through unchanged.
## Summary - Finish compiled App Page spans after cache handling and the Node response lifecycle. - Clear App Route request associations when response generators throw synchronously. ## Why Next can rewrite cached App Page statuses and report errors after handleResponse() resolves. A synchronous generator throw could also retain a stale request association and attach errors from fallback handling to the wrong span. ## Test plan - Run the compiled runtime plugin tests. - Run Next 15.4.1 App Page rendering and error tests. - Run changed-line coverage and the full lint suite.
e0e7655 to
7a3d98f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a3d98f30e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| /** @param {NextErrorContext} ctx */ | ||
| error ({ span, error, req }) { | ||
| if (!span && req) { | ||
| span = this.#deferredRequestContexts.get(req)?.currentStore?.span |
There was a problem hiding this comment.
When an ISR App Page serves a stale cache entry, Next resolves the cached response first and keeps the responseGenerator running in the background; if that background render reports onRequestError before the Node response emits finish/close, the error message uses the same req, so this deferred lookup retrieves the foreground cached response context and marks that user-facing span as failed. That makes successful stale-cache hits look like errored requests even though only revalidation failed, so the deferred lookup needs to distinguish foreground errors from background revalidation errors.
Useful? React with 👍 / 👎.
| ctx.responseFinished = true | ||
| if (!ctx.handlerFinished) return | ||
|
|
||
| this.#finishDeferred(ctx) |
There was a problem hiding this comment.
For compiled App Page requests where handleResponse resolves before the Node response emits finish/close, any exception from the configured hooks.request now escapes this response listener because #finishDeferred calls #finish directly instead of through the diagnostic-channel subscriber wrapper that normally catches plugin errors. A throwing hook is user code and should only disable/log the plugin; here it can throw out of ServerResponse.emit('finish') and crash the app.
Useful? React with 👍 / 👎.
| it('clears the App Route request association after a synchronous generator error', async () => { | ||
| const nextRequest = { | ||
| error: new Error('unrelated Next request error'), | ||
| headers: {}, | ||
| method: 'GET', | ||
| url: '/api/generator-error', | ||
| } | ||
|
|
||
| class AppRouteRouteModule extends RouteModule { | ||
| definition = { pathname: '/api/generator-error' } | ||
|
|
||
| handle () { | ||
| return Promise.resolve({ status: 200 }) | ||
| } | ||
|
|
||
| /** @param {{responseGenerator: () => Promise<unknown>}} options */ | ||
| async handleResponse ({ responseGenerator }) { | ||
| assert.throws(responseGenerator, /synchronous generator error/) | ||
| const response = await this.handle(nextRequest, {}) | ||
| return { value: { status: response.status } } | ||
| } | ||
| } | ||
| getCompiledRuntimeHook('app-route')({ AppRouteRouteModule }) |
There was a problem hiding this comment.
Not a required change, but it wouldnt hurt to have an actual compiled next test app, and verifying the caching behavior that way instead of via manually constructed fixtures
wconti27
left a comment
wconti27
left a comment
There was a problem hiding this comment.
Both codex comments also look legitimate as well
2 participants
Summary
Compiled App Route and App Page request spans started inside response generators, so cache hits had no span, stale background revalidations created request spans, and App Route hooks received a synthetic response. Starting at the shared RouteModule response boundary covers the foreground request once and passes the Node response through unchanged.
Refs: #9627
Refs: #9682