Uh oh!
There was an error while loading. Please reload this page.
fix: read-your-writes + global-scope idempotency correctness under the run-ops split - #4284
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 (10)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (9)
📜 Recent review details⏰ Context from checks skipped due to timeout. (24)
WalkthroughThe changes add primary fallbacks for replica-miss run, batch, waitpoint, and session lookups across API, realtime, resource, presenter, routing, and execution paths. Batch routes also mark not-found responses as retryable. Run-store routing now uses owning-store reads for classifiable runs and colocated dedup probes. Split-mode global idempotency handling adds claim TTL calculation, compare-and-delete reset, cross-database winner resolution, bounded cleared-winner reacquisition, and explicit claim publish outcomes. 🚥 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 |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
@trigger.dev/buildtrigger.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/sdkcommit: |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
…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
e4c9b5cCompareDead 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.
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.)
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
1c6e6ff to
2941d71CompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
2941d71 to
6ae9b8aCompareUh oh!
There was an error while loading. Please reload this page.
…me batch hardening Production follow-ups from the CodeRabbit/Claude/Devin review. Caller-driven guard tests are in the stacked tests PR; the two test edits here are coupled to the production change (the publishClaim signature change invalidates a main assertion; the fan-out removal obsoletes a main test). - Claim TTL floor (TRIGGER_MOLLIFIER_CLAIM_MIN_TTL_SECONDS, default 5) independent of the customer key TTL, so a short key TTL can't expire the claim mid-pipeline and let a loser re-claim. - publishClaim returns the buffer CAS result; the trigger success path detects + logs a no-op'd publish. - reacquireClearedGlobalWinner fails closed with a retryable 503 (exhaustion / unfindable winner); the expired/failed clear-and-recreate path routes through it too (Devin), so that recreate is serialised. - Realtime batch route re-reads the owning primary on a replica miss (backend-agnostic; closes the Electric ShapeStream permanent-404 for self-hosters). - Remove the classifiable-id cross-store fan-out from findRun: a run's id-shape fixes its residency for life, so the single-store read is correct and the fan-out was dead code.
568b3bf to
1c625e1CompareUh oh!
There was an error while loading. Please reload this page.
… (#4285) ## Stacked on #4284 — tests only This PR contains **only the tests** that guard the production fixes in #4284 (its base). Review #4284 first; this branch adds no production code. ## What Caller-driven replica-lag and idempotency guards for every fixed site: - Each guard **drives the real exported caller** (route loader/action, presenter `.call()`, service, or engine method) against a **real Postgres** with the owning replica frozen via the shared `laggingReplica` testcontainer primitive — never a store-seam reimplementation. - For a **fixed** site the guard goes **RED when the production change is reverted**; for a **tolerated read-view** site it's a caller-driven **GREEN** proof the miss self-heals (returns null/empty, no mutation, row live on primary). - The **global-scope idempotency** guard drives the real dedup + claim path through a **real `MollifierBuffer` over a Redis testcontainer** (real SETNX/poll/publish), and covers the cross-DB **andWait** waitpoint wiring and the **expired/failed clear-and-recreate** reacquire cases. Run with `vitest --no-file-parallelism` (testcontainers). Verified GREEN, and revert→RED verified per fixed site.
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.