fix!: typed read-loop exit — routine server recycles are not errors; Disconnected carries the reason

[jlucaso1]

Problem

A routine server-initiated stream recycle (clean EOF — the normal WhatsApp reconnect path) surfaced as an ERROR in error trackers: read_messages_loop returned Err even for the case its own body classifies as info!, and #[instrument(err(Debug))] defaults to ERROR regardless of that classification. The Disconnected event also carried no reason, so consumers couldn't tell a recycle from a real failure without parsing logs.

Changes

• read_messages_loop returns Result<ReadLoopExit, ReadLoopError>: clean recycle is Ok(ServerRecycle(reason)); Err is reserved for genuine failures (startup, dirty transport error, lost event channel). The span's err(Debug) stays at ERROR and is now correct by construction — the routine/anomalous distinction lives once, in the type, instead of being re-derived per consumer (logs, tracing, error trackers, embedders without tracing).
• connect_graph's err() drops to warn: run() already classifies its failures (debug! for a transient handshake retry, error! otherwise) — the default ERROR double-reported the real case and promoted every transient retry to an issue.
• BREAKING: events::Disconnected now carries reason: DisconnectReason (Serialize, snake_case). DisconnectReason is re-exported from transport alongside TransportEvent.
• wa.conn.read_loop / wa.conn.connect spans tag lid/pn via record_identity_on_span, same as wa.iq / wa.send.message / wa.conn.run (tracing: tag wa.iq / wa.send.message / wa.conn.run spans with account identity #951).

Test plan

• cargo check --workspace --all-targets
• cargo clippy -p whatsapp-rust --features tracing --all-targets
• cargo test -p whatsapp-rust --features tracing --lib (879) / cargo test -p wacore --lib (1039)

[coderabbitai]

Warning

Review limit reached

@jlucaso1, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 17 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Repository UI (base), Organization UI (inherited)

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 80607f5d-b4be-4f86-86e1-55abb3673117

📥 Commits

Reviewing files that changed from the base of the PR and between cc6013f and c892c9e.

📒 Files selected for processing (2)

• src/client/lifecycle.rs
• src/client/node_io.rs

📝 Walkthrough

Walkthrough

I want to be direct: this changes how disconnects get classified and reported, and now we know why a connection dropped, not just that it dropped. ReadLoopExit/ReadLoopError replace generic errors, DisconnectReason is serialized, and Event::Disconnected carries the reason through Client::run. Connect span tracing is also tuned to avoid noisy false-positive error reports.

Changes

Disconnect Reason Propagation

Layer / File(s)	Summary
Serializable DisconnectReason and Disconnected payload wacore/src/net.rs, wacore/src/types/events.rs, src/transport.rs	DisconnectReason derives Serialize with snake_case naming, Disconnected struct gains a reason field, and DisconnectReason is re-exported from transport.rs.
Typed read loop outcomes src/client/node_io.rs	ReadLoopExit and ReadLoopError enums replace anyhow::Error; startup, shutdown, expected-disconnect, transport-disconnect, and channel-closed paths all return typed results, with into_reason() mapping errors to reasons.
Run dispatch and connect span tuning src/client/lifecycle.rs	Client::run derives an optional reason from the read loop result and dispatches Event::Disconnected with it after cleanup; connect_graph's tracing instrument now explicitly sets warn-level error classification to avoid spurious error reports on transient handshake retries.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

• oxidezap/whatsapp-rust#485: Refactors the same read-loop disconnect expectation handling this PR types with ReadLoopExit/ReadLoopError.
• oxidezap/whatsapp-rust#663: Also propagates a structured DisconnectReason through TransportEvent::Disconnected matching logic.
• oxidezap/whatsapp-rust#785: Touches the same lifecycle.rs/node_io.rs disconnect classification code paths.

Suggested labels: breaking-change, api-design

Look, I need this to be bulletproof before it ships — every disconnect needs a reason attached, no excuses, and the tracing noise needs to actually go away. That's it. Ship it when it's right.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: typed read-loop exits and reason-carrying disconnects.
Description check	✅ Passed	The description matches the PR's changes to read-loop typing, tracing severity, and disconnect reasons.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/read-loop-typed-exit-disconnect-reason

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[greptile-apps]

Greptile Summary

This PR refactors the read-loop exit taxonomy so that routine server-initiated stream recycles surface as Ok(ServerRecycle(reason)) instead of Err(...), meaning the tracing span's err(Debug) capture and error trackers now fire only for genuine failures. It also fixes a stale-flag bug where intentional_reconnect was only consumed on error exits, and adds reason: DisconnectReason to the Disconnected event as a breaking change.

• ReadLoopExit / ReadLoopError cleanly partition outcomes; ReadLoopError::into_reason() maps failures without an explicit reason to DisconnectReason::Unknown.
• intentional_reconnect is now unconditionally swapped on every exit (before the match), closing a bug where an Ok(Expected) exit left the flag set and silently suppressed the next connection's Disconnected event.
• connect_graph span's err(level = \"warn\") prevents transient handshake retries from being double-reported as errors, and record_identity_on_span is added consistently to both the read-loop and connect spans.

Confidence Score: 5/5

Safe to merge — the changes are well-reasoned, all exit paths are correctly classified, and the stale-flag bug fix is sound.

The type-level redesign correctly partitions all exit conditions. The unconditional consumption of intentional_reconnect before the match closes a real bug. The Disconnected breaking change is minimal and well-contained. No reconnect timing or error-counter behavior is regressed relative to the prior code.

No files require special attention.

Important Files Changed

Filename	Overview
src/client/lifecycle.rs	Consumes intentional_reconnect unconditionally before the match (bug fix), maps all three exit arms to Option<DisconnectReason>, and dispatches Disconnected with reason. Logic is correct and the stale-flag fix is well-motivated.
src/client/node_io.rs	Introduces ReadLoopExit and ReadLoopError enums; read_messages_loop return type changed from Result<(), anyhow::Error> to Result<ReadLoopExit, ReadLoopError>. Clean shutdown correctly returns Ok(ServerRecycle), dirty transport error returns Err(Transport(...)), all other expected exits return Ok(Expected).
src/transport.rs	Re-exports DisconnectReason alongside TransportEvent; minimal one-line change.
wacore/src/net.rs	Adds serde::Serialize + #[serde(rename_all = "snake_case")] to DisconnectReason. Tests for is_clean_shutdown remain unchanged. Serialization shape is correct.
wacore/src/types/events.rs	Adds reason: DisconnectReason to Disconnected, a documented breaking change. Field is pub and serializable, consistent with the rest of the event types.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[read_messages_loop] --> B{Exit condition}
    B -->|Shutdown signal| C["Ok(ReadLoopExit::Expected)"]
    B -->|expected_disconnect during frame| C
    B -->|expected_disconnect during Disconnected event| C
    B -->|Server close — is_clean_shutdown| D["Ok(ReadLoopExit::ServerRecycle(reason))"]
    B -->|Transport error — not clean| E["Err(ReadLoopError::Transport(reason))"]
    B -->|Event channel closed unexpectedly| F["Err(ReadLoopError::ChannelClosed)"]
    B -->|No noise socket / not connected| G["Err(ReadLoopError::NotStarted)"]

    C --> H{lifecycle.rs match}
    D --> H
    E --> H
    F --> H
    G --> H

    H -->|Expected| I[None — no Disconnected event]
    H -->|ServerRecycle + expected/intentional| I
    H -->|ServerRecycle + unexpected| J["Some(reason) → Disconnected event dispatched"]
    H -->|Err + expected/intentional| I
    H -->|Err + unexpected| K["Some(e.into_reason()) → Disconnected event dispatched"]

    J --> L[auto_reconnect_errors++ → fibonacci backoff]
    K --> L
    I --> M{expected_disconnect flag?}
    M -->|true| N[reset error counter → immediate reconnect]
    M -->|false| L

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[read_messages_loop] --> B{Exit condition}
    B -->|Shutdown signal| C["Ok(ReadLoopExit::Expected)"]
    B -->|expected_disconnect during frame| C
    B -->|expected_disconnect during Disconnected event| C
    B -->|Server close — is_clean_shutdown| D["Ok(ReadLoopExit::ServerRecycle(reason))"]
    B -->|Transport error — not clean| E["Err(ReadLoopError::Transport(reason))"]
    B -->|Event channel closed unexpectedly| F["Err(ReadLoopError::ChannelClosed)"]
    B -->|No noise socket / not connected| G["Err(ReadLoopError::NotStarted)"]

    C --> H{lifecycle.rs match}
    D --> H
    E --> H
    F --> H
    G --> H

    H -->|Expected| I[None — no Disconnected event]
    H -->|ServerRecycle + expected/intentional| I
    H -->|ServerRecycle + unexpected| J["Some(reason) → Disconnected event dispatched"]
    H -->|Err + expected/intentional| I
    H -->|Err + unexpected| K["Some(e.into_reason()) → Disconnected event dispatched"]

    J --> L[auto_reconnect_errors++ → fibonacci backoff]
    K --> L
    I --> M{expected_disconnect flag?}
    M -->|true| N[reset error counter → immediate reconnect]
    M -->|false| L

Loading

_{Reviews (2): Last reviewed commit: "fix: consume intentional_reconnect on ev..." | Re-trigger Greptile}

[coderabbitai]

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/lifecycle.rs`:
- Around line 343-385: The graceful-exit branch in the disconnect handling logic
leaves `intentional_reconnect` set, which can incorrectly suppress later
`Disconnected` events. Update the `match loop_result` handling in
`src/client/lifecycle.rs` so the `ReadLoopExit::Expected` path also clears or
consumes `intentional_reconnect` just like the `ServerRecycle` and `Err` paths
do, ensuring stale reconnect state does not leak into the next connection cycle.

🪄 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 (base), Organization UI (inherited)

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: cfb09b94-d07c-46db-bd0f-0d3f5cae00f8

📥 Commits

Reviewing files that changed from the base of the PR and between 053b3ab and cc6013f.

📒 Files selected for processing (5)

• src/client/lifecycle.rs
• src/client/node_io.rs
• src/transport.rs
• wacore/src/net.rs
• wacore/src/types/events.rs

[cubic-dev-ai]

2 issues found across 5 files

Confidence score: 3/5

• In src/client/lifecycle.rs, reconnect() can leave intentional_reconnect set when the read loop exits with ReadLoopExit::Expected, which can cause a later real disconnect to be misclassified and skip a Disconnected event; that risks missed client state transitions after a genuine drop — clear/reset the flag on expected exit paths before merging.
• In src/client/node_io.rs, the ReadLoopExit/ReadLoopError doc comments include PR-history context instead of project-preferred rationale-focused docs, which is low runtime risk but may make future maintenance harder — trim comments to the enduring "why" per AGENTS.md before or shortly after merge.

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[codspeed-hq]

Merging this PR will not alter performance

✅ 188 untouched benchmarks

---

_{Comparing fix/read-loop-typed-exit-disconnect-reason (c892c9e) with main (053b3ab)}

[github-actions]

📦 Binary size report

Metric	main	PR	Δ
bin size (stripped)	10.62 MiB	10.62 MiB	+1.41 KiB (+0.01%) 🔺
bin .text	8.65 MiB	8.65 MiB	+1.38 KiB (+0.02%) 🔺
bin allocated (text+data+bss)	10.62 MiB	10.62 MiB	+3.97 KiB (+0.04%) 🔺
llvm-lines wacore	498,531	498,549	+18 (+0.00%) 🔺
llvm-lines wacore copies	17,059	17,061	+2 (+0.01%) 🔺
llvm-lines whatsapp-rust lib	714,603	714,987	+384 (+0.05%) 🔺
llvm-lines whatsapp-rust lib copies	23,204	23,210	+6 (+0.03%) 🔺
deps crates (Cargo.lock)	466	466	0

.text per crate

Crate	main	PR	Δ
.text whatsapp_rust	1.54 MiB	1.54 MiB	+1.18 KiB (+0.07%) 🔺
.text wacore	533.60 KiB	533.60 KiB	0
.text wacore_binary	157.67 KiB	157.67 KiB	0
.text wacore_libsignal	174.81 KiB	174.81 KiB	0
.text wacore_appstate	156.10 KiB	156.10 KiB	0
.text wacore_noise	26.05 KiB	26.05 KiB	0
.text waproto	1.60 MiB	1.60 MiB	0
.text whatsapp_rust_sqlite_storage	481.73 KiB	481.73 KiB	0
.text whatsapp_rust_tokio_transport	43.56 KiB	43.46 KiB	-98 B (-0.22%) 🔽
.text whatsapp_rust_ureq_http_client	8.95 KiB	9.05 KiB	+98 B (+1.07%) ⚠️
.text std	1007.29 KiB	1007.46 KiB	+172 B (+0.02%) 🔺
.text other deps	2.94 MiB	2.94 MiB	0

Top movers (cargo-bloat attribution)

Crate	main	PR	Δ
whatsapp_rust	1.54 MiB	1.54 MiB	+1.18 KiB (+0.07%)

Baseline: 053b3abfd (latest main run) · Head: cedb626e1 · Graphs

[cubic-dev-ai]

0 issues found across 2 files (changes from recent commits).

<sub>Requires human review: This PR restructures the read loop's error handling and adds a typed return enum, changing the Disconnected event from a unit struct to one with a reason field. These are core logic changes with moderate blast radius; they require a human reviewer to validate correctness and backward compatibility.

Re-trigger cubic</sub>