Skip to content

fix(chatwoot-adapter): auto-recover from deleted Chatwoot conversations#45

Open
i350 wants to merge 4 commits into
rmyndharis:mainfrom
i350:fix/recover-on-chatwoot-conv-deletion
Open

fix(chatwoot-adapter): auto-recover from deleted Chatwoot conversations#45
i350 wants to merge 4 commits into
rmyndharis:mainfrom
i350:fix/recover-on-chatwoot-conv-deletion

Conversation

@i350

@i350 i350 commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

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:

  • Inbound: the message is enqueued for retry and re-posted to the same dead id forever (no fresh conversation is ever created).
  • Own-send (message:sent): the message is silently dropped.

Solution

Detect the 404 mid-relay, drop the stale forward + reverse mapping, re-resolve through the existing ensureConversation path (reusing the Chatwoot contact when it still exists), and re-post the message into a fresh conversation.

What changes

File Change
mapping-store.ts Add unlinkByChatId and unlinkByConversationId helpers (delete forward key, scoped reverse key, legacy reverse key)
inbound.ts Wrap relayMessage in try/catch; on 404, unlink stale mapping, call resolveConversation to rebuild, retry once. Second failure surfaces to the existing retry queue (5-attempt cap).
sent.ts Symmetric recovery for the own-send path: on 404, unlink stale mapping, call ensureConversation directly (no resolveConversation on this path), retry once. At-most-once — logs and stops, no retry queue.
inbound.test.ts Two new tests: successful 404 recovery, and double-404 propagating to retry queue
sent.test.ts Two new tests: successful 404 recovery, and double-404 logging both inner and outer failure

Safety bounds

  • Single 404 status only: all other errors (5xx, 422, network) propagate unchanged to existing paths.
  • One rebuild per message: the structural bound (inner try/catch re-throws original error) means one handler invocation = at most one rebuild attempt. No infinite loop.
  • No retry queue entry on recovery success: the 404 is resolved in-band. Only a failed rebuild surfaces to the retry queue (inbound) or logs (own-send).
  • No TypeORM migration needed: store.link after recovery calls mappings.upsert which hits the existing branch and updates the row in place.

Tests

All 33 tests pass (15 sent + 18 inbound). New tests verify:

  • Unlink calls target the correct keys (forward + scoped + legacy reverse)
  • ensureConversation re-resolves the contact by JID (not createContact)
  • One fresh conversation minted on recovery
  • Double-404 produces two complementary log lines (inner: why rebuild failed, outer: overall failure)
  • Inbound double-404 surfaces to retry queue with the original error

@rmyndharis rmyndharis left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. npm run typecheck fails. Both new tests use log: logCalls.push.bind(logCalls)Array.push returns number and takes string[], which doesn't satisfy (m: string, e?: unknown) => void. TS2322 at inbound.test.ts:309 and sent.test.ts:277. This one is red on CI, so it's a blocker.

  2. Media messages aren't covered. postMedia (chatwoot-client.ts:154) throws new Error('Chatwoot postMedia -> ${res.status}') without setting err.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.

  3. The @lid-migrated case doesn't recover on inbound. The mapping can live under the canonical key (@c.us) while msg.chatId is @lid (the dual-lookup in resolveConversation). The recovery only calls unlinkByChatId(sessionId, msg.chatId), which deletes nothing in that case — resolveConversation then 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. foundKey is already available from the dual-lookup — unlink the key the mapping actually lives under (or both keys).

  4. 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.

@i350 i350 changed the title Auto-recover from deleted Chatwoot conversations fix(chatwoot-adapter): auto-recover from deleted Chatwoot conversations Jul 23, 2026

@rmyndharis rmyndharis left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants