fix(signal): canonicalize PN/LID session addressing - #209
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughChangesPN/LID mappings are added as a session-scoped store domain across memory and persistent backends. PN/LID canonicalization
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
packages/store-mongo/src/__tests__/integration.test.ts (1)
1051-1073: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame
lidPnMappingreplaceability/session-scoping assertions duplicated verbatim across three provider integration suites.All three tests exercise identical fixture values and assertion sequence (initial-null, set/replace, reverse-lookup consistency, cross-session isolation, clear), differing only by which provider's
store.stores.lidPnMapping(...)instance they exercise.
packages/store-mongo/src/__tests__/integration.test.ts#L1051-L1073: extract the shared assertion sequence into a reusable helper (e.g. accepting the two mapping-store instances) and call it here.packages/store-mysql/src/__tests__/integration.test.ts#L1084-L1106: call the same shared helper instead of duplicating the assertion sequence.packages/store-redis/src/__tests__/integration.test.ts#L1049-L1071: call the same shared helper instead of duplicating the assertion sequence.🤖 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 `@packages/store-mongo/src/__tests__/integration.test.ts` around lines 1051 - 1073, Extract the duplicated PN/LID mapping assertion sequence from the integration tests into one shared helper that accepts the two mapping-store instances. Update packages/store-mongo/src/__tests__/integration.test.ts#L1051-L1073 to define or use the helper, and replace the duplicated sequence in packages/store-mysql/src/__tests__/integration.test.ts#L1084-L1106 and packages/store-redis/src/__tests__/integration.test.ts#L1049-L1071 with calls to it, preserving each provider’s session-scoped lidPnMapping setup and skip behavior.src/client/coordinators/WaMessageDispatchCoordinator.ts (1)
1385-1399: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRemove redundant address resolution.
pendingTargetAddressesare already resolved canonical addresses. They are extracted frompendingTargets, which were populated from thefanoutAddressesthat underwent resolution at line 1313. Re-resolving them here is redundant and wastes a DB/memory call in the fallback path.♻️ Proposed refactor
- const resolvedPendingAddresses = this.deps.signalAddressResolver - ? await this.deps.signalAddressResolver.resolveMany(pendingTargetAddresses) - : pendingTargetAddresses const hasPendingSessions = - await this.deps.sessionStore.hasSessions(resolvedPendingAddresses) + await this.deps.sessionStore.hasSessions(pendingTargetAddresses) const nextAvailableTargets: { readonly jid: string readonly address: SignalAddress }[] = [] for (let index = 0; index < pendingTargets.length; index += 1) { if (hasPendingSessions[index]) { - nextAvailableTargets.push({ - jid: pendingTargets[index].jid, - address: resolvedPendingAddresses[index] - }) + nextAvailableTargets.push(pendingTargets[index]) } }🤖 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/client/coordinators/WaMessageDispatchCoordinator.ts` around lines 1385 - 1399, Remove the conditional signalAddressResolver.resolveMany call in the pending-target fallback and use pendingTargetAddresses directly as the resolved address list passed to sessionStore.hasSessions and indexed when constructing nextAvailableTargets. Keep the existing pendingTargets iteration and JID mapping unchanged.
🤖 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/WaClientFactory.ts`:
- Around line 1286-1291: Update the REMOVE branch in the device notification
handling around SignalDeviceSyncApi and deleteUserDevices so it derives and
reuses the canonical user JID before deleting device-list entries. Ensure
deleteUserDevices receives the same normalized key used by syncDeviceList,
including for hosted-server JIDs, while preserving the existing signal-address
resolution flow.
- Around line 1202-1204: Update the self-identity handling around signal address
resolution to use the existing isOwnAccountJid(...) account check instead of
comparing normalized user JIDs. Ensure `@lid` values from parsed.fromJid are
recognized as the own account so the existing disconnect path executes, while
preserving behavior for other identities.
In `@src/message/crypto/icdc.ts`:
- Around line 64-66: Update the address comparison in the surrounding function
to safely access localAddress.server and localAddress.device, using an explicit
localAddress truthiness guard or optional chaining while preserving the existing
user, server, and device matching behavior.
In `@src/message/primitives/incoming.ts`:
- Around line 617-618: Guard the non-critical learnMessageLidPnMappings call in
handleIncomingMessageAck so persistence failures do not interrupt message
decryption or acknowledgement. Catch and normalize the error with toError if
needed, then log it at warn level and allow the existing flow to continue; do
not change successful mapping behavior.
In `@src/signal/api/SignalIdentitySyncApi.ts`:
- Around line 127-143: Deduplicate canonical records before batch upserts: in
src/signal/api/SignalIdentitySyncApi.ts lines 127-143, update the identities
construction around setRemoteIdentities to retain one entry per resolved
address.user + ':' + address.device key; in src/signal/group/SenderKeyManager.ts
lines 193-199, update the distributions construction around
upsertSenderKeyDistributions to retain one entry per resolved sender address
key.
In `@src/signal/session/SignalAddressResolver.ts`:
- Around line 75-112: Update learnJidPairInternal to perform an optimistic cache
pre-check before mappingGate.runExclusive: when the cached PN-to-LID mapping
already equals mapping.lidUser, return false immediately without acquiring the
exclusive gate. Keep the existing gated validation and store mutation logic for
cache misses or mismatches, preserving cache consistency and replacement
behavior.
---
Nitpick comments:
In `@packages/store-mongo/src/__tests__/integration.test.ts`:
- Around line 1051-1073: Extract the duplicated PN/LID mapping assertion
sequence from the integration tests into one shared helper that accepts the two
mapping-store instances. Update
packages/store-mongo/src/__tests__/integration.test.ts#L1051-L1073 to define or
use the helper, and replace the duplicated sequence in
packages/store-mysql/src/__tests__/integration.test.ts#L1084-L1106 and
packages/store-redis/src/__tests__/integration.test.ts#L1049-L1071 with calls to
it, preserving each provider’s session-scoped lidPnMapping setup and skip
behavior.
In `@src/client/coordinators/WaMessageDispatchCoordinator.ts`:
- Around line 1385-1399: Remove the conditional
signalAddressResolver.resolveMany call in the pending-target fallback and use
pendingTargetAddresses directly as the resolved address list passed to
sessionStore.hasSessions and indexed when constructing nextAvailableTargets.
Keep the existing pendingTargets iteration and JID mapping 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6631aad7-628f-4f87-b356-e098e027ac1b
📒 Files selected for processing (57)
.changeset/calm-lids-align.mdREADME.mdpackages/store-mongo/src/__tests__/integration.test.tspackages/store-mongo/src/createMongoStore.tspackages/store-mongo/src/index.tspackages/store-mongo/src/lid-pn-mapping.store.tspackages/store-mysql/src/__tests__/integration.test.tspackages/store-mysql/src/connection.tspackages/store-mysql/src/createMysqlStore.tspackages/store-mysql/src/index.tspackages/store-mysql/src/lid-pn-mapping.store.tspackages/store-mysql/src/types.tspackages/store-postgres/src/__tests__/integration.test.tspackages/store-postgres/src/connection.tspackages/store-postgres/src/createPostgresStore.tspackages/store-postgres/src/index.tspackages/store-postgres/src/lid-pn-mapping.store.tspackages/store-postgres/src/types.tspackages/store-redis/README.mdpackages/store-redis/src/__tests__/integration.test.tspackages/store-redis/src/createRedisStore.tspackages/store-redis/src/index.tspackages/store-redis/src/lid-pn-mapping.store.tspackages/store-sqlite/src/__tests__/contracts.test.tspackages/store-sqlite/src/createSqliteStore.tspackages/store-sqlite/src/index.tspackages/store-sqlite/src/lid-pn-mapping.store.tspackages/store-sqlite/src/migrations.tspackages/store-sqlite/src/table-names.tspackages/store-sqlite/src/types.tssrc/client/WaClient.tssrc/client/WaClientFactory.tssrc/client/__tests__/client.test.tssrc/client/coordinators/WaMessageDispatchCoordinator.tssrc/client/coordinators/WaRetryCoordinator.tssrc/client/coordinators/__tests__/retry-coordinator.test.tssrc/message/crypto/icdc.tssrc/message/primitives/__tests__/incoming.test.tssrc/message/primitives/incoming.tssrc/signal/api/SignalIdentitySyncApi.tssrc/signal/api/__tests__/api.test.tssrc/signal/group/SenderKeyManager.tssrc/signal/group/__tests__/sender-key.test.tssrc/signal/index.tssrc/signal/session/SignalAddressResolver.tssrc/signal/session/SignalProtocol.tssrc/signal/session/__tests__/address-resolver.test.tssrc/signal/session/__tests__/resolver.test.tssrc/signal/session/__tests__/session.test.tssrc/signal/session/resolver.tssrc/store/__tests__/create-store.test.tssrc/store/contracts/lid-pn-mapping.store.tssrc/store/createStore.tssrc/store/index.tssrc/store/locks/lid-pn-mapping.lock.tssrc/store/memory/lid-pn-mapping.store.tssrc/store/types.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 57 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 15 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
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/protocol/jid.ts`:
- Around line 17-18: Rename the constant CANONICAL_SIGNAL_USER_JID_OPTIONS to
WA_CANONICAL_SIGNAL_USER_JID_OPTIONS in src/protocol/jid.ts lines 17-18, and
update every usage in src/protocol/jid.ts lines 228-240 to reference the new
name.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 63abf035-ba92-4498-a1f4-f16ad1c6a2f6
📒 Files selected for processing (4)
src/protocol/__tests__/protocol.test.tssrc/protocol/jid.tssrc/signal/session/SignalAddressResolver.tssrc/signal/session/__tests__/address-resolver.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/signal/session/SignalAddressResolver.ts
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
when I made zapo, I chose not to add LID mapping due to the high overhead on the database, in memory (if using a cache), and in hot paths; the only send paths that still use PNs in official clients are peer ones (e.g PDO), which zapo already maps or those from unofficial libraries that still send via PN. What do you think? Since the PR is marked as a fix, is there a problematic scenario I can reproduce? Note: zapo already has a tiny lid map inside device list cache |
Interesting point. I created the PR with compliance in mind, but I agree that in this case, performance matters more. I'll close this soon and thank you for the feedback |
2 participants
Signal sessions were keyed directly by the address on the outer stanza. When WhatsApp alternated the same device between a phone-number JID and a LID JID, the two forms could create independent ratchets: a reply advanced the LID session while a later PN-addressed message loaded stale state and failed authentication.
Mirror WhatsApp Web's ordering and address semantics:
maybeProcesMsgInfoForLid()runs beforedecryptE2EPayload(), while the Signal address layer resolves a PN through its current LID (including hosted → hosted.lid). Learn conservative sender, participant, and recipient mappings plus authoritative peer and latest-recipient mappings, then canonicalize Signal sessions, identities, sender keys, retry paths, and fanout through one shared resolver. The one-to-one mapping follows the configured session provider and is persisted by all official stores.Add a real Signal regression covering a PN-addressed prekey message with sender LID metadata, a LID-addressed reply, a fresh resolver, and a later PN message without repeated metadata. Provider tests cover replacement, reverse lookup, session isolation, and Redis TTL behavior.
AI-assisted contribution: implementation and tests were developed with Codex; protocol behavior was verified against the deobfuscated WhatsApp Web bundle.
Summary by CodeRabbit
lidPnMapping) across MongoDB, MySQL, PostgreSQL, Redis, and SQLite.lidPnMappingpersistence and TTL behavior.