fix(queue): make one-answer-per-ticket hold under concurrency and delivery failure - #187
fix(queue): make one-answer-per-ticket hold under concurrency and delivery failure#187NathanTarbert wants to merge 2 commits into
Conversation
…livery failure Rebuilt from main rather than rebased. PR #170 carried both the customer-facing fix and this infrastructure work; #172 took the fix and shipped, so what remains is the half that was never reviewed. A rebase would have replayed 24 commits that predate the #180 hotfix and, as the two-dot diff showed, would have DELETED the SystemConfig repair migration, the schema-drift guard in start.sh, and its CI gate. Rebuilding on current main keeps all three. #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. Two ways that happens today, neither needing a duplicate inbound event: - runWithTimeout in worker.ts is a Promise.race that does not cancel the handler. AI_RESPONSE has a 120s timeout over a deliberately slow pipeline, so on timeout attempt 1 keeps running while attempt 2 starts a second later, reads messages before attempt 1's message.create, and both post. - AI_RESPONSE runs at concurrency 4. The database now arbitrates. Message gains a nullable responseKey with a unique index on (ticketId, responseKey); exactly one PRIMARY_AI_RESPONSE row can exist per ticket, and the row is 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 rather than a hope. 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. A response starts PENDING before any external post; successful delivery marks DELIVERED; one needing a human stays PENDING until that escalation is durable, then becomes ESCALATED. If delivery itself ends PENDING, a retry schedules a delayed check that pulls in a human only if the response is still pending, so the one-post rule holds without racing the original handler. Job gains claimToken so a timed-out attempt cannot write back over the attempt that replaced it. Stale PROCESSING jobs are reclaimed after a grace period, and graceful shutdown shares one drain promise so signal handlers and the app cannot each half-drain. Also here: - generator.ts read only content[0], silently discarding text from multi-block model responses, and now concatenates all text blocks in order. It also throws on empty text rather than publishing nothing — unrequested scope, filed as #178 for an explicit decision. - Postmark ticket creation is idempotent, backed by a partial unique index on Ticket(sourceId) WHERE source = 'EMAIL'. Reply detection falls back to In-Reply-To and References so a reply without a plus-address stops becoming a second ticket with its own answer. - The Teams acknowledgement card no longer claims an AI is reviewing the question when no AI job was enqueued. Restores the /health rework that #180 deferred (d0c6f99). #180's own start.sh guard catches the SystemConfig cause; it does not catch the class. Any other boot-time failure — unreachable database, bad credentials, a throw inside buildSyncEngine unrelated to schema — still exits before the port binds, leaving Railway nothing to report but a five-minute timeout indistinguishable from a broken image. The port now binds first, boot state is tracked, and /health answers 503 with the phase and reason while boot is unfinished or failed. Fail-fast is retained: an unbooted worker is never reported healthy, because its sync mappings come from the database and one running against a schema it does not match would write wrong statuses to Linear. railway.toml drops healthcheckTimeout from Railway's 300s default to 180s, above BOOT_FAILURE_LINGER_MS, so the probe reads the reason before the process exits to be restarted. This is the reviewed version of that work recovered from d0c6f99~1, not a rewrite — it carries the review rounds that made the boot-failure path reachable and shutdown survive a signal during boot. Worker tests go 2 -> 24. Verification: turbo typecheck 10/10, turbo test 10/10 (1924 tests). Confirmed by diff that start.sh, both Dockerfiles, .github/workflows/ci.yml, .env.example and the 20260812 SystemConfig migration are byte-identical to main. Three findings from the split brief are NOT fixed here and block merge: escalationEnqueued is assigned and never read in the non-recovery path; the In-Reply-To/References resolution trusts attacker-controlled headers, so anyone holding a Message-ID from a thread can append to that ticket; and responseError carries DELIVERY_CONFIRMED / ESCALATION_REQUIRED control state in a free-text error column, in the same change that introduced an enum for exactly that.
…elta The split brief named three findings that had to be fixed before this work could merge. All three lived in the half of PR #170 that never had a review pass. 1. escalationEnqueued was assigned and never read on the main path. enqueueEscalationAtomically returns false when its compare-and-set found the response row in a state other than PENDING, meaning no escalation was queued. That false was discarded: nothing logged, and the handler returned success with `escalated` computed from local conditions rather than from what actually happened. A response that reached nobody and summoned nobody reported itself handled. A no-op CAS now reads the row's actual responseState and splits the two cases that were previously identical. DELIVERED means no escalation was needed, so the job succeeds honestly with escalated: false. ESCALATED means someone else already queued it, so escalated: true. PENDING, missing, or unreadable means the handoff did not happen and the job fails loudly. `escalated` is now derived from the durable handoff instead of inferred. 2. Header-based email reply resolution trusted attacker-controlled input. findTicketByReplyMessageIds matched candidate IDs from In-Reply-To and References against Ticket.sourceId and Message.attachments.postmarkMessageId. Those headers are sender-supplied, and a Message-ID from a thread is KNOWN to every participant who was ever CC'd rather than having to be guessed. Anyone holding one could append a message to that ticket and reopen it. No AI response is spent on a reply and nothing is echoed back, so this was integrity and noise rather than disclosure — but materially wider than the MailboxHash path it supplements. Header matching now additionally requires the sender to be an existing participant: ticket.user.email, or an address parsed out of an existing Message.author on that ticket, or anyone at ticket.account.domain. The Message.author path is load-bearing rather than defensive — email tickets never set userId, so the opening sender is identified only by the "Name <email>" message label. Deriving a domain from user.email or a message author was considered and rejected: a ticket opened from an address at a freemail provider would make every address at that provider a participant, which reopens the hole while looking like a fix. Account.domain is opt-in CRM data and cannot silently widen to a public provider. normalizeParticipantEmail also keeps free-form author values that are not addresses (System, slack:U123, Outpost AI) from counting as identities. An unauthorized candidate is treated as unresolved, so the mail falls through to the orphaned-reply path and is filed as its own ticket with no AI job. A legitimate sender writing from an unrecognised address never loses their message. MailboxHash is deliberately NOT gated — it is a plus-addressed token we generate, and gating it breaks the legitimate reply path. Two supporting changes the fix required: findFirst became findMany so candidates are scanned oldest-first for the first AUTHORIZED match, since otherwise a chain naming somebody else's older ticket would refuse the sender's own legitimate reply; and MAX_REPLY_MESSAGE_IDS caps the sender-supplied IN (...) fan-out that requiring participants necessarily widens. 3. responseError carried lifecycle state in a free-text error column. DELIVERY_CONFIRMED: and ESCALATION_REQUIRED: string prefixes encoded control state in the column an operator reads to find out what went wrong — so any ops surface would render DELIVERY_CONFIRMED as an error. In the same change that introduced MessageResponseState for exactly this purpose. Both moved to dedicated additive columns: deliveryConfirmed Boolean @default(false) and escalationRequiredReason String?. Columns rather than new enum values, deliberately: both markers describe a response that is still PENDING, and MessageResponseState records the OUTCOME, so adding them would have destroyed the outcome they are sub-states of and silently broken every responseState === 'PENDING' comparison in the handler. The escalation signal is one nullable text column rather than a boolean plus a reason, so the fact and its payload cannot drift apart. responseError now holds only real error text. Migration 20260813000000_add_message_response_substates is two ADD COLUMNs, no index, and needs no backfill: DEFAULT false makes existing rows correct by construction, and a row that somehow did hold a marker reads as "not confirmed", which is the safe direction. Verified against the merged CI drift gate by having Prisma 6.19.3 emit the required SQL via migrate diff and comparing it to the hand-written file; they are identical. No Postgres in the sandbox, so CI's own migrate deploy round-trip was not executed locally. One deliberate behaviour change falls out of (3): independent columns can both be set, which a single string prefix made impossible. The re-answer gate therefore checks the owed escalation BEFORE the confirmed-delivery repair — dropping a promised human handoff is the worse failure. Neither branch reposts. Verification: turbo typecheck 10/10, turbo test 10/10 (1949 tests, up from 1924). Every fix carries red-green verification; 16 separate mutations were each observed RED and restored to GREEN, including one confirming that gating the MailboxHash path turns 10 tests red, which pins that boundary. Existing assertions that referenced the old markers were updated and each re-checked for degenerating to always-pass. Confirmed by diff that start.sh, both Dockerfiles, .github/workflows/ci.yml, .env.example and the 20260812 SystemConfig migration remain byte-identical to main.
…onse guard Both blockers were coverage, not logic. Reverting the call site in `generate()` to the pre-PR first-block-only expression left 254/254 passing, because the new test only exercised the exported helper — so a bad merge or a later refactor could restore the exact defect this PR fixes with CI green. #187 touches this file, which is where that would come from. Relaxing `!responseText.trim()` to `!responseText` also left the suite green, and whitespace-only is the mode the API can realistically produce for this call shape, so the untested half was the reachable one. Drives a `[thinking, text]` response through `generate()` via aimock's `reasoning` option, asserting the answer survives and `degraded` is false; and a `' \n '` fixture asserting the fallback. Both new tests now die under their mutation. The existing empty-response test also asserts the reason, not just the fallback text: aimock builds `content: ''` as `[{type:'text', text:''}]`, but a real `content: []` reaches the same fallback via TypeError, so without that assertion the test could pass for the wrong reason. Also from review: - Join text blocks with a blank line instead of concatenating. Two text blocks are only adjacent because something non-text sat between them, so they were separate emissions — gluing them yields `...first step.Next you...`. Filtering explicitly rather than mapping non-text to `''` makes the separator apply where it should and nowhere else. - Type the fixture as `Anthropic.ContentBlock[]` instead of casting through `unknown`. The cast was hiding real drift: `ToolUseBlock` now requires `caller`, and the typecheck said so the moment it was removed. 23 tests in the file, 1015 in the package. Typecheck adds no errors over base (same 10, all unbuilt-`shared` module resolution).
|
Status check on this PR, since four successors have now been split out of it and This should be closed, not merged
#224 renames the claim-token migration. This branch still carries the old folder. If #224 lands first and this is then merged, the repo has two migrations that each run What has landed, and what has notLanded via the splits: the one-answer-per-ticket state machine and its migrations (#191, merged), the generator text-block fix (#223), and the claim fencing plus shutdown drain (#224). #222 addresses the groundedness gate from a separate lineage. Genuinely still only here:
Worth being accurate about the scope of that: Suggested pathOpen a fifth split off current One sequencing note before that split goes up: Not closing it myself since it is your PR — flagging so the migration collision doesn't get discovered at deploy time. |
|
Closing this unmerged, per @jerelvelarde's status check above. Agreed on the reasoning, and the migration collision is the deciding factor rather than just the conflicts. Why closed rather than merged: this branch still carries Successors:
Still only here, and needs a fifth split off current To be accurate about that scope: Sequencing note for whoever picks up the split, and this one matters: Nothing here is lost — the branch stays for reference. |
CPK-7926 Split /health out of PR #187, then close #187 unmerged (do NOT merge it)
PR #187 is the pre-split combined branch. It should not be merged — once #190, #191, and the third PR land, all 25 of its files will have shipped through them. Close it with a comment naming the three successors so the history stays legible. One correction to carry forwardOn 2026-08-17, PR #188 was closed in favour of #187 on the grounds that #187 was "the real change." That was right about #188 — #188 targeted Net: neither #187 nor #188 merges. The work lands as #190 + #191 + the third PR. Do not close #187 before the third PR is opened — it is currently the only place that scope exists as reviewable code. |
2 participants
The second half of #170, rebuilt on current
main. #172 took the customer-facing fix and shipped; this is the infrastructure work that came along with it and never had a review pass. #170 is closed in favour of this — its diff still showed the pre-hotfix state and its title described work that has already merged.Rebuilt rather than rebased, and that mattered. The old branch predates the #180 hotfix, so replaying it would have deleted the
SystemConfigrepair migration (−93), the schema-drift guard instart.sh(−210), and its CI gate (−257). Verified by diff that all six hotfix files are byte-identical tomain.Why this exists
#172 shipped two layers of the one-answer-per-ticket 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:
runWithTimeoutinworker.tsis aPromise.racethat does not cancel the handler.AI_RESPONSEhas a 120s timeout over a deliberately slow pipeline, so on timeout attempt 1 keeps running while attempt 2 starts a second later, reads messages before attempt 1'smessage.create, and both post.AI_RESPONSEruns at concurrency 4.What it does
The database arbitrates.
Message.responseKeywith a unique index 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.
responseStateisPENDING → DELIVERED | ESCALATED, every transition a compare-and-set viaupdateManyguarded onresponseKeyand current state — never a blindupdateby id. A response startsPENDINGbefore any external post. Successful delivery marksDELIVERED. One needing a human staysPENDINGuntil that escalation is durable, then becomesESCALATED. If delivery itself endsPENDING, a retry schedules a delayed check that pulls in a human only if the response is still pending, so the one-post rule holds without racing the original handler.Job ownership is fenced.
Job.claimTokenstops a timed-out attempt writing back over the attempt that replaced it. StalePROCESSINGjobs are reclaimed after a grace period. Graceful shutdown shares one drain promise so signal handlers and the app can't each half-drain.Also here:
generator.tsread onlycontent[0], silently discarding text from multi-block model responses — now concatenates all text blocks in order (it also throws on empty text, unrequested scope, filed as #178 for an explicit decision). Postmark ticket creation is idempotent behind a partial unique index onTicket(sourceId) WHERE source = 'EMAIL', and reply detection falls back toIn-Reply-To/Referencesso a reply without a plus-address stops becoming a second ticket with its own answer. The Teams acknowledgement card no longer claims an AI is reviewing when no AI job was enqueued.Restores the
/healthrework that #180 deferred (d0c6f99). #180'sstart.shguard catches theSystemConfigcause; it does not catch the class. Any other boot failure — unreachable database, bad credentials, a throw insidebuildSyncEngineunrelated to schema — still exits before the port binds, leaving Railway a five-minute timeout indistinguishable from a broken image. The port now binds first, boot state is tracked, and/healthanswers 503 with the phase and reason while boot is unfinished or failed. Fail-fast is retained: an unbooted worker is never reported healthy, because its sync mappings come from the database and one running against a mismatched schema would write wrong statuses to Linear.railway.tomlcutshealthcheckTimeoutfrom Railway's 300s default to 180s, aboveBOOT_FAILURE_LINGER_MS, so the probe reads the reason before the process exits to restart.This is the reviewed version of that work recovered from
d0c6f99~1, not a rewrite — it keeps the rounds that made the boot-failure path reachable and shutdown survive a signal during boot. Worker tests go 2 → 24.The three blockers from the split brief are fixed
escalationEnqueuedwas assigned and never read. Afalsereturn means the compare-and-set found the row in a state other thanPENDING, so no escalation was queued — silently discarded, withescalatedcomputed from local conditions. A response that reached nobody and summoned nobody reported itself handled. Now a no-op CAS reads the row's actual state and splits the two previously-identical cases:DELIVEREDsucceeds honestly withescalated: false,ESCALATEDreportstrue, andPENDING/missing/unreadable fails loudly.Header-based email reply resolution trusted attacker-controlled input.
In-Reply-To/Referencesare sender-supplied, and a Message-ID is known to every thread participant rather than needing to be guessed — so anyone ever CC'd could append to that ticket and reopen it. Integrity and noise rather than disclosure (no AI response is spent, nothing echoes back), but wider than theMailboxHashpath it supplements.Header matching now requires the sender to be an existing participant:
ticket.user.email, an address parsed from an existingMessage.authoron that ticket, or anyone atticket.account.domain. TheMessage.authorpath is load-bearing — email tickets never setuserId, so the opening sender exists only as a"Name <email>"label. Deriving a domain fromuser.emailwas considered and rejected: a ticket opened from a freemail address would make every address at that provider a participant, reopening the hole while looking like a fix. An unauthorized candidate falls through to the orphaned-reply path, so a legitimate sender on an unrecognised address never loses their message.MailboxHashis deliberately not gated — gating it turns 10 tests red, which pins the boundary.responseErrorcarried lifecycle state in a free-text error column.DELIVERY_CONFIRMED:andESCALATION_REQUIRED:prefixes lived in the column an operator reads to find out what went wrong, so any ops surface would renderDELIVERY_CONFIRMEDas an error — in the same change that introducedMessageResponseStatefor exactly this. Both moved to dedicated additive columns. Columns rather than enum values deliberately: both markers describe a response that is stillPENDING, andMessageResponseStaterecords the outcome, so adding them would have destroyed the outcome they are sub-states of and broken everyresponseState === 'PENDING'comparison.Tests
1,949 pass, typecheck 10/10. Every fix carries red-green verification — 16 separate mutations each observed RED and restored to GREEN.
Reviewer notes
This branch has never had a full review round. That was the entire reason for the split. The three blockers are fixed and individually verified, but the protocol that found nine defects in the original work has not run against these 4,073 lines, and this lands in shared job-queue infrastructure where failures are silent.
Where I'd look hardest:
20260813000000migration was not round-tripped against a real database — no Postgres in the sandbox. It was verified by having Prisma 6.19.3 emit the expected SQL viamigrate diffand comparing byte-for-byte, which is strong but not the same thing. The merged CI drift gate runsmigrate deployagainst real Postgres and is the actual proof.runWithTimeoutstill does not cancel. The uniqueness claim means a timed-out attempt can no longer double-post, but it keeps burning a pipeline slot and model tokens to produce a response that will be discarded. Tracked in AI_RESPONSE can double-post under timeout-retry #176./healthsemantics — 503-with-a-reason preserves fail-fast while making it diagnosable. If you'd rather the worker come up degraded and keep processing job types that don't need sync mappings, that's a different tradeoff and worth saying now.Related: #169 (remaining inbound idempotency) · #171 (whether email auto-answers) · #173–#179 (follow-ups filed from the split)