Add stream error handlers to prevent process crashes#3385
Add stream error handlers to prevent process crashes#3385rajin-kichannagari wants to merge 4 commits into
Conversation
Attach error handlers to streaming responses to prevent unhandled error events from crashing the Node.js process when the LCS connection drops mid-stream. Changes: - Add error handler to nodeStream in POST /v1/query endpoint (router.ts) - Add error handler to response object to detect client disconnections - Add error handlers to both source and transform streams in notebooks query endpoint - Ensure streams are properly destroyed on error to prevent hanging connections Addresses security vulnerability identified in Red Hat Product Security threat model (dated Mar 15, 2026). Co-Authored-By: Rajin Kichannagari <rkichann@redhat.com> Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
rajin-kichannagari
requested review from
a team,
karthikjeeyar,
rohitkrai03 and
yangcao77
as code owners
|
This pull request adds a new top-level directory under |
Missing ChangesetsThe following package(s) are changed by this PR but do not have a changeset:
See CONTRIBUTING.md for more information about how to add changesets. Changed Packages
|
Fixes TS7006 'Parameter implicitly has an any type' errors in CI. Also removes leftover debug console.log. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3385 +/- ##
==========================================
- Coverage 52.87% 52.85% -0.02%
==========================================
Files 2408 2408
Lines 87639 87668 +29
Branches 24294 24295 +1
==========================================
+ Hits 46336 46341 +5
- Misses 39800 39824 +24
Partials 1503 1503
*This pull request uses carry forward flags. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Found 3 issues:
-
Transform stream error handler doesn't destroy upstream body stream
(resource leak)When
transformStreamerrors,body.destroy()is never called. The
upstream stream keeps reading data, causing memory leaks. -
Double-destroy race condition in response error handler
body.destroy() triggers body.on('error') which calls
transformStream.destroy(), then line 536 calls it again.
transformStream.destroy() executes twice.
-
Upstream fetch not cancelled on client disconnect (connection pool
exhaustion)nodeStream.destroy()doesn't cancel the underlying fetch. The upstream
HTTP connection stays open, draining data from Lightspeed backend even after
client disconnects.
| } | ||
| }); | ||
|
|
||
| // Handle client disconnection (RHIDP-13064) |
| if (fetchResponse.body) { | ||
| const nodeStream = Readable.fromWeb(fetchResponse.body as any); | ||
|
|
||
| // Handle stream errors (RHIDP-13064) |
- Add AbortController to cancel underlying fetch on client disconnect, preventing connection pool exhaustion - Add body.destroy() + abortController.abort() in transform stream error handler to prevent resource leaks - Remove redundant transformStream.destroy() from client disconnect handler to avoid double-destroy race condition - Remove internal ticket references from inline comments Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
rajin-kichannagari
force-pushed
the
fix/stream-error-handling-rhidp-13064
branch
from
13b99a6 to
1063b5a
Compare
|
2 participants
Summary
Attach error handlers to streaming responses to prevent unhandled error events from crashing the Node.js process when the LCS connection drops mid-stream.
Addresses security vulnerability identified in Red Hat Product Security threat model (dated Mar 15, 2026).
Changes
/v1/queryendpoint (router.ts)/v1/sessions/:sessionId/query)Affected Endpoints
/v1/queryendpoint (Lightspeed router)/v1/sessions/:sessionId/queryendpoint (Notebooks router)Checklist
Testing note: Verified with manual tests simulating LCS stream failures and client disconnections. Error handlers work correctly and prevent process crashes. Existing unit tests verify the happy path.