perf(usync): batch LID-PN re-learn from device response off the cold group-send path - #680
Conversation
…path process_device_list_response learned every LID↔PN mapping in a usync device response by awaiting add_lid_pn_mapping per entry — a serial record + DB write + migration per mapping. On a large cold group send the device usync echoes the same mappings query_info already learned (via its batched, detached learn), so the send blocked on ~N serial awaited DB writes for pairs we already had. Route it through learn_lid_pn_mappings_batch instead: one detached transaction for genuinely-new pairs, internally guarded against re-learning durable ones (same path query_info uses). Falls back to the per-mapping path if the owning Arc<Client> isn't available. Allocation is unchanged (the records are still built); the win is latency — the serial awaited writes leave the cold-send critical path. Bench (group-send, 800 members, mock server, single A/B): ack_avg 64.50 -> 55.48 ms (-14%) ack_max 139.33 -> 95.68 ms (-31%, the cold first send) pong_avg 80.56 -> 73.39 ms (-9%) CPU total unchanged (0.150 s); dhat total unchanged (~16.6 MB).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
Walkthroughprocess_device_list_response now batches LID↔phone-number persistence via learn_lid_pn_mappings_batch when Client is available, collecting pairs first; otherwise it falls back to per-mapping add_lid_pn_mapping and warns on failures. A Tokio test ensures the batched path warms lid_pn_cache synchronously before returning. ChangesBatched LID↔Phone Number Persistence
Sequence DiagramsequenceDiagram
participant Response as process_device_list_response
participant Weak as self_weak
participant Client
participant Batch as learn_lid_pn_mappings_batch
participant Fallback as add_lid_pn_mapping
Response->>Response: collect (lid, pn) pairs
Response->>Weak: upgrade to Client
alt Client obtained
Weak-->>Client: owned reference
Response->>Batch: call with collected pairs
Batch-->>Response: batch persist complete
else Client unavailable
Weak-->>Response: None
Response->>Response: log warning
loop per mapping
Response->>Fallback: persist individual mapping
Fallback-->>Response: result
Response->>Response: warn on failure
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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: 6fe7a1dd7d
ℹ️ 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: 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/usync.rs`:
- Around line 84-87: The warn! call is logging raw PII (mapping.lid and
mapping.phone_number); replace those with non-identifying representations (e.g.,
deterministic hash or masked/redacted string) before logging. Update the
usync.rs code path that emits warn! to compute a safe identifier (e.g.,
hash_and_truncate(mapping.lid) and hash_and_truncate(mapping.phone_number) or
mask_last_n()) and use those values in the warn! message instead of
mapping.lid/mapping.phone_number so logs never contain raw LIDs or phone
numbers.
🪄 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
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2f5d8042-7ab8-4040-9016-4d7de2337fd8
📒 Files selected for processing (1)
src/usync.rs
|
@claude review |
|
Claude finished @jlucaso1's task in 3m 0s —— View job Code Review
SummaryThe change is well-motivated, narrowly scoped, and follows an already-established pattern: The dedup-by-phone + The Findings1. Codex P1 (ordering): real-but-narrow concern, worth a direct response from the author. The race shape Codex describes is real in theory: the previous That said, two important mitigations limit the blast radius:
Practically, that narrows the concern to standalone usyncs that are not preceded by Suggested action: acknowledge this in the PR description or a one-line code comment noting the standalone-usync window, so a future reader understands why the fallback path is kept and what guarantee was traded for the latency win. No need to revert; the existing locking discipline is the actual correctness barrier. 2. CodeRabbit P1 (PII in warn log) — pre-existing, not introduced here. The
So this is a pre-existing log hygiene issue, not a regression. Reasonable to defer to a separate redaction-pass PR that covers the whole module consistently rather than tackling one line here. 3. Test coverage — minor gap, optional. The new batched path in Nits
VerdictSolid latency-only optimization with measurements that line up with the mechanism. The ordering concern Codex raises is real but limited to a narrow window the codebase's locking already partially covers, and not strictly a regression for the cold-group-send path the PR targets. I'd merge with (optionally) a one-line comment acknowledging the standalone-usync ordering window, and leave the PII-log cleanup for a focused follow-up. |
Benchmark Results67 unchanged benchmark(s)
|
Addresses the code review on #680: - Reword the comment to the invariant (route usync mappings through the same batched learner query_info uses) and add an ordering note: detaching the migration trades the awaited PN->LID migration ordering on standalone usync paths (sync_own_device_list / flush_pending_device_sync), but the per-address session_lock_for both the migration and encrypt take is the real barrier (no interleave); the group-send path is unchanged since query_info already learns these pairs detached upstream. No revert (per review). - Add process_response_warms_lid_pn_cache_synchronously: non-empty lid_mappings is resolvable in-cache the moment the call returns (persist runs detached), locking the new path's synchronous-warm contract. PII-in-log finding skipped: pre-existing (verbatim from the prior loop) and out of scope per the owner.
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/usync.rs`:
- Around line 466-497: The test
process_response_warms_lid_pn_cache_synchronously is insufficient because it
never forces the batched path (learn_lid_pn_mappings_batch) to run detached —
both learn_lid_pn_mappings_batch and add_lid_pn_mapping currently warm
lid_pn_cache before returning, so the test can pass even when self_weak fails to
upgrade and falls back to the slow path. Modify the test to assert or arrange
that the batched branch is taken: ensure the fixture (create_test_client /
Client) can provide a valid Arc<Client> upgrade from self_weak (or otherwise
mock/spy the client so learn_lid_pn_mappings_batch is invoked), and then verify
the detached behavior (for example, by stubbing/instrumenting
learn_lid_pn_mappings_batch to run asynchronously and ensuring lid_pn_cache is
not populated until the detached task runs) rather than relying on
add_lid_pn_mapping; reference process_device_list_response,
learn_lid_pn_mappings_batch, add_lid_pn_mapping, lid_pn_cache and self_weak when
making the change.
🪄 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
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1753d832-b241-4895-b9c0-b46b73cb17dd
📒 Files selected for processing (1)
src/usync.rs
The test could have passed via the per-mapping fallback (both paths warm the cache). create_test_client populates self_weak (client.rs sets it right after the Arc is built), so the batched branch is in fact the one taken; assert the self_weak upgrade to pin that deterministically. Skipped the reviewer's "verify cache is not populated until the detached task runs" suggestion: the batched path warms the cache synchronously by design (record_lid_pn_in_memory is sync; only the persist is detached), so that assertion would contradict the intended behavior the test locks.
1 participant
Context
Continued group-send benchmark profiling against the current main (post #677/#678/#679). The big bottleneck —
migrate_signal_sessions_on_lid_discovery(10 GB on an 800-member cold send) — is already fixed (#677): the cold send now allocates only ~16.6 MB. Profiling the remainder surfaced one clean, latency-only redundancy.Finding
process_device_list_response(the usync device-list handler insideget_user_devices) learned each LID↔PN mapping by awaitingadd_lid_pn_mappingper entry — a serialrecord + DB write + migrationper mapping. On a large cold group send, the device usync echoes the same mappingsquery_infoalready learned moments earlier (via its batched, detachedlearn_lid_pn_mappings_batch), so the send blocked on ~N serial awaited DB writes for pairs we already had.CPU was never the issue (0.15 s total for 40 sends to 800 members); this was wall-clock latency from serial awaited
spawn_blockingSQLite hops on the send's critical path.Change
Route the usync re-learn through
learn_lid_pn_mappings_batch(the same pathquery_infouses): one detached transaction for genuinely-new pairs, internally guarded against re-learning already-durable ones. Falls back to the per-mapping path if the owningArc<Client>isn't available. No new ordering risk —query_infoalready triggers these migrations detached beforeget_user_devicesruns.Measurement (group-send bench, 800 members, mock server, single A/B)
Multiple independent metrics moved together, consistent with removing the serial awaited writes from the cold send.
Scope note
The remaining client-side cost for large groups (per-member device/LID resolution on every send:
get_current_lid/resolve_lookup_keysString churn) is O(members × messages) but CPU-cheap (0.15 s total), so it isn't pursued here. The dominant per-send latency (pong ~73 ms) is server/network fanout + the per-(group,sender) chain-lock serializing the bench flood, not client work.Tests: clippy
--all-targets -D warningsclean; whatsapp-rust lib suite (659) green; existingprocess_response_preserves_omitted_users(merge-safety) unaffected.