feat(webapp): RUN_OPS_SHARDS config, topology and N-way store wiring - #4764
feat(webapp): RUN_OPS_SHARDS config, topology and N-way store wiring#4764d-cs wants to merge 18 commits into
Conversation
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ith an injected shard resolver Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Includes the cross-field boot refinement requiring RUN_OPS_DATABASE_URL when the shard list is non-empty, since gen-1 v1 ids resolve to the new store permanently. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…actory Dedupes buildRunOpsWriterClient/buildRunOpsReplicaClient into a single buildRunOpsClient parameterized by role and the resolved pool knobs. The control-plane builders (buildWriterClient/buildReplicaClient) are a separate path and stay untouched. Every resolved value matches the former builders, so split-on deployments are byte-identical. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…get per pool Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
selectRunOpsTopology gains a shard loop and returns a keyed shard map. An aliasOf:"new" descriptor reuses the new store's clients by reference and opens no pool. Each real shard gets its own resilience budget and the new-role pool knobs merged with its per-shard overrides. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…the shard table at boot buildRunStore now produces one dedicated store per shard descriptor and the N-way router via RoutingRunStore.fromShards, keeping the two-store compat router when no shards are configured. The topology singleton logs the resolved shard table (key, address fingerprint, role) only when RUN_OPS_SHARDS is non-empty, so the unset case adds no output. The fingerprint is an address, never an identity claim. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… descriptor keys computeMintShard now intersects the active shard set with routableKeys (the RUN_OPS_SHARDS descriptor keys), so a stored key with no descriptor is never minted into and falls back to gen-1. The empty-set check runs first, so an unconfigured deployment is unchanged. Inert until the gen-2 write path wires in resolveMintShard. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…at/lint Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
WalkthroughThe change adds validated Merge Risk: 🟠 High · up to A non-empty shard configuration can be accepted while the application silently continues with the fallback topology, and current N-way routing can miss or double-count waitpoint work or mishandle connection failures. These correctness and availability risks make the PR unsafe to merge without fixes or explicit owner acceptance. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description clearly explains the RUN_OPS_SHARDS implementation, scope, ordering constraint, testing, and changelog. It omits the template checklist, issue-closing line, and screenshots section, but the core required information is complete. ✨ Finishing Touches 💡 1📝 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.
…ri-13429 # Conflicts: # packages/core/src/v3/isomorphic/friendlyId.test.ts
@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: |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/core/src/v3/isomorphic/friendlyId.ts (1)
43-47: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd required crumb markers to the new code and tests. The changed paths do not contain the required crumb instrumentation.
packages/core/src/v3/isomorphic/friendlyId.ts#L43-L47: add crumb instrumentation for shard-character validation.packages/core/src/v3/isomorphic/friendlyId.ts#L246-L343: add crumb instrumentation for waitpoint ID generation, parsing, and derivation.packages/core/src/v3/isomorphic/friendlyId.test.ts#L422-L590: add crumb instrumentation for the new test paths.Source: Coding guidelines
packages/core/src/v3/isomorphic/friendlyId.test.ts (1)
472-480: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDo not mock the system clock.
vi.useFakeTimers()replaces global time. Capture timestamps before and aftergenerateWaitpointId()and assert thatparsed.timestampis within those bounds.As per coding guidelines, “Never mock anything - use testcontainers instead.”
Proposed test change
- vi.useFakeTimers(); - try { - vi.setSystemTime(new Date("2026-08-21T12:00:00.000Z")); - const parsed = parseWaitpointId(generateWaitpointId("DATETIME")); - if (parsed.format !== "b32hexW") throw new Error("unreachable"); - expect(parsed.timestamp.toISOString()).toBe("2026-08-21T12:00:00.000Z"); - } finally { - vi.useRealTimers(); - } + const startedAt = Date.now(); + const parsed = parseWaitpointId(generateWaitpointId("DATETIME")); + const completedAt = Date.now(); + if (parsed.format !== "b32hexW") throw new Error("unreachable"); + expect(parsed.timestamp.getTime()).toBeGreaterThanOrEqual(startedAt); + expect(parsed.timestamp.getTime()).toBeLessThanOrEqual(completedAt);Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b3a91159-ae69-4aca-b3e6-566a40418e1f
📒 Files selected for processing (2)
packages/core/src/v3/isomorphic/friendlyId.test.tspackages/core/src/v3/isomorphic/friendlyId.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Build and publish previews
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
**/*.{ts,tsx}: Prefer static imports over dynamic imports. Only use dynamicimport()when:
- Circular dependencies cannot be resolved otherwise
- Code splitting is genuinely needed for performance
- The module must be loaded conditionally at runtime
Files:
packages/core/src/v3/isomorphic/friendlyId.tspackages/core/src/v3/isomorphic/friendlyId.test.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use zod for validation in packages/core and apps/webapp
Files:
packages/core/src/v3/isomorphic/friendlyId.tspackages/core/src/v3/isomorphic/friendlyId.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Files:
packages/core/src/v3/isomorphic/friendlyId.tspackages/core/src/v3/isomorphic/friendlyId.test.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
**/*.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
Do not use high-cardinality attributes in OTEL metrics such as UUIDs/IDs (envId, userId, runId, projectId, organizationId), unbounded integers (itemCount, batchSize, retryCount), timestamps (createdAt, startTime), or free-form strings (errorMessage, taskName, queueName)
When exporting OTEL metrics via OTLP to Prometheus, be aware that the exporter automatically adds unit suffixes to metric names (e.g., 'my_duration_ms' becomes 'my_duration_ms_milliseconds', 'my_counter' becomes 'my_counter_total'). Account for these transformations when writing Grafana dashboards or Prometheus queries
Files:
packages/core/src/v3/isomorphic/friendlyId.tspackages/core/src/v3/isomorphic/friendlyId.test.ts
packages/core/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (packages/core/CLAUDE.md)
Never import the root package (
@trigger.dev/core). Always use subpath imports such as@trigger.dev/core/v3,@trigger.dev/core/v3/utils,@trigger.dev/core/logger, or@trigger.dev/core/schemas
Files:
packages/core/src/v3/isomorphic/friendlyId.tspackages/core/src/v3/isomorphic/friendlyId.test.ts
packages/core/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Import subpaths only (never root).
Files:
packages/core/src/v3/isomorphic/friendlyId.tspackages/core/src/v3/isomorphic/friendlyId.test.ts
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Add crumbs as you write code — not just when debugging. Mark lines with
//@Crumbsor wrap blocks in `// `#region` `@crumbs. They stay on the
branch throughout development and are stripped byagentcrumbs strip
before merge.
Do not invent new namespaces — pick from this table or ask first.
Do not flag//@Crumbsmarkers, `#region `@crumbsblocks, or agentcrumbs
imports in reviews. These are temporary debug instrumentation stripped
before merge. Data logged in crumbs (IDs, names, values) never reaches
production.
Files:
packages/core/src/v3/isomorphic/friendlyId.tspackages/core/src/v3/isomorphic/friendlyId.test.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use vitest for all tests in the Trigger.dev repository
**/*.{test,spec}.{ts,tsx}: We use vitest exclusively. Never mock anything - use testcontainers instead.
Test files go next to source files (e.g.,MyService.ts->MyService.test.ts).
Files:
packages/core/src/v3/isomorphic/friendlyId.test.ts
🧠 Learnings (1)
📚 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:
packages/core/src/v3/isomorphic/friendlyId.test.ts
- Make probeOrder a true reverse of precedence so the merge and probe paths agree on a duplicate id, matching the RoutingRunStore invariant. - Split resolveRunOpsPoolKnobs into a pure applyPoolKnobOverrides (tested with literal defaults, no env import) plus an env-reading defaults function. - Move the pure boot-table helpers to runOpsShardTable.ts so their test does not construct the db.server Prisma topology. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ectRunOpsTopology Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@coderabbitai Addressed all review feedback in commits 20f0ee3 and 908fcb5: Fixed (4):
Declined (2), with reasoning in the thread replies:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/webapp/app/db.server.ts (3)
1085-1088: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse the validated
envexport for Prisma flags.
buildRunOpsClientreadsprocess.env.VERBOSE_PRISMA_LOGS,process.env.VERY_SLOW_QUERY_THRESHOLD_MS, andprocess.env.PRISMA_LOG_TO_STDOUT. Inapps/webapp/app/**/*.ts, read environment variables throughenvinstead. Add these fields toenv.server.tsif needed, then useenv.*here.As per coding guidelines: “Access environment variables through the
envexport fromapp/env.server.ts; never useprocess.envdirectly.”Also applies to: 1111-1112, 1128-1131
Source: Coding guidelines
1127-1134: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle
client.$connect()before logging success.
buildRunOpsClientlogs${connectedLabel}before Prisma 6.14.0 finishesengine.start(). A rejected connection is unhandled in production. Await or handle the promise before emitting the success log.
645-655: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHonor
connectTimeoutin driver-adapter pools.
pg@8.15.6usesconnectionTimeoutMillisfor both pool acquisition and new connections.@prisma/adapter-pg@6.14.0passes the existing pool through unchanged. Therefore, adapter mode ignoresconnectTimeoutand appliespoolTimeoutto both operations. Define the timeout precedence, implement it, and add a test with different timeout values.Source: MCP tools
apps/webapp/test/runOpsDbTopology.test.ts (1)
151-156: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftReplace mocked shard builders with testcontainers-backed builders.
baseBuildersusesvi.fn().mockReturnValue(...). These mocks can let topology tests pass while real Prisma client construction, connection settings, or per-shard wiring is incompatible. Use testcontainers-backed builders and assert against real clients.As per coding guidelines: “We use vitest exclusively. Never mock anything - use testcontainers instead.”
Source: Coding guidelines
♻️ Duplicate comments (1)
apps/webapp/test/runOpsPoolKnobs.test.ts (1)
2-5: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winKeep this test outside the
env.server.tsmodule graph.Line 2 statically imports
runOpsPoolKnobs.server.ts. That module readsenvinpoolKnobDefaults, so this test still indirectly importsenv.server.ts.Move
applyPoolKnobOverridesandResolvedPoolKnobsinto a dependency-free module. Move this test beside that module. Keep environment resolution in the server-only module.As per coding guidelines: “Do not import
env.server.tsdirectly or indirectly into test files” and “Test files go next to source files”.#!/bin/bash set -euo pipefail # Inspect the test import and the imported module's environment dependency. rg -n -C 2 'runOpsPoolKnobs\.server|applyPoolKnobOverrides' \ apps/webapp/test/runOpsPoolKnobs.test.ts rg -n -C 2 'env\.|env\.server' \ apps/webapp/app/v3/runOpsPoolKnobs.server.tsSource: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 38158b7e-a94d-4d27-b34e-b7acc1cdc9af
📒 Files selected for processing (7)
apps/webapp/app/db.server.tsapps/webapp/app/v3/runOpsPoolKnobs.server.tsapps/webapp/app/v3/runOpsShardTable.tsapps/webapp/app/v3/runStore.server.tsapps/webapp/test/runOpsDbTopology.test.tsapps/webapp/test/runOpsPoolKnobs.test.tsapps/webapp/test/runOpsShardBootTable.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (43)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (14, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (24, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (8, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (18, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (12, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (19, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (13, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (20, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (16, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (23, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (17, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (21, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (6, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (7, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (2, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (11, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (22, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (4, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (15, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (10, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (5, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (1, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (9, 24)
- GitHub Check: sdk-compat / Node.js 22.23 (warp-ubuntu-latest-x64-4x)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (3, 24)
- GitHub Check: sdk-compat / Node.js 24.18 (warp-ubuntu-latest-x64-4x)
- GitHub Check: sdk-compat / Node.js 26.4 (warp-ubuntu-latest-x64-4x)
- GitHub Check: e2e / 🧪 CLI v3 tests (warp-ubuntu-latest-x64-4x - npm)
- GitHub Check: sdk-compat / Deno Runtime
- GitHub Check: sdk-compat / Bun Runtime
- GitHub Check: packages / 🧪 Unit Tests: Packages (3, 3)
- GitHub Check: sdk-compat / Cloudflare Workers
- GitHub Check: e2e / 🧪 CLI v3 tests (warp-windows-latest-x64-8x - pnpm)
- GitHub Check: internal / 🧪 Unit Tests: Internal
- GitHub Check: obsmap / 🧪 Unit Tests: Observability Map
- GitHub Check: e2e / 🧪 CLI v3 tests (warp-ubuntu-latest-x64-4x - pnpm)
- GitHub Check: runops-guard / runops-guard
- GitHub Check: e2e / 🧪 CLI v3 tests (warp-windows-latest-x64-8x - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (1, 2)
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (2, 2)
- GitHub Check: packages / 🧪 Unit Tests: Packages (1, 3)
- GitHub Check: packages / 🧪 Unit Tests: Packages (2, 3)
🧰 Additional context used
📓 Path-based instructions (12)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
**/*.{ts,tsx}: Prefer static imports over dynamic imports. Only use dynamicimport()when:
- Circular dependencies cannot be resolved otherwise
- Code splitting is genuinely needed for performance
- The module must be loaded conditionally at runtime
Files:
apps/webapp/app/v3/runOpsShardTable.tsapps/webapp/test/runOpsPoolKnobs.test.tsapps/webapp/test/runOpsShardBootTable.test.tsapps/webapp/test/runOpsDbTopology.test.tsapps/webapp/app/v3/runStore.server.tsapps/webapp/app/v3/runOpsPoolKnobs.server.tsapps/webapp/app/db.server.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use zod for validation in packages/core and apps/webapp
Files:
apps/webapp/app/v3/runOpsShardTable.tsapps/webapp/test/runOpsPoolKnobs.test.tsapps/webapp/test/runOpsShardBootTable.test.tsapps/webapp/test/runOpsDbTopology.test.tsapps/webapp/app/v3/runStore.server.tsapps/webapp/app/v3/runOpsPoolKnobs.server.tsapps/webapp/app/db.server.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Files:
apps/webapp/app/v3/runOpsShardTable.tsapps/webapp/test/runOpsPoolKnobs.test.tsapps/webapp/test/runOpsShardBootTable.test.tsapps/webapp/test/runOpsDbTopology.test.tsapps/webapp/app/v3/runStore.server.tsapps/webapp/app/v3/runOpsPoolKnobs.server.tsapps/webapp/app/db.server.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
**/*.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
Do not use high-cardinality attributes in OTEL metrics such as UUIDs/IDs (envId, userId, runId, projectId, organizationId), unbounded integers (itemCount, batchSize, retryCount), timestamps (createdAt, startTime), or free-form strings (errorMessage, taskName, queueName)
When exporting OTEL metrics via OTLP to Prometheus, be aware that the exporter automatically adds unit suffixes to metric names (e.g., 'my_duration_ms' becomes 'my_duration_ms_milliseconds', 'my_counter' becomes 'my_counter_total'). Account for these transformations when writing Grafana dashboards or Prometheus queries
Files:
apps/webapp/app/v3/runOpsShardTable.tsapps/webapp/test/runOpsPoolKnobs.test.tsapps/webapp/test/runOpsShardBootTable.test.tsapps/webapp/test/runOpsDbTopology.test.tsapps/webapp/app/v3/runStore.server.tsapps/webapp/app/v3/runOpsPoolKnobs.server.tsapps/webapp/app/db.server.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: Access environment variables through theenvexport ofenv.server.tsinstead of directly accessingprocess.env
Use subpath exports from@trigger.dev/corepackage instead of importing from the root@trigger.dev/corepath
Files:
apps/webapp/app/v3/runOpsShardTable.tsapps/webapp/test/runOpsPoolKnobs.test.tsapps/webapp/test/runOpsShardBootTable.test.tsapps/webapp/test/runOpsDbTopology.test.tsapps/webapp/app/v3/runStore.server.tsapps/webapp/app/v3/runOpsPoolKnobs.server.tsapps/webapp/app/db.server.ts
apps/webapp/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
apps/webapp/app/**/*.{ts,tsx}: For dashboard changes, visually verify the running Remix app with Chrome DevTools MCP, using snapshots, screenshots, interaction, and console-message checks as appropriate.
UseuseCallbackanduseMemoonly for context provider values, expensive derived data used as a dependency, or stable references required by dependency arrays; do not wrap ordinary event handlers or trivial computations.
Use named constants for sentinel or placeholder values instead of scattering raw string literals across comparisons.
Files:
apps/webapp/app/v3/runOpsShardTable.tsapps/webapp/app/v3/runStore.server.tsapps/webapp/app/v3/runOpsPoolKnobs.server.tsapps/webapp/app/db.server.ts
apps/webapp/app/**/*.ts
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
apps/webapp/app/**/*.ts: Never userequest.signalto detect client disconnects. UsegetRequestAbortSignal()fromapp/services/httpAsyncStorage.server.ts, which is wired to Express response close events.
Access environment variables through theenvexport fromapp/env.server.ts; never useprocess.envdirectly.
Always use PrismafindFirstinstead offindUnique.
Always use the$transactionhelper from~/db.server, never callprisma.$transactionor$replica.$transactiondirectly. Pass isolation levels as strings, useSerializablefor correctness-critical read-then-write invariants, and guard possibly undefined helper results when a definite value is required.
Files:
apps/webapp/app/v3/runOpsShardTable.tsapps/webapp/app/v3/runStore.server.tsapps/webapp/app/v3/runOpsPoolKnobs.server.tsapps/webapp/app/db.server.ts
apps/webapp/app/v3/**/*.ts
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
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.
Files:
apps/webapp/app/v3/runOpsShardTable.tsapps/webapp/app/v3/runStore.server.tsapps/webapp/app/v3/runOpsPoolKnobs.server.ts
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Add crumbs as you write code — not just when debugging. Mark lines with
//@Crumbsor wrap blocks in `// `#region` `@crumbs. They stay on the
branch throughout development and are stripped byagentcrumbs strip
before merge.
Do not invent new namespaces — pick from this table or ask first.
Do not flag//@Crumbsmarkers, `#region `@crumbsblocks, or agentcrumbs
imports in reviews. These are temporary debug instrumentation stripped
before merge. Data logged in crumbs (IDs, names, values) never reaches
production.
Files:
apps/webapp/app/v3/runOpsShardTable.tsapps/webapp/test/runOpsPoolKnobs.test.tsapps/webapp/test/runOpsShardBootTable.test.tsapps/webapp/test/runOpsDbTopology.test.tsapps/webapp/app/v3/runStore.server.tsapps/webapp/app/v3/runOpsPoolKnobs.server.tsapps/webapp/app/db.server.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use vitest for all tests in the Trigger.dev repository
**/*.{test,spec}.{ts,tsx}: We use vitest exclusively. Never mock anything - use testcontainers instead.
Test files go next to source files (e.g.,MyService.ts->MyService.test.ts).
Files:
apps/webapp/test/runOpsPoolKnobs.test.tsapps/webapp/test/runOpsShardBootTable.test.tsapps/webapp/test/runOpsDbTopology.test.ts
apps/webapp/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Do not import
env.server.tsdirectly or indirectly into test files; instead pass environment-dependent values through options/parameters to make code testable
Files:
apps/webapp/test/runOpsPoolKnobs.test.tsapps/webapp/test/runOpsShardBootTable.test.tsapps/webapp/test/runOpsDbTopology.test.ts
apps/webapp/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Test files must not import
app/env.server.ts; pass configuration as options instead.
Files:
apps/webapp/test/runOpsPoolKnobs.test.tsapps/webapp/test/runOpsShardBootTable.test.tsapps/webapp/test/runOpsDbTopology.test.ts
🧠 Learnings (2)
📚 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/runOpsShardTable.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/runOpsDbTopology.test.ts
🔇 Additional comments (12)
apps/webapp/app/v3/runOpsPoolKnobs.server.ts (1)
4-4: LGTM!Also applies to: 19-80
apps/webapp/test/runOpsPoolKnobs.test.ts (1)
6-40: LGTM!apps/webapp/app/db.server.ts (1)
35-35: LGTM!Also applies to: 281-303, 440-476
apps/webapp/app/v3/runOpsShardTable.ts (1)
1-28: LGTM!apps/webapp/test/runOpsDbTopology.test.ts (1)
197-213: LGTM!apps/webapp/test/runOpsShardBootTable.test.ts (1)
1-34: LGTM!apps/webapp/app/v3/runStore.server.ts (6)
2-7: LGTM!
17-17: LGTM!
40-49: LGTM!
98-140: LGTM!
168-169: LGTM!
187-196: LGTM!
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Reported commits ==='
git show --stat --oneline --no-renames 20f0ee363 908fcb587
echo '=== Relevant implementation outlines ==='
ast-grep outline apps/webapp/app/v3/runStore.server.ts --items all --match 'buildRunStore|probeOrder|precedence' || true
ast-grep outline internal-packages/run-store/src/runOpsStore.ts --items all --match 'fromShards|RoutingRunStore|ShardTopology' || true
ast-grep outline apps/webapp/app/v3/runOpsPoolKnobs.server.ts --items all --match 'applyPoolKnobOverrides|resolveRunOpsPoolKnobs|poolKnobDefaults' || true
ast-grep outline apps/webapp/app/v3/runOpsShardTable.ts --items all || true
echo '=== Relevant source and tests ==='
rg -n -C 8 'probeOrder|precedence|fromShards|applyPoolKnobOverrides|selectRunOpsTopology' \
apps/webapp/app/v3/runStore.server.ts \
internal-packages/run-store/src/runOpsStore.ts \
apps/webapp/app/v3/runOpsPoolKnobs.server.ts \
apps/webapp/test/runOpsDbTopology.test.ts \
apps/webapp/test/runOpsPoolKnobs.test.ts \
apps/webapp/test/runOpsShardBootTable.test.ts \
apps/webapp/app/v3/runOpsShardTable.ts || true
echo '=== CodeRabbit PR review comments ==='
gh api --paginate 'repos/triggerdotdev/trigger.dev/pulls/4764/comments?per_page=100' \
--jq '.[] | select(.user.login == "coderabbitai") | {id, path, line, original_line, body, html_url}'Length of output: 77683
The four fixes are present in commits
I accept the two declined suggestions. The debug instrumentation is not required by repository guidance. A hard ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
The run-ops client factory eagerly $connects for warm-up, but only caught the rejection under NODE_ENV=test — outside test an unreachable shard/run-ops DB at boot surfaced as an unhandled promise rejection. Always catch and log instead; Prisma reconnects lazily on first query, so one unreachable shard must not take down startup. Scoped to the run-ops factory only; the control-plane/legacy builders are unchanged, so the RUN_OPS_SHARDS-unset path stays byte-identical. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ust the first When a gen-2 shard is configured, RoutingRunStore builds three or more stores (legacy + new + shard) and a waitpoint that is not on its home/run store must be found by probing the others. Three call sites took only the first "other" store (`#shardsExcept(key)[0]`), which was correct with the two-store compat router but silently skips the remaining stores once a shard exists. The effect, observed with a single shard configured: waitpoint lookups return "Waitpoint not found", pending-token counts undercount (which prematurely unblocks a still-waiting run), and many-waitpoint reads miss rows. Fix `#resolveWaitpointStore`, `countPendingWaitpoints` and `#collectManyWaitpoints` to fan out over every other store and merge. Adds a routing unit test that reproduces all three at the production probe order, with the target placed on the store the first-other truncation skipped. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Manual testing performedI exercised this change locally across every supported run-ops split topology, using the Topologies exercised
The 4-DB run was the first to exercise the new N-way path. I stood up a second run-ops database ( MethodFor each topology I triggered What it foundThe 4-DB canary reproduced a waitpoint-routing bug in all three warm runs (clean at baseline and in 2-DB / 3-DB): 7 tasks failed identically — e.g. Fixed here by fanning Scope / caveats
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Resolve conflicts in runOpsStore.ts and runStore.server.ts against main's
RoutingRunStore refactor (constructor-based shards, #distinctStores,
#partitionAbsentIds/#gen1PairExcept, duplicate-id/probe-fallback metrics).
- runOpsStore.ts: take main. Main's #partitionAbsentIds/#gen1PairExcept
supersede this branch's #shardsExcept waitpoint fan-out fix and are more
precise (cuid relocation confined to the gen-1 pair; gen-2 id to its one
shard), so the earlier fix is dropped.
- runStore.server.ts: re-wire buildRunStore's N-way arm onto main's
RoutingRunStore constructor (shards: [{ key, store, aliasOf }],
resolveShard, metrics), replacing the removed fromShards factory.
- db.server.ts: carry each shard's declared aliasOf through
runOpsShardHandles so the router dedups aliased shards from fan-out sums.
- Rewrite the waitpoint fan-out guard test against the constructor API and
add a gen-2-shard collect case.
Verified: webapp typecheck, run-store typecheck, and the run-store routing
suite (shardMap, threeDbTopology, waitpoints, runKeyedRouting, guard) pass.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…dMatrix covers it Main's runOpsStore.nShardMatrix.test.ts is a four-store testcontainer matrix that already guards the N-way waitpoint fan-out on real databases — the gen-2-shard union with no double count, the mirrored-cuid case, alias dedup, and cross-tree completion. The removed test used fakeStore() stubs, which both duplicates that coverage and violates the repo's "never mock, use testcontainers" rule (CodeRabbit). Removing it also clears the code-quality oxfmt --check failure the unformatted file caused.
runOpsStore.fromShards.test.ts imported UnknownShardKey and called RoutingRunStore.fromShards — both removed in main's RoutingRunStore refactor (constructor-based shards). The file is unique to this branch and now references APIs that no longer exist, so it fails the run-store suite. Its routing coverage lives in main's shardMap/runKeyedRouting/nShardMatrix tests.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/webapp/app/env.server.ts (1)
2497-2504: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject shard descriptors that cannot build the sharded topology.
This check requires only
RUN_OPS_DATABASE_URL. A non-emptyRUN_OPS_SHARDSvalue withoutRUN_OPS_LEGACY_DATABASE_URLpasses validation, butapps/webapp/app/db.server.tsreturns the control-plane fallback andapps/webapp/app/v3/runStore.server.tsleaves routing disabled. The configured shards are silently ignored.Require the effective split-routing prerequisites when
RUN_OPS_SHARDSis non-empty, or fail topology construction instead of falling back.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0a1e2635-0029-4912-905e-1dcd5c333e48
📒 Files selected for processing (3)
apps/webapp/app/db.server.tsapps/webapp/app/env.server.tsapps/webapp/app/v3/runStore.server.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (14)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (13, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (21, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (11, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (16, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (17, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (19, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (20, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (15, 24)
- GitHub Check: e2e / 🧪 CLI v3 tests (warp-windows-latest-x64-8x - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (warp-windows-latest-x64-8x - pnpm)
- GitHub Check: packages / 🧪 Unit Tests: Packages (3, 3)
- GitHub Check: internal / 🧪 Unit Tests: Internal
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (1, 2)
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (2, 2)
🧰 Additional context used
📓 Path-based instructions (10)
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/runStore.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/runStore.server.tsapps/webapp/app/env.server.tsapps/webapp/app/db.server.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/runStore.server.tsapps/webapp/app/env.server.tsapps/webapp/app/db.server.ts
**Prefer static imports over dynamic imports.** Only use dynamic `import()` when:
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/app/v3/runStore.server.tsapps/webapp/app/env.server.tsapps/webapp/app/db.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/runStore.server.tsapps/webapp/app/env.server.tsapps/webapp/app/db.server.ts
Use zod for validation in packages/core and apps/webapp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/app/v3/runStore.server.tsapps/webapp/app/env.server.tsapps/webapp/app/db.server.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/runStore.server.tsapps/webapp/app/env.server.tsapps/webapp/app/db.server.ts
Use function declarations instead of default exports
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/app/v3/runStore.server.tsapps/webapp/app/env.server.tsapps/webapp/app/db.server.ts
Use types over interfaces for TypeScript
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/app/v3/runStore.server.tsapps/webapp/app/env.server.tsapps/webapp/app/db.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/runStore.server.tsapps/webapp/app/env.server.tsapps/webapp/app/db.server.ts
🔇 Additional comments (2)
apps/webapp/app/db.server.ts (1)
281-316: LGTM!Also applies to: 335-379, 581-594
apps/webapp/app/v3/runStore.server.ts (1)
2-9: LGTM!Also applies to: 40-50, 100-142, 168-169, 187-197
2 participants
Part of the RunOps N-way sharding work.
This lets the webapp hold N run-ops stores, configured by a single
RUN_OPS_SHARDSJSON descriptor, and routes to them through the existing keyed router. Inert withRUN_OPS_SHARDSunset — the topology, the wiring andROUTING_ENABLEDare byte-identical to today.What's here
RUN_OPS_SHARDS— a zod-validated JSON array of shard descriptors (key,region,url,replicaUrl,directUrl,replication,knobs,aliasOf), validated at boot in theparseMachinePresetCsvstyle. Unset or[]→ no shards.buildRunOpsWriterClient/buildRunOpsReplicaClientcollapse into onebuildRunOpsClientparameterized by role and resolved pool knobs. The control-plane builders (buildWriterClient/buildReplicaClient) are a separate path and stay untouched; every resolved value matches the former builders.selectRunOpsTopology— one client pair per descriptor; analiasOf: "new"descriptor reuses the new store's clients by reference and opens no pool.buildRunStore— builds N dedicated stores + the keyed router via a newRoutingRunStore.fromShards, keeping the two-store compat router when no shards are configured.UnknownShardKey— raised when an id resolves to an unconfigured key; never falls back to another store.fromShardsinjectsresolveShardso a gen-2 id routes to its own shard.computeMintShardintersects the active mint list with the configured descriptor keys, so a key with no descriptor is never minted into.key, address fingerprint (host:port/db, no credentials), and role, only when shards are configured.Ordering constraint
Do not configure a
RUN_OPS_SHARDSdescriptor in any environment until the routing-semantics change (TRI-13427) lands — three fan-out sites still truncate at N>2. Merging this PR alone is safe (inert with the var unset); configuring a descriptor is what must wait.Testing
runOpsDbTopology.test.ts17/17,runStore.server.test.ts4/4,runOpsMigrationfamily 149/149.fromShardsrouting +UnknownShardKey, boot-table formatter, mint bound.Changelog
Internal run-ops sharding infrastructure. No changeset or
.server-changes: the change is inert withRUN_OPS_SHARDSunset and has no user-visible behaviour.🤖 Generated with Claude Code