Motir

Type: implementation defect on the CI-feedback path (a concurrency/latency defect in shipped code, not a planning mistake — no lesson). Sibling in mechanism, not in cause, of MOTIR-3396 (approve blew the same 5 s budget) and MOTIR-1753 (SSR blew it) — but where those transactions were long because the WORK was long, this one is long because it waits: it holds a row lock across writes that run on other connections.

Parent: none (root sibling, bare id 19). Reported out of band from production Sentry, not from a card — there is no finding story and no discovery epic, and no blocked_by / blocks edge into a not-done card, so the EDGE test in log-bug.md sends it to rung 2 → parentless. relates_to MOTIR-4199, the open bug on the same function.

Observed: production Sentry, auto.function.nextjs.on_request_error, unhandled, POST /api/github/webhook → applyCiStatusFeedback → ciState.

PrismaClientKnownRequestError
Invalid `prisma.githubCheckRun.upsert()` invocation:
Transaction API error: A query cannot be executed on an expired transaction.
The timeout for this transaction was 5000 ms, however 5379 ms passed since the
start of the transaction.
  ../../../lib/services/changeRequestCiFeedback.ts:353  in ciState
  ../../../lib/services/changeRequestCiFeedback.ts:317  in applyCiStatusFeedback
  ../../../app/api/github/webhook/route.ts:43           in POST

The mechanism, read on origin/main (847d44ec9)

lib/services/changeRequestCiFeedback.ts:317 opens the terminal-verdict transaction through withSystemContext, and that wrapper is the ONE context in lib/workspaces/context.ts that takes no TransactionBudget (:164–171) — withWorkspaceContext (:103) and withWorkspaceServiceContext (:338) both accept one, the latter since MOTIR-1972 and the former since MOTIR-3396. So this path runs on Prisma's default timeout: 5000 / maxWait: 2000 and has no way to say otherwise.

Inside that one transaction, in order:

Two facts compose into the failure:

1. The lock is held across foreign work. commentsService.addComment / editComment each open their own withWorkspaceContext transaction on a different pooled connection (lib/services/commentsService.ts:254, :357), each preceded by pre-transaction reads (resolveComment, resolveGatedWorkItem, mention resolution) and followed by their own writes. The header at :305–316 says the lock is deliberate — the comment body is read-derived, and MOTIR-2946's duplicate-comment race is what it prevents — and it explicitly reasons about deadlock ("touches neither github_pull_request nor github_check_run, so no cycle can form"). It never reasons about hold time. The holder's critical section is O(delivered cards) × (comment round trips) wide.
2. Every check at a head commit contends for that one row. A motir-core pull request carries ~34 check names — MOTIR-2946's title is literally "a motir-core PR buries its work item under ~34 comments", one per check — and CI finishes them in bursts. Each terminal delivery is its own webhook request opening its own transaction and queuing on the same FOR UPDATE.

Where the 5379 ms went, and why the failing line is the tell. The statement that expired is the FIRST write after the lock and two indexed reads; those three are sub-millisecond on this data. Prisma's budget is measured from the start of the transaction BODY (connection acquisition is the separate maxWait, which fails differently), so essentially the whole 5.4 s was spent blocked in lockById at :322 — waiting behind one or more holders that were mid-comment-write. That is contention, not a slow query: nothing here scans, and the queue is k − 1 holds deep for the k-th delivery.

Why it is worse than a 500

app/api/github/webhook/route.ts:43 calls githubWebhookService.handleEvent with no try/catch, and CI events are handled inline (githubWebhookService.ts:171–172, 423 — check_suite / check_run go straight to applyCiStatusFeedback; unlike push, nothing is enqueued onto the job substrate). So P2028 rolls the whole transaction back and the route 500s — and GitHub does not automatically retry a failed webhook delivery. Consequences, in severity order:

1. A failure conclusion can be lost permanently. The row is never written, so a later green delivery at the same sha folds over recorded rows that contain no red vote, writes "✅ CI passing — all N checks succeeded … This work is verified", and promoteDeliveredCardsOnGreen moves the card implemented → in_review. That is a false green on a red build — the exact harm MOTIR-3823 and MOTIR-4199 exist to prevent, arriving through a third door: not null read as green, not a prefix read as the whole set, but a dropped row.
2. If the lost delivery was the LAST check, no further delivery arrives to re-derive: the comment stays interim and the card stays at implemented forever, needing a manual redelivery.
3. The 500s are load-shaped — they fire hardest on the busiest pull requests, i.e. exactly when the verdict matters.
4. Secondary hazard: the outer transaction holds a pool connection while the inner comment transaction demands a second one. Under pool pressure the inner call waits on maxWait (2 s) inside the outer's 5 s budget — self-amplifying, and a plausible source of the holder's own slowness.

