fix(backend-native): unguarded .unwrap() in js_stream_push_chunk#10882
Open
tlangton3 wants to merge 1 commit into
Open
fix(backend-native): unguarded .unwrap() in js_stream_push_chunk#10882tlangton3 wants to merge 1 commit into
tlangton3 wants to merge 1 commit into
Conversation
…m to UserError via reject `js_stream_push_chunk` in the JS-Rust streaming bridge previously called `.unwrap()` on the result of `transform_response`. If the chunk pushed from JS contained a value of an unexpected shape (e.g. a JS array where a primitive was expected), `transform_response` returned `Err` and `.unwrap()` panicked inside Neon's bridge context — Neon recovers the panic by surfacing it as `"internal error in Neon module: called Result::unwrap()..."` rather than the underlying error message, and depending on the surrounding JS state can leak an unhandled rejection on Node's tick queue. This is distinct from cube-js#10875's BigQuery production crash (which fires upstream of `js_stream_push_chunk` and is fixed in a sibling commit to `BigQueryDriver.stream`). It is, however, a separate latent panic vector across the FFI boundary that's worth hardening — the kind of "theoretical bomb" the next driver to mis-shape a chunk would detonate. Replaces the `.unwrap()` with a match arm that: - calls `JsWriteStream::reject(err_msg)` so the error flows through the existing mpsc channel and the wire layer can emit a structured Postgres `ErrorResponse`, - routes the JS-side per-chunk callback through the same `wait_for_future_and_execute_callback` wrapper used on the success path so async-callback timing is consistent (no Zalgo). Adds a regression test (`does not crash node when a stream chunk fails to transform`) using a synthetic `FakeMalformedRowStream` that pushes a chunk whose field values are nested arrays — guaranteed to fail `JsValueObject::get` and exercise the new error arm. Verified the test fails when the fix is reverted (Neon's panic-wrapper string appears in the wire output) and passes with the fix (the underlying transform error reaches the client cleanly).
github-actions
Bot
added
rust
tlangton3
changed the title
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
1 participant
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
Replaces an unguarded
.unwrap()in the JS-Rust streaming bridge (js_stream_push_chunk) with areject()-based error flow.The
.unwrap()is only reachable if a driver pushes a chunk shape that failsJsValueObject::get's primitive-type extraction (e.g. a JS array where the schema declares a primitive). Well-behaved drivers don't currently exercise this path. When it does fire, Neon's panic-recovery wrapper hides the underlyingCubeErrormessage — callers see"internal error in Neon module: called Result::unwrap()..."instead of the actual cause. Panicking across the Neon FFI boundary is also an undesirable class of bug independent of practical reachability.Found while investigating #10875 (the actual fix for that is in #10877).
Changes
Replaced the
.unwrap()with an error arm that:JsWriteStream::reject(err_msg)to surface the error through the Rust mpsc channel to the wire layer.wait_for_future_and_execute_callbackto prevent the Node side from hanging. Both notifications are required here to satisfy both ends of the FFI bridge.Testing
Added
FakeMalformedRowStreamtotest/response-fake.tsto push a chunk with invalid nested arrays. The new test (does not crash node when a stream chunk fails to transform) verifies that the underlying error reaches the wire cleanly without triggering Neon's panic-recovery wrapper.