fix(send): close LID↔PN zombie path for group prekey 406 latency spikes - #579
Conversation
|
@claude review |
|
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:
📝 WalkthroughWalkthroughInvalidate device-registry cache entries around backend deletes during canonical-key flips and PN→LID migration, pre-populate LID→PN cache from group participant data, treat HTTP 406 prekey responses as whole-batch failures, and add tests for cache invalidation and LID/PN behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Cache
participant Backend
participant Logger
Client->>Cache: invalidate(original_user / pn) (before delete)
Client->>Backend: delete_devices(original_user / pn) (async)
alt delete succeeds
Backend-->>Client: OK
Client->>Cache: invalidate(original_user / pn) (after delete)
else delete fails
Backend-->>Client: Err
Client->>Logger: warn! delete failure
Client->>Cache: invalidate(original_user / pn) (after delete)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Claude finished @jlucaso1's task in 1m 38s —— View job PR Review: fix(send): close LID↔PN zombie path for group prekey 406 latency spikes
OverallWell-analyzed root cause and clean fix. The PR description is thorough — the asymmetric key-storage diagnosis and WA Web cross-references make the rationale easy to verify. The five changes are cohesive and all address the same zombie-formation path. No correctness issues found. A few observations below, none blocking. Fix 1 —
|
![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: b466aa7a4f
ℹ️ 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".
| .learn_lid_pn_mapping_fast( | ||
| lid_user.as_str(), | ||
| &pn_jid.user, | ||
| crate::lid_pn_cache::LearningSource::Other, |
There was a problem hiding this comment.
query_info now pushes every participant phone_number into learn_lid_pn_mapping_fast, but this path does not verify that phone_number is actually a PN JID. The group parser accepts any JID for phone_number (wacore/src/stanza/groups.rs::parse_participants), so a malformed or unexpected value can poison the global LID↔PN cache and trigger incorrect migrations/deletions in device registry state. Please gate this learning call with pn_jid.is_pn() (as done later in prepare_group_stanza) before persisting/migrating.
Useful? React with 👍 / 👎.
![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 the current code and only fix it if needed.
Inline comments:
In `@src/client/device_registry.rs`:
- Around line 171-179: The TOCTOU occurs because
device_registry_cache.invalidate(&original_user).await runs before
backend.delete_devices(&original_user).await, allowing a race that can
repopulate the cache from the DB after invalidate but before delete; after a
successful backend.delete_devices(&original_user).await you must call
device_registry_cache.invalidate(&original_user).await again (or otherwise
serialize alias migration/lookups for that key) so the cache is guaranteed to
reflect the DB delete; update the code paths around
device_registry_cache.invalidate and backend.delete_devices calls (the
alias-flip/remove logic) to perform a second invalidate immediately after delete
when delete succeeds.
🪄 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: 3e3594e0-72f7-4f4a-8b89-e0155453f31d
📒 Files selected for processing (3)
src/client/device_registry.rssrc/features/groups.rswacore/src/send.rs
Benchmark Results59 unchanged benchmark(s)
|
b466aa7 to
9b643a7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/device_registry.rs (1)
170-187:⚠️ Potential issue | 🟡 MinorThe TOCTOU fix is mostly there, but the conditional second invalidation leaves a gap.
Look, I appreciate the double-invalidation approach to close the race window. But here's the thing: when
backend.delete_devices()fails (lines 175-179), you skip the second invalidation (line 181 is inside theelsebranch).The race scenario:
- First invalidate removes cache entry
- Concurrent reader sees cache miss, queries DB, finds stale row, repopulates cache
delete_devices()fails (network blip, whatever)- Second invalidate never runs
- Zombie is back in cache AND still in DB
The previous review comment suggested always doing the second invalidate. The delete failure is already logged, and the zombie entry might get cleaned up on a subsequent operation anyway. But skipping the second invalidate means you've left the cache potentially dirty.
Proposed fix
if canonical_key != original_user { self.device_registry_cache.invalidate(&original_user).await; - // Drop the DB row and re-invalidate the cache: a concurrent reader - // between the two steps can resurrect the cache from the DB row - // that's about to be deleted. if let Err(e) = backend.delete_devices(&original_user).await { warn!( "Failed to delete stale device row under {} after canonical flip: {e}", original_user ); - } else { - self.device_registry_cache.invalidate(&original_user).await; } + // Always re-invalidate after attempting delete: a concurrent reader + // may have repopulated the cache from the DB row during the window. + self.device_registry_cache.invalidate(&original_user).await; debug!(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/device_registry.rs` around lines 170 - 187, The current TOCTOU fix invalidates the cache before calling backend.delete_devices() but only runs the second invalidate in the success branch, leaving a window when delete fails; change the logic in the block that handles canonical_key != original_user so that device_registry_cache.invalidate(&original_user).await is always called a second time regardless of the result of backend.delete_devices(&original_user).await, keeping the existing warn! logging when delete_devices returns Err(e) but moving the second invalidate out of the else branch (so both invalidations occur even on error); reference device_registry_cache.invalidate, backend.delete_devices, canonical_key and original_user to locate and update the code.
🤖 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 542-550: The second cache invalidation must run regardless of
whether backend.delete_devices(pn).await succeeds to avoid the TOCTOU race;
update the LID migration block so you always call
self.device_registry_cache.invalidate(pn).await after trying
backend.delete_devices(pn).await (i.e., perform
backend.delete_devices(pn).await, log any Err from that call using the existing
warn! path, and then unconditionally call
self.device_registry_cache.invalidate(pn).await). Ensure you reference the
existing symbols device_registry_cache.invalidate and backend.delete_devices and
keep the surrounding comment about invalidating before+after delete.
In `@src/features/groups.rs`:
- Around line 212-231: The current pre-population loop calls
learn_lid_pn_mapping_fast and awaits only the in-memory update while that
function spawns a detached background task to persist to DB, creating a
potential in-memory/DB inconsistency; update the fix by either using a
synchronous/persisting API (or adding a variant that awaits persistence) instead
of learn_lid_pn_mapping_fast, or at minimum replace LearningSource::Other with a
more specific enum value like LearningSource::GroupQuery (add that enum variant
in lid_pn cache if missing) so the source is explicit for observability;
references: the loop calling learn_lid_pn_mapping_fast and the LearningSource
enum in lid_pn.rs.
---
Outside diff comments:
In `@src/client/device_registry.rs`:
- Around line 170-187: The current TOCTOU fix invalidates the cache before
calling backend.delete_devices() but only runs the second invalidate in the
success branch, leaving a window when delete fails; change the logic in the
block that handles canonical_key != original_user so that
device_registry_cache.invalidate(&original_user).await is always called a second
time regardless of the result of backend.delete_devices(&original_user).await,
keeping the existing warn! logging when delete_devices returns Err(e) but moving
the second invalidate out of the else branch (so both invalidations occur even
on error); reference device_registry_cache.invalidate, backend.delete_devices,
canonical_key and original_user to locate and update the code.
🪄 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: 1c4f5f29-2024-4f89-b015-e5fe36281276
📒 Files selected for processing (3)
src/client/device_registry.rssrc/features/groups.rswacore/src/send.rs
Field report: 23 batch 406s in 3h45m on the same group, same 7 stale
devices, causing 12-41s latency spikes per send. Root cause was an
asymmetric key-storage path in the device registry:
- `maybe_compute_skdm_devices` resolves LID participants to PN form
before usync, so `update_device_list` stores records under the PN key
when `lid_pn_cache` has no LID↔PN mapping.
- On 406, `stale_device_users` is populated with LID strings (from
`distribution_list`, LID-form in LID-mode groups).
- `invalidate_device_cache(lid)` resolves via `lid_pn_cache`; with no
mapping it returns `Unknown { user: lid }` and only invalidates the
LID key. The PN-keyed registry row survives → zombie on every send.
The LID↔PN mapping was never learned because `query_info` builds a
`lid_to_pn_map` for its own `GroupInfo` but never fed the global
`lid_pn_cache`. Silent-observer participants had no other path to
populate it. WA Web's `Create/OrReplaceDisplayNamesAndLidPnMappingsJob`
populates the cache from group data with `learningSource: "other"`.
Changes:
- `groups.rs::query_info`: push each `lid→pn` pair into `lid_pn_cache`
via `learn_lid_pn_mapping_fast` before returning.
- `wacore::send.rs::encrypt_for_devices`: drop the O(N·RTT) per-device
retry on batch 406 — the server returns all-or-nothing here, so every
individual retry also failed. Match WA Web `GroupSkmsgJob`: log,
mark `had_406=true`, continue without those devices.
- `wacore::send.rs::prepare_group_stanza`: emit both LID and PN aliases
in `stale_device_users` when the group knows the mapping, so
invalidation cleans the registry row regardless of which key form it
was stored under (defensive against races where `lid_pn_cache` isn't
populated yet).
- `device_registry.rs::update_device_list`: delete the old DB row when
the canonical key flips, not just the cache entry.
- `device_registry.rs::migrate_device_registry_on_lid_discovery`:
delete the PN-keyed DB row during migration, not just the cache.
9b643a7 to
850c81b
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/features/groups.rs (1)
212-231: 🧹 Nitpick | 🔵 TrivialThis fixes the zombie path — but
LearningSource::Otheris pretty generic.Look, this change is critical. The whole point of this PR is to close the LID↔PN zombie path, and populating
lid_pn_cachefrom group participant data ensuresinvalidate_device_cachecan resolve both aliases. That's exactly what we need.However, using
LearningSource::Otherat line 226 makes it harder to trace where mappings came from in logs and debugging. When we're debugging latency spikes at scale, observability matters. Consider adding aLearningSource::GroupQueryvariant for better traceability.Also worth noting:
learn_lid_pn_mapping_fastspawns a detached background task for DB persistence. The.awaithere only waits for the in-memory cache update. If the background persist fails, the mapping survives in memory but won't survive a restart. For this fix's purpose (current session'sinvalidate_device_cachecalls), that's acceptable — but be aware of the consistency window.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/groups.rs` around lines 212 - 231, Replace the generic LearningSource::Other used when populating the lid_pn_cache from group participants with a specific variant to improve observability: add a new enum variant LearningSource::GroupQuery to the crate::lid_pn_cache::LearningSource definition, then update the call site in the loop inside the groups code that calls learn_lid_pn_mapping_fast (the call with parameters lid_user.as_str(), &pn_jid.user, crate::lid_pn_cache::LearningSource::Other, false) to use crate::lid_pn_cache::LearningSource::GroupQuery instead; leave the existing await behavior as-is since persistence is handled by the background task spawned by learn_lid_pn_mapping_fast.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/features/groups.rs`:
- Around line 212-231: Replace the generic LearningSource::Other used when
populating the lid_pn_cache from group participants with a specific variant to
improve observability: add a new enum variant LearningSource::GroupQuery to the
crate::lid_pn_cache::LearningSource definition, then update the call site in the
loop inside the groups code that calls learn_lid_pn_mapping_fast (the call with
parameters lid_user.as_str(), &pn_jid.user,
crate::lid_pn_cache::LearningSource::Other, false) to use
crate::lid_pn_cache::LearningSource::GroupQuery instead; leave the existing
await behavior as-is since persistence is handled by the background task spawned
by learn_lid_pn_mapping_fast.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ee23cd51-fe70-4a93-817d-f6a7ecbbfa1c
📒 Files selected for processing (3)
src/client/device_registry.rssrc/features/groups.rswacore/src/send.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 850c81b34d
ℹ️ 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".
| for (lid_user, pn_jid) in &lid_to_pn_map { | ||
| if !pn_jid.is_pn() { | ||
| continue; |
There was a problem hiding this comment.
query_info now promotes every lid_to_pn_map entry into the global LID↔PN cache, but this loop only validates pn_jid.is_pn() and never verifies that lid_user actually came from a LID JID. The group parser accepts any JID in participant.jid (wacore/src/iq/groups.rs), so an unexpected PN/non-LID participant in a LID-addressing response can poison the global mapping and trigger wrong registry/session migrations in learn_lid_pn_mapping_fast. Please gate learning on a LID-typed participant key (e.g., only learn when the original participant JID is LID).
Useful? React with 👍 / 👎.
Adds unit + e2e coverage for the fixes in this branch so the regression
can't return silently.
- Extracts the stale-device-users collection in wacore/src/send.rs into
`collect_stale_device_users`; the old inline logic stays unchanged,
just reachable from tests.
- Unit tests (src/client/device_registry.rs):
- U1 update_device_list canonical flip deletes old DB row
- U2 migrate_device_registry deletes PN DB row
- U3 invalidate_device_cache clears both aliases (cache + DB)
- U4 TOCTOU: second invalidate clears cache resurrected between the
pre-delete invalidate and delete_devices
- Unit tests (wacore/src/send.rs):
- emits LID and PN alias when mapping known
- emits only LID when mapping unknown
- dedups multiple devices of the same user
- skips successfully encrypted devices
- PN-mode group never emits an alias
- skips non-PN alias (malformed server response)
- empty/None distribution list yields empty
- E2E (tests/e2e/tests/groups.rs):
- E1 query_info populates lid_pn_cache with each LID participant's PN
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 1387-1439: The test
test_update_device_list_toctou_second_invalidate_clears_resurrected_cache
currently seeds client.device_registry_cache before calling
client.update_device_list so the first invalidate already clears it; add a
deterministic interleaving that re-populates the PN cache between the
update_device_list's first invalidate and the subsequent DB delete. Modify the
test to install a hook/latch (e.g., a oneshot or barrier) into the update/delete
path used by update_device_list (injectable via the test client from
create_test_client or by wrapping the backend delete method) that pauses
execution after the first invalidate, then in the test thread re-insert the
legacy record with client.device_registry_cache.insert(pn.into(), legacy).await,
then release the latch so the delete runs and the second invalidate can clear
the resurrected cache; keep assertions on device_registry_cache.get and
backend.get_devices unchanged.
🪄 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: b5d30a72-89b2-479f-ab2c-f5dc8c9abc90
📒 Files selected for processing (3)
src/client/device_registry.rstests/e2e/tests/groups.rswacore/src/send.rs
| /// U4 — TOCTOU regression: even if the cache got repopulated between the | ||
| /// pre-delete `invalidate` and the DB `delete_devices`, the post-delete | ||
| /// `invalidate` wipes it out. Simulated by pre-seeding both cache and DB | ||
| /// under PN, then running the canonical-flip write. | ||
| #[tokio::test] | ||
| async fn test_update_device_list_toctou_second_invalidate_clears_resurrected_cache() { | ||
| use wacore::store::traits::{DeviceInfo, DeviceListRecord}; | ||
|
|
||
| let client = create_test_client().await; | ||
| let pn = "15550000044"; | ||
| let lid = "100000000000044"; | ||
| let backend = client.persistence_manager.backend(); | ||
|
|
||
| let legacy = DeviceListRecord { | ||
| user: pn.to_string(), | ||
| devices: vec![DeviceInfo { | ||
| device_id: 9, | ||
| key_index: None, | ||
| }], | ||
| timestamp: wacore::time::now_secs(), | ||
| phash: None, | ||
| raw_id: None, | ||
| }; | ||
| backend.update_device_list(legacy.clone()).await.unwrap(); | ||
| // Pre-populate cache[PN] directly to emulate a concurrent reader that | ||
| // loaded from the DB row between the two invalidate calls. | ||
| client.device_registry_cache.insert(pn.into(), legacy).await; | ||
|
|
||
| setup_lid_pn(&client, lid, pn).await; | ||
|
|
||
| client | ||
| .update_device_list(DeviceListRecord { | ||
| user: pn.to_string(), | ||
| devices: vec![DeviceInfo { | ||
| device_id: 10, | ||
| key_index: None, | ||
| }], | ||
| timestamp: wacore::time::now_secs(), | ||
| phash: None, | ||
| raw_id: None, | ||
| }) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| assert!( | ||
| client.device_registry_cache.get(pn).await.is_none(), | ||
| "second invalidate must clear the pre-populated cache entry" | ||
| ); | ||
| assert!( | ||
| backend.get_devices(pn).await.unwrap().is_none(), | ||
| "PN DB row must still be gone" | ||
| ); | ||
| } |
There was a problem hiding this comment.
TOCTOU test does not currently exercise the interleaving it claims.
At Line 1411-Line 1413 the PN cache is pre-seeded before update_device_list. The first invalidate (Line 176) already clears it, so this test still passes even if the second invalidate is removed. Add a deterministic hook/interleaving that repopulates PN cache between the first invalidate and delete to actually guard the race fix.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/device_registry.rs` around lines 1387 - 1439, The test
test_update_device_list_toctou_second_invalidate_clears_resurrected_cache
currently seeds client.device_registry_cache before calling
client.update_device_list so the first invalidate already clears it; add a
deterministic interleaving that re-populates the PN cache between the
update_device_list's first invalidate and the subsequent DB delete. Modify the
test to install a hook/latch (e.g., a oneshot or barrier) into the update/delete
path used by update_device_list (injectable via the test client from
create_test_client or by wrapping the backend delete method) that pauses
execution after the first invalidate, then in the test thread re-insert the
legacy record with client.device_registry_cache.insert(pn.into(), legacy).await,
then release the latch so the delete runs and the second invalidate can clear
the resurrected cache; keep assertions on device_registry_cache.get and
backend.get_devices unchanged.
…pings
On first `query_info` of a large LID-mode group, the previous per-participant
`learn_lid_pn_mapping_fast` loop spawned one detached task per entry: N
tokio tasks, N DB transactions, N SQLite connection acquisitions, N signal
cache flushes. WA Web issues a single `createLidPnMappings({ mappings,
flushImmediately: true, learningSource: "other" })` from `QueryGroupJob`,
so this change does the same.
Changes:
- `wacore::store::traits`: `put_lid_mappings(&[LidPnMappingEntry])` on the
Backend trait, default impl loops; backends override for a single
transaction.
- `sqlite-storage`: override runs N upserts inside one
`conn.transaction()` on one `spawn_blocking` + one pool connection. The
old `put_lid_mapping` now delegates to the plural path.
- `client::lid_pn`: `learn_lid_pn_mappings_batch(Vec<(String, String)>, …)`
+ `persist_and_migrate_lid_pn_batch`. Cache warm is synchronous for every
entry; persist + per-new-mapping migrations run in one detached task.
- `features::groups::query_info`: replaces the N-call loop with the
batched call.
Allocation budget per entry: 2 `String` for (lid, phone_number) in the
input tuple, which then move into `LidPnEntry` and (via `into_iter`) into
`LidPnMappingEntry` — no clones on either hop. `is_new` tracked via a
parallel `Vec<bool>` instead of a `HashSet<String>`, cutting a
`phone_number.clone()` per entry.
Tests (all green under `cargo test --lib --workspace --exclude e2e-tests`):
- `test_learn_lid_pn_mappings_batch_populates_cache_synchronously`
- `test_learn_lid_pn_mappings_batch_empty_is_noop`
- `test_learn_lid_pn_mappings_batch_offline_skips_persist`
Also renames `test_update_device_list_toctou_second_invalidate_clears_resurrected_cache`
to `…_canonical_flip_clears_warm_cache`: the prior name overclaimed what
the test exercises (the first invalidate already clears the pre-seeded
cache, so the window between invalidate1 and delete isn't actually
reached). Deterministic TOCTOU coverage would need a Backend wrapper with
a pre-delete hook — out of scope. The double-invalidate remains as
defense-in-depth.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
storages/sqlite-storage/src/sqlite_store.rs (1)
1893-1932:⚠️ Potential issue | 🟠 MajorRoute this batch write through
with_retrylike every other write in this file.The new
put_lid_mappingsbypasses the proven retry-and-serialize pattern (with_retry) that handles transientSQLITE_BUSY/LOCKEDerrors. Every other write method in this store—10+ call sites—useswith_retry, which retries up to 5 times and serializes writes. Without it, one lock contention error silently drops the entire batch.Suggested direction
async fn put_lid_mappings(&self, entries: &[LidPnMappingEntry]) -> Result<()> { if entries.is_empty() { return Ok(()); } let device_id = self.device_id; let entries: Vec<LidPnMappingEntry> = entries.to_vec(); - let pool = self.pool.clone(); - tokio::task::spawn_blocking(move || -> Result<()> { - let mut conn = pool - .get() - .map_err(|e| StoreError::Connection(e.to_string()))?; - conn.transaction::<_, diesel::result::Error, _>(|conn| { + self.with_retry("put_lid_mappings", || { + let entries = entries.clone(); + Box::new(move |conn: &mut SqliteConnection| { + conn.transaction::<_, diesel::result::Error, _>(|conn| { for entry in &entries { diesel::insert_into(lid_pn_mapping::table) .values(( lid_pn_mapping::lid.eq(&entry.lid), lid_pn_mapping::phone_number.eq(&entry.phone_number), @@ )) .execute(conn)?; } Ok(()) }) - .map_err(|e| StoreError::Database(e.to_string()))?; - Ok(()) }) - .await - .map_err(|e| StoreError::Database(e.to_string()))??; - Ok(()) + .await }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storages/sqlite-storage/src/sqlite_store.rs` around lines 1893 - 1932, put_lid_mappings currently performs a direct spawn_blocking DB transaction and therefore bypasses the store's retry-and-serialize logic; wrap the DB write in the existing with_retry helper instead of calling spawn_blocking directly. Specifically, change put_lid_mappings to call with_retry (the same helper used by other write methods) and move the connection/transaction closure (the code that gets pool.get(), starts conn.transaction and executes the insert/on_conflict loop using device_id and entries) into the closure passed to with_retry so transient SQLITE_BUSY/LOCKED errors are retried and writes are serialized; preserve the same return types and error mapping (StoreError::Connection/Database) when adapting the closure.
♻️ Duplicate comments (1)
src/client/device_registry.rs (1)
1387-1449:⚠️ Potential issue | 🟡 MinorThis regression test still doesn’t protect the second invalidate.
The pre-seeded PN cache is already gone after the first invalidate, so this stays green even if the post-delete invalidate is removed. You still need a deterministic interleaving hook around
delete_devices()to cover the race you’re describing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/device_registry.rs` around lines 1387 - 1449, The test test_update_device_list_canonical_flip_clears_warm_cache is not exercising the TOCTOU window because the pre-seeded cache is cleared by the first invalidate; to make the race deterministic, wrap the Backend returned by client.persistence_manager.backend() with a test shim that implements the Backend trait and overrides delete_devices to run an injected async hook (e.g., await a oneshot or barrier) before delegating to the inner backend, then in the test insert that shim backend and use the hook to suspend execution between the first invalidate and the actual DB delete so you can assert the cache state both before and after the delete; reference the Backend trait, delete_devices, client.persistence_manager.backend(), update_device_list, and device_registry_cache when locating where to inject and use the shim.
🤖 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/lid_pn.rs`:
- Around line 667-734: Add a test that verifies the online
(flushImmediately=false) batch actually persists by calling
Client::learn_lid_pn_mappings_batch with is_offline=false (or the equivalent
parameter that triggers persistence), then await until
persistence_manager.backend().get_lid_mapping(lid) returns Some(mapping)
(retry/poll with timeout) to ensure persist_and_migrate_lid_pn_batch invoked
backend.put_lid_mappings; also add an assertion that a migration path ran (e.g.,
check the migrated flag/state or call the migration helper used in
persist_and_migrate_lid_pn_batch) for a newly learned mapping so the test fails
if persistence or migration is removed.
---
Outside diff comments:
In `@storages/sqlite-storage/src/sqlite_store.rs`:
- Around line 1893-1932: put_lid_mappings currently performs a direct
spawn_blocking DB transaction and therefore bypasses the store's
retry-and-serialize logic; wrap the DB write in the existing with_retry helper
instead of calling spawn_blocking directly. Specifically, change
put_lid_mappings to call with_retry (the same helper used by other write
methods) and move the connection/transaction closure (the code that gets
pool.get(), starts conn.transaction and executes the insert/on_conflict loop
using device_id and entries) into the closure passed to with_retry so transient
SQLITE_BUSY/LOCKED errors are retried and writes are serialized; preserve the
same return types and error mapping (StoreError::Connection/Database) when
adapting the closure.
---
Duplicate comments:
In `@src/client/device_registry.rs`:
- Around line 1387-1449: The test
test_update_device_list_canonical_flip_clears_warm_cache is not exercising the
TOCTOU window because the pre-seeded cache is cleared by the first invalidate;
to make the race deterministic, wrap the Backend returned by
client.persistence_manager.backend() with a test shim that implements the
Backend trait and overrides delete_devices to run an injected async hook (e.g.,
await a oneshot or barrier) before delegating to the inner backend, then in the
test insert that shim backend and use the hook to suspend execution between the
first invalidate and the actual DB delete so you can assert the cache state both
before and after the delete; reference the Backend trait, delete_devices,
client.persistence_manager.backend(), update_device_list, and
device_registry_cache when locating where to inject and use the shim.
🪄 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: c6adf787-2b68-4b5a-b979-59e7b71a2e6e
📒 Files selected for processing (5)
src/client/device_registry.rssrc/client/lid_pn.rssrc/features/groups.rsstorages/sqlite-storage/src/sqlite_store.rswacore/src/store/traits.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4f7714281
ℹ️ 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".
| // Always run the second invalidate; even if delete fails, the | ||
| // cache may carry resurrected data that shouldn't stick. | ||
| self.device_registry_cache.invalidate(pn).await; | ||
| if let Err(e) = backend.delete_devices(pn).await { |
There was a problem hiding this comment.
migrate_device_registry_on_lid_discovery now always deletes pn after writing the migrated row, but if pn == lid this deletes the same row that was just updated and also invalidates its cache entry. That leaves no registry record for the user and can force repeated device re-fetch/406 loops. This equality case is reachable from the new group-based mapping learning path because participant keys are not type-validated before migration is scheduled, so the delete should be skipped (or migration should be rejected) when both keys are equal.
Useful? React with 👍 / 👎.
…st path Two follow-ups on the LID-PN batch work: - `storages/sqlite-storage`: `put_lid_mappings` now wraps the N-upsert transaction in `with_retry`, matching the rest of the write path. The previous direct `spawn_blocking` bypassed the write semaphore + retry loop that other writes (`put_app_state_mutation_macs`, `store_sent_message`, `clear_sender_key_devices`, …) use to handle transient `SQLITE_BUSY`/`LOCKED`. The batch transaction is N× more likely to contend than the single-entry path, so this matters more for the plural variant than it did for the single. The Vec is shared across retry attempts via `Arc` so a retry only bumps the refcount — no re-clone of the batch. - `src/client/lid_pn`: new `test_learn_lid_pn_mappings_batch_online_persists_and_migrates` seeds a PN-keyed device registry row, fires the batch with `is_offline = false`, then polls until the device row is observable under the LID key (which strictly happens after both `put_lid_mappings` and `migrate_device_registry_on_lid_discovery` complete). Guards against a regression that would silently skip either step.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a291b63e2b
ℹ️ 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".
| if pn_jid.is_pn() { | ||
| batch.push((lid_user.as_str().to_string(), pn_jid.user.to_string())); |
There was a problem hiding this comment.
The new batch-learning path only checks pn_jid.is_pn() before pushing (lid_user, pn) into learn_lid_pn_mappings_batch, but lid_user is accepted verbatim from group participants. GroupParticipantResponse::try_from_node_ref accepts any JID shape for participant.jid, so a non-LID participant in a LID-mode response can be written into the global LID↔PN cache and trigger incorrect device/session migrations for the wrong user key. Please require the participant key to be LID before enqueueing the mapping.
Useful? React with 👍 / 👎.
| if let Err(e) = backend.delete_devices(pn).await { | ||
| warn!("Failed to delete PN-keyed device row during LID migration: {e}"); |
There was a problem hiding this comment.
migrate_device_registry_on_lid_discovery now deletes pn after writing the migrated row under lid, but there is no guard for pn == lid. In that case the delete removes the just-written canonical row and leaves both cache and DB without the registry entry, which can force repeated re-fetch/406 behavior for that user. Add an equality check and skip migration/delete when both keys are the same.
Useful? React with 👍 / 👎.
1 participant
Summary
Field report against alpha.12: 23 batch 406s on the same group in 3h45m, same stale devices across batches, producing 12-41s latency spikes per group send. This PR closes the zombie-formation loop end-to-end.
Root cause
Asymmetric key-storage in the device registry:
maybe_compute_skdm_devicesresolves LID participants to PN form before usync; usync responses are persisted under the PN key.stale_device_usersis LID form (fromdistribution_listin LID-mode groups).invalidate_device_cache(lid)resolves aliases vialid_pn_cache; with no mapping it only invalidates the LID key, leaving the PN-keyed row as a zombie → every subsequent send refetches the same stale devices.The LID↔PN mapping was never learned for silent-observer participants because
query_infobuiltlid_to_pn_maponly insideGroupInfo— never fed the globallid_pn_cache. WA Web'sCreate/OrReplaceDisplayNamesAndLidPnMappingsJobpopulates the cache from group data withlearningSource: "other".Changes
groups.rs::query_info: push eachlid→pnpair intolid_pn_cachevialearn_lid_pn_mapping_fast(synchronous cache update + detached persist), guarded bypn_jid.is_pn(). Closes the silent-observer gap. WA Web parity.wacore::send.rs::encrypt_for_devices: drop the O(N·RTT) per-device retry on batch 406. The server returns all-or-nothing here; the field log shows every individual retry also returned 406. Match WA WebGroupSkmsgJob: log, markhad_406, continue without those devices.wacore::send.rs::prepare_group_stanza: emit both LID and PN aliases instale_device_userswhenGroupInfoknows the mapping (guarded withpn_jid.is_pn()). Defensive againstlid_pn_cachenot being populated yet.device_registry.rs::update_device_list: delete the stale DB row when the canonical key flips, not just the cache. Closes a zombie-creation path.device_registry.rs::migrate_device_registry_on_lid_discovery: delete the PN-keyed DB row during LID migration, not just the cache.Review notes
Codex reviewed twice and all flags were addressed:
self_weak.upgrade()safe, Fix 2 doesn't lose salvageable bundles, no races in Fix 1.pn_jid.is_pn()guard missing in Fix 3 → added.pn_jid.is_pn()guard missing in Fix 1 too → added.WA Web compliance:
Create/OrReplaceDisplayNamesAndLidPnMappingsJob.js:54(learningSource: "other").Send/GroupSkmsgJob.js:18-43(try/catch + continue).Test plan
cargo clippy --all --testscleancargo test -p whatsapp-rust --lib(390 passed)cargo test -p e2e-tests --test groups --test messaging --test offline_messages --test concurrent_disconnectall green