fix(signal): serialize sender-key mutations - #1043
Merged
Merged
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:
📝 WalkthroughWalkthroughSender-key mutations now serialize through per-chain locks, while rotation and recovery paths use group-distribution locks. Device-tracking resets are centralized, cache flushing occurs after lock release, Jid-based rotation APIs are wired through callers, and contention and failure tests cover the updated ordering. ChangesSender-key serialization and rotation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GroupEvent
participant Client
participant DistributionLock
participant ChainLock
participant SignalCache
participant PersistenceManager
GroupEvent->>Client: trigger sender-key rotation
Client->>DistributionLock: acquire group_distribution_lock
Client->>ChainLock: wait for sender-key chain mutation
Client->>SignalCache: delete sender-key state
Client->>PersistenceManager: reset sender_key_devices
Client-->>DistributionLock: release group lock
Client->>SignalCache: flush cache batch
Possibly related PRs
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 |
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
|
| Filename | Overview |
|---|---|
| src/client/sender_keys.rs | Core change: adds reset_sender_key_device_tracking with durable clear + cold-mark fallback, refactors rotation functions to accept &Jid, and extracts rotate_own_sender_key_state to run under the distribution lock. Lock ordering (distribution → chain) is consistent and correct throughout. |
| src/features/signal.rs | Adds chain lock acquisition inside decrypt_group_message with explicit drop before the flush, matching the intended lock ordering (chain released before flush to avoid holding it during I/O). Docstring correctly updated. |
| src/message/special.rs | Adds chain lock to SKDM ingest (handle_sender_key_distribution_message); guard is held only during process_sender_key_distribution_message and released at function return — no flush is called afterward, so no unnecessary lock extension. |
| src/send/mod.rs | Distribution guard now wraps the entire group/status send path; reset_sender_key_device_tracking is called before creating a new chain (both cold and warm paths); phash-mismatch path correctly moves cache invalidation into the durable reset. Changes are logically consistent across all four modified send sub-paths. |
| src/retry.rs | Unknown-participant rotation now acquires distribution lock before clearing and uses durable reset; flush_signal_cache_batch_safe_logged correctly moved outside the distribution lock to persist key deletions before the retry handler returns. |
| wacore/src/store/signal_cache.rs | delete_sender_key now acquires the chain lock before mutating the cache, preventing a rotation from overwriting a state written by an in-flight encrypt/decrypt. Lock order (chain → sender_keys mutex) is consistent with all callers. |
| src/portable_cache.rs | Adds CapacityStats and saturating eviction/block counters; capacity_stats() reads under a read lock; eviction guard prevents evicting live lanes. Logic is correct and the new tests in client/tests.rs exercise the edge cases. |
| src/handlers/notification/device.rs | Identity-change handler acquires distribution guard for status@broadcast before deleting own sender keys, preventing a concurrent status send from marking participants with stale has_key=true rows. |
| src/store/persistence_manager.rs | Test-only fault-injection fields (AtomicBool) added under #[cfg(test)] to simulate clear and status-write failures; correctly gated out of production builds. |
| src/test_utils.rs | Adds wait_for_lock_waiter with a 5-second tokio::time::timeout to detect lock contention in tests; robust implementation appropriate for CI environments. |
| wacore/libsignal/src/protocol/group_cipher.rs | Doc comments updated on group_decrypt, process_sender_key_distribution_message, and create_sender_key_distribution_message to document the chain lock precondition. No logic changes. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Send as send_group_message
participant Rotate as force_rotate_own_sender_key
participant DistLock as distribution_lock (group X)
participant ChainLock as chain_lock (own key, group X)
participant SignalCache as SignalStoreCache
participant DB as PersistenceManager
Note over Send,DB: Serialized own-key path (distribution lock → chain lock)
Send->>DistLock: acquire distribution_guard
DistLock-->>Send: held
Send->>DB: reset_sender_key_device_tracking (DB clear + cache invalidate)
Send->>SignalCache: delete_sender_key → acquires chain_lock internally
ChainLock-->>SignalCache: held
SignalCache->>SignalCache: remove from in-memory cache
SignalCache-->>ChainLock: release
Send->>SignalCache: group_encrypt / create_skdm (under chain_lock)
Send->>DB: update_sender_key_devices
Send-->>DistLock: drop distribution_guard
Send->>SignalCache: flush (persist key + tracker to DB)
Note over Rotate,DB: Rotation waits for any in-flight encrypt
Rotate->>DistLock: acquire distribution_guard
DistLock-->>Rotate: held (blocks any concurrent send)
Rotate->>SignalCache: delete_sender_key → tries to acquire chain_lock
ChainLock--xRotate: blocked (in-flight group_encrypt holds it)
Note over Rotate: waits…
ChainLock-->>Rotate: released after encrypt completes
Rotate->>DB: reset_sender_key_device_tracking
Rotate-->>DistLock: drop distribution_guard
Rotate->>SignalCache: flush
Note over Send,DB: Other-key ops serialized by chain lock only (no distribution lock)
Send->>ChainLock: acquire for inbound group_decrypt / SKDM ingest
ChainLock-->>Send: held
Send->>SignalCache: group_decrypt / process_skdm (ratchet advance)
Send-->>ChainLock: release
Send->>SignalCache: flush_signal_cache_batch_safe
%%{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 Send as send_group_message
participant Rotate as force_rotate_own_sender_key
participant DistLock as distribution_lock (group X)
participant ChainLock as chain_lock (own key, group X)
participant SignalCache as SignalStoreCache
participant DB as PersistenceManager
Note over Send,DB: Serialized own-key path (distribution lock → chain lock)
Send->>DistLock: acquire distribution_guard
DistLock-->>Send: held
Send->>DB: reset_sender_key_device_tracking (DB clear + cache invalidate)
Send->>SignalCache: delete_sender_key → acquires chain_lock internally
ChainLock-->>SignalCache: held
SignalCache->>SignalCache: remove from in-memory cache
SignalCache-->>ChainLock: release
Send->>SignalCache: group_encrypt / create_skdm (under chain_lock)
Send->>DB: update_sender_key_devices
Send-->>DistLock: drop distribution_guard
Send->>SignalCache: flush (persist key + tracker to DB)
Note over Rotate,DB: Rotation waits for any in-flight encrypt
Rotate->>DistLock: acquire distribution_guard
DistLock-->>Rotate: held (blocks any concurrent send)
Rotate->>SignalCache: delete_sender_key → tries to acquire chain_lock
ChainLock--xRotate: blocked (in-flight group_encrypt holds it)
Note over Rotate: waits…
ChainLock-->>Rotate: released after encrypt completes
Rotate->>DB: reset_sender_key_device_tracking
Rotate-->>DistLock: drop distribution_guard
Rotate->>SignalCache: flush
Note over Send,DB: Other-key ops serialized by chain lock only (no distribution lock)
Send->>ChainLock: acquire for inbound group_decrypt / SKDM ingest
ChainLock-->>Send: held
Send->>SignalCache: group_decrypt / process_skdm (ratchet advance)
Send-->>ChainLock: release
Send->>SignalCache: flush_signal_cache_batch_safe
Reviews (6): Last reviewed commit: "fix(signal): serialize sender-key mutati..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
jlucaso1
force-pushed
the
fix/sender-key-mutation-locking
branch
from
July 15, 2026 20:14
c483340 to
98f26ea
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/device_registry.rs`:
- Around line 2837-2877: The concurrency tests around the sender-key rotation
and the analogous block near the second referenced range do not
deterministically prove the spawned task reaches the contested operation. Add a
synchronization latch or blocking store hook immediately before the protected
mutation, await that signal before asserting the in-flight state, and release it
only after the competing operation is exercised; remove reliance on the 30 ms
sleep while preserving the existing lock-contention assertions.
In `@src/client/sender_keys.rs`:
- Around line 174-181: The sender-key rotation paths must keep tracking
force-cold when durable tracker clearing fails: in src/client/sender_keys.rs
lines 174-181, update the clear_sender_key_devices handling to propagate or
retry failures, or persist a force-cold rotation generation instead of
invalidating the cache after a failed clear; in src/send/mod.rs lines 1345-1375,
ensure deletion and cache flushing are not treated as a substitute for durable
tracker clearing, preserving force-cold behavior until the tracker state is
durably cleared.
In `@src/retry.rs`:
- Line 395: Update the flow around group_distribution_lock so that, after the
guard is released, it immediately calls flush_signal_cache_batch_safe_logged
before any early returns at the affected paths, ensuring the rotation tombstone
is durably persisted before persist_signal_state_pre_wire or return.
🪄 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: b214def9-6979-4148-b864-b965b60a9f9f
📒 Files selected for processing (13)
src/client/device_registry.rssrc/client/sender_keys.rssrc/features/groups.rssrc/features/signal.rssrc/handlers/notification/device.rssrc/handlers/notification/groups.rssrc/message/special.rssrc/message/tests.rssrc/retry.rssrc/send/mod.rswacore/libsignal/src/protocol/group_cipher.rswacore/libsignal/src/protocol/storage/traits.rswacore/src/store/signal_cache.rs
![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: 98f26eab81
ℹ️ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/handlers/notification/device.rs (1)
270-281: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftReset status SKDM tracking within this rotation generation.
The guard protects only key deletion; existing
has_key=truerows survive. If the next status send persists its new key but fails before updating tracking, later sends can treat recipients as warm and omit the new SKDM.Use one locked rotation primitive that retires the key, durably clears—or force-colds—the tracker, invalidates its cache, then releases the guard before flushing. The PR objective requires key retirement and tracker reset to remain in one distribution generation.
🤖 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/handlers/notification/device.rs` around lines 270 - 281, Update the status rotation flow around group_distribution_lock and delete_sender_key to use a single locked rotation primitive that retires each recipient’s sender key and durably clears or force-colds the corresponding has_key tracker, including cache invalidation. Keep both operations within the same distribution generation, then release the guard before any flush or follow-up work.
🤖 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.
Outside diff comments:
In `@src/handlers/notification/device.rs`:
- Around line 270-281: Update the status rotation flow around
group_distribution_lock and delete_sender_key to use a single locked rotation
primitive that retires each recipient’s sender key and durably clears or
force-colds the corresponding has_key tracker, including cache invalidation.
Keep both operations within the same distribution generation, then release the
guard before any flush or follow-up work.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b80f9642-41cd-4648-b758-3a00ff7453b7
📒 Files selected for processing (13)
src/client/device_registry.rssrc/client/sender_keys.rssrc/features/groups.rssrc/features/signal.rssrc/handlers/notification/device.rssrc/handlers/notification/groups.rssrc/message/special.rssrc/message/tests.rssrc/retry.rssrc/send/mod.rswacore/libsignal/src/protocol/group_cipher.rswacore/libsignal/src/protocol/storage/traits.rswacore/src/store/signal_cache.rs
jlucaso1
force-pushed
the
fix/sender-key-mutation-locking
branch
from
July 15, 2026 20:46
98f26ea to
545fcc2
Compare
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/client/device_registry.rs`:
- Around line 1042-1050: Consolidate the duplicated async wait_for_lock_waiter
test helper: retain the canonical definition in src/client/device_registry.rs
lines 1042-1050 or move it into shared test utilities, and update callers there
to use the shared symbol. Remove the duplicate definition from src/send/mod.rs
lines 2267-2276 and import the shared helper, preserving its timeout,
strong-count polling, and yield 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: efbbfd92-39bc-4b71-96fe-af0c044a2cab
📒 Files selected for processing (14)
src/client/device_registry.rssrc/client/sender_keys.rssrc/features/groups.rssrc/features/signal.rssrc/handlers/notification/device.rssrc/handlers/notification/groups.rssrc/message/special.rssrc/message/tests.rssrc/retry.rssrc/send/mod.rssrc/store/persistence_manager.rswacore/libsignal/src/protocol/group_cipher.rswacore/libsignal/src/protocol/storage/traits.rswacore/src/store/signal_cache.rs
jlucaso1
force-pushed
the
fix/sender-key-mutation-locking
branch
from
July 15, 2026 21:05
545fcc2 to
4369444
Compare
greptile-apps
Bot
dismissed
their stale review
July 15, 2026 21:05
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: 4369444289
ℹ️ 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/client/sender_keys.rs`:
- Around line 171-186: Update the failure log in rotate_own_sender_key_state so
it does not hard-code force_rotate_own_sender_key as the caller. Preserve
accurate caller context for both force_rotate_own_sender_key and
rotate_sender_key_on_participant_remove, using an explicit caller indicator or
equivalent propagated context when reporting reset_sender_key_device_tracking
failures.
🪄 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: 9090a622-8838-4245-aaaa-f731dc4f28bb
📒 Files selected for processing (15)
src/client/device_registry.rssrc/client/sender_keys.rssrc/features/groups.rssrc/features/signal.rssrc/handlers/notification/device.rssrc/handlers/notification/groups.rssrc/message/special.rssrc/message/tests.rssrc/retry.rssrc/send/mod.rssrc/store/persistence_manager.rssrc/test_utils.rswacore/libsignal/src/protocol/group_cipher.rswacore/libsignal/src/protocol/storage/traits.rswacore/src/store/signal_cache.rs
jlucaso1
force-pushed
the
fix/sender-key-mutation-locking
branch
from
July 15, 2026 21:34
4369444 to
ceac54c
Compare
greptile-apps
Bot
dismissed
their stale review
July 15, 2026 21:34
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
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/client/lifecycle.rs`:
- Around line 270-273: Add production telemetry for the group_distribution_locks
cache configured in the lifecycle builder, recording current cache size and
eviction rates while preserving the existing evict_guard behavior. Use the
repository’s established cache metrics/instrumentation symbols and ensure the
metrics identify this cache for monitoring against its configured capacity.
🪄 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: d0632355-c8a7-4aa9-86b7-d6059ecc2338
📒 Files selected for processing (18)
src/cache_config.rssrc/client/device_registry.rssrc/client/lifecycle.rssrc/client/sender_keys.rssrc/client/tests.rssrc/features/groups.rssrc/features/signal.rssrc/handlers/notification/device.rssrc/handlers/notification/groups.rssrc/message/special.rssrc/message/tests.rssrc/retry.rssrc/send/mod.rssrc/store/persistence_manager.rssrc/test_utils.rswacore/libsignal/src/protocol/group_cipher.rswacore/libsignal/src/protocol/storage/traits.rswacore/src/store/signal_cache.rs
jlucaso1
force-pushed
the
fix/sender-key-mutation-locking
branch
from
July 15, 2026 23:14
ceac54c to
3675760
Compare
greptile-apps
Bot
dismissed
their stale review
July 15, 2026 23:14
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Why
Sender-key records use load/mutate/store semantics. Writers that bypassed the chain lock could overwrite a newer ratchet state, and an encrypt racing a rotation could restore a retired key after deletion.
A tracker reset is DB-first and only invalidates its cache after a durable reset. If row deletion fails, it falls back to marking every existing row cold; if that also fails, the send remains fail-closed. This preserves availability for operation-specific clear failures without allowing stale
has_key=truerows onto a new chain.Status posts share the distribution lane through their final recipient marks, preventing a phash reset from racing stale marks back into the tracker. Held lanes cannot be capacity-evicted into a second mutex. Unknown-participant rotations flush their tombstone before later retry throttles or repair exits.
Client::memory_report()now exposes the lane count plus cumulative capacity evictions and blocked evictions, so operators can derive rates and tune the soft cap. The counters use the cache's existing write lock and only change under capacity pressure; they add no allocation, atomic, label cardinality, or below-cap hot-path work.The warm group path still reuses the existing
Arclocks without cloningSenderKeyRecordor allocating a new lock. The eviction scan only runs under capacity pressure, and the DB read/allocation fallback only runs after a failed clear. Test failure injectors are compiled out of production.Validation
cargo fmt --all -- --checkcargo check -p whatsapp-rustcargo clippy --all --testscargo test --workspace --exclude e2e-testswhatsapp-benchswith rebuilt, interleaved binaries (group-send, 200 members, 100 messages at 10 msg/s): 100% ACK and 9.71 msg/s median on both main and PR; client CPU 0.290s → 0.300s and peak RSS 20.9 MB → 21.0 MB, with no material regressionCloses #1037