Walkthrough

Add explicit end-on-close/error handling across Node renderer and RSC streams: composite error listeners now forward errors and explicitly end downstream PassThrough/result streams when appropriate; tests, fixtures, demo routes/components, and a Ruby helper were added to exercise and prevent hung HTTP responses on upstream stream errors. (37 words)

Changes

Sequence Diagram(s)

sequenceDiagram
  participant Source as Readable (source)
  participant Handler as handleStreamError
  participant Dest as PassThrough (destination)
  participant Server as Fastify / HTTP Response
  participant Reporter as ErrorReporter

  Note over Source,Dest: Normal flow: source -> pipe -> PassThrough -> HTTP response
  Source->>Handler: provide stream & attach listeners
  Handler->>Dest: create PassThrough and pipe source -> PassThrough
  Source->>Dest: data chunks
  Dest->>Server: stream chunks to client

  Note over Source,Reporter: On source error
  Source-->>Handler: 'error' event (err)
  Handler->>Reporter: call onError(err)
  alt source.destroyed && !dest.readableEnded
    Handler->>Dest: end() destination (rgba(255,0,0,0.5))
    Dest->>Server: end response
  else
    Handler->>Dest: keep destination open (rgba(255,165,0,0.5))
  end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 I nibbled at a leaking stream,

Found the gaps where spinners dream.
I listen, call the watcher true,
Close the gate when streams undo.
🥕 Now responses finish — hop anew.

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 @coderabbitai help to get the list of available commands and usage tips.}

size-limit report 📦

Greptile Summary

This PR addresses two real and well-diagnosed production issues in the streaming SSR pipeline: (1) responses hanging forever when a source stream is destroyed because pipe() silently unpipes without ending the destination, and (2) errors in the RSC payload injection pipeline being silently absorbed before reaching error reporters like Sentry.

The core fix is a new safePipe utility (duplicated in both react-on-rails-pro and node-renderer packages) that closes the Node.js pipe() gap by listening to the 'close' event — which fires after both normal end() and destroy() — and ensuring the destination is properly ended in all cases. This is then applied consistently across the streaming pipeline: injectRSCPayload, streamingUtils, transformRSCNodeStream, and RSCRequestTracker. On the Ruby side, a safety net in consumer_stream_async resolves first_chunk_var with the raised error to prevent Rails request hangs for pre-first-chunk failures.

Key changes and observations:

• safePipe utility: Correctly attaches listeners before pipe() to avoid theoretical races; handles both Node.js Readable and React PipeableStream (which lacks .on()).
• injectRSCPayload: Switches from finished() (which rejects on stream destruction) to a 'close'-based promise that always resolves — preventing dangling RSC promises. However, Promise.allSettled(rscPromises) at line 277 discards rejected results, meaning errors from for await RSC stream processing can still be silently dropped.
• RSCRequestTracker: The new 'close' handler on the source RSC stream correctly unblocks for await consumers in startRSC when the source is destroyed without an error event.
• Ruby helper: The pattern of resolving Async::Variable with an exception object and raising it at the call site is idiomatic for the Async gem.
• The test suite is thorough, with the handleStreamError.test.ts suite including a documentation test that proves Node.js pipe() does not propagate source errors.

Confidence Score: 4/5

• This PR is safe to merge with one logic issue to address: RSC stream errors from for await processing remain silently dropped via uninspected Promise.allSettled results in injectRSCPayload.ts.
• The core fixes are well-reasoned and correctly address the two root causes (pipe() not ending destinations on destroy, and three silent catch blocks). The safePipe utility, RSCRequestTracker tee fix, and Ruby safety net are all correct. The Promise.allSettled without result inspection at line 277 partially contradicts the PR's own goal of fixing silent error absorption for RSC streams — errors from stream2 for await loops would still be dropped if they occur after htmlStream has closed. All other logic is sound and tests are comprehensive.
• packages/react-on-rails-pro/src/injectRSCPayload.ts — the Promise.allSettled call does not inspect rejected results, leaving RSC for-await errors silently dropped.

Important Files Changed

Sequence Diagram