Interaction with the open MOTIR-4199 / PR #2552

Not a duplicate — different defect, same function, and they touch. #2552 is careful to put its host round trip OUTSIDE the lock (its own comment: "that question costs a network round trip, which must not be made while holding a row lock"), which is the right instinct. But it also adds reconcileRecordedCheckSet inside the locked transaction, creating up to (expected − recorded) rows per delivery, plus one more listByPrAndSha in phase 1. That widens the critical section this bug is about. Whichever lands second should be measured against the other; the fix here should be written so #2552's reconcile keeps working.

Fix direction (measure, do not pre-commit)

The rule from MOTIR-3396's fix applies verbatim: make the work inside the transaction smaller FIRST, and raise a budget second, with an argument.

1. Get commentsService out of the critical section. The lock exists only to make "does this (pr, sha, card) already have a comment?" a read-derived decision that cannot race. Cheaper shapes: a UNIQUE constraint on github_ci_feedback_comment(pull_request_id, commit_sha, work_item_id) so the create races safely and the loser edits (or discards) instead of holding a lock; or a short claim transaction that decides the writer, releases, then writes comments unlocked. Either keeps MOTIR-2946's one-comment-per-(change request, head sha) and MOTIR-3770's one-comment-per-delivered-card guarantees, which are the constraints the fix must not break.
2. Make the delivery durable. Record the check row in its OWN short transaction before any comment work, or move the whole terminal path onto the Postgres job queue (MOTIR-3413) so a failure retries instead of vanishing, and the webhook acks fast. This half is what stops a dropped failure; it is independent of (1) and arguably the more important one.
3. Only then: give withSystemContext a TransactionBudget parameter (it is the only wrapper in that file without one, and its absence was not a decision — the same sentence MOTIR-3396 wrote about withWorkspaceContext) and pass an argued budget here. A raised budget alone does not fix this: hold time scales with concurrent deliveries, so any fixed number is beaten by a pull request with more checks.

Acceptance criteria

1. A test drives N concurrent terminal deliveries at one head sha for a pull request whose comment writes are slow, and every one of them records its check row — no P2028, no 500 from the webhook route. Today's code fails this; assert against the real fixed counterfactual (a delivery whose row is missing), not a ratio.
2. No foreign-connection work inside the change-request lock — asserted structurally, not by timing: with the lock held, the transaction issues no commentsService call. (A timing assertion here is a flake generator.)
3. A delivery whose conclusion is failure is never lost: after an induced failure of the comment write / an expired budget, a subsequent green delivery at the same sha still sees the red vote, writes the failing comment, and does not promote.
4. MOTIR-2946 stays green (one comment per (change request, head sha), no duplicate on a concurrent double delivery) and MOTIR-3770 stays green (one comment per delivered card, N cards on a session pull request).
5. If a TransactionBudget is added to withSystemContext, the call site carries the argument for the number, in the shape lib/workspaces/context.ts's TransactionBudget doc demands, and the argument records what the work inside was reduced to first.
6. #2552's reconcileRecordedCheckSet still runs correctly under the new locking shape (or is explicitly re-sited by this card, saying so).

Context refs

• motir-core lib/services/changeRequestCiFeedback.ts — the transaction (:317), lockById (:322), the failing upsert (:353), the comment loop (:384–396); the pending arm's own withSystemContext (:245) and the phase-1 resolve (:143).
• motir-core lib/workspaces/context.ts — withSystemContext with no budget (:164), withWorkspaceContext with one (:103), the TransactionBudget contract (:375–405).
• motir-core lib/repositories/githubPullRequestRepository.ts:132 — lockById.
• motir-core lib/services/commentsService.ts:254 / :357 — the inner transactions.
• motir-core app/api/github/webhook/route.ts:43; lib/services/githubWebhookService.ts:171–172, 423 — inline handling, no retry.
• MOTIR-2946 (why the lock is there) · MOTIR-3770 (the per-card comment loop) · MOTIR-4199 / PR #2552 (same function, in flight) · MOTIR-3396 (the budget precedent and the order of its fix).

Resolution

(open — filled by the closing subtask)