Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions src/client/lifecycle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,8 @@ impl Client {
}
} else {
wacore::telemetry::connect("ok");
let unexpected_disconnect = if self.read_messages_loop().await.is_err() {
let loop_result = self.read_messages_loop().await;
let unexpected_disconnect = if let Err(e) = loop_result {
// Check intentional_reconnect AFTER read loop exits — reconnect()
// sets this flag while the loop is running, so it must be read here.
if self.expected_disconnect.load(Ordering::Relaxed)
Expand All @@ -298,9 +299,12 @@ impl Client {
debug!("Message loop exited during expected disconnect.");
false
} else {
warn!(
"Message loop exited with an error. Will attempt to reconnect if enabled."
);
// read_messages_loop already logged the cause at the right level
// (info for a clean server recycle, warn for a real transport
// error), so keep this at debug to avoid re-flagging a benign
// reconnect as an error. Still treated as an unexpected
// disconnect for the event dispatch + reconnect below.
debug!("Message loop exited, will reconnect if enabled: {e:#}");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep unclassified read-loop exits visible

When the transport event channel closes without delivering a Disconnected event, read_messages_loop takes the Err(_) branch in src/client/node_io.rs and only emits a debug message before returning Err("Transport event channel closed"). This new generic debug log is therefore the only record of that unexpected disconnect, so custom/buggy transports that drop the sender without a reason will reconnect and dispatch Disconnected without any warn/error even though the cause was not classified as clean.

Useful? React with 👍 / 👎.

true
}
} else if self.expected_disconnect.load(Ordering::Relaxed) {
Expand Down
13 changes: 11 additions & 2 deletions src/client/node_io.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,14 @@ impl Client {
},
Ok(crate::transport::TransportEvent::Disconnected(reason)) => {
if !self.expected_disconnect.load(Ordering::Relaxed) {
debug!("Transport disconnected unexpectedly: {reason}");
// Classify the level: a routine server recycle (clean EOF /
// normal close) is logged quietly, but a real transport error
// stays at WARN so it's never hidden behind reconnect noise.
if reason.is_clean_shutdown() {
info!("Connection closed by server ({reason}); reconnecting.");
} else {
warn!("Transport disconnected: {reason}; reconnecting.");
}
return Err(anyhow::anyhow!("Transport disconnected: {reason}"));
} else {
debug!("Transport disconnected as expected: {reason}");
Expand Down Expand Up @@ -330,7 +337,9 @@ impl Client {
if self.expected_disconnect.load(Ordering::Relaxed) {
debug!("Received <xmlstreamend/>, expected disconnect.");
} else {
warn!("Received <xmlstreamend/>, treating as disconnect.");
// A bare <xmlstreamend/> is the server cleanly ending the stream
// (a recycle). We reconnect, so this is routine, not an error.
info!("Received <xmlstreamend/> (server stream end); reconnecting.");
}
self.notify_connection_shutdown();
return;
Expand Down
14 changes: 13 additions & 1 deletion src/keepalive.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,19 @@ impl Client {
}
Err(e) => {
let result = classify_keepalive_error(&e);
warn!(target: "Client/Keepalive", "Keepalive ping failed: {e:?}");
// A fatal-classified error means the connection is already gone
// (e.g. NotConnected): the disconnect is being handled elsewhere, so
// this ping failure is benign teardown collateral, not a keepalive
// problem. A transient failure (timeout, unexpected server response)
// is a real keepalive issue and stays loud.
match result {
KeepaliveResult::FatalFailure => {
debug!(target: "Client/Keepalive", "Keepalive skipped, connection already closing: {e:?}");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep real keepalive send failures at warn

This debug branch now covers every FatalFailure, but classify_keepalive_error maps more than teardown cases to fatal, including IqError::Socket(_), EncryptSend(_), ClientState(_), InternalChannelClosed, and EncodeError(_). If a keepalive ping fails because the socket/send pipeline breaks while the client still thinks it is connected, this becomes the only keepalive log before the loop exits, so a real connection/send failure is hidden at debug instead of staying loud.

Useful? React with 👍 / 👎.

}
KeepaliveResult::TransientFailure | KeepaliveResult::Ok => {
warn!(target: "Client/Keepalive", "Keepalive ping failed: {e:?}");
}
}
result
}
}
Expand Down
76 changes: 76 additions & 0 deletions wacore/src/net.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,30 @@ impl std::fmt::Display for DisconnectReason {
}
}

impl DisconnectReason {
/// Whether this is a benign, server-initiated stream recycle (the normal
/// WhatsApp reconnect path) rather than a transport-level error.
///
/// Used only to pick a log level: a clean shutdown is logged quietly (the
/// reconnect is routine), while everything else stays loud so a genuine
/// transport failure is never hidden behind reconnect noise. Deliberately
/// conservative — anything ambiguous returns `false` (stays loud): a read/IO
/// error, an abnormal close code, or an unreported reason.
pub fn is_clean_shutdown(&self) -> bool {
match self {
// EOF with no Close frame is how the WA server recycles a connection.
Self::StreamEnded => true,
// A Close frame with a normal / going-away / no code is graceful; any
// other code (protocol/server error, restart, etc.) stays loud.
Self::ServerClose { code, .. } => matches!(code, None | Some(1000) | Some(1001)),
// A transport read/IO error is a real failure — never quiet.
Self::ReadError(_) => false,
// Unknown reason: stay loud, don't assume it was benign.
Self::Unknown => false,
}
}
}

/// An event produced by the transport layer.
#[derive(Debug, Clone)]
pub enum TransportEvent {
Expand Down Expand Up @@ -176,3 +200,55 @@ pub trait HttpClient: Send + Sync {
))
}
}

#[cfg(test)]
mod tests {
use super::DisconnectReason;

// Happy paths: benign server-initiated recycles must classify as clean so
// their reconnect is logged quietly.
#[test]
fn clean_shutdowns_are_classified_clean() {
assert!(DisconnectReason::StreamEnded.is_clean_shutdown());
assert!(
DisconnectReason::ServerClose {
code: Some(1000),
reason: String::new()
}
.is_clean_shutdown()
);
assert!(
DisconnectReason::ServerClose {
code: Some(1001),
reason: "going away".to_string()
}
.is_clean_shutdown()
);
assert!(
DisconnectReason::ServerClose {
code: None,
reason: String::new()
}
.is_clean_shutdown()
);
}

// Bad paths: a real transport error, an abnormal close code, or an unreported
// reason must NOT be classified clean — they have to stay loud so genuine
// failures are never hidden behind reconnect noise.
#[test]
fn real_errors_are_never_classified_clean() {
assert!(!DisconnectReason::ReadError("connection reset".to_string()).is_clean_shutdown());
assert!(!DisconnectReason::Unknown.is_clean_shutdown());
for code in [1002u16, 1006, 1011, 1012, 1013, 3000, 4000] {
assert!(
!DisconnectReason::ServerClose {
code: Some(code),
reason: String::new()
}
.is_clean_shutdown(),
"close code {code} must not be treated as a clean shutdown"
);
}
}
}
Loading