fix(webapp,run-store): make run-ops sharding failures visible and unroutable ids a 404 - #4868
Draft
d-cs wants to merge 2 commits into
Draft
fix(webapp,run-store): make run-ops sharding failures visible and unroutable ids a 404#4868d-cs wants to merge 2 commits into
d-cs wants to merge 2 commits into
Conversation
|
Contributor
Observability mapAs of 20/100 over 454 measured of 472 entry points (base 20, no change) What this PR changed FIX FIRST
AUDIT 3 of 50 sensitive mutations record an actor. 47 without one. What the score is made ofThe score and findings here are report-only and never gate the merge. Separately, a required test suite keeps this tool's symbol and route lists in sync with the code they name, and can fail a pull request that renames or removes a symbol they reference, or that adds the first route with a segment they anticipate. Each failure names the list to edit. The rules and their reasons: internal-packages/observability-map/README.md. |
Contributor
WalkthroughThe changes classify unroutable waitpoint IDs as 404 responses. Replication now emits typed publication configuration errors and records per-source metrics. Mint-shard parsing reports invalid stored configurations while retaining gen-1 fallback. Read-through operations emit serving-source metrics. Routing stores record per-shard routed operations without counting boot-time initialization. Tests cover each behavior. Merge Risk: 🔵 Low · up to This PR is mergeable with explicit owner follow-up: one shard-set parsing path can still silently fall back without reporting malformed stored values, and several newly added diagnostic paths lack required trace markers. The resulting risk is bounded to observability and error diagnosis. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description gives a detailed and relevant summary of the changes and design, but it does not follow the repository template. It omits the issue-closing line, checklist, explicit Testing section with test steps, Changelog section, and Screenshots section.
✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
apps/webapp/test/waitpointTokenUnroutableId.test.ts (2)
16-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftRemove the module mocks and dynamic import.
Lines 16 and 30 mock application modules. Line 61 delays module loading to support those mocks. Replace this harness with container-backed dependencies, then statically import the route module.
As per coding guidelines, tests must use Vitest without mocks, use testcontainers, and prefer static imports.
Also applies to: 61-61
Source: Coding guidelines
1-8: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePlace these tests beside their route sources.
Split this coverage into route-adjacent test files. The current file is under
apps/webapp/testwhile it covers routes underapps/webapp/app/routes.As per coding guidelines, test files go next to source files.
Source: Coding guidelines
apps/webapp/test/runsReplicationMetrics.test.ts (1)
6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPlace this unit test beside its source file.
Move
apps/webapp/test/runsReplicationMetrics.test.tstoapps/webapp/app/services/runsReplicationMetrics.server.test.ts. This test only exercisesbuildRunsReplicationSourceMetricswith an in-memory registry and does not require the app-level test directory.As per coding guidelines: “Test files go next to source files (e.g.,
MyService.ts->MyService.test.ts).”Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: d7be3ac5-9289-4b0c-b9c9-b1efdce2349f
📒 Files selected for processing (24)
apps/webapp/app/routes/api.v1.waitpoints.tokens.$waitpointFriendlyId.callback.$hash.tsapps/webapp/app/routes/api.v1.waitpoints.tokens.$waitpointFriendlyId.complete.tsapps/webapp/app/services/runsReplicationInstance.server.tsapps/webapp/app/services/runsReplicationMetrics.server.tsapps/webapp/app/services/runsReplicationService.server.tsapps/webapp/app/v3/runOpsMigration/mintShardAssignment.test.tsapps/webapp/app/v3/runOpsMigration/mintShardAssignment.tsapps/webapp/app/v3/runOpsMigration/mintShardGrace.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.test.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.tsapps/webapp/app/v3/runOpsMigration/readThroughSourceMetric.server.test.tsapps/webapp/app/v3/runOpsMigration/readThroughSourceMetric.server.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.tsapps/webapp/app/v3/runStore.server.tsapps/webapp/test/runsReplicationMetrics.test.tsapps/webapp/test/runsReplicationPublicationMisconfigured.test.tsapps/webapp/test/waitpointTokenUnroutableId.test.tsinternal-packages/replication/src/client.publicationMisconfigured.test.tsinternal-packages/replication/src/client.tsinternal-packages/replication/src/errors.tsinternal-packages/run-store/src/routingStoreMetrics.tsinternal-packages/run-store/src/runOpsStore.newMethods.test.tsinternal-packages/run-store/src/runOpsStore.shardMap.test.tsinternal-packages/run-store/src/runOpsStore.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
📜 Review details
🧰 Additional context used
📓 Path-based instructions (15)
Use Remix flat-file route conventions with dot-separated segments; for example, `api.v1.tasks.$taskId.trigger.ts` maps to `/api/v1/tasks/:taskId/trigger`.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/routes/api.v1.waitpoints.tokens.$waitpointFriendlyId.callback.$hash.tsapps/webapp/app/routes/api.v1.waitpoints.tokens.$waitpointFriendlyId.complete.ts
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/mintShardAssignment.test.tsapps/webapp/app/v3/runOpsMigration/mintShardGrace.tsapps/webapp/app/v3/runOpsMigration/mintShardAssignment.tsapps/webapp/app/v3/runStore.server.tsapps/webapp/app/v3/runOpsMigration/readThroughSourceMetric.server.test.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.tsapps/webapp/app/v3/runOpsMigration/readThroughSourceMetric.server.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.test.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.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/mintShardAssignment.test.tsapps/webapp/app/routes/api.v1.waitpoints.tokens.$waitpointFriendlyId.callback.$hash.tsapps/webapp/app/services/runsReplicationInstance.server.tsapps/webapp/app/services/runsReplicationService.server.tsapps/webapp/app/v3/runOpsMigration/mintShardGrace.tsapps/webapp/app/v3/runOpsMigration/mintShardAssignment.tsapps/webapp/app/services/runsReplicationMetrics.server.tsapps/webapp/app/v3/runStore.server.tsapps/webapp/app/routes/api.v1.waitpoints.tokens.$waitpointFriendlyId.complete.tsapps/webapp/app/v3/runOpsMigration/readThroughSourceMetric.server.test.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.tsapps/webapp/app/v3/runOpsMigration/readThroughSourceMetric.server.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.test.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.ts
We use vitest exclusively. **Never mock anything** - use testcontainers instead.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/test/runsReplicationMetrics.test.tsapps/webapp/test/waitpointTokenUnroutableId.test.tsinternal-packages/run-store/src/runOpsStore.newMethods.test.tsapps/webapp/app/v3/runOpsMigration/mintShardAssignment.test.tsapps/webapp/test/runsReplicationPublicationMisconfigured.test.tsinternal-packages/replication/src/client.publicationMisconfigured.test.tsapps/webapp/app/v3/runOpsMigration/readThroughSourceMetric.server.test.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.test.tsinternal-packages/run-store/src/runOpsStore.shardMap.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/test/runsReplicationMetrics.test.tsapps/webapp/test/waitpointTokenUnroutableId.test.tsapps/webapp/app/v3/runOpsMigration/mintShardAssignment.test.tsapps/webapp/test/runsReplicationPublicationMisconfigured.test.tsapps/webapp/app/v3/runOpsMigration/readThroughSourceMetric.server.test.tsapps/webapp/app/v3/runOpsMigration/readThrough.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/mintShardAssignment.test.tsapps/webapp/app/routes/api.v1.waitpoints.tokens.$waitpointFriendlyId.callback.$hash.tsapps/webapp/app/services/runsReplicationInstance.server.tsapps/webapp/app/services/runsReplicationService.server.tsapps/webapp/app/v3/runOpsMigration/mintShardGrace.tsapps/webapp/app/v3/runOpsMigration/mintShardAssignment.tsapps/webapp/app/services/runsReplicationMetrics.server.tsapps/webapp/app/v3/runStore.server.tsapps/webapp/app/routes/api.v1.waitpoints.tokens.$waitpointFriendlyId.complete.tsapps/webapp/app/v3/runOpsMigration/readThroughSourceMetric.server.test.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.tsapps/webapp/app/v3/runOpsMigration/readThroughSourceMetric.server.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.test.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.ts
**Prefer static imports over dynamic imports.** Only use dynamic `import()` when:
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/test/runsReplicationMetrics.test.tsapps/webapp/test/waitpointTokenUnroutableId.test.tsinternal-packages/run-store/src/runOpsStore.newMethods.test.tsinternal-packages/replication/src/errors.tsapps/webapp/app/v3/runOpsMigration/mintShardAssignment.test.tsapps/webapp/app/routes/api.v1.waitpoints.tokens.$waitpointFriendlyId.callback.$hash.tsapps/webapp/app/services/runsReplicationInstance.server.tsapps/webapp/app/services/runsReplicationService.server.tsapps/webapp/test/runsReplicationPublicationMisconfigured.test.tsinternal-packages/run-store/src/routingStoreMetrics.tsapps/webapp/app/v3/runOpsMigration/mintShardGrace.tsapps/webapp/app/v3/runOpsMigration/mintShardAssignment.tsapps/webapp/app/services/runsReplicationMetrics.server.tsinternal-packages/replication/src/client.tsapps/webapp/app/v3/runStore.server.tsinternal-packages/replication/src/client.publicationMisconfigured.test.tsapps/webapp/app/routes/api.v1.waitpoints.tokens.$waitpointFriendlyId.complete.tsapps/webapp/app/v3/runOpsMigration/readThroughSourceMetric.server.test.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.tsinternal-packages/run-store/src/runOpsStore.tsapps/webapp/app/v3/runOpsMigration/readThroughSourceMetric.server.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.test.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.tsinternal-packages/run-store/src/runOpsStore.shardMap.test.ts
Add crumbs as you write code — not just when debugging. Mark lines with
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/test/runsReplicationMetrics.test.tsapps/webapp/test/waitpointTokenUnroutableId.test.tsinternal-packages/run-store/src/runOpsStore.newMethods.test.tsinternal-packages/replication/src/errors.tsapps/webapp/app/v3/runOpsMigration/mintShardAssignment.test.tsapps/webapp/app/routes/api.v1.waitpoints.tokens.$waitpointFriendlyId.callback.$hash.tsapps/webapp/app/services/runsReplicationInstance.server.tsapps/webapp/app/services/runsReplicationService.server.tsapps/webapp/test/runsReplicationPublicationMisconfigured.test.tsinternal-packages/run-store/src/routingStoreMetrics.tsapps/webapp/app/v3/runOpsMigration/mintShardGrace.tsapps/webapp/app/v3/runOpsMigration/mintShardAssignment.tsapps/webapp/app/services/runsReplicationMetrics.server.tsinternal-packages/replication/src/client.tsapps/webapp/app/v3/runStore.server.tsinternal-packages/replication/src/client.publicationMisconfigured.test.tsapps/webapp/app/routes/api.v1.waitpoints.tokens.$waitpointFriendlyId.complete.tsapps/webapp/app/v3/runOpsMigration/readThroughSourceMetric.server.test.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.tsinternal-packages/run-store/src/runOpsStore.tsapps/webapp/app/v3/runOpsMigration/readThroughSourceMetric.server.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.test.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.tsinternal-packages/run-store/src/runOpsStore.shardMap.test.ts
Use zod for validation in packages/core and apps/webapp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/runsReplicationMetrics.test.tsapps/webapp/test/waitpointTokenUnroutableId.test.tsapps/webapp/app/v3/runOpsMigration/mintShardAssignment.test.tsapps/webapp/app/routes/api.v1.waitpoints.tokens.$waitpointFriendlyId.callback.$hash.tsapps/webapp/app/services/runsReplicationInstance.server.tsapps/webapp/app/services/runsReplicationService.server.tsapps/webapp/test/runsReplicationPublicationMisconfigured.test.tsapps/webapp/app/v3/runOpsMigration/mintShardGrace.tsapps/webapp/app/v3/runOpsMigration/mintShardAssignment.tsapps/webapp/app/services/runsReplicationMetrics.server.tsapps/webapp/app/v3/runStore.server.tsapps/webapp/app/routes/api.v1.waitpoints.tokens.$waitpointFriendlyId.complete.tsapps/webapp/app/v3/runOpsMigration/readThroughSourceMetric.server.test.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.tsapps/webapp/app/v3/runOpsMigration/readThroughSourceMetric.server.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.test.tsapps/webapp/app/v3/runOpsMigration/readThrough.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/test/runsReplicationMetrics.test.tsapps/webapp/test/waitpointTokenUnroutableId.test.tsapps/webapp/app/v3/runOpsMigration/mintShardAssignment.test.tsapps/webapp/test/runsReplicationPublicationMisconfigured.test.tsapps/webapp/app/v3/runOpsMigration/readThroughSourceMetric.server.test.tsapps/webapp/app/v3/runOpsMigration/readThrough.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/test/runsReplicationMetrics.test.tsapps/webapp/test/waitpointTokenUnroutableId.test.tsapps/webapp/app/v3/runOpsMigration/mintShardAssignment.test.tsapps/webapp/app/routes/api.v1.waitpoints.tokens.$waitpointFriendlyId.callback.$hash.tsapps/webapp/app/services/runsReplicationInstance.server.tsapps/webapp/app/services/runsReplicationService.server.tsapps/webapp/test/runsReplicationPublicationMisconfigured.test.tsapps/webapp/app/v3/runOpsMigration/mintShardGrace.tsapps/webapp/app/v3/runOpsMigration/mintShardAssignment.tsapps/webapp/app/services/runsReplicationMetrics.server.tsapps/webapp/app/v3/runStore.server.tsapps/webapp/app/routes/api.v1.waitpoints.tokens.$waitpointFriendlyId.complete.tsapps/webapp/app/v3/runOpsMigration/readThroughSourceMetric.server.test.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.tsapps/webapp/app/v3/runOpsMigration/readThroughSourceMetric.server.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.test.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.ts
Use vitest for all tests in the Trigger.dev repository
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/runsReplicationMetrics.test.tsapps/webapp/test/waitpointTokenUnroutableId.test.tsinternal-packages/run-store/src/runOpsStore.newMethods.test.tsapps/webapp/app/v3/runOpsMigration/mintShardAssignment.test.tsapps/webapp/test/runsReplicationPublicationMisconfigured.test.tsinternal-packages/replication/src/client.publicationMisconfigured.test.tsapps/webapp/app/v3/runOpsMigration/readThroughSourceMetric.server.test.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.test.tsinternal-packages/run-store/src/runOpsStore.shardMap.test.ts
Use function declarations instead of default exports
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/runsReplicationMetrics.test.tsapps/webapp/test/waitpointTokenUnroutableId.test.tsinternal-packages/run-store/src/runOpsStore.newMethods.test.tsinternal-packages/replication/src/errors.tsapps/webapp/app/v3/runOpsMigration/mintShardAssignment.test.tsapps/webapp/app/routes/api.v1.waitpoints.tokens.$waitpointFriendlyId.callback.$hash.tsapps/webapp/app/services/runsReplicationInstance.server.tsapps/webapp/app/services/runsReplicationService.server.tsapps/webapp/test/runsReplicationPublicationMisconfigured.test.tsinternal-packages/run-store/src/routingStoreMetrics.tsapps/webapp/app/v3/runOpsMigration/mintShardGrace.tsapps/webapp/app/v3/runOpsMigration/mintShardAssignment.tsapps/webapp/app/services/runsReplicationMetrics.server.tsinternal-packages/replication/src/client.tsapps/webapp/app/v3/runStore.server.tsinternal-packages/replication/src/client.publicationMisconfigured.test.tsapps/webapp/app/routes/api.v1.waitpoints.tokens.$waitpointFriendlyId.complete.tsapps/webapp/app/v3/runOpsMigration/readThroughSourceMetric.server.test.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.tsinternal-packages/run-store/src/runOpsStore.tsapps/webapp/app/v3/runOpsMigration/readThroughSourceMetric.server.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.test.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.tsinternal-packages/run-store/src/runOpsStore.shardMap.test.ts
Use types over interfaces for TypeScript
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/runsReplicationMetrics.test.tsapps/webapp/test/waitpointTokenUnroutableId.test.tsinternal-packages/run-store/src/runOpsStore.newMethods.test.tsinternal-packages/replication/src/errors.tsapps/webapp/app/v3/runOpsMigration/mintShardAssignment.test.tsapps/webapp/app/routes/api.v1.waitpoints.tokens.$waitpointFriendlyId.callback.$hash.tsapps/webapp/app/services/runsReplicationInstance.server.tsapps/webapp/app/services/runsReplicationService.server.tsapps/webapp/test/runsReplicationPublicationMisconfigured.test.tsinternal-packages/run-store/src/routingStoreMetrics.tsapps/webapp/app/v3/runOpsMigration/mintShardGrace.tsapps/webapp/app/v3/runOpsMigration/mintShardAssignment.tsapps/webapp/app/services/runsReplicationMetrics.server.tsinternal-packages/replication/src/client.tsapps/webapp/app/v3/runStore.server.tsinternal-packages/replication/src/client.publicationMisconfigured.test.tsapps/webapp/app/routes/api.v1.waitpoints.tokens.$waitpointFriendlyId.complete.tsapps/webapp/app/v3/runOpsMigration/readThroughSourceMetric.server.test.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.tsinternal-packages/run-store/src/runOpsStore.tsapps/webapp/app/v3/runOpsMigration/readThroughSourceMetric.server.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.test.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.tsinternal-packages/run-store/src/runOpsStore.shardMap.test.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/test/runsReplicationMetrics.test.tsapps/webapp/test/waitpointTokenUnroutableId.test.tsinternal-packages/run-store/src/runOpsStore.newMethods.test.tsinternal-packages/replication/src/errors.tsapps/webapp/app/v3/runOpsMigration/mintShardAssignment.test.tsapps/webapp/app/routes/api.v1.waitpoints.tokens.$waitpointFriendlyId.callback.$hash.tsapps/webapp/app/services/runsReplicationInstance.server.tsapps/webapp/app/services/runsReplicationService.server.tsapps/webapp/test/runsReplicationPublicationMisconfigured.test.tsinternal-packages/run-store/src/routingStoreMetrics.tsapps/webapp/app/v3/runOpsMigration/mintShardGrace.tsapps/webapp/app/v3/runOpsMigration/mintShardAssignment.tsapps/webapp/app/services/runsReplicationMetrics.server.tsinternal-packages/replication/src/client.tsapps/webapp/app/v3/runStore.server.tsinternal-packages/replication/src/client.publicationMisconfigured.test.tsapps/webapp/app/routes/api.v1.waitpoints.tokens.$waitpointFriendlyId.complete.tsapps/webapp/app/v3/runOpsMigration/readThroughSourceMetric.server.test.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.tsinternal-packages/run-store/src/runOpsStore.tsapps/webapp/app/v3/runOpsMigration/readThroughSourceMetric.server.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.test.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.tsinternal-packages/run-store/src/runOpsStore.shardMap.test.ts
🧠 Learnings (5)
📚 Learning: 2026-08-24T12:38:01.585Z
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 4755
File: apps/webapp/app/v3/runOpsMigration/mintShardAssignment.test.ts:1-9
Timestamp: 2026-08-24T12:38:01.585Z
Learning: Keep mint-shard assignment logic in the pure `mintShardAssignment.ts` module so it remains independent of `env.server.ts`, and test that logic in `mintShardAssignment.test.ts` without importing environment-bound modules. Keep runtime and environment integration in the `runOpsMintShard.server.ts` wrapper; do not reintroduce module-level boot warnings tied to the removed `RUN_OPS_MINT_SHARDS` setting.
Applied to files:
apps/webapp/app/v3/runOpsMigration/mintShardAssignment.test.tsapps/webapp/app/v3/runOpsMigration/mintShardAssignment.ts
📚 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/test/runsReplicationPublicationMisconfigured.test.tsinternal-packages/replication/src/client.publicationMisconfigured.test.tsapps/webapp/app/v3/runOpsMigration/readThroughSourceMetric.server.test.ts
📚 Learning: 2026-05-28T20:02:10.647Z
Learnt from: myftija
Repo: triggerdotdev/trigger.dev PR: 3772
File: apps/webapp/test/findOrCreateBackgroundWorker.test.ts:1-1
Timestamp: 2026-05-28T20:02:10.647Z
Learning: In the triggerdotdev/trigger.dev monorepo, for the `apps/webapp` package use the established convention of storing Vitest tests (unit, integration, and e2e) under `apps/webapp/test/` rather than colocating them next to source files. Do not flag files located in `apps/webapp/test/` as violating any rule that says to colocate tests with source.
Applied to files:
apps/webapp/test/runsReplicationPublicationMisconfigured.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/services/runsReplicationMetrics.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/readThroughSourceMetric.server.ts
🔇 Additional comments (12)
apps/webapp/app/v3/runOpsMigration/mintShardGrace.ts (1)
70-71: LGTM!Also applies to: 73-90, 99-100, 108-108, 112-112
apps/webapp/app/v3/runOpsMigration/mintShardAssignment.ts (1)
11-11: LGTM!Also applies to: 170-172, 187-187
apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts (1)
69-89: LGTM!Also applies to: 118-118
apps/webapp/app/v3/runOpsMigration/mintShardAssignment.test.ts (1)
317-330: LGTM!Also applies to: 332-339, 341-348
internal-packages/replication/src/errors.ts (1)
6-23: LGTM!apps/webapp/app/services/runsReplicationMetrics.server.ts (1)
1-38: LGTM!apps/webapp/app/services/runsReplicationInstance.server.ts (1)
12-12: LGTM!Also applies to: 253-255
internal-packages/run-store/src/routingStoreMetrics.ts (1)
7-23: LGTM!apps/webapp/app/v3/runStore.server.ts (1)
138-159: LGTM!internal-packages/run-store/src/runOpsStore.ts (1)
173-178: LGTM!Also applies to: 210-210
internal-packages/run-store/src/runOpsStore.newMethods.test.ts (1)
232-232: LGTM!internal-packages/run-store/src/runOpsStore.shardMap.test.ts (1)
352-355: LGTM!Also applies to: 371-379, 399-402, 404-420, 592-592, 708-716, 837-837, 957-961, 979-983
Comment on lines
+612
to
+618
| this.events.emit( | ||
| "error", | ||
| new PublicationMisconfiguredError(validationError, { | ||
| publicationName: this.options.publicationName, | ||
| table: this.options.table, | ||
| }) | ||
| ); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add the required diagnostic trace markers to the new paths.
These production and regression paths add new failure classification and metric/reporting behavior but do not include the repository-required trace markers. Add an approved marker or region at each listed site so failures remain attributable during debugging.
📍 Affects 2 files
internal-packages/replication/src/client.ts#L612-L618(this comment)apps/webapp/app/routes/api.v1.waitpoints.tokens.$waitpointFriendlyId.callback.$hash.ts#L106-L110
Source: Coding guidelines
| // the shard it routed to, so a cohort ramp could only be inferred from the databases themselves. | ||
| it("counts every routed operation against the shard it landed on", async () => { | ||
| const routed: string[] = []; | ||
| const { router } = buildNShardRouter(["a", "b"], { routed }); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Use a testcontainer-backed fixture for this new test.
buildNShardRouter creates fakeStore(...) test doubles, and this new assertion runs through them. Replace the fake stores with a testcontainer-backed fixture so the metric is verified through the production storage implementation.
As per coding guidelines, test files must use vitest exclusively and must never mock anything; use testcontainers instead.
Source: Coding guidelines
…outable ids a 404 Four failure modes from a manual pass over a multi-database run-ops setup, three of them silent: - A replication source whose publication carries no tables replicates nothing while boot succeeds and the service looks healthy. The client now raises a typed PublicationMisconfiguredError and the webapp counts it per source, so it alarms. - Completing a wait token with an id naming a database the deployment has no store for answered 500. Every API route builder now answers 404, matching the run routes. - An unparseable stored shard set still degrades to the previous id format, but the parse failure is now reported instead of vanishing, on both the read and write paths. - Per-shard observability, so a gradual rollout can be watched: runops_shard_routed_total and runops_read_through_source_total.
d-cs
force-pushed
the
fix/runops-sharding-qa-followups
branch
from
September 1, 2026 16:28
ad9e703 to
ff518b5
Compare
… routes The API route builders cover the routes built on them, but routes that handle their own errors never reach that mapping. Four bare API routes answered 500 for an id naming a database the deployment has no store for, and ten dashboard and resource routes threw straight through to an error page. Each now takes the not-found path it already has for a run that does not exist: the API routes return 404, the dashboard routes redirect or 404 as they already did. A shared helper keeps that uniform, and every one logs first, so a shard key dropped from a config meant to be append-only still alarms rather than reading as an absent run.
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.
Summary
Four failure modes found while exercising a multi-database run-ops setup by hand. Three were silent: the system kept serving requests while quietly doing the wrong thing.
Design
Publication misconfiguration. The replication client already detected an empty publication and logged it on every reconnect, but a log line is not alertable. It now raises a typed
PublicationMisconfiguredError, counted per source asruns_replication_publication_misconfigured_total. No boot-time check was added: it would need a live query per source before the service exists, so a transient blip would fail closed and refuse to boot, which is worse than the problem being solved. The counter increments on every retry, so a nonzero rate is the alarm and it clears when the publication is repaired.Repairing the publication restores replication going forward but does not backfill rows written while it was empty, so catching this early is the point.
Unroutable IDs. All seven API route builders now map this to a 404, including the two worker builders that previously fell through to a 500. Both new call sites log before answering, so a shard key dropped from a config meant to be append-only still alarms rather than turning every live token on it into a quiet not-found.
Routes that handle their own errors never reach a builder, so a sweep covers those too: four bare API routes that answered 500 (run result, run tags, reschedule, batch results) and ten dashboard and resource routes that threw straight through to an error page. Each takes the not-found path it already had for a run that does not exist, and logs first.
Shard-set parse failures. The parsing module is pure by design and its tests depend on that, so it reports through a callback and the caller logs, matching the existing operator reports.
Observability.
runops_shard_routed_total{shard}counts ID-routed store resolutions, andrunops_read_through_source_total{source,shard}records which store served a read-through. The first is deliberately a relative ramp signal rather than a request count: fan-outs and probe hits resolve no single shard, so they are not counted, and the help text says so. Label children are cached rather than hashed per call, since both sit on hot read paths.Each fix has a test that fails if the corresponding production change is reverted, with two exceptions worth naming: the lines binding the metric recorders into the production service and read-through defaults are module-scope wiring that the tests inject around.