Carry the facts a delivery was decided under - #579
Conversation
Issue #540, window 2. A colleague's reply whose delivery died between the INSERT and the claim carries its shape and no role: the claim is the statement that writes the role, and it never ran. Read as the responder's — which is what shipped — a WATCHER's row is silently mis-served, because the takeover correctly answers `not-owed` and the observer's lost ingestion, the whole of what that route owed, leaves no trace anywhere. Reading it as the observer's would be wrong in the other direction and more often: on the far commoner responder's route a real handover would never be armed. So the verdict is its own, and it does both honest things instead of guessing. The takeover is armed, which is free where it was not owed — the recovery re-asks every gate and answers `not-owed` without touching the conversation — and the gap is reported at `warn`, naming which of the two stories this could be and what the inbox looks like now. No new column: the fact was already on the row. `claimedAt` is null exactly when nothing has claimed it, whoever wrote it, and an unclaimed row is one nothing could have stated a role on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…under Every reader of a delivery's route role rebuilds it from the binding as it stands now, and an administrative write can land between Chatwoot emitting the event and that reading. The inbox now counts the writes that move who routes it — responder bound, rebound or unbound, observer attached or detached, and the agent deleted — and the ledger row records the counter it was RECEIVED under. Stamped at INSERT and not by the claim, which is the whole of its value: the role is stated by the claim, so the rows a later reader has to reason about are exactly the ones that stranded before it, and a claim-time stamp would be null on all of them. It costs one indexed read, on the detached half, after the ack. Window 1: a delivery that arrived on a bot route, whose binding moved since receipt, and whose resolution now finds no runtime at all, is left PENDING for the sweep instead of settling PROCESSED having looked at nothing. On an observer-only inbox that settle was a customer message lost from the only memory the inbox has, with nobody told. The same empty reading under an unmoved binding settles exactly as before — an inbox nothing of ours answers, or a bot that still owns an older conversation. Window 4: the recovery's refusal of bot equality across a rebind now needs two independent facts, the binding stamp AND the generation. The stamp alone refuses too much (the issue's own objection to it): an inbox bound while its traffic was in flight produces that pair, and the delivery is left DEAD with a customer unanswered. The generation alone refuses too much in the other direction: it counts observer movements too, which say nothing about whether bot equality proves the RESPONDER's role. A row with no generation keeps the stamp alone, which is what every row an older build wrote already had. The migration is additive and the previous release runs beside it: the inbox counter defaults to 0 and the delivery column stays null, which every reader here treats as "this row cannot say" rather than as generation zero. No backfill, deliberately — writing 0 onto an existing row would claim it arrived under the binding that stands today. Refs #540
…decided An observer beside a responder stays quiet about a message because the responder's OWN delivery of it folds it into memory. Which of the two happens was being answered by reading the responder agent's mode and switch at the moment the observer asked — and the two deliveries are concurrent by construction, one message fanned to two routes. A switch flipped between them leaves the observer silent about a message the responder never recorded, or appending a second copy of one it did. The claim now states what its own route does with a message it does not answer, beside the role it already states, and the observer reads that statement off the sibling row. A sibling that has not claimed yet says nothing and the mode reading stands, which is what every delivery did before this column — the window is narrowed to the gap between a sibling's insert and its claim, not closed, and a test asserts that fallback so a later reading cannot widen it. `routeIngests` is now the recorded fact plus the hand-off decided here, so what a delivery records and what it acts on cannot drift apart. Refs #540
`observeInbox` attached the bot on Chatwoot and only then committed `InboxObserver`, so inside that window the receiver had nothing to read: no row, and — when a promotion committed in the same window — not even the monitoring mode that was standing in for the row. A delivery landing there was read as the responder's or as nobody's, and the compensating detach undoes an attachment, not a delivery already enqueued. The row now goes in first with no stamp and is stamped when the fork answers, so the window states itself. What the row MEANS therefore changed, from "Chatwoot agreed" to "this inbox is spoken for": every reader that asks whether an agent observes an inbox counts a pending row, because the attachment may already be live upstream and answering no is the unsafe side. Three ask something narrower and read the stamp — the receiver, which reports the attach window; the observe tick, which retries rather than acting on a binding that has not landed; and the compensation, which must not read its own pending row as somebody else depending on the attachment. Counting the pending row is also what closes the window at its source: a mode change and a deletion are both refused while an agent observes anything, so the promotion that used to slip through the window no longer commits at all. Two tests that asserted the old race now assert that refusal. The audit line keeps its old meaning by reading only confirmed rows: the state before this call is not the same as the rows before this write, since the pending row is one of them. `attached_at` defaults to now, and the default is the rolling deploy: the previous release names no such column, so every row it writes lands confirmed rather than as an attach that never lands. It is also the backfill — ADD COLUMN with a default fills the existing rows, with none of the silent zero-row failure a data statement over a FORCE-RLS table has. Refs #540
The observer section named five open windows and pointed at issue #540 for all of them. It now says what closed each — the delivery's binding generation at receipt, the claim's record of whether its route remembers, the sweep's own verdict for a row that stranded before any claim, and the observer row written before the fork is asked — and what is left, which is narrowed rather than closed and is named where it happens: a sibling that has not claimed yet says nothing about its memory, and the reattach loop's read-then-write is a binding race older than this feature and not something a delivery's counter can hold. Refs #540
fazer-ai-bot
left a comment
fazer-ai-bot
left a comment
There was a problem hiding this comment.
🔍 Codex Review
Several binding transitions can bypass or predate the new generation fence, allowing stale route resolutions to be accepted. The pending-observer and sibling lookup changes also introduce race-dependent attachment and ingestion errors.
Reviewed commit: 3ad1e320f6 · gpt-5.6-sol · effort high
| // READ BEFORE THE ROW IS WRITTEN and carried into it (issue #540): what the delivery records has | ||
| // to be the world it ARRIVED in, and every reading taken later is about a world an administrative | ||
| // write may already have moved. One indexed read, on the detached half — the 200 is long since | ||
| // out, so it costs the ack nothing. | ||
| const bindingGeneration = await inboxBindingGenerationAt( |
There was a problem hiding this comment.
When a binding change commits after the webhook is received but before this detached database read completes, the ledger records the post-change generation rather than the generation at receipt. If that change removes the runtime, route resolution sees the same generation and settles the row as processed instead of leaving it for the sweep, recreating the silent-loss window this fence is intended to close. Capture the generation on the receive path rather than after dispatch.
codex · gpt-5.6-sol · effort high · confidence 0.98
There was a problem hiding this comment.
Fixed, and not on the receive path. That path is deliberately read-free and the reason is measured (issue #228): the ack waits on the pool it shares with every turn, ingest and compaction in the process, and Chatwoot takes the bot off a conversation when the ack is slow — putting a query there trades a bounded gap for the failure this module already paid for once.
What was closable is closed: the generation is now read INSIDE the transaction that writes the ledger row (recordDelivery, inboxBindingGenerationIn), so the read and the insert commit together and a binding landing between them can no longer put a world on the row that the message never arrived in — which was the actual read-then-write gap.
What is left is the hop between the ack and the detached task, and it is strictly shorter than the hop between Chatwoot emitting the event and our receiving it. No column written on this side can cover that one, so the residual is stated in the code rather than claimed away.
| // ./../chatwoot/management.ts. Deleting an agent unbinds every inbox it answered, which is | ||
| // the same movement `bindInbox`'s unbind makes and has to be counted the same way — a | ||
| // delivery in flight would otherwise re-derive its route from a binding that is gone and | ||
| // read the counter as saying nothing had changed. The `where` already names only inboxes | ||
| // this agent held, so every row it touches really moved. | ||
| bindingGeneration: { increment: 1 }, |
There was a problem hiding this comment.
softDisconnectChatwootInstance still clears agent_id with the raw update at management.ts:984-990 without incrementing binding_generation. For a delivery already stamped before a disconnect, the later empty route resolution therefore sees equal generations and settles the delivery even though the responder binding was removed. Include the generation increment in that same raw update.
codex · gpt-5.6-sol · effort high · confidence 0.99
There was a problem hiding this comment.
Fixed, and by removing the list rather than adding to it. softDisconnectChatwootInstance is the second site this finding is really about, and there was no reason to expect it to be the last — so binding_generation is now stepped by a database trigger (20260907090003_binding_generation_triggers) and never from application code. The four explicit increments are gone, and the disconnect, the delete-agent unbind and anything added next are counted without anybody remembering to.
New test: the counter follows writers that never call bindInbox: a raw unbind, and an account disconnect.
| -- ADDITIVE AND COMPATIBLE WITH THE PREVIOUS RELEASE SERVING BESIDE IT, which is what a rolling | ||
| -- deploy requires (docs/deploy.md). The inbox counter has a default, so a release that does not know | ||
| -- the column writes rows that read as generation zero and never move it -- which is exactly what | ||
| -- "nothing has changed" should look like. The delivery column is nullable and the previous release | ||
| -- leaves it null, which every reader added here treats as "this row cannot say" rather than as |
There was a problem hiding this comment.
During the documented rolling deployment, the previous release can still bind, unbind, observe, or unobserve inboxes without updating this new column. The default keeps those inboxes at generation zero even when their bindings move, so new readers can treat a stale route derivation as current and silently settle a delivery. The migration needs a database-level mechanism that also counts writes from the old release, or the deployment cannot safely overlap versions.
codex · gpt-5.6-sol · effort high · confidence 0.97
There was a problem hiding this comment.
Fixed, same change as the sibling finding, and this is the half that made a trigger the only honest answer. An application-side counter stands still for the whole length of the overlap exactly while a reader on the new release is trusting it, which is worse than no counter: it turns "the world may have moved" into a confident "it did not".
The trigger is the database-level mechanism you asked for — BEFORE UPDATE ON inboxes WHEN (OLD.agent_id IS DISTINCT FROM NEW.agent_id) plus AFTER INSERT OR DELETE ON inbox_observers — so the previous release counts without knowing the column exists. The test exercises the raw statement shape that release writes.
| let wrotePendingRow = false; | ||
| if (!alreadyObserving) { | ||
| try { | ||
| await runScopedOn(base, ctx, (db) => | ||
| db.inboxObserver.create({ | ||
| // EXPLICITLY NULL, against the column's own default. The default exists so that anything | ||
| // which does not know about pending rows — the previous release during a rolling deploy, a | ||
| // fixture, a repair by hand — writes a confirmed one; this is the single writer that means | ||
| // the null. | ||
| data: { tenantId, inboxId, agentId, attachedAt: null }, |
There was a problem hiding this comment.
With two overlapping observes of the same pair, the second preflight sees the first call's new pending row and sets alreadyObserving to true. If the first call then fails and deletes that row while the second POST succeeds upstream but returns an ambiguous error, the second catch skips both row cleanup and detachment because it believes a pre-existing binding existed, leaving an observer attachment with no database row. Track confirmed and pending rows separately for compensation.
codex · gpt-5.6-sol · effort high · confidence 0.91
There was a problem hiding this comment.
Fixed. alreadyObserving now means a CONFIRMED row (attachedAt !== null), so another call's pending row no longer reads as a binding to defer to. Read as not-yet-observing, this call's insert loses to the unique index — which changes nothing, since the row it wanted is already there — and its compensation asks the same question every other one asks: does a COMPLETED call still depend on this attachment.
New test: an observe that meets another call's pending row still takes its own attachment back, which fails without the change.
| chatwootInstanceId: instanceId, | ||
| conversationId, | ||
| ...(message.column === "inbound" | ||
| ? { inboundMessageId: message.id } | ||
| : { humanReplyMessageId: message.id }), | ||
| routeAgentBotId: responderBotId, |
There was a problem hiding this comment.
For an inbound message, both its message_created delivery and a later transcription message_updated delivery can carry the same inboundMessageId. This query can therefore select the update rather than the current event's responder sibling; if the responder mode changed between those events, the observer uses the wrong routeRemembers value and may duplicate or omit memory ingestion. Include the current event in the sibling identity.
codex · gpt-5.6-sol · effort high · confidence 0.9
There was a problem hiding this comment.
Fixed: the query now matches event as well. You are right that both the creation and the transcription write-back name the same inboundMessageId (that is issue #478's own doing), so without it the observer could read the other delivery's decision — and where the mode moved between the two, it is exactly the wrong one.
New test: a sibling delivery of a different event does not answer for this one, which fails without the filter.
…tion's Round 1 of the review, five findings, all fixed. The counter was stepped by the call sites that move a binding, and a counter kept that way is only as good as the list of writers somebody remembered. Two holes on the first pass, and they are the same hole twice: an account disconnect, which unbinds every inbox of an account with a raw UPDATE of its own, and the PREVIOUS RELEASE, which moves bindings for the whole length of a rolling deploy and names no such column at all — so a reader on the new release takes a stale route derivation for a current one, the single reading the column exists to refuse. A trigger has no list: this release counts, the previous one counts, a repair by hand counts. The generation is now read inside the transaction that writes the ledger row, so a binding committing between the read and the insert can no longer put a world on the row that the message never arrived in. The review asked for the receive path instead; that path is deliberately read-free (issue #228, measured), and what is left uncovered — the hop between the ack and the detached task — is strictly shorter than the emission-to-receipt hop no column written on this side can ever cover. `alreadyObserving` now means a CONFIRMED row. Reading another call's pending row as one was the worst of both answers: this call wrote no row AND its compensation skipped the detach, so a failure on the other side left an attachment upstream with nothing here naming it. The sibling whose memory decision the observer reads is now matched on the event too. One customer message reaches the ledger twice — the creation and the transcription write-back — and both name it through `inboundMessageId`, so without this the observer could read the other delivery's decision. The RLS-bypass rule stops counting a trigger function's body as the migration's own DML: that statement runs later, once per row, in the writer's own transaction and under the writer's own context. A `DO` block deliberately still counts, since a backfill wrapped in one is exactly what the rule is for. Refs #540
fazer-ai-bot
left a comment
fazer-ai-bot
left a comment
There was a problem hiding this comment.
🔍 Codex Review
The pending-observer state can become permanently inconsistent when an existing bulk-repair path attaches it remotely without marking it confirmed. This leaves observation work retrying despite a live Chatwoot attachment.
Reviewed commit: 5d02bbb9b2 · gpt-5.6-sol · effort high
| db.inboxObserver.create({ | ||
| // EXPLICITLY NULL, against the column's own default. The default exists so that anything | ||
| // which does not know about pending rows — the previous release during a rolling deploy, a | ||
| // fixture, a repair by hand — writes a confirmed one; this is the single writer that means | ||
| // the null. | ||
| data: { tenantId, inboxId, agentId, attachedAt: null }, |
There was a problem hiding this comment.
If the process dies after creating this null-stamped row but before its own attach/stamp, a later ensureAgentBotAndReattach—such as reconnectInbox(..., { always: true }) for another inbox of the same persona—includes it because its queries ignore attachedAt. The helper successfully attaches the observer remotely but never stamps the row, so Chatwoot sends events while the receiver and observe jobs indefinitely classify the binding as still attaching. Either exclude pending rows from bulk reattachment or stamp them after a successful remote attach.
codex · gpt-5.6-sol · effort high · confidence 0.95
There was a problem hiding this comment.
Fixed by the first of your two options: the bulk reattach now reads confirmed bindings only.
The second one is worse, and it is worth saying why rather than just picking. Stamping the row here confirms a call that has not finished, and the stamp survives both halves of that call's compensation — the delete takes only UNSTAMPED rows, and the detach skips a binding that stands — so a refused observe would be left with a row and an attachment. Skipped, the two sides agree: nothing attached upstream, and a row that says the observe never completed. Observing again settles both in one call, which is the repair the console already offers for every other leak on this path.
New test: the bulk reattach passes over an observer row Chatwoot never confirmed, which fails without the change.
…firmed Round 2 of the review, one finding. `ensureAgentBotAndReattach` walks a persona's bindings and re-asserts them upstream, and its observer query did not care whether Chatwoot had ever agreed to one. A pending row left by a process death was therefore attached for real while nothing stamped it, so the fork delivered to a bot whose row still said "attaching" — which every reader added by window 5 believes indefinitely: the observe tick retries for good and the receiver keeps reporting a window that will never close. Stamping it there instead would be worse. The call that wrote the row can still be refused, and a stamp survives both halves of its compensation — the delete, which only takes unstamped rows, and the detach, which skips a binding that stands — leaving a row for an observe that was turned down. Skipped, the two sides agree: no attachment upstream, and a row that says the observe never completed. Observing again is the repair, and it settles both in one call. Refs #540
fazer-ai-bot
left a comment
fazer-ai-bot
left a comment
There was a problem hiding this comment.
🔍 Codex Review
The patch leaves a pending-observer race in the reattachment path, weakens the RLS migration guard by stripping executable DO blocks, and introduces a non-restartable multi-statement trigger migration. These issues can produce orphaned Chatwoot attachments or block/incorrectly validate deployments.
Reviewed commit: cb0a2f6498 · gpt-5.6-sol · effort high
| // Skipped, the two sides agree: no attachment upstream, and a row that says the observe | ||
| // never completed. The repair is the one the console already offers for it — observing | ||
| // again, which asks the fork and stamps the row in its own transaction. | ||
| attachedAt: { not: null }, |
There was a problem hiding this comment.
If a confirmed observer from the initial snapshot is unobserved and a new observe inserts an unstamped row before this loop reaches it, both stands and stillStands accept that pending row because their predicates omit attachedAt. The helper can then attach a bot for an observe it does not own and report that attachment as healthy; if the owning observe subsequently aborts before learning the bot ID, its cleanup cannot detach this attachment, leaving Chatwoot delivering to an observer no confirmed row names. Apply the confirmed-row predicate to both rechecks as well.
codex · gpt-5.6-sol · effort high · confidence 0.93
There was a problem hiding this comment.
Fixed: both rechecks now carry the same predicate as the snapshot. You are right that the snapshot is a snapshot — and the failure it opens is the nastier direction, since the loop reports the attachment healthy while the observe that owns the row cannot detach it (it never learned the bot id).
New test: the reattach re-asks for the stamp, not just for a row, on each inbox it reaches, which changes a row mid-loop and fails without the recheck.
| // Replaced by a marker rather than deleted, so a second function in the same file is still found | ||
| // by the next pass and the offsets before it are untouched. | ||
| out = `${out.slice(0, openAt)}FUNCTION_BODY${out.slice(closeAt + tag[0].length)}`; |
There was a problem hiding this comment.
Replacing only the dollar-quoted body leaves the original CREATE FUNCTION header in out, so resetting re.lastIndex makes the next iteration match the same function again and treat the next dollar tag—including a later DO $$ block—as another function body. Existing migrations such as 20260903120000_rename_http_tools_named_after_natives and 20260827000000_rls_split_tenant_and_fleet_policies have exactly this function-then-DO shape, so tablesWrittenBy/tablesReadBy now discard executable migration DML and can let an unbracketed FORCE-RLS migration pass; remove the declaration along with its body or advance the scan beyond it.
codex · gpt-5.6-sol · effort high · confidence 0.99
There was a problem hiding this comment.
Fixed, and this one was worse than the false positive it removed — thank you for catching it. Leaving the header behind and rescanning from the start matched the same CREATE FUNCTION again and swallowed the next dollar-quoted block as its body, which on 20260827000000_rls_split_tenant_and_fleet_policies and 20260903120000_rename_http_tools_named_after_natives is a DO $$ that really does run: the rule would have gone on reporting clean while reading none of it.
The header now goes with the body and the scan resumes after it. Two tests added for exactly those shapes: function-then-DO, and two functions in one file each followed by a statement.
| CREATE TRIGGER inboxes_bump_binding_generation | ||
| BEFORE UPDATE ON "inboxes" | ||
| FOR EACH ROW | ||
| WHEN (OLD.agent_id IS DISTINCT FROM NEW.agent_id) | ||
| EXECUTE FUNCTION bump_binding_generation_on_inbox(); |
There was a problem hiding this comment.
If deployment is interrupted after this first trigger is created but before the second one is installed, resolving the failed migration as rolled back and redeploying will fail immediately with duplicate_object at this statement, leaving the rollout blocked until someone manually edits database state. Migration files are explicitly non-transactional in this repository (.claude/rules/prisma.md:26-29), so both trigger creations need restart-safe handling such as CREATE OR REPLACE TRIGGER or an appropriate conditional drop.
codex · gpt-5.6-sol · effort high · confidence 0.9
There was a problem hiding this comment.
Fixed: CREATE OR REPLACE TRIGGER on both. A migration file is not a transaction here, so an interruption between the two statements would meet duplicate_object on the retry and stop the rollout until somebody edited database state by hand.
Replace rather than a conditional drop, and the difference matters for this particular object: a DROP TRIGGER IF EXISTS followed by a CREATE leaves a window in which a write moves a binding and nothing counts it, which is the one thing the counter is for. CREATE OR REPLACE TRIGGER needs Postgres 14 and docs/deploy.md pins pgvector/pgvector:pg17.
…ce the trigger Round 3 of the review, three findings. The reattach's two rechecks now ask for a confirmed row like the snapshot does. A binding confirmed when the list was read can be unobserved, and a new observe insert its unstamped row, before the loop reaches that inbox: read without the stamp, the loop attaches a bot for an observe it does not own and reports the attachment healthy, and if that observe then aborts before it learns the bot id, its own compensation cannot detach what the loop put there. `stripFunctionBodies` was worse than the false positive it removed. It replaced the body but left the `CREATE FUNCTION` header behind and rescanned from the start, so the same header matched again and swallowed the next dollar-quoted block as if it were its body — and two existing migrations have exactly the function-then-`DO` shape, so the RLS rule quietly stopped reading DML that really does run. The header goes with the body now and the scan resumes after it, with tests for function-then-DO and for two functions in one file. Both triggers are created with `CREATE OR REPLACE`. A migration file is not a transaction here, so a deploy interrupted between the two statements would meet `duplicate_object` on the retry and stop until somebody edited database state by hand. Replace is also atomic where a drop-then-create leaves a window in which a write is not counted at all. Refs #540
fazer-ai-bot
left a comment
fazer-ai-bot
left a comment
There was a problem hiding this comment.
🔍 Codex Review
The pending-row implementation still permits the promotion race it is intended to close, leaving an invalid production observer binding. The new route-memory fact can also become inconsistent with the delivery's actual ingestion behavior and cause duplicate memory entries.
Reviewed commit: 0952e8d785 · gpt-5.6-sol · effort high
| // which does not know about pending rows — the previous release during a rolling deploy, a | ||
| // fixture, a repair by hand — writes a confirmed one; this is the single writer that means | ||
| // the null. | ||
| data: { tenantId, inboxId, agentId, attachedAt: null }, |
There was a problem hiding this comment.
When updateAgent locks the agent and counts zero observers immediately before this insert, the insert's foreign-key KEY SHARE lock remains compatible with FOR NO KEY UPDATE, so both transactions can commit. The final mode check then sees this call's pending row in before.observers and exempts it as though it were a previously confirmed binding, allowing the row to be stamped after the agent becomes production and leaving a production agent on a non-answering observer route. Only a row with attachedAt !== null should qualify for that exemption, or the pending insert must serialize with mode updates.
codex · gpt-5.6-sol · effort high · confidence 0.96
There was a problem hiding this comment.
Fixed: only a row with attachedAt !== null exempts. You are right about the lock compatibility — KEY SHARE from the foreign key and FOR NO KEY UPDATE on the agent do not conflict, so the promotion and the pending insert both commit, and the exemption was then reading the row this same call had just written. It is the worst shape this feature can produce: window 5 reached through the fix for window 5.
New test: a promotion that raced past updateAgent's own refusal is still caught by the mode recheck, which lands the promotion with a raw UPDATE (what the race leaves behind) and fails without the change.
| const routeRemembers = | ||
| rt === null | ||
| ? false | ||
| : rt.enabled && (ingestsContinuously(rt.mode) || observer !== null); |
There was a problem hiding this comment.
If the responder delivery claims while its runtime is in test mode, this records false; however, when that agent changes to monitoring before the turn or gate completes, the existing handedToObserver path still makes this same delivery ingest the message. The sibling observer now trusts the persisted false over the current mode and ingests the same message too, duplicating the shared contact-inbox memory. The recorded fact must account for this later handoff, or false cannot be treated as authoritative for such deliveries.
codex · gpt-5.6-sol · effort high · confidence 0.91
There was a problem hiding this comment.
Fixed: the row is corrected when the hand-over happens, guarded on routeRemembers: false so it can only ever move to true — a redelivery or a replay leaves a row that already says true alone.
Correcting it rather than widening what false means, because false has to stay authoritative: it is the whole of what the observer reads instead of a mode that may have moved. Writing null on any delivery that could hand over would put every test-mode responder back on the mode reading, which is the window this column closed.
Best-effort and not thrown: this delivery's own work is the ingestion, and taking that away to report a stale sibling fact would trade a duplicate line in memory for a missing one.
Assertion added to a TEST agent flipped to monitoring inside its turn still hands the message to the observer — the row now has to say true — and it fails without the correction.
…a binding Round 4 of the review, two findings. The mode recheck exempted this call's own pending row. `updateAgent` refuses a mode change while the agent observes anything, but the two writes do not serialize: it counts observers and locks the agent `FOR NO KEY UPDATE`, and the pending insert's foreign key takes only `KEY SHARE`, which is compatible — so a promotion and the pending row can both commit. The exemption then saw the row this call had just written, skipped the refusal and stamped a confirmed observer binding for an agent that answers: the exact state window 5 exists to prevent, reached through the fix for it. Only a confirmed row exempts now. `routeRemembers` recorded `false` for a responder whose runtime was in test mode at the claim, and a flip to monitoring discovered mid-delivery (`handedToObserver`) then made that same delivery fold the message in anyway. The observer beside it trusts the recorded fact over the current mode, so the stale `false` would have it append the same message to the same contact-inbox thread a second time. The row is corrected when the hand-over happens, guarded so it can only move `false` to `true`. Refs #540
fazer-ai-bot
left a comment
fazer-ai-bot
left a comment
There was a problem hiding this comment.
🔍 Codex Review
Pending observer rows can become permanent, silently unhealthy bindings, and their cleanup introduces a lock-order inversion with existing binding operations. Both issues can leave local and Chatwoot observer state inconsistent.
Reviewed commit: 6f138a0134 · gpt-5.6-sol · effort high
| // which does not know about pending rows — the previous release during a rolling deploy, a | ||
| // fixture, a repair by hand — writes a confirmed one; this is the single writer that means | ||
| // the null. | ||
| data: { tenantId, inboxId, agentId, attachedAt: null }, |
There was a problem hiding this comment.
If the process exits after this committed insert but before the final upsert, no timeout or reconciler ever settles the null stamp. toInboxDto still exposes the row as an observer, while reconcileInboxBots only checks whether the persona's bot exists; when that bot already exists for another inbox, the UI reports this inbox as active and offers no reconnect even though addInboxObserver may never have run, and observation jobs treat it as attaching indefinitely. Surface pending rows as unhealthy or add durable expiry/recovery.
codex · gpt-5.6-sol · effort high · confidence 0.96
| await runScopedOn(base, ctx, (db) => | ||
| db.inboxObserver.deleteMany({ | ||
| where: { tenantId, inboxId, agentId, attachedAt: null }, | ||
| }), |
There was a problem hiding this comment.
When this cleanup races bindInbox or unobserveInbox, the DELETE locks the observer row first and its new AFTER DELETE trigger then waits to update the inbox, while the competing transaction deliberately locks the inbox first and then waits to delete the same observer row. That lock inversion can produce PostgreSQL 40P01; because this path catches the error and continues, it may leave a pending local row while remote compensation detaches the observer. Acquire the inbox lock before this delete, matching the other observer-removal paths.
codex · gpt-5.6-sol · effort high · confidence 0.92
2 participants
Closes #540.
A Chatwoot delivery did not record under which binding state it arrived. Every reader of the route's role — the live path, the sweep, the recovery — rebuilt that role from the binding as it stands now, and the binding can have moved between the emission of the event and the reading. #476 (PR #505) closed what ordinary traffic reaches; five windows were left, each needing a concurrent administrative operation within seconds of a specific delivery. The rollout of the observer is exactly when those operations happen.
Five commits, one layer each, every one with tests that fail without it.
The delivery records the world it arrived in
inboxes.binding_generationcounts every write that moves who routes an inbox — responder bound, rebound or unbound, observer attached or detached, agent deleted — in the same transaction as the binding itself. The ledger row records that counter at INSERT, not at the claim, and that is the whole of its value: the role is stated by the claim, so the rows a later reader has to reason about are exactly the ones that stranded before it.The route states what it does with a message it does not answer
chatwoot_webhook_deliveries.route_remembers, written by the claim beside the role. Window 3: an observer beside a responder stands down because the responder's own delivery folds the message into memory, and that was decided by reading the responder's mode at the moment the observer asked — with the two deliveries concurrent by construction. It now reads the sibling's own statement. A sibling that has not claimed yet says nothing and the mode reading stands, which is what every delivery did before; that fallback has a test of its own so a later reading cannot widen it.A row that never claimed states no role
Window 2: a process dying between the INSERT and the claim left a row read as
owed-takeover, marked PROCESSED and armed for a recovery that restores no ingestion and reports no gap.claimed_at === nullis the fact that nothing stated the role, and the sweep now gives it a verdict of its own that does both honest things: it arms the takeover — free where it was not owed, since the recovery re-asks every gate — and files the gap line, instead of guessing between the two stories.The observer row is written before the fork is asked
Window 5:
observeInboxattached on Chatwoot and only then committedInboxObserver, so inside that window the receiver had no row and — after a concurrent promotion — not even the monitoring mode that stood in for it. The row now goes in unstamped and is stamped when the fork answers. What the row MEANS changed, from "Chatwoot agreed" to "this inbox is spoken for": every reader that gates a refusal counts a pending row, and three read the stamp itself — the receiver (which reports the attach window), the observe tick (which retries rather than acting on a binding that has not landed) and the compensation (which must not read its own pending row as somebody else depending on the attachment).Counting the pending row also closes the window at its source: a mode change and a deletion are both refused while an agent observes anything, so the promotion that used to slip through no longer commits at all. Two tests that asserted the old race now assert that refusal.
Migrations
Three, all additive, all safe with the previous release serving beside them (docs/deploy.md):
inboxes.binding_generation INT NOT NULL DEFAULT 0chatwoot_webhook_deliveries.binding_generation INT NULLandroute_remembers BOOLEAN NULL— null is "this row cannot say", never generation zero and neverfalseinbox_observers.attached_at TIMESTAMP NULL DEFAULT CURRENT_TIMESTAMP— the default is the rolling deploy (the previous release names no such column, so its rows land confirmed) and it is also the backfill, with none of the silent zero-row failure a data statement over a FORCE-RLS table hasdocs/chatwoot.mdnow says what closed each window and what is left.