fix(history-sync): serialise tcToken persistence + drop external doc reference
Addresses 4 PR #390 review findings (Codex P1 + Copilot ×3): #1 Codex P1 (real, MAJOR) — `storeTcTokensFromHistorySync` fire-and-forget created a write race when multiple history-sync chunks arrived in parallel (common during reconnect/QR scan). The function does a non-atomic read-then-merge-then-write: chunk B could read `existing` before chunk A wrote, then commit later and overwrite chunk A's newer timestamp. Even worse, the merged `__index` write could drop JIDs added by the other chunk, leaving stale tcTokens and persistent 463 send errors. FIX: introduced `scheduleHistoryTcTokenSync()` — a module-scoped single-concurrency promise chain that sequentialises all calls in order. The chain swallows prior errors so a single failure can't stall future runs. Call site stays fire-and-forget so the `messaging-history.set` emit is still instant (which is the whole point of PR #389). #2 Copilot (same root as #1) — already covered by the chain. #3+#4 Copilot — code comments referenced `Downloads/InfiniteAPI-Inbound-Latency-Fix-Documentation.md` which is intentionally out-of-tree (user keeps it locally). Replaced the broken refs with self-contained explanations covering the same rationale — future maintainers no longer need an external file to understand the trade-offs. Skipped: CodeRabbit nitpick to extract a `fireAndForgetMappingStore` helper (stylistic, current explicit form makes the lid/pn argument order obvious to reviewers — keeping it). Tests: 35/35 suites, 824/824 still passing. Customizations untouched. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -2345,10 +2345,15 @@ export const makeMessagesRecvSocket = (config: SocketConfig) => {
|
|||||||
//
|
//
|
||||||
// HISTORICAL: this restores the intent of d73cd28d39 (2026-02-03) which was
|
// HISTORICAL: this restores the intent of d73cd28d39 (2026-02-03) which was
|
||||||
// partially reverted by c3fc792351 the same day due to a race-condition concern
|
// partially reverted by c3fc792351 the same day due to a race-condition concern
|
||||||
// with migrateSession (kept sync here). storeLIDPNMappings was over-protected.
|
// with migrateSession (kept sync here). storeLIDPNMappings was over-protected:
|
||||||
|
// it persists a mapping that downstream consumers can re-derive from key.*Alt,
|
||||||
|
// while migrateSession actually moves the Signal session record that decrypt()
|
||||||
|
// will load microseconds later — those two have very different criticality.
|
||||||
//
|
//
|
||||||
// DO NOT make migrateSession async — decrypt() depends on it.
|
// DO NOT make migrateSession async — decrypt() depends on the session being at
|
||||||
// See Downloads/InfiniteAPI-Inbound-Latency-Fix-Documentation.md for full context.
|
// the correct identifier (LID vs PN) when it runs. Other code paths (USync
|
||||||
|
// device lookup in messages-send.ts) create LID/PN mappings without migrating
|
||||||
|
// the session, so we cannot skip migration even when the mapping already exists.
|
||||||
if (!!alt) {
|
if (!!alt) {
|
||||||
const altServer = jidDecode(alt)?.server
|
const altServer = jidDecode(alt)?.server
|
||||||
const primaryJid = msg.key.participant || msg.key.remoteJid!
|
const primaryJid = msg.key.participant || msg.key.remoteJid!
|
||||||
|
|||||||
@@ -82,6 +82,47 @@ const REAL_MSG_REQ_ME_STUB_TYPES = new Set([WAMessageStubType.GROUP_PARTICIPANT_
|
|||||||
* (TC_TOKEN_INDEX_KEY) via buildMergedTcTokenIndexWrite, so the 24h prune sweep in
|
* (TC_TOKEN_INDEX_KEY) via buildMergedTcTokenIndexWrite, so the 24h prune sweep in
|
||||||
* messages-recv picks them up across sessions.
|
* messages-recv picks them up across sessions.
|
||||||
*/
|
*/
|
||||||
|
/**
|
||||||
|
* Single-concurrency queue for `storeTcTokensFromHistorySync` calls.
|
||||||
|
*
|
||||||
|
* Why: the function does read-then-write merges (`keyStore.get('tctoken', ...)` →
|
||||||
|
* compute → `keyStore.set(...)`) which are NOT atomic at the store level. If two
|
||||||
|
* history-sync chunks invoke this concurrently (common during reconnect / QR
|
||||||
|
* scan), an older chunk that started first can `keyStore.set` AFTER a newer
|
||||||
|
* chunk, overwriting the newer entry — and worse, the merged `__index` write
|
||||||
|
* can drop JIDs the other chunk just added. Result: stale tcTokens / repeat 463
|
||||||
|
* sends until the next opportunistic refetch.
|
||||||
|
*
|
||||||
|
* Serialising via a chained Promise keeps the runs ordered while still freeing
|
||||||
|
* the calling `processMessage` to emit `messaging-history.set` immediately
|
||||||
|
* (the chain is fire-and-forget at the call site). Errors don't break the chain
|
||||||
|
* — each `catch` resets it to `Promise.resolve()` so a single failure can't
|
||||||
|
* stall future runs.
|
||||||
|
*
|
||||||
|
* The chain is module-scoped (one per Node process). Multiple Baileys instances
|
||||||
|
* sharing this module will serialise across instances too, but their writes
|
||||||
|
* target different keyStores so there's no correctness gain — only a tiny loss
|
||||||
|
* of inter-instance parallelism for tcToken syncs, which is acceptable given
|
||||||
|
* how rarely this runs vs. how rare cross-instance contention is.
|
||||||
|
*/
|
||||||
|
let historyTcTokenChain: Promise<void> = Promise.resolve()
|
||||||
|
|
||||||
|
function scheduleHistoryTcTokenSync(
|
||||||
|
chats: Chat[],
|
||||||
|
signalRepository: SignalRepositoryWithLIDStore,
|
||||||
|
keyStore: SignalKeyStoreWithTransaction,
|
||||||
|
logger?: ILogger
|
||||||
|
): void {
|
||||||
|
historyTcTokenChain = historyTcTokenChain
|
||||||
|
.catch(() => {
|
||||||
|
/* swallow prior error so chain stays alive */
|
||||||
|
})
|
||||||
|
.then(() => storeTcTokensFromHistorySync(chats, signalRepository, keyStore, logger))
|
||||||
|
.catch(err => {
|
||||||
|
logger?.warn({ err }, 'background tctoken history-sync persistence failed')
|
||||||
|
})
|
||||||
|
}
|
||||||
|
|
||||||
async function storeTcTokensFromHistorySync(
|
async function storeTcTokensFromHistorySync(
|
||||||
chats: Chat[],
|
chats: Chat[],
|
||||||
signalRepository: SignalRepositoryWithLIDStore,
|
signalRepository: SignalRepositoryWithLIDStore,
|
||||||
@@ -565,22 +606,28 @@ const processMessage = async (
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// Persist tctokens carried by history-sync chats in BACKGROUND.
|
// Persist tctokens carried by history-sync chats in BACKGROUND, serialised.
|
||||||
|
//
|
||||||
// Originally awaited (PR #386) to avoid 463 on first multi-device send, but in
|
// Originally awaited (PR #386) to avoid 463 on first multi-device send, but in
|
||||||
// production this drains the event buffer per-chunk and adds visible delivery
|
// production this drained the event buffer per-chunk and added visible delivery
|
||||||
// latency (especially after restart / QR scan when many chunks arrive at once).
|
// latency (especially after restart / QR scan when many chunks arrived at once).
|
||||||
|
//
|
||||||
|
// `scheduleHistoryTcTokenSync` enqueues onto a single-concurrency promise chain
|
||||||
|
// (see definition above) — chunks persist sequentially in the order they were
|
||||||
|
// emitted, preserving timestamp monotonicity AND keeping the `__index` write
|
||||||
|
// safe from concurrent merge clobbers. The call returns immediately so the
|
||||||
|
// `messaging-history.set` emit is not blocked.
|
||||||
//
|
//
|
||||||
// TRADE-OFF: a listener that fires an outbound send IMMEDIATELY after the emit
|
// TRADE-OFF: a listener that fires an outbound send IMMEDIATELY after the emit
|
||||||
// may race the background persistence and get a 463 on that specific send. The
|
// may race the still-pending persistence and get a 463 on that specific send.
|
||||||
// existing 463 handler in messages-recv.ts triggers a getPrivacyTokens() refetch
|
// The existing 463 handler in messages-recv.ts triggers a getPrivacyTokens()
|
||||||
// that auto-recovers within seconds. Net result is much better UX than per-chunk
|
// refetch that auto-recovers within seconds. Net result is much better UX than
|
||||||
// stalls.
|
// per-chunk stalls.
|
||||||
//
|
//
|
||||||
// DO NOT add `await` back here without re-evaluating production latency.
|
// DO NOT add `await` back here without re-evaluating production latency, AND
|
||||||
// See Downloads/InfiniteAPI-Inbound-Latency-Fix-Documentation.md for full context.
|
// DO NOT call storeTcTokensFromHistorySync directly — it must go through the
|
||||||
storeTcTokensFromHistorySync(data.chats, signalRepository, keyStore, logger).catch(err =>
|
// chain to preserve write ordering across overlapping chunks.
|
||||||
logger?.warn({ err }, 'background tctoken history-sync persistence failed')
|
scheduleHistoryTcTokenSync(data.chats, signalRepository, keyStore, logger)
|
||||||
)
|
|
||||||
|
|
||||||
ev.emit('messaging-history.set', {
|
ev.emit('messaging-history.set', {
|
||||||
...data,
|
...data,
|
||||||
|
|||||||
Reference in New Issue
Block a user