fix(client): make it plain which call reads the socket - #1258
Conversation
`Client::connect` established the connection and returned `()`. Nothing read it: `read_messages_loop` runs inside `run`, so a consumer that connected and then waited for events waited on a socket whose frames sat in the transport channel with no reader. The failure mode is silence, because the channel buffers: no timeout, no error and no log to follow. The docs pointed nowhere either. `connect`'s doc covered only the boxing of its future, and `run` had no item doc at all. `connect` now hands back a `#[must_use]` `Connection` that has to be driven with `read_until_disconnected`, so the compiler says at the call site what used to cost hours of tracing. Driving it reads exactly one connection: the read loop, the teardown and the `Disconnected` dispatch that `run`'s loop body already performed, now shared with it so the two cannot drift. Connecting without a reconnect loop stays possible and gains a reader of its own, which claims `is_running` for its duration because that flag is how the rest of the client asks whether anything is reading. A connection dropped undriven warns, since nothing legitimate does that. The connection borrows the client, so the connect path keeps its size and its allocations.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesConnection lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Client
participant Connection
participant Transport
participant EventDispatcher
Client->>Transport: complete handshake
Transport-->>Client: established transport
Client-->>Connection: return connection handle
Client->>Connection: read_until_disconnected
Connection->>Transport: read and decode frames
Transport-->>Connection: disconnect reason
Connection->>EventDispatcher: dispatch Disconnected event
Connection-->>Client: return disconnect result
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
| Filename | Overview |
|---|---|
| src/client/lifecycle.rs | Introduces the must-use connection handle, shared connection-driving lifecycle, shutdown-race retraction, keepalive relocation, and regression coverage without leaving an eligible follow-up defect. |
| src/client.rs | Exports Connection and adds the non-exhaustive shutdown connection error. |
| src/lib.rs | Re-exports the new public Connection type from the crate root and prelude. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Client::connect] --> B[Version fetch and transport setup]
B --> C[Noise handshake]
C --> D{Shutdown observed?}
D -- Yes --> E[Retract transport resources]
E --> F[Return ConnectError::Shutdown]
D -- No --> G[Return must-use Connection]
G --> H[Connection::read_until_disconnected]
H --> I[Start keepalive]
I --> J[Read and decode frames]
J --> K[Clean up connection state]
K --> L[Dispatch unexpected disconnect]
M[Client::run] --> A
L --> M
Reviews (7): Last reviewed commit: "fix(client): retract a connection publis..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
`read_until_disconnected` cleared `is_running` after its await, so a caller that dropped the future (a wrapping timeout, an aborted task) left the flag standing. The client would then advertise a reader that no longer exists: `run()` refuses to start as already running, and `can_reach_server` admits requests nothing will ever decode the answers to. A scope guard restores it on every exit, cancellation included.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Confidence score: 3/5
- In
src/client/lifecycle.rs,drive_connection().awaitlacks a cancellation guard, so a cancelled directly-driven read can skipcleanup_connectionand leave lifecycle state/resources partially torn down, which risks stale connection behavior on subsequent operations — add a guard that guarantees teardown runs on cancellation (not justis_runningrestoration).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/client/lifecycle.rs">
<violation number="1" location="src/client/lifecycle.rs:1520">
P2: The guard correctly restores `is_running` when a directly-driven read is cancelled, but it only restores that one flag. `drive_connection().await` is awaited without a cancellation guard, so its teardown (`cleanup_connection_state`, which clears `is_connected` and releases the transport/socket) never runs on the drop/cancel path — the exact path this change targets. Since `Connection` is consumed by value, a cancelled read cannot be re-driven, and the next `connect()` returns `AlreadyConnected` because `is_connected` is still true, leaving `run()` looping on that error and the socket leaked. Consider wrapping the drive so teardown also happens when the read is abandoned (e.g. hoisting the cleanup/flag restoration into a `scopeguard` around the whole await, or calling `cleanup_connection_state` from a drop-based guard so both `is_running` and connection state are released together).</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| this.client.stop_supervision_loop(); | ||
| }) | ||
| }); | ||
| this.client.drive_connection().await |
There was a problem hiding this comment.
P2: The guard correctly restores is_running when a directly-driven read is cancelled, but it only restores that one flag. drive_connection().await is awaited without a cancellation guard, so its teardown (cleanup_connection_state, which clears is_connected and releases the transport/socket) never runs on the drop/cancel path — the exact path this change targets. Since Connection is consumed by value, a cancelled read cannot be re-driven, and the next connect() returns AlreadyConnected because is_connected is still true, leaving run() looping on that error and the socket leaked. Consider wrapping the drive so teardown also happens when the read is abandoned (e.g. hoisting the cleanup/flag restoration into a scopeguard around the whole await, or calling cleanup_connection_state from a drop-based guard so both is_running and connection state are released together).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/client/lifecycle.rs, line 1520:
<comment>The guard correctly restores `is_running` when a directly-driven read is cancelled, but it only restores that one flag. `drive_connection().await` is awaited without a cancellation guard, so its teardown (`cleanup_connection_state`, which clears `is_connected` and releases the transport/socket) never runs on the drop/cancel path — the exact path this change targets. Since `Connection` is consumed by value, a cancelled read cannot be re-driven, and the next `connect()` returns `AlreadyConnected` because `is_connected` is still true, leaving `run()` looping on that error and the socket leaked. Consider wrapping the drive so teardown also happens when the read is abandoned (e.g. hoisting the cleanup/flag restoration into a `scopeguard` around the whole await, or calling `cleanup_connection_state` from a drop-based guard so both `is_running` and connection state are released together).</comment>
<file context>
@@ -1508,12 +1508,16 @@ impl Connection<'_> {
+ this.client.stop_supervision_loop();
+ })
+ });
+ this.client.drive_connection().await
}
}
</file context>
The teardown lives after an await, so an abandoned read leaves the socket open and the client connected, the same way an abandoned `run` does. Only the reader flag comes back on its own; state the rest instead of letting a caller find out.
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/lifecycle.rs`:
- Around line 1508-1527: Preserve the single-reader contract in
read_until_disconnected by refusing to call drive_connection when
client.is_running is already set. Make the ownership claim atomic and return the
existing failure result for a second reader; only the caller that successfully
claims the reader may install the release guard and drive the connection.
🪄 Autofix
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: 8c35f747-6daf-4e8a-ad72-606b158d2316
📒 Files selected for processing (3)
src/client.rssrc/client/lifecycle.rssrc/lib.rs
| pub async fn read_until_disconnected(self) -> Option<DisconnectReason> { | ||
| // Reading is what the Drop warning asks for, so the drop that ends this | ||
| // call must not fire it. | ||
| let this = std::mem::ManuallyDrop::new(self); | ||
| // `is_running` is how the rest of the client asks whether anything is | ||
| // reading (`can_reach_server`, `is_terminal`). Left false, a request | ||
| // sent over this connection would be refused for want of the reader | ||
| // that is right here. `run` sets it before connecting and owns clearing | ||
| // it, so only a directly driven connection claims it. | ||
| // Guarded, not a store after the await: a caller that drops this future | ||
| // (a wrapping timeout, an aborted task) stops reading too, and a flag | ||
| // left standing would advertise a reader that no longer exists. | ||
| let owns_reader = !this.client.is_running.swap(true, Ordering::SeqCst); | ||
| let _release_reader = owns_reader.then(|| { | ||
| scopeguard::guard((), |_| { | ||
| this.client.stop_supervision_loop(); | ||
| }) | ||
| }); | ||
| this.client.drive_connection().await | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find every caller of read_until_disconnected and drive_connection, plus every is_running mutation.
set -euo pipefail
rg -nP -C4 '\bread_until_disconnected\s*\(' --type=rust
echo '--- drive_connection ---'
rg -nP -C4 '\bdrive_connection\s*\(' --type=rust
echo '--- is_running writes ---'
rg -nP -C3 'is_running\s*\.\s*(store|swap|compare_exchange)' --type=rustRepository: oxidezap/whatsapp-rust
Length of output: 160
🏁 Script executed:
#!/bin/bash
set -u
echo "Repo files matching lifecycle.rs:"
git ls-files | grep 'lifecycle\.rs$' || true
echo "Top-level files:"
git ls-files | head -50Repository: oxidezap/whatsapp-rust
Length of output: 1529
🏁 Script executed:
#!/bin/bash
set -u
echo "Current directory:"
pwd
echo "Git status:"
git status --short
echo "Find lifecycle.rs:"
find . -type f -name lifecycle.rs 2>/dev/null
echo "Find DriveConnection/read_until symbols in tracked files:"
grep -RInE "read_until_disconnected|drive_connection|is_running|already_connected|AlreadyConnected" . --include='*.rs' 2>/dev/null | head -200Repository: oxidezap/whatsapp-rust
Length of output: 7674
🏁 Script executed:
#!/bin/bash
set -u
echo "Current directory:"
pwd
echo "Git status:"
git status --short
echo "Find lifecycle.rs:"
find . -type f -name lifecycle.rs 2>/dev/null
echo "Find symbols in tracked files:"
grep -RInE "read_until_disconnected|drive_connection|is_running|already_connected|AlreadyConnected" . --include='*.rs' 2>/dev/null | head -200Repository: oxidezap/whatsapp-rust
Length of output: 7647
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "src/client/lifecycle.rs Connection/read_until_disconnected/drive_connection lines:"
sed -n '100,115p;1448,1540p' src/client/lifecycle.rs
echo "--- run/connect context ---"
sed -n '550,620p;720,990p' src/client/lifecycle.rs
echo "--- client connect/AlreadyConnected ---"
sed -n '680,830p' src/client.rs
python3 - <<'PY'
from pathlib import Path
src = Path("src/client/lifecycle.rs").read_text()
# Read the exact read_until_disconnected body as non-mutative evidence.
for i, line in enumerate(src.splitlines(), 1):
if "pub async fn read_until_disconnected(self)" in line:
end = i + 30
print(f"--- first 22 lines after read_until_disconnected ---")
for j in range(i, min(end, len(src.splitlines())+1)):
print(f"{j}: {src.splitlines()[j-1]}")
PYRepository: oxidezap/whatsapp-rust
Length of output: 29395
Refuse a second reader or keep the single-reader contract.
read_until_disconnected calls drive_connection() even when is_running is already true, and run() drives drive_connection() while is_running is true. If connect_graph() returns a Connection, another caller can drive the same link in parallel. That can clear shared connection state and dispatch Disconnected from two readers. Keep Connection exclusive by rejecting the reader when is_running is set, or change the design so multiple readers are explicit.
🤖 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/lifecycle.rs` around lines 1508 - 1527, Preserve the single-reader
contract in read_until_disconnected by refusing to call drive_connection when
client.is_running is already set. Make the ownership claim atomic and return the
existing failure result for a second reader; only the caller that successfully
claims the reader may install the release guard and drive the connection.
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbbb38cec3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let _release_reader = owns_reader.then(|| { | ||
| scopeguard::guard((), |_| { | ||
| this.client.stop_supervision_loop(); | ||
| }) |
There was a problem hiding this comment.
When a timeout or task abort drops read_until_disconnected after read_messages_loop has taken and dropped transport_events, this guard only clears is_running; it leaves is_connected and the transport slots populated. A subsequent run() therefore passes its reader check but connect_graph() returns AlreadyConnected on every retry, while no receiver remains to process frames, so the client cannot recover without an explicit disconnect(). Cancellation needs to trigger connection teardown (or otherwise make the stale connection reconnectable), rather than merely releasing the reader flag.
Useful? React with 👍 / 👎.
| Ok(node_io::ReadLoopExit::Expected) => { | ||
| debug!("Message loop exited gracefully (expected disconnect)."); | ||
| None | ||
| } |
There was a problem hiding this comment.
When a directly driven fresh-pairing connection receives the protocol's 515, the handler sets expected_disconnect and wakes the read loop, which reaches this Expected branch. Returning None makes that server-required reconnect indistinguishable from a caller-requested shutdown, even though run() relies on the still-private expected_disconnect flag to reconnect immediately; a consumer of the new public single-connection API therefore cannot tell that pairing needs another connection and may stop with pairing/bootstrap incomplete. Return a distinct expected-reconnect outcome (or otherwise expose it) instead of reporting every expected exit as a requested end.
Useful? React with 👍 / 👎.
| this.client.stop_supervision_loop(); | ||
| }) | ||
| }); | ||
| this.client.drive_connection().await |
There was a problem hiding this comment.
After a directly driven connection ends through an internally expected transition such as the post-pairing 515, drive_connection cleans up but leaves expected_disconnect set; only run() clears it at the top of its loop. Even if a direct-API consumer independently knows to call connect() again, the replacement connection inherits that flag, so handle_success rejects its login and the read loop exits on its first decoded frame. Reset or consume the retired connection's flag before driving a subsequently established direct connection.
Useful? React with 👍 / 👎.
`expected_disconnect` is the previous connection's verdict, and only `run` cleared it, at the top of each attempt. A caller that drives connections itself had nowhere to learn of it: after an end that sets the flag (the 515 that follows pairing), its next connection came up with the flag still set, so `handle_success` refused the login and the read loop exited on the first frame it decoded. Clear it with the other per-connection state in the connect graph, where both callers pass. Also states on `read_until_disconnected` what `None` covers, since a protocol-driven end like that 515 reports no reason either, and what an abandoned read leaves behind.
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5494a0c7cc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // the read loop exit on the first frame it decodes and `handle_success` | ||
| // refuse the login. `run` already clears it before each attempt; a | ||
| // caller driving connections itself has nowhere else to learn of it. | ||
| self.expected_disconnect.store(false, Ordering::Relaxed); |
There was a problem hiding this comment.
When connect() is called after disconnect()/signal_shutdown_sync()—or races just after either sets the terminal flags—this store erases expected_disconnect even though the one-shot shutdown notifier remains fired. The handshake later resets the per-connection shutdown signal, and a directly driven Connection can process frames and satisfy can_reach_server(), effectively resurrecting a client whose terminal shutdown has already been published. Check the terminal shutdown state and reject the attempt rather than clearing its verdict.
Useful? React with 👍 / 👎.
| .detach(); | ||
|
|
||
| Ok(()) | ||
| Ok(Connection { client: self }) |
There was a problem hiding this comment.
If a caller holds the returned Connection for at least the first 15–30 second keepalive interval before polling read_until_disconnected, the keepalive task spawned just above calls send_iq while is_running is still false. send_and_wait_iq returns NotConnected, which is classified as fatal, so the keepalive loop exits permanently even after the caller starts reading; that connection then has no idle ping or dead-socket watchdog. Start keepalive when the connection is driven, or otherwise prevent the pre-reader tick from terminating it.
Useful? React with 👍 / 👎.
…with the reader Two holes the direct-connection path opened, both invisible until something actually reads that connection. Shutdown is published once and for good, and `run` already refuses to start after it. `connect` did not, so a connection driven after `disconnect` came up on a client whose application had been told it was finished. It now refuses with `ConnectError::Shutdown`. The keepalive was spawned at the end of connect, before anything reads. Its ping goes through `can_reach_server`, so a tick taken in that window is refused as NotConnected, which the loop treats as fatal and exits on: the connection then runs with no idle ping and no dead-socket watchdog. It starts where the reading starts instead, which for `run` is the same instant.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f9f1ea0c1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if self.shutdown_signal().is_fired() { | ||
| return Err(ConnectError::Shutdown); |
There was a problem hiding this comment.
When disconnect() races with an in-flight connect(), shutdown can fire immediately after this one-time check while connect_graph is awaiting version resolution, transport creation, or the handshake. Fresh evidence in the final code is that connect_graph subsequently clears expected_disconnect, resets the per-connection shutdown notifier, and can publish its transport after disconnect() has already completed cleanup, returning a live Connection from a terminally shut-down client. Recheck shutdown within the serialized attempt before publishing the socket, and tear down the newly opened transport if shutdown won the race.
Useful? React with 👍 / 👎.
…shed The refusal at the entry of `connect` cannot stand for the whole attempt: a version fetch, a transport and a handshake are awaited after it, and a shutdown landing in that window was published over, handing back a live connection on a client the application had been told was finished. Read it again before the handshake and before publishing, closing the socket opened for a client that is no longer there. The run loop reports that abandonment at debug rather than error: it is about to exit on the same shutdown, so the attempt did not fail, it was called off.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
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/lifecycle.rs`:
- Around line 905-913: Synchronize the final shutdown check with transport
publication in connect_graph, covering the awaits and slot-locking path around
the existing transport installation. Ensure signal_shutdown_sync() cannot race
past the check: if shutdown wins, disconnect the local transport and return
ConnectError::Shutdown without publishing it; otherwise publish and return the
connection. Add a deterministic test that fires shutdown after handshake
completes but before publication.
🪄 Autofix
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: 919e0f2f-6ccc-4870-8f1a-59d8c210debe
📒 Files selected for processing (2)
src/client.rssrc/client/lifecycle.rs
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
The check before the handshake still left the slot locks between it and a reachable connection, and `signal_shutdown_sync` runs no cleanup of its own, so a shutdown landing across those awaits left a socket installed on a client that had announced it was gone. Read the signal once more after the slots are filled and undo the publish if it fired; nothing has been announced at that point, so clearing the slots and closing the socket is the whole retraction. From there the connection is reachable with no await in between, so a later shutdown is an ordinary one on a live connection.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
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/lifecycle.rs`:
- Around line 956-967: The final shutdown check and connection publication in
the connection lifecycle method must be serialized through one shared
synchronous transition gate, preventing shutdown from interleaving between
shutdown_signal().is_fired() and is_connected.store(true). Reuse this gate in
terminal shutdown paths; when shutdown acquires it first, retract publication,
clear transport state, disconnect the local transport, and return
ConnectError::Shutdown. Add a deterministic test covering shutdown winning
between the final check and connection publication.
🪄 Autofix
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: 2dd43b8e-d0ce-4300-809d-4ac393a67f37
📒 Files selected for processing (1)
src/client/lifecycle.rs
| // The last word, because the slot locks above are the final awaits a | ||
| // shutdown could land across, and `signal_shutdown_sync` runs no | ||
| // cleanup of its own. Nothing has been announced yet, so undoing the | ||
| // publish here is the whole retraction. | ||
| if self.shutdown_signal().is_fired() { | ||
| let orphan = self.transport.lock().await.take(); | ||
| *self.transport_events.lock().await = None; | ||
| *self.noise_socket.lock().unwrap_or_else(|p| p.into_inner()) = None; | ||
| if let Some(orphan) = orphan { | ||
| orphan.disconnect().await; | ||
| } | ||
| return Err(ConnectError::Shutdown); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Serialize final connection publication with shutdown.
shutdown_signal().is_fired() is only a point-in-time check. If signal_shutdown_sync() runs after Line 960 returns false and before is_connected.store(true) at Line 970, this method returns Ok(Connection) after terminal shutdown.
Use one shared synchronous transition gate for the final shutdown check and connection publication. Use the same gate in terminal shutdown paths. If shutdown wins, retract and close the local transport. Add a deterministic interleaving test.
🤖 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/lifecycle.rs` around lines 956 - 967, The final shutdown check and
connection publication in the connection lifecycle method must be serialized
through one shared synchronous transition gate, preventing shutdown from
interleaving between shutdown_signal().is_fired() and is_connected.store(true).
Reuse this gate in terminal shutdown paths; when shutdown acquires it first,
retract publication, clear transport state, disconnect the local transport, and
return ConnectError::Shutdown. Add a deterministic test covering shutdown
winning between the final check and connection publication.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Confidence score: 3/5
- In
src/client/lifecycle.rs, there is a shutdown/connect race where shutdown can occur after the check but beforeis_connected.store(true), allowing a “live” connection to be published without a reader and potentially leaving it open; this risks leaked resources and inconsistent lifecycle state — serialize shutdown and connection publication (or make the state transition atomic) sosignal_shutdown_synccannot be bypassed by timing.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/client/lifecycle.rs">
<violation number="1" location="src/client/lifecycle.rs:960">
P1: A shutdown can still publish a live connection by landing after this check but before `is_connected.store(true)`, leaving the readerless connection open because `signal_shutdown_sync` only signals. Serialize shutdown with final publication (or make shutdown retract a concurrently published connection) so this check is a real publication boundary.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // shutdown could land across, and `signal_shutdown_sync` runs no | ||
| // cleanup of its own. Nothing has been announced yet, so undoing the | ||
| // publish here is the whole retraction. | ||
| if self.shutdown_signal().is_fired() { |
There was a problem hiding this comment.
P1: A shutdown can still publish a live connection by landing after this check but before is_connected.store(true), leaving the readerless connection open because signal_shutdown_sync only signals. Serialize shutdown with final publication (or make shutdown retract a concurrently published connection) so this check is a real publication boundary.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/client/lifecycle.rs, line 960:
<comment>A shutdown can still publish a live connection by landing after this check but before `is_connected.store(true)`, leaving the readerless connection open because `signal_shutdown_sync` only signals. Serialize shutdown with final publication (or make shutdown retract a concurrently published connection) so this check is a real publication boundary.</comment>
<file context>
@@ -952,6 +952,21 @@ impl Client {
+ // shutdown could land across, and `signal_shutdown_sync` runs no
+ // cleanup of its own. Nothing has been announced yet, so undoing the
+ // publish here is the whole retraction.
+ if self.shutdown_signal().is_fired() {
+ let orphan = self.transport.lock().await.take();
+ *self.transport_events.lock().await = None;
</file context>
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
2 participants
Summary
A consumer that calls
connect().awaitand then waits for events waits forever: the handshake completes, the transport receives bytes, and nothing decodes them, becauseread_messages_loopruns insiderun, not insideconnect. The transport channel buffers, so the frames just sit there with no reader; there is no timeout, no error and no log that says so, which is why this reads as a broken server and costs hours to pin down. Documenting it is not enough for a bug whose only symptom is silence, soconnectnow returns a#[must_use] Connectionthat has to be driven withread_until_disconnected, and the compiler reports the mistake at the call site instead. Driving one connection is exactly whatrun's loop body already did (read loop, teardown,Disconnecteddispatch), now extracted and shared with it, so the direct path and the reconnect loop cannot drift on what a connection ending means. Connecting without the reconnect loop stays supported and finally has a way to be read.Audit
Each of these was checked against the tree before designing anything:
connecthad a doc comment, but it only explained the boxed future (src/client/lifecycle.rs:707-710before this change). Nothing said the established connection is not read.runhad no item doc at all (src/client/lifecycle.rs:552), only the internal note about deliberately not instrumenting its span.read_messages_loopispub(crate)(src/client/node_io.rs:177) and is called from exactly one place,run's loop body (src/client/lifecycle.rs:598). A consumer could neither call it nor discover it from the public API.async_channelfilled by the transport task, and its only consumer is the read loop, so the frames simply queue. The keepalive loop was spawned byconnect, but its ping goes throughcan_reach_server, which requiresis_runningand so refuses on a client that never started a reader: even the dead-socket watchdog never fires.agent_docs/page usesconnectwithoutrun. Every entry point in the tree goes throughBot::run/Bot::spawn(README.md:57,examples/*.rs,tests/e2e/src/lib.rs), which callClient::run(src/bot.rs:546,src/bot.rs:557). The only in-tree callers ofClient::connectarerunitself and two tests (src/client/builder.rs:800,src/client/lifecycle.rs:1515), so nothing depended on connecting and not reading.is_terminalandcan_reach_serverboth reason about it), so the design had to keep it working rather than makeconnectprivate.Making that concept a first-class path turned up three things the audit had not, each harmless only while nothing read a directly established connection:
expected_disconnectwas cleared only at the top ofrun's loop (src/client/lifecycle.rs:583), so a caller driving connections itself carried the retired connection's verdict into the next one.connecthad no shutdown check, unlikerun, so a connection established afterdisconnectwas one the application had already been told did not exist. One check is also not enough: a version fetch, a transport, a handshake and the slot locks are awaited after it.connect, so a tick taken before anything read the connection was refused asNotConnected, which its loop classifies as fatal and exits on.Changes
Client::connectresolves toResult<Connection<'_>, ConnectError>instead ofResult<(), ConnectError>.Connectionis#[must_use], so the oldclient.connect().await?;becomes anunused_must_userather than silently doing nothing. Migration:client.connect().await?.read_until_disconnected().await;, orclient.run().awaitfor a session that reconnects. Acompile_faildoctest onconnectpins the old form as no longer accepted, next to ano_runtwin for the new one.Connection::read_until_disconnectedis the public single-connection read: read loop, connection teardown,Disconnecteddispatch, returning the disconnect reason (Nonewhen there was no failure to report). No reconnect, so a session that must not outlive its first connection now has a supported route.run's per-connection half is extracted intoClient::drive_connectionand shared with the above. Pure extraction: same order, same logs, same event, andrunkeepstelemetry::connect("ok")at the same point.connectclearsexpected_disconnectwith the rest of the per-connection state. Onlyruncleared it before, so a connection driven directly after an end that sets the flag (the 515 that follows pairing) came up with it still set:handle_successrefuses the login and the read loop exits on the first frame it decodes.runclears it before it connects, so its path is unchanged.connectrefuses a shut-down client with the newConnectError::Shutdown, the same refusalrunalready makes. It reads the signal again before the handshake, and once more after the slots are filled, undoing the publish if a shutdown landed across those awaits:signal_shutdown_syncruns no cleanup of its own, so a socket installed there would outlive the client that announced it was gone.runreports the abandonment at debug rather than error, since it is about to exit on the same shutdown.ConnectErroris#[non_exhaustive], so the new variant only affects code matching it exhaustively.connect, so it cannot take a tick before a reader exists and exit as fatal. Forrunthat is the same instant; for a directly driven connection it is the difference between having a dead-socket watchdog and not.is_runningfor its duration and gives it back through a scope guard, so a dropped read (a wrapping timeout, an aborted task) does not leave the flag standing. That flag is already the client's answer to "is anything reading" (can_reach_server,is_terminal); leaving it false would refuse every request sent over a connection that does have a reader, and leaving it stuck true would advertise a reader that is gone.runsets it before connecting, so its path is untouched by this.Connectiondropped without being read logs a warning. That is the answer to the "nobody reads for N seconds" question: there is no N. Drop is the moment we know for certain, it costs no timer and no task, and nothing legitimate connects and then never reads.runandconnectstating which one reads, plus corrections to theis_terminal,can_reach_serverandstop_supervision_loopdocs, which describedis_runningas "run()'s supervision loop" and now have a second reader to account for.Connectionborrows the client, sosize_of::<Result<Connection, ConnectError>>() == size_of::<Result<(), ConnectError>>()(asserted in a test) and connect allocates exactly what it did before. The added shutdown reads are loads on a sticky flag, and the retraction allocates nothing on the path that succeeds.Four things the AI reviewers raised that this deliberately does not do:
read_until_disconnecteddoes not runcleanup_connection_state, so the socket stays open and the client keeps reporting itself connected untildisconnect()releases it. That is not new and not specific to this path: it is what dropping therunfuture has always done, andcleanup_connection_stateis async, so no drop guard can await it. Spawning a detached teardown instead would be worse than the state it fixes: cleanup only clears the transport slots before the close it awaits, which is safe today because every caller awaits cleanup before connecting again, and a detached one could strip a connection the consumer establishes right after. The reader flag is restored because that one is a synchronous store, and the rest of the contract is documented on the method.is_connected.store(true). What remains after the retraction above is a window with no awaits in it, and losing it does not strand anything: the shutdown signal is sticky (wacore/src/runtime.rs:252-273), and bothdisconnectandsignal_shutdown_syncfirenotify_connection_shutdown. A shutdown before the connection's notifier is reset is caught by the retraction; one after it leaves the fresh notifier fired, so the read loop returnsExpectedon its first poll anddrive_connectiontears the connection down in full. Closing that window properly would mean a new cross-cutting mutex on the connect path that every terminal path must take, includingsignal_shutdown_sync, which exists precisely to be callable fromDropwhere nothing can be awaited or blocked on. That is a worse trade than the self-healing state it replaces.is_runningis set. As proposed this would breakrun, which setsis_runningbefore it connects and then drives through the same call. Two concurrent readers are also not reachable: aConnectiononly exists afteris_connectedis true, andconnectrefuses withAlreadyConnectedfrom that point on. A hypothetical second reader would in any case fail immediately, becauseread_messages_looptakes the transport-event receiver out of its slot.Nonelike a requested end does. Telling the two apart would mean a new public outcome type for a distinction a caller acts on the same way (connect again), so this documents whatNonecovers instead.Tests, all in
src/client/lifecycle.rs:a_driven_connection_decodes_the_first_frame_and_reports_its_endis the regression: a frame is queued on a connection, the recommended path is driven, and the assertion is on the decoded node (anEvent::RawNode) and on the returned disconnect reason, not on a log.a_connection_nobody_reads_leaves_the_frame_queuedpins what connecting alone guarantees (the socket is up) and what it does not (the frame is still queued, no node decoded, the reader's inlet untouched). It is the reported symptom, turned into a test.a_cancelled_read_gives_the_reader_flag_backcovers the failure side of claimingis_running: the read is cancelled mid-connection and the flag must come back.connecting_clears_the_retired_connection_expected_disconnectcovers the inherited-verdict fix; it fails without it.connecting_is_refused_after_the_client_is_shut_downcovers the shutdown refusal at the entry point. The reads inside the connect graph are not covered by tests of their own, and neither is the interleaving the reviewers asked for: reaching them needs a handshake to complete, which no unit fixture can do without a mock server.handing_back_a_connection_costs_the_caller_nothingasserts the size equality above.connect_rejects_an_already_connected_client, usesexpect_errand so requiresDebugon the Ok type;Connectionimplements it rather than the test changing.Validation
Both new guarantees were checked against their own absence: with
#[must_use]removed the migration doctest reports "Test compiled successfully, but it's markedcompile_fail", and with theexpected_disconnectreset removed its test fails. The full workspace clippy could not run locally (thevoip-cliexample needs libasound, absent in this environment); full matrix left to CI.