perf(webapp,run-store): grouped run-ops reads + mint-kind flip grace#4227
perf(webapp,run-store): grouped run-ops reads + mint-kind flip grace#4227d-cs wants to merge 21 commits into
Conversation
Several run-ops read paths issued one database query per item where a single batched query returns the same data: batch results hydrated each member run separately, the run retrieve endpoint resolved each locked worker version separately, the dedicated-schema relation hydrator ran per row, and a few services read runs in per-id loops. These now group into batched reads. Adds a grouped findRunsByIds to the run store (one residency-partitioned read for a set of run ids) and routes the presenters and services through it. The dedicated-schema relation hydrator batches across all rows per relation. The waitpoint connected-runs read also regains its display limit at the fetch, so it no longer loads every connected run into memory before slicing to five.
…n unrequested findRunsByIds forces `id` into the select so it can key the result map, but did not remove it afterwards, so returned rows carried an `id` the declared payload type did not include. Delete the injected id from each value when the caller's select did not request it; the map stays keyed by id.
…list The waitpoint detail view capped the connected-run lookup before checking the runs still existed, so a run deleted after connecting could take a slot ahead of a live one and under-count (or empty) the list. Join to the run table so the cap only ever lands on connections whose run still exists.
Batched relation hydration returned the same row object to every parent linking the same target, so two parents connected to one run shared a single instance and an in-place edit to one leaked into the other. Return a shallow clone on the bare-projection path so each parent gets a distinct object.
The connected-runs relation read fetched every connection id for a waitpoint with no existence check or limit, so a heavily-connected waitpoint could pull an unbounded id list into the grouped run lookup. Existence-join to the run table and cap at the shared connected-runs limit on both schema branches, and slice the cross-store union to the same bound.
The dedicated-schema relation hydrators fetched full target rows and then stripped unrequested fields in memory, pulling wide columns even when the caller only selected a couple. Push the caller's select into the target findMany so only the requested columns are read. No change to results.
Flipping an organization's run-ops mint kind (which database new runs are minted on) took effect independently per process as each cached value expired. For a window after a flip, two concurrent triggers using the same idempotency key could mint on different databases, where the per-database unique constraint can't dedupe them, producing a duplicate run. The flip is now a deterministic wall-clock cutover: the admin feature-flag routes stamp the previously-effective kind and a flip timestamp onto the organization, and the mint read resolves the old kind until flippedAt + grace (default 90s, chosen to outlast the caches a flip drains through), after which every process crosses to the new kind together. During the window all processes agree on one database, so a concurrent same-key collision lands on a single database and the existing unique-constraint retry returns the one idempotent run. A same-target re-save carries the in-flight stamp forward, so it can't slide the cutover.
…clock The grace-window cutover was timestamped with the webapp process's own clock, so clock skew between processes could shift the boundary. Read the time from the control-plane database instead, so the flip is anchored to one authoritative clock rather than whichever process handled the flag change.
The grace cutover compares the reader's wall clock against a DB-clock flippedAt stamp, so it assumes clock-synced hosts (skew well under the grace window). Records that invariant and the accepted residual on effectiveMintKind. No behavior change.
Global runOpsMintKind flips now stamp the previous kind and flip time like per-org flips, so a global flip is a deterministic cutover and cannot route two concurrent triggers that share an idempotency key to different databases. The resolver reads the grace stamp from the same source as the kind.
…isses RoutingRunStore.findRun routed a classifiable id to its single owning store by id shape and returned null on a miss, so a run whose physical residency diverges from its id shape would 404 even though it exists. Read the owning store first, then fall back to the other store only on a miss, mirroring the unrouted fan-out. The found-in-owning-store path is unchanged (single read).
|
WalkthroughAdds deterministic mint-kind flip grace handling with server-side timestamp stamping, new feature-flag metadata, and cache-aware resolution. Replaces several per-record database reads with grouped queries and batched relation hydration across legacy and dedicated stores. Adds bounded connected-run retrieval, residency fallback routing, projection isolation, and presenter integrations. Expands integration coverage for grouped reads, split-store behavior, schedule residency, connected-run limits, mint flips, and cross-database waitpoint resumption. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…int kind stampMintKindFlip injected the default mint kind into every org feature-flag save, so an unrelated flag change wrote an explicit per-org runOpsMintKind override, pinning the org to that kind and making a later global flip silently skip it. Only stamp when the save actually sets runOpsMintKind.
The raw queries reading a waitpoint's connected runs relied on search_path, so they broke on deployments using a non-public database schema. Qualify the tables with the configured schema so they resolve correctly.
…ields An org's first per-org mint-flag override computed its cutover grace window against the wrong baseline, which could skip the grace window and briefly let two concurrent triggers sharing an idempotency key resolve to different databases. Seed the baseline from the effective global flag, strip the derived stamp fields from the request body, and apply the read-and-stamp atomically.
…d schema The dedicated-schema hydrator for a waitpoint's connectedRuns relation fetched an unbounded list of connections and full run rows. Bound it per parent with a window function so a waitpoint with many connections cannot pull a large result set for a display-only field.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…w SQL Replaces the raw connected-runs query, which JOINed the waitpoint connection table onto the large TaskRun table, with two indexed ORM reads joined in memory: read the connection rows, then resolve the runs by id. The id lookup can only plan as a primary-key lookup, so the query planner never scans TaskRun, and Prisma qualifies the schema per client so the previous manual schema handling is gone.
How idempotency was testedTwo properties were checked directly against the running system, one for each failure mode a two-database split introduces: 1. Concurrent dedup, on each database. With the residency flag fixed to one database, a burst of concurrent triggers sharing a single idempotency key was issued. The result is exactly one run: the first insert wins and every other caller receives that same run id, because the per-database unique index on the idempotency key rejects the duplicate inserts. Repeated with the flag fixed to the other database: same result, one run. 2. Dedup across the residency flip. A key is first used while new runs mint into database A. The flag is then flipped so new runs mint into database B, and the same key is triggered again. It returns the original run id from database A rather than minting a second run in database B. This is the case a per-database unique constraint cannot catch on its own (the constraint in B cannot see the row in A), so it confirms the pre-mint existing-run lookup spans both databases. Together these close both gaps a split could open: a race within one database (rejected by the unique constraint) and a key reused across the cutover (caught by the cross-database lookup before a second run is minted). |
| const existingRows = await prisma.featureFlag.findMany({ | ||
| where: { | ||
| key: { | ||
| in: [ | ||
| FEATURE_FLAG.runOpsMintKind, | ||
| FEATURE_FLAG.runOpsMintKindPrev, | ||
| FEATURE_FLAG.runOpsMintKindFlippedAt, | ||
| ], | ||
| }, | ||
| }, | ||
| select: { key: true, value: true }, | ||
| }); | ||
| const existingGlobal: Record<string, unknown> = {}; | ||
| for (const row of existingRows) { | ||
| existingGlobal[row.key] = row.value; | ||
| } | ||
|
|
||
| // Anchor the cutover to the control-plane DB clock, not this process's wall clock. | ||
| const [{ now: controlPlaneNow }] = await prisma.$queryRaw< | ||
| { now: Date }[] | ||
| >`SELECT now() AS now`; | ||
|
|
||
| flagsToWrite = stampMintKindFlip( | ||
| existingGlobal, | ||
| { ...requestedFlags }, | ||
| controlPlaneNow.getTime(), | ||
| env.RUN_OPS_MINT_FLIP_GRACE_MS | ||
| ) as Partial<FeatureFlagCatalog>; | ||
| } | ||
|
|
||
| const setMultipleFlags = makeSetMultipleFlags(prisma); | ||
| const updatedFlags = await setMultipleFlags(featureFlags); | ||
| const updatedFlags = await setMultipleFlags(flagsToWrite); |
There was a problem hiding this comment.
🟡 Concurrent global mint-kind flag flips can clobber each other's grace-window metadata
The global feature-flag save reads existing flags and writes the stamped result without a transaction or row lock (admin.api.v1.feature-flags.ts:46-77), so two concurrent requests can both read the same state and one silently overwrites the other's grace stamp.
Impact: A lost grace stamp means the deterministic cutover window doesn't fire, briefly reopening the cross-DB duplicate window the grace mechanism was designed to close.
Race condition: read-then-write without locking on the global flags route
The per-org routes at apps/webapp/app/routes/admin.api.v1.orgs.$organizationId.feature-flags.ts:83-124 and apps/webapp/app/routes/admin.api.v2.orgs.$organizationId.feature-flags.ts:150-176 both use prisma.$transaction(async (tx) => { ... SELECT ... FOR UPDATE ... }) to atomically read, stamp, and write the grace metadata.
The global flags route at apps/webapp/app/routes/admin.api.v1.feature-flags.ts:46-77 does the same read→stamp→write sequence but without any transaction or lock:
- Line 46-61: reads existing global flags via
prisma.featureFlag.findMany - Line 68-73: computes the stamp via
stampMintKindFlip - Line 77: writes via
setMultipleFlags(individual upserts)
If two requests race, both read the same existingGlobal, both compute a stamp against it, and the second write silently overwrites the first's runOpsMintKindPrev / runOpsMintKindFlippedAt values. The per-org routes explicitly document this as the "read-then-write race" they lock against.
Prompt for agents
The global feature-flags route (admin.api.v1.feature-flags.ts) performs a read-then-write of the mint-kind grace stamp (lines 46-77) without any transaction or row-level lock, unlike the per-org routes which both use prisma.$transaction with SELECT ... FOR UPDATE to prevent concurrent clobbering.
To fix: wrap the existing-flags read (featureFlag.findMany), the DB-clock read (SELECT now()), the stampMintKindFlip computation, and the setMultipleFlags write in a single prisma.$transaction. Inside the transaction, use a SELECT ... FOR UPDATE on the three mint-flag FeatureFlag rows to serialize concurrent flips. This mirrors the pattern already used in admin.api.v1.orgs.$organizationId.feature-flags.ts lines 83-124 and admin.api.v2.orgs.$organizationId.feature-flags.ts lines 150-176.
The FeatureFlag table uses individual rows keyed by `key`, so the FOR UPDATE should lock the three rows with keys FEATURE_FLAG.runOpsMintKind, FEATURE_FLAG.runOpsMintKindPrev, and FEATURE_FLAG.runOpsMintKindFlippedAt.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
Two threads on the run-ops split path.
Read path: per-item run reads are batched into grouped queries, a waitpoint's connected-run reads are bounded, and the dedicated-schema relation hydrators fetch only the requested columns instead of whole rows. Retrieve also falls back to the other database when a routed read misses, so a run whose physical residency diverges from its id shape is still found rather than returning a spurious not-found. Fewer and lighter queries on the run read path, with no change to results.
Mint-kind flip safety: flipping which database new runs mint to is now a deterministic wall-clock cutover, for both per-org and global flips. For a grace window every process resolves the same database, so a flip cannot route two concurrent triggers that share an idempotency key to different databases (which would bypass the per-database unique constraint and create a duplicate run).
Supersedes the earlier #4205 and #4208.
Draft: validation in progress.