fix: read-your-writes + global-scope idempotency correctness under the run-ops split#4284
fix: read-your-writes + global-scope idempotency correctness under the run-ops split#4284d-cs wants to merge 4 commits into
Conversation
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (24)
🧰 Additional context used📓 Path-based instructions (9)**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
{packages/core,apps/webapp}/**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{test,spec}.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.ts📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
Files:
apps/webapp/**/*.{ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
apps/webapp/**/*.test.{ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
apps/**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/**/*.{test,spec}.{ts,tsx}📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
🧠 Learnings (17)📚 Learning: 2026-03-22T13:26:12.060ZApplied to files:
📚 Learning: 2026-03-22T19:24:14.403ZApplied to files:
📚 Learning: 2026-05-18T08:21:27.694ZApplied to files:
📚 Learning: 2026-05-18T08:21:27.694ZApplied to files:
📚 Learning: 2026-06-13T19:53:13.759ZApplied to files:
📚 Learning: 2026-06-17T17:13:49.929ZApplied to files:
📚 Learning: 2026-06-23T13:04:21.413ZApplied to files:
📚 Learning: 2026-05-07T12:25:18.271ZApplied to files:
📚 Learning: 2026-05-28T20:02:10.647ZApplied to files:
📚 Learning: 2026-05-12T21:04:05.815ZApplied to files:
📚 Learning: 2026-06-25T18:21:51.905ZApplied to files:
📚 Learning: 2026-07-03T17:10:21.498ZApplied to files:
📚 Learning: 2026-05-18T14:40:02.173ZApplied to files:
📚 Learning: 2026-06-04T18:16:35.386ZApplied to files:
📚 Learning: 2026-06-09T17:58:04.699ZApplied to files:
📚 Learning: 2026-06-16T09:19:47.637ZApplied to files:
📚 Learning: 2026-07-18T14:01:28.319ZApplied to files:
🔇 Additional comments (1)
WalkthroughThe changes add replica-first primary fallbacks across run, batch, waitpoint, session, metadata, realtime, stream, and presenter reads. Several batch routes now mark not-found responses as retryable. Engine and store operations use primary-visible reads for read-your-writes cases. Split idempotency handling now resolves winners across databases, centralizes existing-run processing, and supports bounded reacquisition after safely resetting cleared Redis claims. 🚥 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 |
![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.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
@trigger.dev/build
trigger.dev
@trigger.dev/core
@trigger.dev/python
@trigger.dev/react-hooks
@trigger.dev/redis-worker
@trigger.dev/rsc
@trigger.dev/schema-to-json
@trigger.dev/sdk
commit: |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…bal-scope idempotency correctness under the run-ops split Production code only — the guarding tests are in the stacked PR. 1) Read-your-writes: run-store reads that gate a mutation or feed a public GET / realtime response were routed to a lagging read replica, so a just-written run/waitpoint/batch could spuriously miss under replica lag. Route those reads to the owning primary (findRun/findWaitpoint/findBatchTaskRunByFriendlyId -> *OnPrimary, a primary re-read on a miss, or a retryable 404 where the SDK polls). Additive: the happy path is unchanged; a primary read happens only on a miss. 2) Global-scope idempotency across the split: a global-scope key carries no per-run salt, so the same (env, task, key) triggered concurrently from parents resident on different run-ops DBs could dedup-miss on each DB and create a duplicate run (the per-DB unique index can't enforce cross-DB uniqueness). Serialize such triggers (global scope, or scope-absent, while split is active) through the existing Redis idempotency claim, resolve the winner by id across both DBs, and reacquire the claim on the expired/failed clear-and-recreate path. run/attempt scope embed the run id in their hash and never contend.
bf4ee4f to
e4c9b5c
Compare
d-cs
changed the title
Dead since #4272 inlined per-write residency routing at each call site — the private helper had zero callers (only doc-comments referenced it by name), tripping eslint no-unused-private-class-members. Remove it and reword the comments that referenced it.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the read-your-writes routing and the global-scope idempotency claim, plus a completeness sweep of run/waitpoint/batch/snapshot read-after-write paths across the engine and webapp. Core routing looks correct and complete — no read that must observe its own write still reads the replica, and the readOnlyPrisma → prisma change in runAttemptSystem is a genuine correctness fix. Four inline notes: two edges worth tightening (both re-open the duplicate/stale-read class this PR closes) and two minors. None a blocker.
(Note: the third inline note is attached as a file-level comment on internal-packages/run-store/src/runOpsStore.ts because its target — the findRunOnPrimary() classifiable-id path around lines 811-821 — is unchanged code outside the PR's diff hunks.)
There was a problem hiding this comment.
Re findRunOnPrimary() (this file, the classifiable-id path): it reads only the classified owner's primary; unlike routed findRun, it doesn't fan out to the other store's primary on a miss, so the findRun($replica) ?? findRunOnPrimary() idiom can still 404 a diverged-residency run. Either fan out to the other store's primary here or standardize those call sites on findRun(where, args, prisma). (findWaitpointOnPrimary already fans out via #resolveWaitpointStore, so it's fine.)
There was a problem hiding this comment.
Refuting this one — it isn't a bug for runs. A run keeps its id for its entire life and never migrates between stores, so it always lives on exactly the store its id-shape classifies to; findRunOnPrimary reading that single classified store is correct, and a cross-store fan-out would be dead code. The residency-divergence premise holds for waitpoints (drain-on-read can relocate them, which is why findWaitpointOnPrimary fans out) but not for runs.
In fact I went the other way in 1c6e6ff6e and removed the pre-existing classifiable-id cross-store fan-out from findRun for the same reason. (The unclassifiable-where fan-out stays — with no id there's nothing to route by, so both stores must be probed.)
There was a problem hiding this comment.
Conceding this — you're right that a run's residency is fixed by its id-shape, so the single-store read is correct and a cross-store fan-out would be dead code. Making findRun symmetric with findRunOnPrimary (both single-store on 6ae9b8af) is the cleaner call.
One follow-up that falls out of removing the fan-out: runOpsStore.residencyMismatchFallback.test.ts on the #4285 branch still asserts the classifiable-id on-miss fan-out — it expects a #new-resident run that classifies LEGACY to be found via owning-store-miss → other-store fallback. Against this head that path is gone, so the test contradicts the new behaviour and would fail once the branches reconcile. Either drop it along with the fan-out, or keep the invariant it guards.
1c6e6ff to
2941d71
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const idempotencyKeyScope = request.body.options?.idempotencyKeyOptions?.scope; | ||
| const globalUnderSplit = | ||
| (idempotencyKeyScope === "global" || idempotencyKeyScope === undefined) && | ||
| (await isSplitEnabled()); | ||
|
|
||
| const claimEligible = | ||
| !request.body.options?.resumeParentOnCompletion && | ||
| !request.body.options?.debounce && | ||
| !request.options?.oneTimeUseToken && | ||
| (await resolveOrgMollifierFlag({ | ||
| envId: request.environment.id, | ||
| orgId: request.environment.organizationId, | ||
| taskId: request.taskId, | ||
| orgFeatureFlags: | ||
| (request.environment.organization?.featureFlags as | ||
| | Record<string, unknown> | ||
| | null | ||
| | undefined) ?? null, | ||
| })); | ||
| (globalUnderSplit || | ||
| (!request.body.options?.resumeParentOnCompletion && | ||
| (await resolveOrgMollifierFlag({ | ||
| envId: request.environment.id, | ||
| orgId: request.environment.organizationId, | ||
| taskId: request.taskId, | ||
| orgFeatureFlags: | ||
| (request.environment.organization?.featureFlags as | ||
| | Record<string, unknown> | ||
| | null | ||
| | undefined) ?? null, | ||
| })))); |
There was a problem hiding this comment.
🔍 global-scope claim now runs for non-mollifier orgs, adding Redis RTT to the trigger hot path
In idempotencyKeys.server.ts, claimEligible now short-circuits to true via globalUnderSplit (idempotencyKeyScope === 'global' || undefined) && isSplitEnabled()), bypassing the per-org mollifier flag entirely. Per the inline comment this is intentional (the claim is the only cross-DB mutex a global-scope key has), but it means that once the run-ops split is enabled, EVERY global-scope (and scope-absent) idempotency-keyed trigger — including from orgs that never opted into the mollifier — now pays a Redis SETNX round-trip on triggerTask, the system's highest-throughput path (see apps/webapp/CLAUDE.md hot-path guidance). Additionally resolveOrgMollifierFlag is no longer evaluated when globalUnderSplit is true due to || short-circuiting; it is a pure in-memory read with no side effects so this is benign. Flagging the latency/throughput implication for confirmation that it was weighed against the split rollout.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Confirmed and accepted — this is a deliberate correctness-over-latency choice, and the "non-opted-in orgs pay a Redis RTT" concern doesn't apply: the mollifier is being rolled out to all orgs, so there's no opted-out class taking a penalty — every org is on it. The claim is the only cross-DB mutex a global-scope (or scope-absent) key has under the split (the per-DB unique index can't enforce cross-DB uniqueness), so the SETNX on that path is required for dedup correctness, and it was weighed against the rollout. Agreed the resolveOrgMollifierFlag short-circuit is benign.
…tch reads Follow-ups from review of the read-your-writes / idempotency work. Tests are included here for now and will be split into the stacked tests PR. - Claim TTL floor: the mollifier claim TTL was min(max, keyTTL), so a short customer idempotencyKeyTTL (1-2s) could expire the claim mid-pipeline and let a polling loser re-claim -> cross-DB duplicate under the split. Floor it at a pipeline bound (TRIGGER_MOLLIFIER_CLAIM_MIN_TTL_SECONDS, default 5), independent of the key TTL. - Publish compare-and-set: publishClaim returns the buffer CAS result; the trigger success path detects a no-op'd publish (claim expired, another claimant moved in) and logs it rather than silently treating its own run as canonical. - Reacquire exhaustion: reacquireClearedGlobalWinner failed OPEN on exhausting the bounded cleared-winner reacquires (or an unfindable winner), creating an unserialised run that can dual-create across DBs. Fail CLOSED with a retryable 503 so the SDK retry re-serialises (mirrors the timed_out branch). - Realtime batch route: findResource re-reads the owning primary on a replica miss, so the Electric ShapeStream consumer (self-hosters), which ignores the retryable-404 header, doesn't strand a live just-created batch on a permanent 404. - Cleanup: remove the classifiable-id cross-store fan-out from findRun. Runs keep their id for life and never migrate between stores, so a run lives on exactly the store its id-shape classifies to; probing the other store on a miss was dead defensive code. (Unclassifiable-where fan-out stays; it has no id to route by.)
2941d71 to
6ae9b8a
Compare
| // Unclassifiable where (e.g. spanId, idempotencyKey): the run may live on either DB, | ||
| // so fan out NEW-first then LEGACY rather than defaulting to NEW — defaulting silently | ||
| // misses legacy-resident runs (span detail, idempotency-dedup probe, etc.). | ||
| return this.#findRunUnrouted(where, args, onPrimary); |
There was a problem hiding this comment.
🔍 findRun drops cross-store on-miss fan-out for classifiable ids
RoutingRunStore.findRun for a classifiable id previously routed to the owning store and, on a miss, fell back to the OTHER store (#findRunRouted, now removed). The new code (internal-packages/run-store/src/runOpsStore.ts:231-236) reads only the single store named by id-shape. The PR also deletes runOpsStore.residencyMismatchFallback.test.ts, whose header explicitly documented the failure mode this fallback guarded: a pre-#4154 base62 run physically resident on NEW that classifies LEGACY would route to LEGACY, miss, and return null → a spurious 404, even though the run is physically present on the other DB.
Mitigating factor: findRunOnPrimary's classifiable branch (runOpsStore.ts:792-796) was ALREADY single-store before this PR, so the read-your-writes primary path never had this fallback; the fan-out only existed on the replica findRun path. So this change makes findRun consistent with findRunOnPrimary rather than introducing a wholly new gap. Whether it manifests in production depends on whether any residency≠classification runs still exist post-#4154. If such runs exist, this is a regression: reads across the whole surface touched by this PR (realtime subscriptions, metadata GET, run detail, stream routes) could spuriously 404 for those runs. Worth confirming with the team that the #4154 migration eliminated all residency-mismatched runs before relying on id-shape as 'destiny'.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Confirmed as intended. Under the current design a run's residency is fixed by its id-shape for its whole life — runs never migrate between stores — so no residency≠classification rows exist. The fan-out (and its residencyMismatchFallback test) guarded the old pre-#4154 base62 divergence case that this design eliminates, so both are removed and findRun/findRunOnPrimary are now symmetric single-store reads. (The test still lives on the stacked tests branch #4285 and will be dropped when the branches reconcile — thanks for flagging.)
…he claim Devin review follow-up. Only the claim-LOSER cleared path (reacquireClearedGlobalWinner) was claim-serialised; the INITIAL existingRun probe's expired/failed path cleared the key and fell through to an UNSERIALISED recreate. Under the split, two concurrent cross-residency triggers on an expired/failed global-scope key could each recreate on their own DB (the per-DB unique index can't dedup cross-residency). Route that initial-probe recreate through the same reacquireClearedGlobalWinner claim serialisation when global-under-split; non-split / non-global keeps the plain recreate.
2 participants
What & why
Two related correctness fixes for the run-ops DB split. Under the split, run-store reads can route to a lagging read replica; a just-written run/waitpoint/batch can then be missed, causing a wrong decision.
1. Read-your-writes → owning primary. Surfaced first as an intermittent
wait.until({ idempotencyKey })re-wait on retry. Auditing the run-store read surface found the same class at sibling sites (some gating mutations or returning spurious 404s, others tolerable/self-healing). Reads that must observe their own writes now route to the owning primary (findRun/findWaitpoint/findBatchTaskRunByFriendlyId→*OnPrimary, a primary re-read on a miss, or a retryable 404 where the SDK polls). Read-view reads stay on the replica. All additive — the happy path is unchanged.2. Global-scope idempotency across the split. A
global-scope key carries no per-run salt, so the same(env, task, key)triggered concurrently from parents resident on different run-ops DBs could dedup-miss on each DB and create a duplicate (the per-DB unique index can't enforce cross-DB uniqueness). Such triggers (global scope, or scope-absent, while split is active) are serialized through the existing Redis idempotency claim, the loser resolves the winner by id across both DBs, and the claim is reacquired on the expired/failed clear-and-recreate path.run/attemptscope embed the run id and never contend.Stacked for review
This is the base of a 2-PR stack, split so review is easier:
Validation
Local run-ops split, both 2-DB and 3-DB, fresh boot on this branch: SDK canary 64/71 (only the known concurrency/input-streams/s3 failures), quarantine sweep 0 unexpected (340 pass / 16 known / 4 local) in each topology, dashboard e2e 0 failed. No product regressions.