fix(program): a measure's rate is its own rows, and the Maui e2e suite runs the configuration the pilot deploys - #543
Merged
Conversation
added 4 commits
September 8, 2026 17:23
…e runs the configuration the pilot deploys The programs overview served the whole run's official evidence under each measure's name. `aggregateOfficialRun` paged `listOutcomes(runId)` unfiltered, under a comment claiming a run evaluates one measure with one engine — true of a MEASURE-scoped run, false of the ALL_PROGRAMS run the pilot actually runs, which holds a row per (subject, measure) pair. On the sandbox CMS122's card carried CMS125's initial population, CMS125's 59.2% and `ecqmId: 125FHIR`. The MeasureReport and reconciliation routes were unaffected (both refuse a multi-measure run with 422); the overview was the one unguarded caller. `listOutcomes` now takes an optional `measureId` and narrows in SQL, before the page window, in both stores — the app-side alternative would page 120,000 rows once per measure on a six-measure nightly. `runProducedOfficialEvidence` is scoped the same way, so a run that mixes an official and an authored measure (ADR-072 admits one) no longer answers for both from whichever row sorted first. The Maui Playwright job ran with no VSAC credential, so cms2/cms130/cms165/cms137 booted `official-pending` and the suite's green was about a stack nobody deploys. It now vendors the six terminology sidecars with the credential and sets WORKWELL_OFFICIAL_MEASURES to the deployed value. Because a missing sidecar degrades rather than crashes, the backend's own `runnable=` boot line is asserted — without it the job runs two measures and reports green, which is what it had been doing since 2026-09-06. The job is dispatch-only and this is a public repo, so the key is scoped to a single step that runs before the frontend and e2e dependency trees exist, and the completed expansions are not cached: an Actions cache is restorable by a fork pull request. Eleven failing specs, none a backend regression: three needed the routed stack, two asserted the case fan-out ADR-078 removed (now bounds, with the out-of-population gap named), three signed in as the quality lead for pages that are engineering-gated to ADMIN — two of those had been asserting against an AccessDenied panel — one contradicted the §6.3 patient-column contract, and one matched a PCP option inside a closed select. 43 tests to 26 with six measures covered instead of two; four workers and one shared sign-in per role replace ~40 logins. The roster's Status/PCP/Age/Sex/Page-size selects had no accessible name (Chrome computes none from the wrapping label) and now carry aria-labels, matching Panel/System/Segment.
…torage state cannot break the twh project
Review found the boot-line guard close to vacuous. `classifyRunnable` returns `official-pending` for
one reason — the id is absent from WORKWELL_OFFICIAL_MEASURES — and never returns it for
cms122/cms125, which reach the `authored` branch first. So a routing list that lost those two would
print `cms122:authored`, pass a negative grep, and run the pilot's flagship measures on the authored
engine while `runs.spec`'s 48 x 6 held either way. The check is now a positive exact match on
`<id>:official`, verified against the four boot-line shapes including the one the old job produced.
Terminology is not part of the classification at all, so the sidecar control is and always was the
`truncated` refusal in the vendor step; the comment and the journal said otherwise and now do not.
The maui specs declared `test.use({ storageState })` at describe scope. The twh job runs
`npx playwright test` with no `--project`, which loads BOTH projects, and Playwright resolves the
`page` fixture before the `beforeEach` that calls `test.skip` — so those specs would have thrown
ENOENT on the twh run instead of skipping. The session helpers now return `{}` off the maui profile.
Verified by listing both projects with no `.auth/` present.
Storage state is per (role, spec file) rather than per role. Contexts built from one state share a
refresh-token family, and presenting an already-rotated token revokes the family, so two parallel
workers refreshing in the same window would sign each other out with failures pointing nowhere near
the cause. Nine sign-ins instead of two, still far short of the ~40 the per-test login cost.
Three assertion floors restored, all the vacuous-guard shape: the roster search asserted only
`rows <= 2`, satisfied by a search that matched nothing; the chip/worklist bound was satisfied by any
chip sum when the worklist rendered 0; and the drill-down's `rowCount <= chip.count` passed at zero,
which is what a chip href that stopped filtering would produce — the settle-wait accepts the empty
state, so nothing else would have caught it. The out-of-population relaxation only ever justified
MISSING_DATA, so OVERDUE and DUE_SOON now require a non-empty chip to list cases.
…beside them
The first four-worker CI run came back "4 flaky, 22 passed" — green only because retries hid it. One
flake was the roster reading "0 patients" for a full 20 seconds while `runs.spec`'s manual
ALL_PROGRAMS run was in flight; the others were a roster filter, a panel filter and a case-detail
navigation, all under the same window.
Green-by-retry is how a suite starts rotting, so the ordering is now a constraint rather than a hope:
the Maui project splits into `maui` (every read-only spec, still four workers) and `maui-writes`
(`runs.spec` alone) with `dependencies: ["maui"]`, which makes Playwright finish the first before
starting the second. `case-workflow` stays in the parallel set: its mutations are case-level, and
IN_PROGRESS is inside ACTIVE_CASE_STATUSES so it does not move an Open Worklist count.
Not asserted as a product bug: the roster read model already excludes in-flight runs in SQL
(`UPPER(r2.status) IN ('COMPLETED','PARTIAL_FAILURE')`) and again in app code, so the mechanism behind
the empty read is not yet identified. Removing the overlap makes the suite honest either way; if the
empty roster shows up somewhere a run is not in flight, that is the thread to pull.
The remaining flake was mine, not contention. Splitting the jelly-bean tests left the second one calling readChips straight out of a beforeEach that waits only for a heading — which renders before the measure cards have data. locator.count() is a snapshot, so a slow load read zero chips and the caller failed on 'chips.length > 0' while the page was simply not ready. The wait now lives inside readChips, where every caller gets it, and it doubles as the assertion that a routed measure renders a card at all.
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Taleef7
added a commit
that referenced
this pull request
Sep 9, 2026
…atched (#544) * perf(case): a case detail reads its one outcome row, not the whole run 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. * perf(case): every case surface reads its one outcome row, through one 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. * perf(profile): a patient's cases are filtered in SQL, not read from the 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. * perf(run): the case pass writes a chunk at a time, not a row at a time 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. * fix(run): review fixes — a per-row run id, a duplicate that no longer 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. * docs(journal): the 2026-09-09 perf entry * style(run): indent the audit-batch catch body * fix(store): the batched compare-and-set guards every field the plan was 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. --------- Co-authored-by: Taleef <taleef@gmail.com>
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 things the six-measure flip exposed once it was live on the sandbox: a dashboard rate that was the wrong measure's, and an e2e suite that had stopped testing the pilot's configuration.
A measure's rate was the whole run's
aggregateOfficialRunpagedlistOutcomes(runId)with no measure filter, under a comment asserting that "a run evaluates one measure with one engine". That is true of a MEASURE-scoped run and false of the ALL_PROGRAMS run the pilot actually runs, which holds one row per (subject, measure) pair. So it summed every measure the run touched and served that one number under whichever measure was asked about.On the sandbox, CMS122's card carried CMS125's initial population, CMS125's 59.2% and
ecqmId: 125FHIR. With six measures routed it gets six times wronger.The MeasureReport and reconciliation routes were unaffected — both refuse a multi-measure run with a 422 via
distinctMeasuresForRun— so the programs overview was the single unguarded consumer.listOutcomesnow takes an optionalmeasureIdand narrows in SQL, before the page window, on both stores. Filtering after the read would page 120,000 rows once per measure on a six-measure nightly.runProducedOfficialEvidenceis scoped the same way, so a run mixing an official and an authored measure (ADR-072 admits one) no longer answers for both from whichever row sorted first.Guarded by two tests chosen so they cannot pass vacuously:
The e2e suite had been green by absence since 2026-09-03
The job is
workflow_dispatch-only, so nothing ran it across seven merges. A manual dispatch returned 32 passed, 11 failed. None was a backend regression:official-pendingand the specs feat(corpus): MM-1 U2 — the Maui roster is a 20,000-patient data-first corpus, and a multi-rate measure is read as every one of its rates (ADR-073/074/075) #528 added for them waited 30s for chips that could not exist.employeeExternalIdwhere a patient deployment emitspatientExternalId) — the stack was right and the test was wrong.<option>inside a closed select, because the panel filters gave those options names containing clinic names.<label>, sogetByLabel(/^PCP$/)resolved nothing. They now carryaria-labels, matching Panel/System/Segment which already had them.The job now runs the configuration the sandbox deploys — it vendors the six terminology sidecars with the VSAC credential and sets
WORKWELL_OFFICIAL_MEASURES. A suite that boots a stack nobody deploys is not a pilot test. Confirmed by the backend's own boot line on every run since:On the credential, since this is a public repo
The key is scoped to a single step, never job-level
env, and that step runs before the frontend and e2e dependency trees are installed — so it executes with the backend tree only. The job isworkflow_dispatch-gated, which needs write access, and GitHub does not pass secrets to fork-PR workflows regardless.The completed expansions are deliberately not cached. They are VSAC value-set expansions — licensed content, gitignored for that reason — and an Actions cache is restorable by a fork pull request. Re-vendoring costs 45s.
Speed
The suite was never slow; the failures were. Of the original 8m24s, the passing tests were about 20 seconds and the rest was 11 failures burning 20-60s timeouts and retrying.
Fewer tests, more covered: six measures instead of two, and the two vacuous assertions made real. The trimming was collapsing per-page loops and a drill-down that navigated and reloaded once per chip per measure. Beyond that: four workers instead of one, and one sign-in per (role, spec file) instead of ~40.
What review caught, and what the first CI runs caught
Three rounds, all recorded because they are the interesting part:
classifyRunnablereturnsofficial-pendingfor one reason — the id missing fromWORKWELL_OFFICIAL_MEASURES— and never returns it for cms122/cms125, which reach theauthoredbranch first. A routing list that lost those two would have printedcms122:authored, passed a negative grep, and run the pilot's flagship measures on the authored engine whileruns.spec's 48x6 held either way. Now a positive exact match on<id>:official, tested against four boot-line shapes including the one the old job produced. Terminology is not part of that classification at all, so the sidecar control is and always was thetruncatedrefusal in the vendor step.npx playwright testwith no--project, loading both projects, and Playwright resolvesstorageStatebefore thebeforeEachthat skips — so the maui specs would have thrown ENOENT instead of skipping. The session helpers now return{}off the maui profile; verified by listing both projects with no.auth/present.rows <= 2(satisfied by matching nothing), a chip/worklist bound satisfied by any sum when the worklist rendered 0, and a drill-downrowCount <= chip.countthat passed at zero — which is precisely what a chip href that stopped filtering would produce, and the settle-wait accepts the empty state, so nothing else would have caught it.Then CI itself caught what review could not: the first four-worker run came back 4 flaky, 22 passed — green only via retry, including a roster reading "0 patients" for 20 seconds while
runs.spec's ALL_PROGRAMS run was in flight. The Maui project now splits intomaui(read-only, four workers) andmaui-writes(runs.specalone) withdependencies: ["maui"], making the ordering a constraint rather than a hope. The last remaining flake was mine: the split leftreadChipscounting locators before the cards had rendered.Not asserted as a product bug: the roster read model already excludes in-flight runs in SQL and again in app code, so the mechanism behind that empty read is unidentified. If an empty roster appears when no run is in flight, that is the thread to pull.
Verification
pnpm typecheckclean; backend fhir/program/export 288 pass, routes 397 pass, SQLite store contract 89 pass.postgres:16); CI covers it.Known and not addressed here
The live six-measure run is slow — about 9.7 (subject, measure) pairs per second against Neon, steady between 17% and 50% completion. Outcomes are already batched; the case pass is not, and costs 2-3 sequential round trips per pair (
findByKey, then insert/update, then the audit append). That is ~300,000 round trips for a 120,000-pair run, and the flat rate says it is a fixed per-pair charge rather than anything compounding. Batching it per chunk the wayrecordOutcomesalready batches outcomes is a follow-up PR, deliberately separate: it touches the idempotency contract (DATA_MODEL_CONTRACTS §4) and ADR-076 d2's compare-and-set, so the store-contract tests go first.