Fix silent error absorption in RSC payload injection#2486
Fix silent error absorption in RSC payload injection#2486AbanoubGhadban wants to merge 2 commits into2402-streaming-hang-on-errorfrom
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
size-limit report 📦
|
PR Review: Fix silent error absorption in RSC payload injectionThis PR tackles several important issues: silent error swallowing in the RSC streaming pipeline, concurrent upload race conditions, and generator fixes. The implementation is generally solid. Here are my findings: Positive highlights
Issues requiring attention1. Double error emission in
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| * termination is handled by the 'close' event below. | ||
| * Report errors on htmlStream by emitting them on resultStream, where they | ||
| * propagate to handleStreamError → errorReporter in the node renderer. | ||
| * Error alone is not the end of the stream — termination is handled by the |
There was a problem hiding this comment.
Double error emission risk
When htmlStream emits an error, finished(htmlStream) (called inside startRSC) will also reject with that same error. This means resultStream.emit('error', ...) gets called twice for a single htmlStream error — once here and once in the startRSC catch block — resulting in duplicate Sentry/errorReporter reports.
Consider removing this handler and relying solely on the startRSC catch block, or deduplicate using a seen-error guard:
let lastReportedErr: Error | undefined;
htmlStream.on('error', (err) => {
const error = err instanceof Error ? err : new Error(String(err));
if (error !== lastReportedErr) {
lastReportedErr = error;
resultStream.emit('error', error);
}
});There was a problem hiding this comment.
Good catch — this was a real double-emission bug. Fixed in eb254a4.
The htmlStream.on('error') handler now only emits on resultStream when startRSC hasn't been called yet (!rscPromise). Once startRSC is running, finished(htmlStream) in the startRSC catch block handles reporting — the handler still exists to prevent Node.js from throwing on unhandled 'error' event but no longer double-reports.
This correctly covers both edge cases:
- Error before first data chunk (startRSC not running): htmlStream.on('error') reports it
- Error after data starts flowing (startRSC running): finished() rejection → catch block reports it
| @@ -55,5 +56,5 @@ export default function transformRSCStream(stream: NodeJS.ReadableStream): NodeJ | |||
| }, | |||
| }); | |||
|
|
|||
There was a problem hiding this comment.
This correctly adds the 'close'-event fix (the missed pipe site from PR #2407), but without an onError callback, errors emitted by the RSC node stream will still be silently dropped — they won't reach errorReporter.
If error visibility is desired here (consistent with the goal of this PR), consider:
return safePipe(stream as Readable, htmlExtractor, (err) => {
// forward to caller or log — RSC stream errors should be observable
htmlExtractor.emit('error', err);
});Or at minimum document why silent error suppression is intentional for this specific pipe.
There was a problem hiding this comment.
The safePipe change here was specifically to fix the close event gap (the pipe() destination-not-ended-on-destroy issue that PR #2407 missed at this site). Error reporting for the RSC source stream is a pre-existing concern that this PR doesn't make worse.
Regarding the suggested fix — emitting on the destination (htmlExtractor) would actually break the pipe. In Node.js v20+, pipe() attaches an error handler on the destination that calls source.unpipe(destination), so destination.emit('error', err) would disconnect the pipe and stop data flow.
The proper way to add error reporting here would require changes to the caller (getReactServerComponent.server.ts) to provide an error callback, which is outside this PR's scope. The Transform's own try/catch in its transform callback already handles parse errors (line 53-54). If the source stream is destroyed, safePipe ensures htmlExtractor is properly ended via the close handler — React's createFromNodeStream sees the stream end and handles the incomplete payload via its error boundary.
Happy to open a follow-up issue for RSC source stream error reporting if desired.
|
Thanks for the thorough review! Addressed the findings: Issue 1 (Double error emission) — Fixed in eb254a4. The Issue 2 (transformRSCNodeStream without onError) — Intentional. The Issues 3, 4, 5 — These are about code in the base PR #2407 (rscWebpackConfig loader appending, orphan cleanup mtime vs ctime, barrier safety valve), not changes introduced by this PR. The diff reviewed appears to include the full range from the base branch's base rather than just this PR's changes. |
|
PR Review — Fix silent error absorption in RSC payload injection (#2486) This PR bundles several well-scoped fixes into RC 4. The error-reporting improvements in injectRSCPayload.ts and the streaming pipeline clean-up are the headline changes; the concurrent-upload race-condition fixes and generator wiring fix are also solid. Overall the approach is sound. A few things worth addressing before merge:
path.basename("..") returns ".." on all platforms, so a filename of literally ".." passes the !safeFilename guard and produces: path.join(uploadDir, "..") // → parent of the per-request upload dir The file is then registered under savedFilePath pointing at the parent directory of the upload dir, and later copyUploadedAssets copies from that path to the bundle directory. On most filesystems writing to a directory path as a file will fail, but the path still escapes the intended sandbox. Even though this is an internal service, the fix is cheap: if (!safeFilename || safeFilename === "." || safeFilename === "..") {
The orphan-cleanup interval has no clearInterval and keeps a reference that prevents the event loop from exiting naturally. In the cluster master process that is usually fine at runtime, but it can cause test harnesses to hang and makes graceful shutdown harder to reason about. Recommend storing the handle and calling .unref() so it never blocks exit: const cleanupTimer = setInterval(() => { ... }, ORPHAN_CLEANUP_INTERVAL_MS);
The guard "if (!rscPromise)" in htmlStream.on("error", ...) is correct for the steady-state case, but there is a narrow window where both paths can emit:
This can produce two "error" events on resultStream, leading to double Sentry reports. A simple boolean flag (e.g. errorForwarded) alongside rscPromise would close the window completely.
const firstFailure = settled.find((r) => r.status === "rejected"); If two bundles reject, the second rejection reason is swallowed. Consider logging all rejected reasons before rethrowing the first, or aggregating them into an AggregateError, to make multi-bundle failure debugging easier. MINOR / NITS
Overall this is high-quality work. The safePipe abstraction is cleaner than the scattered inline close-listeners it replaces, and the per-request UUID upload directory is the right fix for the race condition. Addressing items 1 and 2 above is recommended before tagging the release. |
| // preventing path traversal attacks (e.g. filename "../../etc/shadow"). | ||
| const safeFilename = path.basename(part.filename); | ||
| if (!safeFilename) { | ||
| throw new Error( |
There was a problem hiding this comment.
The path-traversal guard has a gap: path.basename('..') returns '..' on all platforms, so a filename that is literally '..' passes this check. The resulting destinationPath would be the parent of this.uploadDir, and part.value.savedFilePath would point there too — meaning copyUploadedAssets later reads from the parent directory rather than an isolated file.
Add an explicit rejection for the dot-segment cases:
| // preventing path traversal attacks (e.g. filename "../../etc/shadow"). | |
| const safeFilename = path.basename(part.filename); | |
| if (!safeFilename) { | |
| throw new Error( | |
| const safeFilename = path.basename(part.filename); | |
| if (!safeFilename || safeFilename === '.' || safeFilename === '..') { | |
| throw new Error( | |
| `onFile: received file with empty or invalid filename: ${JSON.stringify(part.filename)}`, | |
| ); | |
| } |
There was a problem hiding this comment.
This is about code in the base PR #2407 (worker.ts upload handling), not changes introduced by this PR. The diff reviewed includes the full range from the base branch's base rather than just this PR's single commit.
|
|
||
| // Periodically clean up orphaned per-request upload directories that workers | ||
| // failed to remove (e.g. after a crash). Each worker creates uploads/<UUID>/ | ||
| // directories that are normally cleaned up in the onResponse hook; this timer |
There was a problem hiding this comment.
This interval is never stopped and holds a strong reference to the event loop. In the cluster master process that is generally fine at runtime, but it prevents the process from exiting cleanly in test environments and complicates graceful-shutdown handling.
Call .unref() so the interval does not keep the process alive:
| // directories that are normally cleaned up in the onResponse hook; this timer | |
| const cleanupTimer = setInterval(() => { | |
| void (async () => { |
and after the setInterval(...) call:
cleanupTimer.unref();There was a problem hiding this comment.
Same as above — this is about code in the base PR #2407 (master.ts orphan cleanup), not changes introduced by this PR.
| * propagate to handleStreamError → errorReporter in the node renderer. | ||
| * Error alone is not the end of the stream — termination is handled by the | ||
| * 'close' event below. | ||
| * | ||
| * We only emit here when startRSC hasn't been called yet. Once startRSC is |
There was a problem hiding this comment.
The !rscPromise guard correctly prevents double-emission in the common case, but there is a subtle window: if htmlStream emits error in the same microtask flush as the finished(htmlStream) rejection (both triggered by the same underlying stream error), the error event fires synchronously while rscPromise is already set, so this handler is skipped — good. However if the ordering is reversed (error event fires before finished() settles), the event handler emits here and then the startRSC catch block also emits. This is unlikely in practice but possible.
A boolean flag shared between the two code paths would be more robust:
let errorForwarded = false;
htmlStream.on('error', (err) => {
if (!rscPromise && !errorForwarded) {
errorForwarded = true;
resultStream.emit('error', err instanceof Error ? err : new Error(String(err)));
}
});And in the startRSC catch block:
} catch (e) {
if (!errorForwarded) {
errorForwarded = true;
resultStream.emit('error', e instanceof Error ? e : new Error(String(e)));
}
endResultStream();
}There was a problem hiding this comment.
The "subtle window" described here cannot occur due to Node.js's synchronous event handling guarantees:
- `rscPromise = startRSC()` is set synchronously when the first data chunk arrives (line 286)
- Inside `startRSC`, `finished(htmlStream)` registers its internal event listeners synchronously before the first `await` (line 261). There is no async gap between `rscPromise` being set and `finished()` listening.
- When `htmlStream` emits `'error'`, all registered listeners fire synchronously in registration order. Both our handler and `finished()`'s internal handler execute in the same synchronous call stack.
So the invariant holds: if `rscPromise` is set (our handler skips), `finished()` is guaranteed to be listening and will handle the error in the catch block. If `rscPromise` is not set, `startRSC` hasn't been called, so `finished()` isn't listening, and only our handler reports.
There is no ordering reversal possible — both the check (`!rscPromise`) and `finished()`'s listener registration happen synchronously before any event loop tick.
AbanoubGhadban
force-pushed
the
2402-streaming-hang-on-error
branch
from
436e7e3 to
59f02c0
Compare
Errors in injectRSCPayload (htmlStream errors, startRSC catch, rscPromise catch) were silently swallowed, preventing them from reaching errorReporter and Sentry. This change emits those errors on resultStream so they propagate through handleStreamError to the error reporting infrastructure without breaking the pipe or causing Fastify to return error responses. Also introduces a shared safePipe utility that replaces the scattered pipe+close boilerplate from PR #2407 with a single function that handles the Node.js pipe() gap (destination not ended on source destroy) and provides an optional onError callback for non-fatal error reporting. Closes #2450 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When htmlStream errors after startRSC is running, both the
htmlStream.on('error') handler and finished(htmlStream) rejection in
startRSC's catch block would emit on resultStream, causing duplicate
error reports. Now the htmlStream error handler only emits when startRSC
hasn't been called yet; once running, finished() handles it.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
AbanoubGhadban
force-pushed
the
abanoub/2450-inject-rsc-payload-error-logging
branch
from
eb254a4 to
9621073
Compare
PR Review: Fix silent error absorption in RSC payload injectionOverall this is a well-motivated PR. The 1.
|
| destination: T, | ||
| onError?: (err: Error) => void, | ||
| ): T { | ||
| source.pipe(destination); |
There was a problem hiding this comment.
Listeners are attached after pipe(), whereas utils.ts's safePipe attaches them before calling pipe(). Prefer the before-pipe order to eliminate any theoretical race with a synchronous 'error' emission during piping:
| source.pipe(destination); | |
| if (typeof (source as Readable).on === 'function') { | |
| const readableSource = source as Readable; | |
| if (onError) { | |
| readableSource.on('error', onError); | |
| } | |
| // 'close' fires after both normal 'end' and destroy(). | |
| // On normal end, pipe() already forwards 'end' to the destination — this is a no-op. | |
| // On destroy, pipe() unpipes but does NOT end the destination — we do it here. | |
| readableSource.on('close', () => { | |
| if (!destination.writableEnded) { | |
| destination.end(); | |
| } | |
| }); | |
| } | |
| source.pipe(destination); |
1 participant
Summary
injectRSCPayload.tswhere three error handlers (htmlStream.on('error', () => {}),catch {}instartRSC, and.catch(() => endResultStream())) swallowed errors, preventing them from reachingerrorReporter/SentrysafePipeutility that replaces the scattered pipe+close boilerplate with a single function handling the Node.jspipe()gap (destination not ended on source destroy) and optionalonErrorcallback for non-fatal error reportingtransformRSCNodeStream.tsthat PR Fix streaming renders hanging forever when errors occur during SSR #2407 did not coverHow it works
Errors emitted on
resultStreaminsideinjectRSCPayloadpropagate throughsafePipe'sonErrorat thepipeToTransformsite →emitError()→readableStream→handleStreamError→errorReporter. The pipe stays intact, data continues flowing, and Fastify never sees the errors.Test plan
injectRSCPayload.test.ts— 7 tests passstreamServerRenderedReactComponent.test.jsx— 16 tests passRSCRequestTracker.test.ts— 8 tests passstreamBackpressure.e2e.test.tsx— 4 tests passhandleStreamError.test.ts— 6 tests passstreamErrorHang.test.ts— 3 tests passCloses #2450
🤖 Generated with Claude Code