fix(client): preserve recipient in <ack>, soften unknown stream:error - #633
Merged
Conversation
Updated stream error handling to maintain connection during unknown errors and added recipient attribute handling in ACK nodes.
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughUnknown or ack-shaped ChangesClient stream handling and ACK encoding fixes
Sequence Diagram(s)sequenceDiagram
participant Socket
participant Client
participant StreamHandler
Socket->>Client: deliver <stream:error> (no code or contains <ack/>)
Client->>StreamHandler: handle_stream_error
StreamHandler->>Client: dispatch StreamError event (no expected_disconnect)
alt should_disconnect true
StreamHandler->>Client: clear is_logged_in / set expected_disconnect
Client->>Client: notify_connection_shutdown()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client.rs (1)
3216-3336:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t clear auth state on the non-fatal
stream:errorpath.Line 3217 runs before you decide whether this error should actually tear the connection down. After this change,
429,503, and code-less / ack-shapedstream:errorstanzas keep the socket alive, so the client can end up connected withis_logged_in == false. That flipsis_fully_ready()false and can breakwait_for_connected()and other login-gated flows even though we intentionally stayed online.Suggested fix
pub(crate) async fn handle_stream_error(&self, node: &wacore_binary::NodeRef<'_>) { - self.is_logged_in.store(false, Ordering::Relaxed); - let mut attrs = node.attrs(); let code_cow = attrs.optional_string("code"); let code = code_cow.as_deref().unwrap_or(""); @@ if should_disconnect { + self.is_logged_in.store(false, Ordering::Relaxed); let transport_opt = self.transport.lock().await.clone(); if let Some(transport) = transport_opt { self.runtimeIf any event handlers need to observe the logged-out state during dispatch, set the flag inside each fatal branch before
dispatch(...)instead of once up front. Also pin this with a regression assert in the new unknown-stream-error tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client.rs` around lines 3216 - 3336, In handle_stream_error, don't unconditionally clear self.is_logged_in at the top; instead set self.is_logged_in.store(false, Ordering::Relaxed) only inside the fatal branches that actually log out / disconnect (the conflict branch where conflict_type != "", and the explicit fatal code branches: "515", "516", "401", "409" — and any other branches you consider fatal) immediately before setting expected_disconnect/enable_auto_reconnect and dispatching the LoggedOut/StreamReplaced events; leave 429, "503", and the default/code-less branch untouched so the client remains logged in. Also add a regression assertion in the new unknown-stream-error test to ensure is_logged_in remains true for non-fatal errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/client.rs`:
- Around line 3216-3336: In handle_stream_error, don't unconditionally clear
self.is_logged_in at the top; instead set self.is_logged_in.store(false,
Ordering::Relaxed) only inside the fatal branches that actually log out /
disconnect (the conflict branch where conflict_type != "", and the explicit
fatal code branches: "515", "516", "401", "409" — and any other branches you
consider fatal) immediately before setting
expected_disconnect/enable_auto_reconnect and dispatching the
LoggedOut/StreamReplaced events; leave 429, "503", and the default/code-less
branch untouched so the client remains logged in. Also add a regression
assertion in the new unknown-stream-error test to ensure is_logged_in remains
true for non-fatal errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d2a1cbeb-9930-4c9f-b640-b0ee24796e12
📒 Files selected for processing (1)
src/client.rs
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client.rs (1)
3314-3343:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't clear login state for non-fatal
stream:error.This change intentionally keeps unknown / ack-shaped
stream:errorstanzas on the current socket, but Line 3225 still setsis_logged_intofalsebefore we know whether the branch will disconnect. That leaves the client in a live-but-logged-out state after informational errors, which can breakwait_for_connected()and any callers checkingis_logged_in().Suggested fix
pub(crate) async fn handle_stream_error(&self, node: &wacore_binary::NodeRef<'_>) { - self.is_logged_in.store(false, Ordering::Relaxed); - let mut attrs = node.attrs(); let code_cow = attrs.optional_string("code"); let code = code_cow.as_deref().unwrap_or(""); let conflict_type = node .get_optional_child("conflict") @@ if should_disconnect { + self.is_logged_in.store(false, Ordering::Relaxed); let transport_opt = self.transport.lock().await.clone(); if let Some(transport) = transport_opt { self.runtime .spawn(Box::pin(async move { transport.disconnect().await;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client.rs` around lines 3314 - 3343, The handler currently clears self.is_logged_in regardless of whether the stream error is fatal; update the logic so is_logged_in is only set to false when should_disconnect is true (i.e., when you actually tear down the connection). Concretely, remove or defer the code that sets self.is_logged_in = false from the early/unknown-error path and move it into the branch where you perform the disconnect (the block that checks should_disconnect, acquires transport via self.transport.lock().await, spawns transport.disconnect(), calls self.notify_connection_shutdown(), and logs the shutdown). This preserves login state for non-fatal/ack-shaped stream errors while ensuring it is cleared when you intentionally disconnect.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client.rs`:
- Around line 5896-5969: Add a test that exercises the real encoder: instead of
only calling build_ack_node(), call encode_ack_bytes(&incoming.as_node_ref(),
Some(&own_device_pn)) to get the serialized ACK, then parse the serialized bytes
back into a Node (e.g. via the repository's node-decoding helper such as
decode_node_from_bytes or equivalent) and assert the parsed ACK node has the
correct "class" and that "recipient" is present/echoed when the incoming had one
and absent when it did not; this ensures encode_ack_bytes (not just
build_ack_node) preserves/omits the recipient as expected.
---
Outside diff comments:
In `@src/client.rs`:
- Around line 3314-3343: The handler currently clears self.is_logged_in
regardless of whether the stream error is fatal; update the logic so
is_logged_in is only set to false when should_disconnect is true (i.e., when you
actually tear down the connection). Concretely, remove or defer the code that
sets self.is_logged_in = false from the early/unknown-error path and move it
into the branch where you perform the disconnect (the block that checks
should_disconnect, acquires transport via self.transport.lock().await, spawns
transport.disconnect(), calls self.notify_connection_shutdown(), and logs the
shutdown). This preserves login state for non-fatal/ack-shaped stream errors
while ensuring it is cleared when you intentionally disconnect.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b9c33023-50bc-4098-a4a8-751963a3d1fb
📒 Files selected for processing (1)
src/client.rs
Refactor stream error handling to only clear is_logged_in in fatal cases, ensuring session remains alive for non-fatal errors.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client.rs (1)
6114-6129: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAssert the per-connection shutdown signal stays quiet here.
This is the core contract. Right now these tests only prove the flags stayed stable. If
notify_connection_shutdown()slips back into the non-fatal path, both tests still pass and we get the reconnect storm again. Add an explicit!client.connection_shutdown_signal().is_fired()assertion in both cases.Proposed test tightening
client.handle_stream_error(&node.as_node_ref()).await; assert!( client.is_logged_in.load(Ordering::Relaxed), "unknown stream:error must NOT log the client out" ); + assert!( + !client.connection_shutdown_signal().is_fired(), + "unknown stream:error must NOT fire per-connection shutdown" + ); assert!( !client.expected_disconnect.load(Ordering::Relaxed), "unknown stream:error must not mark the disconnect as expected" );client.handle_stream_error(&node.as_node_ref()).await; assert!( client.is_logged_in.load(Ordering::Relaxed), "ack-shaped stream:error must NOT log the client out" ); + assert!( + !client.connection_shutdown_signal().is_fired(), + "ack-shaped stream:error must NOT fire per-connection shutdown" + ); assert!( !client.expected_disconnect.load(Ordering::Relaxed), "ack-shaped stream:error must not mark the disconnect as expected" );Also applies to: 6138-6155
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client.rs` around lines 6114 - 6129, Add assertions to ensure the per-connection shutdown signal remains unfired after handling unknown stream errors: after calling client.handle_stream_error(&node.as_node_ref()) in the test block that sets client.is_logged_in and checks flags, also assert !client.connection_shutdown_signal().is_fired() so notify_connection_shutdown() hasn't been invoked; do the same in the other test case around lines 6138-6155. This uses the existing NodeBuilder and connection_shutdown_signal() API and prevents slipping notify_connection_shutdown() into the non-fatal path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/client.rs`:
- Around line 6114-6129: Add assertions to ensure the per-connection shutdown
signal remains unfired after handling unknown stream errors: after calling
client.handle_stream_error(&node.as_node_ref()) in the test block that sets
client.is_logged_in and checks flags, also assert
!client.connection_shutdown_signal().is_fired() so notify_connection_shutdown()
hasn't been invoked; do the same in the other test case around lines 6138-6155.
This uses the existing NodeBuilder and connection_shutdown_signal() API and
prevents slipping notify_connection_shutdown() into the non-fatal path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 69b6441a-31bd-444f-91c8-1d1fb9b4f5fb
📒 Files selected for processing (1)
src/client.rs
Refactor comments for clarity and add unit test for ACK encoding.
…gence Drop the four per-branch is_logged_in.store(false) calls in 515/516/401/409 now that the if-should_disconnect block clears it once for every opt-in path (including conflict). Rewrite the catch-all comment to make the deliberate divergence from whatsmeow explicit: whatsmeow eagerly clears isLoggedIn, but here is_fully_ready() gates prekey uploads, so we keep it true while the socket is still alive.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client.rs`:
- Around line 3316-3323: The log here is intentionally non-fatal but still uses
error! which contradicts the “soften unknown stream:error” intent; change the
severity from error! to warn! for the Unknown stream error logging (the call
that currently does error!("Unknown stream error: {}",
DisplayableNodeRef(node))) so it doesn't trigger pager/alerting while still
recording the event before calling
self.core.event_bus.dispatch(Event::StreamError(...)).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 94fc71fd-1670-4c6b-88ff-9b3f08fbe765
📒 Files selected for processing (1)
src/client.rs
…o warn 429 and 503 are explicit server-side rejections of the current session. Pre-PR, the catch-all cleared is_logged_in for every code (including 429/503) — too aggressive for unknown/<ack/>-shaped routing errors, but correct for these two: while the socket is still up and reconnect hasn't fired yet, outgoing sends would otherwise either retry against a dying socket (503) or fuel the rate-limit the server just flagged (429). Whatsmeow clears isLoggedIn at the very top for all branches; this lines up Rust with that intent for 429/503 specifically, leaving the unknown catch-all alone so prekey uploads keep working through routing-error storms. Downgrade the unknown-stream-error log from error! to warn!: the connection is intentionally preserved, so this is not a fatal event and shouldn't page on logs that match error severity. Tests updated: 429/503 now also assert is_logged_in flips to false and expected_disconnect stays false.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client.rs (1)
3247-3305:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear
is_logged_inbefore firing logout/replaced events.This path now emits
LoggedOut/StreamReplacedwhileis_logged_inis stilltrue, because the shared clear only happens later in the disconnect block. That means handlers can observe a stale session state on the fatal path, which is inconsistent with the “dispatch after cleanup so handlers see cleared state” pattern elsewhere in this file.Suggested fix
+ let mut disconnect_event = None; + if !conflict_type.is_empty() { info!( "Got stream error indicating client was removed or replaced (conflict={}). Logging out.", conflict_type ); self.expected_disconnect.store(true, Ordering::Relaxed); self.enable_auto_reconnect.store(false, Ordering::Relaxed); - let event = if conflict_type == "replaced" { + disconnect_event = Some(if conflict_type == "replaced" { Event::StreamReplaced(crate::types::events::StreamReplaced) } else { Event::LoggedOut(crate::types::events::LoggedOut { on_connect: false, reason: ConnectFailureReason::LoggedOut, }) - }; - self.core.event_bus.dispatch(event); + }); should_disconnect = true; } else { match code { "515" => { @@ "516" => { info!("Got 516 stream error (device removed). Logging out."); self.expected_disconnect.store(true, Ordering::Relaxed); self.enable_auto_reconnect.store(false, Ordering::Relaxed); - self.core.event_bus.dispatch(Event::LoggedOut( + disconnect_event = Some(Event::LoggedOut( crate::types::events::LoggedOut { on_connect: false, reason: ConnectFailureReason::LoggedOut, }, - )); + )); should_disconnect = true; } "401" => { info!("Got 401 stream error (unauthorized). Logging out."); self.expected_disconnect.store(true, Ordering::Relaxed); self.enable_auto_reconnect.store(false, Ordering::Relaxed); - self.core.event_bus.dispatch(Event::LoggedOut( + disconnect_event = Some(Event::LoggedOut( crate::types::events::LoggedOut { on_connect: false, reason: ConnectFailureReason::LoggedOut, }, - )); + )); should_disconnect = true; } "409" => { info!("Got 409 stream error (conflict). Another session replaced this one."); self.expected_disconnect.store(true, Ordering::Relaxed); self.enable_auto_reconnect.store(false, Ordering::Relaxed); - self.core - .event_bus - .dispatch(Event::StreamReplaced(crate::types::events::StreamReplaced)); + disconnect_event = + Some(Event::StreamReplaced(crate::types::events::StreamReplaced)); should_disconnect = true; } @@ if should_disconnect { self.is_logged_in.store(false, Ordering::Relaxed); + if let Some(event) = disconnect_event { + self.core.event_bus.dispatch(event); + } let transport_opt = self.transport.lock().await.clone(); if let Some(transport) = transport_opt { self.runtimeAlso applies to: 3344-3346
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client.rs` around lines 3247 - 3305, Before dispatching LoggedOut or StreamReplaced events, clear the client's is_logged_in flag so handlers don't observe a stale logged-in state; specifically, set self.is_logged_in.store(false, Ordering::Relaxed) (or call the shared clear/login-reset helper if one exists) immediately before any self.core.event_bus.dispatch(...) in the conflict_type branch (where Event::StreamReplaced / Event::LoggedOut are created) and in the individual error-code branches ("516", "401", "409" and the other conflict path), preserving the existing expected_disconnect and enable_auto_reconnect changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client.rs`:
- Around line 3307-3321: The "429" and "503" match arms currently change
teardown flags (self.is_logged_in, self.auto_reconnect_errors) but no longer
emit the structured Event::StreamError, making these conditions invisible to
consumers; update the "429" and "503" branches inside the stream error match to
construct and send/publish an Event::StreamError (including the status code
string and a brief message) to the same event sink/dispatcher used elsewhere
before or immediately after changing self.is_logged_in and auto_reconnect_errors
so callers receive the stream error while the socket teardown behavior remains
unchanged (refer to Event::StreamError, the "429"/"503" match arms,
self.is_logged_in.store, and self.auto_reconnect_errors.fetch_add).
---
Outside diff comments:
In `@src/client.rs`:
- Around line 3247-3305: Before dispatching LoggedOut or StreamReplaced events,
clear the client's is_logged_in flag so handlers don't observe a stale logged-in
state; specifically, set self.is_logged_in.store(false, Ordering::Relaxed) (or
call the shared clear/login-reset helper if one exists) immediately before any
self.core.event_bus.dispatch(...) in the conflict_type branch (where
Event::StreamReplaced / Event::LoggedOut are created) and in the individual
error-code branches ("516", "401", "409" and the other conflict path),
preserving the existing expected_disconnect and enable_auto_reconnect changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e6aee783-390f-450c-8ad0-930eb22e2855
📒 Files selected for processing (1)
src/client.rs
Comment on lines
3307
to
+3321
| "429" => { | ||
| // Server signalled rate-limit on this session: mark logged-out so | ||
| // outgoing sends bail fast instead of being interpreted as abuse | ||
| // while we wait for the (likely-imminent) reconnect. | ||
| warn!( | ||
| "Got 429 stream error (rate limited). Will auto-reconnect with extended backoff." | ||
| ); | ||
| self.is_logged_in.store(false, Ordering::Relaxed); | ||
| self.auto_reconnect_errors.fetch_add(5, Ordering::Relaxed); | ||
| } | ||
| "503" => { | ||
| // Server is going down/restarting: mark logged-out so sends fail | ||
| // fast against the soon-to-die socket. Auto-reconnect handles recovery. | ||
| info!("Got 503 service unavailable, will auto-reconnect."); | ||
| self.is_logged_in.store(false, Ordering::Relaxed); |
There was a problem hiding this comment.
Keep 429 and 503 visible via Event::StreamError.
These branches changed the teardown behavior, but they also stopped surfacing a structured stream-error event. That makes rate-limit and service-unavailable rejections invisible to consumers exactly in the cases where the transport is intentionally left alive for the socket layer to finish the shutdown.
Suggested fix
"429" => {
// Server signalled rate-limit on this session: mark logged-out so
// outgoing sends bail fast instead of being interpreted as abuse
// while we wait for the (likely-imminent) reconnect.
warn!(
"Got 429 stream error (rate limited). Will auto-reconnect with extended backoff."
);
self.is_logged_in.store(false, Ordering::Relaxed);
+ self.core.event_bus.dispatch(Event::StreamError(
+ crate::types::events::StreamError {
+ code: code.to_string(),
+ raw: Some(node.to_owned()),
+ },
+ ));
self.auto_reconnect_errors.fetch_add(5, Ordering::Relaxed);
}
"503" => {
// Server is going down/restarting: mark logged-out so sends fail
// fast against the soon-to-die socket. Auto-reconnect handles recovery.
info!("Got 503 service unavailable, will auto-reconnect.");
self.is_logged_in.store(false, Ordering::Relaxed);
+ self.core.event_bus.dispatch(Event::StreamError(
+ crate::types::events::StreamError {
+ code: code.to_string(),
+ raw: Some(node.to_owned()),
+ },
+ ));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "429" => { | |
| // Server signalled rate-limit on this session: mark logged-out so | |
| // outgoing sends bail fast instead of being interpreted as abuse | |
| // while we wait for the (likely-imminent) reconnect. | |
| warn!( | |
| "Got 429 stream error (rate limited). Will auto-reconnect with extended backoff." | |
| ); | |
| self.is_logged_in.store(false, Ordering::Relaxed); | |
| self.auto_reconnect_errors.fetch_add(5, Ordering::Relaxed); | |
| } | |
| "503" => { | |
| // Server is going down/restarting: mark logged-out so sends fail | |
| // fast against the soon-to-die socket. Auto-reconnect handles recovery. | |
| info!("Got 503 service unavailable, will auto-reconnect."); | |
| self.is_logged_in.store(false, Ordering::Relaxed); | |
| "429" => { | |
| // Server signalled rate-limit on this session: mark logged-out so | |
| // outgoing sends bail fast instead of being interpreted as abuse | |
| // while we wait for the (likely-imminent) reconnect. | |
| warn!( | |
| "Got 429 stream error (rate limited). Will auto-reconnect with extended backoff." | |
| ); | |
| self.is_logged_in.store(false, Ordering::Relaxed); | |
| self.core.event_bus.dispatch(Event::StreamError( | |
| crate::types::events::StreamError { | |
| code: code.to_string(), | |
| raw: Some(node.to_owned()), | |
| }, | |
| )); | |
| self.auto_reconnect_errors.fetch_add(5, Ordering::Relaxed); | |
| } | |
| "503" => { | |
| // Server is going down/restarting: mark logged-out so sends fail | |
| // fast against the soon-to-die socket. Auto-reconnect handles recovery. | |
| info!("Got 503 service unavailable, will auto-reconnect."); | |
| self.is_logged_in.store(false, Ordering::Relaxed); | |
| self.core.event_bus.dispatch(Event::StreamError( | |
| crate::types::events::StreamError { | |
| code: code.to_string(), | |
| raw: Some(node.to_owned()), | |
| }, | |
| )); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/client.rs` around lines 3307 - 3321, The "429" and "503" match arms
currently change teardown flags (self.is_logged_in, self.auto_reconnect_errors)
but no longer emit the structured Event::StreamError, making these conditions
invisible to consumers; update the "429" and "503" branches inside the stream
error match to construct and send/publish an Event::StreamError (including the
status code string and a brief message) to the same event sink/dispatcher used
elsewhere before or immediately after changing self.is_logged_in and
auto_reconnect_errors so callers receive the stream error while the socket
teardown behavior remains unchanged (refer to Event::StreamError, the
"429"/"503" match arms, self.is_logged_in.store, and
self.auto_reconnect_errors.fetch_add).
Collaborator
|
Thanks |
oonid
pushed a commit
to oonid/whatsapp-rust-sqlx
that referenced
this pull request
May 25, 2026
…oxidezap#633) Co-authored-by: João Lucas <jlucaso@hotmail.com>
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
2 participants
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.
Fix: stop spurious stream disconnects on
<stream:error><ack/>notificationsSummary
Two independent fixes — one in the ACK encoder, one in the stream-error handler — that together stop the reconnect storm users have been seeing under the JS bridge as
DisconnectReason.badSession (500).encode_ack_bytes) now copies therecipientattribute from the incoming stanza onto the outgoing<ack/>, matching whatsmeow. Without it, server cannot route the ACK back to the originating companion/device and replies with<stream:error><ack/>…</stream:error>.handle_stream_errorcatch-all) no longer treats unknown / code-less stream errors as fatal. It logs + dispatches theStreamErrorevent and leaves the connection alone, exactly like whatsmeow's default branch (connectionevents.go:66-69). The socket layer remains responsible for detecting a real teardown.Either fix in isolation reduces the symptom; together they remove both the trigger and the over-reaction.
Problem
A bot running this stack repeatedly reconnects with no actionable error. The JS bridge surfaces it as
badSession (500), but the server never sends<stream:error code="500">— that "500" is a bridge-side fallback forexpected_disconnectwithout a recognised code.Across 19 captured Rust debug logs (7 distinct accounts, ~2-hour window), every disconnect matched this exact shape:
The
<ack>inside the<stream:error>is the server's reference to the message stanza whose ACK it rejected. Immediately after reconnect, that same message is redelivered (offline="10",offline="12"), confirming the previous ACK never registered.Affected stanzas were a mix of:
recipient="…@lid"andpeer_recipient_pn="…".addressing_mode="lid") routed through hosted recipients.<enc decrypt-fail="hide">+<meta tag_reason="rereg_recovery_request">).Common denominator: they are stanzas where WhatsApp Web / whatsmeow propagate
recipientinto the ACK. Rust drops it.Fix A — copy
recipientin the ACK encoderRoot cause (Fix A)
whatsmeow/receipt.go:143-177(Client.sendAck):The server uses
recipientto dispatch the ACK to the originating device. Without it the ACK is unroutable; the server signals that by closing the stream with<stream:error><ack …/></stream:error>. TheBotJIDMapremap is for Meta's BotServer and has no analogue inwhatsapp-rust, so we intentionally do not port it.Before
whatsapp-rust/src/client.rs::encode_ack_bytes(production path):After
The
#[cfg(test)]mirrorbuild_ack_nodegets the same shape so structural assertions stay possible without touching the wire encoder.Fix B — don't proactively disconnect on unknown stream errors
Root cause (Fix B)
whatsmeow/connectionevents.go:19-69deliberately ignores stream errors it doesn't recognise:default: cli.Log.Errorf("Unknown stream error: %s", node.XMLString()) go cli.dispatchEvent(&events.StreamError{Code: code, Raw: node}) }There is no
expectDisconnect(), no socket close, no reconnect trigger. The design relies on the read loop noticing if the server actually ends the stream (viaxmlstreamendor a closed socket).whatsapp-rustdiverges: its catch-all setsexpected_disconnect = trueand the function unconditionally callsnotify_connection_shutdown()for every branch, including429,503, and unknown. That converts an informational stanza into a full shutdown + reconnect cycle, which under high traffic snowballs into the loop the logs show.Before
whatsapp-rust/src/client.rs::handle_stream_error(excerpt):After
Two concrete behavior changes:
expected_disconnect. If the server really closes the stream afterwards, the existing socket-level path handles it; if it doesn't, the connection lives.notify_connection_shutdown()(and the matchinginfo!log) is now inside theshould_disconnectblock. The implicit side effect —429and503were also triggering shutdown — is gone.This brings Rust's behavior matrix in line with whatsmeow:
code="515"code="516"/"401"/"409"/ conflictcode="429"code="503"<ack/>body)Tests
Five new unit tests in
src/client.rs(existing test module).Encoder (Fix A)
test_build_ack_node_for_message_with_recipient_preserves_recipient— message stanza withrecipient="146991363395800@lid"produces an ACK that echoes the same value.test_build_ack_node_for_receipt_with_recipient_preserves_recipient— same for<receipt>.test_build_ack_node_for_message_without_recipient_omits_recipient— regression guard: never synthesise an emptyrecipient.Handler (Fix B)
test_stream_error_unknown_keeps_connection_alive— bare<stream:error/>(nocode, no body) leaves bothexpected_disconnectandenable_auto_reconnectuntouched.test_stream_error_ack_shaped_does_not_force_shutdown— the exact shape seen in production (<stream:error><ack class="message" type="text" id="…"/></stream:error>) does not flipexpected_disconnect.All fictitious JIDs, per
AGENTS.md("No real PII in tests").Local results:
Workspace-wide (
cargo test --workspace --exclude e2e-tests) passes 511 + sub-crate tests with no regressions.e2e-testsis skipped because it requires the mock server (perAGENTS.md).Alignment with
whatsapp-rust/AGENTS.mdreceipt.go:143-177for A,connectionevents.go:19-69for B). Each change cites its whatsmeow line range in code comments.if let.if let Some(_) = …single-bind pattern already present; no nestedif letintroduced.146991363395800@lid,155500012345:48@s.whatsapp.net, etc.).DeviceCommand+PersistenceManager::process_command().thiserrorfor typed errors, no.unwrap()outside tests.Resultpaths; no new.unwrap()introduced outside tests.cargo fmt --all,cargo clippy --all --tests,cargo test --all.For reference:
NackInvalidHostedCompanionStanza = 493already exists inwacore/src/protocol/nack.rs. That nack family is exactly what the server is signalling with the ack-shaped stream error before this fix — surfaced at the stream layer instead of as a nack.