feat(core): expose typed stanza responses and retries - #1069
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds typed stanza response APIs, updates ACK/NACK wire construction and error handling, introduces allocation-free display comparisons, and refactors retry processing around atomic counters, typed outcomes, and policy-driven key inclusion. ChangesTyped stanza responses
Retry processing
Supporting maintenance
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Client
participant RetryPipeline
participant PortableCache
participant Transport
Caller->>Client: request_message_retry(stanza, options)
Client->>RetryPipeline: validate and process retry
RetryPipeline->>PortableCache: atomically increment retry count
RetryPipeline->>Transport: send retry receipt or PDO request
Transport-->>Caller: typed outcome or error
Possibly related PRs
Suggested labels: 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/message/retry.rs | Adds explicit retry requests and centralizes automatic and manual retry handling. |
| src/message/receive.rs | Uses shared envelope selection and monotonic sender retry-count pre-seeding. |
| src/portable_cache.rs | Adds atomic by-reference cache updates for retry-count reconciliation. |
| src/retry.rs | Extends retry receipt construction, outcomes, key handling, and durability behavior. |
| src/features/stanza.rs | Defines the public typed stanza response and retry API. |
Reviews (8): Last reviewed commit: "fix(core): harden explicit stanza operat..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
![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: 7bf9dae72f
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/node_io.rs (1)
597-696: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFour near-identical snapshot→encode→drop blocks — let's not repeat ourselves four times.
send_ack_for,acknowledge_stanza,send_transport_ack, andspawn_node_transport_ackall do the same "snapshot device, callencode_ack_bytes, drop snapshot" dance with slightly different error handling. Every one of these is a spot where a future edit could forget thedrop(device)or mis-order it. Worth pulling into one private helper that returnsResult<Vec<u8>, StanzaResponseError>and let each caller decide how to handle the error (log-and-return vs propagate).♻️ Sketch of the shared helper
fn encode_ack_from_snapshot( &self, node: &wacore_binary::NodeRef<'_>, ) -> Result<Vec<u8>, crate::features::StanzaResponseError> { let device = self.persistence_manager.get_device_snapshot(); let result = encode_ack_bytes(node, device.pn.as_ref()); drop(device); result }Not blocking — the current code is correct, just repetitive.
🤖 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 597 - 696, Add a private helper near the ack methods, such as encode_ack_from_snapshot, that obtains the device snapshot, calls encode_ack_bytes, and releases the snapshot before returning its Result<Vec<u8>, StanzaResponseError>. Update send_ack_for, acknowledge_stanza, send_transport_ack, and spawn_node_transport_ack to use this helper while preserving each method’s existing error handling and logging behavior.
🤖 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.rs`:
- Around line 1047-1062: Update value_refs_display_equal so the Jid–Jid branch
compares the fields that determine their wire display, respecting the documented
agent suppression rules used by Jid::is_same_chat_as instead of relying on
derived Jid equality. Keep the comparison allocation-free and preserve the
existing String–String and cross String/Jid behavior so encode_ack_bytes and
build_ack_node correctly drop participants matching from by display form.
In `@tests/e2e/tests/memory_soak.rs`:
- Around line 208-215: Update the heap-growth calculation in the dhat-heap
reporting block to use a signed difference instead of saturating_sub, allowing
decreases to be represented. Format the delta with an explicit sign while
preserving the existing first and last heap byte values in the Tracked heap log.
---
Outside diff comments:
In `@src/client/node_io.rs`:
- Around line 597-696: Add a private helper near the ack methods, such as
encode_ack_from_snapshot, that obtains the device snapshot, calls
encode_ack_bytes, and releases the snapshot before returning its Result<Vec<u8>,
StanzaResponseError>. Update send_ack_for, acknowledge_stanza,
send_transport_ack, and spawn_node_transport_ack to use this helper while
preserving each method’s existing error handling and logging behavior.
🪄 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: a55a239d-8352-4b3f-9765-b89da3c68c9e
📒 Files selected for processing (23)
src/client.rssrc/client/node_io.rssrc/client/tests.rssrc/features/mod.rssrc/features/stanza.rssrc/lib.rssrc/message.rssrc/message/retry.rssrc/message/tests.rssrc/portable_cache.rssrc/prekeys.rssrc/receipt.rssrc/retry.rstests/e2e/tests/app_state.rstests/e2e/tests/chat_actions.rstests/e2e/tests/memory_soak.rstests/e2e/tests/messaging.rstests/e2e/tests/profile.rswacore/binary/src/jid.rswacore/binary/src/node.rswacore/src/message_processing.rswacore/src/protocol/nack.rswacore/src/protocol/retry.rs
There was a problem hiding this comment.
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 (2)
src/message/tests.rs (2)
5567-5582: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winUse reserved fictitious NANP JIDs in these fixtures.
The new
1555...values use555as the NPA. Replace them with the repository’s fictional format, e.g.12025550111, and update matching assertions.Based on learnings, fictitious NANP test numbers must use
1+ a real NPA +555+ a line number from0100through0199.Also applies to: 5664-5666, 5674-5674, 5690-5693, 5711-5717, 6055-6096
🤖 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/message/tests.rs` around lines 5567 - 5582, Replace all fictitious NANP JIDs and matching assertions in the affected fixtures with the repository format: 1 + a real NPA + 555 + a line number from 0100 through 0199, such as 12025550111. Update every corresponding participant, sender, recipient, and expected-value reference across the affected tests.Source: Learnings
3612-3662: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBind this test to
MAX_DECRYPT_RETRIES.The task count, expected successes/failures, returned values, and final count all hardcode
5. Derive them fromMAX_DECRYPT_RETRIESso this atomicity test remains valid if retry policy changes.Proposed fix
- // Spawn 10 concurrent increment tasks + let max_retries = usize::from(MAX_DECRYPT_RETRIES); + let attempts = max_retries * 2; let mut tasks = JoinSet::new(); - for _ in 0..10 { + for _ in 0..attempts { ... - 5, + max_retries, ... - 5, + attempts - max_retries, ... - vec![1, 2, 3, 4, 5], + (1..=MAX_DECRYPT_RETRIES).collect::<Vec<_>>(), ... - assert_eq!(final_count, Some(5), "Final count should be capped at 5"); + assert_eq!(final_count, Some(MAX_DECRYPT_RETRIES));🤖 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/message/tests.rs` around lines 3612 - 3662, Update the concurrent increment test around increment_retry_count to derive the task count, expected successful and failed result counts, returned value range, and final count from MAX_DECRYPT_RETRIES instead of hardcoded 5 values. Preserve the existing atomicity assertions and adapt the task count so it still exercises both successful and exhausted increments when the retry limit changes.
🤖 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 `@wacore/binary/src/jid.rs`:
- Around line 1213-1219: Standardize both fictional NANP JID fixtures by
replacing 15551234567 with 12025550111 in wacore/binary/src/jid.rs lines
1213-1219 and src/client/tests.rs lines 2453-2459; retain the existing
hosted-JID display comparison in src/client/tests.rs.
---
Outside diff comments:
In `@src/message/tests.rs`:
- Around line 5567-5582: Replace all fictitious NANP JIDs and matching
assertions in the affected fixtures with the repository format: 1 + a real NPA +
555 + a line number from 0100 through 0199, such as 12025550111. Update every
corresponding participant, sender, recipient, and expected-value reference
across the affected tests.
- Around line 3612-3662: Update the concurrent increment test around
increment_retry_count to derive the task count, expected successful and failed
result counts, returned value range, and final count from MAX_DECRYPT_RETRIES
instead of hardcoded 5 values. Preserve the existing atomicity assertions and
adapt the task count so it still exercises both successful and exhausted
increments when the retry limit changes.
🪄 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: ca4f7ec3-8867-4948-8255-8c71d3245ca9
📒 Files selected for processing (8)
src/client.rssrc/client/node_io.rssrc/client/tests.rssrc/message/tests.rstests/e2e/tests/memory_soak.rswacore/binary/src/attrs.rswacore/binary/src/jid.rswacore/src/messages.rs
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: c4f2e7ddbb
ℹ️ 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".
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: d05996cd68
ℹ️ 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".
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/message/tests.rs`:
- Around line 6113-6155: Replace the sender JID fixture in
explicit_retry_while_disconnected_preserves_budget_and_prekeys with a properly
formatted fictitious NANP number: country code 1, a real non-555 NPA, exchange
555, and line number 0100–0199. Keep the sender’s device suffix and all
retry-cache assertions unchanged.
🪄 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: af5c5964-e537-4e8b-844e-7e5bf5c99d42
📒 Files selected for processing (7)
src/client/lifecycle.rssrc/message/receive.rssrc/message/retry.rssrc/message/tests.rssrc/receipt.rswacore/binary/src/attrs.rswacore/src/messages.rs
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
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: 451443e349
ℹ️ 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".
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: 435e3f72d4
ℹ️ 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".
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
1 participant
Summary
Protocol behavior
force || stateless || retry_count >= 2policy independently of the diagnostic reasonValidation
cargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-features -- -D warningscargo test --workspace --exclude e2e-tests