fix: add #[must_use] to AbortHandle and clear message queues on disconnect - #459
Conversation
…nnect - Add #[must_use] to AbortHandle so the compiler warns when a spawned task's handle is silently dropped (forces explicit .detach()) - Invalidate message_queues in cleanup_connection_state() so stale per-chat workers don't survive reconnects with outdated crypto state
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds message queue invalidation to connection cleanup logic and introduces a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d01850866
ℹ️ 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".
| // Drop per-chat message queue senders so workers exit via channel close. | ||
| // Without this, stale workers from the old connection survive reconnects | ||
| // holding outdated signal/crypto state. | ||
| self.message_queues.invalidate_all(); |
There was a problem hiding this comment.
Avoid dropping chat-queue map before workers finish
Calling self.message_queues.invalidate_all() in cleanup_connection_state removes the sender entry immediately, but existing per-chat workers keep running until their channel drains (MessageHandler loops on rx.recv() in src/handlers/message.rs). If a disconnect happens with buffered messages and reconnect is fast, the next incoming message for that chat creates a second worker/queue while the old worker is still draining, so messages for the same chat can be processed concurrently and out of order (breaking the mailbox guarantee used to preserve Signal session ordering).
Useful? React with 👍 / 👎.
Summary
Minimal, targeted fixes from the resource management audit — only the changes that provide real value without adding complexity.
#[must_use]onAbortHandle: The compiler now warns when a spawned task's handle is silently dropped (which would abort the task). Forces every spawn site to make an explicit choice:.detach()for fire-and-forget, or store the handle for lifecycle management. Zero runtime cost.message_queues.invalidate_all()incleanup_connection_state(): Drops per-chat message queue senders on disconnect so workers exit via channel close. Without this, stale workers from the previous connection survived reconnects holding outdated signal/crypto state.Why not TaskTracker?
The original approach (PR #458) added a
TaskTrackercontainer for structured task lifecycle management. After thorough review, this was overengineered — the existing cooperative cancellation viaconnection_generation,shutdown_notifier, andis_shutting_down()already handles task lifecycle correctly (matching WhatsApp Web's AbortController pattern). The TaskTracker added complexity (Arc reference cycles, unbounded handle growth, abort timing issues with 515 reconnect cycles) without improving stability.Test plan
cargo clippy --all --tests— zero warningscargo test --all --exclude e2e-tests— all pass#[must_use]produces compiler warning on unhandledruntime.spawn()resultmessage_queues.invalidate_all()placement is afteris_connected = false(workers see disconnected state on any in-flight sends)Summary by CodeRabbit