feat(events): observe-only ServerAck event for server <ack> stanzas - #989
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughA new observe-only ChangesServerAck event dispatch
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: 🚥 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 |
|---|---|
| wacore/src/types/events.rs | Adds EventKind::ServerAck at the end of the enum (preserving existing bit indexes), updates the build-time bitmask overflow assertion to point at the new last variant, and defines the ServerAck payload struct deriving Debug + Clone + Serialize — consistent with all other event payload types. |
| src/client/node_io.rs | Hoists ack_id/ack_error reads to the top of handle_ack_response, adds an interest-gated dispatch block before the waiter branch, and reuses the cached attrs in both the nack-logging block and the new dispatch — clean refactor with no behavioral change to the waiter path. |
| src/client/tests.rs | New test covers all four dispatch scenarios (plain ack, nack, id-less ack, and simultaneous waiter+handler); uses fictitious JIDs per the coding convention, let-chain assertions, and tokio timeout for the waiter leg. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Server
participant handle_ack_response
participant EventBus
participant ResponseWaiters
Server->>handle_ack_response: server ack stanza
handle_ack_response->>handle_ack_response: read ack_id and ack_error
alt error attribute present
handle_ack_response->>handle_ack_response: warn nack code
end
handle_ack_response->>EventBus: has_handler_for(ServerAck)?
alt handler subscribed AND id present
handle_ack_response->>EventBus: dispatch Event::ServerAck
EventBus->>EventBus: deliver to subscribed handlers
end
handle_ack_response->>ResponseWaiters: remove(id)?
alt waiter found
handle_ack_response->>ResponseWaiters: send OwnedNodeRef
handle_ack_response-->>Server: returns true
else no waiter
handle_ack_response-->>Server: returns false
end
%%{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 Server
participant handle_ack_response
participant EventBus
participant ResponseWaiters
Server->>handle_ack_response: server ack stanza
handle_ack_response->>handle_ack_response: read ack_id and ack_error
alt error attribute present
handle_ack_response->>handle_ack_response: warn nack code
end
handle_ack_response->>EventBus: has_handler_for(ServerAck)?
alt handler subscribed AND id present
handle_ack_response->>EventBus: dispatch Event::ServerAck
EventBus->>EventBus: deliver to subscribed handlers
end
handle_ack_response->>ResponseWaiters: remove(id)?
alt waiter found
handle_ack_response->>ResponseWaiters: send OwnedNodeRef
handle_ack_response-->>Server: returns true
else no waiter
handle_ack_response-->>Server: returns false
end
Reviews (4): Last reviewed commit: "test(client): cover ServerAck dispatch a..." | 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}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 3 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Auto-approved: Adds an observe-only ServerAck event for server acks; no impact on existing logic, gated on handler interest.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Auto-approved: Adds an observe-only ServerAck event for server stanzas. Low-impact, allocation-free when unused, with tests.
Re-trigger cubic
|
Hey @JeanCapixaba, thanks a lot for this one and welcome. Really solid first PR. It is well scoped, the description actually explains the why, and the tests cover the cases that matter. Exactly the kind of contribution we like to get. The approach is right, so nothing here is about the idea itself. A couple of things I would like to sort before it lands. First, credit where it is due. Making this observe-only instead of blocking the send on the ack is the correct call, it keeps us in line with how WA Web behaves and avoids a latency regression. Gating the dispatch on has_handler_for so the hot ack path stays allocation free is also spot on, it keeps the ack_miss_path_does_not_heap_allocate guarantee from #827 intact. And appending the EventKind at the end to keep the bit indexes stable is the right instinct. The main thing I want to nail down before this becomes public API is the payload. Right now it is { id, error }, but the server ack covers every outgoing stanza class, not just messages (message, receipt, notification, call). Without class on the event, a consumer cannot tell a message ack apart from an ack for a receipt we sent, so the send to server-accept latency story only really works if they already recorded the id on their side. Can you carry class, and probably from, on the event too? They are already on the node so it is basically free, and it turns this from a firehose the consumer has to correlate blind into something actually usable for the case you are describing. If the server ack also has a t timestamp, worth checking a capture, that would make the latency measurement cleaner too. Tied to that: since consumers will match on Event::ServerAck { id, error }, adding a field later is a breaking change even though the enum is non_exhaustive (that only guards the variant list, not the fields). So I would either mark the variant itself non_exhaustive, or wrap it in a small named struct like we already do for Receipt(Receipt), so it can grow without breaking anyone. Getting the shape right now saves a churn PR later. Smaller stuff, not blocking: The description and commit message say #604 moved ack waiting behind response_waiters, but #604 is the SKDM flow PR. I think you meant #827 (the allocation free ack path this builds on) or #978 (the waiter refactor). Worth fixing so the history stays honest. |
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.
64fa711 to
2b57a09
Compare
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
|
Payload sorted in 2b57a09 (squashed to one commit with a corrected message):
Test updated to cover class/from/t on a message ack and empty/None on a nack that omits them. |
|
Also, thank you for the warm welcome and for taking the time to write such a thorough review, especially on a first PR from an outsider. The class/from/t suggestion genuinely made the API better: what I had was enough for my own use case, but as you said, without class it's a firehose for anyone else. Happy to iterate again if anything else comes up on the next pass. Great project, by the way, the ack path was a pleasure to dig into. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/tests.rs`:
- Around line 211-277: The new ack coverage in
test_ack_dispatches_server_ack_event still misses the core guarantee that
ServerAck dispatch is independent of waiter resolution. Extend this test (or add
a sibling one) to register an ack waiter for a real id, call handle_ack_response
on the same ack node, and assert both the Event::ServerAck is emitted via
TestEventCollector and the waiter resolves successfully. Use the existing
create_test_client, register_handler, and handle_ack_response flow so the
behavior is verified end-to-end.
In `@wacore/src/types/events.rs`:
- Around line 1189-1210: Mark the ServerAck payload struct as non-exhaustive to
prevent external exhaustive construction and destructuring from breaking when
new fields are added later. Update the ServerAck definition in the events types
module by adding the non_exhaustive attribute directly on the ServerAck struct
so callers using Event::ServerAck and ServerAck remain forward-compatible.
🪄 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: b56b374d-0dc4-4dee-a593-bf9aff948906
📒 Files selected for processing (3)
src/client/node_io.rssrc/client/tests.rswacore/src/types/events.rs
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: Adds new Event::ServerAck to core ack path. Though additive and gated, it modifies critical handler logic and introduces a new public event variant, requiring human review for safety.
Re-trigger cubic
|
Thank you <3 |
2 participants
Motivation
The ack path resolves the internal
response_waitersmap (kept allocation-free by #827, reworked by #978) with no public hook, so consumers have no way to observe server acks. Two things are invisible from the outside:SendResultresolves at socket write, and receipts only cover delivery, so there is currently no way to measure when the server accepted a sent message.handle_ack_responsebut only surface aswarn!logs; a consumer cannot react to them programmatically.What this does
Adds an observe-only
Event::ServerAck(ServerAck), dispatched inhandle_ack_responsefor every server<ack>that carries an id — before and independently of the internal waiter resolution, so it never interacts with the send/phash flow.The payload is a named struct (same pattern as
Receipt(Receipt), so it can grow without breaking consumer matches):Design notes
EventBus::has_handler_for(EventKind::ServerAck), so the hot ack path allocates nothing when no handler subscribes —ack_miss_path_does_not_heap_allocatestays green.EventKind::ServerAckis appended at the end so existing bit indexes don't shift; the build-time ceiling tripwire now points at it.id/errorare read once at the top ofhandle_ack_responseand reused by the nack diagnostics, the dispatch and the waiter branch.Tests
test_ack_dispatches_server_ack_event: message ack carries class/from/t, nack carries the code with absent attrs staying empty/None, and an id-less<ack>dispatches nothing.test_ack_waiter_resolves,test_ack_without_matching_waiter,ack_miss_path_does_not_heap_allocate) unchanged and green.cargo fmt/cargo clippy -p whatsapp-rust -p wacore --testsclean;cargo test -p whatsapp-rust --lib944 passed.