fix(chatwoot-adapter): auto-recover from deleted Chatwoot conversations#45
fix(chatwoot-adapter): auto-recover from deleted Chatwoot conversations#45i350 wants to merge 4 commits into
Conversation
rmyndharis
left a comment
There was a problem hiding this comment.
Requested changes — thanks for tackling this, @i350. The problem is real, the write-up is one of the clearest I've seen here, and the overall approach is the right one. There are just a few gaps to close before this can merge.
Verified against main: the dangling-mapping scenario plays out exactly as described. relayInbound resolves the cached conversation id, the dead id 404s, the message cycles through the retry queue against the same stale id and is dead-lettered after 5 attempts — and every subsequent message from that chat repeats the cycle. Own-send drops the message with only a log line. No self-healing exists today, so this fix is genuinely needed. Detecting the 404 mid-relay and rebuilding through ensureConversation is a sound, minimal approach — better than relying on a conversation_deleted webhook, which would be fragile and more intrusive. The safety bounds (404-only, one rebuild per message, second failure falls through to existing paths) are well thought out.
That said, a few things need attention:
-
npm run typecheckfails. Both new tests uselog: logCalls.push.bind(logCalls)—Array.pushreturnsnumberand takesstring[], which doesn't satisfy(m: string, e?: unknown) => void. TS2322 atinbound.test.ts:309andsent.test.ts:277. This one is red on CI, so it's a blocker. -
Media messages aren't covered.
postMedia(chatwoot-client.ts:154) throwsnew Error('Chatwoot postMedia -> ${res.status}')without settingerr.status, so a 404 on a photo/voice/document relay never triggers the recovery — it still dead-letters. It's a one-line fix in the client, and without it the coverage doesn't quite match what the PR body promises. -
The @lid-migrated case doesn't recover on inbound. The mapping can live under the canonical key (@c.us) while
msg.chatIdis @lid (the dual-lookup inresolveConversation). The recovery only callsunlinkByChatId(sessionId, msg.chatId), which deletes nothing in that case —resolveConversationthen re-finds the same stale row via the canonical key, posts to the dead id again, and the second 404 sends the message off to be dead-lettered. Given how much of this codebase deals with the @lid migration (#609, #615), this one is worth handling rather than documenting away.foundKeyis already available from the dual-lookup — unlink the key the mapping actually lives under (or both keys). -
Minor: recovery also runs inside the retry drain (drain calls
relayInbound), so in the catastrophic double-404 scenario each drain attempt can mint a fresh orphan conversation — bounded at 5, but you may want to skip the rebuild when invoked from the drain.
One small correction to the PR body: without the patch the message isn't re-posted "forever" — it's retried 5 times and dead-lettered. The practical impact is as you describe (the chat is permanently broken for all subsequent messages), but it's worth stating the mechanism precisely.
The tests themselves are genuinely good — seeding the mapping in the same backing map the unlink helpers delete against exercises the real semantics, not just the mocks. Once the typecheck errors and the unlink-key issue are fixed (and ideally the postMedia status), I'll be happy to merge this.
Skip the rebuild inside the retry drain
rmyndharis
left a comment
There was a problem hiding this comment.
Nice iteration — the foundKey refactor and carrying err.status through postMedia both look right, and the suite is green. Two things I'd resolve before merge:
1. recoverOn404 isn't wired into the drain path. relayInbound takes opts.recoverOn404 (default true) and the comment says the drain flips it to false, but the drain closure in index.ts never passes it:
(sessionId, _chatId, msg) => relayInbound(buildDeps(readConfig(ctx.config), sessionId), sessionId, msg),So it stays true. If a queued message keeps 404-ing after a rebuild, each drain attempt (up to the retry cap) unlinks + rebuilds and mints a new orphan conversation instead of just burning the retry budget. Narrow case, but the guard is inert as written — either pass { recoverOn404: false } here, or drop the flag and its comment.
2. Own-send recovery still unlinks msg.chatId, not the found key. The foundKey fix landed in relayInbound but not handleSent:
await deps.store.unlinkByChatId(sessionId, msg.chatId);
conversationId = await ensureConversation(deps, sessionId, msg.chatId, { ... });For a contact migrated to @lid, findMappedConversation matches via the canonical @c.us fallback, but the row lives under the @c.us forward key — so unlinking msg.chatId (@lid) is a no-op, the stale @c.us → deadConv row survives, and ensureConversation(@lid) → searchContact(@lid) misses the existing @c.us contact → duplicate contact + split conversation (the #31/#42 case). findMappedConversation should return the key it matched on (as resolveConversation now does) so the recovery unlinks the right one; reusing the mapping's contactId/sourceId before unlinking would avoid the contact lookup entirely.
A migrated-contact recovery test would lock both paths down — the current 404 tests all use a non-migrated key.
Problem
When an operator deletes a Chatwoot conversation out-of-band, the adapter's cached mapping
conv:<sessionId>:<chatId> → { conversationId: <deleted id> }becomes dangling. The next inbound WhatsApp message relays to the dead conversation id and gets a 404. Without this patch:Solution
Detect the 404 mid-relay, drop the stale forward + reverse mapping, re-resolve through the existing
ensureConversationpath (reusing the Chatwoot contact when it still exists), and re-post the message into a fresh conversation.What changes
mapping-store.tsunlinkByChatIdandunlinkByConversationIdhelpers (delete forward key, scoped reverse key, legacy reverse key)inbound.tsrelayMessagein try/catch; on 404, unlink stale mapping, callresolveConversationto rebuild, retry once. Second failure surfaces to the existing retry queue (5-attempt cap).sent.tsensureConversationdirectly (noresolveConversationon this path), retry once. At-most-once — logs and stops, no retry queue.inbound.test.tssent.test.tsSafety bounds
store.linkafter recovery callsmappings.upsertwhich hits the existing branch and updates the row in place.Tests
All 33 tests pass (15 sent + 18 inbound). New tests verify:
ensureConversationre-resolves the contact by JID (notcreateContact)