sequenceDiagram
    participant React as React renderToPipeableStream
    participant PHS as pipeableHtmlStream
    participant HS as htmlStream (PassThrough)
    participant RSC as RSCRequestTracker
    participant SC as startRSC()
    participant RS as resultStream (PassThrough)
    participant FW as Fastify / handleStreamError

    Note over PHS,HS: safePipe(pipeableHtmlStream, htmlStream, onError)
    PHS->>HS: pipe() + 'close' handler

    React->>PHS: renders HTML chunks
    PHS->>HS: data chunks
    HS->>SC: first 'data' triggers startRSC()
    SC->>RSC: onRSCPayloadGenerated callback

    RSC-->>SC: stream2 (RSC payload chunks)
    SC->>RS: push RSC init scripts + HTML + RSC payload chunks

    alt Normal end
        PHS-->>HS: pipe forwards 'end'
        HS->>SC: 'close' resolves promise
        SC->>SC: await Promise.allSettled(rscPromises)
        SC->>RS: endResultStream()
    else Source destroyed (error)
        PHS->>HS: safePipe 'close' → destination.end()
        PHS--xRS: onError callback → resultStream.emit('error')
        HS->>SC: 'close' resolves promise
        SC->>RS: endResultStream()
    end

    RS->>FW: PassThrough firewall (errors never reach Fastify)
    Note over RS,FW: handleStreamError wraps RS in PassThrough

_{Last reviewed commit: 7e28e58}

Code Review – PR #2407 (Fix streaming renders hanging forever when errors occur during SSR)

The approach is sound and well-explained. The use of 'close' as the reliable termination signal is correct, and the test suite is comprehensive. A few issues to address before merging:

1. Ruby: raise e drops the original backtrace (bug)

In react_on_rails_pro_helper.rb, the rescue block uses raise e, which re-raises as a new exception with a new backtrace pointing to the raise line, discarding the original call stack. The correct Ruby idiom to re-raise and preserve backtrace is bare raise (no argument).

rescue StandardError => e
  begin
    first_chunk_var.value = nil
  rescue StandardError
    # Already resolved, safe to ignore
  end
  raise   # not: raise e
end

2. RSCRequestTracker.ts: misleading guard — writableEnded is not set by push(null)

The new 'close' handler comments say "On normal end, pipe() already forwards 'end' — this is a no-op." But this code does not use pipe() — it uses manual on('data')/on('end') forwarding. Calling stream1.push(null) sets the readable-side EOF flag, but does not set writableEnded = true. That flag is only set when end() is called.

Consequence: on a normal source end, both stream1.push(null) (from the 'end' handler) and stream1.end() (from the 'close' handler) are called. In Node.js 14+ this is effectively a no-op, but it's semantically incorrect and the comment is wrong. Consider tracking whether the end was already delivered via a boolean flag, or checking stream1.readableEnded (which is set synchronously when 'end' emits, before 'close' fires).

Also applies when the source is destroyed with an error: stream1.destroy(err) does not set writableEnded, so stream1.end() is also called afterward. Adding a !stream1.destroyed check would make the guard more robust:

(stream as Readable).on('close', () => {
  if (!stream1.destroyed && !stream1.writableEnded) stream1.end();
  if (!stream2.destroyed && !stream2.writableEnded) stream2.end();
});

3. injectRSCPayload.ts: errors are silently swallowed with no logging

Two places now silently discard errors:

a) The htmlStream.on('error', () => {}) handler has no logging at all. If htmlStream emits an error, the caller has no visibility. At minimum, log the error even if not propagating it.

b) The startRSC catch block:

} catch {
  endResultStream();
}

The bound variable (err) was removed, so there is no way to log the actual error. This replaces the previous resultStream.emit('error', err) which, while causing the hanging issue, at least surfaced the error. The new code should log the error to stderr or to whatever logger is used in this package.

4. injectRSCPayload.ts: rscPromise.catch in the 'end' handler is unreachable dead code

startRSC catches all of its own errors internally:

const startRSC = async () => {
  try { ... } catch { endResultStream(); }  // never re-throws
};

Since startRSC never rejects, rscPromise always resolves. The catch(() => endResultStream()) on the chain in the 'end' handler is therefore unreachable. This is misleading — it looks like a safety net but provides none. Either remove it or make startRSC re-throw so the catch is actually reachable.

5. injectRSCPayload.ts: cleanup() calls resultStream.end() without checking writableEnded

If startRSC catches an error and calls endResultStream() (which ends the result stream), rscPromise still resolves (per point 4), and the 'end' handler's rscPromise.then(cleanup) will execute cleanup(), which calls resultStream.end() unconditionally. Node.js handles a second end() call gracefully, but cleanup() should guard the same way endResultStream() does:

