fix(ssr): surface response stream errors in logs and OTel traces#321
Open
sauravvarma wants to merge 1 commit into
Open
fix(ssr): surface response stream errors in logs and OTel traces#321sauravvarma wants to merge 1 commit into
sauravvarma wants to merge 1 commit into
Conversation
Node swallows response stream errors such as ERR_STREAM_WRITE_AFTER_END when res has no error listener: the write is dropped, nothing is logged, and a truncated document ships silently. This is how the streaming race in #320 went unnoticed in production. Attach one guarded error listener per response in renderMarkUp that logs every stream error with the request URL. When OTEL_ENABLE is set, the error is also recorded as a dedicated responseStreamError span in the request's trace via the new createStreamErrorRecorder helper in otel.js. The recorder captures the ambient context while the request span is still active, because the error event usually fires after that span has ended and recordException on an ended span is dropped. Observability only: streaming behavior and response bytes are unchanged. The race itself is tracked in #320.
|
DeputyDev will no longer review pull requests automatically.To request a review, simply comment #review on your pull request—this will trigger an on-demand review whenever you need it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Makes response stream errors during SSR streaming observable. Today they are invisible: Node swallows errors like
ERR_STREAM_WRITE_AFTER_ENDwhen the response has noerrorlistener, so the write is dropped, nothing is logged anywhere, and a truncated document ships silently. That silence is exactly how the streaming race in #320 went unnoticed (every SSR response on react-dom 19 currently loses React's closing</body></html>chunk with a swallowed write-after-end on every request).This PR is observability only. It does not fix the race (#320 tracks that); it makes the failure, and any future response stream error, impossible to miss in logs and traces.
What changed
src/server/renderer/handler.jsxrenderMarkUpnow attaches a singleerrorlistener toresbefore streaming starts. Every stream error is logged with the request URL:The listener is guarded by a
WeakSetbecauserenderMarkUpcan run twice for one response (the fetcher error path re-renders with a 404) anderrorlisteners must not stack.src/otel.jscreateStreamErrorRecorder(serviceName, name = "responseStreamError")helper. Stream errors fire on the response'serrorevent, typically after the span that performed the offending write has already ended, andrecordExceptionon an ended span is silently dropped. The helper therefore capturescontext.active()while the request span is alive and, per error, emits a dedicatedresponseStreamErrorspan into the same trace: statusERROR,exceptionevent, and anerror.typeattribute carrying the error code. Same parenting approach asresponseFlushMiddleware's post-end spans.OTEL_ENABLEis set, through the existing lazyotel.jsimport; when tracing is off, the recorder isnulland only the console log runs.How it was verified
eslinton both files reports the same 5 pre-existing errors asmain, nothing new.@opentelemetry/sdk-trace-node): recorder created inside an activerenderMarkUpspan, span ended, error fired on a later tick. All checks pass: error span exported, sametraceIdas the request span, parented to therenderMarkUpspan, statusERRORwith the error message,exceptionevent recorded,error.type = ERR_STREAM_WRITE_AFTER_END.apps/catalyst-core-test(packed current branch,catalyst build,NODE_ENV=production catalyst serve): every request to/now produces the log line above; on this branch'smainthe same requests produce no output at all.<script src>count, and, per SSR streaming race: pipe(res) in onShellReady competes with res.write()/res.end() in onAllReady, truncating every SSR response #320, still no</html>; that bug is intentionally untouched here).A side benefit: response error paths that end in
res.destroy(err)(fatal render errors, client disconnects) now also have a listener, so they are logged instead of relying on Node's default handling.Refs #320.