perf: granular cache patching instead of invalidate+refetch - #382
Conversation
Replace cache invalidation with in-place patching for device and group notifications, matching WhatsApp Web's addParticipantInfo / removeParticipantInfo / bulkCreateOrReplace patterns. Device notifications (type="devices"): - Add: append device to cached Vec<Jid> + DeviceListRecord - Remove: filter out device from both caches - Update: update key_index in DeviceListRecord Group notifications (type="w:gp2"): - Add: extend GroupInfo.participants + update LID-PN maps - Remove: filter GroupInfo.participants + clean LID-PN maps Client-initiated (Groups::add/remove_participants): - Patch cached GroupInfo inline instead of invalidating All patches are no-ops if the cache entry doesn't exist — the next read fetches fresh from the backend. Zero risk of stale data since the notification carries the exact diff. Eliminates a full IQ round-trip on every participant/device change.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds granular in-memory cache patching: three device patch APIs, group participant add/remove mutations on cached GroupInfo, and notification handler updates to call these patch methods instead of invalidating full caches; tests added for device and group behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant Notification as Notification Service
participant Handler as Notification Handler
participant Client as Client (patch APIs)
participant Cache as In-memory Caches
participant EventBus as Event Dispatcher
Notification->>Handler: deliver device/group notification
Handler->>Client: determine per-item action (add / remove / update)
Client->>Cache: read current cache entry
Cache-->>Client: return cached DeviceInfo / GroupInfo or miss
Client->>Cache: write patched entry (add / remove / update)
Client->>EventBus: emit DeviceListUpdate / GroupUpdate
EventBus-->>Handler: subscribers receive update
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
|
@claude review |
|
Claude finished @jlucaso1's task in 1m 26s —— View job Code Review: Granular Cache Patching
Overall this is a clean, well-structured optimization. The approach of patching caches in-place instead of invalidate+refetch is sound and correctly mirrors the WhatsApp Web patterns. The safety property (no-op on cache miss) is the right call. A few observations: Looks Good
Minor Issues
ArchitectureThe design correctly separates concerns:
This makes the mutation logic easily testable (and indeed well-tested) without needing async runtime or client infrastructure. Verdict: This PR looks good to merge. The approach is sound, the edge cases are handled correctly, and the test coverage is solid. The minor issues noted above are all low-risk and don't block merging. |
Bencher Report![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/features/groups.rs (1)
230-240:⚠️ Potential issue | 🟠 MajorPatch only the participants the server actually accepted.
Both branches ignore
resultand mutateGroupInfowith every requested JID. Partial add/remove failures will leave the cache ahead of the authoritative group state until someone invalidates it manually. Filter the patch to successfulParticipantChangeResponses, or invalidate on any non-success.Also applies to: 249-259
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/groups.rs` around lines 230 - 240, The current code patches GroupInfo with every requested JID regardless of the server response; change the update logic after self.client.execute(AddParticipantsIq::new(jid, participants)).await? so it only applies participants that the server accepted (inspect result for successful ParticipantChangeResponse entries) and add only those to GroupInfo via info.add_participants(&new_accepted). If the result indicates any non-success for the request, instead avoid a partial patch and invalidate the cache entry (e.g., remove or refresh the GroupInfo) rather than applying a subset; apply the same fix for the corresponding remove-participants branch that also reads result and calls info.remove_participants.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/device_registry.rs`:
- Around line 229-236: patch_device_add and patch_device_remove currently only
update cache entry for from_jid.to_non_ad(), leaving the other alias (PN/LID)
stale; change both functions to resolve all cached aliases for the user (e.g.,
both PN and LID forms) via the same lookup used by invalidate_device_cache and
iterate over those keys from get_device_cache(), applying the same add/remove
logic to every existing cache entry (check devices.iter().any(...), push or
retain accordingly, then insert back) so both aliases are patched consistently.
- Around line 241-249: The current logic mutates the in-memory
device_registry_cache (e.g., the block that gets record from
self.device_registry_cache.get(...) and then pushes a new
wacore::store::traits::DeviceInfo and calls
self.device_registry_cache.insert(...)) but does not persist the updated
DeviceListRecord, causing has_device() to fall back to stale
persistence_manager.backend() after cache
eviction/invalidate_device_cache()/restart. Replace the cache-only insert with a
call to update_device_list() to persist the patched DeviceListRecord (same
change needed for the similar blocks at the other occurrences referenced around
the 272-279 and 294-299 ranges), ensuring the code constructs the updated
DeviceListRecord and invokes update_device_list(key, updated_record) instead of
only calling device_registry_cache.insert(...).
In `@src/features/groups.rs`:
- Around line 235-239: The get→mutate→insert on TypedCache for group updates
(using self.client.get_group_cache(), group_cache.get(jid),
info.add_participants(...) and group_cache.insert(...)) is racy; wrap the
read-modify-write inside the per-chat lock provided by Client::chat_locks (use
the lock keyed by jid) or call the client's atomic mutate helper so updates to
the same group serialize; apply the same change to the similar block around the
other occurrence (the code around lines 254–258) so both notification-driven and
client-initiated add/remove paths perform the mutation while holding the
per-group lock.
In `@src/handlers/notification.rs`:
- Around line 345-351: The Update branch currently loops patching each device
via client.patch_device_update(notification.user(), device).await but doesn't
handle hash-only updates where op.devices may be empty; change the
DeviceNotificationType::Update handling to check if op.devices.is_empty() and,
if so, call invalidate_device_cache(notification.user()).await as the fallback,
otherwise iterate and call client.patch_device_update for each device (keeping
existing notification.user() usage).
In `@wacore/src/client/context.rs`:
- Around line 87-98: The loop in Context (the for (jid, phone_number) in new {
... }) skips already-cached participants before updating
lid_to_pn_map/pn_to_lid_map, so a later phone_number (None -> Some) never
backfills maps; change the logic to update the maps when addressing_mode ==
AddressingMode::Lid and phone_number.is_some() even if the participant already
exists (i.e., move the map-update block ahead of the continue or perform it
unconditionally before the continue), and add a regression test that exercises
Groups::add_participants() where the first insert uses (jid, None) and a
subsequent call provides Some(phone_number) and asserts that
phone_device_jid_to_lid() returns the expected mapping.
---
Outside diff comments:
In `@src/features/groups.rs`:
- Around line 230-240: The current code patches GroupInfo with every requested
JID regardless of the server response; change the update logic after
self.client.execute(AddParticipantsIq::new(jid, participants)).await? so it only
applies participants that the server accepted (inspect result for successful
ParticipantChangeResponse entries) and add only those to GroupInfo via
info.add_participants(&new_accepted). If the result indicates any non-success
for the request, instead avoid a partial patch and invalidate the cache entry
(e.g., remove or refresh the GroupInfo) rather than applying a subset; apply the
same fix for the corresponding remove-participants branch that also reads result
and calls info.remove_participants.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5d73db44-eff8-46fc-8eef-5885b54dd5f3
📒 Files selected for processing (4)
src/client/device_registry.rssrc/features/groups.rssrc/handlers/notification.rswacore/src/client/context.rs
| let group_cache = self.client.get_group_cache().await; | ||
| if let Some(mut info) = group_cache.get(jid).await { | ||
| let new: Vec<_> = participants.iter().map(|p| (p.clone(), None)).collect(); | ||
| info.add_participants(&new); | ||
| group_cache.insert(jid.clone(), info).await; |
There was a problem hiding this comment.
Serialize group cache patching per group.
This is a non-atomic get() → mutate → insert() on TypedCache. If a notification patch races with a client-initiated add/remove, both paths can read the same GroupInfo and the last insert drops the other delta. Use a per-group lock or a dedicated atomic mutate helper here.
As per coding guidelines, Use Client::chat_locks to serialize per-chat operations for concurrency safety.
Also applies to: 254-258
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/groups.rs` around lines 235 - 239, The get→mutate→insert on
TypedCache for group updates (using self.client.get_group_cache(),
group_cache.get(jid), info.add_participants(...) and group_cache.insert(...)) is
racy; wrap the read-modify-write inside the per-chat lock provided by
Client::chat_locks (use the lock keyed by jid) or call the client's atomic
mutate helper so updates to the same group serialize; apply the same change to
the similar block around the other occurrence (the code around lines 254–258) so
both notification-driven and client-initiated add/remove paths perform the
mutation while holding the per-group lock.
- Patch all PN/LID aliases in device_cache via jids_for_lookup helper, not just from_jid.to_non_ad() (CodeRabbit) - Persist patched DeviceListRecords to backend via update_device_list so changes survive cache eviction/restart (CodeRabbit) - Fall back to invalidate on hash-only device Update notifications where op.devices is empty (CodeRabbit) - Backfill LID-PN maps even for existing participants: move map update before the dedup check so None→Some(phone) fills correctly when server notification follows client-initiated add (CodeRabbit) - Document get→mutate→insert race in group cache patching as acceptable for a best-effort cache (CodeRabbit) - Add regression test for LID map backfill case (CodeRabbit)
Filter ParticipantChangeResponse by status="200" before patching GroupInfo. Partial failures (403, 409) no longer leave the cache ahead of the authoritative group state. (CodeRabbit outside-diff)
1 participant
Summary
addParticipantInfo/removeParticipantInfo/bulkCreateOrReplacepatterns from captured JSBefore → After
Changes
wacore/src/client/context.rs—GroupInfo::add_participants()andremove_participants()mutation methods with LID-PN map maintenance + 7 unit testssrc/client/device_registry.rs—patch_device_add(),patch_device_remove(),patch_device_update()methods + 6 unit testssrc/handlers/notification.rs— Device notification handler now patches instead of invalidating; group notification handler patchesGroupInfoin-place for add/remove actionssrc/features/groups.rs—Groups::add_participants()andremove_participants()patch cache inline instead of invalidatingSafety
All patches are no-ops if the cache entry doesn't exist — the next read fetches fresh from the backend. Zero risk of stale data since the notification carries the exact diff.
Test plan
cargo clippy --all-targetscleancargo fmt --allcleanSummary by CodeRabbit
New Features
Tests