fix(node): forward streamAllMessages errors to JavaScript callbacks#3837
Draft
XHFkindergarten wants to merge 1 commit into
Draft
fix(node): forward streamAllMessages errors to JavaScript callbacks#3837XHFkindergarten wants to merge 1 commit into
XHFkindergarten wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3837 +/- ##
=======================================
Coverage 85.18% 85.18%
=======================================
Files 412 412
Lines 64132 64132
=======================================
Hits 54630 54630
Misses 9502 9502 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
Forward per-item
SubscribeErrorvalues emitted byConversations::stream_all_messagesto the Node N-API callback instead oflogging and discarding them.
The stream continues consuming subsequent items after reporting the error.
Context
#899 and #1182 introduced stream error propagation so external SDKs could
observe and handle failures.
#1938 later made
stream_all_messagesa special case: errors are logged anddiscarded before crossing the N-API boundary. At that time the Node
AsyncStreamended when an error reached it. xmtp-js #1118 subsequentlychanged that behavior: item errors are passed to
onErrorand the streamremains active.
As a result, the original reason for suppressing these errors no longer
applies. Keeping the suppression now makes the Node SDK's
onErrorhandlerunreachable for aggregate message-stream item errors, while conversation,
single-conversation message, consent, preference, Mobile, and WASM streams
still expose their errors.
This makes failures associated with reports such as #2351 and #3541 silent to
Node applications and prevents consumers from applying an explicit recovery
policy.
Behavior
Before:
Rust stream item Err -> warning log -> no JavaScript callbackAfter:
Rust stream item Err -> callback(error, undefined) -> stream continuesThere is no public API signature change.
Scope
This PR restores observability at the Node binding boundary. It does not claim
to fix the underlying transport or MLS delivery omissions tracked by #2351 and
#3541, and it does not change cursor or retry behavior.
Verification
cargo check --locked -p bindings_node --libyarn workspace @xmtp/node-sdk test test/streams.test.tseslint node-sdk/test/streams.test.tsRefs #899, #1182, #1938, #2351, #3541.
Note
Forward
streamAllMessageserrors to JavaScript callbacks instead of silently skipping themConversations.streamasync method were logged and dropped; the JS callback never received them.OkandErrresults are forwarded to the JSThreadsafeFunctionviaErrorWrapper, so callers can handle per-item failures without the stream terminating.onErrorwhile subsequent values are still delivered andonFailis not called.Macroscope summarized 34a1b14.