feat(queue): one AI answer per ticket, arbitrated by the database - #191
Conversation
Split out of #187 so the response lifecycle is reviewable on its own. Rebuilt on main rather than rebased — the pre-split branch predates the #180 hotfix and replaying it would have deleted the SystemConfig repair migration, the schema-drift guard and its CI gate. PR #172 shipped two layers of the rule: reply paths no longer enqueue, and a re-answer gate reads the ticket's history. Both are check-then-act against a snapshot, so neither survives two jobs running at once — and that needs no duplicate inbound event. runWithTimeout is a Promise.race that does not cancel the handler, AI_RESPONSE has a 120s timeout over a deliberately slow pipeline, and it runs at concurrency 4. On timeout attempt 1 keeps going while attempt 2 starts, reads messages before attempt 1's message.create, and both post. The database now arbitrates. Message.responseKey with a unique index on (ticketId, responseKey) means exactly one PRIMARY_AI_RESPONSE per ticket, claimed BEFORE any platform post-back, so the loser catches a narrowly-identified P2002 and returns without posting. Nullable on purpose: historical BOT rows stay valid and still prove a ticket was answered. Delivery is a state machine. responseState is PENDING -> DELIVERED | ESCALATED, every transition a compare-and-set via updateMany guarded on responseKey and current state, never a blind update by id. Two sub-states live in their own columns rather than as string prefixes in responseError: deliveryConfirmed (the post succeeded but the state write did not, so a retry repairs instead of reposting) and escalationRequiredReason (a human handoff is owed and not yet durable). They are columns and not enum values because responseState records the OUTCOME and both describe a response that is still PENDING — folding them in would destroy the outcome they are sub-states of. Findings from the review round on #187, all fixed here: STRANDED RESPONSES HAD NO SWEEPER. Four reviewers. Only the owning job could advance a PENDING response, so when that job dead-lettered — and a delivery failure is exactly what exhausts MAX_JOB_ATTEMPTS — the response stayed PENDING forever, no human was summoned, and the reporter got silence. Precisely the failure the state machine exists to prevent. A new PENDING_RESPONSE_SWEEP recurring handler settles them: escalating through the existing enqueueEscalationAtomically, repairing deliveryConfirmed rows to DELIVERED, guarded by both a 20-minute age threshold derived from RESPONSE_RECOVERY_AFTER_MS and an authoritative live-owning-job check so it cannot race the owner. Backed by @@index([responseState, createdAt]) — without it the sweep is a full scan of the fastest-growing table on a fixed schedule. RECOVERY PATHS DROPPED AN OWED ESCALATION. Four reviewers, and my own fault twice over. enqueueEscalationAtomically returns false when its compare-and-set matched no rows, meaning nothing was queued. Both recovery functions surfaced that in their payload and returned success anyway. Worse: worker.ts discards result.data entirely and writes only status COMPLETED, so escalated: false was never persisted anywhere — a dropped handoff left zero trace. Both functions now read the row's actual responseState and split the cases: DELIVERED succeeds honestly, ESCALATED reports true, PENDING/missing/unreadable fails loudly. Failing is bounded — worker.ts dead-letters at MAX_JOB_ATTEMPTS = 5, so a permanently unqueueable escalation converges to a visible DEAD_LETTER row rather than looping; success: true was the branch with no bound on the damage. When the earlier fix for the main path was dispatched, its instructions said the recovery functions already did this correctly and to mirror them. That came from the split brief and was wrong: they read the value but never acted on it. The main path got fixed against a false premise and the recovery paths kept the bug. THE ESCALATION TRANSACTION ERASED ITS OWN DIAGNOSTIC. It set responseError: null, wiping the delivery-failure text written moments before — in the one path whose reader most needs it. Now only escalationRequiredReason is cleared, and recoverPendingResponse renders the preserved error into the escalation reason. An orphaned escalationRequiredReason could survive a DELIVERED result, leaving a row saying a human is required next to a state saying delivered, which nothing reads. The marker is now cleared when DELIVERED is accepted, and a failed clear fails the job rather than returning a hidden contradiction. Unreachable today — only this handler writes those columns — pinned so it stays that way. The confirmed-delivery repair omitted the responseKey === PRIMARY_AI_RESPONSE guard its sibling branches carry, so a non-primary AI BOT row could be repaired as if it were the ticket's one answer. Genuinely unreachable today (legacy rows carry the deliveryConfirmed=false / responseState=null defaults); guard added for consistency against a future writer. A null ticket description was string-concatenated into the classifier input as the literal "null". Now a filtered join, so an absent description contributes nothing. TEAMS, because both depend on aiJobEnqueued: The AI job was enqueued before the Teams conversationReference was persisted, so the worker could claim it in between and fall back to a hardcoded global serviceUrl, failing delivery for tenants in other regions. InboundHandler.handle now accepts ticketAdditionalInfo and writes it in the same insert as the ticket, before createJob; the post-hoc update is gone and the ordering is pinned via mock.invocationCallOrder. Deliberately not forwarded to the orphaned-reply path. Reporter-controlled title text was rendered into a Markdown-capable Adaptive Card TextBlock. Now escaped in the builder — chosen over stripping or RichTextBlock/TextRun, which depend on the client honouring non-Markdown inlines. The contract that the builder does not sanitize is updated accordingly. And a test defect I shipped in #172 and had to remove here: the buildResponseCard leak guard asserted no TKT- appeared while passing an input containing no TKT-, so it could not fail, under a comment claiming the input carried a leak. That was my hand-merge of two agents' rewrites of the same file — the exact failure mode the original fix existed to close. Since the card no longer accepts a displayId, responseText is rendered verbatim by design and no input can legitimately leak, so the tautological case is deleted and the sibling JSON.stringify guard is kept, parametrized, and proven able to fail by planting a ticketDisplayId in a submit payload. PENDING_RESPONSE_SWEEP is registered in apps/worker/src/index.ts here rather than deferred. Unregistered job types are never claimed (issue #154), so without the registration the sweeper would sit inert — a safety net that silently does not run is worse than one that fails loudly. Verification: turbo typecheck 10/10, turbo test 10/10 (1929 tests, up from 1765 on main). Every fix carries red-green verification; 20 mutations were each observed RED and restored to GREEN, including two applied to the recovery functions separately to prove they are independently covered. Not verified locally: the new index and its migration were not round-tripped against a real database — no Postgres or Docker in the sandbox. The merged CI drift gate runs migrate deploy against real Postgres and is the first real check.
jerelvelarde
left a comment
There was a problem hiding this comment.
Read this one closely rather than on green CI, as the description asks. The core design is right, and the specific thing that makes it right is that the claim happens before any platform post-back.
prisma.message.create with responseKey: PRIMARY_AI_RESPONSE_KEY runs at step 5, and the platform adapter call comes after it. That ordering is the whole fix: the @@unique([ticketId, responseKey]) index elects one winner while the losing job is still holding a generated string it has not sent anywhere. The pre-check on ticket history stays useful as a cost optimization but is correctly no longer load-bearing for correctness — which is exactly what #172 got wrong, since both of its layers were check-then-act against a snapshot.
Things I verified rather than took on trust:
enqueueEscalationAtomically is a genuine CAS. updateMany filtered on responseState: 'PENDING' inside a transaction, count !== 1 short-circuits, and the job.create lives inside the same transaction so an insert failure rolls the state change back and leaves the row retryable. Routing the sweep through this same function instead of a second hand-rolled transition is what makes repeated sweeps idempotent for free.
Leaving responseError alone in that transaction is the right call, and the comment explaining why is the most valuable one in the PR. The escalation that most needs a diagnostic is the delivery-failure one, and that text was written moments earlier — clearing it would have destroyed the only record of why the reporter got nothing, precisely as a human is asked to pick the thread up.
false from the CAS is never treated as success. Every caller re-reads responseState and only accepts DELIVERED (nothing was owed) or ESCALATED (someone else summoned the human); anything else is reported unhandled. The escalationSkippedState === 'DELIVERED' && requiredEscalationRecorded branch that clears the owed marker is a nice catch — without it a row would simultaneously claim "a human is required" and "delivered", and nothing could ever act on that pair since requiredEscalationReason only reads PENDING rows.
The sweep's two guards are both necessary and correctly ordered. Age off CURRENT_TIMESTAMP rather than the worker clock, and then the authoritative live-owner test reading job status directly. Treating a missing job row as not live is the right direction — a dead-lettered or JOB_CLEANUP-reaped job is exactly the stranding this exists to catch. STRANDED_RESPONSE_AFTER_MS = 4 × RESPONSE_RECOVERY_AFTER_MS clears the takeover delay plus its retry ladder with real headroom.
deliveryConfirmed taking precedence over escalation in the sweep mirrors the owning handler's prior-response gate, so an already-answered reporter never gets a human summoned at them. Good that the two paths agree by construction rather than by coincidence.
The sub-state columns being kept out of responseError is the right modelling call — that column is read as "what went wrong" on an ops surface, and folding lifecycle into it is how the previous version got confusing.
Including PENDING_RESPONSE_SWEEP registration in apps/worker/src/index.ts here rather than deferring is correct given #154: an unregistered type is never claimed, so a sweeper shipped without its registration is inert code that reads as a working backstop. Worth the small scope bleed.
The new @@index([responseState, createdAt]) is not optional — without it a 5-minute scheduled job full-scans the fastest-growing table in the schema. Good that it shipped with the sweep rather than after the first incident.
Two non-blocking nits, both in pending-response-sweep.ts:
settleStrandedResponsereturns'repaired'unconditionally on thedeliveryConfirmedbranch without inspectingupdateMany'scount. The escalation branch below it goes to real trouble to distinguish "settled by someone else" from "never settled", and argues in its own comment that discarding that distinction "would recreate the bug this handler exists to fix, one level up" — the repair branch should hold itself to the same standard. Benign today (the row is DELIVERED or ESCALATED either way, and the reporter has their answer), but it is the one place in the PR where a CAS result is dropped on the floor.- The zero-candidate early return omits
alreadySettledfrom itsdatashape while the normal return includes it, so anything readingdata.alreadySettledgetsundefinedon an idle run.
Ordering note: this shares schema.prisma with #190, which I have also approved and which should land first (it closes the only open security issue on the launch path). Expect a trivial conflict in the Message model — both add fields and a @@unique in the same block. Re-request review after the rebase if the approval gets dismissed.
Approving.
CPK-7924 2. Merge PR #191 — one AI answer per ticket, arbitrated by the database
PR #191 — What it closes
Why the merged #172 wasn't enough#172 shipped two layers — reply paths stop enqueueing, and a re-answer gate reads the ticket's history. Both are check-then-act against a snapshot, so neither survives two concurrent jobs, and that needs no duplicate inbound event: This replaces that with Worth a reviewer's attentionThe PR's own description is unusually candid about what the review round found — a stranded-response class with no sweeper, two recovery paths that dropped an owed escalation and left no trace, an escalation transaction that erased its own diagnostic, and a tautological test shipped in #172 that could not fail. That history is the reason this is worth reading closely rather than rubber-stamping on green CI. Claimed: 1,929 tests pass (up from 1,765 on
|
Resolves one conflict in packages/outpost/db/prisma/schema.prisma, in `model Message`. Both sides added distinct fields in the same region and neither touched the other's, so the resolution is a union: - this branch: `sourceMessageId` + @@unique([ticketId, sourceMessageId]), the indexed dedup slot for inbound provider message IDs (Postmark today). - main (#191): `responseKey`, `responseState`, `responseJobId`, `responseError`, `deliveryConfirmed`, `escalationRequiredReason`, @@unique([ticketId, responseKey]), and @@index([responseState, createdAt]) for PENDING_RESPONSE_SWEEP. The shared tail (`attachments`, `createdAt`) is kept once. The two unique constraints cover different columns and do not interact. Verified on the merged result, not assumed: - prisma validate (workspace-pinned binary): schema is valid - all five migrations present from both sides; none dropped in the merge - typecheck 10/10 - full suite green: 1075 shared, 679 web, every package passing
Fallout from merging main. This test survived the textual merge and then failed, which is the useful kind of conflict: `message-create.test.ts` had no textual conflict at all, so nothing flagged it. It asserted that a genuine reply enqueues AI_RESPONSE. Since #172 and #191, `InboundHandler` never enqueues for a reply on any platform — Outpost answers once per ticket, on the opening message, and a human owns the thread after that. The assertion was written before that rule existed. What the test is actually here to prove is that the `message.id === threadId` gate distinguishes the starter message from a reply, so it now asserts the reply still gets its Message record, plus an explicit check that nothing was enqueued. That pins the one-answer rule on this path too. The gate stays load-bearing: neutering it fails the two starter-message tests.
Two of three PRs split out of #187. Sibling: #190 (email/Postmark). Third (worker correctness) to follow; deferred work named at the bottom.
Rebuilt on
main, not rebased — the pre-split branch predates the #180 hotfix, and replaying it would have deleted theSystemConfigrepair migration, the schema-drift guard, and its CI gate.Why the rule needed a database behind it
#172 shipped two layers: reply paths no longer enqueue, and a re-answer gate reads the ticket's history. Both are check-then-act against a snapshot, so neither survives two jobs at once — and that needs no duplicate inbound event:
runWithTimeoutis aPromise.racethat does not cancel the handlerAI_RESPONSEhas a 120s timeout over a deliberately slow pipelineOn timeout, attempt 1 keeps running while attempt 2 starts, reads messages before attempt 1's
message.create, and both post.What this does
The database arbitrates.
Message.responseKey, unique on(ticketId, responseKey)— exactly onePRIMARY_AI_RESPONSEper ticket, claimed before any platform post-back, so the loser catches a narrowly-identified P2002 and returns without posting. Nullable on purpose: historicalBOTrows stay valid and still prove a ticket was answered.Delivery is a state machine.
PENDING → DELIVERED | ESCALATED, every transition a compare-and-set guarded onresponseKeyand current state — never a blindupdateby id.Two sub-states live in their own columns, not as string prefixes in
responseError:deliveryConfirmed(the post succeeded but the state write didn't, so a retry repairs rather than reposts) andescalationRequiredReason(a handoff is owed and not yet durable). Columns rather than enum values becauseresponseStaterecords the outcome and both describe a response stillPENDING— folding them in would destroy the outcome they're sub-states of.Findings from the review round, all fixed here
Stranded responses had no sweeper. Four reviewers. Only the owning job could advance a
PENDINGresponse — so when that job dead-lettered, and a delivery failure is exactly what exhaustsMAX_JOB_ATTEMPTS, the response stayedPENDINGforever, no human was summoned, and the reporter got silence. Precisely the failure the state machine exists to prevent.A new
PENDING_RESPONSE_SWEEPrecurring handler settles them: escalates through the existingenqueueEscalationAtomically, repairsdeliveryConfirmedrows toDELIVERED, guarded by both a 20-minute age threshold derived fromRESPONSE_RECOVERY_AFTER_MSand an authoritative live-owning-job check so it can't race the owner. Backed by@@index([responseState, createdAt])— without it the sweep is a full scan of the fastest-growing table on a fixed schedule.Recovery paths dropped an owed escalation, and left no trace at all. Four reviewers.
enqueueEscalationAtomicallyreturnsfalsewhen its CAS matched no rows, meaning nothing was queued — both recovery functions surfaced that in their payload and returned success anyway. Worse:worker.tsdiscardsresult.dataentirely and writes onlystatus: COMPLETED, soescalated: falsewas never persisted anywhere.Both now read the row's actual
responseStateand split the cases:DELIVEREDsucceeds honestly,ESCALATEDreports true,PENDING/missing/unreadable fails loudly. Failing is bounded —worker.tsdead-letters atMAX_JOB_ATTEMPTS = 5, so a permanently unqueueable escalation converges to a visibleDEAD_LETTERrow.success: truewas the branch with no bound on the damage.Worth recording how this survived: when the main path was fixed earlier, the instructions said "the two recovery functions already do this correctly — mirror that pattern." That came from the split brief and was wrong — they read the value but never acted on it. The main path was fixed against a false premise while the recovery paths kept the bug.
The escalation transaction erased its own diagnostic — it set
responseError: null, wiping the delivery-failure text written moments before, in the one path whose reader most needs it. Now onlyescalationRequiredReasonis cleared, andrecoverPendingResponserenders the preserved error into the escalation reason.An orphaned
escalationRequiredReasoncould survive aDELIVEREDresult, leaving a row saying a human is required beside a state saying delivered — a combination nothing reads. Cleared whenDELIVEREDis accepted; a failed clear now fails the job rather than returning a hidden contradiction.The confirmed-delivery repair omitted a guard its siblings carry (
responseKey === PRIMARY_AI_RESPONSE), so a non-primary AIBOTrow could be repaired as if it were the ticket's one answer. Genuinely unreachable today — legacy rows carry thedeliveryConfirmed=false/responseState=nulldefaults — guard added anyway, pinned.A null description became the literal
"null"in the classifier input via string concatenation. Now a filtered join.Teams, because both depend on
aiJobEnqueuedThe AI job was enqueued before the conversation reference was persisted, so the worker could claim it in between and fall back to a hardcoded global
serviceUrl, failing delivery for tenants in other regions.InboundHandler.handlenow acceptsticketAdditionalInfoand writes it in the same insert as the ticket, beforecreateJob. Ordering pinned viamock.invocationCallOrder. Deliberately not forwarded to the orphaned-reply path.Reporter-controlled title text was rendered into a Markdown-capable Adaptive Card TextBlock. Now escaped in the builder — chosen over stripping or
RichTextBlock/TextRun, which depend on the client honouring non-Markdown inlines.A test defect I shipped in #172 and removed here
The
buildResponseCardleak guard asserted noTKT-appeared while passing an input containing noTKT-— so it could not fail — under a comment claiming the input carried a leak. That was my hand-merge of two agents' rewrites of the same file: the exact failure mode the original fix existed to close. Since the card no longer accepts a displayId,responseTextis rendered verbatim by design and no input can legitimately leak, so the tautological case is deleted and the siblingJSON.stringifyguard is kept, parametrized, and proven able to fail by planting aticketDisplayIdin a submit payload.Registration is included, deliberately
PENDING_RESPONSE_SWEEPis registered inapps/worker/src/index.tshere rather than deferred to the worker PR. Unregistered job types are never claimed (#154), so without it the sweeper would sit inert — a safety net that silently does not run is worse than one that fails loudly. That file is not touched by the sibling worker PR, so there's no conflict.Tests
1,929 pass (up from 1,765 on
main), typecheck 10/10. 20 mutations each observed RED and restored to GREEN — including two applied to the recovery functions separately, to prove they're independently covered rather than one test carrying both.Not verified locally: the new index and its migration were not round-tripped against a real database — no Postgres or Docker in the sandbox. The merged CI drift gate runs
migrate deployagainst real Postgres and is the first real check on it.Siblings, follow-ups, and what is deliberately not here
availableSlotsis always max, per-type concurrency limits never bind, and job types starve behind 120sAI_RESPONSEwork. Also: the reclaim predicate missesPROCESSINGrows withlockedAt IS NULLand can permanently silence a scheduled type; fenced writes returningcount === 0are silent; andresult.datais discarded so no handler's outcome detail is observable./healthboot rework — deferred a second time, correctly. Four reviewers found it makes a healthy worker report 503 and get restarted mid-job, becauseSTALE_POLL_MSis 60s against 120s/300s job timeouts. It cannot land until the worker poll loop is fixed, because the poll loop is what makes it wrong. I argued for re-landing it against a reviewer's judgement and was wrong.runWithTimeoutstill doesn't cancel, so a timed-out attempt burns a pipeline slot even though it can no longer double-post · Pass message author identity into the AI conversation history #173 conversation history discardsMessage.author· Replace the one-response counter with a human-presence gate #174 whether "answer once, ever" should become a human-presence gate · Decide the generator's empty-response behaviour #178 generator empty-response behaviour · Document the Prisma partial-index drift on Ticket_email_sourceId_key #179 the raw-SQL partial index invisible tomigrate diff