fix(client): release transport and feed guards before awaiting - #1221
Conversation
Edition 2024 shortened the life of an `if let` scrutinee temporary only for
the `else` branch. In the arm that matches, the temporary is still alive for
the whole body, because the binding may borrow from it. So in
`if let Some(x) = mutex.lock()....take() { x.something().await }` the guard is
held for the entire body, and the same is true of a `match` scrutinee.
That bites four teardown paths in the client: `disconnect`, `reconnect`,
`reconnect_immediately` and `cleanup_connection_state` all closed the socket
with the `transport` guard still held. `WsTransport::disconnect` writes a real
TLS close frame with no timeout, and first waits on the sink mutex the sender
task may be holding, while `connect_internal` installs the next connection's
transport through that same `transport` mutex. A close that stalls therefore
parks the whole reconnect. Hoisting the value out of the guard mirrors what
the stream-error path in `node_io` already does.
`cleanup_connection_state` had one more of the same shape: the
`app_state_processor` guard was held across `clear_key_cache().await`, which
takes the processor's own cache lock, so every `get_app_state_processor()`
waited behind it.
In the video facade, `attach_endpoints` and `detach_endpoints` aborted the
previous feed under the `feed` mutex. `AbortHandle::abort` runs a closure the
`Runtime` supplied; a runtime that cancels synchronously drops the task inline,
and anything in that drop touching `feed` re-enters a non-reentrant
`std::sync::Mutex`. Not reachable with the current tokio runtime, whose abort
is just a flag, but the trait admits runtimes where it is. `AbortHandle::abort`
itself called the closure under its own `abort_fn` lock for the same reason and
now takes it out first.
Both fixes come with a regression test that fails on the previous code: one
proves the transport slot stays installable while a close is in flight, one
drives attach/replace/detach against a runtime whose abort re-enters the feed
lock.
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change updates asynchronous cleanup in client lifecycle and video endpoint handling. Mutex-protected handles are extracted before awaiting closure or invoking cancellation callbacks. Tests cover parked transport shutdown and reentrant video-feed cancellation. ChangesAsynchronous cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 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 | Releases transport and app-state processor guards before asynchronous teardown and adds lifecycle race regression tests. |
| src/voip/facade.rs | Moves feed cancellation outside the feed mutex and tests reentrant runtime cancellation. |
| wacore/src/runtime.rs | Takes the one-shot abort callback under lock and invokes it only after releasing the internal mutex. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Acquire mutex] --> B[Take or clone guarded value]
B --> C[Release mutex guard]
C --> D[Await disconnect or invoke abort callback]
C --> E[Concurrent caller may acquire mutex]
Reviews (2): Last reviewed commit: "fix(client): clear connection slots befo..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 1174-1176: Update the lifecycle cleanup method containing the
transport.take and disconnect call to prevent post-await cleanup from affecting
a replacement connection: retire or detach transport, transport_events, and
noise_socket before the first await, or guard each cleanup action with the
captured closed_generation. Extend the parked-close test to publish a
replacement transport_events and noise_socket and verify they remain intact.
🪄 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: d60c4ab4-c624-4df4-b9d4-fedbda9fe4ec
📒 Files selected for processing (3)
src/client/lifecycle.rssrc/voip/facade.rswacore/src/runtime.rs
![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: 03770574f8
ℹ️ 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".
| // idempotent, so the redundant call from `Client::disconnect()` is | ||
| // safe. | ||
| if let Some(transport) = self.transport.lock().await.take() { | ||
| let transport = self.transport.lock().await.take(); |
There was a problem hiding this comment.
When a new connect() progresses while this old transport's disconnect() is parked—the exact concurrency the new regression test permits—it can install its transport, event receiver, and Noise socket at lines 845–847. Once the old close resumes, this cleanup leaves the replacement transport in place but unconditionally clears the replacement's transport_events and noise_socket at lines 1178–1179, after which read_messages_loop() fails with NotStarted; the remaining cleanup also resets state belonging to that new connection. The teardown must either prevent publication until all old-generation cleanup is complete or conditionally clear only state associated with the transport/generation it took.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, and introduced by this change: the held guard used to block connect_internal's first install, so it could not publish a whole connection inside the close window. Fixed in 13ddd6f by clearing all three slots before the close is awaited, with a test that fails the other way round.
Generated by Claude Code
![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
📦 Binary size report
.text per crate
Baseline: |
Releasing the transport guard removed an accidental barrier: while cleanup held it across the close, a concurrent `connect()` could not finish publishing its transport, event receiver and Noise socket, because the first of those three installs blocked on the same mutex. Without the guard it can publish all three while the old close is still in flight, and cleanup then cleared the replacement's `transport_events` and `noise_socket` instead of the ones it was tearing down, leaving the new connection with no read loop. Clear all three slots in the same pass that takes the transport, before the close is awaited, so cleanup only ever retires state that existed when it started. Ordering between shutdown, disconnect and cleanup is unchanged; this moves two lines within cleanup itself.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13ddd6f333
ℹ️ 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 transport = self.transport.lock().await.take(); | ||
| *self.transport_events.lock().await = None; | ||
| *self.noise_socket.lock().await = None; |
There was a problem hiding this comment.
Fresh evidence after the prior comment: moving these clears before disconnect().await still leaves the handoff racy because the three slots use separate mutex acquisitions. If connect_graph() publishes its replacement transport and event receiver at lines 845–847 after this take() but before the following assignments, cleanup can erase the replacement receiver or Noise socket; connect() then returns with is_connected == true, while read_messages_loop() fails with NotStarted. Serialize publication and teardown as one operation, or condition each clear on the connection generation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The narrow window between the three acquisitions is real, but it is not something this PR created or widened: before it, the same window sat between the guard drop and the two clears. What the PR did widen was the whole-close window, and that one is gone.
Neither remedy is a small change here. The generation check does not work as written: connect_internal does not stamp a generation when it publishes at 845-847, and cleanup itself bumps connection_generation on entry (line 1121), so both connections carry the same value at that point and the clears cannot tell them apart. Serializing publication with teardown means one mutex over all three slots, which is a restructuring of the connect path.
It would also not be sufficient. Cleanup goes on after the close to reset media_conn, the app-state key cache and request map, the chat lanes and the receipt state, none of which is generation-scoped either, so a connect() genuinely concurrent with a teardown is unsafe well beyond these three slots. In the real call graph it does not happen: the run loop awaits cleanup before connecting, and disconnect() clears is_running first. Making that interleaving safe is a teardown-generation change worth its own PR, not a rider on a guard fix.
Generated by Claude Code
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Confidence score: 4/5
- In
src/client/lifecycle.rs, the mid-close publish window is improved, but clearing the three slots via separate lock acquisitions can still race withconnect_internal, leaving partially reset lifecycle state and causing intermittent reconnect/close misbehavior — clear all three fields under one lock scope (or one atomic state transition) to remove the interleaving risk.
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:1179">
P2: Release of the transport guard is the right fix and closes the long mid-close publish window, but the three slots are still cleared in three separate non-atomic lock acquisitions. If a racing `connect_internal` (the exact concurrency this PR enables) publishes its new transport, transport_events and noise_socket *between* the `take()` on line 1178 and these two `= None` clears, its freshly published events and noise_socket get stripped even though its transport survives — tearing down a connection it doesn't own. The existing regression test only publishes after the close is entered (i.e. after these clears have already run), so it never exercises this narrower interleaving. Consider hardening the publish/teardown handoff (e.g. a generation check) or at least covering this interleaving deterministically.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // publish its own transport, events and socket while the close is in flight; clearing | ||
| // afterwards would strip the replacement connection instead of the one being torn down. | ||
| let transport = self.transport.lock().await.take(); | ||
| *self.transport_events.lock().await = None; |
There was a problem hiding this comment.
P2: Release of the transport guard is the right fix and closes the long mid-close publish window, but the three slots are still cleared in three separate non-atomic lock acquisitions. If a racing connect_internal (the exact concurrency this PR enables) publishes its new transport, transport_events and noise_socket between the take() on line 1178 and these two = None clears, its freshly published events and noise_socket get stripped even though its transport survives — tearing down a connection it doesn't own. The existing regression test only publishes after the close is entered (i.e. after these clears have already run), so it never exercises this narrower interleaving. Consider hardening the publish/teardown handoff (e.g. a generation check) or at least covering this interleaving deterministically.
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 1179:
<comment>Release of the transport guard is the right fix and closes the long mid-close publish window, but the three slots are still cleared in three separate non-atomic lock acquisitions. If a racing `connect_internal` (the exact concurrency this PR enables) publishes its new transport, transport_events and noise_socket *between* the `take()` on line 1178 and these two `= None` clears, its freshly published events and noise_socket get stripped even though its transport survives — tearing down a connection it doesn't own. The existing regression test only publishes after the close is entered (i.e. after these clears have already run), so it never exercises this narrower interleaving. Consider hardening the publish/teardown handoff (e.g. a generation check) or at least covering this interleaving deterministically.</comment>
<file context>
@@ -1171,12 +1171,16 @@ impl Client {
+ // publish its own transport, events and socket while the close is in flight; clearing
+ // afterwards would strip the replacement connection instead of the one being torn down.
let transport = self.transport.lock().await.take();
+ *self.transport_events.lock().await = None;
+ *self.noise_socket.lock().await = None;
if let Some(transport) = transport {
</file context>
There was a problem hiding this comment.
Answered in the parallel thread on line 1180. Short version: that narrow window predates this PR (it sat between the guard drop and the clears), a generation check cannot work until connect_internal stamps one at publish time, and cleanup keeps resetting other connection-scoped state after the close anyway. Worth its own change, not a rider here.
Generated by Claude Code
2 participants
Summary
Edition 2024 shortened the life of an
if letscrutinee temporary, but only for theelsebranch. In the arm that matches, the temporary is still alive for the whole body, because the binding may borrow from it. So inif let Some(x) = mutex.lock()....take() { x.something().await }the guard is held for the entire body, and amatchscrutinee behaves the same way (the edition change never applied to it at all).Four teardown paths in the client had exactly that shape:
disconnect,reconnect,reconnect_immediatelyandcleanup_connection_stateall closed the socket with thetransportguard still held. That matters becauseWsTransport::disconnectwrites a real TLS close frame with no timeout, and before that waits on thesinkmutex the Noise sender task may be holding, whileconnect_internalinstalls the next connection's transport through that sametransportmutex. A close that stalls therefore parks the entire reconnect, not just the socket it was closing.node_io's stream-error path already clones theArc<dyn Transport>out of the guard before awaiting; these four now do the same.cleanup_connection_statehad one more instance of the same shape a few lines down: theapp_state_processorguard was held acrossclear_key_cache().await, which takes the processor's own cache lock, so every concurrentget_app_state_processor()waited behind it.In the video facade,
attach_endpointsanddetach_endpointsaborted the previous feed under thefeedmutex.AbortHandle::abortinvokes aBox<dyn FnOnce>theRuntimesupplied, so what runs there is up to the runtime: one that cancels synchronously drops the task inline, and anything in that drop touchingfeedre-enters a non-reentrantstd::sync::Mutex. This is preventive, not a live bug -TokioRuntime's abort closure ishandle.abort(), which only sets a flag and returns. But theRuntimetrait is public and consumers supply their own, so the hazard is reachable for a runtime the trait permits, which is why the test below has to bring its own runtime to demonstrate it.Second commit, from review: releasing the transport guard removed an accidental barrier. While cleanup held it across the close,
connect_internalcould not publish a full connection in that window, because the first of its three installs blocked on the same mutex. Without the guard it can publish transport, event receiver and Noise socket while the old close is still in flight, and cleanup would then clear the replacement'stransport_eventsandnoise_socketrather than the ones it was tearing down, leaving the new connection with no read loop. Cleanup now retires all three slots in the same pass that takes the transport, before the close is awaited, so it only ever retires state that existed when it started.Evidence
First, the language rule itself, since the whole batch hangs on it. A standalone edition 2024 binary, four shapes, printing whether the mutex is still locked from inside the body:
So the matching arm holds it, the
elsebranch does not (that is the edition 2024 change),matchholds it, and hoisting the value into aletbefore theif letreleases it. That matches the premise this PR is built on.Then the behavioral tests, all of which live in the PR as regressions. On the tree before the fix, with the tests applied:
an_in_flight_socket_close_does_not_park_the_transport_slotinstalls a transport whosedisconnect()parks until released, runscleanup_connection_stateuntil the close is confirmed in flight, and then does whatconnect_internaldoes: install the next transport into the slot. Before the fix that install never completes and the 5s timeout fires.feed_swap_does_not_abort_while_holding_the_feed_lockbuilds a client on a runtime whose abort closure re-entersfeed.lock(), then runs attach / attach-replacing / detach on a dedicated thread so the re-entrant lock parks that thread instead of hanging the suite; the harness reports it viarecv_timeout. Both use polling with a bounded timeout, no fixed sleeps.The review finding got the same treatment.
a_connection_published_during_cleanup_is_not_stripped_by_itpublishes a replacement transport and event receiver while the old close is parked, then asserts both survive cleanup. With the clears left after the await it fails:After both fixes, everything in the crate:
No existing lifecycle or VoIP test needed adapting.
Changes
disconnect,reconnect,reconnect_immediately: clone theArc<dyn Transport>out of the guard, then close. Same shapenode_ioalready uses.cleanup_connection_state: take the transport and cleartransport_eventsandnoise_socketin one pass, then close what it took. Ordering betweenshutdown -> disconnect -> cleanupis unchanged; this moves two lines within cleanup itself.cleanup_connection_state_inner: clone theArc<AppStateProcessor>out beforeclear_key_cache().await, so the processor slot is not held across the cache lock.VideoShared::attach_endpoints/detach_endpoints: take the oldAbortHandleout of the guard beforeabort().AbortHandle::abort: take the closure out ofabort_fnbefore calling it. Same class, and it sits directly in the abort path the facade fix is about, so an abort closure that ever touched its own handle would deadlock there instead.Checked and not changed
The sweep was
if let/matchwhose scrutinee produces a guard, plus the.write()/.read().awaitvariants (none of those exist). What it turned up and why the rest is fine:transports/tokio-transport/src/lib.rs:172-disconnect()holds thesinkguard while writing the close frame. Real, but not the same fix: the body consumes the value it took out, and releasing the guard early would flip a concurrentsend()from blocking to failing fast with "Socket is closed". That is a behavior change in a different crate, so it does not belong in this batch. The client-side fix already stops it cascading into the reconnect.wacore/src/appstate_sync.rs:163- body is a barereturn Ok(cached). No await, no call.src/pair.rs:48and:337- bodies aretx.try_send(()), non-blocking by construction, plus a log line.src/pair_code.rs:1564andsrc/voip/transport.rs:515- both inside#[cfg(test)], on locks that are not shared with anything, bodies trivial.src/client/accessors.rs:323andsrc/handlers/call.rs:125- not the pattern. The first binds the guard in a block for a synchronousresource_report(); the second deliberately holds a lock for a scope.wacore/src/voip/registry.rs-set_media_task(line 1522) has the same shape viamatch, and there are severalif letsites in the same file. Left alone on purpose: that file is handled in a separate batch together with the lock-poison policy.Validation
Workspace-wide clippy could not run here (
alsa-syshas no system ALSA headers in this environment, which stops thevoip-cliexample before it reaches my crates), so the full matrix is left to CI. E2E not run locally; it is green on CI.