feat(webapp): resolve which shard an environment mints run roots into - #4755
feat(webapp): resolve which shard an environment mints run roots into#4755d-cs wants to merge 9 commits into
Conversation
Adds the third stage of the run-id mint gate chain. `resolveMintShard(env)` returns the shard key an environment mints new roots into: the active shard list, then a per-env or per-org pin, then a rendezvous hash of the environment id. With `RUN_OPS_MINT_SHARDS` unset or empty it returns "new", which is today's behaviour, so this merges inert. `computeRunIdMintKind` and `mintFlipGrace.ts` are untouched. The grace pattern is cloned into `mintShardGrace.ts` rather than widened, so the existing cuid/runOpsId flip grace keeps its behaviour. Design notes: - Pure core plus env-bound wrapper, mirroring `runOpsMintKind.server.ts`. Determinism is a property of `computeMintShard` for fixed deps; the wrapper supplies the clock, exactly as `effectiveMintKind` takes `nowMs`. - Zero new queries on the trigger hot path. Both pins live in the org override blob that `mintRunFriendlyId` already holds. - HRW scores `sha256(envId \0 key)` at 64 bits, over a sorted key list, with a lexicographic tie-break. A 32-bit score collides at our environment count, and without the sort two deployments listing the same keys in a different CSV order would place environments differently. - `parseShardCsv` rejects anything outside [a-z0-9] and rejects the reserved keys at boot. `generateRunOpsIdV2` throws on an out-of-alphabet char, so an unvalidated key would become a throw on the mint path. - A pin outside the active set falls through to the hash and reports once per environment per process. Honouring it would leak the drain the active list performs; throwing would fail customer triggers whenever a pinned shard drains. The loud-on-unknown-key rule governs reading an id, not writing one. - "new" is a legal pin value, holding one org or environment on gen-1 while the rest of the fleet mints gen-2. Without it, a non-empty active set moves every environment at once. - The active-set grace is stamped by `RUN_OPS_MINT_SHARDS_PREV` and `RUN_OPS_MINT_SHARDS_FLIPPED_AT`. A prev list with no timestamp is dropped; a timestamp with an empty prev list graces a first activation. No changeset and no `.server-changes` note: nothing user-visible, and no caller carries the returned key into an id yet.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (30)
🧰 Additional context used📓 Path-based instructions (12)**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
{packages/core,apps/webapp}/**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.ts📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
Files:
apps/webapp/**/*.{ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
apps/webapp/app/**/*.{ts,tsx}📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/**/*.ts📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/v3/**/*.ts📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
**/*📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{test,spec}.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/**/*.test.{ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
apps/webapp/**/*.{test,spec}.{ts,tsx}📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
🧠 Learnings (2)📚 Learning: 2026-06-16T09:19:47.637ZApplied to files:
📚 Learning: 2026-05-28T20:02:10.647ZApplied to files:
🪛 ast-grep (0.45.1)apps/webapp/app/v3/featureFlags.server.ts[error] 307-326: Recursive/iterative merge copies attacker-controllable keys from a source object into a target via a computed property assignment without rejecting dangerous keys, allowing prototype pollution. Skip or block "proto", "constructor", and "prototype" keys (e.g. Note: [CWE-1321] Improperly Controlled Modification of Object Prototype Attributes ('Prototype Pollution'). (prototype-pollution-recursive-merge-typescript) 🔇 Additional comments (5)
WalkthroughThe change adds gen-2 mint-shard feature flags with validation, scope locks, environment pins, shard sets, grace metadata, and a fleet-wide override. Shard-set resolution reads shared flags and applies grace-period selection. Global flag updates stamp shard transitions transactionally and preserve server-owned metadata. Mint-shard resolution no longer applies deployment ceilings. It uses effective shard sets, overrides, pins, caching, rendezvous hashing, and generation-1 fallbacks. Unit and integration tests cover validation, persistence, concurrency, caching, and routing behavior. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/webapp/app/v3/featureFlags.ts (1)
95-120: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winReuse
isValidPinValueinstead of duplicating the pin contract.
mintShardGrace.tsalready exportsSHARD_KEY_PATTERN,GEN_1_PIN_VALUE, andisValidPinValue. This file now re-implements that predicate twice: once at Line 100 and once at Line 116. The alphabet regex/^[a-z0-9]$/and the"new"literal exist in three places across the two files.The write-side validator and the read-side resolver must agree. If the alphabet or the gen-1 sentinel changes in
mintShardGrace.ts, these copies keep accepting a pin thatreadPininrunOpsMintShard.server.tsthen discards, which silently un-pins an environment.
mintShardGrace.tsimports only a type from@trigger.dev/core, so importing it here adds no runtime cycle.♻️ Proposed refactor to share one predicate
Add the import at the top of the file:
import { z } from "zod"; +import { isValidPinValue } from "./runOpsMigration/mintShardGrace";Then reuse it in both schemas:
- [FEATURE_FLAG.runOpsMintShard]: z - .string() - .refine((v) => v === "new" || /^[a-z0-9]$/.test(v), 'must be a single [a-z0-9] char, or "new"'), + [FEATURE_FLAG.runOpsMintShard]: z + .string() + .refine(isValidPinValue, 'must be a single [a-z0-9] char, or "new"'), // Per-environment pins as JSON: {"<environmentId>": "<shard key>"}. A JSON string because // this catalog is scalar-only. Rejected at write, so a typo cannot silently un-pin an env. [FEATURE_FLAG.runOpsMintShardEnvPins]: z.string().superRefine((raw, ctx) => { const fail = (message: string) => ctx.addIssue({ code: z.ZodIssueCode.custom, message }); let parsed: unknown; try { parsed = JSON.parse(raw); } catch { return fail("must be valid JSON"); } if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) { return fail("must be a JSON object mapping environment id to shard key"); } for (const [environmentId, value] of Object.entries(parsed)) { - if (typeof value !== "string" || !(value === "new" || /^[a-z0-9]$/.test(value))) { + if (!isValidPinValue(value)) { fail(`"${environmentId}" must map to a single [a-z0-9] char, or "new"`); } } }),apps/webapp/app/v3/runOpsMigration/mintShardGrace.test.ts (1)
119-148: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
mintShardStampWarning.
mintShardGrace.tsexportsmintShardStampWarning, and this file tests every other export. The warning is the only operator signal for a half-configured cutover, whereRUN_OPS_MINT_SHARDS_PREVis set butRUN_OPS_MINT_SHARDS_FLIPPED_ATis not. It has three branches and none are covered.💚 Proposed tests
+describe("mintShardStampWarning", () => { + it("stays quiet while the active set is empty", () => { + expect( + mintShardStampWarning({ shards: "", prev: "a", flippedAt: undefined }) + ).toBeUndefined(); + }); + + it("warns when prev is set but the flip timestamp is not", () => { + expect(mintShardStampWarning({ shards: "a", prev: "b", flippedAt: undefined })).toMatch( + /FLIPPED_AT/ + ); + }); + + it("stays quiet when both halves of the stamp are set", () => { + expect( + mintShardStampWarning({ shards: "a", prev: "b", flippedAt: new Date(T).toISOString() }) + ).toBeUndefined(); + }); + + it("stays quiet when prev is empty", () => { + expect(mintShardStampWarning({ shards: "a", prev: "", flippedAt: undefined })).toBeUndefined(); + }); +});Add
mintShardStampWarningto the import list at Line 3.apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts (1)
162-168: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winGive the shard-set cutover its own grace env var.
Line 165 reuses
RUN_OPS_MINT_FLIP_GRACE_MSasgraceMs.env.server.tsdocuments that variable as the grace for arunOpsMintKindflip, and requires it to exceedRUN_OPS_MINT_FLAG_CACHE_TTL_MSplus the control-plane cache TTL. The two windows cover different things:
RUN_OPS_MINT_FLIP_GRACE_MSabsorbs feature-flag cache staleness across processes.- The shard-set window absorbs deploy skew, because
RUN_OPS_MINT_SHARDSis a deploy-time value and a rolling deploy runs old and new CSVs at the same time.The sizing inputs differ, so one knob cannot serve both. An operator who retunes the flag-cache grace also retunes the shard cutover window without knowing it. If that window becomes shorter than the rolling-deploy duration, pods mint into different shard sets at the same instant, which is the exact condition
_PREVand_FLIPPED_ATexist to prevent.The new
RUN_OPS_MINT_SHARDS_*block already owns_PREVand_FLIPPED_AT. Add the grace there while no caller consumes the returned shard key yet.♻️ Proposed change
In
apps/webapp/app/env.server.ts, next to the other shard variables:RUN_OPS_MINT_SHARDS: shardCsvString(), RUN_OPS_MINT_SHARDS_PREV: shardCsvString(), RUN_OPS_MINT_SHARDS_FLIPPED_AT: z.string().datetime().optional(), + // Cutover window for a RUN_OPS_MINT_SHARDS set change. Must exceed the rolling-deploy + // duration so every process crosses the boundary together. Sized independently of + // RUN_OPS_MINT_FLIP_GRACE_MS, which absorbs feature-flag cache staleness instead. + RUN_OPS_MINT_SHARDS_GRACE_MS: z.coerce.number().int().default(90_000),Then in this file:
return computeMintShard(environment, { resolution: shardResolution, nowMs: Date.now(), - graceMs: env.RUN_OPS_MINT_FLIP_GRACE_MS, + graceMs: env.RUN_OPS_MINT_SHARDS_GRACE_MS, orgFeatureFlags: environment.orgFeatureFlags, onPinRejected: reportPinRejected, });
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 858e4dcd-c0d9-438b-a74a-848f9e0b870f
📒 Files selected for processing (6)
apps/webapp/app/env.server.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/runOpsMigration/mintShardGrace.test.tsapps/webapp/app/v3/runOpsMigration/mintShardGrace.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (32)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (22, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (23, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (24, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (16, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (20, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (1, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (15, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (19, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (18, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (12, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (17, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (13, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (8, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (7, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (21, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (10, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (2, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (9, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (14, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (6, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (3, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (4, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (11, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (5, 24)
- GitHub Check: typecheck / typecheck
- GitHub Check: obsmap / 🧪 Unit Tests: Observability Map
- GitHub Check: fk-cascade-guard / fk-cascade-guard
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (2, 2)
- GitHub Check: runops-guard / runops-guard
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (1, 2)
- GitHub Check: code-quality / code-quality
- GitHub Check: Analyze (javascript-typescript)
🧰 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/runOpsMigration/mintShardGrace.test.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.tsapps/webapp/app/env.server.tsapps/webapp/app/v3/runOpsMigration/mintShardGrace.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/runOpsMigration/mintShardGrace.test.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.tsapps/webapp/app/env.server.tsapps/webapp/app/v3/runOpsMigration/mintShardGrace.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Files:
apps/webapp/app/v3/runOpsMigration/mintShardGrace.test.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.tsapps/webapp/app/env.server.tsapps/webapp/app/v3/runOpsMigration/mintShardGrace.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/app/v3/runOpsMigration/mintShardGrace.test.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.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:
apps/webapp/app/v3/runOpsMigration/mintShardGrace.test.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.tsapps/webapp/app/env.server.tsapps/webapp/app/v3/runOpsMigration/mintShardGrace.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/runOpsMigration/mintShardGrace.test.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.tsapps/webapp/app/env.server.tsapps/webapp/app/v3/runOpsMigration/mintShardGrace.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/app/v3/runOpsMigration/mintShardGrace.test.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.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/runOpsMigration/mintShardGrace.test.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.tsapps/webapp/app/env.server.tsapps/webapp/app/v3/runOpsMigration/mintShardGrace.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/runOpsMigration/mintShardGrace.test.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.tsapps/webapp/app/env.server.tsapps/webapp/app/v3/runOpsMigration/mintShardGrace.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/app/v3/runOpsMigration/mintShardGrace.test.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.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/runOpsMigration/mintShardGrace.test.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.tsapps/webapp/app/v3/runOpsMigration/mintShardGrace.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/runOpsMigration/mintShardGrace.test.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.tsapps/webapp/app/env.server.tsapps/webapp/app/v3/runOpsMigration/mintShardGrace.ts
🧠 Learnings (1)
📚 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/runOpsMintShard.server.tsapps/webapp/app/v3/runOpsMigration/mintShardGrace.ts
🔇 Additional comments (8)
apps/webapp/app/v3/runOpsMigration/mintShardGrace.ts (2)
6-20: LGTM!Also applies to: 29-48, 53-62, 66-79, 83-95
22-25: 🗄️ Data Integrity & IntegrationNo
ShardKeytype mismatch
ShardKeyis"legacy" | "new" | string, so it admits"new"and all single[a-z0-9]characters. The type predicate is sound for these values.> Likely an incorrect or invalid review comment.apps/webapp/app/env.server.ts (1)
7-7: LGTM!Also applies to: 45-60, 2019-2026
apps/webapp/app/v3/featureFlags.ts (2)
29-31: LGTM!Also applies to: 133-141
98-120: 🩺 Stability & AvailabilityNo resolver change is needed.
ZodEffectsuses the resolver’s{ type: "string" }fallback, so both admin UIs renderStringControl. The global page intentionally renders these flags as locked throughGLOBAL_LOCKED_FLAGS.> Likely an incorrect or invalid review comment.apps/webapp/app/v3/runOpsMigration/mintShardGrace.test.ts (1)
1-118: LGTM!apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts (1)
1-13: LGTM!Also applies to: 15-26, 31-44, 48-57, 63-65, 67-81, 90-112, 116-132, 136-146, 148-161
apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.ts (1)
5-41: LGTM!Also applies to: 43-58, 60-80, 82-185, 187-248
| import { describe, expect, it } from "vitest"; | ||
| import { computeMintShard, type MintShardDeps } from "./runOpsMintShard.server"; | ||
| import { type MintShardSetResolution } from "./mintShardGrace"; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
This test imports env.server.ts indirectly. Move the pure core into its own module.
Line 2 imports ./runOpsMintShard.server. That module's Line 3 is import { env } from "~/env.server". Loading this test therefore evaluates env.server.ts, which runs EnvironmentSchema.parse(process.env) at its Line 2500 and requires DATABASE_URL, DIRECT_URL, SESSION_SECRET, MAGIC_LINK_SECRET, a 32-byte ENCRYPTION_KEY, MANAGED_WORKER_SECRET, DEPLOY_REGISTRY_HOST, and CLICKHOUSE_URL. It also executes the module-level side effects at runOpsMintShard.server.ts Lines 116-132, including a logger.warn call.
The test then either fails to load without a complete environment, or passes only because of ambient environment values.
runOpsMintShard.server.ts Line 83 already documents computeMintShard as "PURE CORE — no env, no clock, no I/O; tests drive this directly". Extract that pure core into a module that does not import env.server, and keep only the env-bound wrapper in runOpsMintShard.server.ts.
As per path instructions: "Do not import env.server.ts directly or indirectly into test files; instead pass environment-dependent values through options/parameters" and "Test files must not import app/env.server.ts; pass configuration as options instead."
♻️ Proposed split
Move MintShardDeps, asRecord, readEnvPin, readPin, shardScore, hrwSelect, and computeMintShard into a new mintShardAssignment.ts that imports only node:crypto, the ShardKey type, ~/v3/featureFlags, and ./mintShardGrace.
Then in runOpsMintShard.server.ts:
-import { createHash } from "node:crypto";
import type { ShardKey } from "`@trigger.dev/core/v3/isomorphic`";
import { env } from "~/env.server";
import { logger } from "~/services/logger.server";
-import { FEATURE_FLAG } from "~/v3/featureFlags";
import {
buildMintShardResolution,
- effectiveMintShardSet,
- GEN_1_PIN_VALUE,
- isValidPinValue,
mintShardStampWarning,
type MintShardSetResolution,
} from "./mintShardGrace";
+import { computeMintShard, type MintShardDeps } from "./mintShardAssignment";And in this test file:
-import { computeMintShard, type MintShardDeps } from "./runOpsMintShard.server";
+import { computeMintShard, type MintShardDeps } from "./mintShardAssignment";Rename this file to mintShardAssignment.test.ts so it sits next to its source.
Source: Path instructions
… environment A rolling deploy takes hours, so two pods run different values of RUN_OPS_MINT_SHARDS at the same time. The grace window is sized in seconds, so new pods left it long before old pods were gone: for the rest of the rollout the two placed the same environment on different shards. That is the divergence the grace exists to close. The environment variable is now a ceiling that changes only by deploy. It says which shard keys this deployment can mint into. The live list moves to the control-plane database as runOpsMintShardSet, so every pod reads one shared value whatever config generation it is running. Resolution intersects the two, so a stored key this deployment cannot route is never minted into. RUN_OPS_MINT_SHARDS_PREV and RUN_OPS_MINT_SHARDS_FLIPPED_AT are gone. An environment variable cannot record its own flip time, and an operator cannot know a rollout's end in advance. The stamp is now written server-side against the control-plane clock, under an advisory lock, on a genuine change. Stamping generalizes to N graced flag groups in one transaction under one lock, covering the existing mint-kind trio and the new list. That closes a hole on the global admin flags page, which wrote any catalog key with a bare upsert: a graced key could be set with no stamp, or swept away by a save that omitted it. applyGlobalMintKindFlip stays as a thin wrapper so its route and its test keep working unchanged. Operational rule this creates: every change to RUN_OPS_MINT_SHARDS must land across the whole fleet before the flag selects a key it adds. Routing before minting, which is how the shard topology is already gated.
Three areas of the change had no tests. The pure placement logic was well covered; the production entry point and the safety claims were not. resolveMintShard now takes its list reader as a dependency, the same way computeRunIdMintKind takes its flag reader. That makes the cache, the TTL, the ceiling short-circuit and the read fail-safe testable without a database and without mocking. The fail-safe matters: a failed read returns gen-1 rather than guessing a list, because guessing would move every environment's placement for the length of one blip. The catalog tests pin the claim that a bad value is rejected at write. Until now nothing checked it, so an unroutable shard key or a malformed pin blob could have been stored and only failed later. The scope-lock tests pin each key to the scope its resolver reads: pins are locked globally because they are read from the org blob, and the list is locked per-org because it is deployment-wide. Still not covered, and needing a reviewer with Postgres and a browser: the two admin write routes, and boot refusal on a malformed ceiling.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
apps/webapp/app/v3/featureFlags.ts (1)
126-136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
parseShardCsvinstead of a second inline shard-key regex.
mintShardGrace.tsLines 35-54 already own the canonical CSV contract. That parser rejects the reserved keysnewandlegacyexplicitly and validates againstSHARD_KEY_PATTERN. This catalog entry re-implements the key check with a literal/^[a-z0-9]$/.Both accept the same values today. They can diverge if
SHARD_KEY_PATTERNorRESERVED_SHARD_KEYSchanges, because only one side would follow.mintShardGrace.tsis pure and imports no server-only module, so it is safe to import from this file.♻️ Proposed refactor
+import { parseShardCsv } from "~/v3/runOpsMigration/mintShardGrace"; + // CSV of the shard keys eligible for root minting right now, bounded by RUN_OPS_MINT_SHARDS. // Empty means no gen-2 minting. Reserved keys are rejected: "new" already means gen-1. - [FEATURE_FLAG.runOpsMintShardSet]: z.string().refine( - (v) => - v - .split(",") - .map((s) => s.trim()) - .filter(Boolean) - .every((k) => /^[a-z0-9]$/.test(k)), - "must be a CSV of single [a-z0-9] chars" - ), + [FEATURE_FLAG.runOpsMintShardSet]: z.string().superRefine((raw, ctx) => { + try { + parseShardCsv(raw); + } catch (error) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: error instanceof Error ? error.message : "invalid shard key CSV", + }); + } + }),apps/webapp/app/routes/admin.api.v1.feature-flags.ts (1)
28-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the graced keys from
GRACED_GLOBAL_GROUPSinstead of repeating them here.
featureFlags.server.tsLines 189-210 already declare the graced groups, their derived keys, and the flat key list. This route repeats both the derived-key strip at Lines 29-35 and the primary-key check at Lines 38-40 as literals.A third graced group would require an edit here as well, and a missed edit lets a client-supplied stamp reach the database. Export a small helper from
featureFlags.server.tsthat strips the derived keys and reports whether a graced group is touched, then call it from both write paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c972261e-13d1-4425-b6b0-54122e409753
📒 Files selected for processing (11)
apps/webapp/app/env.server.tsapps/webapp/app/routes/admin.api.v1.feature-flags.tsapps/webapp/app/routes/admin.feature-flags.tsxapps/webapp/app/v3/featureFlags.server.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/runOpsMigration/mintShardGrace.test.tsapps/webapp/app/v3/runOpsMigration/mintShardGrace.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.tsapps/webapp/test/runOpsMintShardFlags.test.tsapps/webapp/test/runOpsMintShardSetFlip.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (13)
**/*.{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/test/runOpsMintShardFlags.test.tsapps/webapp/test/runOpsMintShardSetFlip.test.tsapps/webapp/app/routes/admin.api.v1.feature-flags.tsapps/webapp/app/routes/admin.feature-flags.tsxapps/webapp/app/v3/runOpsMigration/mintShardGrace.test.tsapps/webapp/app/env.server.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/runOpsMigration/mintShardGrace.tsapps/webapp/app/v3/featureFlags.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/test/runOpsMintShardFlags.test.tsapps/webapp/test/runOpsMintShardSetFlip.test.tsapps/webapp/app/routes/admin.api.v1.feature-flags.tsapps/webapp/app/routes/admin.feature-flags.tsxapps/webapp/app/v3/runOpsMigration/mintShardGrace.test.tsapps/webapp/app/env.server.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/runOpsMigration/mintShardGrace.tsapps/webapp/app/v3/featureFlags.server.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Files:
apps/webapp/test/runOpsMintShardFlags.test.tsapps/webapp/test/runOpsMintShardSetFlip.test.tsapps/webapp/app/routes/admin.api.v1.feature-flags.tsapps/webapp/app/routes/admin.feature-flags.tsxapps/webapp/app/v3/runOpsMigration/mintShardGrace.test.tsapps/webapp/app/env.server.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/runOpsMigration/mintShardGrace.tsapps/webapp/app/v3/featureFlags.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/runOpsMintShardFlags.test.tsapps/webapp/test/runOpsMintShardSetFlip.test.tsapps/webapp/app/v3/runOpsMigration/mintShardGrace.test.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.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:
apps/webapp/test/runOpsMintShardFlags.test.tsapps/webapp/test/runOpsMintShardSetFlip.test.tsapps/webapp/app/routes/admin.api.v1.feature-flags.tsapps/webapp/app/v3/runOpsMigration/mintShardGrace.test.tsapps/webapp/app/env.server.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/runOpsMigration/mintShardGrace.tsapps/webapp/app/v3/featureFlags.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/test/runOpsMintShardFlags.test.tsapps/webapp/test/runOpsMintShardSetFlip.test.tsapps/webapp/app/routes/admin.api.v1.feature-flags.tsapps/webapp/app/routes/admin.feature-flags.tsxapps/webapp/app/v3/runOpsMigration/mintShardGrace.test.tsapps/webapp/app/env.server.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/runOpsMigration/mintShardGrace.tsapps/webapp/app/v3/featureFlags.server.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/runOpsMintShardFlags.test.tsapps/webapp/test/runOpsMintShardSetFlip.test.tsapps/webapp/app/v3/runOpsMigration/mintShardGrace.test.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.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/runOpsMintShardFlags.test.tsapps/webapp/test/runOpsMintShardSetFlip.test.tsapps/webapp/app/v3/runOpsMigration/mintShardGrace.test.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.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:
apps/webapp/test/runOpsMintShardFlags.test.tsapps/webapp/test/runOpsMintShardSetFlip.test.tsapps/webapp/app/routes/admin.api.v1.feature-flags.tsapps/webapp/app/routes/admin.feature-flags.tsxapps/webapp/app/v3/runOpsMigration/mintShardGrace.test.tsapps/webapp/app/env.server.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/runOpsMigration/mintShardGrace.tsapps/webapp/app/v3/featureFlags.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/routes/admin.api.v1.feature-flags.tsapps/webapp/app/routes/admin.feature-flags.tsxapps/webapp/app/v3/runOpsMigration/mintShardGrace.test.tsapps/webapp/app/env.server.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/runOpsMigration/mintShardGrace.tsapps/webapp/app/v3/featureFlags.server.ts
apps/webapp/app/routes/**/*.ts
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
apps/webapp/app/routes/**/*.ts: Use Remix flat-file route conventions with dot-separated segments; for example,api.v1.tasks.$taskId.trigger.tsmaps to/api/v1/tasks/:taskId/trigger.
PAT-authenticated API routes must resolve their target organization or project within the caller's membership scope, using a membership filter or a helper such asfindProjectByReforresolveOrganizationForApiUser; RBAC authorization alone is insufficient.
Files:
apps/webapp/app/routes/admin.api.v1.feature-flags.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/routes/admin.api.v1.feature-flags.tsapps/webapp/app/v3/runOpsMigration/mintShardGrace.test.tsapps/webapp/app/env.server.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/runOpsMigration/mintShardGrace.tsapps/webapp/app/v3/featureFlags.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/runOpsMigration/mintShardGrace.test.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/runOpsMigration/mintShardGrace.tsapps/webapp/app/v3/featureFlags.server.ts
🧠 Learnings (3)
📚 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/runOpsMintShardFlags.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/runOpsMintShard.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/runOpsMintShard.server.test.ts
🔇 Additional comments (11)
apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.ts (2)
2-8: This test still imports./runOpsMintShard.server, which imports~/env.serverat its Line 4 and$replicaat its Line 3. The module now also parses the ceiling at load time (Line 124) and creates the process-wideliveCache(Line 187), so loading this test evaluatesEnvironmentSchema.parse(process.env)and the database client module. This was raised on an earlier commit and is unresolved.
24-38: LGTM!Also applies to: 53-73, 272-305, 307-394
apps/webapp/app/v3/runOpsMigration/mintShardGrace.ts (1)
22-27: LGTM!Also applies to: 70-99, 101-133
apps/webapp/app/env.server.ts (1)
47-60: LGTM!Also applies to: 2019-2024
apps/webapp/app/v3/featureFlags.ts (1)
29-36: LGTM!Also applies to: 174-177
apps/webapp/app/routes/admin.feature-flags.tsx (1)
8-19: LGTM!Also applies to: 120-128
apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts (1)
16-25: LGTM!Also applies to: 90-120, 122-146, 154-185, 221-238
apps/webapp/app/v3/runOpsMigration/mintShardGrace.test.ts (1)
7-9: LGTM!Also applies to: 120-232
apps/webapp/test/runOpsMintShardFlags.test.ts (2)
1-70: LGTM!Also applies to: 89-100
72-87: 🎯 Functional CorrectnessKeep the existing assertions.
GLOBAL_LOCKED_FLAGScontains both pin keys.> Likely an incorrect or invalid review comment.apps/webapp/test/runOpsMintShardSetFlip.test.ts (1)
1-190: LGTM!
Pins were per-organization and per-environment only, so completing a cutover meant visiting every organization that still carried a canary pin. There was no way to say "every environment mints here now". runOpsMintShardOverride is a global flag that outranks every pin and the hash. Setting it to "new" holds the whole fleet on the current id format, which is the inverse lever for an emergency. It is honored only while the key is in the active list, so it cannot mint into a drained or unroutable shard; an override outside the list is reported and explicit pins still apply. It is read in the same round-trip as the list it is bounded by, so it costs no extra query on the trigger path, and it is locked per-organization because an organization that could override the cutover lever would defeat it. Also marks the ceiling seam: RUN_OPS_MINT_SHARDS is sourced in exactly one place, and it should be deleted once shard descriptors are configured. The descriptors already name every key this deployment can route, so keeping a second hand-maintained list invites the two to drift.
RUN_OPS_MINT_SHARDS was added on this branch and never deployed, so there is nothing to keep compatible. It duplicated information the shard descriptors will own: a descriptor names every key this deployment can route, so a second hand-maintained list only gives the two a way to disagree. Its only job was to stop the list naming a key with no configured database. Nothing here mints, so that cannot happen yet, and by the time it can the descriptors exist and are the right source. Bounding the list belongs with them. The list flag alone is now the gate. Unset or empty means today's behaviour, which is the state of every deployment that has not set it, so this stays inert on merge. env.server.ts is untouched by this branch again. Dependency this creates: the change that carries a shard key into an id must not land before the descriptors bound the list, or it must bound the list itself.
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b0ba2bce-c4dd-49fa-a966-4f50a30a5df5
📒 Files selected for processing (4)
apps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.tsapps/webapp/test/runOpsMintShardFlags.test.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: Analyze (javascript-typescript)
🧰 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/test/runOpsMintShardFlags.test.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.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/test/runOpsMintShardFlags.test.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Files:
apps/webapp/test/runOpsMintShardFlags.test.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.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/runOpsMintShardFlags.test.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.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:
apps/webapp/test/runOpsMintShardFlags.test.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.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/test/runOpsMintShardFlags.test.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.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/runOpsMintShardFlags.test.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.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/runOpsMintShardFlags.test.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.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:
apps/webapp/test/runOpsMintShardFlags.test.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.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/featureFlags.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.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/featureFlags.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.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/featureFlags.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.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:
apps/webapp/test/runOpsMintShardFlags.test.tsapps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.ts
🔇 Additional comments (2)
apps/webapp/app/v3/featureFlags.ts (2)
102-138: LGTM!Also applies to: 160-168, 181-185
37-38: 🗄️ Data Integrity & IntegrationNo additional change is required for the sparse-record compatibility or previous shard-set handling concerns; the existing validation paths intentionally support these cases.
| // The global override outranks every pin, so one flag completes a cutover without visiting | ||
| // each org. An override outside the active set is ignored, so explicit pins still apply. | ||
| if (isValidPinValue(deps.globalOverride)) { | ||
| const override = deps.globalOverride; | ||
| if (override === GEN_1_PIN_VALUE) { | ||
| return "new"; | ||
| } | ||
| if (activeSet.includes(override)) { | ||
| return override; | ||
| } | ||
| deps.onPinRejected?.({ environmentId: environment.id, pin: override, activeSet }); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add the required crumb markers.
The changed routing, cache, and validation blocks have no @crumbs markers. Add approved crumb markers while developing, then run agentcrumbs strip before merge. If a namespace is required, ask for one instead of inventing one.
apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts#L110-L121: mark the global-override decision block.apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts#L185-L211: mark the cache refresh and read-failure block.apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.ts#L396-L459: mark the added override test block.apps/webapp/test/runOpsMintShardFlags.test.ts#L72-L118: mark the added override validation and scope-lock test blocks.
As per coding guidelines, add crumbs as you write code and mark blocks with // @Crumbs or `// `#region` `@crumbs.
📍 Affects 3 files
apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts#L110-L121(this comment)apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts#L185-L211apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.ts#L396-L459apps/webapp/test/runOpsMintShardFlags.test.ts#L72-L118
Source: Coding guidelines
Review found that this branch silently disabled the unset button for runOpsMintKind on the global admin flags page. The graced keys were skipped by the replace sweep so their stamp could not be bare-written, but that skip covered the operator-supplied key as well as the server-computed ones. The page omits a key to unset it, so the omission was read as "leave alone" and the row survived. Before this branch the same gesture deleted it. A graced group is now all-or-nothing. Submitting its primary writes the group with a fresh stamp. Omitting the primary deletes the primary and its stamp together, because a stamp left behind without its primary keeps being served: an empty list beside a live prev list still resolves to the prev list for the rest of the window, which would mint into a shard just removed. Also from review: - The whole save is one transaction again. The stamp, the upserts and the deletes could previously half-apply across two. - The advisory lock takes the previous id as well as the current one, in a fixed order. A deploy rolls for hours, so renaming it left writers on the older release serializing against nothing. Drop the legacy id next release. - A bad global override is reported once per value rather than once per environment. It applies to the whole fleet, so keying the report by environment turned one misconfiguration into a log line and a retained set entry per environment, on the trigger path. Both reporters are bounded now. - The stamp keys render read-only. They were editable controls whose values were discarded on save. - Groups name their primary and derived keys instead of relying on position. - Corrected a claim in a comment: the cache TTL does not bound cross-process disagreement on its own, because the read goes to a replica. Stated why that is tolerable here specifically. - The deprecated single-group entry point is gone; its test now covers the grouped one.
Observability mapAs of 19/100 over 447 measured of 465 entry points (base 19, 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. |
…up fix Running the Postgres suites surfaced two tests asserting opposite things about the same gesture. One was written before groups became all-or-nothing and expected the list to survive a save that omits it, which is the behaviour that made unset a silent no-op. It is replaced with the property that is actually correct: resubmitting the same list alongside another flag leaves the list and its cutover clock alone. Nothing in the implementation changed here. Only a test that encoded the old bug did.
…ion helper Both graced writes called client.$transaction directly. The repo rule is to use the $transaction helper from db.server, which adds the OTEL span and logs the infrastructure errors the raw client swallows. One of these writes stamps a cutover window and the other deletes flags, so a transaction that silently did not run is the case most worth seeing. The helper resolves undefined instead of throwing when it swallows such an error, so both call sites now treat that as a failure the caller sees.
Both global write routes carried their own copy of two answers: which flag keys are graced, and which are server-computed. The JSON API named all four derived keys in a destructure and both primaries in its branch condition. So adding a graced group needed an edit in three files, and missing one would either write an unstamped flip or accept a stamp from a request body. Both answers now come from the group table. touchesGracedGroup and withoutDerivedKeys are exported and used by the route, so a new group needs no route change at all. The global page's managed-cloud refusal moves into lockedFlagsInPayload, a pure function, for the same reason: it encoded the locked-flag policy inline where nothing could test it. That is what closes the coverage gap. The routes previously held branch logic reachable only through an authenticated request, so it went untested while the function underneath it was well covered. The logic is now pure and tested directly, including that only a graced PRIMARY selects the stamped path: a body holding just a stamp must not reset a cutover clock.
1 participant
Summary
Adds the shard-selection stage of run-id minting.
resolveMintShard(env)returns which run-ops database an environment mints its new run roots into. Resolution order is the active shard list, then a per-environment or per-organization pin, then a rendezvous hash of the environment id.Nothing changes for users on this merge.
RUN_OPS_MINT_SHARDSis unset by default, so every environment resolves to the existing store and minting behaves exactly as it does today. No caller carries the returned key into an id yet.Design
The existing gate that chooses between a cuid id and a run-ops id is untouched. The new stage runs after it, and the grace-window pattern is cloned into a separate module rather than widened, so the current flip behaviour keeps its semantics.
Placement uses rendezvous hashing, so adding a shard moves only about 1/(N+1) of environments and removing one moves only its own. Two details are load-bearing:
sha256(envId \0 key). A 32-bit score collides at our environment count, and an undetected tie would resolve by iteration order.Keys are validated against
[a-z0-9]: a key outside it cannot be stamped into an id, so a bad value fails at boot or is rejected at write rather than throwing later on the mint path.newis accepted as a pin value, which holds one organization or environment on the current id format while the rest of the fleet moves.A pin naming a shard that has left the active list falls through to the hash and reports once per environment. Honouring it would leak the drain the active list exists to perform, and throwing would fail triggers whenever a pinned shard drains.
Pins add no database queries to the trigger path: both levels live in the organization flag blob the mint call site already holds.
Where the active list lives, and why
A rolling deploy takes hours, so two pods run different values of an environment variable at the same time. A list held in the environment therefore splits the fleet for the length of the rollout: new pods place an environment on one shard while old pods place it on another. A grace window measured in seconds cannot cover that, and raising it is not an option because the same knob times the existing mint-kind flip.
So the two roles are separated:
RUN_OPS_MINT_SHARDS(environment) is a ceiling. It says which keys this deployment can mint into, and it changes only by deploy. Unset or empty means no gen-2 minting at all, with no database read.runOpsMintShardSet(global flag) is the live list, selected from the ceiling at runtime and read through a short process-wide cache.Resolution intersects the two, so a stored key this deployment cannot route is never minted into. The grace stamp is written server-side against the control-plane clock under an advisory lock, because an environment variable cannot record its own flip time and an operator cannot know a rollout's end in advance.
This creates one operational rule: a change to
RUN_OPS_MINT_SHARDSmust land across the whole fleet before the flag selects a key it adds. That is routing before minting, the same ordering the shard topology already uses.Global flag stamping, generalized
Graced stamping now covers N flag groups in one transaction under one lock. That closes a hole on the global admin flags page, which wrote any catalog key with a bare upsert: a graced key could be written with no stamp from a request body, or swept away by a save that omitted it.
applyGlobalMintKindFlipremains as a thin wrapper, so its route and its existing test are unchanged.Notes for review
Determinism is a property of the pure core for fixed inputs. The wrapper supplies the clock, the same split
effectiveMintKindalready uses. A failed read of the live list falls back to the current id format rather than guessing.Two pin keys appear in the admin flag pages immediately. They are read only from the organization override blob, so they are locked on the global page. The three list keys are the reverse: global only, and locked in the organization dialog.
test/runOpsMintShardSetFlip.test.tsneeds Postgres and was not run locally. It covers the stamp, the two-group save, lock serialization, and the admin page path.