feat(webapp,run-store): gen-2 shard arms in read-through and idempotency - #4781
Draft
d-cs wants to merge 6 commits into
Draft
feat(webapp,run-store): gen-2 shard arms in read-through and idempotency#4781d-cs wants to merge 6 commits into
d-cs wants to merge 6 commits into
Conversation
|
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe change adds shard-keyed replica and writer maps for run operations. Read-through requests now use explicit identifier kinds and resolve gen-2 IDs to dedicated shard clients. Batch hydration, API presenters, waitpoint resolution, and idempotency lookups use shard-aware routing. Results now expose structured 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description is detailed and directly covers the implementation, behavior changes, merge safety, testing, and issue reference. It does not include the template checklist, changelog, or screenshots sections, but the technical and testing information is sufficiently complete. Full details: Docstring CoverageExplanation Docstring coverage is 31.25% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 16 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
Route read-through by `resolveShard` instead of the binary residency classifier. A gen-2 id takes one read on its own shard's replica and probes no other store; a gen-1 v1 id still reads new only. Callers now declare `idKind`, because a cuid gives no way to tell a run id from a waitpoint id and the two must route differently. A legacy run id reads the legacy replica only, since there is no cuid run migration and the new-store probe cannot find it. A cuid waitpoint keeps the new-first pair probe, which is load-bearing because a cuid waitpoint can be co-located with its run on the new store. `ReadThroughResult` now carries `found` structurally. `source` is an open-ended union once shards exist, so a consumer testing found-ness by listing hit sources would read a gen-2 hit as a miss; discriminating on `found` turns that class of bug into a compile error. An id resolving to an unconfigured shard key logs an error and returns not-found rather than throwing. These ids arrive from callers (a URL param on the waitpoint route) and any base32hex core plus `[a-z0-9]` plus "2" parses as gen-2, so a throw would be a 500 any client could induce by guessing a shard char. Throwing stays correct on the router path, where ids are minted rather than received. The read-your-writes fallback for a gen-2 waitpoint now reads that shard's own writer. The gen-1 new writer is a different database, so reading it would miss and silently disable read-your-writes for the freshly minted token that fallback exists to serve. Inert while RUN_OPS_SHARDS is unset: the shard maps are empty, so every gen-2 arm is unreachable.
…e shard map
Idempotency resolved its client with a binary `ownerEngine(id) === "NEW"`
ternary, which cannot name a gen-2 shard. Both call sites now resolve
through one shard-keyed map, so they cannot disagree about which store
owns an id. An absent key takes an explicit logged branch to the
fallback rather than a silent legacy default.
`clientForShardKey` is the single place an id becomes a client.
`ShardKey` collapses to `string`, so the compiler cannot catch a wrong
key here; the `classify` seam is retyped to return a `ShardKey` so a
`Residency` value ("NEW") can no longer be fed into a shard-key
parameter, which the two differ from each other only by case.
`resolveIdempotencyDedupClient` keeps its policy. The mint-kind branch
has no id to decode and still resolves to the gen-1 pair.
Delete the `isMigrated` branch. Nothing implements it, and the one
production comment recorded that omitting it was deliberate.
The two cross-seam batch hydration sites classified with the binary
`ownerEngine` too. A gen-2 id joined the gen-1 `new` group, missed
there, and — classifying dedicated-family — never reached the legacy
probe either, so it was dropped from a bulk-action page and from batch
results with no error. Both now partition ids by shard key and read each
configured shard once.
`PostgresRunStore._residency` widens to `ShardKey` for call-site
consistency. It stays unused; the store stays unaware of its siblings.
Inert while RUN_OPS_SHARDS is unset: the shard map holds only the two
reserved gen-1 keys, so every partition falls through to today's paths.
…d read The baseline is keyed by line number, so partitioning the batch-results read by shard shifted the four pre-existing entries in ApiBatchResultsPresenter and added one. The new entry is the shard read beside the two gen-1 reads already in the baseline: the same explicitly-passed replica handle, the same read pattern. Net baselined violations in this file go from four to five, all reads, none new in kind.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/webapp/test/apiBatchResultsPresenter.dedicatedSeam.test.ts (1)
341-415: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPlace the presenter test next to its source file.
Move this test to a test file beside
apps/webapp/app/presenters/v3/ApiBatchResultsPresenter.server.ts.As per coding guidelines, “Test files go next to source files.”
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ec81c25e-c811-4174-bd56-5e290e146920
📒 Files selected for processing (17)
apps/webapp/app/presenters/v3/ApiBatchResultsPresenter.server.tsapps/webapp/app/runEngine/concerns/idempotencyKeys.server.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.tsapps/webapp/app/runEngine/concerns/resolveWaitpointThroughReadThrough.server.tsapps/webapp/app/v3/runEngineHandlersShared.server.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.test.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.test.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.tsapps/webapp/app/v3/runOpsMigration/track1-baseline.jsonapps/webapp/app/v3/runOpsMigration/waitpointTokenResolve.server.test.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.test.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.tsapps/webapp/test/apiBatchResultsPresenter.dedicatedSeam.test.tsapps/webapp/test/resolveWaitpointThroughReadThrough.readthrough.test.tsinternal-packages/run-store/src/PostgresRunStore.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: internal / 🧪 Unit Tests: Internal
🧰 Additional context used
📓 Path-based instructions (14)
New code must target Run Engine V2 through the singleton in `app/v3/runEngine.server.ts`; do not reintroduce V1 execution paths. V1 branches may only reject or finalize gracefully with a clean 4xx.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/v3/runOpsMigration/shardHandles.server.test.tsapps/webapp/app/v3/runOpsMigration/waitpointTokenResolve.server.test.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.tsapps/webapp/app/v3/runEngineHandlersShared.server.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.test.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.test.ts
Never use `request.signal` to detect client disconnects. Use `getRequestAbortSignal()` from `app/services/httpAsyncStorage.server.ts`, which is wired to Express response close events.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/v3/runOpsMigration/shardHandles.server.test.tsapps/webapp/app/v3/runOpsMigration/waitpointTokenResolve.server.test.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.tsapps/webapp/app/v3/runEngineHandlersShared.server.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.tsapps/webapp/app/runEngine/concerns/resolveWaitpointThroughReadThrough.server.tsapps/webapp/app/runEngine/concerns/idempotencyKeys.server.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.test.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.test.tsapps/webapp/app/presenters/v3/ApiBatchResultsPresenter.server.ts
We use vitest exclusively. **Never mock anything** - use testcontainers instead.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/app/v3/runOpsMigration/shardHandles.server.test.tsapps/webapp/test/apiBatchResultsPresenter.dedicatedSeam.test.tsapps/webapp/app/v3/runOpsMigration/waitpointTokenResolve.server.test.tsapps/webapp/test/resolveWaitpointThroughReadThrough.readthrough.test.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.test.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.test.ts
Test files must not import `app/env.server.ts`; pass configuration as options instead.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/v3/runOpsMigration/shardHandles.server.test.tsapps/webapp/test/apiBatchResultsPresenter.dedicatedSeam.test.tsapps/webapp/app/v3/runOpsMigration/waitpointTokenResolve.server.test.tsapps/webapp/test/resolveWaitpointThroughReadThrough.readthrough.test.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.test.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.test.ts
For dashboard changes, visually verify the running Remix app with Chrome DevTools MCP, using snapshots, screenshots, interaction, and console-message checks as appropriate.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/v3/runOpsMigration/shardHandles.server.test.tsapps/webapp/app/v3/runOpsMigration/waitpointTokenResolve.server.test.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.tsapps/webapp/app/v3/runEngineHandlersShared.server.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.tsapps/webapp/app/runEngine/concerns/resolveWaitpointThroughReadThrough.server.tsapps/webapp/app/runEngine/concerns/idempotencyKeys.server.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.test.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.test.tsapps/webapp/app/presenters/v3/ApiBatchResultsPresenter.server.ts
**Prefer static imports over dynamic imports.** Only use dynamic `import()` when:
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/app/v3/runOpsMigration/shardHandles.server.test.tsinternal-packages/run-store/src/PostgresRunStore.tsapps/webapp/test/apiBatchResultsPresenter.dedicatedSeam.test.tsapps/webapp/app/v3/runOpsMigration/waitpointTokenResolve.server.test.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.tsapps/webapp/test/resolveWaitpointThroughReadThrough.readthrough.test.tsapps/webapp/app/v3/runEngineHandlersShared.server.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.tsapps/webapp/app/runEngine/concerns/resolveWaitpointThroughReadThrough.server.tsapps/webapp/app/runEngine/concerns/idempotencyKeys.server.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.test.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.test.tsapps/webapp/app/presenters/v3/ApiBatchResultsPresenter.server.ts
Add crumbs as you write code — not just when debugging. Mark lines with
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/app/v3/runOpsMigration/shardHandles.server.test.tsinternal-packages/run-store/src/PostgresRunStore.tsapps/webapp/test/apiBatchResultsPresenter.dedicatedSeam.test.tsapps/webapp/app/v3/runOpsMigration/waitpointTokenResolve.server.test.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.tsapps/webapp/test/resolveWaitpointThroughReadThrough.readthrough.test.tsapps/webapp/app/v3/runEngineHandlersShared.server.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.tsapps/webapp/app/runEngine/concerns/resolveWaitpointThroughReadThrough.server.tsapps/webapp/app/runEngine/concerns/idempotencyKeys.server.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.test.tsapps/webapp/app/v3/runOpsMigration/track1-baseline.jsonapps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.test.tsapps/webapp/app/presenters/v3/ApiBatchResultsPresenter.server.ts
Use zod for validation in packages/core and apps/webapp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/app/v3/runOpsMigration/shardHandles.server.test.tsapps/webapp/test/apiBatchResultsPresenter.dedicatedSeam.test.tsapps/webapp/app/v3/runOpsMigration/waitpointTokenResolve.server.test.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.tsapps/webapp/test/resolveWaitpointThroughReadThrough.readthrough.test.tsapps/webapp/app/v3/runEngineHandlersShared.server.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.tsapps/webapp/app/runEngine/concerns/resolveWaitpointThroughReadThrough.server.tsapps/webapp/app/runEngine/concerns/idempotencyKeys.server.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.test.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.test.tsapps/webapp/app/presenters/v3/ApiBatchResultsPresenter.server.ts
Do not import `env.server.ts` directly or indirectly into test files; instead pass environment-dependent values through options/parameters to make code testable
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
apps/webapp/app/v3/runOpsMigration/shardHandles.server.test.tsapps/webapp/test/apiBatchResultsPresenter.dedicatedSeam.test.tsapps/webapp/app/v3/runOpsMigration/waitpointTokenResolve.server.test.tsapps/webapp/test/resolveWaitpointThroughReadThrough.readthrough.test.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.test.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.test.ts
Access environment variables through the `env` export of `env.server.ts` instead of directly accessing `process.env`
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
apps/webapp/app/v3/runOpsMigration/shardHandles.server.test.tsapps/webapp/test/apiBatchResultsPresenter.dedicatedSeam.test.tsapps/webapp/app/v3/runOpsMigration/waitpointTokenResolve.server.test.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.tsapps/webapp/test/resolveWaitpointThroughReadThrough.readthrough.test.tsapps/webapp/app/v3/runEngineHandlersShared.server.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.tsapps/webapp/app/runEngine/concerns/resolveWaitpointThroughReadThrough.server.tsapps/webapp/app/runEngine/concerns/idempotencyKeys.server.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.test.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.test.tsapps/webapp/app/presenters/v3/ApiBatchResultsPresenter.server.ts
Use vitest for all tests in the Trigger.dev repository
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/app/v3/runOpsMigration/shardHandles.server.test.tsapps/webapp/test/apiBatchResultsPresenter.dedicatedSeam.test.tsapps/webapp/app/v3/runOpsMigration/waitpointTokenResolve.server.test.tsapps/webapp/test/resolveWaitpointThroughReadThrough.readthrough.test.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.test.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.test.ts
Use function declarations instead of default exports
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/app/v3/runOpsMigration/shardHandles.server.test.tsinternal-packages/run-store/src/PostgresRunStore.tsapps/webapp/test/apiBatchResultsPresenter.dedicatedSeam.test.tsapps/webapp/app/v3/runOpsMigration/waitpointTokenResolve.server.test.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.tsapps/webapp/test/resolveWaitpointThroughReadThrough.readthrough.test.tsapps/webapp/app/v3/runEngineHandlersShared.server.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.tsapps/webapp/app/runEngine/concerns/resolveWaitpointThroughReadThrough.server.tsapps/webapp/app/runEngine/concerns/idempotencyKeys.server.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.test.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.test.tsapps/webapp/app/presenters/v3/ApiBatchResultsPresenter.server.ts
Use types over interfaces for TypeScript
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/app/v3/runOpsMigration/shardHandles.server.test.tsinternal-packages/run-store/src/PostgresRunStore.tsapps/webapp/test/apiBatchResultsPresenter.dedicatedSeam.test.tsapps/webapp/app/v3/runOpsMigration/waitpointTokenResolve.server.test.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.tsapps/webapp/test/resolveWaitpointThroughReadThrough.readthrough.test.tsapps/webapp/app/v3/runEngineHandlersShared.server.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.tsapps/webapp/app/runEngine/concerns/resolveWaitpointThroughReadThrough.server.tsapps/webapp/app/runEngine/concerns/idempotencyKeys.server.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.test.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.test.tsapps/webapp/app/presenters/v3/ApiBatchResultsPresenter.server.ts
When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
Files:
apps/webapp/app/v3/runOpsMigration/shardHandles.server.test.tsinternal-packages/run-store/src/PostgresRunStore.tsapps/webapp/test/apiBatchResultsPresenter.dedicatedSeam.test.tsapps/webapp/app/v3/runOpsMigration/waitpointTokenResolve.server.test.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.tsapps/webapp/test/resolveWaitpointThroughReadThrough.readthrough.test.tsapps/webapp/app/v3/runEngineHandlersShared.server.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.tsapps/webapp/app/runEngine/concerns/resolveWaitpointThroughReadThrough.server.tsapps/webapp/app/runEngine/concerns/idempotencyKeys.server.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.test.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.test.tsapps/webapp/app/presenters/v3/ApiBatchResultsPresenter.server.ts
🧠 Learnings (3)
📚 Learning: 2026-06-16T09:19:47.637Z
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 3960
File: apps/webapp/test/prismaInfrastructureErrorCapture.test.ts:0-0
Timestamp: 2026-06-16T09:19:47.637Z
Learning: In this repo’s Vitest setup, `vitest.config.ts` uses `globals: true`, so identifiers like `vi`, `describe`, `it`, and `expect` are available as globals in Vitest test files. During code review, do not flag missing `vi`/`describe`/`it`/`expect` imports as a runtime error or correctness issue when they’re used in `*.test.ts/tsx` or `*.spec.ts/tsx` files. Explicit imports are still preferred for consistency, but they’re not required for runtime behavior.
Applied to files:
apps/webapp/app/v3/runOpsMigration/waitpointTokenResolve.server.test.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.test.ts
📚 Learning: 2026-06-04T18:16:35.386Z
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 3836
File: apps/supervisor/src/backpressure/backpressureMonitor.ts:3-5
Timestamp: 2026-06-04T18:16:35.386Z
Learning: When reviewing TypeScript in this repo, apply the rule “prefer type aliases over interfaces” only to data/object shapes and union/intersection type modeling. If an interface is being used as a behavioral contract for collaborators to implement (e.g., method-shape interfaces that define required behavior, such as `BackpressureLogger` / `BackpressureSignalSource` in `apps/supervisor/src/backpressure/backpressureMonitor.ts`), keep it as an `interface` and do not flag it as a type-alias-vs-interface violation.
Applied to files:
apps/webapp/app/v3/runOpsMigration/shardHandles.server.ts
📚 Learning: 2026-05-12T21:04:05.815Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3542
File: apps/webapp/app/components/sessions/v1/SessionStatus.tsx:1-3
Timestamp: 2026-05-12T21:04:05.815Z
Learning: In this Remix + TypeScript codebase, do not flag a server/client boundary violation when a file imports only types from a module matching `*.server`.
Specifically, it’s safe to import types using `import type { Foo } from "*.server"` or `import { type Foo } from "*.server"` because TypeScript erases type-only imports at compile time and they emit no JavaScript, so they won’t cross the Remix server/client bundle boundary.
Only raise the boundary concern for value imports (e.g., `import { Foo }` without `type`, or `import Foo`), since those produce JavaScript output.
Applied to files:
apps/webapp/app/v3/runOpsMigration/shardHandles.server.ts
🔇 Additional comments (2)
apps/webapp/app/runEngine/concerns/idempotencyKeys.server.ts (1)
1-1: LGTM!Also applies to: 16-19, 36-48, 189-191, 654-662
apps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.ts (1)
4-4: LGTM!Also applies to: 13-39, 86-129
Building it at module scope dereferenced the db.server run-ops handles at import time. This module is imported by triggerTask, so any test that mocks `~/db.server` without those two exports failed to collect — runEngineBatchTriggerStoreRouting and routesBatchGetReplicaLag both did. Before this change those handles were only read inside the two methods that use them. Restore that: resolve the map on first use and memoise it, since the trigger path is the hottest in the system. No test behaviour changes; this is the inert-by-default property the change was supposed to have in the first place.
d-cs
force-pushed
the
feat/gen2-readthrough-idempotency-tri-13431
branch
from
August 25, 2026 17:05
ad425cc to
8455b57
Compare
@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: |
… the gen-1 read Raised in review. When the shard map was non-empty but lacked the id's shard key, the member fell into the gen-1 group. The new replica is a different database so it missed, and a dedicated-family id never reaches the legacy probe, so the member vanished from the results with no error — the exact silent drop this change set out to remove. Report it and drop it, matching hydrateRunsAcrossSeam. The gen-2 tests now use the N-shard fixture that arrived with main, so the shard is a real separate database and a member seeded there is genuinely absent from the gen-1 store. That removes the handle-shadowing proxy the earlier version of the test needed to discriminate.
…uard Replacing the ownerEngine try/catch with resolveShard removed the only caller of the seam's `logger.warn`. That branch was already unreachable: classification has been total since the classifier stopped throwing. The dep now asks for `error` alone rather than carrying a hook nothing calls. The guard baseline is keyed by line number, so the three-way partition in ApiBatchResultsPresenter shifted four of its five entries. Still five, all reads, in the one file, none new in kind.
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.
Gives read-through and idempotency their gen-2 shard arms, so an id that names its own shard is read there and nowhere else.
Stacked on #4764 (
feat/run-ops-shards-tri-13429), which suppliesresolveShard,runOpsShardHandlesand the keyed router. Review that one first.TRI-13431
What changes
Read-through routes by
resolveShard, not by the binary residency classifier. A gen-2 id reads its own shard's replica once and probes no other store. A gen-1 v1 id still reads new only.Callers now declare
idKind. A cuid gives no way to tell a run id from a waitpoint id, and the two must route differently:There is no default, because a default would pick one of those arms silently. The field
runIdis renamed toid, since it carried both kinds already.ReadThroughResultcarriesfound.sourceis an open-ended union once shards exist, so a consumer testing found-ness by listing the hit sources reads a gen-2 hit as a miss. One consumer did exactly that. Discriminating onfoundmakes that class of bug a compile error rather than something a reviewer has to spot.Idempotency resolves its client through one shard-keyed map. Both call sites go through
clientForShardKey, so they cannot disagree about which store owns an id. An absent key takes an explicit logged branch to the fallback, not a silent legacy default. Theclassifyseam is retyped to return aShardKey:Residency("NEW") and the reserved shard keys ("new") differ only by case, andShardKeycollapses tostring, so the compiler would not have caught feeding one into the other.The dead
isMigratedbranch is deleted. Nothing implemented it, and the one production comment recorded that omitting it was deliberate.PostgresRunStore._residencywidens toShardKey. Still unused; the store stays unaware of its siblings.Two behaviour fixes found while doing the above
An unconfigured shard key logs and returns not-found instead of throwing. The waitpoint route takes the id from a URL parameter, and any base32hex core plus
[a-z0-9]plus"2"parses as gen-2. The route turns a throw into a 500, so throwing here would let any authenticated client generate 500s and error logs by guessing shard chars, of which there are 36. An error-logged not-found is neither silent nor a misroute. Throwing stays correct on the router path, where ids are minted rather than received.The two cross-seam batch hydration sites were gen-2 blind.
hydrateRunsAcrossSeamandApiBatchResultsPresenterclassified with the binaryownerEngine, so a gen-2 run id joined the gen-1newgroup, missed there, and — classifying dedicated-family — never reached the legacy probe either. The id was dropped from a bulk-action page and from batch results with no error. Both now partition ids by shard key and read each configured shard once.Also: a gen-2 waitpoint that missed its shard replica fell back to the gen-1 new writer, a different database, silently disabling read-your-writes for the freshly minted token that fallback exists to serve. It now falls back to its own shard's writer.
Merge safety
Inert while
RUN_OPS_SHARDSis unset: the shard maps are empty, so every gen-2 arm is unreachable, and gen-2 minting is not live yet.The one live change is the gen-1 run arm, and it removes work rather than adding it.
RoutingRunStore.findRunnever forwards the caller's client object — it routes by id and reads only the client's presence and replica brand — soreadRunForEvent's "new" closure already resolved a legacy-classified run id to the legacy store. The arm removes a duplicated read of the legacy replica. A test pins this, because a future caller passing a raw client and a run id would lose the pre-cutover 27-char case, which is new-resident but classifies legacy.Testing
14 tests added, testcontainers throughout, no mocks. 22 affected test files pass; typecheck, lint, format and knip are clean.
Both arms were verified by neutralising them and confirming the new tests fail. The batch-results test needed rewriting after that check: the first version passed with the fix neutralised, because it used one container as both the gen-1 new client and the shard replica, so it was not testing what it claimed.
Note for review: run testcontainer suites in small batches. Sixteen at once starves Docker and everything times out at 60 seconds.
The run-ops legacy-guard baseline is refreshed in its own commit. The baseline is keyed by line number, so partitioning the batch-results read shifted four pre-existing entries and added one. Baselined violations in that file go from four to five, all reads; the new one is the shard read beside two gen-1 reads already there.
No changeset and no
.server-changesentry: a user notices nothing while the flag is unset.