const cleanup = () => {
  if (flushTimeout) clearTimeout(flushTimeout);
  flush();
  if (!resultStream.writableEnded) resultStream.end(); // add guard
};

Minor observations

• The fixtures/stream-test-bundle.js sets global.Readable = require('stream').Readable. This is intentional for the VM test harness, but a comment explaining it's a test-only fixture would help future readers.
• Several tests use setTimeout(resolve, 50) for synchronization. These are potentially flaky on heavily loaded CI systems. Event-driven synchronization (waiting on stream events directly) would be more reliable, though the 5s test timeout provides enough headroom in practice.

PR Review: Fix Streaming SSR Hangs on Error

Overall Assessment: The root cause diagnosis is accurate and well-documented. Using 'close' as the reliable termination signal (rather than 'error') is the correct Node.js stream approach, and the fix is applied consistently across all affected layers. The PR includes good test coverage including unit tests that document the pipe() non-propagation behavior.

A few issues worth addressing before merge:

1. Silent Error Swallowing (Most Significant)

injectRSCPayload.ts has replaced error propagation with complete silence:

• htmlStream.on('error', () => {}) — no-op handler, errors are completely discarded
• } catch { endResultStream(); } — error is discarded without logging
• .catch(() => endResultStream()) — same pattern

The original code emitted errors to resultStream. The PR description correctly notes that emitting on resultStream causes Fastify to destroy the socket mid-response. However, the fix goes too far in the other direction: errors are now invisible to any consumer or debugging system. At minimum, errors should be logged (e.g., via an onError callback or a logger) even if they're not propagated downstream.

Recommendation: Pass an error callback or integrate with the existing errorReporter pattern used elsewhere in the codebase, so errors are visible without causing socket destruction.

2. Unnecessary Arrow Function Wrapper (utils.ts)

stream.on('error', (error) => {
  onError(error);
});

This is functionally identical to stream.on('error', onError) — the wrapper adds no value.

3. Dead Code in StreamShellErrorDemo.jsx

const StreamShellErrorDemo = () => {
  throw new Error('Component crashed immediately during shell render!');
  return <div>This will never render</div>; // unreachable
};

The return statement after throw is unreachable dead code. The component works fine without it (TypeScript doesn't require a return after an unconditional throw).

4. Undocumented Addition: server_side_hello_world_hooks Controller Action

The PR adds a controller action for server_side_hello_world_hooks which is not mentioned in the PR description or summary table. The route and view already existed, but the controller action was apparently missing. If this is a pre-existing bug fix, it should be noted in the description to make the change auditable.

Positive Observations

• The table in the PR description explaining stream.pipe() behavior for normal end, destroy(err), and emit('error') is an excellent reference and should be preserved in a code comment or architecture doc.
• The 'close'-over-'error' approach is correctly applied at every layer of the pipeline.
• The writableEnded guards before calling .end() prevent double-end errors.
• The Ruby rescue block correctly re-raises after resolving first_chunk_var so callers aren't left hanging.
• Test coverage is thorough, including both unit tests for stream semantics and E2E tests verifying actual HTTP response completion.

PR Review: Fix streaming renders hanging forever when errors occur during SSR

This is a well-researched fix for a genuine Node.js stream semantics issue. The root cause analysis is accurate, and the approach of using 'close' as the termination signal is the correct solution. The comments and documentation throughout are exemplary.

A few issues worth addressing:

1. Silent error swallowing in injectRSCPayload.ts (three places)

The most significant concern is that errors are now silently swallowed in three locations, replacing the old behavior that at least propagated errors to consumers via resultStream.emit('error', err):

• Line 270–272: The startRSC catch block calls endResultStream() but discards the error entirely. Failures during RSC processing (e.g., a component stream erroring) become invisible.
• Line 303: htmlStream.on('error', () => {}) is a no-op. While preventing an uncaught exception crash is correct, the error should at minimum be logged.
• Line 347: .catch(() => endResultStream()) in the 'end' handler discards any error thrown from rscPromise.

Downstream consumers (and developers debugging production issues) lose all visibility into why a response body ended early. At minimum, these should log the error. If there's a logger available in this context, passing the error to it would be appropriate.

2. cleanup function calls resultStream.end() without a writableEnded guard (line 334)

endResultStream() (line 199–205) guards with if (!resultStream.writableEnded), but the cleanup function called from the 'end' handler (line 334) calls resultStream.end() directly without that guard.

Failure scenario: startRSC awaits finished(htmlStream), then Promise.all(rscPromises) — if that rejects, the catch block calls endResultStream() (setting writableEnded = true). startRSC then resolves (it swallows the error), so rscPromise.then(cleanup) fires and cleanup calls resultStream.end() on an already-ended stream. In recent Node.js versions this is a no-op, but it is semantically incorrect and may behave differently across Node versions.

Fix: add if (!resultStream.writableEnded) guard in cleanup, or better — have cleanup call endResultStream() directly.

3. AlwaysFailsAsync demo component uses the wrong Suspense pattern

In StreamErrorDemo.jsx, AlwaysFailsAsync returns a Promise instead of throwing one (or using use()). In React's streaming SSR, a component suspends by throwing a Promise (legacy) or using the use() hook (React 19). Returning a Promise from a function component produces an invalid React element, which React will fail to render — but not via the Suspense boundary mechanism. The error will likely be a shell error rather than a Suspense boundary error, so the demo may not accurately demonstrate issue #2402's specific scenario (mid-stream Suspense boundary failure).

Minor: server_side_hello_world_hooks action added without apparent template

The controller adds a server_side_hello_world_hooks action (no template visible in the diff). The route already exists in routes.rb, so a 404 isn't a concern, but if this action was added intentionally here, a corresponding template should be included.

PR Review: Fix streaming renders hanging forever when errors occur during SSR

The root-cause analysis and overall approach are correct and well-documented. Using 'close' as the authoritative termination signal (rather than 'error' or 'end') is the right design for Node.js stream error handling. The test coverage — both unit and E2E — is solid and directly validates the bug being fixed.

A few issues worth addressing before merge:

1. Silent error swallowing in startRSC catch block (injectRSCPayload.ts:270)

} catch {
  endResultStream();
}

