Skip to content

fix(iq): cancellation-safe IQ response waiters (unblocks try_join!) + named fan-out consts - #978

Merged
jlucaso1 merged 4 commits into
mainfrom
claude/perf-audit-parallelization-v42g7v
Jul 4, 2026
Merged

fix(iq): cancellation-safe IQ response waiters (unblocks try_join!) + named fan-out consts#978
jlucaso1 merged 4 commits into
mainfrom
claude/perf-audit-parallelization-v42g7v

Conversation

@jlucaso1

@jlucaso1 jlucaso1 commented Jul 4, 2026

Copy link
Copy Markdown
Collaborator

Summary

Follow-up to #975, taking two items from that PR's "Declined / follow-up" list as a focused refactor.

1. Cancellation-safe IQ response waiters — unblocks try_join! (main change)

send_and_wait_iq registers an entry in response_waiters and only removed it on the send-fail / timeout / shutdown paths — never when the future itself is dropped mid-await (cancellation). So a fail-fast futures::try_join! over two IQs drops the losing side the instant its sibling errors and leaks that waiter; a lingering waiter suppresses keepalives for the life of the connection. That's exactly why #975 had to keep is_on_whatsapp on join! (await both) instead of the fail-fast try_join!.

Root-cause fix: an RAII ResponseWaiterGuard that removes the entry on Drop, covering every exit path including cancellation. To remove from Drop (which can't .await), response_waiters moves from an async_lock::Mutex to a std::sync::Mutex — matching its node_waiters sibling (a trivial critical section never held across an await; the one drain site is block-scoped so the guard releases before its awaits). The four hand-rolled remove() calls in send_and_wait_iq collapse into the guard.

is_on_whatsapp now uses try_join! (fail-fast) — restoring the original sequential error-latency profile while keeping the two IQs concurrent.

2. Name the bounded fan-out concurrency limits (quick win)

The five buffer_unordered(16) sites (session probe, device-list resolve, status-LID resolve, companion-identity load, LID→PN resolve) get per-module named consts — matching the existing HISTORY_SYNC_CONCURRENCY / MEDIA_REUPLOAD_CONCURRENCY / APPSTATE_BLOB_DOWNLOAD_CONCURRENCY style so they're greppable and independently tunable.

Why a sync mutex is safe here

Every response_waiters lock site is a single-statement map op (insert / remove / len / is_empty), never held across an .await. Access is centralized through a Client::response_waiters_guard() helper (poison-recovering, mirroring the node_waiters helpers) so no call site reaches for a bare .lock().unwrap(); the only direct lock left is ResponseWaiterGuard::drop, which owns its Arc. The one site that binds the guard (cleanup_connection_state's drain) is block-scoped so it drops before the following awaits. Verified: the spawned client future stays Send, and wasm32 --no-default-features builds.

Review follow-ups (applied)

  • de-async register_ack_waiter — its only await was the response_waiters lock; now a sync std::sync::Mutex insert, so it's a plain fn (.await dropped at all call sites).
  • response_waiters_guard() helper — centralizes the poison-recovering lock; every &self/&Client access routed through it.
  • let-chain in node_io — the IQ-response waiter removal is folded into the outer let-chain (matching handle_ack_response, per the repo's let-chain guideline).
  • debug_assert! on req_id uniqueness — the guard removes by req_id; every send_and_wait_iq caller derives req_id from the monotonic generate_request_id() (verified across the codebase, incl. the sole InfoQuery.id setter in pair_code.rs), so an id is never in flight twice and the remove-by-id is unambiguous. The assert documents/enforces that invariant and catches any future caller that breaks it (a per-waiter token would be dead weight for a structurally-unreachable hazard).

Deliberately out of scope

  • ACK-waiter & VoIP place_call cancellation-safety — the ack/offer waiters were never cancellation-safe (pre-existing; this PR only migrated the lock syntax there, not the cleanup structure). Those paths also carry pending-call/registry state beyond the waiter, so extending the RAII pattern to them is a separate, larger change — a good follow-up now that the guard exists as a template.
  • history-sync in-flight-counter generation-scoping — floated as a second quick win but it needs threading connection_generation through the task lifecycle; more delicate than quick. The existing previous <= 1 → store(0) clamp already prevents the underflow; the remaining premature-idle edge is a separate, careful change.

Testing

  • New unit tests: ResponseWaiterGuard removes-on-drop, and is a no-op when the waiter was already resolved.
  • Existing coverage exercises the mutex change end-to-end: test_ack_waiter_resolves (resolve path), relay_waiter_no_ack_removes_response_waiter (voip no-ack cleanup, --features voip), the keepalive suite.
  • 921 lib tests pass; cargo clippy --lib --tests clean; cargo fmt --all; wasm32 --no-default-features builds. Binary size ~11 KiB smaller (async→sync mutex + collapsed cleanup).

… is_on_whatsapp

send_and_wait_iq registered a response_waiters entry and only removed it on the
send-fail / timeout / shutdown paths — never when the future itself was dropped
mid-await (cancellation). So a fail-fast futures::try_join! over two IQs would
drop the losing side the instant its sibling errored and leak that waiter; a
lingering waiter suppresses keepalives for the life of the connection. That's
why is_on_whatsapp had to use join! (await both) instead of try_join!.

Fix the root cause with an RAII ResponseWaiterGuard that removes the entry on
Drop, covering every exit path including cancellation. This requires the map to
be lockable from Drop, so response_waiters moves from an async Mutex to a
std::sync::Mutex (matching the node_waiters sibling: a trivial critical section
never held across an await; the one drain site is block-scoped so the guard is
released before its awaits). The four hand-rolled removes in send_and_wait_iq
collapse into the guard. is_on_whatsapp now uses try_join! for fail-fast.

Also name the bounded fan-out concurrency limits (buffer_unordered(16) at the
session-probe, device-list, status-LID, companion-identity, and LID→PN sites)
as per-module consts, matching the existing HISTORY_SYNC_CONCURRENCY /
MEDIA_REUPLOAD_CONCURRENCY style, so they're greppable and independently tunable.

Tests: ResponseWaiterGuard removes-on-drop and no-op-when-resolved; existing ack,
voip no-ack, and keepalive suites cover the mutex change. 921 lib tests pass;
clippy clean; wasm32 --no-default-features builds.
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@coderabbitai

coderabbitai Bot commented Jul 4, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The response_waiters map moves from async_lock::Mutex to std::sync::Mutex with poison-recovery unwrapping across client, messaging, node I/O, keepalive, send, and voip code. A new ResponseWaiterGuard RAII type ensures cleanup on cancellation in send_and_wait_iq. Several hard-coded concurrency literals become named constants, and is_on_whatsapp switches to fail-fast try_join!.

Changes

Response Waiter Synchronization Refactor

Layer / File(s) Summary
ResponseWaiterMap contract
src/client.rs
Adds ResponseWaiterMap type alias and changes the response_waiters field to Arc<std::sync::Mutex<ResponseWaiterMap>>.
Lifecycle init and teardown
src/client/lifecycle.rs, src/client/accessors.rs
Updates initialization to use the sync mutex and reworks cleanup_connection_state to swap out waiters synchronously, avoiding holding the lock across awaits; memory reporting handles poison recovery.
RAII waiter cleanup
src/request.rs
Introduces ResponseWaiterGuard that removes waiter entries on Drop, wires it into send_and_wait_iq, removes manual cleanup on timeout paths, and adds unit tests.
Waiter consumers
src/client/messaging.rs, src/client/node_io.rs, src/keepalive.rs, src/client/tests.rs
Switches register_ack_waiter, IQ/ack response resolution, and keepalive pending-check to non-async locking with poison recovery; updates tests accordingly.
Send and VoIP cleanup paths
src/send/mod.rs, src/voip/facade.rs
Updates waiter removal on send failure/timeout in status/message send and phash validation, and offer/relay cleanup in voip calls, to non-async locking with poison recovery; updates related tests.
Concurrency constants and fail-fast contacts
src/client/sessions.rs, src/features/groups.rs, src/prekeys.rs, src/send/mod.rs, src/usync.rs, src/features/contacts.rs
Replaces hard-coded 16 concurrency literals with named constants in several buffer_unordered calls; changes is_on_whatsapp from join! to try_join! for fail-fast error handling.

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

Sequence Diagram(s)

sequenceDiagram
    participant Caller
    participant send_and_wait_iq
    participant ResponseWaiterGuard
    participant response_waiters
    participant node_io

    Caller->>send_and_wait_iq: send IQ, await response
    send_and_wait_iq->>response_waiters: insert(id, tx)
    send_and_wait_iq->>ResponseWaiterGuard: create guard(id, waiters)
    alt IQ response arrives
        node_io->>response_waiters: remove(id), resolve tx
        send_and_wait_iq->>Caller: return response
        ResponseWaiterGuard-->>ResponseWaiterGuard: drop (no-op, already removed)
    else timeout / cancellation
        send_and_wait_iq-->>ResponseWaiterGuard: future dropped/timed out
        ResponseWaiterGuard->>response_waiters: remove(id) on Drop
        send_and_wait_iq->>Caller: return IqError::Timeout
    end
Loading

Possibly related PRs

  • oxidezap/whatsapp-rust#310: Both PRs modify Client::cleanup_connection_state teardown logic to drain/remove pending response_waiters.
  • oxidezap/whatsapp-rust#485: Both PRs modify response_waiters lifecycle handling in src/request.rs's waiter-resolution/cleanup code paths.
  • oxidezap/whatsapp-rust#862: Both PRs modify send_and_wait_iq in src/request.rs, one adding an RAII cleanup guard, the other changing the send future signature.

Suggested reviewers: greptile-apps, cubic-dev-ai

Look, this is the kind of change I like: focused, deliberate, and it removes wasted async overhead from a hot path — that's what winning looks like. But I need every poison-recovery unwrap and the new ResponseWaiterGuard drop path stress-tested under cancellation, because a leaked waiter is not a "small bug," it's a broken product experience for billions of messages. Ship it only after the tests prove the guard actually cleans up in every exit branch.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
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.
Title check ✅ Passed The title accurately summarizes the main fix and the added concurrency-constant refactor.
Description check ✅ Passed The description is directly related to the changeset and explains the same waiter-safety and fan-out const updates.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/perf-audit-parallelization-v42g7v

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

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

@greptile-apps

greptile-apps Bot commented Jul 4, 2026

Copy link
Copy Markdown

Greptile Summary

This PR makes IQ response waiters cancellation-safe by introducing ResponseWaiterGuard, an RAII wrapper that removes the waiter entry on drop — covering the cancellation path that the previous explicit remove() calls missed. It also migrates response_waiters from an async mutex to std::sync::Mutex (matching the node_waiters sibling), which is what allows Drop to call remove without .await. Five buffer_unordered(16) magic numbers are replaced with named consts.

  • ResponseWaiterGuard (src/request.rs): The four hand-rolled remove() calls in send_and_wait_iq collapse into the guard's Drop impl; a debug_assert! catches duplicate in-flight IQ ids at test time. Two new unit tests cover the remove-on-drop and already-resolved no-op paths.
  • is_on_whatsapp now uses try_join! (src/features/contacts.rs): Previously forced to use join! to avoid leaking the cancelled sibling's waiter; the RAII guard makes try_join! safe, restoring fail-fast error semantics.
  • Named fan-out consts across five files: bare 16 literals become greppable, independently-tunable named consts consistent with the existing HISTORY_SYNC_CONCURRENCY / MEDIA_REUPLOAD_CONCURRENCY style.

Confidence Score: 5/5

Safe to merge — the RAII guard correctly handles all exit paths, the sync mutex is never held across an .await, and try_join! is now sound.

The core invariant (sync mutex never crosses an await boundary) holds at every lock site: the drain in cleanup_connection_state is block-scoped, let-chain uses of response_waiters_guard() have no await in the body, and register_ack_waiter is a single-statement insert. ResponseWaiterGuard::Drop correctly handles both the already-resolved (no-op) and pending-cancellation cases, and both are directly tested. 921 lib tests pass and the wasm32 build is verified clean.

No files require special attention.

Important Files Changed

Filename Overview
src/request.rs Introduces ResponseWaiterGuard RAII type that removes the waiter entry on every exit path including cancellation; collapses four hand-rolled remove() calls into the guard's Drop; adds two unit tests.
src/client.rs Exports ResponseWaiterMap type alias; migrates response_waiters field from async to std::sync::Mutex, enabling Drop-based cleanup.
src/client/accessors.rs Adds response_waiters_guard() helper centralising poison-recovering unwrap_or_else(into_inner).
src/client/lifecycle.rs Block-scopes the drain in cleanup_connection_state so the sync guard drops before subsequent .awaits, keeping the future Send.
src/client/messaging.rs Removes async keyword from register_ack_waiter; all three call sites drop their .await.
src/client/node_io.rs Replaces lock().await.remove() with response_waiters_guard().remove() at both IQ and ACK dispatch sites; safe since no await appears in the if-body.
src/features/contacts.rs Replaces join! with try_join!; now safe because send_and_wait_iq uses ResponseWaiterGuard.
src/keepalive.rs Replaces async lock().await.is_empty() with sync response_waiters_guard().is_empty(); behaviour unchanged.
src/send/mod.rs Removes .await from two register_ack_waiter sites; collapses three lock().await.remove() calls; manual cleanup on send-failure preserved for ACK waiters outside the guard scope.
src/voip/facade.rs Collapses five verbose lock().await.remove() blocks into response_waiters_guard().remove(); updates tests accordingly.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Caller
    participant send_and_wait_iq
    participant ResponseWaiterMap
    participant ResponseWaiterGuard
    participant node_io

    Caller->>send_and_wait_iq: call (req_id)
    send_and_wait_iq->>ResponseWaiterMap: insert(req_id, tx)
    send_and_wait_iq->>ResponseWaiterGuard: "create (holds Arc<Mutex<Map>>, req_id)"

    alt Normal success path
        node_io->>ResponseWaiterMap: remove(req_id) — sends on tx
        send_and_wait_iq->>Caller: Ok(response)
        ResponseWaiterGuard->>ResponseWaiterMap: remove(req_id) [no-op, already gone]
    else Send failure / timeout / shutdown
        send_and_wait_iq->>Caller: Err(...)
        ResponseWaiterGuard->>ResponseWaiterMap: remove(req_id) [cleans up]
    else Cancellation via try_join! drop
        Note over send_and_wait_iq,ResponseWaiterGuard: Future dropped mid-await
        ResponseWaiterGuard->>ResponseWaiterMap: remove(req_id) [Drop — only path that catches this]
    end
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"}}}%%
sequenceDiagram
    participant Caller
    participant send_and_wait_iq
    participant ResponseWaiterMap
    participant ResponseWaiterGuard
    participant node_io

    Caller->>send_and_wait_iq: call (req_id)
    send_and_wait_iq->>ResponseWaiterMap: insert(req_id, tx)
    send_and_wait_iq->>ResponseWaiterGuard: "create (holds Arc<Mutex<Map>>, req_id)"

    alt Normal success path
        node_io->>ResponseWaiterMap: remove(req_id) — sends on tx
        send_and_wait_iq->>Caller: Ok(response)
        ResponseWaiterGuard->>ResponseWaiterMap: remove(req_id) [no-op, already gone]
    else Send failure / timeout / shutdown
        send_and_wait_iq->>Caller: Err(...)
        ResponseWaiterGuard->>ResponseWaiterMap: remove(req_id) [cleans up]
    else Cancellation via try_join! drop
        Note over send_and_wait_iq,ResponseWaiterGuard: Future dropped mid-await
        ResponseWaiterGuard->>ResponseWaiterMap: remove(req_id) [Drop — only path that catches this]
    end
Loading

Reviews (3): Last reviewed commit: "docs: fix typo (paniced -> panicked) in ..." | Re-trigger Greptile

@github-actions

github-actions Bot commented Jul 4, 2026

Copy link
Copy Markdown

📦 Binary size report

Metric main PR Δ
bin size (stripped) 10.77 MiB 10.76 MiB -10.78 KiB (-0.10%) 🔽
bin .text 8.77 MiB 8.76 MiB -10.88 KiB (-0.12%) 🔽
bin allocated (text+data+bss) 10.77 MiB 10.75 MiB -11.94 KiB (-0.11%) 🔽
llvm-lines wacore 503,173 503,173 0
llvm-lines wacore copies 17,243 17,243 0
llvm-lines whatsapp-rust lib 745,608 744,529 -1,079 (-0.14%) 🔽
llvm-lines whatsapp-rust lib copies 24,265 24,248 -17 (-0.07%) 🔽
deps crates (Cargo.lock) 466 466 0
.text per crate
Crate main PR Δ
.text whatsapp_rust 1.60 MiB 1.60 MiB -8.56 KiB (-0.52%) 🔽
.text wacore 528.64 KiB 528.83 KiB +197 B (+0.04%) 🔺
.text wacore_binary 157.49 KiB 157.49 KiB 0
.text wacore_libsignal 178.30 KiB 178.30 KiB 0
.text wacore_appstate 156.42 KiB 156.42 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 509.25 KiB 509.25 KiB 0
.text whatsapp_rust_tokio_transport 43.61 KiB 43.61 KiB 0
.text whatsapp_rust_ureq_http_client 10.47 KiB 10.47 KiB 0
.text std 1.00 MiB 1020.47 KiB -3.58 KiB (-0.35%) 🔽
.text other deps 2.94 MiB 2.94 MiB +1000 B (+0.03%) 🔺
Top movers (cargo-bloat attribution)
Crate main PR Δ
whatsapp_rust 1.60 MiB 1.60 MiB -8.56 KiB (-0.52%)
std 1.00 MiB 1020.47 KiB -3.58 KiB (-0.35%)

Baseline: 4def65976 (latest main run) · Head: 1d04f517c · Graphs

…tion

Its only await was the response_waiters lock, now a std::sync::Mutex insert, so
the async fn had no await left. Make it a plain fn and drop .await at the send
and voip call sites. (clippy::unused_async is nursery-only, so CI wouldn't have
flagged it.)

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 (4)
src/send/mod.rs (1)

917-928: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Make ACK waiters cancellation-safe too.

The cleanup only runs on send error/timeout. If the future is dropped while send_node is pending—or in send_message_impl during persist_outbound_msg_secret(...).await before spawn_phash_validation owns the receiver—the waiter can stay in response_waiters and suppress keepalives. Use the same RAII guard pattern for ACK waiters, or spawn/pass ownership to phash validation before any post-send await.

Also applies to: 1189-1204, 1879-1939

🤖 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/send/mod.rs` around lines 917 - 928, The ACK waiter cleanup in the send
path is not cancellation-safe, so a dropped future can leave entries behind in
response_waiters and block keepalives. Update the ack-waiter handling in
send_message_impl and the related send flow around send_node and
spawn_phash_validation to use the same RAII-style guard pattern as other
waiters, or transfer ownership to the phash validation task before any await
that can be cancelled. Ensure the waiter is always removed when the operation
ends, even if the future is dropped before send_node completes.
src/voip/facade.rs (1)

538-606: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Clean up VoIP offer registration on cancellation.

If place_call is dropped while client.send_node(offer).await is in flight, this send-failure cleanup never runs, leaving the ACK waiter plus pending/registry state behind without a returned handle to clean it up. Add a scoped guard that reaps the waiter and generation until the offer send succeeds and ownership is transferred to spawn_outgoing_relay_waiter.

🤖 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/voip/facade.rs` around lines 538 - 606, The outgoing call setup in
place_call leaves behind pending_outgoing_calls, the registry generation, and
the ACK waiter if send_node(offer).await is canceled before it returns, because
the current cleanup only runs on an Err path. Add a scoped guard around the
offer send that tracks the call_id/generation and removes the response_waiters
entry plus pending_outgoing_calls and registry state on drop, then explicitly
disarm it only after send_node succeeds and ownership is handed off to
spawn_outgoing_relay_waiter. Use the existing place_call, send_node,
take_pending_if_current, registry.remove_if_current, and response_waiters
symbols to locate the cleanup flow.
src/client/node_io.rs (1)

431-446: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Use a let-chain here, like handle_ack_response does a few hundred lines down.

Same file, same pattern (lock → remove(id) → act on Some), but this one nests a plain if let instead of folding the removal into the outer let-chain. Pick one style — the ack-waiter path already shows the preferred one.

♻️ Proposed fix
         if nr.tag.as_ref() == "iq"
             && let Some(id) = nr.get_attr("id").map(|v| v.as_str())
+            && let Some(waiter) = self
+                .response_waiters
+                .lock()
+                .unwrap_or_else(|p| p.into_inner())
+                .remove(id.as_ref())
         {
-            // Single lock acquisition: try to remove the waiter directly.
-            let waiter = self
-                .response_waiters
-                .lock()
-                .unwrap_or_else(|p| p.into_inner())
-                .remove(id.as_ref());
-            if let Some(waiter) = waiter {
-                if waiter.send(Arc::clone(&node)).is_err() {
-                    warn!(target: "Client/IQ", "Failed to send IQ response to waiter. Receiver was likely dropped.");
-                }
-                return;
-            }
+            if waiter.send(Arc::clone(&node)).is_err() {
+                warn!(target: "Client/IQ", "Failed to send IQ response to waiter. Receiver was likely dropped.");
+            }
+            return;
         }

As per coding guidelines: "Always use let-chains (if let Some(x) = foo && let Some(y) = x.bar { ... }) instead of nested if let blocks."

🤖 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/node_io.rs` around lines 431 - 446, The IQ response handling in
node_io::handle_* uses a nested if let after locking response_waiters, but the
project style prefers let-chains like handle_ack_response. Refactor this block
to fold the remove(id) result into the outer condition, so the waiter lookup and
send path use a single let-chain instead of an inner if let; keep the existing
behavior and warning in the Some(waiter) case.

Source: Coding guidelines

src/client.rs (1)

470-533: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Alright, this is the kind of infrastructure work I like to see — sync mutex, short critical section, RAII-friendly. Good.

One thing bugs me though: .lock().unwrap_or_else(|p| p.into_inner()) gets copy-pasted at every call site touching this field (lifecycle.rs, accessors.rs, messaging.rs, node_io.rs x2, keepalive.rs). A Client already has a helper pattern for this (resolve_node_waiters, clear_sent_node_waiters for the node_waiters sibling) — consider adding something like fn response_waiters_guard(&self) -> MutexGuard<'_, ResponseWaiterMap> here so future call sites can't accidentally use a bare .lock().unwrap() and panic on poison.

♻️ Proposed helper
+impl Client {
+    #[inline]
+    pub(crate) fn response_waiters_guard(&self) -> std::sync::MutexGuard<'_, ResponseWaiterMap> {
+        self.response_waiters
+            .lock()
+            .unwrap_or_else(|p| p.into_inner())
+    }
+}
🤖 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 470 - 533, The new `response_waiters` sync mutex
is being accessed with repeated `.lock().unwrap_or_else(|p| p.into_inner())`
patterns across multiple call sites, which is easy to misuse and can lead to
panic-prone direct locks later. Add a `Client` helper like
`response_waiters_guard(&self) -> MutexGuard<'_, ResponseWaiterMap>` near the
`response_waiters` field, and update the existing usages in the affected paths
(for example the same way `resolve_node_waiters` and `clear_sent_node_waiters`
encapsulate `node_waiters`) so all access goes through the helper and
consistently handles poison recovery.
🤖 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/request.rs`:
- Around line 39-44: ResponseWaiterGuard::drop currently removes waiters by
req_id only, so an older displaced guard can delete a newer replacement inserted
by add_waiter. Update the waiter registration/removal flow in
ResponseWaiterGuard and add_waiter to either reject duplicate req_id inserts or
associate each waiter with a unique token/generation and only remove the entry
if it still matches that token. Make sure the fix covers both the guard drop
path and the waiter map update logic referenced by
add_waiter/ResponseWaiterGuard.

---

Outside diff comments:
In `@src/client.rs`:
- Around line 470-533: The new `response_waiters` sync mutex is being accessed
with repeated `.lock().unwrap_or_else(|p| p.into_inner())` patterns across
multiple call sites, which is easy to misuse and can lead to panic-prone direct
locks later. Add a `Client` helper like `response_waiters_guard(&self) ->
MutexGuard<'_, ResponseWaiterMap>` near the `response_waiters` field, and update
the existing usages in the affected paths (for example the same way
`resolve_node_waiters` and `clear_sent_node_waiters` encapsulate `node_waiters`)
so all access goes through the helper and consistently handles poison recovery.

In `@src/client/node_io.rs`:
- Around line 431-446: The IQ response handling in node_io::handle_* uses a
nested if let after locking response_waiters, but the project style prefers
let-chains like handle_ack_response. Refactor this block to fold the remove(id)
result into the outer condition, so the waiter lookup and send path use a single
let-chain instead of an inner if let; keep the existing behavior and warning in
the Some(waiter) case.

In `@src/send/mod.rs`:
- Around line 917-928: The ACK waiter cleanup in the send path is not
cancellation-safe, so a dropped future can leave entries behind in
response_waiters and block keepalives. Update the ack-waiter handling in
send_message_impl and the related send flow around send_node and
spawn_phash_validation to use the same RAII-style guard pattern as other
waiters, or transfer ownership to the phash validation task before any await
that can be cancelled. Ensure the waiter is always removed when the operation
ends, even if the future is dropped before send_node completes.

In `@src/voip/facade.rs`:
- Around line 538-606: The outgoing call setup in place_call leaves behind
pending_outgoing_calls, the registry generation, and the ACK waiter if
send_node(offer).await is canceled before it returns, because the current
cleanup only runs on an Err path. Add a scoped guard around the offer send that
tracks the call_id/generation and removes the response_waiters entry plus
pending_outgoing_calls and registry state on drop, then explicitly disarm it
only after send_node succeeds and ownership is handed off to
spawn_outgoing_relay_waiter. Use the existing place_call, send_node,
take_pending_if_current, registry.remove_if_current, and response_waiters
symbols to locate the cleanup flow.
🪄 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: 09898b4e-eca6-4c58-baf3-8098040106dc

📥 Commits

Reviewing files that changed from the base of the PR and between 4def659 and d7d72ad.

📒 Files selected for processing (15)
  • src/client.rs
  • src/client/accessors.rs
  • src/client/lifecycle.rs
  • src/client/messaging.rs
  • src/client/node_io.rs
  • src/client/sessions.rs
  • src/client/tests.rs
  • src/features/contacts.rs
  • src/features/groups.rs
  • src/keepalive.rs
  • src/prekeys.rs
  • src/request.rs
  • src/send/mod.rs
  • src/usync.rs
  • src/voip/facade.rs

Comment thread src/request.rs

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

Requires human review: This PR switches core IQ waiter synchronization from async_lock::Mutex to std::sync::Mutex and adds a Drop guard for safety. Please verify correctness and test coverage.

Re-trigger cubic

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

1 issue found and verified against the latest diff

Confidence score: 3/5

  • In src/request.rs, the drop handler removes waiters by req_id alone, so if a newer in-flight request reuses the same ID, dropping the older future can delete the newer waiter; that can lead to missed wakeups or a request that never resolves after merge. Before merging, make waiter removal identity-safe (e.g., include a per-request generation/token or only remove when the stored waiter matches the dropping future).

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread src/request.rs
…id uniqueness

Review follow-ups on the sync-mutex migration:

- Add Client::response_waiters_guard() (poison-recovering lock, mirroring the
  node_waiters helpers) and route every &self/&Client access through it, so no
  call site can reach for a bare .lock().unwrap() that would panic on poison.
  The only direct lock left is ResponseWaiterGuard::drop, which owns its Arc.
- Fold the IQ-response waiter removal into the outer let-chain (matching
  handle_ack_response) per the repo's let-chain style guideline.
- debug_assert! that a req_id isn't already in flight before inserting. req_ids
  come from the monotonic generate_request_id() (verified: every send_and_wait_iq
  caller, incl. the sole InfoQuery.id setter in pair_code, uses it), so a given
  id is never in flight twice — the invariant that makes the guard's remove-by-id
  unambiguous. The assert catches any future caller that breaks it.

921 lib tests pass; clippy clean; voip no-ack test passes.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

1 issue found across 9 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread src/client/accessors.rs Outdated

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

0 issues found across 1 file (changes from recent commits).

Requires human review: Core IQ waiter refactor changes lock type and adds RAII guard. Impact on connection reliability is high; requires human review.

Re-trigger cubic

@jlucaso1
jlucaso1 merged commit 61df380 into main Jul 4, 2026
17 checks passed
@jlucaso1
jlucaso1 deleted the claude/perf-audit-parallelization-v42g7v branch July 4, 2026 16:36
JeanCapixaba added a commit to JeanCapixaba/whatsapp-rust that referenced this pull request Jul 7, 2026
The ack path resolves the internal response_waiters map (kept
allocation-free by oxidezap#827, reworked by oxidezap#978) with no public hook, so
consumers have no way to observe server acks. This adds an observe-only
Event::ServerAck dispatched in handle_ack_response for every <ack> that
carries an id, before and independently of the waiter resolution, so it
never interacts with the send/phash flow.

Use cases: measuring send -> server-accept latency (which tells
server-side acceptance apart from fan-out), and surfacing nack codes
(463/479/...) programmatically instead of scraping warn! logs.

The payload is a named ServerAck struct (same pattern as
Receipt(Receipt), so it can grow without breaking matches) carrying id,
class, from, the server t timestamp when present (whatsmeow reads the
same attribute into SendResponse.Timestamp), and the nack error code.
Server acks cover every outgoing stanza class, so class lets consumers
filter message acks without correlating ids blind.

The dispatch is gated on EventBus::has_handler_for(EventKind::ServerAck)
so the hot ack path stays allocation-free when nobody subscribes
(ack_miss_path_does_not_heap_allocate stays green). EventKind::ServerAck
is appended at the end to keep existing EventInterest bit indexes
stable; the build-time ceiling tripwire now points at it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants