Repository navigation
fix(chat-store): correlate amendment operation acknowledgements - #236
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (11)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe chat store now bounds deferred acknowledgements and operation-ID correlation, and reports acknowledgement admission loss through flush and stop barriers. WhatsApp send paths handle local recording failures and register operation IDs for acknowledgement matching. ChangesChat-store acknowledgement flow
Priority: ⬇️ Low Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant WhatsApp as WhatsApp send path
participant ChatStore
participant writer
participant server
participant apply_event
WhatsApp->>ChatStore: record_outgoing
ChatStore->>writer: enqueue outgoing row
WhatsApp->>server: send message
server-->>apply_event: ServerAck
apply_event->>writer: apply_server_ack_with_operations
writer->>writer: retry matching deferred ACK after row insertion
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change makes outgoing sends fail early when local recording is refused. It also bounds and correlates acknowledgements for edit, revoke, reaction and similar operations. No actionable merge-blocking risk was found. Confirm that final-head CI passes before merging. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 71.60% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 81 functions across 10 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
c71d4c4 to
91c555b
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Keep admission loss visible when a later barrier fails, while assigning a fresh response generation so an older result cannot clear newer failures. Cover cancellation and distinct failure generations with regressions.
Bring PR #235's cancellation regression fix into the operation-ACK branch without rewriting its history.
![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: 743df6cbe8
ℹ️ 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".
Keep a bounded summary of the first unseen error and permanent write/ACK loss. Cover a cancelled backend error followed by writer overflow through the public flush API.
Incorporate the stacked base fix, propagate deferred ACK admission loss into the retained barrier outcome, and regress cancellation before a later ACK-capacity failure.
Retain write admission loss in this branch. The stacked operation-ACK branch adds ACK-loss reporting and its public API regression; do not promise that extension in the base documentation or helper.
Keep the operation-ACK branch's tested source tree unchanged. The base now exposes writer loss only; this branch extends the retained outcome with ACK-loss reporting.
Capture writer rejection under a short lock when a control barrier is queued, leaving later rejection for the next barrier. Keep the batch failure alongside the captured loss flag. Regress both flush/close diagnostics and a rejection following an already-queued flush.
Preserve the ACK-loss extension while incorporating causal barrier snapshots and batch-error diagnostics, with the base regressions.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef1df375dc
ℹ️ 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".
Keep the first writer error across refused inbound durability callbacks and subsequent retries. Regress suspension, repair and delivery of the original transaction cause without a prior flush.
Preserve the base writer failure. Make the enqueue-only ID announcement follow outgoing record admission, with synthetic channel/store coverage for accepted, full and stopped writer cases.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ffe500695
ℹ️ 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}
1 participant
Amendment ACKs use fresh stanza IDs, separately from the target message ID. This change correlates known operations, prevents unknown operation bursts from evicting already-admitted early message ACKs, and refuses normal message sends whose outgoing record was not admitted.
Refs #231. #235 has been merged into main as
09bfb1f. This PR now targets main. Merge commit174712bincorporates that base without rewriting branch history and preserves all operation ACK, writer-loss and enqueue-only fixes. Engine and Cargo.lock remain unchanged.Behavior:
SendResult.message_idbefore the target recorder. List responses register the generated ID before sending; forwarding and status preserve remotely successful results while logging any post-send tracking refusal. No operation message rows are invented.send_message_with_options. UI sends report the pre-generated ID as failed; wire APIs return an error. Theenqueue_onlywire ID is announced only after successful outgoing admission. A media upload may precede final-message admission.Limits:
Validation:
174712b: 372 chat-store + 279 session tests passed (651 total), 5 ignored, no failures; formatting and Clippy across all targets/features with warnings denied passed.6a21829is byte-identical to the previously validated combined integration: 375 chat-store and 2045 native workspace tests passed, 23 ignored in the workspace, no failures. Workspace Clippy/fmt, WASM type checking and the workflow's WASM lint policy passed.Final-head CI and Pages are tracked on
174712b. At 16:04 UTC, the full Linux job passed, including workspace formatting, dependency checks, Clippy, tests, doctests, CLI budget and plugins. Windows/macOS/web/xtask/MSRV and Pages Build remain queued. The GitHub PR reports mergeable with no remaining conflicts. These current-head gates must pass before this PR is fully green.Codex completed final-head review of
174712bat 15:57 UTC without new findings. CodeRabbit reviewed the actual 11-file diff against main and APPROVED174712bat 15:59 UTC without actionable comments.No PR was merged by this agent. No structural Pages change, engine change, real-account execution or destructive migration was performed.
Summary by CodeRabbit
Bug Fixes
Documentation