The original emitted resultStream.emit('error', err), passing the error along. The new code drops err entirely — no logging, no reporting. If finished(htmlStream) or RSC promise processing fails, that failure is now invisible. Even if we don't want to propagate the error to Fastify (to avoid socket destruction), the error should at minimum be logged or passed to a reporter so operators can observe it.

2. Silent htmlStream.on('error', () => {}) handler (injectRSCPayload.ts:303)

htmlStream.on('error', () => {});

Again, the error is completely discarded. The intent (prevent unhandled-error crash) is correct, but the implementation loses diagnostic information. Even a console.error or calling the existing error reporter here would help when debugging production issues.

3. Misleading comment in RSCRequestTracker.ts (line ~163)

// On normal end, pipe() already forwards 'end' — this is a no-op.

This comment was copy-pasted from handleStreamError but doesn't apply here — this code doesn't use pipe(). It manually forwards data with stream.on('data'). On a normal end, stream1.push(null) is called, but writableEnded stays false (only stream.end() sets it). So the close handler will call stream1.end() / stream2.end() on normal termination — it is not a no-op. The behavior is still correct, but the comment is actively misleading.

4. close handler doesn't guard against already-destroyed streams in RSCRequestTracker.ts

(stream as Readable).on('close', () => {
  if (!stream1.writableEnded) stream1.end();
  if (!stream2.writableEnded) stream2.end();
});

When the source errors, stream.on('error') calls stream1.destroy(err). After that, close fires and calls stream1.end() on an already-destroyed stream. Calling end() on a destroyed PassThrough is a no-op in practice, but the intent (and guard condition) would be clearer with !stream1.writableEnded && !stream1.destroyed.

5. writableEnded check after push(null) in RSCRequestTracker.ts

Related to #3: push(null) signals EOF on the readable side but does not set writableEnded. This means for normal stream ends, the close handler always executes stream1.end() and stream2.end(). This is safe — calling end() after push(null) is harmless — but it contradicts the comment claiming it's a no-op.

Minor: Stray controller action in the diff

def server_side_hello_world_hooks
  stream_view_containing_react_components(template: "/pages/server_side_hello_world_hooks")
end

This action is added in pages_controller.rb and the route already existed. Was this intentionally included? If so, a brief note in the PR description or commit message would clarify why it's part of this fix.

Summary: The fix is mechanically sound. The main concern is that two error paths (startRSC catch, htmlStream.on('error')) now silently swallow errors that previously surfaced. In production, this trades a potential socket-destruction problem for invisible failures. At minimum, add error reporting/logging in both paths.