perf(run): reads proportionate to what they show, and the case pass batched - #544
Merged
Merged
Conversation
added 7 commits
September 9, 2026 10:34
Opening a case measured 43s on a cold read and ~4s warm against the pilot's six-measure nightly, with no run in flight — so this was never load. `routes/cases.ts` fetched every outcome row of the run, `evidence_json` blobs included, and then `.find()`d the single row the page renders. The cost grew with the roster rather than with anything the page shows: 120,000 rows over the wire into a single-replica worker to use one of them. `listOutcomes` gains an optional `subjectId` alongside the `measureId` added in #543, filtered in SQL in both stores, and the route asks for `{ subjectId, measureId, limit: 1 }`. `limit: 1` is exactly equivalent to the old `.find()`: the store orders by `evaluated_at ASC, id ASC`, so the first row under the same filter is the row `.find()` would have returned. Both columns are index-friendly — `outcomes` carries `(subject_id, measure_id, evaluation_period)` and `(run_id)`. The store-contract test pins the composition of the two filters and asserts the result is the same row the unfiltered `.find()` picked, so an implementation that narrowed differently fails rather than quietly returning a neighbour.
… definition The whole-run read fixed in the previous commit was written out eight times. A read-path audit found it at case-actions (the detail returned by EVERY case mutation — assign, escalate, resolve, priority), case-outreach (twice per send: renderContext then buildDetail, and again on every "Preview message"), appointment-service (so the case detail page paid it TWICE per view, once for the case and once for its appointments), routes/ai (before the explanation cache is consulted, so even a cache hit paid it), case-rerun, both MCP case tools, and the auditor case packet. All now call `outcomeForCase(outcomes, lastRunId, subjectId, measureId)` — one row, chosen in SQL. A shared helper rather than eight edits, so the ninth copy has somewhere to go instead. `audit-packet.ts` was invisible to the grep that found the others: it contains two literal NUL bytes as a composite-key joiner, so grep classifies it as binary and skips it. Intentional and committed, but worth knowing it hides from search.
…he whole tenant
The patient profile — the page a quality lead opens to see one person's quality — read
`listCases({ limit: 100000 })`, every case in the tenant, and kept the ones whose employeeId matched.
It scaled with the practice's case count rather than with the patient being looked at.
`CaseQuery` gains `employeeId`, filtered in SQL on both stores. The contract test pins that it
composes with `measureId` rather than replacing it, and that omitting it does not filter — a filter
that silently won the other would go unnoticed at demo scale and only bite the pilot.
The nightly evaluates 120,000 (subject, measure) pairs and the case pass awaited two round trips per pair — a SELECT then an INSERT or UPDATE — plus a third for each audit event. Measured on the pilot at 9.5 pairs a second, about three and a half hours; the run of 2026-09-08 was killed by a deploy at 87,000 and then showed RUNNING for sixteen hours. Outcomes were already batched; the case pass was not. `CaseStore.upsertFromOutcomes` takes a chunk and returns one result per input, in input order, null exactly where the single-row call returns null. Postgres reads every existing row for the chunk's keys in one `unnest` join, plans in memory with the same pure `planCaseUpsert`/`planNextAction`, then writes one multi-row INSERT and one set-based UPDATE. `CaseEventStore.appendAudits` does the same for the ledger. Measured against a real postgres:16, 3,000 pairs, results asserted identical to the sequential path: 5,250 round trips to 12, and 4,151ms to 553ms locally — where a local socket pays none of the ~40ms Neon costs per trip. At that RTT the case pass alone was ~140 minutes of pure latency per nightly. ADR-076 d2 survives as a compare-and-set in the WHERE of the set-based UPDATE, comparing the `next_action` we READ. A row an operator moved in between matches nothing and falls back to `upsertFromOutcome` for that row alone — the proven path, with its re-read, its three attempts and its action-preserving fallback. `planNextAction` stays the single definition of the rule; expressing it as a SQL CASE would make the pure function dead where it matters. Same for a key another writer inserted first. A duplicate key inside one batch THROWS rather than resolving arbitrarily: a set-based UPDATE would apply one of the two silently where the sequential path applied both in order. The SQLite floor is a loop, and says so — the batching buys round trips, and a local file has none. That means every batch-shaped contract test passes on the floor without exercising set-based SQL, so the Postgres ceiling is where this is really tested. Both failures found while writing it were Postgres-only: an INSERT placeholder computed from a moving `binds.length` inside the row loop, and an ambiguous `RETURNING` once the UPDATE joined a VALUES alias carrying the same column names. The audit batch is awaited through `Promise.resolve().then(...)`, not a bare `.catch()` on the call: a `.catch()` handles a rejection, but a synchronous throw escapes it and would take the cycle rollover and the terminal event down with it. The old per-row call had the same latent hole.
… fails the run, and tests that can fail Three reviewers, two of them independent, converged on the same list. CRITICAL, and a real data defect: the set-based UPDATE hoisted `toUpdate[0]`'s `runId` and stamped it on every row in the chunk, while the INSERT path used the row's own. Reproduced against a live postgres:16 — a two-row batch with different run ids wrote the first id to both. `last_run_id` is the evidence pin §6.5 relies on to survive outcome compaction, and it is what `countByLastRun` counts, so a case would have been pinned to a run that did not produce it. The pipeline passes one run id per chunk today, so it was latent — and invisible to the SQLite floor, which loops and is correct by construction. `last_run_id` is now a per-row column in the VALUES list, and the probe passes. The duplicate-key refusal was correct in the store and wrong at the boundary: it threw inside the chunk loop, so a repeated key would fail a three-hour run outright where the per-row loop had absorbed it last-wins. A repeat is reachable — the live WebChart path builds items per fetched bundle, so two bundles for one patient produce two items for one key, in the same chunk. The pipeline now collapses duplicates last-wins, matching what it replaced, and logs a WARN so the upstream duplication is still visible. The store keeps the throw as the backstop. `appendAudits` was all-or-nothing per 500-row statement, so one malformed payload cost up to 500 ledger entries against the rule that every state change is audited. A failed sub-chunk now falls back to a row at a time — the same "losers take the proven path" shape the case store uses — and still propagates so the caller hears that the ledger is incomplete. Tests that could not fail, which is the defect class this repo names and I have now hit three times: - the "operator mid-batch" contract test patched BEFORE the batch, so the pre-read saw OPERATOR and the row WON the compare-and-set. It never reached the fallback. Renamed to what it actually proves, and a real race added in the Postgres file — it interposes on the `unnest` pre-read, which is the window the batch cannot see. It belongs there because the floor has no set-based UPDATE to lose. - the equivalence test ran against a fresh store, so every input took the INSERT path and the set-based UPDATE never executed. A new test seeds first and mixes inserts, updates and no-ops in one batch, and pins the two §4 guarantees worth pinning for a batch: IN_PROGRESS survives, a human closure is not reopened. - nothing crossed the 500-row sub-chunk boundary. A 1,200-row batch with every third input a no-op now does, so a shifted index shows up as a null in the wrong slot. Two comments asserted invariants that had stopped being true: the progress counters advance before any case is written now (they still describe persisted OUTCOMES, which is what they always counted — the comment claimed cases), and a mid-run failure is coarser than per-row, in the recoverable direction. Also: raw NUL bytes had crept into five source files as key separators. A NUL makes git and grep treat a `.ts` file as binary, so the file drops out of diffs and out of every search — `audit-packet.ts` had been hiding from code search this way for months, which is why a whole-run read in it survived. Same runtime value, written as `\u0000`. Both stores now join duplicate keys identically; the floor had a space, so a subject id containing a space would have been refused by one store and accepted by the other.
Owner
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad101a107a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+399
to
+401
| WHERE c.employee_id = v.employee_id AND c.measure_id = v.measure_id AND c.evaluation_period = v.evaluation_period | ||
| AND c.next_action IS NOT DISTINCT FROM v.expected_next_action | ||
| AND c.next_action_source IS NOT DISTINCT FROM v.expected_next_action_source |
There was a problem hiding this comment.
When this batch has pre-read an OPEN case and scheduleAppointment concurrently changes only its status to IN_PROGRESS, these predicates still match because that action does not modify either next_action field. The update then writes the stale planned status (OPEN) and silently undoes the scheduling state, violating the state-aware upsert contract that preserves IN_PROGRESS. Include the status-related planning fields in the compare-and-set or re-read/lock the row before applying the update.
Useful? React with 👍 / 👎.
…as made from Codex P2 on #544, and it is right. The CAS compared `next_action` and `next_action_source` only, but `planCaseUpsert` reads `status`, `current_outcome_status` and `closed_by`, and `planNextAction` reads the two action columns plus `current_outcome_status` — five inputs, two guarded. So a concurrent write touching neither action column slipped through. `scheduleAppointment` is exactly that: `patchCase(caseId, { status: "IN_PROGRESS" })` and nothing else. The batch's planned `status: "OPEN"` then landed on top of it and silently undid the scheduling, against §4's most-cited guarantee. The single-row path guards the same two columns and has the same hole, but its read-to-write window is microseconds; a batch plans a whole chunk and issues an INSERT before its UPDATE, so the window is orders of magnitude wider. The batch is now strictly stricter than the per-row path, which is the right asymmetry: a row that fails the wider guard falls back to `upsertFromOutcome`, which re-reads, re-plans against the row as it now is, and gets the correct answer. The new Postgres test interposes on the `unnest` pre-read and patches only the status, reproducing the exact scenario. Verified non-vacuous: with the three added predicates removed it fails on "the operator's IN_PROGRESS survived the batch's stale plan", and passes with them. Fixing it also caught a cast/column misalignment of my own — the three new expected_* values are pushed before `runId`, so `::uuid` had to move from position 14 to 17. The VALUES cast list now carries a numbered comment mapping every position, because this is the second time an off-by-one there has cost a debugging round.
Owner
Author
|
Fixed in db8489d — you were right, and the mechanism is exactly as described.
The compare-and-set now covers all five — "only write if the row is still the row I planned from". A row that fails it falls back to the per-row Worth noting the single-row path has the same two-column guard and the same hole; what made it worth closing here is that a batch plans a whole chunk and issues an INSERT before its UPDATE, so the read-to-write window goes from microseconds to hundreds of milliseconds. The batch is now strictly stricter than the per-row path. Added a Postgres test that interposes on the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two measurements on the live pilot stack started this, both with no run in flight.
Opening a case took 43 seconds cold and about 4 seconds warm. The route fetched every outcome row of the whole run —
evidence_jsonblobs included — and then.find()d the one row the page renders. Roughly 87,000 rows across the wire to use one of them, growing with the roster rather than with anything the page shows.The nightly ran at 9.5 (subject, measure) pairs a second — about three and a half hours. Outcomes were already batched; the case pass was not. Per pair it awaited a SELECT, then an INSERT or UPDATE, then an audit insert.
Reads
A read-path audit found the case-detail query copied in eight places, several worse than the original:
case-actionsis the detail returned by every case mutation, so assigning a case paid it too;case-outreachpaid it twice per send and again on every "Preview message";appointment-servicemeant the case page paid it twice per view; androutes/aipaid it before consulting its own explanation cache, so even a cache hit cost four seconds.All eight now call one
outcomeForCasehelper — one row, chosen in SQL.listOutcomesgained an optionalsubjectIdbeside themeasureIdadded in #543.limit: 1is exactly equivalent to the.find()it replaces: both adapters orderevaluated_at ASC, id ASC, so the first row under the same filter is the row.find()returned. Verified equivalent at 20k and 120k rows — same row id.The patient profile had the same shape one level up:
listCases({ limit: 100000 }), every case in the tenant, filtered in JavaScript.CaseQuerygainedemployeeId.The batched case pass
CaseStore.upsertFromOutcomestakes an evaluation chunk and returns one result per input, in input order,nullexactly where the single-row call returns null. Postgres reads the chunk's existing rows in oneunnestjoin, plans in memory with the same pureplanCaseUpsert/planNextAction, then writes one multi-row INSERT and one set-based UPDATE.CaseEventStore.appendAuditsdoes the same for the ledger.Measured against a real postgres:16, 3,000 pairs, results asserted identical to the sequential path:
Local has no network. At Neon's ~40 ms per round trip the case pass alone was about 140 minutes of the nightly, which is most of it.
ADR-076 d2 survives as a compare-and-set in the WHERE of the set-based UPDATE, comparing the
next_actionthe batch read. A row an operator moved in between matches nothing and falls back toupsertFromOutcomefor that row alone — the proven path, with its re-read, three attempts and action-preserving fallback.planNextActionstays the single definition of the rule; expressing it as a SQLCASEwould have made the pure function dead exactly where it matters.A duplicate key inside one batch throws in the store rather than resolving arbitrarily — a set-based UPDATE would apply one of two silently where the sequential path applied both in order. The pipeline collapses duplicates last-wins before calling, matching what it replaced, and logs a WARN so upstream duplication stays visible.
A local Postgres, which is the part that made this safe
Every store change in this repo has carried a "verified only by CI" caveat.
docker compose -f infra/docker-compose.yml up -d postgresretires it: the ceiling runs 98/98 with nothing skipped.It paid for itself immediately. Both bugs in the new SQL were Postgres-only and passed on the SQLite floor — an INSERT placeholder computed from a moving
binds.lengthinside the row loop, and an ambiguousRETURNINGonce the UPDATE joined aVALUESalias carrying the same column names. The floor is a loop by design and can catch neither.What review caught
Three reviewers; the two that completed converged independently on the same list.
CRITICAL — a real data defect. The set-based UPDATE hoisted
toUpdate[0]'srunIdand stamped it on every row in the chunk, while the INSERT used each row's own. Reproduced against the live database: a two-row batch with different run ids wrote the first id to both.last_run_idis the evidence pin §6.5 relies on to survive outcome compaction, and it is whatcountByLastRuncounts — a case would have been pinned to a run that did not produce it. Latent (the pipeline passes one run id per chunk) and structurally invisible to the floor. Now a per-row column; probe passes.The duplicate-key refusal was right in the store and wrong at the boundary. It threw inside the chunk loop, so a repeated key would fail a three-hour run outright where the per-row loop absorbed it. Reachable: the live WebChart path builds items per fetched bundle, so two bundles for one patient produce two items for one key, in the same chunk.
appendAuditswas all-or-nothing per 500-row statement, so one malformed payload cost up to 500 ledger entries against the rule that every state change is audited. A failed sub-chunk now falls back to a row at a time and still propagates.Three tests could not fail — the defect class this repo names, and my third time hitting it:
unnestpre-read — the window the batch cannot see. It belongs there because the floor has no set-based UPDATE to lose.Two comments asserted invariants that had stopped being true, and were corrected rather than left: the progress counters advance before any case is written (they still describe persisted outcomes, which is what they always counted — the comment claimed cases), and a mid-run failure is coarser than per-row, in the recoverable direction.
A reviewability fix worth its own line
Raw NUL bytes had crept into five source files as key separators. A NUL makes git and grep classify a
.tsfile as binary, so it drops out of diffs and out of every code search —audit-packet.tshad been hiding that way, which is why the whole-run read inside it survived the audit that caught the other seven. Same runtime value, written\u0000. The two stores also disagreed on what a duplicate is: the floor joined keys with a space, the ceiling with NUL, so a subject id containing a space would have been refused by one and accepted by the other.Verification
pnpm typecheckclean.Deliberately not in this PR
GET /api/runs/:id/outcomesis unbounded on the pilot profile. Bounding it means changing whatX-Total-Countpromises (it is the visible count, and visibility is an app-side predicate over a directory built from the rows), which is a contract change on an admin surface and belongs in its own decision.WORKWELL_INCREMENTAL_EVALstays off. A cache hit still persists the copied-forward outcome and still runs the case upsert, so it would have bought little while the writes dominated. It is the next lever now that they do not, and it should be turned on as a measured step rather than folded in here.Separately, and not fixed here: the 2026-09-08 six-measure run failed because merging #543 redeployed and restarted the worker mid-run, and the orphaned row then showed RUNNING for about sixteen hours before a sweep marked it FAILED. A long run cannot survive a deploy and has no resume. This shrinks the window considerably; it does not close it.