From f2d9670d0ed9079501810771e21b3b7e601f614d Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Fri, 21 Aug 2026 17:54:43 +0100 Subject: [PATCH 01/17] feat(webapp): resolve which shard an environment mints run roots into 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. --- apps/webapp/app/env.server.ts | 27 ++ apps/webapp/app/v3/featureFlags.ts | 37 ++- .../v3/runOpsMigration/mintShardGrace.test.ts | 148 +++++++++++ .../app/v3/runOpsMigration/mintShardGrace.ts | 95 +++++++ .../runOpsMintShard.server.test.ts | 248 ++++++++++++++++++ .../runOpsMigration/runOpsMintShard.server.ts | 169 ++++++++++++ 6 files changed, 722 insertions(+), 2 deletions(-) create mode 100644 apps/webapp/app/v3/runOpsMigration/mintShardGrace.test.ts create mode 100644 apps/webapp/app/v3/runOpsMigration/mintShardGrace.ts create mode 100644 apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.ts create mode 100644 apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts diff --git a/apps/webapp/app/env.server.ts b/apps/webapp/app/env.server.ts index c9179306124..a7c572cd34e 100644 --- a/apps/webapp/app/env.server.ts +++ b/apps/webapp/app/env.server.ts @@ -4,6 +4,7 @@ import { BoolEnv } from "./utils/boolEnv"; import { isValidDatabaseUrl } from "./utils/db"; import { isValidRegex } from "./utils/regex"; import { isValidDuration } from "./services/realtime/duration.server"; +import { parseShardCsv } from "./v3/runOpsMigration/mintShardGrace"; // `z.string()` constrained to a `parseDuration`-parseable string (e.g. // `7d`, `1h`). Validated at boot so a typo'd duration fails fast. @@ -41,6 +42,23 @@ const parseMachinePresetCsv = (raw: string, ctx: z.RefinementCtx): MachinePreset return out; }; +// A CSV of gen-2 mint shard keys, validated at boot by parseShardCsv. Kept as the raw string: +// the resolution is built once in runOpsMintShard.server.ts, and this only has to fail fast. +const shardCsvString = () => + z + .string() + .default("") + .superRefine((raw, ctx) => { + try { + parseShardCsv(raw); + } catch (error) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: error instanceof Error ? error.message : "invalid shard key CSV", + }); + } + }); + const GithubAppEnvSchema = z.preprocess( (val) => { const obj = val as any; @@ -1998,6 +2016,15 @@ const EnvironmentSchema = z // (stale or fresh) resolves to the same kind for the whole window. See mintFlipGrace.ts. RUN_OPS_MINT_FLIP_GRACE_MS: z.coerce.number().int().default(90_000), + // Gen-2 mint shards — CSV of single-char [a-z0-9] keys eligible for ROOT minting. Unset or + // empty means no gen-2 minting, which is today's behaviour. Validated at boot: an invalid + // key would mint an id that cannot be routed. _PREV + _FLIPPED_AT stamp a set change so + // every process crosses the cutover together; set both, or the grace never applies. + // Removing a key stops new roots on it and never stops routing it. See mintShardGrace.ts. + RUN_OPS_MINT_SHARDS: shardCsvString(), + RUN_OPS_MINT_SHARDS_PREV: shardCsvString(), + RUN_OPS_MINT_SHARDS_FLIPPED_AT: z.string().datetime().optional(), + // Session replication (Postgres → ClickHouse sessions_v1). Shares Redis // with the runs replicator for leader locking but has its own slot and // publication so the two consume independently. diff --git a/apps/webapp/app/v3/featureFlags.ts b/apps/webapp/app/v3/featureFlags.ts index 7c775799178..740fccebb87 100644 --- a/apps/webapp/app/v3/featureFlags.ts +++ b/apps/webapp/app/v3/featureFlags.ts @@ -26,6 +26,9 @@ export const FEATURE_FLAG = { // Grace-linger stamp carried alongside runOpsMintKind on flip. See mintFlipGrace.ts. runOpsMintKindPrev: "runOpsMintKindPrev", runOpsMintKindFlippedAt: "runOpsMintKindFlippedAt", + // Gen-2 mint shard pins, read from the org override blob only. See runOpsMintShard.server.ts. + runOpsMintShard: "runOpsMintShard", + runOpsMintShardEnvPins: "runOpsMintShardEnvPins", queueMetricsUiEnabled: "queueMetricsUiEnabled", // Per-organization rollout for creating additional environment API keys. additionalApiKeysEnabled: "additionalApiKeysEnabled", @@ -89,6 +92,32 @@ export const FeatureFlagCatalog = { // by stampMintKindFlip on a genuine flip. Display-only (see ORG_LOCKED_FLAGS). [FEATURE_FLAG.runOpsMintKindPrev]: z.enum(["cuid", "runOpsId"]), [FEATURE_FLAG.runOpsMintKindFlippedAt]: z.string().datetime(), + // Pins one org to a gen-2 mint shard. "new" holds the org on gen-1 run-ops ids, which is how + // a canary keeps the fleet's default while one org moves. Only honored while the key is in + // the active set (RUN_OPS_MINT_SHARDS); a drained key falls through to the hash. + [FEATURE_FLAG.runOpsMintShard]: z + .string() + .refine((v) => v === "new" || /^[a-z0-9]$/.test(v), 'must be a single [a-z0-9] char, or "new"'), + // Per-environment pins as JSON: {"": ""}. 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))) { + fail(`"${environmentId}" must map to a single [a-z0-9] char, or "new"`); + } + } + }), // Per-org access to the Queue Metrics dashboard UI (view only; emission is global and // separate). Off unless enabled for the org. [FEATURE_FLAG.queueMetricsUiEnabled]: z.coerce.boolean(), @@ -101,11 +130,15 @@ export const FeatureFlagCatalog = { export type FeatureFlagKey = keyof typeof FeatureFlagCatalog; -// Infrastructure flags that are read-only on the global flags page. -// Shown with current/resolved value but no controls. +// Infrastructure flags, plus org-scoped-only flags, that are read-only on the global flags +// page. Shown with current/resolved value but no controls. An org-scoped-only flag belongs +// here because its resolver never reads a global row, so an editable global control would +// offer a setting that does nothing. export const GLOBAL_LOCKED_FLAGS: FeatureFlagKey[] = [ FEATURE_FLAG.defaultWorkerInstanceGroupId, FEATURE_FLAG.taskEventRepository, + FEATURE_FLAG.runOpsMintShard, + FEATURE_FLAG.runOpsMintShardEnvPins, ]; // Flags that are read-only on the org-level dialog. diff --git a/apps/webapp/app/v3/runOpsMigration/mintShardGrace.test.ts b/apps/webapp/app/v3/runOpsMigration/mintShardGrace.test.ts new file mode 100644 index 00000000000..9358685e1e8 --- /dev/null +++ b/apps/webapp/app/v3/runOpsMigration/mintShardGrace.test.ts @@ -0,0 +1,148 @@ +import { describe, expect, it } from "vitest"; +import { generateRunOpsIdV2 } from "@trigger.dev/core/v3/isomorphic"; +import { + buildMintShardResolution, + effectiveMintShardSet, + isValidPinValue, + parseShardCsv, + SHARD_KEY_PATTERN, + type MintShardSetResolution, +} from "./mintShardGrace"; + +const GRACE_MS = 90_000; +const T = 1_000_000; + +describe("parseShardCsv", () => { + it("returns an empty list for unset, empty and whitespace input", () => { + expect(parseShardCsv(undefined)).toEqual([]); + expect(parseShardCsv("")).toEqual([]); + expect(parseShardCsv(" ")).toEqual([]); + expect(parseShardCsv(",,")).toEqual([]); + }); + + it("trims, dedupes and SORTS, so operator typing order cannot change HRW", () => { + expect(parseShardCsv("b, a ,b")).toEqual(["a", "b"]); + expect(parseShardCsv("a,b,c")).toEqual(parseShardCsv("c,b,a")); + expect(parseShardCsv("b,c,a")).toEqual(parseShardCsv("a,c,b")); + }); + + it("accepts every one of the 36 legal shard keys", () => { + const all = "abcdefghijklmnopqrstuvwxyz0123456789".split(""); + expect(parseShardCsv(all.join(","))).toEqual([...all].sort()); + }); + + it("throws on a key outside [a-z0-9]", () => { + // generateRunOpsIdV2 throws on these; an unvalidated key MUST fail at boot, not at mint. + expect(() => parseShardCsv("A")).toThrow(/shard key/i); + expect(() => parseShardCsv("ab")).toThrow(/shard key/i); + expect(() => parseShardCsv("a,-")).toThrow(/shard key/i); + expect(() => parseShardCsv("a,_")).toThrow(/shard key/i); + }); + + it("rejects the reserved keys by name", () => { + expect(() => parseShardCsv("new")).toThrow(/reserved/i); + expect(() => parseShardCsv("a,legacy")).toThrow(/reserved/i); + }); +}); + +// Core does not export its shard-char pattern, so pin the local one to the real minter. +describe("shard alphabet agrees with the core minter", () => { + it("accepts exactly the characters generateRunOpsIdV2 accepts", () => { + const candidates = [ + ..."abcdefghijklmnopqrstuvwxyz0123456789".split(""), + ..."ABZ-_. +/é!".split(""), + "", + "ab", + ]; + + for (const candidate of candidates) { + let minterAccepts = true; + try { + generateRunOpsIdV2(candidate); + } catch { + minterAccepts = false; + } + + expect(SHARD_KEY_PATTERN.test(candidate)).toBe(minterAccepts); + } + }); +}); + +describe("isValidPinValue", () => { + it('accepts a shard key, and accepts "new" as the gen-1 hold value', () => { + expect(isValidPinValue("a")).toBe(true); + expect(isValidPinValue("7")).toBe(true); + expect(isValidPinValue("new")).toBe(true); + }); + + it("rejects legacy, and rejects anything outside the alphabet", () => { + expect(isValidPinValue("legacy")).toBe(false); + expect(isValidPinValue("A")).toBe(false); + expect(isValidPinValue("ab")).toBe(false); + expect(isValidPinValue("")).toBe(false); + }); +}); + +describe("effectiveMintShardSet", () => { + it("returns set when there is no stamp", () => { + const r: MintShardSetResolution = { set: ["a", "b"] }; + expect(effectiveMintShardSet(r, T, GRACE_MS)).toEqual(["a", "b"]); + }); + + it("returns set when flippedAtMs is absent even though prevSet is present", () => { + const r: MintShardSetResolution = { set: ["a", "b"], prevSet: ["a"] }; + expect(effectiveMintShardSet(r, T, GRACE_MS)).toEqual(["a", "b"]); + }); + + it("serves prevSet inside the window and set at/after the boundary", () => { + const r: MintShardSetResolution = { set: ["a", "b"], prevSet: ["a"], flippedAtMs: T }; + expect(effectiveMintShardSet(r, T, GRACE_MS)).toEqual(["a"]); + expect(effectiveMintShardSet(r, T + GRACE_MS - 1, GRACE_MS)).toEqual(["a"]); + // Boundary is exclusive on the prev side, so every process crosses it together. + expect(effectiveMintShardSet(r, T + GRACE_MS, GRACE_MS)).toEqual(["a", "b"]); + expect(effectiveMintShardSet(r, T + GRACE_MS + 1, GRACE_MS)).toEqual(["a", "b"]); + }); + + it("represents a graced first activation as an empty prevSet", () => { + const r: MintShardSetResolution = { set: ["a"], prevSet: [], flippedAtMs: T }; + expect(effectiveMintShardSet(r, T, GRACE_MS)).toEqual([]); + expect(effectiveMintShardSet(r, T + GRACE_MS, GRACE_MS)).toEqual(["a"]); + }); + + it("serves a drain through the window", () => { + const r: MintShardSetResolution = { set: ["a"], prevSet: ["a", "b"], flippedAtMs: T }; + expect(effectiveMintShardSet(r, T + 1, GRACE_MS)).toEqual(["a", "b"]); + expect(effectiveMintShardSet(r, T + GRACE_MS, GRACE_MS)).toEqual(["a"]); + }); +}); + +describe("buildMintShardResolution", () => { + it("omits prevSet entirely when no flip timestamp is configured", () => { + // A prevSet with no timestamp can never apply, so it MUST NOT linger. + const r = buildMintShardResolution({ shards: "a,b", prev: "a", flippedAt: undefined }); + expect(r).toEqual({ set: ["a", "b"], prevSet: undefined, flippedAtMs: undefined }); + }); + + it("keeps an empty prevSet when a flip timestamp IS configured", () => { + const r = buildMintShardResolution({ + shards: "a", + prev: "", + flippedAt: new Date(T).toISOString(), + }); + expect(r).toEqual({ set: ["a"], prevSet: [], flippedAtMs: T }); + }); + + it("parses the flip timestamp and sorts both lists", () => { + const r = buildMintShardResolution({ + shards: "b,a", + prev: "c,a", + flippedAt: new Date(T).toISOString(), + }); + expect(r).toEqual({ set: ["a", "b"], prevSet: ["a", "c"], flippedAtMs: T }); + }); + + it("treats an unparseable timestamp as no stamp at all", () => { + const r = buildMintShardResolution({ shards: "a", prev: "b", flippedAt: "not-a-date" }); + expect(r).toEqual({ set: ["a"], prevSet: undefined, flippedAtMs: undefined }); + }); +}); diff --git a/apps/webapp/app/v3/runOpsMigration/mintShardGrace.ts b/apps/webapp/app/v3/runOpsMigration/mintShardGrace.ts new file mode 100644 index 00000000000..bfa3139cc25 --- /dev/null +++ b/apps/webapp/app/v3/runOpsMigration/mintShardGrace.ts @@ -0,0 +1,95 @@ +import type { ShardKey } from "@trigger.dev/core/v3/isomorphic"; + +// Index 24 of a gen-2 id sits inside the pod name `runner-`, and a DNS-1123 label accepts +// lowercase only, so the alphabet is 36 keys and no wider. Core keeps its copy private; +// mintShardGrace.test.ts pins this pattern to generateRunOpsIdV2 instead. +export const SHARD_KEY_PATTERN = /^[a-z0-9]$/; + +// Neither may enter the active set: "new" already means "mint a gen-1 run-ops id" and +// "legacy" means the cuid store, which minting never selects. +const RESERVED_SHARD_KEYS: readonly string[] = ["new", "legacy"]; + +// "new" IS legal as a PIN, 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. +export const GEN_1_PIN_VALUE = "new"; + +export type MintShardSetResolution = { + set: string[]; + prevSet?: string[]; + flippedAtMs?: number; +}; + +export function isValidPinValue(value: unknown): value is ShardKey { + if (typeof value !== "string") return false; + return value === GEN_1_PIN_VALUE || SHARD_KEY_PATTERN.test(value); +} + +// Throws rather than dropping a bad key: generateRunOpsIdV2 throws on an out-of-alphabet char, +// so an unvalidated key must fail at boot and never at mint. +export function parseShardCsv(raw: string | undefined | null): string[] { + const keys = (raw ?? "") + .split(",") + .map((s) => s.trim()) + .filter(Boolean); + + const unique = new Set(); + for (const key of keys) { + if (RESERVED_SHARD_KEYS.includes(key)) { + throw new Error(`"${key}" is a reserved key and cannot be an active mint shard`); + } + if (!SHARD_KEY_PATTERN.test(key)) { + throw new Error(`invalid shard key "${key}": must be a single char in [a-z0-9]`); + } + unique.add(key); + } + + // Sorted so no placement can depend on the order an operator typed the CSV in. + return [...unique].sort(); +} + +// Cutover boundary, mirroring effectiveMintKind. `nowMs` is the reader's wall clock while +// `flippedAtMs` is operator-supplied, so this assumes NTP-synced hosts with skew << graceMs, +// letting every process cross [flippedAtMs, flippedAtMs + graceMs) together (OLD then NEW). +export function effectiveMintShardSet( + r: MintShardSetResolution, + nowMs: number, + graceMs: number +): string[] { + if (r.prevSet === undefined || r.flippedAtMs === undefined) { + return r.set; + } + return nowMs < r.flippedAtMs + graceMs ? r.prevSet : r.set; +} + +// A prevSet with no timestamp can never apply, so it is dropped. A timestamp with an EMPTY +// prevSet is meaningful: it graces a first activation, serving no shards for the window. +export function buildMintShardResolution(source: { + shards: string | undefined; + prev: string | undefined; + flippedAt: string | undefined; +}): MintShardSetResolution { + const parsed = source.flippedAt !== undefined ? Date.parse(source.flippedAt) : NaN; + const flippedAtMs = Number.isNaN(parsed) ? undefined : parsed; + + return { + set: parseShardCsv(source.shards), + prevSet: flippedAtMs === undefined ? undefined : parseShardCsv(source.prev), + flippedAtMs, + }; +} + +// Returns a message to log when the stamp is half-configured, otherwise undefined. Stays quiet +// while the active set is empty, so an unconfigured deployment logs nothing at boot. +export function mintShardStampWarning(source: { + shards: string | undefined; + prev: string | undefined; + flippedAt: string | undefined; +}): string | undefined { + if (parseShardCsv(source.shards).length === 0) { + return undefined; + } + if (parseShardCsv(source.prev).length > 0 && source.flippedAt === undefined) { + return "RUN_OPS_MINT_SHARDS_PREV is set but RUN_OPS_MINT_SHARDS_FLIPPED_AT is not; the shard-set grace window will never apply"; + } + return undefined; +} diff --git a/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.ts b/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.ts new file mode 100644 index 00000000000..806a0c1aea8 --- /dev/null +++ b/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.ts @@ -0,0 +1,248 @@ +import { describe, expect, it } from "vitest"; +import { computeMintShard, type MintShardDeps } from "./runOpsMintShard.server"; +import { type MintShardSetResolution } from "./mintShardGrace"; + +const GRACE_MS = 90_000; +const T = 1_000_000; + +// Cuid-shaped ids, not sequential integers: a sequential space does not model the real +// key distribution the hash has to spread. +function envIds(count: number): string[] { + const ids: string[] = []; + for (let i = 0; i < count; i++) { + ids.push(`cm${(i * 2654435761).toString(36).padStart(10, "0")}${i.toString(36)}zzq`); + } + return ids; +} + +function deps( + resolution: MintShardSetResolution, + overrides: Partial = {} +): MintShardDeps { + return { + resolution, + nowMs: T + GRACE_MS + 1, + graceMs: GRACE_MS, + orgFeatureFlags: undefined, + ...overrides, + }; +} + +function orgFlags(flags: Record) { + return { orgFeatureFlags: flags }; +} + +function place(ids: string[], resolution: MintShardSetResolution): Map { + const out = new Map(); + for (const id of ids) { + out.set(id, computeMintShard({ id }, deps(resolution))); + } + return out; +} + +describe("computeMintShard — the no-shards answer", () => { + it("returns new when the active set is unset", () => { + expect(computeMintShard({ id: "env_1" }, deps({ set: [] }))).toBe("new"); + }); + + it("returns new when the active set is empty even with a stale stamp present", () => { + const resolution: MintShardSetResolution = { set: [], prevSet: ["a"], flippedAtMs: T }; + // The empty check MUST run before the grace, so an unset set is an unconditional kill switch. + expect(computeMintShard({ id: "env_1" }, deps(resolution, { nowMs: T + 1 }))).toBe("new"); + }); + + it("returns new when the grace serves an empty prevSet", () => { + const resolution: MintShardSetResolution = { set: ["a"], prevSet: [], flippedAtMs: T }; + expect(computeMintShard({ id: "env_1" }, deps(resolution, { nowMs: T + 1 }))).toBe("new"); + }); +}); + +describe("computeMintShard — determinism", () => { + it("returns the same value for the same environment on every call", () => { + const resolution: MintShardSetResolution = { set: ["a", "b", "c"] }; + const first = computeMintShard({ id: "env_stable" }, deps(resolution)); + for (let i = 0; i < 1000; i++) { + expect(computeMintShard({ id: "env_stable" }, deps(resolution))).toBe(first); + } + }); + + it("ignores the order the operator listed the keys in", () => { + const ids = envIds(200); + const canonical = place(ids, { set: ["a", "b", "c"] }); + for (const permutation of [ + ["c", "b", "a"], + ["b", "a", "c"], + ["a", "c", "b"], + ]) { + expect(place(ids, { set: permutation })).toEqual(canonical); + } + }); +}); + +describe("computeMintShard — pins", () => { + const resolution: MintShardSetResolution = { set: ["a", "b"] }; + + it("lets a per-env pin override the hash", () => { + const ids = envIds(50); + for (const id of ids) { + const pinned = computeMintShard( + { id }, + deps(resolution, orgFlags({ runOpsMintShardEnvPins: JSON.stringify({ [id]: "b" }) })) + ); + expect(pinned).toBe("b"); + } + }); + + it("lets a per-org pin override the hash when no per-env pin is set", () => { + const ids = envIds(50); + for (const id of ids) { + expect(computeMintShard({ id }, deps(resolution, orgFlags({ runOpsMintShard: "a" })))).toBe( + "a" + ); + } + }); + + it("lets a per-env pin beat a per-org pin", () => { + const result = computeMintShard( + { id: "env_1" }, + deps( + resolution, + orgFlags({ + runOpsMintShard: "a", + runOpsMintShardEnvPins: JSON.stringify({ env_1: "b" }), + }) + ) + ); + expect(result).toBe("b"); + }); + + it("holds an environment on gen-1 when the pin is new", () => { + expect( + computeMintShard({ id: "env_1" }, deps(resolution, orgFlags({ runOpsMintShard: "new" }))) + ).toBe("new"); + expect( + computeMintShard( + { id: "env_1" }, + deps(resolution, orgFlags({ runOpsMintShardEnvPins: JSON.stringify({ env_1: "new" }) })) + ) + ).toBe("new"); + }); + + it("falls through to the hash and reports when the pin is outside the active set", () => { + // Honouring a drained pin would leak the drain; throwing would fail customer triggers. + const rejected: string[] = []; + const result = computeMintShard( + { id: "env_1" }, + deps(resolution, { + ...orgFlags({ runOpsMintShard: "z" }), + onPinRejected: (info) => rejected.push(info.pin), + }) + ); + expect(result).toBe(computeMintShard({ id: "env_1" }, deps(resolution))); + expect(rejected).toEqual(["z"]); + }); + + it("honours a pin to a drained key for the whole grace window, then falls through", () => { + const draining: MintShardSetResolution = { set: ["a"], prevSet: ["a", "b"], flippedAtMs: T }; + const pinnedToB = orgFlags({ runOpsMintShard: "b" }); + expect(computeMintShard({ id: "env_1" }, deps(draining, { ...pinnedToB, nowMs: T + 1 }))).toBe( + "b" + ); + expect( + computeMintShard({ id: "env_1" }, deps(draining, { ...pinnedToB, nowMs: T + GRACE_MS })) + ).not.toBe("b"); + }); + + it("ignores an unparseable pin blob rather than un-pinning silently", () => { + const result = computeMintShard( + { id: "env_1" }, + deps(resolution, orgFlags({ runOpsMintShard: "a", runOpsMintShardEnvPins: "{not json" })) + ); + expect(result).toBe("a"); + }); + + it("falls back to the org pin when the blob holds an invalid value for this env", () => { + const result = computeMintShard( + { id: "env_1" }, + deps( + resolution, + orgFlags({ + runOpsMintShard: "a", + runOpsMintShardEnvPins: JSON.stringify({ env_1: "LEGACY" }), + }) + ) + ); + expect(result).toBe("a"); + }); + + it("ignores an invalid org pin value", () => { + const result = computeMintShard( + { id: "env_1" }, + deps(resolution, orgFlags({ runOpsMintShard: "legacy" })) + ); + expect(result).toBe(computeMintShard({ id: "env_1" }, deps(resolution))); + }); +}); + +describe("computeMintShard — rendezvous properties", () => { + const ids = envIds(10_000); + + it("spreads roughly evenly across the active set", () => { + for (const set of [ + ["a", "b"], + ["a", "b", "c"], + ["a", "b", "c", "d"], + ]) { + const counts = new Map(); + for (const shard of place(ids, { set }).values()) { + counts.set(shard, (counts.get(shard) ?? 0) + 1); + } + expect(counts.size).toBe(set.length); + const expected = ids.length / set.length; + for (const count of counts.values()) { + expect(Math.abs(count - expected) / expected).toBeLessThan(0.1); + } + } + }); + + it("moves about 1/(N+1) of environments when a shard is added", () => { + const cases: Array<{ from: string[]; to: string[]; expected: number }> = [ + { from: ["a"], to: ["a", "b"], expected: 1 / 2 }, + { from: ["a", "b"], to: ["a", "b", "c"], expected: 1 / 3 }, + { from: ["a", "b", "c"], to: ["a", "b", "c", "d"], expected: 1 / 4 }, + ]; + + for (const { from, to, expected } of cases) { + const before = place(ids, { set: from }); + const after = place(ids, { set: to }); + const added = to.filter((k) => !from.includes(k)); + let moved = 0; + for (const id of ids) { + if (before.get(id) === after.get(id)) continue; + moved++; + // HRW's defining property: a mover lands on the ADDED shard, never on a survivor. + expect(added).toContain(after.get(id)); + } + expect(Math.abs(moved / ids.length - expected) / expected).toBeLessThan(0.1); + } + }); + + it("moves only the environments that hashed to a removed shard", () => { + const before = place(ids, { set: ["a", "b", "c"] }); + const after = place(ids, { set: ["a", "b"] }); + for (const id of ids) { + if (before.get(id) === "c") { + expect(after.get(id)).not.toBe("c"); + } else { + expect(after.get(id)).toBe(before.get(id)); + } + } + }); + + it("also moves pinned environments when their shard is removed", () => { + // Criterion 6 is a property of the hash only. A pin to a removed key moves too. + const pinnedToC = orgFlags({ runOpsMintShard: "c" }); + expect(computeMintShard({ id: "env_1" }, deps({ set: ["a", "b", "c"] }, pinnedToC))).toBe("c"); + expect(computeMintShard({ id: "env_1" }, deps({ set: ["a", "b"] }, pinnedToC))).not.toBe("c"); + }); +}); diff --git a/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts b/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts new file mode 100644 index 00000000000..20f20fcf944 --- /dev/null +++ b/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts @@ -0,0 +1,169 @@ +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"; + +export type MintShardDeps = { + resolution: MintShardSetResolution; + nowMs: number; + graceMs: number; + orgFeatureFlags: unknown; + onPinRejected?: (info: { environmentId: string; pin: string; activeSet: string[] }) => void; +}; + +function asRecord(value: unknown): Record | undefined { + if (!value || typeof value !== "object" || Array.isArray(value)) return undefined; + return value as Record; +} + +// Map keys are environment INTERNAL ids (cuids), not friendly ids. An unparseable blob, or a +// blob whose value for this environment is invalid, yields no per-env pin and lets the +// per-org scalar decide — never a silent un-pin straight to the hash. +function readEnvPin(raw: unknown, environmentId: string): ShardKey | undefined { + if (typeof raw !== "string") return undefined; + + let parsed: unknown; + try { + parsed = JSON.parse(raw); + } catch { + return undefined; + } + + const pins = asRecord(parsed); + const pin = pins?.[environmentId]; + return isValidPinValue(pin) ? pin : undefined; +} + +// Both pins live in the org override blob the trigger path already holds, so resolving a mint +// shard costs no query. +function readPin(orgFeatureFlags: unknown, environmentId: string): ShardKey | undefined { + const blob = asRecord(orgFeatureFlags); + if (!blob) return undefined; + + const envPin = readEnvPin(blob[FEATURE_FLAG.runOpsMintShardEnvPins], environmentId); + if (envPin !== undefined) return envPin; + + const scalar = blob[FEATURE_FLAG.runOpsMintShard]; + return isValidPinValue(scalar) ? scalar : undefined; +} + +// 64 bits: a 32-bit score collides at this system's environment count, and an undetected tie +// would resolve by iteration order. The NUL separates the fields so no two input pairs can +// concatenate alike. This hash input is FROZEN once gen-2 minting is live: changing it +// re-places every environment, silently. +function shardScore(environmentId: string, key: string): bigint { + return createHash("sha256").update(`${environmentId}\0${key}`).digest().readBigUInt64BE(0); +} + +function hrwSelect(environmentId: string, activeSet: string[]): string { + let bestKey = activeSet[0]; + let bestScore = shardScore(environmentId, bestKey); + + for (let i = 1; i < activeSet.length; i++) { + const key = activeSet[i]; + const score = shardScore(environmentId, key); + if (score > bestScore || (score === bestScore && key > bestKey)) { + bestKey = key; + bestScore = score; + } + } + + return bestKey; +} + +// PURE CORE — no env, no clock, no I/O; tests drive this directly. Deterministic for fixed +// deps, which is what lets run minting and token minting agree on one answer. +// +// The empty-set check runs BEFORE the grace, so an unset active list is an unconditional kill +// switch that a stale stamp cannot reopen. A pin outside the active set falls through to the +// hash rather than throwing: honouring it would leak the drain the active list performs, and +// throwing would fail customer triggers whenever a pinned shard drains. +export function computeMintShard(environment: { id: string }, deps: MintShardDeps): ShardKey { + if (deps.resolution.set.length === 0) { + return "new"; + } + + const activeSet = effectiveMintShardSet(deps.resolution, deps.nowMs, deps.graceMs); + if (activeSet.length === 0) { + return "new"; + } + + const pin = readPin(deps.orgFeatureFlags, environment.id); + if (pin !== undefined) { + if (pin === GEN_1_PIN_VALUE) { + return "new"; + } + if (activeSet.includes(pin)) { + return pin; + } + deps.onPinRejected?.({ environmentId: environment.id, pin, activeSet }); + } + + return hrwSelect(environment.id, activeSet); +} + +// ENV-BOUND wrapper — the only place env is read. The resolution is built once: these are +// deploy-time values, so re-parsing per mint would burn CPU on the hottest path in the system. +const shardResolution: MintShardSetResolution = buildMintShardResolution({ + shards: env.RUN_OPS_MINT_SHARDS, + prev: env.RUN_OPS_MINT_SHARDS_PREV, + flippedAt: env.RUN_OPS_MINT_SHARDS_FLIPPED_AT, +}); + +const stampWarning = mintShardStampWarning({ + shards: env.RUN_OPS_MINT_SHARDS, + prev: env.RUN_OPS_MINT_SHARDS_PREV, + flippedAt: env.RUN_OPS_MINT_SHARDS_FLIPPED_AT, +}); +if (stampWarning) { + logger.warn(`[runOpsMintShard] ${stampWarning}`, { + RUN_OPS_MINT_SHARDS: env.RUN_OPS_MINT_SHARDS, + RUN_OPS_MINT_SHARDS_PREV: env.RUN_OPS_MINT_SHARDS_PREV, + }); +} + +// Once per environment per process: a stale pin sits on the root-trigger path and would +// otherwise log on every trigger for that environment, indefinitely. +const reportedPins = new Set(); + +function reportPinRejected(info: { + environmentId: string; + pin: string; + activeSet: string[]; +}): void { + if (reportedPins.has(info.environmentId)) return; + reportedPins.add(info.environmentId); + logger.error("[runOpsMintShard] pinned shard is not in the active set; using the hash", info); +} + +/** + * Which shard an environment mints new roots into. Call only after resolveRunIdMintKind has + * returned "runOpsId". Returns "new" to mean a gen-1 run-ops id, which is today's behaviour. + * + * Async despite doing no I/O, so the deploy-free active-set layer can add a read later without + * changing every call site. + * + * @knipignore the gen-2 write-path change is the first production caller; drop this tag there. + */ +export async function resolveMintShard(environment: { + id: string; + // Pass environment.organization.featureFlags from the trigger call site. + orgFeatureFlags?: unknown; +}): Promise { + return computeMintShard(environment, { + resolution: shardResolution, + nowMs: Date.now(), + graceMs: env.RUN_OPS_MINT_FLIP_GRACE_MS, + orgFeatureFlags: environment.orgFeatureFlags, + onPinRejected: reportPinRejected, + }); +} From c84b03db4455ffd026c8ab863001b92313bb794e Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Mon, 24 Aug 2026 09:42:26 +0100 Subject: [PATCH 02/17] fix(webapp): hold the active mint-shard list in the database, not the 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. --- apps/webapp/app/env.server.ts | 12 +- .../app/routes/admin.api.v1.feature-flags.ts | 11 +- .../webapp/app/routes/admin.feature-flags.tsx | 45 +---- apps/webapp/app/v3/featureFlags.server.ts | 124 ++++++++++-- apps/webapp/app/v3/featureFlags.ts | 24 +++ .../v3/runOpsMigration/mintShardGrace.test.ts | 122 +++++++++-- .../app/v3/runOpsMigration/mintShardGrace.ts | 82 ++++++-- .../runOpsMintShard.server.test.ts | 59 +++++- .../runOpsMigration/runOpsMintShard.server.ts | 92 ++++++--- .../test/runOpsMintShardSetFlip.test.ts | 190 ++++++++++++++++++ 10 files changed, 623 insertions(+), 138 deletions(-) create mode 100644 apps/webapp/test/runOpsMintShardSetFlip.test.ts diff --git a/apps/webapp/app/env.server.ts b/apps/webapp/app/env.server.ts index a7c572cd34e..851454acb2a 100644 --- a/apps/webapp/app/env.server.ts +++ b/apps/webapp/app/env.server.ts @@ -2016,14 +2016,12 @@ const EnvironmentSchema = z // (stale or fresh) resolves to the same kind for the whole window. See mintFlipGrace.ts. RUN_OPS_MINT_FLIP_GRACE_MS: z.coerce.number().int().default(90_000), - // Gen-2 mint shards — CSV of single-char [a-z0-9] keys eligible for ROOT minting. Unset or - // empty means no gen-2 minting, which is today's behaviour. Validated at boot: an invalid - // key would mint an id that cannot be routed. _PREV + _FLIPPED_AT stamp a set change so - // every process crosses the cutover together; set both, or the grace never applies. - // Removing a key stops new roots on it and never stops routing it. See mintShardGrace.ts. + // Gen-2 mint shards — CSV of single-char [a-z0-9] keys this deployment can mint roots into. + // Unset or empty means no gen-2 minting, which is today's behaviour. Validated at boot: an + // invalid key would mint an id that cannot be routed. This is a CEILING, not the live list: + // it changes only by deploy, and the runOpsMintShardSet flag selects from it at runtime. + // A rolling deploy runs two values of this var at once, so it must never be the ramp lever. RUN_OPS_MINT_SHARDS: shardCsvString(), - RUN_OPS_MINT_SHARDS_PREV: shardCsvString(), - RUN_OPS_MINT_SHARDS_FLIPPED_AT: z.string().datetime().optional(), // Session replication (Postgres → ClickHouse sessions_v1). Shares Redis // with the runs replicator for leader locking but has its own slot and diff --git a/apps/webapp/app/routes/admin.api.v1.feature-flags.ts b/apps/webapp/app/routes/admin.api.v1.feature-flags.ts index 8cd4f77873e..ad5d51592f4 100644 --- a/apps/webapp/app/routes/admin.api.v1.feature-flags.ts +++ b/apps/webapp/app/routes/admin.api.v1.feature-flags.ts @@ -3,7 +3,7 @@ import { json } from "@remix-run/server-runtime"; import { prisma } from "~/db.server"; import { env } from "~/env.server"; import { requireAdminApiRequest } from "~/services/personalAccessToken.server"; -import { applyGlobalMintKindFlip, makeSetMultipleFlags } from "~/v3/featureFlags.server"; +import { applyGlobalGracedFlips, makeSetMultipleFlags } from "~/v3/featureFlags.server"; import { validatePartialFeatureFlags } from "~/v3/featureFlags"; export async function action({ request }: ActionFunctionArgs) { @@ -29,14 +29,15 @@ export async function action({ request }: ActionFunctionArgs) { const { runOpsMintKindPrev: _ignoredPrev, runOpsMintKindFlippedAt: _ignoredFlippedAt, + runOpsMintShardSetPrev: _ignoredSetPrev, + runOpsMintShardSetFlippedAt: _ignoredSetFlippedAt, ...requestedFlags } = validationResult.data; - // A global mint-kind flip stamps its grace window under a lock (applyGlobalMintKindFlip); - // any other flag save writes directly. + // A change to a graced group stamps its window under a lock; any other save writes directly. const updatedFlags = - requestedFlags.runOpsMintKind !== undefined - ? await applyGlobalMintKindFlip(prisma, requestedFlags, env.RUN_OPS_MINT_FLIP_GRACE_MS) + requestedFlags.runOpsMintKind !== undefined || requestedFlags.runOpsMintShardSet !== undefined + ? await applyGlobalGracedFlips(prisma, requestedFlags, env.RUN_OPS_MINT_FLIP_GRACE_MS) : await makeSetMultipleFlags(prisma)(requestedFlags); return json({ diff --git a/apps/webapp/app/routes/admin.feature-flags.tsx b/apps/webapp/app/routes/admin.feature-flags.tsx index be0f6174622..0b52d324dc1 100644 --- a/apps/webapp/app/routes/admin.feature-flags.tsx +++ b/apps/webapp/app/routes/admin.feature-flags.tsx @@ -5,17 +5,18 @@ import { json } from "@remix-run/server-runtime"; import { typedjson, useTypedLoaderData } from "remix-typedjson"; import { z } from "zod"; import { LockClosedIcon } from "@heroicons/react/20/solid"; -import { boundedIn, prisma } from "~/db.server"; +import { prisma } from "~/db.server"; import { env } from "~/env.server"; import { dashboardAction, dashboardLoader } from "~/services/routeBuilders/dashboardBuilder"; import { FEATURE_FLAG, GLOBAL_LOCKED_FLAGS, + type FeatureFlagKey, type FlagControlType, getAllFlagControlTypes, validatePartialFeatureFlags, } from "~/v3/featureFlags"; -import { flags as getGlobalFlags } from "~/v3/featureFlags.server"; +import { flags as getGlobalFlags, replaceGlobalFeatureFlags } from "~/v3/featureFlags.server"; import { featuresForRequest } from "~/features.server"; import { Button } from "~/components/primitives/Buttons"; import { Callout } from "~/components/primitives/Callout"; @@ -116,39 +117,15 @@ export const action = dashboardAction( ); } - const validatedFlags = validationResult.data as Record; - const controlTypes = getAllFlagControlTypes(); - const catalogKeys = Object.keys(controlTypes); - - const keysToDelete: string[] = []; - const upsertOps: ReturnType[] = []; - - for (const key of catalogKeys) { - if (key in validatedFlags) { - upsertOps.push( - prisma.featureFlag.upsert({ - where: { key }, - create: { key, value: validatedFlags[key] as any }, - update: { value: validatedFlags[key] as any }, - }) - ); - } else { - // On cloud, never delete locked flags (they're not in the payload - // because the UI doesn't include them). Locally, delete everything - // the user didn't include - full control. - const isProtected = isManagedCloud && GLOBAL_LOCKED_FLAGS.includes(key); - if (!isProtected) { - keysToDelete.push(key); - } - } - } + const catalogKeys = Object.keys(getAllFlagControlTypes()) as FeatureFlagKey[]; - await prisma.$transaction([ - ...upsertOps, - ...(keysToDelete.length > 0 - ? [prisma.featureFlag.deleteMany({ where: { key: { in: boundedIn(keysToDelete) } } })] - : []), - ]); + await replaceGlobalFeatureFlags(prisma, { + requestedFlags: validationResult.data, + catalogKeys, + // On cloud, never delete locked flags (the UI omits them). Locally, full control. + isProtected: (key) => isManagedCloud && GLOBAL_LOCKED_FLAGS.includes(key), + graceMs: env.RUN_OPS_MINT_FLIP_GRACE_MS, + }); return json({ success: true }); } diff --git a/apps/webapp/app/v3/featureFlags.server.ts b/apps/webapp/app/v3/featureFlags.server.ts index b32a4578640..e30b5df4cec 100644 --- a/apps/webapp/app/v3/featureFlags.server.ts +++ b/apps/webapp/app/v3/featureFlags.server.ts @@ -8,6 +8,8 @@ import { FeatureFlagCatalog, } from "~/v3/featureFlags"; import { stampMintKindFlip } from "~/v3/runOpsMigration/mintFlipGrace"; +import { stampMintShardSetFlip } from "~/v3/runOpsMigration/mintShardGrace"; +import { boundedIn } from "~/db.server"; export type FlagsOptions = { key: T; @@ -182,24 +184,41 @@ export function makeSetMultipleFlags(_prisma: PrismaClientOrTransaction = prisma // Read -> stamp -> write the global mint-kind grace metadata in one transaction. The three // FeatureFlag rows may not exist yet, so a row FOR UPDATE can't lock them; an advisory xact lock // serializes concurrent global flips so one can't clobber another's grace stamp (mirrors per-org). -export async function applyGlobalMintKindFlip( +// Every group of global flags whose value carries its own grace stamp. One transaction and one +// lock cover all of them, so a save that flips two groups can never stamp one and lose the other. +const GRACED_GLOBAL_GROUPS = [ + { + keys: [ + FEATURE_FLAG.runOpsMintKind, + FEATURE_FLAG.runOpsMintKindPrev, + FEATURE_FLAG.runOpsMintKindFlippedAt, + ] as FeatureFlagKey[], + stamp: stampMintKindFlip, + }, + { + keys: [ + FEATURE_FLAG.runOpsMintShardSet, + FEATURE_FLAG.runOpsMintShardSetPrev, + FEATURE_FLAG.runOpsMintShardSetFlippedAt, + ] as FeatureFlagKey[], + stamp: stampMintShardSetFlip, + }, +] as const; + +// Keys the graced path owns. They never take a bare upsert and never enter the replace sweep, +// because a server-computed stamp must not be written from a request body nor swept away. +const GRACED_GLOBAL_KEYS: FeatureFlagKey[] = GRACED_GLOBAL_GROUPS.flatMap((g) => g.keys); + +export async function applyGlobalGracedFlips( client: PrismaClient, requestedFlags: Partial>, graceMs: number ): Promise<{ key: string; value: any }[]> { return client.$transaction(async (tx) => { - await tx.$executeRaw`SELECT pg_advisory_xact_lock(hashtext('runops-global-mint-kind-flip'))`; + await tx.$executeRaw`SELECT pg_advisory_xact_lock(hashtext('runops-global-graced-flag-flip'))`; const existingRows = await tx.featureFlag.findMany({ - where: { - key: { - in: [ - FEATURE_FLAG.runOpsMintKind, - FEATURE_FLAG.runOpsMintKindPrev, - FEATURE_FLAG.runOpsMintKindFlippedAt, - ], - }, - }, + where: { key: { in: GRACED_GLOBAL_KEYS } }, select: { key: true, value: true }, }); const existingGlobal: Record = {}; @@ -207,16 +226,83 @@ export async function applyGlobalMintKindFlip( existingGlobal[row.key] = row.value; } - // Anchor the cutover to the control-plane DB clock, not this process's wall clock. + // Anchor the cutover to the control-plane DB clock, not this process's wall clock. A rolling + // deploy spans hours, so every pod must date the window against one shared clock. const [{ now }] = await tx.$queryRaw<{ now: Date }[]>`SELECT now() AS now`; - const stamped = stampMintKindFlip( - existingGlobal, - { ...requestedFlags }, - now.getTime(), - graceMs - ) as Partial>; + let stamped: Record = { ...requestedFlags }; + for (const group of GRACED_GLOBAL_GROUPS) { + stamped = group.stamp(existingGlobal, stamped, now.getTime(), graceMs); + } - return makeSetMultipleFlags(tx)(stamped); + return makeSetMultipleFlags(tx)(stamped as Partial>); }); } + +/** @deprecated Prefer applyGlobalGracedFlips, which stamps every graced group in one lock. */ +export async function applyGlobalMintKindFlip( + client: PrismaClient, + requestedFlags: Partial>, + graceMs: number +): Promise<{ key: string; value: any }[]> { + return applyGlobalGracedFlips(client, requestedFlags, graceMs); +} + +// Replace-semantics write for the global admin flags page: upsert submitted catalog flags, delete +// omitted ones unless protected, and route any graced group through the stamped path. +export async function replaceGlobalFeatureFlags( + client: PrismaClient, + params: { + requestedFlags: Partial>; + catalogKeys: FeatureFlagKey[]; + isProtected: (key: FeatureFlagKey) => boolean; + graceMs: number; + } +): Promise { + // Derived stamp fields are computed server-side; never trust them from the body. + const requestedFlags: Record = { ...params.requestedFlags }; + for (const group of GRACED_GLOBAL_GROUPS) { + for (const derived of group.keys.slice(1)) { + delete requestedFlags[derived]; + } + } + + const touchesGracedGroup = GRACED_GLOBAL_GROUPS.some( + (group) => requestedFlags[group.keys[0]] !== undefined + ); + if (touchesGracedGroup) { + await applyGlobalGracedFlips( + client, + requestedFlags as Partial>, + params.graceMs + ); + } + + const upsertOps: ReturnType[] = []; + const keysToDelete: string[] = []; + + for (const key of params.catalogKeys) { + if (GRACED_GLOBAL_KEYS.includes(key)) { + continue; + } + if (key in requestedFlags) { + const value = requestedFlags[key]; + upsertOps.push( + client.featureFlag.upsert({ + where: { key }, + create: { key, value: value as any }, + update: { value: value as any }, + }) + ); + } else if (!params.isProtected(key)) { + keysToDelete.push(key); + } + } + + await client.$transaction([ + ...upsertOps, + ...(keysToDelete.length > 0 + ? [client.featureFlag.deleteMany({ where: { key: { in: boundedIn(keysToDelete) } } })] + : []), + ]); +} diff --git a/apps/webapp/app/v3/featureFlags.ts b/apps/webapp/app/v3/featureFlags.ts index 740fccebb87..4dcf7ab7743 100644 --- a/apps/webapp/app/v3/featureFlags.ts +++ b/apps/webapp/app/v3/featureFlags.ts @@ -29,6 +29,11 @@ export const FEATURE_FLAG = { // Gen-2 mint shard pins, read from the org override blob only. See runOpsMintShard.server.ts. runOpsMintShard: "runOpsMintShard", runOpsMintShardEnvPins: "runOpsMintShardEnvPins", + // The active mint-shard list, global only. Lives here rather than in the environment because a + // rolling deploy runs two environment values at once for hours. See mintShardGrace.ts. + runOpsMintShardSet: "runOpsMintShardSet", + runOpsMintShardSetPrev: "runOpsMintShardSetPrev", + runOpsMintShardSetFlippedAt: "runOpsMintShardSetFlippedAt", queueMetricsUiEnabled: "queueMetricsUiEnabled", // Per-organization rollout for creating additional environment API keys. additionalApiKeysEnabled: "additionalApiKeysEnabled", @@ -118,6 +123,21 @@ export const FeatureFlagCatalog = { } } }), + // 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" + ), + // Grace stamp: the previously-effective list and the flip time, written by + // stampMintShardSetFlip on a genuine change. Display-only (see ORG_LOCKED_FLAGS). + [FEATURE_FLAG.runOpsMintShardSetPrev]: z.string(), + [FEATURE_FLAG.runOpsMintShardSetFlippedAt]: z.string().datetime(), // Per-org access to the Queue Metrics dashboard UI (view only; emission is global and // separate). Off unless enabled for the org. [FEATURE_FLAG.queueMetricsUiEnabled]: z.coerce.boolean(), @@ -151,6 +171,10 @@ export const ORG_LOCKED_FLAGS: FeatureFlagKey[] = [ // System-wide only — orgs must not be able to override these kill switches. FEATURE_FLAG.additionalApiKeyIssuanceEnabled, FEATURE_FLAG.additionalApiKeyLookupEnabled, + // The active mint-shard list is deployment-wide; only the pins are per-org. + FEATURE_FLAG.runOpsMintShardSet, + FEATURE_FLAG.runOpsMintShardSetPrev, + FEATURE_FLAG.runOpsMintShardSetFlippedAt, ]; // Create a Zod schema from the existing catalog diff --git a/apps/webapp/app/v3/runOpsMigration/mintShardGrace.test.ts b/apps/webapp/app/v3/runOpsMigration/mintShardGrace.test.ts index 9358685e1e8..3d26cb9fd1a 100644 --- a/apps/webapp/app/v3/runOpsMigration/mintShardGrace.test.ts +++ b/apps/webapp/app/v3/runOpsMigration/mintShardGrace.test.ts @@ -1,11 +1,12 @@ import { describe, expect, it } from "vitest"; import { generateRunOpsIdV2 } from "@trigger.dev/core/v3/isomorphic"; import { - buildMintShardResolution, effectiveMintShardSet, isValidPinValue, parseShardCsv, + readMintShardSetResolution, SHARD_KEY_PATTERN, + stampMintShardSetFlip, type MintShardSetResolution, } from "./mintShardGrace"; @@ -116,33 +117,118 @@ describe("effectiveMintShardSet", () => { }); }); -describe("buildMintShardResolution", () => { - it("omits prevSet entirely when no flip timestamp is configured", () => { +describe("readMintShardSetResolution", () => { + it("returns an empty set for an absent record", () => { + expect(readMintShardSetResolution(undefined)).toEqual({ set: [] }); + expect(readMintShardSetResolution({})).toEqual({ set: [] }); + }); + + it("reads and sorts the trio", () => { + const r = readMintShardSetResolution({ + runOpsMintShardSet: "b,a", + runOpsMintShardSetPrev: "c,a", + runOpsMintShardSetFlippedAt: new Date(T).toISOString(), + }); + expect(r).toEqual({ set: ["a", "b"], prevSet: ["a", "c"], flippedAtMs: T }); + }); + + it("omits prevSet when no flip timestamp is stored", () => { // A prevSet with no timestamp can never apply, so it MUST NOT linger. - const r = buildMintShardResolution({ shards: "a,b", prev: "a", flippedAt: undefined }); + const r = readMintShardSetResolution({ + runOpsMintShardSet: "a,b", + runOpsMintShardSetPrev: "a", + }); expect(r).toEqual({ set: ["a", "b"], prevSet: undefined, flippedAtMs: undefined }); }); - it("keeps an empty prevSet when a flip timestamp IS configured", () => { - const r = buildMintShardResolution({ - shards: "a", - prev: "", - flippedAt: new Date(T).toISOString(), + it("keeps an empty prevSet when a timestamp IS stored, which graces a first activation", () => { + const r = readMintShardSetResolution({ + runOpsMintShardSet: "a", + runOpsMintShardSetPrev: "", + runOpsMintShardSetFlippedAt: new Date(T).toISOString(), }); expect(r).toEqual({ set: ["a"], prevSet: [], flippedAtMs: T }); }); - it("parses the flip timestamp and sorts both lists", () => { - const r = buildMintShardResolution({ - shards: "b,a", - prev: "c,a", - flippedAt: new Date(T).toISOString(), + it("degrades a stored value it cannot parse to an empty list instead of throwing", () => { + // Boot may throw on a bad env var. The mint path must never throw on a bad stored value. + expect(() => readMintShardSetResolution({ runOpsMintShardSet: "NOPE" })).not.toThrow(); + expect(readMintShardSetResolution({ runOpsMintShardSet: "NOPE" }).set).toEqual([]); + expect(readMintShardSetResolution({ runOpsMintShardSet: 42 }).set).toEqual([]); + expect( + readMintShardSetResolution({ + runOpsMintShardSet: "a", + runOpsMintShardSetFlippedAt: "not-a-date", + }) + ).toEqual({ set: ["a"], prevSet: undefined, flippedAtMs: undefined }); + }); +}); + +describe("stampMintShardSetFlip", () => { + it("does nothing when the save omits the set", () => { + // Omitting the set is an unrelated flag change; it must not inject a default or reset the clock. + const outgoing = { someOtherFlag: true } as Record; + expect(stampMintShardSetFlip({ runOpsMintShardSet: "a" }, outgoing, T, GRACE_MS)).toEqual({ + someOtherFlag: true, }); - expect(r).toEqual({ set: ["a", "b"], prevSet: ["a", "c"], flippedAtMs: T }); }); - it("treats an unparseable timestamp as no stamp at all", () => { - const r = buildMintShardResolution({ shards: "a", prev: "b", flippedAt: "not-a-date" }); - expect(r).toEqual({ set: ["a"], prevSet: undefined, flippedAtMs: undefined }); + it("stamps prev and flippedAt on a genuine change", () => { + const stamped = stampMintShardSetFlip( + { runOpsMintShardSet: "a" }, + { runOpsMintShardSet: "a,b" }, + T, + GRACE_MS + ); + expect(stamped.runOpsMintShardSetPrev).toBe("a"); + expect(stamped.runOpsMintShardSetFlippedAt).toBe(new Date(T).toISOString()); + }); + + it("stamps an empty prev on a first activation", () => { + const stamped = stampMintShardSetFlip({}, { runOpsMintShardSet: "a" }, T, GRACE_MS); + expect(stamped.runOpsMintShardSetPrev).toBe(""); + expect(stamped.runOpsMintShardSetFlippedAt).toBe(new Date(T).toISOString()); + }); + + it("treats a reordered list as no change", () => { + const stamped = stampMintShardSetFlip( + { runOpsMintShardSet: "a,b" }, + { runOpsMintShardSet: "b,a" }, + T, + GRACE_MS + ); + expect(stamped.runOpsMintShardSetFlippedAt).toBeUndefined(); + }); + + it("carries an in-flight stamp forward rather than resetting the cutover clock", () => { + const existing = { + runOpsMintShardSet: "a,b", + runOpsMintShardSetPrev: "a", + runOpsMintShardSetFlippedAt: new Date(T).toISOString(), + }; + const stamped = stampMintShardSetFlip( + existing, + { runOpsMintShardSet: "a,b" }, + T + 1000, + GRACE_MS + ); + expect(stamped.runOpsMintShardSetPrev).toBe("a"); + expect(stamped.runOpsMintShardSetFlippedAt).toBe(new Date(T).toISOString()); + }); + + it("stamps prev as the CURRENTLY-EFFECTIVE set when a second flip lands mid-window", () => { + // Two flips inside one window must not strand the original prev; prev is what readers serve now. + const existing = { + runOpsMintShardSet: "a,b", + runOpsMintShardSetPrev: "a", + runOpsMintShardSetFlippedAt: new Date(T).toISOString(), + }; + const stamped = stampMintShardSetFlip( + existing, + { runOpsMintShardSet: "a,b,c" }, + T + 1000, + GRACE_MS + ); + expect(stamped.runOpsMintShardSetPrev).toBe("a"); }); }); diff --git a/apps/webapp/app/v3/runOpsMigration/mintShardGrace.ts b/apps/webapp/app/v3/runOpsMigration/mintShardGrace.ts index bfa3139cc25..65d8af6f417 100644 --- a/apps/webapp/app/v3/runOpsMigration/mintShardGrace.ts +++ b/apps/webapp/app/v3/runOpsMigration/mintShardGrace.ts @@ -19,6 +19,12 @@ export type MintShardSetResolution = { flippedAtMs?: number; }; +// Flag keys holding the active set and its grace stamp. Named here so the pure module can read +// a flag record without importing the catalog. +const SET_KEY = "runOpsMintShardSet"; +const SET_PREV_KEY = "runOpsMintShardSetPrev"; +const SET_FLIPPED_AT_KEY = "runOpsMintShardSetFlippedAt"; + export function isValidPinValue(value: unknown): value is ShardKey { if (typeof value !== "string") return false; return value === GEN_1_PIN_VALUE || SHARD_KEY_PATTERN.test(value); @@ -61,35 +67,67 @@ export function effectiveMintShardSet( return nowMs < r.flippedAtMs + graceMs ? r.prevSet : r.set; } -// A prevSet with no timestamp can never apply, so it is dropped. A timestamp with an EMPTY -// prevSet is meaningful: it graces a first activation, serving no shards for the window. -export function buildMintShardResolution(source: { - shards: string | undefined; - prev: string | undefined; - flippedAt: string | undefined; -}): MintShardSetResolution { - const parsed = source.flippedAt !== undefined ? Date.parse(source.flippedAt) : NaN; +// The active set lives in the control-plane database, not in the environment. A deploy rolls +// for hours, so two pods can hold different environment values at the same time; only a shared +// row lets every pod agree on one set. Boot may reject a bad environment value, but the mint +// path must never throw on a bad stored value, so an unreadable list degrades to empty. +function readStoredCsv(value: unknown): string[] { + if (typeof value !== "string") return []; + try { + return parseShardCsv(value); + } catch { + return []; + } +} + +// Reads the { set, prevSet, flippedAtMs } trio out of one flag record. Pure. A prevSet with no +// timestamp can never apply, so it is dropped. A timestamp with an EMPTY prevSet is meaningful: +// it graces a first activation, serving no shards for the window. +export function readMintShardSetResolution( + flags: Record | null | undefined +): MintShardSetResolution { + const source = flags ?? {}; + const flippedAtRaw = source[SET_FLIPPED_AT_KEY]; + const parsed = typeof flippedAtRaw === "string" ? Date.parse(flippedAtRaw) : NaN; const flippedAtMs = Number.isNaN(parsed) ? undefined : parsed; return { - set: parseShardCsv(source.shards), - prevSet: flippedAtMs === undefined ? undefined : parseShardCsv(source.prev), + set: readStoredCsv(source[SET_KEY]), + prevSet: flippedAtMs === undefined ? undefined : readStoredCsv(source[SET_PREV_KEY]), flippedAtMs, }; } -// Returns a message to log when the stamp is half-configured, otherwise undefined. Stays quiet -// while the active set is empty, so an unconfigured deployment logs nothing at boot. -export function mintShardStampWarning(source: { - shards: string | undefined; - prev: string | undefined; - flippedAt: string | undefined; -}): string | undefined { - if (parseShardCsv(source.shards).length === 0) { - return undefined; +// Stamps a grace window only when the outgoing set differs from the stored one. prev becomes the +// set readers serve right now, so a second flip inside one window cannot strand the first. A save +// that leaves the set unchanged carries any in-flight stamp forward, so it cannot reset the clock. +export function stampMintShardSetFlip( + existingFlags: Record | null | undefined, + outgoingFlags: Record, + nowMs: number, + graceMs: number +): Record { + // Only act when the save actually SETS the list. Omitting it must not inject a default. + if (typeof outgoingFlags[SET_KEY] !== "string") { + return outgoingFlags; + } + + const existing = existingFlags ?? {}; + const outgoingSet = readStoredCsv(outgoingFlags[SET_KEY]); + const storedSet = readStoredCsv(existing[SET_KEY]); + + if (outgoingSet.join(",") !== storedSet.join(",")) { + const effective = effectiveMintShardSet(readMintShardSetResolution(existing), nowMs, graceMs); + outgoingFlags[SET_PREV_KEY] = effective.join(","); + outgoingFlags[SET_FLIPPED_AT_KEY] = new Date(nowMs).toISOString(); + return outgoingFlags; + } + + if (existing[SET_PREV_KEY] !== undefined) { + outgoingFlags[SET_PREV_KEY] = existing[SET_PREV_KEY]; } - if (parseShardCsv(source.prev).length > 0 && source.flippedAt === undefined) { - return "RUN_OPS_MINT_SHARDS_PREV is set but RUN_OPS_MINT_SHARDS_FLIPPED_AT is not; the shard-set grace window will never apply"; + if (existing[SET_FLIPPED_AT_KEY] !== undefined) { + outgoingFlags[SET_FLIPPED_AT_KEY] = existing[SET_FLIPPED_AT_KEY]; } - return undefined; + return outgoingFlags; } diff --git a/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.ts b/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.ts index 806a0c1aea8..ff8c178eec2 100644 --- a/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.ts +++ b/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.ts @@ -15,12 +15,15 @@ function envIds(count: number): string[] { return ids; } +const ALL_KEYS = "abcdefghijklmnopqrstuvwxyz0123456789".split(""); + function deps( resolution: MintShardSetResolution, overrides: Partial = {} ): MintShardDeps { return { resolution, + ceiling: ALL_KEYS, nowMs: T + GRACE_MS + 1, graceMs: GRACE_MS, orgFeatureFlags: undefined, @@ -41,14 +44,27 @@ function place(ids: string[], resolution: MintShardSetResolution): Map { - it("returns new when the active set is unset", () => { + it("returns new when the live list is empty", () => { expect(computeMintShard({ id: "env_1" }, deps({ set: [] }))).toBe("new"); }); - it("returns new when the active set is empty even with a stale stamp present", () => { + it("returns new when the deployment configures no ceiling", () => { + // An unconfigured deployment is an unconditional kill switch, whatever the stored list says. + const resolution: MintShardSetResolution = { set: ["a", "b"] }; + expect(computeMintShard({ id: "env_1" }, deps(resolution, { ceiling: [] }))).toBe("new"); + }); + + it("returns new when the stored list names nothing this deployment can route", () => { + const resolution: MintShardSetResolution = { set: ["z"] }; + expect(computeMintShard({ id: "env_1" }, deps(resolution, { ceiling: ["a"] }))).toBe("new"); + }); + + it("returns new when the ceiling is empty even with a stale stamp present", () => { const resolution: MintShardSetResolution = { set: [], prevSet: ["a"], flippedAtMs: T }; - // The empty check MUST run before the grace, so an unset set is an unconditional kill switch. - expect(computeMintShard({ id: "env_1" }, deps(resolution, { nowMs: T + 1 }))).toBe("new"); + // The ceiling gate MUST run before the grace, so no stored value can reopen a closed switch. + expect(computeMintShard({ id: "env_1" }, deps(resolution, { nowMs: T + 1, ceiling: [] }))).toBe( + "new" + ); }); it("returns new when the grace serves an empty prevSet", () => { @@ -246,3 +262,38 @@ describe("computeMintShard — rendezvous properties", () => { expect(computeMintShard({ id: "env_1" }, deps({ set: ["a", "b"] }, pinnedToC))).not.toBe("c"); }); }); + +describe("computeMintShard — the ceiling bounds the stored list", () => { + it("mints only into keys the deployment can route", () => { + const resolution: MintShardSetResolution = { set: ["a", "b", "c"] }; + const ids = envIds(300); + for (const id of ids) { + const shard = computeMintShard({ id }, deps(resolution, { ceiling: ["a", "b"] })); + expect(["a", "b"]).toContain(shard); + } + }); + + it("ignores a pin to a key outside the ceiling", () => { + const resolution: MintShardSetResolution = { set: ["a", "c"] }; + const rejected: string[] = []; + const shard = computeMintShard( + { id: "env_1" }, + deps(resolution, { + ceiling: ["a"], + orgFeatureFlags: { runOpsMintShard: "c" }, + onPinRejected: (info) => rejected.push(info.pin), + }) + ); + expect(shard).toBe("a"); + expect(rejected).toEqual(["c"]); + }); + + it("still honours a gen-1 pin when the ceiling is narrower than the stored list", () => { + const resolution: MintShardSetResolution = { set: ["a", "b"] }; + const shard = computeMintShard( + { id: "env_1" }, + deps(resolution, { ceiling: ["a"], orgFeatureFlags: { runOpsMintShard: "new" } }) + ); + expect(shard).toBe("new"); + }); +}); diff --git a/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts b/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts index 20f20fcf944..8080acce0eb 100644 --- a/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts +++ b/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts @@ -1,19 +1,23 @@ import { createHash } from "node:crypto"; import type { ShardKey } from "@trigger.dev/core/v3/isomorphic"; +import { $replica } from "~/db.server"; 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, + parseShardCsv, + readMintShardSetResolution, type MintShardSetResolution, } from "./mintShardGrace"; export type MintShardDeps = { + // The live list, from the control-plane database. resolution: MintShardSetResolution; + // The keys this deployment can route, from the environment. Bounds the live list. + ceiling: string[]; nowMs: number; graceMs: number; orgFeatureFlags: unknown; @@ -83,16 +87,20 @@ function hrwSelect(environmentId: string, activeSet: string[]): string { // PURE CORE — no env, no clock, no I/O; tests drive this directly. Deterministic for fixed // deps, which is what lets run minting and token minting agree on one answer. // -// The empty-set check runs BEFORE the grace, so an unset active list is an unconditional kill -// switch that a stale stamp cannot reopen. A pin outside the active set falls through to the -// hash rather than throwing: honouring it would leak the drain the active list performs, and -// throwing would fail customer triggers whenever a pinned shard drains. +// The ceiling gate runs BEFORE the grace, so an unconfigured deployment is an unconditional +// kill switch that no stored value can reopen. The live list is then intersected with the +// ceiling, so a stored key this deployment cannot route is never minted into. +// +// A pin outside the active set falls through to the hash rather than throwing: honouring it +// would leak the drain the active list performs, and throwing would fail customer triggers +// whenever a pinned shard drains. export function computeMintShard(environment: { id: string }, deps: MintShardDeps): ShardKey { - if (deps.resolution.set.length === 0) { + if (deps.ceiling.length === 0) { return "new"; } - const activeSet = effectiveMintShardSet(deps.resolution, deps.nowMs, deps.graceMs); + const live = effectiveMintShardSet(deps.resolution, deps.nowMs, deps.graceMs); + const activeSet = live.filter((key) => deps.ceiling.includes(key)); if (activeSet.length === 0) { return "new"; } @@ -111,24 +119,38 @@ export function computeMintShard(environment: { id: string }, deps: MintShardDep return hrwSelect(environment.id, activeSet); } -// ENV-BOUND wrapper — the only place env is read. The resolution is built once: these are -// deploy-time values, so re-parsing per mint would burn CPU on the hottest path in the system. -const shardResolution: MintShardSetResolution = buildMintShardResolution({ - shards: env.RUN_OPS_MINT_SHARDS, - prev: env.RUN_OPS_MINT_SHARDS_PREV, - flippedAt: env.RUN_OPS_MINT_SHARDS_FLIPPED_AT, -}); - -const stampWarning = mintShardStampWarning({ - shards: env.RUN_OPS_MINT_SHARDS, - prev: env.RUN_OPS_MINT_SHARDS_PREV, - flippedAt: env.RUN_OPS_MINT_SHARDS_FLIPPED_AT, -}); -if (stampWarning) { - logger.warn(`[runOpsMintShard] ${stampWarning}`, { - RUN_OPS_MINT_SHARDS: env.RUN_OPS_MINT_SHARDS, - RUN_OPS_MINT_SHARDS_PREV: env.RUN_OPS_MINT_SHARDS_PREV, +// ENV-BOUND wrapper — the only place env is read. The ceiling is parsed once at boot; it is a +// deploy-time value, so re-parsing per mint would burn CPU on the hottest path in the system. +const ceiling: string[] = parseShardCsv(env.RUN_OPS_MINT_SHARDS); + +// The live list is org-independent, so one process-wide entry serves every mint. One query per +// process per TTL, folded into a single round-trip over the three keys. The TTL bounds how long +// two processes can disagree, which is what the grace window is sized against. +const SET_KEYS = [ + FEATURE_FLAG.runOpsMintShardSet, + FEATURE_FLAG.runOpsMintShardSetPrev, + FEATURE_FLAG.runOpsMintShardSetFlippedAt, +]; + +let cachedResolution: { value: MintShardSetResolution; expiresAt: number } | undefined; + +async function readLiveResolution(): Promise { + if (cachedResolution && cachedResolution.expiresAt > Date.now()) { + return cachedResolution.value; + } + + const rows = await $replica.featureFlag.findMany({ + where: { key: { in: SET_KEYS } }, + select: { key: true, value: true }, }); + const flags: Record = {}; + for (const row of rows) { + flags[row.key] = row.value; + } + + const value = readMintShardSetResolution(flags); + cachedResolution = { value, expiresAt: Date.now() + env.RUN_OPS_MINT_FLAG_CACHE_TTL_MS }; + return value; } // Once per environment per process: a stale pin sits on the root-trigger path and would @@ -149,9 +171,6 @@ function reportPinRejected(info: { * Which shard an environment mints new roots into. Call only after resolveRunIdMintKind has * returned "runOpsId". Returns "new" to mean a gen-1 run-ops id, which is today's behaviour. * - * Async despite doing no I/O, so the deploy-free active-set layer can add a read later without - * changing every call site. - * * @knipignore the gen-2 write-path change is the first production caller; drop this tag there. */ export async function resolveMintShard(environment: { @@ -159,8 +178,23 @@ export async function resolveMintShard(environment: { // Pass environment.organization.featureFlags from the trigger call site. orgFeatureFlags?: unknown; }): Promise { + // No ceiling means no gen-2 minting, so skip the read entirely. + if (ceiling.length === 0) { + return "new"; + } + + let resolution: MintShardSetResolution; + try { + resolution = await readLiveResolution(); + } catch (error) { + // Fail safe to gen-1, mirroring the mint-kind gate's fail-safe to cuid. + logger.error("[runOpsMintShard] shard-set read failed; minting gen-1 (fail-safe)", { error }); + return "new"; + } + return computeMintShard(environment, { - resolution: shardResolution, + resolution, + ceiling, nowMs: Date.now(), graceMs: env.RUN_OPS_MINT_FLIP_GRACE_MS, orgFeatureFlags: environment.orgFeatureFlags, diff --git a/apps/webapp/test/runOpsMintShardSetFlip.test.ts b/apps/webapp/test/runOpsMintShardSetFlip.test.ts new file mode 100644 index 00000000000..55ebe43a2d6 --- /dev/null +++ b/apps/webapp/test/runOpsMintShardSetFlip.test.ts @@ -0,0 +1,190 @@ +// The active mint-shard list lives in the control-plane database, not in the environment: a +// rolling deploy runs two environment values at once for hours, so only a shared row lets every +// pod agree on one list. A change must therefore read -> stamp -> write under an advisory lock, +// and must never be writable as a bare upsert from a request body. Real testcontainers Postgres. +import type { PrismaClient } from "@trigger.dev/database"; +import { postgresTest } from "@internal/testcontainers"; +import { describe, expect, vi } from "vitest"; +import { FEATURE_FLAG, type FeatureFlagKey } from "~/v3/featureFlags"; +import { + applyGlobalGracedFlips, + makeSetMultipleFlags, + replaceGlobalFeatureFlags, +} from "~/v3/featureFlags.server"; + +vi.setConfig({ testTimeout: 60_000 }); + +const SET_KEYS: FeatureFlagKey[] = [ + FEATURE_FLAG.runOpsMintShardSet, + FEATURE_FLAG.runOpsMintShardSetPrev, + FEATURE_FLAG.runOpsMintShardSetFlippedAt, +]; + +const MINT_KIND_KEYS: FeatureFlagKey[] = [ + FEATURE_FLAG.runOpsMintKind, + FEATURE_FLAG.runOpsMintKindPrev, + FEATURE_FLAG.runOpsMintKindFlippedAt, +]; + +const CATALOG_KEYS: FeatureFlagKey[] = [ + ...SET_KEYS, + ...MINT_KIND_KEYS, + FEATURE_FLAG.mollifierEnabled, +]; + +const NEVER_PROTECTED = () => false; + +async function readFlags( + prisma: PrismaClient, + keys: FeatureFlagKey[] +): Promise> { + const rows = await prisma.featureFlag.findMany({ + where: { key: { in: keys } }, + select: { key: true, value: true }, + }); + const m: Record = {}; + for (const row of rows) m[row.key] = row.value; + return m; +} + +describe("applyGlobalGracedFlips — the shard-set list is stamped, not bare-written", () => { + postgresTest("a genuine list change stamps prev + flippedAt", async ({ prisma }) => { + await makeSetMultipleFlags(prisma)({ [FEATURE_FLAG.runOpsMintShardSet]: "a" }); + + await applyGlobalGracedFlips(prisma, { [FEATURE_FLAG.runOpsMintShardSet]: "a,b" }, 60_000); + + const m = await readFlags(prisma, SET_KEYS); + expect(m[FEATURE_FLAG.runOpsMintShardSet]).toBe("a,b"); + expect(m[FEATURE_FLAG.runOpsMintShardSetPrev]).toBe("a"); + expect(typeof m[FEATURE_FLAG.runOpsMintShardSetFlippedAt]).toBe("string"); + }); + + postgresTest("a first activation stamps an empty prev, which graces it", async ({ prisma }) => { + await applyGlobalGracedFlips(prisma, { [FEATURE_FLAG.runOpsMintShardSet]: "a" }, 60_000); + + const m = await readFlags(prisma, SET_KEYS); + expect(m[FEATURE_FLAG.runOpsMintShardSetPrev]).toBe(""); + expect(typeof m[FEATURE_FLAG.runOpsMintShardSetFlippedAt]).toBe("string"); + }); + + postgresTest( + "a reordered list is not a change, so the clock is not reset", + async ({ prisma }) => { + await applyGlobalGracedFlips(prisma, { [FEATURE_FLAG.runOpsMintShardSet]: "a,b" }, 60_000); + const first = await readFlags(prisma, SET_KEYS); + + await applyGlobalGracedFlips(prisma, { [FEATURE_FLAG.runOpsMintShardSet]: "b,a" }, 60_000); + const second = await readFlags(prisma, SET_KEYS); + + expect(second[FEATURE_FLAG.runOpsMintShardSetFlippedAt]).toBe( + first[FEATURE_FLAG.runOpsMintShardSetFlippedAt] + ); + } + ); + + postgresTest("both graced groups stamp in ONE save", async ({ prisma }) => { + // A save that flips the kind and the list must not stamp one and lose the other. + await makeSetMultipleFlags(prisma)({ + [FEATURE_FLAG.runOpsMintKind]: "cuid", + [FEATURE_FLAG.runOpsMintShardSet]: "a", + }); + + await applyGlobalGracedFlips( + prisma, + { + [FEATURE_FLAG.runOpsMintKind]: "runOpsId", + [FEATURE_FLAG.runOpsMintShardSet]: "a,b", + }, + 60_000 + ); + + const m = await readFlags(prisma, [...SET_KEYS, ...MINT_KIND_KEYS]); + expect(m[FEATURE_FLAG.runOpsMintKindPrev]).toBe("cuid"); + expect(m[FEATURE_FLAG.runOpsMintShardSetPrev]).toBe("a"); + expect(typeof m[FEATURE_FLAG.runOpsMintKindFlippedAt]).toBe("string"); + expect(typeof m[FEATURE_FLAG.runOpsMintShardSetFlippedAt]).toBe("string"); + }); + + postgresTest("concurrent list changes serialize on the lock", async ({ prisma }) => { + await makeSetMultipleFlags(prisma)({ [FEATURE_FLAG.runOpsMintShardSet]: "a" }); + + await Promise.all([ + applyGlobalGracedFlips(prisma, { [FEATURE_FLAG.runOpsMintShardSet]: "a,b" }, 60_000), + applyGlobalGracedFlips(prisma, { [FEATURE_FLAG.runOpsMintShardSet]: "a,c" }, 60_000), + ]); + + const m = await readFlags(prisma, SET_KEYS); + // Whichever won, the stamp must describe a real predecessor, never be absent. + expect(["a", "a,b", "a,c"]).toContain(m[FEATURE_FLAG.runOpsMintShardSetPrev]); + expect(typeof m[FEATURE_FLAG.runOpsMintShardSetFlippedAt]).toBe("string"); + }); +}); + +describe("replaceGlobalFeatureFlags — the admin page cannot bypass the stamp", () => { + postgresTest("a list change through the page is stamped", async ({ prisma }) => { + await makeSetMultipleFlags(prisma)({ [FEATURE_FLAG.runOpsMintShardSet]: "a" }); + + await replaceGlobalFeatureFlags(prisma, { + requestedFlags: { [FEATURE_FLAG.runOpsMintShardSet]: "a,b" }, + catalogKeys: CATALOG_KEYS, + isProtected: NEVER_PROTECTED, + graceMs: 60_000, + }); + + const m = await readFlags(prisma, SET_KEYS); + expect(m[FEATURE_FLAG.runOpsMintShardSet]).toBe("a,b"); + expect(m[FEATURE_FLAG.runOpsMintShardSetPrev]).toBe("a"); + }); + + postgresTest("a body-supplied stamp is ignored and recomputed", async ({ prisma }) => { + await makeSetMultipleFlags(prisma)({ [FEATURE_FLAG.runOpsMintShardSet]: "a" }); + + await replaceGlobalFeatureFlags(prisma, { + requestedFlags: { + [FEATURE_FLAG.runOpsMintShardSet]: "a,b", + [FEATURE_FLAG.runOpsMintShardSetPrev]: "zzz", + [FEATURE_FLAG.runOpsMintShardSetFlippedAt]: "1999-01-01T00:00:00.000Z", + }, + catalogKeys: CATALOG_KEYS, + isProtected: NEVER_PROTECTED, + graceMs: 60_000, + }); + + const m = await readFlags(prisma, SET_KEYS); + expect(m[FEATURE_FLAG.runOpsMintShardSetPrev]).toBe("a"); + expect(m[FEATURE_FLAG.runOpsMintShardSetFlippedAt]).not.toBe("1999-01-01T00:00:00.000Z"); + }); + + postgresTest("the set trio survives a save that omits the set keys", async ({ prisma }) => { + await replaceGlobalFeatureFlags(prisma, { + requestedFlags: { [FEATURE_FLAG.runOpsMintShardSet]: "a,b" }, + catalogKeys: CATALOG_KEYS, + isProtected: NEVER_PROTECTED, + graceMs: 60_000, + }); + + await replaceGlobalFeatureFlags(prisma, { + requestedFlags: { [FEATURE_FLAG.mollifierEnabled]: true }, + catalogKeys: CATALOG_KEYS, + isProtected: NEVER_PROTECTED, + graceMs: 60_000, + }); + + const m = await readFlags(prisma, SET_KEYS); + expect(m[FEATURE_FLAG.runOpsMintShardSet]).toBe("a,b"); + }); + + postgresTest("an ordinary flag keeps replace semantics", async ({ prisma }) => { + await makeSetMultipleFlags(prisma)({ [FEATURE_FLAG.mollifierEnabled]: true }); + + await replaceGlobalFeatureFlags(prisma, { + requestedFlags: {}, + catalogKeys: CATALOG_KEYS, + isProtected: NEVER_PROTECTED, + graceMs: 60_000, + }); + + const m = await readFlags(prisma, [FEATURE_FLAG.mollifierEnabled]); + expect(m[FEATURE_FLAG.mollifierEnabled]).toBeUndefined(); + }); +}); From bc1fe84340e462e42d94052af1cd21edfa415f5b Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Mon, 24 Aug 2026 09:50:07 +0100 Subject: [PATCH 03/17] test(webapp): cover the mint-shard wrapper, flag schemas and scope locks 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. --- .../runOpsMintShard.server.test.ts | 97 ++++++++++++++++- .../runOpsMigration/runOpsMintShard.server.ts | 89 +++++++++++----- apps/webapp/test/runOpsMintShardFlags.test.ts | 100 ++++++++++++++++++ 3 files changed, 258 insertions(+), 28 deletions(-) create mode 100644 apps/webapp/test/runOpsMintShardFlags.test.ts diff --git a/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.ts b/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.ts index ff8c178eec2..e6a7898b803 100644 --- a/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.ts +++ b/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.ts @@ -1,5 +1,11 @@ import { describe, expect, it } from "vitest"; -import { computeMintShard, type MintShardDeps } from "./runOpsMintShard.server"; +import { + computeMintShard, + resolveMintShardWith, + type MintShardCache, + type MintShardDeps, + type ResolveMintShardDeps, +} from "./runOpsMintShard.server"; import { type MintShardSetResolution } from "./mintShardGrace"; const GRACE_MS = 90_000; @@ -297,3 +303,92 @@ describe("computeMintShard — the ceiling bounds the stored list", () => { expect(shard).toBe("new"); }); }); + +describe("resolveMintShardWith — cache, ceiling short-circuit and fail-safe", () => { + function wrapperDeps( + overrides: Partial = {} + ): ResolveMintShardDeps & { reads: number } { + const state = { + ceiling: ["a", "b"], + readFlags: async () => ({ runOpsMintShardSet: "a,b" }), + cache: { current: undefined as MintShardCache }, + nowMs: T, + ttlMs: 30_000, + graceMs: GRACE_MS, + orgFeatureFlags: undefined as unknown, + reads: 0, + ...overrides, + }; + const wrapped = state.readFlags; + state.readFlags = async () => { + state.reads++; + return wrapped(); + }; + return state; + } + + it("never reads the list when the deployment configures no ceiling", async () => { + const deps = wrapperDeps({ ceiling: [] }); + expect(await resolveMintShardWith({ id: "env_1" }, deps)).toBe("new"); + expect(deps.reads).toBe(0); + }); + + it("reads once, then serves the cache until the TTL expires", async () => { + const deps = wrapperDeps(); + await resolveMintShardWith({ id: "env_1" }, deps); + await resolveMintShardWith({ id: "env_2" }, deps); + await resolveMintShardWith({ id: "env_3" }, deps); + expect(deps.reads).toBe(1); + }); + + it("reads again once the TTL expires", async () => { + const deps = wrapperDeps(); + await resolveMintShardWith({ id: "env_1" }, deps); + deps.nowMs = T + 30_000; + await resolveMintShardWith({ id: "env_1" }, deps); + expect(deps.reads).toBe(2); + }); + + it("falls back to gen-1 when the read throws, and does not poison the cache", async () => { + // A blip must not move every environment's placement, so it returns gen-1 rather than guess. + let fail = true; + const deps = wrapperDeps({ + readFlags: async () => { + if (fail) throw new Error("db down"); + return { runOpsMintShardSet: "a,b" }; + }, + }); + const failures: unknown[] = []; + deps.onReadFailed = (error) => failures.push(error); + + expect(await resolveMintShardWith({ id: "env_1" }, deps)).toBe("new"); + expect(failures).toHaveLength(1); + + fail = false; + expect(["a", "b"]).toContain(await resolveMintShardWith({ id: "env_1" }, deps)); + }); + + it("returns gen-1 when the stored list names nothing inside the ceiling", async () => { + const deps = wrapperDeps({ + ceiling: ["a"], + readFlags: async () => ({ runOpsMintShardSet: "z" }), + }); + expect(await resolveMintShardWith({ id: "env_1" }, deps)).toBe("new"); + }); + + it("agrees with the pure core for the same inputs", async () => { + const deps = wrapperDeps(); + const viaWrapper = await resolveMintShardWith({ id: "env_1" }, deps); + const viaCore = computeMintShard( + { id: "env_1" }, + { + resolution: { set: ["a", "b"] }, + ceiling: ["a", "b"], + nowMs: T, + graceMs: GRACE_MS, + orgFeatureFlags: undefined, + } + ); + expect(viaWrapper).toBe(viaCore); + }); +}); diff --git a/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts b/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts index 8080acce0eb..540f10b56ed 100644 --- a/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts +++ b/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts @@ -123,22 +123,70 @@ export function computeMintShard(environment: { id: string }, deps: MintShardDep // deploy-time value, so re-parsing per mint would burn CPU on the hottest path in the system. const ceiling: string[] = parseShardCsv(env.RUN_OPS_MINT_SHARDS); -// The live list is org-independent, so one process-wide entry serves every mint. One query per -// process per TTL, folded into a single round-trip over the three keys. The TTL bounds how long -// two processes can disagree, which is what the grace window is sized against. const SET_KEYS = [ FEATURE_FLAG.runOpsMintShardSet, FEATURE_FLAG.runOpsMintShardSetPrev, FEATURE_FLAG.runOpsMintShardSetFlippedAt, ]; -let cachedResolution: { value: MintShardSetResolution; expiresAt: number } | undefined; +export type MintShardCache = { value: MintShardSetResolution; expiresAt: number } | undefined; + +export type ResolveMintShardDeps = { + ceiling: string[]; + // Reads the three list rows. Injected so the cache and the fail-safe are testable without a + // database, the same way computeRunIdMintKind takes its flag reader. + readFlags: () => Promise>; + cache: { current: MintShardCache }; + nowMs: number; + ttlMs: number; + graceMs: number; + orgFeatureFlags: unknown; + onPinRejected?: (info: { environmentId: string; pin: string; activeSet: string[] }) => void; + onReadFailed?: (error: unknown) => void; +}; + +// The live list is org-independent, so one process-wide entry serves every mint. One query per +// process per TTL, over one round-trip. The TTL bounds how long two processes can disagree, +// which is what the grace window is sized against. +// +// A failed read falls back to gen-1 rather than guessing a list. Guessing would move every +// environment's placement for the length of one blip. +export async function resolveMintShardWith( + environment: { id: string; orgFeatureFlags?: unknown }, + deps: ResolveMintShardDeps +): Promise { + // No ceiling means no gen-2 minting, so skip the read entirely. + if (deps.ceiling.length === 0) { + return "new"; + } -async function readLiveResolution(): Promise { - if (cachedResolution && cachedResolution.expiresAt > Date.now()) { - return cachedResolution.value; + let resolution: MintShardSetResolution; + const cached = deps.cache.current; + if (cached && cached.expiresAt > deps.nowMs) { + resolution = cached.value; + } else { + try { + resolution = readMintShardSetResolution(await deps.readFlags()); + } catch (error) { + deps.onReadFailed?.(error); + return "new"; + } + deps.cache.current = { value: resolution, expiresAt: deps.nowMs + deps.ttlMs }; } + return computeMintShard(environment, { + resolution, + ceiling: deps.ceiling, + nowMs: deps.nowMs, + graceMs: deps.graceMs, + orgFeatureFlags: deps.orgFeatureFlags, + onPinRejected: deps.onPinRejected, + }); +} + +const liveCache: { current: MintShardCache } = { current: undefined }; + +async function readSetFlags(): Promise> { const rows = await $replica.featureFlag.findMany({ where: { key: { in: SET_KEYS } }, select: { key: true, value: true }, @@ -147,10 +195,7 @@ async function readLiveResolution(): Promise { for (const row of rows) { flags[row.key] = row.value; } - - const value = readMintShardSetResolution(flags); - cachedResolution = { value, expiresAt: Date.now() + env.RUN_OPS_MINT_FLAG_CACHE_TTL_MS }; - return value; + return flags; } // Once per environment per process: a stale pin sits on the root-trigger path and would @@ -178,26 +223,16 @@ export async function resolveMintShard(environment: { // Pass environment.organization.featureFlags from the trigger call site. orgFeatureFlags?: unknown; }): Promise { - // No ceiling means no gen-2 minting, so skip the read entirely. - if (ceiling.length === 0) { - return "new"; - } - - let resolution: MintShardSetResolution; - try { - resolution = await readLiveResolution(); - } catch (error) { - // Fail safe to gen-1, mirroring the mint-kind gate's fail-safe to cuid. - logger.error("[runOpsMintShard] shard-set read failed; minting gen-1 (fail-safe)", { error }); - return "new"; - } - - return computeMintShard(environment, { - resolution, + return resolveMintShardWith(environment, { ceiling, + readFlags: readSetFlags, + cache: liveCache, nowMs: Date.now(), + ttlMs: env.RUN_OPS_MINT_FLAG_CACHE_TTL_MS, graceMs: env.RUN_OPS_MINT_FLIP_GRACE_MS, orgFeatureFlags: environment.orgFeatureFlags, onPinRejected: reportPinRejected, + onReadFailed: (error) => + logger.error("[runOpsMintShard] shard-set read failed; minting gen-1 (fail-safe)", { error }), }); } diff --git a/apps/webapp/test/runOpsMintShardFlags.test.ts b/apps/webapp/test/runOpsMintShardFlags.test.ts new file mode 100644 index 00000000000..81ce7ed7fda --- /dev/null +++ b/apps/webapp/test/runOpsMintShardFlags.test.ts @@ -0,0 +1,100 @@ +// The mint-shard flags carry two safety claims that only the catalog can enforce: a bad value is +// rejected at WRITE (so no unroutable key and no silently-unpinned environment can ever be +// stored), and each key is locked at the scope its resolver does not read. Pure, no containers. +import { describe, expect, it } from "vitest"; +import { + FEATURE_FLAG, + FeatureFlagCatalog, + GLOBAL_LOCKED_FLAGS, + ORG_LOCKED_FLAGS, + validateFeatureFlagValue, +} from "~/v3/featureFlags"; + +describe("runOpsMintShard — the per-org pin", () => { + const key = FEATURE_FLAG.runOpsMintShard; + + it("accepts every legal shard key", () => { + for (const c of "abcdefghijklmnopqrstuvwxyz0123456789") { + expect(validateFeatureFlagValue(key, c).success).toBe(true); + } + }); + + it('accepts "new", which holds an org on gen-1', () => { + expect(validateFeatureFlagValue(key, "new").success).toBe(true); + }); + + it("rejects a value that could never be stamped into an id", () => { + for (const bad of ["A", "ab", "", "-", "legacy", " a", "a,b"]) { + expect(validateFeatureFlagValue(key, bad).success).toBe(false); + } + }); +}); + +describe("runOpsMintShardEnvPins — the per-environment pins", () => { + const key = FEATURE_FLAG.runOpsMintShardEnvPins; + + it("accepts a map of environment id to shard key", () => { + expect( + validateFeatureFlagValue(key, JSON.stringify({ env_1: "a", env_2: "new" })).success + ).toBe(true); + expect(validateFeatureFlagValue(key, "{}").success).toBe(true); + }); + + it("rejects a blob that is not JSON, so a typo cannot silently un-pin every environment", () => { + for (const bad of ["{not json", "", "null", "[]", '"a"', "42"]) { + expect(validateFeatureFlagValue(key, bad).success).toBe(false); + } + }); + + it("rejects a map whose value is not a legal pin", () => { + for (const bad of [{ env_1: "AB" }, { env_1: "legacy" }, { env_1: 1 }, { env_1: "" }]) { + expect(validateFeatureFlagValue(key, JSON.stringify(bad)).success).toBe(false); + } + }); +}); + +describe("runOpsMintShardSet — the active list", () => { + const key = FEATURE_FLAG.runOpsMintShardSet; + + it("accepts an empty list and a CSV of legal keys", () => { + expect(validateFeatureFlagValue(key, "").success).toBe(true); + expect(validateFeatureFlagValue(key, "a").success).toBe(true); + expect(validateFeatureFlagValue(key, "a,b, c").success).toBe(true); + }); + + it("rejects a CSV holding a key that cannot be routed", () => { + for (const bad of ["A", "ab", "a,B", "a,legacy", "a,new", "a;b"]) { + expect(validateFeatureFlagValue(key, bad).success).toBe(false); + } + }); +}); + +describe("scope locks match what each resolver actually reads", () => { + it("locks the pins globally, because the resolver reads them from the org blob only", () => { + expect(GLOBAL_LOCKED_FLAGS).toContain(FEATURE_FLAG.runOpsMintShard); + expect(GLOBAL_LOCKED_FLAGS).toContain(FEATURE_FLAG.runOpsMintShardEnvPins); + }); + + it("locks the list per-org, because it is deployment-wide", () => { + expect(ORG_LOCKED_FLAGS).toContain(FEATURE_FLAG.runOpsMintShardSet); + expect(ORG_LOCKED_FLAGS).toContain(FEATURE_FLAG.runOpsMintShardSetPrev); + expect(ORG_LOCKED_FLAGS).toContain(FEATURE_FLAG.runOpsMintShardSetFlippedAt); + }); + + it("keeps the pins settable per-org, which is the canary lever", () => { + expect(ORG_LOCKED_FLAGS).not.toContain(FEATURE_FLAG.runOpsMintShard); + expect(ORG_LOCKED_FLAGS).not.toContain(FEATURE_FLAG.runOpsMintShardEnvPins); + }); + + it("registers every new key in the catalog, so the admin pages render it", () => { + for (const key of [ + FEATURE_FLAG.runOpsMintShard, + FEATURE_FLAG.runOpsMintShardEnvPins, + FEATURE_FLAG.runOpsMintShardSet, + FEATURE_FLAG.runOpsMintShardSetPrev, + FEATURE_FLAG.runOpsMintShardSetFlippedAt, + ]) { + expect(FeatureFlagCatalog).toHaveProperty(key); + } + }); +}); From 417ba9a0cf7f5211572360b3722627d4ca510a60 Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Mon, 24 Aug 2026 09:58:36 +0100 Subject: [PATCH 04/17] feat(webapp): add a fleet-wide mint-shard override for the final cutover 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. --- apps/webapp/app/v3/featureFlags.ts | 8 +++ .../runOpsMintShard.server.test.ts | 65 +++++++++++++++++++ .../runOpsMigration/runOpsMintShard.server.ts | 43 +++++++++--- apps/webapp/test/runOpsMintShardFlags.test.ts | 20 +++++- 4 files changed, 127 insertions(+), 9 deletions(-) diff --git a/apps/webapp/app/v3/featureFlags.ts b/apps/webapp/app/v3/featureFlags.ts index 4dcf7ab7743..ee4491e7840 100644 --- a/apps/webapp/app/v3/featureFlags.ts +++ b/apps/webapp/app/v3/featureFlags.ts @@ -34,6 +34,8 @@ export const FEATURE_FLAG = { runOpsMintShardSet: "runOpsMintShardSet", runOpsMintShardSetPrev: "runOpsMintShardSetPrev", runOpsMintShardSetFlippedAt: "runOpsMintShardSetFlippedAt", + // Fleet-wide pin for the complete cutover. Beats every per-org and per-env pin. + runOpsMintShardOverride: "runOpsMintShardOverride", queueMetricsUiEnabled: "queueMetricsUiEnabled", // Per-organization rollout for creating additional environment API keys. additionalApiKeysEnabled: "additionalApiKeysEnabled", @@ -138,6 +140,11 @@ export const FeatureFlagCatalog = { // stampMintShardSetFlip on a genuine change. Display-only (see ORG_LOCKED_FLAGS). [FEATURE_FLAG.runOpsMintShardSetPrev]: z.string(), [FEATURE_FLAG.runOpsMintShardSetFlippedAt]: z.string().datetime(), + // Sends every environment to one shard, outranking every pin, so a cutover needs no per-org + // visit. "new" holds the whole fleet on gen-1. Only honored while the key is in the active set. + [FEATURE_FLAG.runOpsMintShardOverride]: z + .string() + .refine((v) => v === "new" || /^[a-z0-9]$/.test(v), 'must be a single [a-z0-9] char, or "new"'), // Per-org access to the Queue Metrics dashboard UI (view only; emission is global and // separate). Off unless enabled for the org. [FEATURE_FLAG.queueMetricsUiEnabled]: z.coerce.boolean(), @@ -175,6 +182,7 @@ export const ORG_LOCKED_FLAGS: FeatureFlagKey[] = [ FEATURE_FLAG.runOpsMintShardSet, FEATURE_FLAG.runOpsMintShardSetPrev, FEATURE_FLAG.runOpsMintShardSetFlippedAt, + FEATURE_FLAG.runOpsMintShardOverride, ]; // Create a Zod schema from the existing catalog diff --git a/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.ts b/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.ts index e6a7898b803..322c1322caf 100644 --- a/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.ts +++ b/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.ts @@ -392,3 +392,68 @@ describe("resolveMintShardWith — cache, ceiling short-circuit and fail-safe", expect(viaWrapper).toBe(viaCore); }); }); + +describe("computeMintShard — the global override wins the complete cutover", () => { + const resolution: MintShardSetResolution = { set: ["a", "b"] }; + + it("beats the hash for every environment", () => { + for (const id of envIds(200)) { + expect(computeMintShard({ id }, deps(resolution, { globalOverride: "b" }))).toBe("b"); + } + }); + + it("beats a per-org pin", () => { + const shard = computeMintShard( + { id: "env_1" }, + deps(resolution, { globalOverride: "b", orgFeatureFlags: { runOpsMintShard: "a" } }) + ); + expect(shard).toBe("b"); + }); + + it("beats a per-env pin, which is the whole point of a cutover", () => { + const shard = computeMintShard( + { id: "env_1" }, + deps(resolution, { + globalOverride: "b", + orgFeatureFlags: { runOpsMintShardEnvPins: JSON.stringify({ env_1: "a" }) }, + }) + ); + expect(shard).toBe("b"); + }); + + it("holds the whole fleet on gen-1 when set to new, whatever any org pinned", () => { + const shard = computeMintShard( + { id: "env_1" }, + deps(resolution, { globalOverride: "new", orgFeatureFlags: { runOpsMintShard: "a" } }) + ); + expect(shard).toBe("new"); + }); + + it("is ignored, and reported, when it names a key outside the active set", () => { + // Honouring it would mint into a drained or unroutable shard. Explicit pins still apply. + const rejected: string[] = []; + const shard = computeMintShard( + { id: "env_1" }, + deps(resolution, { + globalOverride: "z", + orgFeatureFlags: { runOpsMintShard: "a" }, + onPinRejected: (info) => rejected.push(info.pin), + }) + ); + expect(shard).toBe("a"); + expect(rejected).toEqual(["z"]); + }); + + it("is ignored when it is not a legal value", () => { + for (const bad of ["legacy", "AB", "", "a,b"]) { + const shard = computeMintShard({ id: "env_1" }, deps(resolution, { globalOverride: bad })); + expect(shard).toBe(computeMintShard({ id: "env_1" }, deps(resolution))); + } + }); + + it("cannot resurrect minting when the ceiling is empty", () => { + expect( + computeMintShard({ id: "env_1" }, deps(resolution, { globalOverride: "b", ceiling: [] })) + ).toBe("new"); + }); +}); diff --git a/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts b/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts index 540f10b56ed..6f2ea5a591f 100644 --- a/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts +++ b/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts @@ -18,6 +18,8 @@ export type MintShardDeps = { resolution: MintShardSetResolution; // The keys this deployment can route, from the environment. Bounds the live list. ceiling: string[]; + // Fleet-wide pin that beats every per-org and per-env pin. The complete-cutover lever. + globalOverride?: unknown; nowMs: number; graceMs: number; orgFeatureFlags: unknown; @@ -105,6 +107,19 @@ export function computeMintShard(environment: { id: string }, deps: MintShardDep return "new"; } + // 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 }); + } + const pin = readPin(deps.orgFeatureFlags, environment.id); if (pin !== undefined) { if (pin === GEN_1_PIN_VALUE) { @@ -121,15 +136,22 @@ export function computeMintShard(environment: { id: string }, deps: MintShardDep // ENV-BOUND wrapper — the only place env is read. The ceiling is parsed once at boot; it is a // deploy-time value, so re-parsing per mint would burn CPU on the hottest path in the system. +// +// SEAM: this is the one place the ceiling is sourced. Once shard descriptors are configured, the +// ceiling becomes their keys and RUN_OPS_MINT_SHARDS is deleted. Two hand-kept lists would drift. const ceiling: string[] = parseShardCsv(env.RUN_OPS_MINT_SHARDS); -const SET_KEYS = [ +// Read together so the override costs no extra query beyond the list it is bounded by. +const GLOBAL_SHARD_KEYS = [ FEATURE_FLAG.runOpsMintShardSet, FEATURE_FLAG.runOpsMintShardSetPrev, FEATURE_FLAG.runOpsMintShardSetFlippedAt, + FEATURE_FLAG.runOpsMintShardOverride, ]; -export type MintShardCache = { value: MintShardSetResolution; expiresAt: number } | undefined; +type GlobalShardConfig = { resolution: MintShardSetResolution; override: unknown }; + +export type MintShardCache = { value: GlobalShardConfig; expiresAt: number } | undefined; export type ResolveMintShardDeps = { ceiling: string[]; @@ -160,23 +182,28 @@ export async function resolveMintShardWith( return "new"; } - let resolution: MintShardSetResolution; + let config: GlobalShardConfig; const cached = deps.cache.current; if (cached && cached.expiresAt > deps.nowMs) { - resolution = cached.value; + config = cached.value; } else { try { - resolution = readMintShardSetResolution(await deps.readFlags()); + const flags = await deps.readFlags(); + config = { + resolution: readMintShardSetResolution(flags), + override: flags[FEATURE_FLAG.runOpsMintShardOverride], + }; } catch (error) { deps.onReadFailed?.(error); return "new"; } - deps.cache.current = { value: resolution, expiresAt: deps.nowMs + deps.ttlMs }; + deps.cache.current = { value: config, expiresAt: deps.nowMs + deps.ttlMs }; } return computeMintShard(environment, { - resolution, + resolution: config.resolution, ceiling: deps.ceiling, + globalOverride: config.override, nowMs: deps.nowMs, graceMs: deps.graceMs, orgFeatureFlags: deps.orgFeatureFlags, @@ -188,7 +215,7 @@ const liveCache: { current: MintShardCache } = { current: undefined }; async function readSetFlags(): Promise> { const rows = await $replica.featureFlag.findMany({ - where: { key: { in: SET_KEYS } }, + where: { key: { in: GLOBAL_SHARD_KEYS } }, select: { key: true, value: true }, }); const flags: Record = {}; diff --git a/apps/webapp/test/runOpsMintShardFlags.test.ts b/apps/webapp/test/runOpsMintShardFlags.test.ts index 81ce7ed7fda..98b76eb19c8 100644 --- a/apps/webapp/test/runOpsMintShardFlags.test.ts +++ b/apps/webapp/test/runOpsMintShardFlags.test.ts @@ -69,16 +69,33 @@ describe("runOpsMintShardSet — the active list", () => { }); }); +describe("runOpsMintShardOverride — the complete-cutover lever", () => { + const key = FEATURE_FLAG.runOpsMintShardOverride; + + it("accepts a shard key and accepts new", () => { + expect(validateFeatureFlagValue(key, "a").success).toBe(true); + expect(validateFeatureFlagValue(key, "new").success).toBe(true); + }); + + it("rejects anything that is not a single legal key", () => { + for (const bad of ["A", "ab", "", "legacy", "a,b"]) { + expect(validateFeatureFlagValue(key, bad).success).toBe(false); + } + }); +}); + describe("scope locks match what each resolver actually reads", () => { it("locks the pins globally, because the resolver reads them from the org blob only", () => { expect(GLOBAL_LOCKED_FLAGS).toContain(FEATURE_FLAG.runOpsMintShard); expect(GLOBAL_LOCKED_FLAGS).toContain(FEATURE_FLAG.runOpsMintShardEnvPins); }); - it("locks the list per-org, because it is deployment-wide", () => { + it("locks the list and the override per-org, because both are deployment-wide", () => { expect(ORG_LOCKED_FLAGS).toContain(FEATURE_FLAG.runOpsMintShardSet); expect(ORG_LOCKED_FLAGS).toContain(FEATURE_FLAG.runOpsMintShardSetPrev); expect(ORG_LOCKED_FLAGS).toContain(FEATURE_FLAG.runOpsMintShardSetFlippedAt); + // An org that could override the cutover lever would defeat its purpose. + expect(ORG_LOCKED_FLAGS).toContain(FEATURE_FLAG.runOpsMintShardOverride); }); it("keeps the pins settable per-org, which is the canary lever", () => { @@ -93,6 +110,7 @@ describe("scope locks match what each resolver actually reads", () => { FEATURE_FLAG.runOpsMintShardSet, FEATURE_FLAG.runOpsMintShardSetPrev, FEATURE_FLAG.runOpsMintShardSetFlippedAt, + FEATURE_FLAG.runOpsMintShardOverride, ]) { expect(FeatureFlagCatalog).toHaveProperty(key); } From 87a9de2eb569ae941cb990198d12371719243192 Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Mon, 24 Aug 2026 10:01:37 +0100 Subject: [PATCH 05/17] refactor(webapp): drop the mint-shard ceiling env var before it ships 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. --- apps/webapp/app/env.server.ts | 25 ------ .../runOpsMintShard.server.test.ts | 81 +++---------------- .../runOpsMigration/runOpsMintShard.server.ts | 33 ++------ 3 files changed, 14 insertions(+), 125 deletions(-) diff --git a/apps/webapp/app/env.server.ts b/apps/webapp/app/env.server.ts index 851454acb2a..c9179306124 100644 --- a/apps/webapp/app/env.server.ts +++ b/apps/webapp/app/env.server.ts @@ -4,7 +4,6 @@ import { BoolEnv } from "./utils/boolEnv"; import { isValidDatabaseUrl } from "./utils/db"; import { isValidRegex } from "./utils/regex"; import { isValidDuration } from "./services/realtime/duration.server"; -import { parseShardCsv } from "./v3/runOpsMigration/mintShardGrace"; // `z.string()` constrained to a `parseDuration`-parseable string (e.g. // `7d`, `1h`). Validated at boot so a typo'd duration fails fast. @@ -42,23 +41,6 @@ const parseMachinePresetCsv = (raw: string, ctx: z.RefinementCtx): MachinePreset return out; }; -// A CSV of gen-2 mint shard keys, validated at boot by parseShardCsv. Kept as the raw string: -// the resolution is built once in runOpsMintShard.server.ts, and this only has to fail fast. -const shardCsvString = () => - z - .string() - .default("") - .superRefine((raw, ctx) => { - try { - parseShardCsv(raw); - } catch (error) { - ctx.addIssue({ - code: z.ZodIssueCode.custom, - message: error instanceof Error ? error.message : "invalid shard key CSV", - }); - } - }); - const GithubAppEnvSchema = z.preprocess( (val) => { const obj = val as any; @@ -2016,13 +1998,6 @@ const EnvironmentSchema = z // (stale or fresh) resolves to the same kind for the whole window. See mintFlipGrace.ts. RUN_OPS_MINT_FLIP_GRACE_MS: z.coerce.number().int().default(90_000), - // Gen-2 mint shards — CSV of single-char [a-z0-9] keys this deployment can mint roots into. - // Unset or empty means no gen-2 minting, which is today's behaviour. Validated at boot: an - // invalid key would mint an id that cannot be routed. This is a CEILING, not the live list: - // it changes only by deploy, and the runOpsMintShardSet flag selects from it at runtime. - // A rolling deploy runs two values of this var at once, so it must never be the ramp lever. - RUN_OPS_MINT_SHARDS: shardCsvString(), - // Session replication (Postgres → ClickHouse sessions_v1). Shares Redis // with the runs replicator for leader locking but has its own slot and // publication so the two consume independently. diff --git a/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.ts b/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.ts index 322c1322caf..771e52487bd 100644 --- a/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.ts +++ b/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.ts @@ -21,15 +21,12 @@ function envIds(count: number): string[] { return ids; } -const ALL_KEYS = "abcdefghijklmnopqrstuvwxyz0123456789".split(""); - function deps( resolution: MintShardSetResolution, overrides: Partial = {} ): MintShardDeps { return { resolution, - ceiling: ALL_KEYS, nowMs: T + GRACE_MS + 1, graceMs: GRACE_MS, orgFeatureFlags: undefined, @@ -54,23 +51,9 @@ describe("computeMintShard — the no-shards answer", () => { expect(computeMintShard({ id: "env_1" }, deps({ set: [] }))).toBe("new"); }); - it("returns new when the deployment configures no ceiling", () => { - // An unconfigured deployment is an unconditional kill switch, whatever the stored list says. - const resolution: MintShardSetResolution = { set: ["a", "b"] }; - expect(computeMintShard({ id: "env_1" }, deps(resolution, { ceiling: [] }))).toBe("new"); - }); - - it("returns new when the stored list names nothing this deployment can route", () => { - const resolution: MintShardSetResolution = { set: ["z"] }; - expect(computeMintShard({ id: "env_1" }, deps(resolution, { ceiling: ["a"] }))).toBe("new"); - }); - - it("returns new when the ceiling is empty even with a stale stamp present", () => { - const resolution: MintShardSetResolution = { set: [], prevSet: ["a"], flippedAtMs: T }; - // The ceiling gate MUST run before the grace, so no stored value can reopen a closed switch. - expect(computeMintShard({ id: "env_1" }, deps(resolution, { nowMs: T + 1, ceiling: [] }))).toBe( - "new" - ); + it("returns new when the grace serves an empty list", () => { + const resolution: MintShardSetResolution = { set: ["a"], prevSet: [], flippedAtMs: T }; + expect(computeMintShard({ id: "env_1" }, deps(resolution, { nowMs: T + 1 }))).toBe("new"); }); it("returns new when the grace serves an empty prevSet", () => { @@ -269,47 +252,11 @@ describe("computeMintShard — rendezvous properties", () => { }); }); -describe("computeMintShard — the ceiling bounds the stored list", () => { - it("mints only into keys the deployment can route", () => { - const resolution: MintShardSetResolution = { set: ["a", "b", "c"] }; - const ids = envIds(300); - for (const id of ids) { - const shard = computeMintShard({ id }, deps(resolution, { ceiling: ["a", "b"] })); - expect(["a", "b"]).toContain(shard); - } - }); - - it("ignores a pin to a key outside the ceiling", () => { - const resolution: MintShardSetResolution = { set: ["a", "c"] }; - const rejected: string[] = []; - const shard = computeMintShard( - { id: "env_1" }, - deps(resolution, { - ceiling: ["a"], - orgFeatureFlags: { runOpsMintShard: "c" }, - onPinRejected: (info) => rejected.push(info.pin), - }) - ); - expect(shard).toBe("a"); - expect(rejected).toEqual(["c"]); - }); - - it("still honours a gen-1 pin when the ceiling is narrower than the stored list", () => { - const resolution: MintShardSetResolution = { set: ["a", "b"] }; - const shard = computeMintShard( - { id: "env_1" }, - deps(resolution, { ceiling: ["a"], orgFeatureFlags: { runOpsMintShard: "new" } }) - ); - expect(shard).toBe("new"); - }); -}); - describe("resolveMintShardWith — cache, ceiling short-circuit and fail-safe", () => { function wrapperDeps( overrides: Partial = {} ): ResolveMintShardDeps & { reads: number } { const state = { - ceiling: ["a", "b"], readFlags: async () => ({ runOpsMintShardSet: "a,b" }), cache: { current: undefined as MintShardCache }, nowMs: T, @@ -327,12 +274,6 @@ describe("resolveMintShardWith — cache, ceiling short-circuit and fail-safe", return state; } - it("never reads the list when the deployment configures no ceiling", async () => { - const deps = wrapperDeps({ ceiling: [] }); - expect(await resolveMintShardWith({ id: "env_1" }, deps)).toBe("new"); - expect(deps.reads).toBe(0); - }); - it("reads once, then serves the cache until the TTL expires", async () => { const deps = wrapperDeps(); await resolveMintShardWith({ id: "env_1" }, deps); @@ -368,11 +309,8 @@ describe("resolveMintShardWith — cache, ceiling short-circuit and fail-safe", expect(["a", "b"]).toContain(await resolveMintShardWith({ id: "env_1" }, deps)); }); - it("returns gen-1 when the stored list names nothing inside the ceiling", async () => { - const deps = wrapperDeps({ - ceiling: ["a"], - readFlags: async () => ({ runOpsMintShardSet: "z" }), - }); + it("returns gen-1 when the stored list is empty", async () => { + const deps = wrapperDeps({ readFlags: async () => ({ runOpsMintShardSet: "" }) }); expect(await resolveMintShardWith({ id: "env_1" }, deps)).toBe("new"); }); @@ -383,7 +321,6 @@ describe("resolveMintShardWith — cache, ceiling short-circuit and fail-safe", { id: "env_1" }, { resolution: { set: ["a", "b"] }, - ceiling: ["a", "b"], nowMs: T, graceMs: GRACE_MS, orgFeatureFlags: undefined, @@ -451,9 +388,9 @@ describe("computeMintShard — the global override wins the complete cutover", ( } }); - it("cannot resurrect minting when the ceiling is empty", () => { - expect( - computeMintShard({ id: "env_1" }, deps(resolution, { globalOverride: "b", ceiling: [] })) - ).toBe("new"); + it("cannot resurrect minting when the list is empty", () => { + expect(computeMintShard({ id: "env_1" }, deps({ set: [] }, { globalOverride: "b" }))).toBe( + "new" + ); }); }); diff --git a/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts b/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts index 6f2ea5a591f..e7b9e2af8a7 100644 --- a/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts +++ b/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts @@ -8,7 +8,6 @@ import { effectiveMintShardSet, GEN_1_PIN_VALUE, isValidPinValue, - parseShardCsv, readMintShardSetResolution, type MintShardSetResolution, } from "./mintShardGrace"; @@ -16,8 +15,6 @@ import { export type MintShardDeps = { // The live list, from the control-plane database. resolution: MintShardSetResolution; - // The keys this deployment can route, from the environment. Bounds the live list. - ceiling: string[]; // Fleet-wide pin that beats every per-org and per-env pin. The complete-cutover lever. globalOverride?: unknown; nowMs: number; @@ -89,20 +86,15 @@ function hrwSelect(environmentId: string, activeSet: string[]): string { // PURE CORE — no env, no clock, no I/O; tests drive this directly. Deterministic for fixed // deps, which is what lets run minting and token minting agree on one answer. // -// The ceiling gate runs BEFORE the grace, so an unconfigured deployment is an unconditional -// kill switch that no stored value can reopen. The live list is then intersected with the -// ceiling, so a stored key this deployment cannot route is never minted into. +// An empty list is the off state, and it is the state of every deployment that has not set the +// flag. Bounding the list against the shard keys this deployment can actually route belongs with +// the shard descriptors, which own that information; nothing here mints, so nothing can misroute. // // A pin outside the active set falls through to the hash rather than throwing: honouring it // would leak the drain the active list performs, and throwing would fail customer triggers // whenever a pinned shard drains. export function computeMintShard(environment: { id: string }, deps: MintShardDeps): ShardKey { - if (deps.ceiling.length === 0) { - return "new"; - } - - const live = effectiveMintShardSet(deps.resolution, deps.nowMs, deps.graceMs); - const activeSet = live.filter((key) => deps.ceiling.includes(key)); + const activeSet = effectiveMintShardSet(deps.resolution, deps.nowMs, deps.graceMs); if (activeSet.length === 0) { return "new"; } @@ -134,13 +126,6 @@ export function computeMintShard(environment: { id: string }, deps: MintShardDep return hrwSelect(environment.id, activeSet); } -// ENV-BOUND wrapper — the only place env is read. The ceiling is parsed once at boot; it is a -// deploy-time value, so re-parsing per mint would burn CPU on the hottest path in the system. -// -// SEAM: this is the one place the ceiling is sourced. Once shard descriptors are configured, the -// ceiling becomes their keys and RUN_OPS_MINT_SHARDS is deleted. Two hand-kept lists would drift. -const ceiling: string[] = parseShardCsv(env.RUN_OPS_MINT_SHARDS); - // Read together so the override costs no extra query beyond the list it is bounded by. const GLOBAL_SHARD_KEYS = [ FEATURE_FLAG.runOpsMintShardSet, @@ -154,8 +139,7 @@ type GlobalShardConfig = { resolution: MintShardSetResolution; override: unknown export type MintShardCache = { value: GlobalShardConfig; expiresAt: number } | undefined; export type ResolveMintShardDeps = { - ceiling: string[]; - // Reads the three list rows. Injected so the cache and the fail-safe are testable without a + // Reads the list rows. Injected so the cache and the fail-safe are testable without a // database, the same way computeRunIdMintKind takes its flag reader. readFlags: () => Promise>; cache: { current: MintShardCache }; @@ -177,11 +161,6 @@ export async function resolveMintShardWith( environment: { id: string; orgFeatureFlags?: unknown }, deps: ResolveMintShardDeps ): Promise { - // No ceiling means no gen-2 minting, so skip the read entirely. - if (deps.ceiling.length === 0) { - return "new"; - } - let config: GlobalShardConfig; const cached = deps.cache.current; if (cached && cached.expiresAt > deps.nowMs) { @@ -202,7 +181,6 @@ export async function resolveMintShardWith( return computeMintShard(environment, { resolution: config.resolution, - ceiling: deps.ceiling, globalOverride: config.override, nowMs: deps.nowMs, graceMs: deps.graceMs, @@ -251,7 +229,6 @@ export async function resolveMintShard(environment: { orgFeatureFlags?: unknown; }): Promise { return resolveMintShardWith(environment, { - ceiling, readFlags: readSetFlags, cache: liveCache, nowMs: Date.now(), From 5457bfd49d7d08b76747c299991043ed261b39b6 Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Mon, 24 Aug 2026 10:19:37 +0100 Subject: [PATCH 06/17] fix(webapp): keep unset working on the graced global flags 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. --- apps/webapp/app/v3/featureFlags.server.ts | 189 ++++++++++-------- apps/webapp/app/v3/featureFlags.ts | 11 +- .../runOpsMintShard.server.test.ts | 23 ++- .../runOpsMigration/runOpsMintShard.server.ts | 53 ++++- .../test/runOpsMintGlobalFlipLock.test.ts | 8 +- .../test/runOpsMintShardSetFlip.test.ts | 67 +++++++ 6 files changed, 251 insertions(+), 100 deletions(-) diff --git a/apps/webapp/app/v3/featureFlags.server.ts b/apps/webapp/app/v3/featureFlags.server.ts index e30b5df4cec..e9aad8eb4e1 100644 --- a/apps/webapp/app/v3/featureFlags.server.ts +++ b/apps/webapp/app/v3/featureFlags.server.ts @@ -181,23 +181,21 @@ export function makeSetMultipleFlags(_prisma: PrismaClientOrTransaction = prisma }; } -// Read -> stamp -> write the global mint-kind grace metadata in one transaction. The three -// FeatureFlag rows may not exist yet, so a row FOR UPDATE can't lock them; an advisory xact lock -// serializes concurrent global flips so one can't clobber another's grace stamp (mirrors per-org). -// Every group of global flags whose value carries its own grace stamp. One transaction and one -// lock cover all of them, so a save that flips two groups can never stamp one and lose the other. +// Global flag groups whose value carries its own grace stamp. `primary` is operator-supplied; +// `derived` is computed server-side and must never be written from a request body. The pair is +// named explicitly rather than by position, so a group declared in another order stays correct. const GRACED_GLOBAL_GROUPS = [ { - keys: [ - FEATURE_FLAG.runOpsMintKind, + primary: FEATURE_FLAG.runOpsMintKind as FeatureFlagKey, + derived: [ FEATURE_FLAG.runOpsMintKindPrev, FEATURE_FLAG.runOpsMintKindFlippedAt, ] as FeatureFlagKey[], stamp: stampMintKindFlip, }, { - keys: [ - FEATURE_FLAG.runOpsMintShardSet, + primary: FEATURE_FLAG.runOpsMintShardSet as FeatureFlagKey, + derived: [ FEATURE_FLAG.runOpsMintShardSetPrev, FEATURE_FLAG.runOpsMintShardSetFlippedAt, ] as FeatureFlagKey[], @@ -205,51 +203,90 @@ const GRACED_GLOBAL_GROUPS = [ }, ] as const; -// Keys the graced path owns. They never take a bare upsert and never enter the replace sweep, -// because a server-computed stamp must not be written from a request body nor swept away. -const GRACED_GLOBAL_KEYS: FeatureFlagKey[] = GRACED_GLOBAL_GROUPS.flatMap((g) => g.keys); +const GRACED_GLOBAL_KEYS: FeatureFlagKey[] = GRACED_GLOBAL_GROUPS.flatMap((g) => [ + g.primary, + ...g.derived, +]); -export async function applyGlobalGracedFlips( - client: PrismaClient, - requestedFlags: Partial>, - graceMs: number -): Promise<{ key: string; value: any }[]> { - return client.$transaction(async (tx) => { - await tx.$executeRaw`SELECT pg_advisory_xact_lock(hashtext('runops-global-graced-flag-flip'))`; +function gracedGroupFor(key: FeatureFlagKey) { + return GRACED_GLOBAL_GROUPS.find((g) => g.primary === key || g.derived.includes(key)); +} - const existingRows = await tx.featureFlag.findMany({ - where: { key: { in: GRACED_GLOBAL_KEYS } }, - select: { key: true, value: true }, - }); - const existingGlobal: Record = {}; - for (const row of existingRows) { - existingGlobal[row.key] = row.value; +// Strips every derived key: a grace stamp is computed here, never accepted from a caller. +function withoutDerivedKeys( + requestedFlags: Partial> +): Record { + const out: Record = { ...requestedFlags }; + for (const group of GRACED_GLOBAL_GROUPS) { + for (const derived of group.derived) { + delete out[derived]; } + } + return out; +} - // Anchor the cutover to the control-plane DB clock, not this process's wall clock. A rolling - // deploy spans hours, so every pod must date the window against one shared clock. - const [{ now }] = await tx.$queryRaw<{ now: Date }[]>`SELECT now() AS now`; - - let stamped: Record = { ...requestedFlags }; - for (const group of GRACED_GLOBAL_GROUPS) { - stamped = group.stamp(existingGlobal, stamped, now.getTime(), graceMs); - } +// The rows may not exist yet, so a row FOR UPDATE cannot lock them; an advisory xact lock +// serializes concurrent global flips instead, so one cannot clobber another's stamp. +// +// Two lock ids are taken, in a fixed order. The second is this release's name; the first is the +// name an older release still takes. A deploy rolls for hours, so both versions write at once, +// and dropping the old id would leave those writers serializing against nothing. Remove the +// legacy id one release after this one ships. +async function lockGracedGroups(tx: PrismaClientOrTransaction): Promise { + await tx.$executeRaw`SELECT pg_advisory_xact_lock(hashtext('runops-global-mint-kind-flip'))`; + await tx.$executeRaw`SELECT pg_advisory_xact_lock(hashtext('runops-global-graced-flag-flip'))`; +} - return makeSetMultipleFlags(tx)(stamped as Partial>); +// Reads each group's current rows and returns the requested flags plus a fresh stamp for every +// group the save actually changes. A group whose primary the save omits is left untouched. +async function stampGracedGroups( + tx: PrismaClientOrTransaction, + requestedFlags: Record, + graceMs: number +): Promise> { + const existingRows = await tx.featureFlag.findMany({ + where: { key: { in: GRACED_GLOBAL_KEYS } }, + select: { key: true, value: true }, }); + const existingGlobal: Record = {}; + for (const row of existingRows) { + existingGlobal[row.key] = row.value; + } + + // Anchor the cutover to the control-plane DB clock, not this process's wall clock. A rolling + // deploy spans hours, so every pod must date the window against one shared clock. + const [{ now }] = await tx.$queryRaw<{ now: Date }[]>`SELECT now() AS now`; + + let stamped: Record = { ...requestedFlags }; + for (const group of GRACED_GLOBAL_GROUPS) { + stamped = group.stamp(existingGlobal, stamped, now.getTime(), graceMs); + } + return stamped; } -/** @deprecated Prefer applyGlobalGracedFlips, which stamps every graced group in one lock. */ -export async function applyGlobalMintKindFlip( +// Merge-semantics write: sets what the caller asked for, stamps any graced group it changes, and +// touches nothing else. Used by the JSON admin API. +export async function applyGlobalGracedFlips( client: PrismaClient, requestedFlags: Partial>, graceMs: number ): Promise<{ key: string; value: any }[]> { - return applyGlobalGracedFlips(client, requestedFlags, graceMs); + return client.$transaction(async (tx) => { + await lockGracedGroups(tx); + const stamped = await stampGracedGroups(tx, withoutDerivedKeys(requestedFlags), graceMs); + return makeSetMultipleFlags(tx)(stamped as Partial>); + }); } -// Replace-semantics write for the global admin flags page: upsert submitted catalog flags, delete -// omitted ones unless protected, and route any graced group through the stamped path. +// Replace-semantics write for the global admin flags page: submitted flags upsert, omitted ones +// delete unless protected. One transaction covers the stamp, the upserts and the deletes, so a +// save cannot half-apply. +// +// A graced group is all-or-nothing. Submitting its primary writes the group with a fresh stamp. +// Omitting its primary deletes the primary AND its stamp together, because a stamp left behind +// without its primary keeps being served: {set: [], prevSet: [a], flippedAt: t} resolves to [a] +// for the rest of the window, which would mint into a shard the operator just removed. The +// delete ignores `isProtected` for the derived keys for the same reason. export async function replaceGlobalFeatureFlags( client: PrismaClient, params: { @@ -259,50 +296,40 @@ export async function replaceGlobalFeatureFlags( graceMs: number; } ): Promise { - // Derived stamp fields are computed server-side; never trust them from the body. - const requestedFlags: Record = { ...params.requestedFlags }; - for (const group of GRACED_GLOBAL_GROUPS) { - for (const derived of group.keys.slice(1)) { - delete requestedFlags[derived]; - } - } + const requestedFlags = withoutDerivedKeys(params.requestedFlags); - const touchesGracedGroup = GRACED_GLOBAL_GROUPS.some( - (group) => requestedFlags[group.keys[0]] !== undefined - ); - if (touchesGracedGroup) { - await applyGlobalGracedFlips( - client, - requestedFlags as Partial>, - params.graceMs - ); - } + await client.$transaction(async (tx) => { + await lockGracedGroups(tx); + const stamped = await stampGracedGroups(tx, requestedFlags, params.graceMs); - const upsertOps: ReturnType[] = []; - const keysToDelete: string[] = []; + const toWrite: Record = {}; + const keysToDelete: string[] = []; - for (const key of params.catalogKeys) { - if (GRACED_GLOBAL_KEYS.includes(key)) { - continue; - } - if (key in requestedFlags) { - const value = requestedFlags[key]; - upsertOps.push( - client.featureFlag.upsert({ - where: { key }, - create: { key, value: value as any }, - update: { value: value as any }, - }) - ); - } else if (!params.isProtected(key)) { - keysToDelete.push(key); + for (const key of params.catalogKeys) { + const group = gracedGroupFor(key); + + if (group) { + if (requestedFlags[group.primary] !== undefined) { + if (stamped[key] !== undefined) { + toWrite[key] = stamped[key]; + } + } else if (!params.isProtected(group.primary)) { + keysToDelete.push(key); + } + continue; + } + + if (key in requestedFlags) { + toWrite[key] = requestedFlags[key]; + } else if (!params.isProtected(key)) { + keysToDelete.push(key); + } } - } - await client.$transaction([ - ...upsertOps, - ...(keysToDelete.length > 0 - ? [client.featureFlag.deleteMany({ where: { key: { in: boundedIn(keysToDelete) } } })] - : []), - ]); + await makeSetMultipleFlags(tx)(toWrite as Partial>); + + if (keysToDelete.length > 0) { + await tx.featureFlag.deleteMany({ where: { key: { in: boundedIn(keysToDelete) } } }); + } + }); } diff --git a/apps/webapp/app/v3/featureFlags.ts b/apps/webapp/app/v3/featureFlags.ts index ee4491e7840..f241aaf4da2 100644 --- a/apps/webapp/app/v3/featureFlags.ts +++ b/apps/webapp/app/v3/featureFlags.ts @@ -101,7 +101,7 @@ export const FeatureFlagCatalog = { [FEATURE_FLAG.runOpsMintKindFlippedAt]: z.string().datetime(), // Pins one org to a gen-2 mint shard. "new" holds the org on gen-1 run-ops ids, which is how // a canary keeps the fleet's default while one org moves. Only honored while the key is in - // the active set (RUN_OPS_MINT_SHARDS); a drained key falls through to the hash. + // the active list; a drained key falls through to the hash. [FEATURE_FLAG.runOpsMintShard]: z .string() .refine((v) => v === "new" || /^[a-z0-9]$/.test(v), 'must be a single [a-z0-9] char, or "new"'), @@ -125,8 +125,8 @@ export const FeatureFlagCatalog = { } } }), - // 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. + // CSV of the shard keys eligible for root minting right now. Empty means no gen-2 minting. + // Reserved keys are rejected, because "new" already means gen-1. [FEATURE_FLAG.runOpsMintShardSet]: z.string().refine( (v) => v @@ -166,6 +166,11 @@ export const GLOBAL_LOCKED_FLAGS: FeatureFlagKey[] = [ FEATURE_FLAG.taskEventRepository, FEATURE_FLAG.runOpsMintShard, FEATURE_FLAG.runOpsMintShardEnvPins, + // Grace stamps are computed server-side. An editable control here would discard what it saves. + FEATURE_FLAG.runOpsMintKindPrev, + FEATURE_FLAG.runOpsMintKindFlippedAt, + FEATURE_FLAG.runOpsMintShardSetPrev, + FEATURE_FLAG.runOpsMintShardSetFlippedAt, ]; // Flags that are read-only on the org-level dialog. diff --git a/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.ts b/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.ts index 771e52487bd..417ce8e2673 100644 --- a/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.ts +++ b/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.ts @@ -51,6 +51,11 @@ describe("computeMintShard — the no-shards answer", () => { expect(computeMintShard({ id: "env_1" }, deps({ set: [] }))).toBe("new"); }); + it("returns new when a stale stamp is present but both lists are empty", () => { + const resolution: MintShardSetResolution = { set: [], prevSet: [], flippedAtMs: T }; + expect(computeMintShard({ id: "env_1" }, deps(resolution, { nowMs: T + 1 }))).toBe("new"); + }); + it("returns new when the grace serves an empty list", () => { const resolution: MintShardSetResolution = { set: ["a"], prevSet: [], flippedAtMs: T }; expect(computeMintShard({ id: "env_1" }, deps(resolution, { nowMs: T + 1 }))).toBe("new"); @@ -252,7 +257,7 @@ describe("computeMintShard — rendezvous properties", () => { }); }); -describe("resolveMintShardWith — cache, ceiling short-circuit and fail-safe", () => { +describe("resolveMintShardWith — cache, read failure and fail-safe", () => { function wrapperDeps( overrides: Partial = {} ): ResolveMintShardDeps & { reads: number } { @@ -374,13 +379,27 @@ describe("computeMintShard — the global override wins the complete cutover", ( deps(resolution, { globalOverride: "z", orgFeatureFlags: { runOpsMintShard: "a" }, - onPinRejected: (info) => rejected.push(info.pin), + onOverrideRejected: (info) => rejected.push(info.override), }) ); expect(shard).toBe("a"); expect(rejected).toEqual(["z"]); }); + it("reports a bad override WITHOUT the environment id, so one line covers the fleet", () => { + // Keying the report by environment would log once per environment for a fleet-wide setting. + const seen: Array<{ override: string }> = []; + for (const id of envIds(50)) { + computeMintShard( + { id }, + deps(resolution, { globalOverride: "z", onOverrideRejected: (i) => seen.push(i) }) + ); + } + expect(seen).toHaveLength(50); + expect(new Set(seen.map((i) => i.override))).toEqual(new Set(["z"])); + expect(seen.every((i) => !("environmentId" in i))).toBe(true); + }); + it("is ignored when it is not a legal value", () => { for (const bad of ["legacy", "AB", "", "a,b"]) { const shard = computeMintShard({ id: "env_1" }, deps(resolution, { globalOverride: bad })); diff --git a/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts b/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts index e7b9e2af8a7..02a432fb537 100644 --- a/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts +++ b/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts @@ -3,6 +3,8 @@ import type { ShardKey } from "@trigger.dev/core/v3/isomorphic"; import { $replica } from "~/db.server"; import { env } from "~/env.server"; import { logger } from "~/services/logger.server"; +import { BoundedTtlCache } from "~/services/realtime/boundedTtlCache"; +import { singleton } from "~/utils/singleton"; import { FEATURE_FLAG } from "~/v3/featureFlags"; import { effectiveMintShardSet, @@ -21,6 +23,7 @@ export type MintShardDeps = { graceMs: number; orgFeatureFlags: unknown; onPinRejected?: (info: { environmentId: string; pin: string; activeSet: string[] }) => void; + onOverrideRejected?: (info: { override: string; activeSet: string[] }) => void; }; function asRecord(value: unknown): Record | undefined { @@ -109,7 +112,8 @@ export function computeMintShard(environment: { id: string }, deps: MintShardDep if (activeSet.includes(override)) { return override; } - deps.onPinRejected?.({ environmentId: environment.id, pin: override, activeSet }); + // Fleet-wide, so it is reported once for the value, not once per environment. + deps.onOverrideRejected?.({ override, activeSet }); } const pin = readPin(deps.orgFeatureFlags, environment.id); @@ -138,6 +142,11 @@ type GlobalShardConfig = { resolution: MintShardSetResolution; override: unknown export type MintShardCache = { value: GlobalShardConfig; expiresAt: number } | undefined; +// A misconfiguration is reported again after this long, so a still-broken pin stays visible +// without logging on every trigger. +const REPORT_TTL_MS = 3_600_000; +const REPORT_MAX_ENTRIES = 10_000; + export type ResolveMintShardDeps = { // Reads the list rows. Injected so the cache and the fail-safe are testable without a // database, the same way computeRunIdMintKind takes its flag reader. @@ -148,12 +157,15 @@ export type ResolveMintShardDeps = { graceMs: number; orgFeatureFlags: unknown; onPinRejected?: (info: { environmentId: string; pin: string; activeSet: string[] }) => void; + onOverrideRejected?: (info: { override: string; activeSet: string[] }) => void; onReadFailed?: (error: unknown) => void; }; -// The live list is org-independent, so one process-wide entry serves every mint. One query per -// process per TTL, over one round-trip. The TTL bounds how long two processes can disagree, -// which is what the grace window is sized against. +// The live list is org-independent, so one process-wide entry serves every mint: one query per +// process per TTL, over one round-trip. Two processes can therefore disagree for the TTL PLUS the +// replica lag behind the read, which can exceed graceMs. That is tolerable here and only here, +// because a gen-2 id carries its own shard key, so disagreement cannot misroute an existing run; +// it only decides where the next root lands, and every failure direction is toward gen-1. // // A failed read falls back to gen-1 rather than guessing a list. Guessing would move every // environment's placement for the length of one blip. @@ -186,10 +198,13 @@ export async function resolveMintShardWith( graceMs: deps.graceMs, orgFeatureFlags: deps.orgFeatureFlags, onPinRejected: deps.onPinRejected, + onOverrideRejected: deps.onOverrideRejected, }); } -const liveCache: { current: MintShardCache } = { current: undefined }; +const liveCache = singleton("runOpsMintShardCache", (): { current: MintShardCache } => ({ + current: undefined, +})); async function readSetFlags(): Promise> { const rows = await $replica.featureFlag.findMany({ @@ -203,20 +218,37 @@ async function readSetFlags(): Promise> { return flags; } -// Once per environment per process: a stale pin sits on the root-trigger path and would -// otherwise log on every trigger for that environment, indefinitely. -const reportedPins = new Set(); +// A stale pin sits on the root-trigger path, so it would otherwise log on every trigger for that +// environment forever. Bounded, because the set of pinned environments is operator-controlled but +// not operator-bounded, and an unbounded Set on this path is a leak. +const reportedPins = singleton( + "runOpsMintShardReportedPins", + () => new BoundedTtlCache(REPORT_TTL_MS, REPORT_MAX_ENTRIES) +); function reportPinRejected(info: { environmentId: string; pin: string; activeSet: string[]; }): void { - if (reportedPins.has(info.environmentId)) return; - reportedPins.add(info.environmentId); + if (reportedPins.get(info.environmentId) !== undefined) return; + reportedPins.set(info.environmentId, true); logger.error("[runOpsMintShard] pinned shard is not in the active set; using the hash", info); } +// Keyed by the override value, not by environment: one bad override applies to the whole fleet, +// so one line is the correct volume. Keying by environment would log once per environment. +const reportedOverrides = singleton( + "runOpsMintShardReportedOverrides", + () => new BoundedTtlCache(REPORT_TTL_MS, REPORT_MAX_ENTRIES) +); + +function reportOverrideRejected(info: { override: string; activeSet: string[] }): void { + if (reportedOverrides.get(info.override) !== undefined) return; + reportedOverrides.set(info.override, true); + logger.error("[runOpsMintShard] override shard is not in the active set; ignoring it", info); +} + /** * Which shard an environment mints new roots into. Call only after resolveRunIdMintKind has * returned "runOpsId". Returns "new" to mean a gen-1 run-ops id, which is today's behaviour. @@ -236,6 +268,7 @@ export async function resolveMintShard(environment: { graceMs: env.RUN_OPS_MINT_FLIP_GRACE_MS, orgFeatureFlags: environment.orgFeatureFlags, onPinRejected: reportPinRejected, + onOverrideRejected: reportOverrideRejected, onReadFailed: (error) => logger.error("[runOpsMintShard] shard-set read failed; minting gen-1 (fail-safe)", { error }), }); diff --git a/apps/webapp/test/runOpsMintGlobalFlipLock.test.ts b/apps/webapp/test/runOpsMintGlobalFlipLock.test.ts index 1492fd02c43..b3acd7e45eb 100644 --- a/apps/webapp/test/runOpsMintGlobalFlipLock.test.ts +++ b/apps/webapp/test/runOpsMintGlobalFlipLock.test.ts @@ -5,7 +5,7 @@ import type { PrismaClient } from "@trigger.dev/database"; import { postgresTest } from "@internal/testcontainers"; import { describe, expect, vi } from "vitest"; import { FEATURE_FLAG } from "~/v3/featureFlags"; -import { applyGlobalMintKindFlip, makeSetMultipleFlags } from "~/v3/featureFlags.server"; +import { applyGlobalGracedFlips, makeSetMultipleFlags } from "~/v3/featureFlags.server"; vi.setConfig({ testTimeout: 60_000 }); @@ -25,11 +25,11 @@ async function readGlobalMint(prisma: PrismaClient): Promise { +describe("applyGlobalGracedFlips — transactional stamp + serialized flips", () => { postgresTest("a genuine global flip stamps prev + flippedAt", async ({ prisma }) => { await makeSetMultipleFlags(prisma)({ [FEATURE_FLAG.runOpsMintKind]: "cuid" }); - await applyGlobalMintKindFlip(prisma, { [FEATURE_FLAG.runOpsMintKind]: "runOpsId" }, 60_000); + await applyGlobalGracedFlips(prisma, { [FEATURE_FLAG.runOpsMintKind]: "runOpsId" }, 60_000); const m = await readGlobalMint(prisma); expect(m[FEATURE_FLAG.runOpsMintKind]).toBe("runOpsId"); @@ -44,7 +44,7 @@ describe("applyGlobalMintKindFlip — transactional stamp + serialized flips", ( await Promise.all( Array.from({ length: 8 }, () => - applyGlobalMintKindFlip(prisma, { [FEATURE_FLAG.runOpsMintKind]: "runOpsId" }, 60_000) + applyGlobalGracedFlips(prisma, { [FEATURE_FLAG.runOpsMintKind]: "runOpsId" }, 60_000) ) ); diff --git a/apps/webapp/test/runOpsMintShardSetFlip.test.ts b/apps/webapp/test/runOpsMintShardSetFlip.test.ts index 55ebe43a2d6..1cc3c2e009a 100644 --- a/apps/webapp/test/runOpsMintShardSetFlip.test.ts +++ b/apps/webapp/test/runOpsMintShardSetFlip.test.ts @@ -174,6 +174,73 @@ describe("replaceGlobalFeatureFlags — the admin page cannot bypass the stamp", expect(m[FEATURE_FLAG.runOpsMintShardSet]).toBe("a,b"); }); + postgresTest( + "omitting the list DELETES it, so unset still turns minting off", + async ({ prisma }) => { + // The admin page's unset button omits the key. If the save skipped it, unset would be a + // silent no-op and gen-2 minting would stay armed. + await replaceGlobalFeatureFlags(prisma, { + requestedFlags: { [FEATURE_FLAG.runOpsMintShardSet]: "a,b" }, + catalogKeys: CATALOG_KEYS, + isProtected: NEVER_PROTECTED, + graceMs: 60_000, + }); + + await replaceGlobalFeatureFlags(prisma, { + requestedFlags: {}, + catalogKeys: CATALOG_KEYS, + isProtected: NEVER_PROTECTED, + graceMs: 60_000, + }); + + const m = await readFlags(prisma, SET_KEYS); + // The stamp goes with it: a stamp without its list keeps being served for the whole window. + expect(m[FEATURE_FLAG.runOpsMintShardSet]).toBeUndefined(); + expect(m[FEATURE_FLAG.runOpsMintShardSetPrev]).toBeUndefined(); + expect(m[FEATURE_FLAG.runOpsMintShardSetFlippedAt]).toBeUndefined(); + } + ); + + postgresTest("omitting the mint kind still deletes its trio", async ({ prisma }) => { + await replaceGlobalFeatureFlags(prisma, { + requestedFlags: { [FEATURE_FLAG.runOpsMintKind]: "runOpsId" }, + catalogKeys: CATALOG_KEYS, + isProtected: NEVER_PROTECTED, + graceMs: 60_000, + }); + + await replaceGlobalFeatureFlags(prisma, { + requestedFlags: {}, + catalogKeys: CATALOG_KEYS, + isProtected: NEVER_PROTECTED, + graceMs: 60_000, + }); + + const m = await readFlags(prisma, MINT_KIND_KEYS); + expect(m[FEATURE_FLAG.runOpsMintKind]).toBeUndefined(); + expect(m[FEATURE_FLAG.runOpsMintKindPrev]).toBeUndefined(); + expect(m[FEATURE_FLAG.runOpsMintKindFlippedAt]).toBeUndefined(); + }); + + postgresTest("a protected list is not deleted when omitted", async ({ prisma }) => { + await replaceGlobalFeatureFlags(prisma, { + requestedFlags: { [FEATURE_FLAG.runOpsMintShardSet]: "a" }, + catalogKeys: CATALOG_KEYS, + isProtected: NEVER_PROTECTED, + graceMs: 60_000, + }); + + await replaceGlobalFeatureFlags(prisma, { + requestedFlags: {}, + catalogKeys: CATALOG_KEYS, + isProtected: (key) => key === FEATURE_FLAG.runOpsMintShardSet, + graceMs: 60_000, + }); + + const m = await readFlags(prisma, SET_KEYS); + expect(m[FEATURE_FLAG.runOpsMintShardSet]).toBe("a"); + }); + postgresTest("an ordinary flag keeps replace semantics", async ({ prisma }) => { await makeSetMultipleFlags(prisma)({ [FEATURE_FLAG.mollifierEnabled]: true }); From 28bf05fe258e85bb306e69970743b940cd7974c0 Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Mon, 24 Aug 2026 10:27:15 +0100 Subject: [PATCH 07/17] test(webapp): drop a stale assertion that contradicted the graced-group 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. --- apps/webapp/test/runOpsMintShardSetFlip.test.ts | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/apps/webapp/test/runOpsMintShardSetFlip.test.ts b/apps/webapp/test/runOpsMintShardSetFlip.test.ts index 1cc3c2e009a..ec354b92075 100644 --- a/apps/webapp/test/runOpsMintShardSetFlip.test.ts +++ b/apps/webapp/test/runOpsMintShardSetFlip.test.ts @@ -155,16 +155,20 @@ describe("replaceGlobalFeatureFlags — the admin page cannot bypass the stamp", expect(m[FEATURE_FLAG.runOpsMintShardSetFlippedAt]).not.toBe("1999-01-01T00:00:00.000Z"); }); - postgresTest("the set trio survives a save that omits the set keys", async ({ prisma }) => { + postgresTest("a co-submitted flag does not disturb a resubmitted list", async ({ prisma }) => { await replaceGlobalFeatureFlags(prisma, { requestedFlags: { [FEATURE_FLAG.runOpsMintShardSet]: "a,b" }, catalogKeys: CATALOG_KEYS, isProtected: NEVER_PROTECTED, graceMs: 60_000, }); + const first = await readFlags(prisma, SET_KEYS); await replaceGlobalFeatureFlags(prisma, { - requestedFlags: { [FEATURE_FLAG.mollifierEnabled]: true }, + requestedFlags: { + [FEATURE_FLAG.runOpsMintShardSet]: "a,b", + [FEATURE_FLAG.mollifierEnabled]: true, + }, catalogKeys: CATALOG_KEYS, isProtected: NEVER_PROTECTED, graceMs: 60_000, @@ -172,6 +176,10 @@ describe("replaceGlobalFeatureFlags — the admin page cannot bypass the stamp", const m = await readFlags(prisma, SET_KEYS); expect(m[FEATURE_FLAG.runOpsMintShardSet]).toBe("a,b"); + // Resubmitting the same list is not a flip, so the cutover clock is not reset. + expect(m[FEATURE_FLAG.runOpsMintShardSetFlippedAt]).toBe( + first[FEATURE_FLAG.runOpsMintShardSetFlippedAt] + ); }); postgresTest( From 86402de1f142a77d4208951f5e300b7f93463688 Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Mon, 24 Aug 2026 10:53:31 +0100 Subject: [PATCH 08/17] fix(webapp): route the graced flag writes through the traced transaction 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. --- apps/webapp/app/v3/featureFlags.server.ts | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/apps/webapp/app/v3/featureFlags.server.ts b/apps/webapp/app/v3/featureFlags.server.ts index e9aad8eb4e1..b5d65a13cab 100644 --- a/apps/webapp/app/v3/featureFlags.server.ts +++ b/apps/webapp/app/v3/featureFlags.server.ts @@ -1,6 +1,6 @@ import { type z } from "zod"; import type { PrismaClient } from "@trigger.dev/database"; -import { prisma, type PrismaClientOrTransaction } from "~/db.server"; +import { $transaction, prisma, type PrismaClientOrTransaction } from "~/db.server"; import { FEATURE_FLAG, type FeatureFlagCatalogSchema, @@ -271,11 +271,18 @@ export async function applyGlobalGracedFlips( requestedFlags: Partial>, graceMs: number ): Promise<{ key: string; value: any }[]> { - return client.$transaction(async (tx) => { + const applied = await $transaction(client, "applyGlobalGracedFlips", async (tx) => { await lockGracedGroups(tx); const stamped = await stampGracedGroups(tx, withoutDerivedKeys(requestedFlags), graceMs); return makeSetMultipleFlags(tx)(stamped as Partial>); }); + + // The helper resolves undefined rather than throwing when Prisma swallows an infrastructure + // error. This write stamps a cutover window, so a transaction that did not run must be loud. + if (!applied) { + throw new Error("applyGlobalGracedFlips: transaction did not complete"); + } + return applied; } // Replace-semantics write for the global admin flags page: submitted flags upsert, omitted ones @@ -298,7 +305,7 @@ export async function replaceGlobalFeatureFlags( ): Promise { const requestedFlags = withoutDerivedKeys(params.requestedFlags); - await client.$transaction(async (tx) => { + const applied = await $transaction(client, "replaceGlobalFeatureFlags", async (tx) => { await lockGracedGroups(tx); const stamped = await stampGracedGroups(tx, requestedFlags, params.graceMs); @@ -331,5 +338,12 @@ export async function replaceGlobalFeatureFlags( if (keysToDelete.length > 0) { await tx.featureFlag.deleteMany({ where: { key: { in: boundedIn(keysToDelete) } } }); } + + return true; }); + + // This write deletes flags, so a transaction that did not run must reach the caller. + if (!applied) { + throw new Error("replaceGlobalFeatureFlags: transaction did not complete"); + } } From 039b6e6582ad8bd5be775e42aa07c99ff28923e7 Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Mon, 24 Aug 2026 11:13:37 +0100 Subject: [PATCH 09/17] refactor(webapp): derive the graced-flag write routing from one table 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. --- .../app/routes/admin.api.v1.feature-flags.ts | 7 +- .../webapp/app/routes/admin.feature-flags.tsx | 16 +-- apps/webapp/app/v3/featureFlags.server.ts | 8 +- apps/webapp/app/v3/featureFlags.ts | 15 +++ .../test/globalFlagWriteRouting.test.ts | 111 ++++++++++++++++++ 5 files changed, 145 insertions(+), 12 deletions(-) create mode 100644 apps/webapp/test/globalFlagWriteRouting.test.ts diff --git a/apps/webapp/app/routes/admin.api.v1.feature-flags.ts b/apps/webapp/app/routes/admin.api.v1.feature-flags.ts index ad5d51592f4..963e971538f 100644 --- a/apps/webapp/app/routes/admin.api.v1.feature-flags.ts +++ b/apps/webapp/app/routes/admin.api.v1.feature-flags.ts @@ -3,7 +3,12 @@ import { json } from "@remix-run/server-runtime"; import { prisma } from "~/db.server"; import { env } from "~/env.server"; import { requireAdminApiRequest } from "~/services/personalAccessToken.server"; -import { applyGlobalGracedFlips, makeSetMultipleFlags } from "~/v3/featureFlags.server"; +import { + applyGlobalGracedFlips, + makeSetMultipleFlags, + touchesGracedGroup, + withoutDerivedKeys, +} from "~/v3/featureFlags.server"; import { validatePartialFeatureFlags } from "~/v3/featureFlags"; export async function action({ request }: ActionFunctionArgs) { diff --git a/apps/webapp/app/routes/admin.feature-flags.tsx b/apps/webapp/app/routes/admin.feature-flags.tsx index 0b52d324dc1..2ba402bf2f4 100644 --- a/apps/webapp/app/routes/admin.feature-flags.tsx +++ b/apps/webapp/app/routes/admin.feature-flags.tsx @@ -14,6 +14,7 @@ import { type FeatureFlagKey, type FlagControlType, getAllFlagControlTypes, + lockedFlagsInPayload, validatePartialFeatureFlags, } from "~/v3/featureFlags"; import { flags as getGlobalFlags, replaceGlobalFeatureFlags } from "~/v3/featureFlags.server"; @@ -96,17 +97,12 @@ export const action = dashboardAction( const { isManagedCloud } = featuresForRequest(request); - // On managed cloud, reject if payload includes locked flags - if (isManagedCloud) { - const lockedInPayload = Object.keys(parsed.data.flags).filter((key) => - GLOBAL_LOCKED_FLAGS.includes(key) + const lockedInPayload = lockedFlagsInPayload(Object.keys(parsed.data.flags), isManagedCloud); + if (lockedInPayload.length > 0) { + return json( + { error: `Cannot modify locked flags: ${lockedInPayload.join(", ")}` }, + { status: 400 } ); - if (lockedInPayload.length > 0) { - return json( - { error: `Cannot modify locked flags: ${lockedInPayload.join(", ")}` }, - { status: 400 } - ); - } } const validationResult = validatePartialFeatureFlags(parsed.data.flags); diff --git a/apps/webapp/app/v3/featureFlags.server.ts b/apps/webapp/app/v3/featureFlags.server.ts index b5d65a13cab..21dfe91bf21 100644 --- a/apps/webapp/app/v3/featureFlags.server.ts +++ b/apps/webapp/app/v3/featureFlags.server.ts @@ -212,8 +212,14 @@ function gracedGroupFor(key: FeatureFlagKey) { return GRACED_GLOBAL_GROUPS.find((g) => g.primary === key || g.derived.includes(key)); } +// True when a save changes any graced group, and therefore needs the stamped path. Derived from +// the group table, so adding a group cannot leave a caller silently writing an unstamped flip. +export function touchesGracedGroup(requestedFlags: Record): boolean { + return GRACED_GLOBAL_GROUPS.some((group) => requestedFlags[group.primary] !== undefined); +} + // Strips every derived key: a grace stamp is computed here, never accepted from a caller. -function withoutDerivedKeys( +export function withoutDerivedKeys( requestedFlags: Partial> ): Record { const out: Record = { ...requestedFlags }; diff --git a/apps/webapp/app/v3/featureFlags.ts b/apps/webapp/app/v3/featureFlags.ts index f241aaf4da2..e6354764126 100644 --- a/apps/webapp/app/v3/featureFlags.ts +++ b/apps/webapp/app/v3/featureFlags.ts @@ -190,6 +190,21 @@ export const ORG_LOCKED_FLAGS: FeatureFlagKey[] = [ FEATURE_FLAG.runOpsMintShardOverride, ]; +/** + * Locked flags present in a payload the global page must refuse. On managed cloud the page never + * offers them, so their presence means the request did not come from that page. Locally an admin + * may unlock and edit them, so nothing is refused. + */ +export function lockedFlagsInPayload( + payloadKeys: string[], + isManagedCloud: boolean +): FeatureFlagKey[] { + if (!isManagedCloud) return []; + return payloadKeys.filter((key): key is FeatureFlagKey => + GLOBAL_LOCKED_FLAGS.includes(key as FeatureFlagKey) + ); +} + // Create a Zod schema from the existing catalog export const FeatureFlagCatalogSchema = z.object(FeatureFlagCatalog); export type FeatureFlagCatalog = z.infer; diff --git a/apps/webapp/test/globalFlagWriteRouting.test.ts b/apps/webapp/test/globalFlagWriteRouting.test.ts new file mode 100644 index 00000000000..0032973d2af --- /dev/null +++ b/apps/webapp/test/globalFlagWriteRouting.test.ts @@ -0,0 +1,111 @@ +// The two global write routes used to carry their own copy of "which keys are graced" and "which +// keys are derived". A new graced group would then need an edit in three places, and missing one +// means an unstamped flip or a stamp written straight from a request body. Both routes now derive +// both answers from the group table, and these tests pin that. Pure, no containers. +import { describe, expect, it } from "vitest"; +import { FEATURE_FLAG, lockedFlagsInPayload } from "~/v3/featureFlags"; +import { touchesGracedGroup, withoutDerivedKeys } from "~/v3/featureFlags.server"; + +describe("touchesGracedGroup — decides whether a save needs the stamped path", () => { + it("is true for a mint-kind change", () => { + expect(touchesGracedGroup({ [FEATURE_FLAG.runOpsMintKind]: "runOpsId" })).toBe(true); + }); + + it("is true for a shard-list change", () => { + expect(touchesGracedGroup({ [FEATURE_FLAG.runOpsMintShardSet]: "a,b" })).toBe(true); + }); + + it("is false for an ordinary flag, which writes directly", () => { + expect(touchesGracedGroup({ [FEATURE_FLAG.mollifierEnabled]: true })).toBe(false); + expect(touchesGracedGroup({})).toBe(false); + }); + + it("is false when only a DERIVED key is present", () => { + // A body carrying only a stamp changes no group. Treating it as a flip would let a caller + // reset a cutover clock without touching the value the clock dates. + expect(touchesGracedGroup({ [FEATURE_FLAG.runOpsMintKindPrev]: "cuid" })).toBe(false); + expect(touchesGracedGroup({ [FEATURE_FLAG.runOpsMintShardSetPrev]: "a" })).toBe(false); + }); + + it("covers every graced primary, so a new group needs no route edit", () => { + // The routes no longer name these keys. If a group is added and this list is not, the next + // assertion fails rather than the group silently skipping the stamp. + const gracedPrimaries = [FEATURE_FLAG.runOpsMintKind, FEATURE_FLAG.runOpsMintShardSet]; + for (const key of gracedPrimaries) { + expect(touchesGracedGroup({ [key]: "x" })).toBe(true); + } + // Every key the strip removes belongs to a group whose primary is one of the above. + const derived = Object.keys( + withoutDerivedKeys({ + [FEATURE_FLAG.runOpsMintKindPrev]: "cuid", + [FEATURE_FLAG.runOpsMintKindFlippedAt]: "t", + [FEATURE_FLAG.runOpsMintShardSetPrev]: "a", + [FEATURE_FLAG.runOpsMintShardSetFlippedAt]: "t", + } as Record) + ); + expect(derived).toEqual([]); + }); +}); + +describe("withoutDerivedKeys — a stamp is never taken from a request body", () => { + it("strips both stamps and keeps everything else", () => { + const out = withoutDerivedKeys({ + [FEATURE_FLAG.runOpsMintKind]: "runOpsId", + [FEATURE_FLAG.runOpsMintKindPrev]: "spoofed", + [FEATURE_FLAG.runOpsMintKindFlippedAt]: "1999-01-01T00:00:00.000Z", + [FEATURE_FLAG.runOpsMintShardSet]: "a,b", + [FEATURE_FLAG.runOpsMintShardSetPrev]: "spoofed", + [FEATURE_FLAG.runOpsMintShardSetFlippedAt]: "1999-01-01T00:00:00.000Z", + [FEATURE_FLAG.mollifierEnabled]: true, + } as Record); + + expect(out).toEqual({ + [FEATURE_FLAG.runOpsMintKind]: "runOpsId", + [FEATURE_FLAG.runOpsMintShardSet]: "a,b", + [FEATURE_FLAG.mollifierEnabled]: true, + }); + }); + + it("does not mutate its input", () => { + const input = { [FEATURE_FLAG.runOpsMintKindPrev]: "cuid" } as Record; + withoutDerivedKeys(input); + expect(input[FEATURE_FLAG.runOpsMintKindPrev]).toBe("cuid"); + }); +}); + +describe("lockedFlagsInPayload — what the global page refuses", () => { + it("refuses a locked flag on managed cloud, where the page never offers one", () => { + const refused = lockedFlagsInPayload( + [FEATURE_FLAG.taskEventRepository, FEATURE_FLAG.mollifierEnabled], + true + ); + expect(refused).toEqual([FEATURE_FLAG.taskEventRepository]); + }); + + it("refuses the mint-shard pins, which are per-org only", () => { + expect(lockedFlagsInPayload([FEATURE_FLAG.runOpsMintShard], true)).toEqual([ + FEATURE_FLAG.runOpsMintShard, + ]); + expect(lockedFlagsInPayload([FEATURE_FLAG.runOpsMintShardEnvPins], true)).toEqual([ + FEATURE_FLAG.runOpsMintShardEnvPins, + ]); + }); + + it("refuses a grace stamp, which the server owns", () => { + expect(lockedFlagsInPayload([FEATURE_FLAG.runOpsMintShardSetFlippedAt], true)).toEqual([ + FEATURE_FLAG.runOpsMintShardSetFlippedAt, + ]); + }); + + it("allows the shard list, because that is the page's ramp lever", () => { + expect(lockedFlagsInPayload([FEATURE_FLAG.runOpsMintShardSet], true)).toEqual([]); + }); + + it("refuses nothing when not managed cloud, where an admin may unlock and edit", () => { + expect(lockedFlagsInPayload([FEATURE_FLAG.taskEventRepository], false)).toEqual([]); + }); + + it("refuses nothing for an empty payload", () => { + expect(lockedFlagsInPayload([], true)).toEqual([]); + }); +}); From 8c40565a6e6880da529c6649d2ded347599ad196 Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Mon, 24 Aug 2026 11:32:22 +0100 Subject: [PATCH 10/17] fix(webapp): actually route the JSON flag API through the graced-group helpers The previous commit claimed both global write routes derive their graced-key knowledge from the group table. Only one did. The JSON API imported the two helpers and called neither, still naming all four derived keys in a destructure and both primaries in its branch condition. The edit that was supposed to replace that body silently matched nothing, and nothing caught it: unused imports are not type errors, knip checks exports rather than imports within a file, and the repo lint task is broken on its own config so it never ran. The claim went into a commit message unverified. The harm was real rather than cosmetic. A third graced group would have fallen through to the direct write, which sets its stamp from the request body with no lock, which is the failure the helpers exist to prevent. Also from the same review: - The advisory-lock comment had its reasoning inverted. The LEGACY id is the operative one, because an older release takes only that id and is what a rolling deploy has to serialize against. The new id adds nothing until every writer takes it. - Two test comments claimed more than their tests did. The routing tests cover the helpers, not the routes, and cannot show that a route calls them; that is held by review. Said so. - The concurrency test admitted every outcome, so it passed with the lock removed. It now asserts the stored pair is a coherent history: prev is what the other writer left, never the winner's own set. - Merged a duplicate import of the same module. --- .../app/routes/admin.api.v1.feature-flags.ts | 22 ++++++++----------- apps/webapp/app/v3/featureFlags.server.ts | 12 +++++----- .../test/globalFlagWriteRouting.test.ts | 13 +++++------ .../test/runOpsMintShardSetFlip.test.ts | 14 +++++++++--- 4 files changed, 32 insertions(+), 29 deletions(-) diff --git a/apps/webapp/app/routes/admin.api.v1.feature-flags.ts b/apps/webapp/app/routes/admin.api.v1.feature-flags.ts index 963e971538f..e9da02effd9 100644 --- a/apps/webapp/app/routes/admin.api.v1.feature-flags.ts +++ b/apps/webapp/app/routes/admin.api.v1.feature-flags.ts @@ -30,20 +30,16 @@ export async function action({ request }: ActionFunctionArgs) { ); } - // Derived grace-stamp fields are computed server-side; never trust them from the body. - const { - runOpsMintKindPrev: _ignoredPrev, - runOpsMintKindFlippedAt: _ignoredFlippedAt, - runOpsMintShardSetPrev: _ignoredSetPrev, - runOpsMintShardSetFlippedAt: _ignoredSetFlippedAt, - ...requestedFlags - } = validationResult.data; + // Both the strip and the branch derive from the graced-group table, so adding a group needs + // no edit here. Naming the keys inline is how a new group ends up writing its stamp straight + // from the request body, with no lock. + const requestedFlags = withoutDerivedKeys(validationResult.data) as Partial< + typeof validationResult.data + >; - // A change to a graced group stamps its window under a lock; any other save writes directly. - const updatedFlags = - requestedFlags.runOpsMintKind !== undefined || requestedFlags.runOpsMintShardSet !== undefined - ? await applyGlobalGracedFlips(prisma, requestedFlags, env.RUN_OPS_MINT_FLIP_GRACE_MS) - : await makeSetMultipleFlags(prisma)(requestedFlags); + const updatedFlags = touchesGracedGroup(requestedFlags) + ? await applyGlobalGracedFlips(prisma, requestedFlags, env.RUN_OPS_MINT_FLIP_GRACE_MS) + : await makeSetMultipleFlags(prisma)(requestedFlags); return json({ success: true, diff --git a/apps/webapp/app/v3/featureFlags.server.ts b/apps/webapp/app/v3/featureFlags.server.ts index 21dfe91bf21..0054f131494 100644 --- a/apps/webapp/app/v3/featureFlags.server.ts +++ b/apps/webapp/app/v3/featureFlags.server.ts @@ -1,6 +1,5 @@ import { type z } from "zod"; import type { PrismaClient } from "@trigger.dev/database"; -import { $transaction, prisma, type PrismaClientOrTransaction } from "~/db.server"; import { FEATURE_FLAG, type FeatureFlagCatalogSchema, @@ -9,7 +8,7 @@ import { } from "~/v3/featureFlags"; import { stampMintKindFlip } from "~/v3/runOpsMigration/mintFlipGrace"; import { stampMintShardSetFlip } from "~/v3/runOpsMigration/mintShardGrace"; -import { boundedIn } from "~/db.server"; +import { $transaction, boundedIn, prisma, type PrismaClientOrTransaction } from "~/db.server"; export type FlagsOptions = { key: T; @@ -234,10 +233,11 @@ export function withoutDerivedKeys( // The rows may not exist yet, so a row FOR UPDATE cannot lock them; an advisory xact lock // serializes concurrent global flips instead, so one cannot clobber another's stamp. // -// Two lock ids are taken, in a fixed order. The second is this release's name; the first is the -// name an older release still takes. A deploy rolls for hours, so both versions write at once, -// and dropping the old id would leave those writers serializing against nothing. Remove the -// legacy id one release after this one ships. +// Two lock ids are taken, in a fixed order. The FIRST is the operative one: an older release +// takes only that id, and a deploy rolls for hours, so it is the id that serializes across both +// versions. The second is this release's name and adds nothing until every writer takes it. +// Renaming without keeping the old id is what would leave the two versions unserialized. Remove +// the legacy id one release after this one ships, when nothing takes it alone. async function lockGracedGroups(tx: PrismaClientOrTransaction): Promise { await tx.$executeRaw`SELECT pg_advisory_xact_lock(hashtext('runops-global-mint-kind-flip'))`; await tx.$executeRaw`SELECT pg_advisory_xact_lock(hashtext('runops-global-graced-flag-flip'))`; diff --git a/apps/webapp/test/globalFlagWriteRouting.test.ts b/apps/webapp/test/globalFlagWriteRouting.test.ts index 0032973d2af..3193919b5ca 100644 --- a/apps/webapp/test/globalFlagWriteRouting.test.ts +++ b/apps/webapp/test/globalFlagWriteRouting.test.ts @@ -1,7 +1,8 @@ -// The two global write routes used to carry their own copy of "which keys are graced" and "which -// keys are derived". A new graced group would then need an edit in three places, and missing one -// means an unstamped flip or a stamp written straight from a request body. Both routes now derive -// both answers from the group table, and these tests pin that. Pure, no containers. +// Both global write routes used to carry their own copy of "which keys are graced" and "which +// keys are derived", so a new group needed an edit in three places and missing one meant an +// unstamped flip or a stamp taken from a request body. These tests cover the two helpers the +// routes now call. They do NOT reach a route: both actions sit behind admin auth, so that the +// routes call these helpers rather than their own copies is held by review, not by a test. import { describe, expect, it } from "vitest"; import { FEATURE_FLAG, lockedFlagsInPayload } from "~/v3/featureFlags"; import { touchesGracedGroup, withoutDerivedKeys } from "~/v3/featureFlags.server"; @@ -27,9 +28,7 @@ describe("touchesGracedGroup — decides whether a save needs the stamped path", expect(touchesGracedGroup({ [FEATURE_FLAG.runOpsMintShardSetPrev]: "a" })).toBe(false); }); - it("covers every graced primary, so a new group needs no route edit", () => { - // The routes no longer name these keys. If a group is added and this list is not, the next - // assertion fails rather than the group silently skipping the stamp. + it("recognises every graced primary the group table declares", () => { const gracedPrimaries = [FEATURE_FLAG.runOpsMintKind, FEATURE_FLAG.runOpsMintShardSet]; for (const key of gracedPrimaries) { expect(touchesGracedGroup({ [key]: "x" })).toBe(true); diff --git a/apps/webapp/test/runOpsMintShardSetFlip.test.ts b/apps/webapp/test/runOpsMintShardSetFlip.test.ts index ec354b92075..8773a878faa 100644 --- a/apps/webapp/test/runOpsMintShardSetFlip.test.ts +++ b/apps/webapp/test/runOpsMintShardSetFlip.test.ts @@ -105,7 +105,7 @@ describe("applyGlobalGracedFlips — the shard-set list is stamped, not bare-wri expect(typeof m[FEATURE_FLAG.runOpsMintShardSetFlippedAt]).toBe("string"); }); - postgresTest("concurrent list changes serialize on the lock", async ({ prisma }) => { + postgresTest("concurrent list changes do not interleave", async ({ prisma }) => { await makeSetMultipleFlags(prisma)({ [FEATURE_FLAG.runOpsMintShardSet]: "a" }); await Promise.all([ @@ -114,8 +114,16 @@ describe("applyGlobalGracedFlips — the shard-set list is stamped, not bare-wri ]); const m = await readFlags(prisma, SET_KEYS); - // Whichever won, the stamp must describe a real predecessor, never be absent. - expect(["a", "a,b", "a,c"]).toContain(m[FEATURE_FLAG.runOpsMintShardSetPrev]); + const set = m[FEATURE_FLAG.runOpsMintShardSet]; + const prev = m[FEATURE_FLAG.runOpsMintShardSetPrev]; + + // The pair must be a consistent history, not a mix of the two writers. The winner's set is + // one of the two, and prev is what the OTHER writer left behind: either the original "a", or + // the loser's set when the loser committed first. "a,b" beside prev "a,b" would mean one + // writer read its own uncommitted state, and prev naming the winner's own set is incoherent. + expect(["a,b", "a,c"]).toContain(set); + expect(["a", "a,b", "a,c"]).toContain(prev); + expect(prev).not.toBe(set); expect(typeof m[FEATURE_FLAG.runOpsMintShardSetFlippedAt]).toBe("string"); }); }); From 4edaee2eec33142be93d3e78dfc0388c3b19031b Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Mon, 24 Aug 2026 11:50:33 +0100 Subject: [PATCH 11/17] fix(webapp): disclose cascaded stamp deletes, and write only changed flags Two defects this branch introduced. The confirm dialog understated a deletion. Unsetting a graced primary clears its two stamps, and this branch moved those stamps into the locked set, so they left the page's editable keys and the change list stopped mentioning them. Three rows were deleted and one was shown. Before this branch the stamps were editable, so all three appeared. The change list moves into buildFlagChangeList, which adds the cascade. Only an unset cascades: a change re-stamps instead. A stamp that is not stored is not listed. The key topology moves to the shared flag module, since the page and the server both need it and a second copy would drift. The save also wrote every submitted flag. Stamping needs read-then-write, so this branch replaced a batch transaction with an interactive one, where each upsert is its own round trip against the interactive timeout. It now reads the submitted keys once and writes only the values that differ, so a typical save costs two round trips rather than one per flag. --- .../app/components/admin/flagChangeList.ts | 53 +++++++ .../webapp/app/routes/admin.feature-flags.tsx | 31 +--- apps/webapp/app/v3/featureFlags.server.ts | 62 +++++--- apps/webapp/app/v3/featureFlags.ts | 25 ++++ apps/webapp/test/globalFlagChangeList.test.ts | 136 ++++++++++++++++++ 5 files changed, 256 insertions(+), 51 deletions(-) create mode 100644 apps/webapp/app/components/admin/flagChangeList.ts create mode 100644 apps/webapp/test/globalFlagChangeList.test.ts diff --git a/apps/webapp/app/components/admin/flagChangeList.ts b/apps/webapp/app/components/admin/flagChangeList.ts new file mode 100644 index 00000000000..4cc1c9c6d31 --- /dev/null +++ b/apps/webapp/app/components/admin/flagChangeList.ts @@ -0,0 +1,53 @@ +import { derivedFlagsClearedWith } from "~/v3/featureFlags"; + +export type FlagChange = + | { key: string; type: "added"; newVal: string } + | { key: string; type: "removed"; oldVal: string } + | { key: string; type: "changed"; oldVal: string; newVal: string }; + +/** + * What a global flag save will do, for the confirm dialog. + * + * A graced primary that is unset also clears its stamps. Those keys are locked, so they never + * appear in `editableKeys`, and listing only the editable keys understated the deletion. + */ +export function buildFlagChangeList(params: { + editableKeys: readonly string[]; + lockedKeys: readonly string[]; + initialValues: Record; + newValues: Record; +}): FlagChange[] { + const { editableKeys, initialValues, newValues } = params; + + return editableKeys.flatMap((key) => { + const wasSet = key in initialValues; + const isSet = key in newValues; + const oldVal = initialValues[key]; + const newVal = newValues[key]; + + if (!wasSet && !isSet) return []; + if (wasSet && isSet && stableValue(oldVal) === stableValue(newVal)) return []; + + if (!wasSet && isSet) { + return [{ key, type: "added", newVal: String(newVal) }]; + } + + if (wasSet && !isSet) { + // Only an unset clears the stamps. A change re-stamps instead. + const cascaded = derivedFlagsClearedWith(key) + .filter((derived) => derived in initialValues) + .map((derived) => ({ + key: derived, + type: "removed", + oldVal: String(initialValues[derived]), + })); + return [{ key, type: "removed", oldVal: String(oldVal) }, ...cascaded]; + } + + return [{ key, type: "changed", oldVal: String(oldVal), newVal: String(newVal) }]; + }); +} + +function stableValue(value: unknown): string { + return JSON.stringify(value ?? null); +} diff --git a/apps/webapp/app/routes/admin.feature-flags.tsx b/apps/webapp/app/routes/admin.feature-flags.tsx index 2ba402bf2f4..043cd86ff4e 100644 --- a/apps/webapp/app/routes/admin.feature-flags.tsx +++ b/apps/webapp/app/routes/admin.feature-flags.tsx @@ -30,6 +30,7 @@ import { DialogFooter, } from "~/components/primitives/Dialog"; import { cn } from "~/utils/cn"; +import { buildFlagChangeList } from "~/components/admin/flagChangeList"; import { UNSET_VALUE, BooleanControl, @@ -471,35 +472,7 @@ function ConfirmDialog({ .filter((key) => !lockedKeys.includes(key)) .sort(); - type Change = - | { key: string; type: "added"; newVal: string } - | { key: string; type: "removed"; oldVal: string } - | { key: string; type: "changed"; oldVal: string; newVal: string }; - - const changes = editableKeys.flatMap((key) => { - const wasSet = key in initialValues; - const isSet = key in newValues; - const oldVal = initialValues[key]; - const newVal = newValues[key]; - - if (!wasSet && !isSet) return []; - if (wasSet && isSet && stableStringify(oldVal) === stableStringify(newVal)) return []; - - if (!wasSet && isSet) { - return [{ key, type: "added" as const, newVal: String(newVal) }]; - } - if (wasSet && !isSet) { - return [{ key, type: "removed" as const, oldVal: String(oldVal) }]; - } - return [ - { - key, - type: "changed" as const, - oldVal: String(oldVal), - newVal: String(newVal), - }, - ]; - }); + const changes = buildFlagChangeList({ editableKeys, lockedKeys, initialValues, newValues }); return ( diff --git a/apps/webapp/app/v3/featureFlags.server.ts b/apps/webapp/app/v3/featureFlags.server.ts index 0054f131494..7760df8fef9 100644 --- a/apps/webapp/app/v3/featureFlags.server.ts +++ b/apps/webapp/app/v3/featureFlags.server.ts @@ -5,6 +5,7 @@ import { type FeatureFlagCatalogSchema, type FeatureFlagKey, FeatureFlagCatalog, + GRACED_FLAG_GROUPS, } from "~/v3/featureFlags"; import { stampMintKindFlip } from "~/v3/runOpsMigration/mintFlipGrace"; import { stampMintShardSetFlip } from "~/v3/runOpsMigration/mintShardGrace"; @@ -180,27 +181,12 @@ export function makeSetMultipleFlags(_prisma: PrismaClientOrTransaction = prisma }; } -// Global flag groups whose value carries its own grace stamp. `primary` is operator-supplied; -// `derived` is computed server-side and must never be written from a request body. The pair is -// named explicitly rather than by position, so a group declared in another order stays correct. -const GRACED_GLOBAL_GROUPS = [ - { - primary: FEATURE_FLAG.runOpsMintKind as FeatureFlagKey, - derived: [ - FEATURE_FLAG.runOpsMintKindPrev, - FEATURE_FLAG.runOpsMintKindFlippedAt, - ] as FeatureFlagKey[], - stamp: stampMintKindFlip, - }, - { - primary: FEATURE_FLAG.runOpsMintShardSet as FeatureFlagKey, - derived: [ - FEATURE_FLAG.runOpsMintShardSetPrev, - FEATURE_FLAG.runOpsMintShardSetFlippedAt, - ] as FeatureFlagKey[], - stamp: stampMintShardSetFlip, - }, -] as const; +// The key topology lives in the shared module, because the admin page needs it too. This adds +// the stamping behaviour, which is server-only. +const GRACED_GLOBAL_GROUPS = GRACED_FLAG_GROUPS.map((group) => ({ + ...group, + stamp: group.primary === FEATURE_FLAG.runOpsMintKind ? stampMintKindFlip : stampMintShardSetFlip, +})); const GRACED_GLOBAL_KEYS: FeatureFlagKey[] = GRACED_GLOBAL_GROUPS.flatMap((g) => [ g.primary, @@ -218,6 +204,21 @@ export function touchesGracedGroup(requestedFlags: Record): boo } // Strips every derived key: a grace stamp is computed here, never accepted from a caller. +// Only the flags whose stored value differs. Each write is a round trip inside an interactive +// transaction, so writing an unchanged flag costs a round trip for nothing. +export function flagsNeedingWrite( + requested: Record, + existing: Record +): Record { + const out: Record = {}; + for (const [key, value] of Object.entries(requested)) { + if (JSON.stringify(existing[key] ?? null) !== JSON.stringify(value ?? null)) { + out[key] = value; + } + } + return out; +} + export function withoutDerivedKeys( requestedFlags: Partial> ): Record { @@ -339,7 +340,24 @@ export async function replaceGlobalFeatureFlags( } } - await makeSetMultipleFlags(tx)(toWrite as Partial>); + // One round trip to learn the stored values, then a write only for what actually differs. + // makeSetMultipleFlags upserts sequentially, so an unchanged flag costs a round trip for + // nothing, and this transaction is interactive and holds a pooled connection. + const writeKeys = Object.keys(toWrite); + if (writeKeys.length > 0) { + const storedRows = await tx.featureFlag.findMany({ + where: { key: { in: boundedIn(writeKeys) } }, + select: { key: true, value: true }, + }); + const stored: Record = {}; + for (const row of storedRows) { + stored[row.key] = row.value; + } + + await makeSetMultipleFlags(tx)( + flagsNeedingWrite(toWrite, stored) as Partial> + ); + } if (keysToDelete.length > 0) { await tx.featureFlag.deleteMany({ where: { key: { in: boundedIn(keysToDelete) } } }); diff --git a/apps/webapp/app/v3/featureFlags.ts b/apps/webapp/app/v3/featureFlags.ts index e6354764126..3a88beb54bc 100644 --- a/apps/webapp/app/v3/featureFlags.ts +++ b/apps/webapp/app/v3/featureFlags.ts @@ -190,6 +190,31 @@ export const ORG_LOCKED_FLAGS: FeatureFlagKey[] = [ FEATURE_FLAG.runOpsMintShardOverride, ]; +/** + * Flag groups where the operator sets a `primary` and the server computes the rest. The topology + * lives here, not in the server module, because the admin page needs it too: unsetting a primary + * clears its stamps, and the page has to disclose that. + */ +export const GRACED_FLAG_GROUPS: ReadonlyArray<{ + primary: FeatureFlagKey; + derived: readonly FeatureFlagKey[]; +}> = [ + { + primary: FEATURE_FLAG.runOpsMintKind, + derived: [FEATURE_FLAG.runOpsMintKindPrev, FEATURE_FLAG.runOpsMintKindFlippedAt], + }, + { + primary: FEATURE_FLAG.runOpsMintShardSet, + derived: [FEATURE_FLAG.runOpsMintShardSetPrev, FEATURE_FLAG.runOpsMintShardSetFlippedAt], + }, +]; + +/** The stamps deleted alongside `primary`. Empty unless `primary` is a graced primary. */ +export function derivedFlagsClearedWith(primary: string): FeatureFlagKey[] { + const group = GRACED_FLAG_GROUPS.find((g) => g.primary === primary); + return group ? [...group.derived] : []; +} + /** * Locked flags present in a payload the global page must refuse. On managed cloud the page never * offers them, so their presence means the request did not come from that page. Locally an admin diff --git a/apps/webapp/test/globalFlagChangeList.test.ts b/apps/webapp/test/globalFlagChangeList.test.ts new file mode 100644 index 00000000000..0db0e64f0ee --- /dev/null +++ b/apps/webapp/test/globalFlagChangeList.test.ts @@ -0,0 +1,136 @@ +// Two properties of a global flag save that the admin page had no way to state. +// +// 1. Unsetting a graced primary clears its server-computed stamps too. Those keys are locked, so +// they are absent from the page's editable set, and the confirm dialog listed one removal +// while three rows were deleted. +// 2. A save should write only the flags whose value actually changed. Writing every submitted +// flag costs one round trip each inside an interactive transaction. +import { describe, expect, it } from "vitest"; +import { FEATURE_FLAG, derivedFlagsClearedWith } from "~/v3/featureFlags"; +import { flagsNeedingWrite } from "~/v3/featureFlags.server"; +import { buildFlagChangeList } from "~/components/admin/flagChangeList"; + +const LOCKED = [ + FEATURE_FLAG.runOpsMintKindPrev, + FEATURE_FLAG.runOpsMintKindFlippedAt, + FEATURE_FLAG.runOpsMintShardSetPrev, + FEATURE_FLAG.runOpsMintShardSetFlippedAt, +] as string[]; + +// Sorted, as the dialog sorts before calling: the builder preserves the order it is given. +const EDITABLE = [ + FEATURE_FLAG.runOpsMintKind, + FEATURE_FLAG.runOpsMintShardSet, + FEATURE_FLAG.mollifierEnabled, +].sort() as string[]; + +describe("derivedFlagsClearedWith", () => { + it("names the stamps that go with a graced primary", () => { + expect(derivedFlagsClearedWith(FEATURE_FLAG.runOpsMintKind)).toEqual([ + FEATURE_FLAG.runOpsMintKindPrev, + FEATURE_FLAG.runOpsMintKindFlippedAt, + ]); + expect(derivedFlagsClearedWith(FEATURE_FLAG.runOpsMintShardSet)).toEqual([ + FEATURE_FLAG.runOpsMintShardSetPrev, + FEATURE_FLAG.runOpsMintShardSetFlippedAt, + ]); + }); + + it("names nothing for an ordinary flag, or for a stamp itself", () => { + expect(derivedFlagsClearedWith(FEATURE_FLAG.mollifierEnabled)).toEqual([]); + expect(derivedFlagsClearedWith(FEATURE_FLAG.runOpsMintKindPrev)).toEqual([]); + }); +}); + +describe("buildFlagChangeList — what the confirm dialog must show", () => { + it("lists an added, a changed and a removed flag", () => { + const changes = buildFlagChangeList({ + editableKeys: EDITABLE, + lockedKeys: LOCKED, + initialValues: { mollifierEnabled: true, runOpsMintShardSet: "a" }, + newValues: { runOpsMintShardSet: "a,b", runOpsMintKind: "runOpsId" }, + }); + + expect(changes).toEqual([ + { key: FEATURE_FLAG.mollifierEnabled, type: "removed", oldVal: "true" }, + { key: FEATURE_FLAG.runOpsMintKind, type: "added", newVal: "runOpsId" }, + { key: FEATURE_FLAG.runOpsMintShardSet, type: "changed", oldVal: "a", newVal: "a,b" }, + ]); + }); + + it("discloses the stamps cleared alongside an unset graced primary", () => { + // Three rows are deleted, so three removals must be shown, not one. + const changes = buildFlagChangeList({ + editableKeys: EDITABLE, + lockedKeys: LOCKED, + initialValues: { + runOpsMintShardSet: "a,b", + runOpsMintShardSetPrev: "a", + runOpsMintShardSetFlippedAt: "2026-08-24T00:00:00.000Z", + }, + newValues: {}, + }); + + expect(changes.map((c) => c.key)).toEqual([ + FEATURE_FLAG.runOpsMintShardSet, + FEATURE_FLAG.runOpsMintShardSetPrev, + FEATURE_FLAG.runOpsMintShardSetFlippedAt, + ]); + expect(changes.every((c) => c.type === "removed")).toBe(true); + }); + + it("does not disclose a stamp that is not stored", () => { + const changes = buildFlagChangeList({ + editableKeys: EDITABLE, + lockedKeys: LOCKED, + initialValues: { runOpsMintShardSet: "a,b" }, + newValues: {}, + }); + expect(changes.map((c) => c.key)).toEqual([FEATURE_FLAG.runOpsMintShardSet]); + }); + + it("does not disclose stamps when the primary is only CHANGED", () => { + // A change re-stamps rather than clearing, so nothing is removed. + const changes = buildFlagChangeList({ + editableKeys: EDITABLE, + lockedKeys: LOCKED, + initialValues: { runOpsMintShardSet: "a", runOpsMintShardSetPrev: "" }, + newValues: { runOpsMintShardSet: "a,b" }, + }); + expect(changes.map((c) => c.key)).toEqual([FEATURE_FLAG.runOpsMintShardSet]); + }); + + it("never lists a locked key on its own", () => { + const changes = buildFlagChangeList({ + editableKeys: EDITABLE, + lockedKeys: LOCKED, + initialValues: { runOpsMintShardSetPrev: "a" }, + newValues: {}, + }); + expect(changes).toEqual([]); + }); +}); + +describe("flagsNeedingWrite — one round trip per CHANGED flag, not per submitted flag", () => { + it("drops a submitted flag whose stored value already matches", () => { + const out = flagsNeedingWrite( + { mollifierEnabled: true, hasAiAccess: true }, + { mollifierEnabled: true, hasAiAccess: false } + ); + expect(out).toEqual({ hasAiAccess: true }); + }); + + it("keeps a flag that is absent from storage", () => { + expect(flagsNeedingWrite({ mollifierEnabled: true }, {})).toEqual({ mollifierEnabled: true }); + }); + + it("returns nothing when a save changes nothing", () => { + expect(flagsNeedingWrite({ mollifierEnabled: true }, { mollifierEnabled: true })).toEqual({}); + }); + + it("compares by value, not by reference, so a CSV rewritten the same way is not a write", () => { + expect(flagsNeedingWrite({ runOpsMintShardSet: "a,b" }, { runOpsMintShardSet: "a,b" })).toEqual( + {} + ); + }); +}); From 19ae40224e5505172fc322c3085f04e33a1e632b Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Mon, 24 Aug 2026 12:15:33 +0100 Subject: [PATCH 12/17] test(webapp): assert a protected graced group is kept whole The protected-list case asserted only that the primary survives, so the group-level property was unpinned: protection is keyed off the primary, and the stamps must be kept with it. Third case on this branch of a test claiming more than it asserted. --- apps/webapp/test/runOpsMintShardSetFlip.test.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/apps/webapp/test/runOpsMintShardSetFlip.test.ts b/apps/webapp/test/runOpsMintShardSetFlip.test.ts index 8773a878faa..036f9e87709 100644 --- a/apps/webapp/test/runOpsMintShardSetFlip.test.ts +++ b/apps/webapp/test/runOpsMintShardSetFlip.test.ts @@ -254,7 +254,11 @@ describe("replaceGlobalFeatureFlags — the admin page cannot bypass the stamp", }); const m = await readFlags(prisma, SET_KEYS); + // Kept WHOLE. Protection is keyed off the primary, so the stamps must survive with it; + // asserting only the primary leaves the group-level property unpinned. expect(m[FEATURE_FLAG.runOpsMintShardSet]).toBe("a"); + expect(m[FEATURE_FLAG.runOpsMintShardSetPrev]).toBeDefined(); + expect(m[FEATURE_FLAG.runOpsMintShardSetFlippedAt]).toBeDefined(); }); postgresTest("an ordinary flag keeps replace semantics", async ({ prisma }) => { From 69464b9a64b29f0365628902dfa2bfb8ae045d9d Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Mon, 24 Aug 2026 12:23:53 +0100 Subject: [PATCH 13/17] fix(webapp): bound the fixed-length flag key IN filters CI's oxlint flags an unbounded Prisma `in:` filter: the list length becomes the bind-parameter count, so each distinct length is a separate prepared statement. Both lists here are compile-time constants, but boundedIn is what the neighbouring call sites use and it needs no suppression comment. It pads to the next power of two by repeating the last key, which an IN over a unique column does not notice. --- apps/webapp/app/v3/featureFlags.server.ts | 2 +- apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/apps/webapp/app/v3/featureFlags.server.ts b/apps/webapp/app/v3/featureFlags.server.ts index 7760df8fef9..d45cdfd3919 100644 --- a/apps/webapp/app/v3/featureFlags.server.ts +++ b/apps/webapp/app/v3/featureFlags.server.ts @@ -252,7 +252,7 @@ async function stampGracedGroups( graceMs: number ): Promise> { const existingRows = await tx.featureFlag.findMany({ - where: { key: { in: GRACED_GLOBAL_KEYS } }, + where: { key: { in: boundedIn(GRACED_GLOBAL_KEYS) } }, select: { key: true, value: true }, }); const existingGlobal: Record = {}; diff --git a/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts b/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts index 02a432fb537..9a93eeb224d 100644 --- a/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts +++ b/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts @@ -1,6 +1,6 @@ import { createHash } from "node:crypto"; import type { ShardKey } from "@trigger.dev/core/v3/isomorphic"; -import { $replica } from "~/db.server"; +import { $replica, boundedIn } from "~/db.server"; import { env } from "~/env.server"; import { logger } from "~/services/logger.server"; import { BoundedTtlCache } from "~/services/realtime/boundedTtlCache"; @@ -208,7 +208,7 @@ const liveCache = singleton("runOpsMintShardCache", (): { current: MintShardCach async function readSetFlags(): Promise> { const rows = await $replica.featureFlag.findMany({ - where: { key: { in: GLOBAL_SHARD_KEYS } }, + where: { key: { in: boundedIn(GLOBAL_SHARD_KEYS) } }, select: { key: true, value: true }, }); const flags: Record = {}; From eb4d82a37f65d5b8c64ce100af90e01dc426d472 Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Mon, 24 Aug 2026 12:56:02 +0100 Subject: [PATCH 14/17] fix(webapp): read cascaded stamp values from the unfiltered flag set The cascade disclosure never fired. buildFlagChangeList looked for the stamps in initialValues, but the page builds initialValues by filtering locked keys out, and the stamps are locked. So the confirm dialog still showed one removal while three rows were deleted. The unit test passed because it was handed an initialValues containing the stamps, which the caller cannot produce. The cascade now reads storedValues, the unfiltered set the loader returned, and the tests use the caller's real shape. Verified against the running app: the dialog lists all three rows with their values. --- apps/webapp/app/components/admin/flagChangeList.ts | 13 ++++++++----- apps/webapp/app/routes/admin.feature-flags.tsx | 11 ++++++++++- apps/webapp/test/globalFlagChangeList.test.ts | 14 ++++++++++---- 3 files changed, 28 insertions(+), 10 deletions(-) diff --git a/apps/webapp/app/components/admin/flagChangeList.ts b/apps/webapp/app/components/admin/flagChangeList.ts index 4cc1c9c6d31..512d3758bc7 100644 --- a/apps/webapp/app/components/admin/flagChangeList.ts +++ b/apps/webapp/app/components/admin/flagChangeList.ts @@ -8,16 +8,19 @@ export type FlagChange = /** * What a global flag save will do, for the confirm dialog. * - * A graced primary that is unset also clears its stamps. Those keys are locked, so they never - * appear in `editableKeys`, and listing only the editable keys understated the deletion. + * A graced primary that is unset also clears its stamps. Those keys are locked, so the caller + * filters them out of `initialValues` — the cascade therefore reads `storedValues`, which is the + * unfiltered set the loader returned. Reading `initialValues` finds nothing and understates the + * deletion, which is the defect this parameter exists to prevent. */ export function buildFlagChangeList(params: { editableKeys: readonly string[]; lockedKeys: readonly string[]; initialValues: Record; + storedValues: Record; newValues: Record; }): FlagChange[] { - const { editableKeys, initialValues, newValues } = params; + const { editableKeys, initialValues, storedValues, newValues } = params; return editableKeys.flatMap((key) => { const wasSet = key in initialValues; @@ -35,11 +38,11 @@ export function buildFlagChangeList(params: { if (wasSet && !isSet) { // Only an unset clears the stamps. A change re-stamps instead. const cascaded = derivedFlagsClearedWith(key) - .filter((derived) => derived in initialValues) + .filter((derived) => derived in storedValues) .map((derived) => ({ key: derived, type: "removed", - oldVal: String(initialValues[derived]), + oldVal: String(storedValues[derived]), })); return [{ key, type: "removed", oldVal: String(oldVal) }, ...cascaded]; } diff --git a/apps/webapp/app/routes/admin.feature-flags.tsx b/apps/webapp/app/routes/admin.feature-flags.tsx index f20a03dbf3c..e987812f520 100644 --- a/apps/webapp/app/routes/admin.feature-flags.tsx +++ b/apps/webapp/app/routes/admin.feature-flags.tsx @@ -399,6 +399,7 @@ export default function AdminFeatureFlagsRoute() { open={confirmOpen} onOpenChange={setConfirmOpen} initialValues={initialValues} + storedValues={allFlags} newValues={values} controlTypes={typedControlTypes} lockedKeys={unlocked ? [] : GLOBAL_LOCKED_FLAGS} @@ -465,6 +466,7 @@ function ConfirmDialog({ open, onOpenChange, initialValues, + storedValues, newValues, controlTypes, lockedKeys, @@ -475,6 +477,7 @@ function ConfirmDialog({ open: boolean; onOpenChange: (open: boolean) => void; initialValues: Record; + storedValues: Record; newValues: Record; controlTypes: Record; lockedKeys: readonly string[]; @@ -486,7 +489,13 @@ function ConfirmDialog({ .filter((key) => !lockedKeys.includes(key)) .sort(); - const changes = buildFlagChangeList({ editableKeys, lockedKeys, initialValues, newValues }); + const changes = buildFlagChangeList({ + editableKeys, + lockedKeys, + initialValues, + storedValues, + newValues, + }); return ( diff --git a/apps/webapp/test/globalFlagChangeList.test.ts b/apps/webapp/test/globalFlagChangeList.test.ts index 0db0e64f0ee..269774e28d3 100644 --- a/apps/webapp/test/globalFlagChangeList.test.ts +++ b/apps/webapp/test/globalFlagChangeList.test.ts @@ -48,6 +48,7 @@ describe("buildFlagChangeList — what the confirm dialog must show", () => { editableKeys: EDITABLE, lockedKeys: LOCKED, initialValues: { mollifierEnabled: true, runOpsMintShardSet: "a" }, + storedValues: { mollifierEnabled: true, runOpsMintShardSet: "a" }, newValues: { runOpsMintShardSet: "a,b", runOpsMintKind: "runOpsId" }, }); @@ -59,11 +60,13 @@ describe("buildFlagChangeList — what the confirm dialog must show", () => { }); it("discloses the stamps cleared alongside an unset graced primary", () => { - // Three rows are deleted, so three removals must be shown, not one. + // Three rows are deleted, so three removals must be shown, not one. The caller filters + // locked keys OUT of initialValues, so the stamps are only visible in storedValues. const changes = buildFlagChangeList({ editableKeys: EDITABLE, lockedKeys: LOCKED, - initialValues: { + initialValues: { runOpsMintShardSet: "a,b" }, + storedValues: { runOpsMintShardSet: "a,b", runOpsMintShardSetPrev: "a", runOpsMintShardSetFlippedAt: "2026-08-24T00:00:00.000Z", @@ -84,6 +87,7 @@ describe("buildFlagChangeList — what the confirm dialog must show", () => { editableKeys: EDITABLE, lockedKeys: LOCKED, initialValues: { runOpsMintShardSet: "a,b" }, + storedValues: { runOpsMintShardSet: "a,b" }, newValues: {}, }); expect(changes.map((c) => c.key)).toEqual([FEATURE_FLAG.runOpsMintShardSet]); @@ -94,7 +98,8 @@ describe("buildFlagChangeList — what the confirm dialog must show", () => { const changes = buildFlagChangeList({ editableKeys: EDITABLE, lockedKeys: LOCKED, - initialValues: { runOpsMintShardSet: "a", runOpsMintShardSetPrev: "" }, + initialValues: { runOpsMintShardSet: "a" }, + storedValues: { runOpsMintShardSet: "a", runOpsMintShardSetPrev: "" }, newValues: { runOpsMintShardSet: "a,b" }, }); expect(changes.map((c) => c.key)).toEqual([FEATURE_FLAG.runOpsMintShardSet]); @@ -104,7 +109,8 @@ describe("buildFlagChangeList — what the confirm dialog must show", () => { const changes = buildFlagChangeList({ editableKeys: EDITABLE, lockedKeys: LOCKED, - initialValues: { runOpsMintShardSetPrev: "a" }, + initialValues: {}, + storedValues: { runOpsMintShardSetPrev: "a" }, newValues: {}, }); expect(changes).toEqual([]); From d9a62acc0ec328273d1c5b947011b3a8861a3127 Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Mon, 24 Aug 2026 13:22:15 +0100 Subject: [PATCH 15/17] refactor(webapp): split the pure shard-placement core out of the env wrapper The placement test reached env.server through its import of the .server module, which the webapp guidance forbids: env.server parses the whole environment schema at import, so the test either fails without a complete environment or passes on ambient values. The file already claimed the core was pure. It now is: mintShardAssignment.ts holds the placement decision and the injected-deps resolver and imports no env, no clock and no database. runOpsMintShard.server.ts keeps only the env-bound wrapper, its caches and its reporters. The test moves next to the pure module and no longer pulls env.server into its chain. knip now sees the wrapper as an unused file rather than an unused export, since nothing imports it until a shard key reaches an id. Ignored by path, with a note to drop the entry with that change. --- ...er.test.ts => mintShardAssignment.test.ts} | 2 +- .../v3/runOpsMigration/mintShardAssignment.ts | 189 +++++++++++++++++ .../runOpsMigration/runOpsMintShard.server.ts | 195 +----------------- knip.json | 3 +- 4 files changed, 198 insertions(+), 191 deletions(-) rename apps/webapp/app/v3/runOpsMigration/{runOpsMintShard.server.test.ts => mintShardAssignment.test.ts} (99%) create mode 100644 apps/webapp/app/v3/runOpsMigration/mintShardAssignment.ts diff --git a/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.ts b/apps/webapp/app/v3/runOpsMigration/mintShardAssignment.test.ts similarity index 99% rename from apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.ts rename to apps/webapp/app/v3/runOpsMigration/mintShardAssignment.test.ts index 417ce8e2673..7d5031ee354 100644 --- a/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.test.ts +++ b/apps/webapp/app/v3/runOpsMigration/mintShardAssignment.test.ts @@ -5,7 +5,7 @@ import { type MintShardCache, type MintShardDeps, type ResolveMintShardDeps, -} from "./runOpsMintShard.server"; +} from "./mintShardAssignment"; import { type MintShardSetResolution } from "./mintShardGrace"; const GRACE_MS = 90_000; diff --git a/apps/webapp/app/v3/runOpsMigration/mintShardAssignment.ts b/apps/webapp/app/v3/runOpsMigration/mintShardAssignment.ts new file mode 100644 index 00000000000..910cd4769cd --- /dev/null +++ b/apps/webapp/app/v3/runOpsMigration/mintShardAssignment.ts @@ -0,0 +1,189 @@ +// PURE module: no env, no clock, no database. Kept separate from the .server wrapper so a test +// can drive it without evaluating env.server, whose schema parse demands a full environment. +import { createHash } from "node:crypto"; +import type { ShardKey } from "@trigger.dev/core/v3/isomorphic"; +import { FEATURE_FLAG } from "~/v3/featureFlags"; +import { + effectiveMintShardSet, + GEN_1_PIN_VALUE, + isValidPinValue, + readMintShardSetResolution, + type MintShardSetResolution, +} from "./mintShardGrace"; + +export type MintShardDeps = { + // The live list, from the control-plane database. + resolution: MintShardSetResolution; + // Fleet-wide pin that beats every per-org and per-env pin. The complete-cutover lever. + globalOverride?: unknown; + nowMs: number; + graceMs: number; + orgFeatureFlags: unknown; + onPinRejected?: (info: { environmentId: string; pin: string; activeSet: string[] }) => void; + onOverrideRejected?: (info: { override: string; activeSet: string[] }) => void; +}; + +function asRecord(value: unknown): Record | undefined { + if (!value || typeof value !== "object" || Array.isArray(value)) return undefined; + return value as Record; +} + +// Map keys are environment INTERNAL ids (cuids), not friendly ids. An unparseable blob, or a +// blob whose value for this environment is invalid, yields no per-env pin and lets the +// per-org scalar decide — never a silent un-pin straight to the hash. +function readEnvPin(raw: unknown, environmentId: string): ShardKey | undefined { + if (typeof raw !== "string") return undefined; + + let parsed: unknown; + try { + parsed = JSON.parse(raw); + } catch { + return undefined; + } + + const pins = asRecord(parsed); + const pin = pins?.[environmentId]; + return isValidPinValue(pin) ? pin : undefined; +} + +// Both pins live in the org override blob the trigger path already holds, so resolving a mint +// shard costs no query. +function readPin(orgFeatureFlags: unknown, environmentId: string): ShardKey | undefined { + const blob = asRecord(orgFeatureFlags); + if (!blob) return undefined; + + const envPin = readEnvPin(blob[FEATURE_FLAG.runOpsMintShardEnvPins], environmentId); + if (envPin !== undefined) return envPin; + + const scalar = blob[FEATURE_FLAG.runOpsMintShard]; + return isValidPinValue(scalar) ? scalar : undefined; +} + +// 64 bits: a 32-bit score collides at this system's environment count, and an undetected tie +// would resolve by iteration order. The NUL separates the fields so no two input pairs can +// concatenate alike. This hash input is FROZEN once gen-2 minting is live: changing it +// re-places every environment, silently. +function shardScore(environmentId: string, key: string): bigint { + return createHash("sha256").update(`${environmentId}\0${key}`).digest().readBigUInt64BE(0); +} + +function hrwSelect(environmentId: string, activeSet: string[]): string { + let bestKey = activeSet[0]; + let bestScore = shardScore(environmentId, bestKey); + + for (let i = 1; i < activeSet.length; i++) { + const key = activeSet[i]; + const score = shardScore(environmentId, key); + if (score > bestScore || (score === bestScore && key > bestKey)) { + bestKey = key; + bestScore = score; + } + } + + return bestKey; +} + +// PURE CORE — no env, no clock, no I/O; tests drive this directly. Deterministic for fixed +// deps, which is what lets run minting and token minting agree on one answer. +// +// An empty list is the off state, and it is the state of every deployment that has not set the +// flag. Bounding the list against the shard keys this deployment can actually route belongs with +// the shard descriptors, which own that information; nothing here mints, so nothing can misroute. +// +// A pin outside the active set falls through to the hash rather than throwing: honouring it +// would leak the drain the active list performs, and throwing would fail customer triggers +// whenever a pinned shard drains. +export function computeMintShard(environment: { id: string }, deps: MintShardDeps): ShardKey { + const activeSet = effectiveMintShardSet(deps.resolution, deps.nowMs, deps.graceMs); + if (activeSet.length === 0) { + return "new"; + } + + // 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; + } + // Fleet-wide, so it is reported once for the value, not once per environment. + deps.onOverrideRejected?.({ override, activeSet }); + } + + const pin = readPin(deps.orgFeatureFlags, environment.id); + if (pin !== undefined) { + if (pin === GEN_1_PIN_VALUE) { + return "new"; + } + if (activeSet.includes(pin)) { + return pin; + } + deps.onPinRejected?.({ environmentId: environment.id, pin, activeSet }); + } + + return hrwSelect(environment.id, activeSet); +} + +// Read together so the override costs no extra query beyond the list it is bounded by. + +type GlobalShardConfig = { resolution: MintShardSetResolution; override: unknown }; + +export type MintShardCache = { value: GlobalShardConfig; expiresAt: number } | undefined; + +export type ResolveMintShardDeps = { + // Reads the list rows. Injected so the cache and the fail-safe are testable without a + // database, the same way computeRunIdMintKind takes its flag reader. + readFlags: () => Promise>; + cache: { current: MintShardCache }; + nowMs: number; + ttlMs: number; + graceMs: number; + orgFeatureFlags: unknown; + onPinRejected?: (info: { environmentId: string; pin: string; activeSet: string[] }) => void; + onOverrideRejected?: (info: { override: string; activeSet: string[] }) => void; + onReadFailed?: (error: unknown) => void; +}; + +// The live list is org-independent, so one process-wide entry serves every mint: one query per +// process per TTL, over one round-trip. Two processes can therefore disagree for the TTL PLUS the +// replica lag behind the read, which can exceed graceMs. That is tolerable here and only here, +// because a gen-2 id carries its own shard key, so disagreement cannot misroute an existing run; +// it only decides where the next root lands, and every failure direction is toward gen-1. +// +// A failed read falls back to gen-1 rather than guessing a list. Guessing would move every +// environment's placement for the length of one blip. +export async function resolveMintShardWith( + environment: { id: string; orgFeatureFlags?: unknown }, + deps: ResolveMintShardDeps +): Promise { + let config: GlobalShardConfig; + const cached = deps.cache.current; + if (cached && cached.expiresAt > deps.nowMs) { + config = cached.value; + } else { + try { + const flags = await deps.readFlags(); + config = { + resolution: readMintShardSetResolution(flags), + override: flags[FEATURE_FLAG.runOpsMintShardOverride], + }; + } catch (error) { + deps.onReadFailed?.(error); + return "new"; + } + deps.cache.current = { value: config, expiresAt: deps.nowMs + deps.ttlMs }; + } + + return computeMintShard(environment, { + resolution: config.resolution, + globalOverride: config.override, + nowMs: deps.nowMs, + graceMs: deps.graceMs, + orgFeatureFlags: deps.orgFeatureFlags, + onPinRejected: deps.onPinRejected, + onOverrideRejected: deps.onOverrideRejected, + }); +} diff --git a/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts b/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts index 9a93eeb224d..542384e16f8 100644 --- a/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts +++ b/apps/webapp/app/v3/runOpsMigration/runOpsMintShard.server.ts @@ -1,136 +1,17 @@ -import { createHash } from "node:crypto"; -import type { ShardKey } from "@trigger.dev/core/v3/isomorphic"; import { $replica, boundedIn } from "~/db.server"; import { env } from "~/env.server"; import { logger } from "~/services/logger.server"; import { BoundedTtlCache } from "~/services/realtime/boundedTtlCache"; import { singleton } from "~/utils/singleton"; import { FEATURE_FLAG } from "~/v3/featureFlags"; -import { - effectiveMintShardSet, - GEN_1_PIN_VALUE, - isValidPinValue, - readMintShardSetResolution, - type MintShardSetResolution, -} from "./mintShardGrace"; - -export type MintShardDeps = { - // The live list, from the control-plane database. - resolution: MintShardSetResolution; - // Fleet-wide pin that beats every per-org and per-env pin. The complete-cutover lever. - globalOverride?: unknown; - nowMs: number; - graceMs: number; - orgFeatureFlags: unknown; - onPinRejected?: (info: { environmentId: string; pin: string; activeSet: string[] }) => void; - onOverrideRejected?: (info: { override: string; activeSet: string[] }) => void; -}; - -function asRecord(value: unknown): Record | undefined { - if (!value || typeof value !== "object" || Array.isArray(value)) return undefined; - return value as Record; -} - -// Map keys are environment INTERNAL ids (cuids), not friendly ids. An unparseable blob, or a -// blob whose value for this environment is invalid, yields no per-env pin and lets the -// per-org scalar decide — never a silent un-pin straight to the hash. -function readEnvPin(raw: unknown, environmentId: string): ShardKey | undefined { - if (typeof raw !== "string") return undefined; - - let parsed: unknown; - try { - parsed = JSON.parse(raw); - } catch { - return undefined; - } - - const pins = asRecord(parsed); - const pin = pins?.[environmentId]; - return isValidPinValue(pin) ? pin : undefined; -} - -// Both pins live in the org override blob the trigger path already holds, so resolving a mint -// shard costs no query. -function readPin(orgFeatureFlags: unknown, environmentId: string): ShardKey | undefined { - const blob = asRecord(orgFeatureFlags); - if (!blob) return undefined; - - const envPin = readEnvPin(blob[FEATURE_FLAG.runOpsMintShardEnvPins], environmentId); - if (envPin !== undefined) return envPin; - - const scalar = blob[FEATURE_FLAG.runOpsMintShard]; - return isValidPinValue(scalar) ? scalar : undefined; -} - -// 64 bits: a 32-bit score collides at this system's environment count, and an undetected tie -// would resolve by iteration order. The NUL separates the fields so no two input pairs can -// concatenate alike. This hash input is FROZEN once gen-2 minting is live: changing it -// re-places every environment, silently. -function shardScore(environmentId: string, key: string): bigint { - return createHash("sha256").update(`${environmentId}\0${key}`).digest().readBigUInt64BE(0); -} - -function hrwSelect(environmentId: string, activeSet: string[]): string { - let bestKey = activeSet[0]; - let bestScore = shardScore(environmentId, bestKey); - - for (let i = 1; i < activeSet.length; i++) { - const key = activeSet[i]; - const score = shardScore(environmentId, key); - if (score > bestScore || (score === bestScore && key > bestKey)) { - bestKey = key; - bestScore = score; - } - } - - return bestKey; -} - -// PURE CORE — no env, no clock, no I/O; tests drive this directly. Deterministic for fixed -// deps, which is what lets run minting and token minting agree on one answer. -// -// An empty list is the off state, and it is the state of every deployment that has not set the -// flag. Bounding the list against the shard keys this deployment can actually route belongs with -// the shard descriptors, which own that information; nothing here mints, so nothing can misroute. -// -// A pin outside the active set falls through to the hash rather than throwing: honouring it -// would leak the drain the active list performs, and throwing would fail customer triggers -// whenever a pinned shard drains. -export function computeMintShard(environment: { id: string }, deps: MintShardDeps): ShardKey { - const activeSet = effectiveMintShardSet(deps.resolution, deps.nowMs, deps.graceMs); - if (activeSet.length === 0) { - return "new"; - } - - // 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; - } - // Fleet-wide, so it is reported once for the value, not once per environment. - deps.onOverrideRejected?.({ override, activeSet }); - } - - const pin = readPin(deps.orgFeatureFlags, environment.id); - if (pin !== undefined) { - if (pin === GEN_1_PIN_VALUE) { - return "new"; - } - if (activeSet.includes(pin)) { - return pin; - } - deps.onPinRejected?.({ environmentId: environment.id, pin, activeSet }); - } +import type { ShardKey } from "@trigger.dev/core/v3/isomorphic"; +import { resolveMintShardWith, type MintShardCache } from "./mintShardAssignment"; - return hrwSelect(environment.id, activeSet); -} +// A misconfiguration is reported again after this long, so a still-broken pin stays visible +// without logging on every trigger. +const REPORT_TTL_MS = 3_600_000; +const REPORT_MAX_ENTRIES = 10_000; -// Read together so the override costs no extra query beyond the list it is bounded by. const GLOBAL_SHARD_KEYS = [ FEATURE_FLAG.runOpsMintShardSet, FEATURE_FLAG.runOpsMintShardSetPrev, @@ -138,70 +19,6 @@ const GLOBAL_SHARD_KEYS = [ FEATURE_FLAG.runOpsMintShardOverride, ]; -type GlobalShardConfig = { resolution: MintShardSetResolution; override: unknown }; - -export type MintShardCache = { value: GlobalShardConfig; expiresAt: number } | undefined; - -// A misconfiguration is reported again after this long, so a still-broken pin stays visible -// without logging on every trigger. -const REPORT_TTL_MS = 3_600_000; -const REPORT_MAX_ENTRIES = 10_000; - -export type ResolveMintShardDeps = { - // Reads the list rows. Injected so the cache and the fail-safe are testable without a - // database, the same way computeRunIdMintKind takes its flag reader. - readFlags: () => Promise>; - cache: { current: MintShardCache }; - nowMs: number; - ttlMs: number; - graceMs: number; - orgFeatureFlags: unknown; - onPinRejected?: (info: { environmentId: string; pin: string; activeSet: string[] }) => void; - onOverrideRejected?: (info: { override: string; activeSet: string[] }) => void; - onReadFailed?: (error: unknown) => void; -}; - -// The live list is org-independent, so one process-wide entry serves every mint: one query per -// process per TTL, over one round-trip. Two processes can therefore disagree for the TTL PLUS the -// replica lag behind the read, which can exceed graceMs. That is tolerable here and only here, -// because a gen-2 id carries its own shard key, so disagreement cannot misroute an existing run; -// it only decides where the next root lands, and every failure direction is toward gen-1. -// -// A failed read falls back to gen-1 rather than guessing a list. Guessing would move every -// environment's placement for the length of one blip. -export async function resolveMintShardWith( - environment: { id: string; orgFeatureFlags?: unknown }, - deps: ResolveMintShardDeps -): Promise { - let config: GlobalShardConfig; - const cached = deps.cache.current; - if (cached && cached.expiresAt > deps.nowMs) { - config = cached.value; - } else { - try { - const flags = await deps.readFlags(); - config = { - resolution: readMintShardSetResolution(flags), - override: flags[FEATURE_FLAG.runOpsMintShardOverride], - }; - } catch (error) { - deps.onReadFailed?.(error); - return "new"; - } - deps.cache.current = { value: config, expiresAt: deps.nowMs + deps.ttlMs }; - } - - return computeMintShard(environment, { - resolution: config.resolution, - globalOverride: config.override, - nowMs: deps.nowMs, - graceMs: deps.graceMs, - orgFeatureFlags: deps.orgFeatureFlags, - onPinRejected: deps.onPinRejected, - onOverrideRejected: deps.onOverrideRejected, - }); -} - const liveCache = singleton("runOpsMintShardCache", (): { current: MintShardCache } => ({ current: undefined, })); diff --git a/knip.json b/knip.json index 84456756ca1..c6e8aee8977 100644 --- a/knip.json +++ b/knip.json @@ -25,7 +25,8 @@ "vite/node-globals-shim.js", "app/v3/otlpTransformWorker.ts" ], - "ignoreDependencies": ["@sentry/cli", "assert", "util"] + "ignoreDependencies": ["@sentry/cli", "assert", "util"], + "ignore": ["app/v3/runOpsMigration/runOpsMintShard.server.ts"] }, "internal-packages/dashboard-agent": { "entry": ["trigger.config.ts", "src/investigation-sweep.ts", "src/maintenance.ts"], From b5865444ee9ffc317ac5a8e4a97b19b6cfdffe1c Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Mon, 24 Aug 2026 13:46:28 +0100 Subject: [PATCH 16/17] fix(webapp): give the admin action test a $transaction stand-in The route-action test mocks ~/db.server to inject its own Prisma client, and that mock exported only prisma and boundedIn. replaceGlobalFeatureFlags now takes the traced $transaction helper from the same module, so all five of the existing cases failed with 'No "$transaction" export is defined'. The stand-in keeps the real semantics under test, an interactive transaction over the injected client, and drops only the tracing and the swallowed-error handling, neither of which this test asserts. Found by running a test file my earlier local runs had skipped. That file arrived with #4751, and picking test files by hand is what hid it. --- .../test/adminFeatureFlagsRouteAction.test.ts | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/apps/webapp/test/adminFeatureFlagsRouteAction.test.ts b/apps/webapp/test/adminFeatureFlagsRouteAction.test.ts index a510fbe0f35..1a3575192ae 100644 --- a/apps/webapp/test/adminFeatureFlagsRouteAction.test.ts +++ b/apps/webapp/test/adminFeatureFlagsRouteAction.test.ts @@ -22,6 +22,19 @@ vi.mock("~/db.server", () => ({ return db.client; }, boundedIn, + // The real helper adds tracing around prisma.$transaction and resolves undefined when it + // swallows an infrastructure error. Neither is under test here, but the transactional + // semantics are, so this stands in with the same shape and a real interactive transaction. + $transaction: async ( + client: PrismaClient, + nameOrFn: unknown, + fnOrOptions?: unknown + ): Promise => { + const fn = (typeof nameOrFn === "function" ? nameOrFn : fnOrOptions) as ( + tx: PrismaClient + ) => Promise; + return client.$transaction((tx) => fn(tx as unknown as PrismaClient)); + }, })); import { action } from "~/routes/admin.feature-flags"; From 128d9aa7a320b19417248d24b9012c92aca8c511 Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Mon, 24 Aug 2026 14:24:07 +0100 Subject: [PATCH 17/17] fix(webapp): coalesce concurrent shard-list refreshes, and use the real txn helper Two review findings. The cache had a lost-update race. Two misses each issued a read, so a slower read landing after a faster one wrote its older snapshot back into the cache for a whole TTL. Refreshes are now single-flight: concurrent misses await one read, and the in-flight handle is cleared on settle so a failure still lets the next call retry. Three tests cover it with a deferred promise through the injected reader, no mocking. The admin action test's transaction stand-in was a reimplementation. It now delegates to the same shared helper the production wrapper wraps, so the transactional semantics, the nesting case and the retry behaviour are the real ones. Only the wrapper's tracing span and infrastructure-error logging are absent, and neither is asserted there. --- .../mintShardAssignment.test.ts | 60 +++++++++++++++++++ .../v3/runOpsMigration/mintShardAssignment.ts | 31 +++++++--- .../test/adminFeatureFlagsRouteAction.test.ts | 27 +++++---- 3 files changed, 100 insertions(+), 18 deletions(-) diff --git a/apps/webapp/app/v3/runOpsMigration/mintShardAssignment.test.ts b/apps/webapp/app/v3/runOpsMigration/mintShardAssignment.test.ts index 7d5031ee354..d88e64e1d75 100644 --- a/apps/webapp/app/v3/runOpsMigration/mintShardAssignment.test.ts +++ b/apps/webapp/app/v3/runOpsMigration/mintShardAssignment.test.ts @@ -319,6 +319,66 @@ describe("resolveMintShardWith — cache, read failure and fail-safe", () => { expect(await resolveMintShardWith({ id: "env_1" }, deps)).toBe("new"); }); + it("coalesces concurrent misses into ONE read", async () => { + // Two misses must share a single read. Otherwise a slower read landing after a faster one + // writes its older snapshot back into the cache for a whole TTL. + let release: (flags: Record) => void = () => {}; + const gate = new Promise>((resolve) => { + release = resolve; + }); + const deps = wrapperDeps({ readFlags: () => gate }); + + const both = Promise.all([ + resolveMintShardWith({ id: "env_1" }, deps), + resolveMintShardWith({ id: "env_2" }, deps), + ]); + release({ runOpsMintShardSet: "a,b" }); + await both; + + expect(deps.reads).toBe(1); + }); + + it("does not let a slower read overwrite a newer one", async () => { + // The slow read starts first and finishes last. Its result must not become the cached + // value, because the fast read already published a newer snapshot. + let releaseSlow: (flags: Record) => void = () => {}; + const slow = new Promise>((resolve) => { + releaseSlow = resolve; + }); + let call = 0; + const deps = wrapperDeps({ + readFlags: () => { + call++; + return call === 1 ? slow : Promise.resolve({ runOpsMintShardSet: "c" }); + }, + }); + + const first = resolveMintShardWith({ id: "env_1" }, deps); + const second = resolveMintShardWith({ id: "env_2" }, deps); + releaseSlow({ runOpsMintShardSet: "a" }); + await Promise.all([first, second]); + + // One read served both, so there is no second snapshot to race with. + expect(deps.reads).toBe(1); + expect(deps.cache.current?.value.resolution.set).toEqual(["a"]); + }); + + it("clears the in-flight refresh after a failure, so the next call retries", async () => { + let fail = true; + const deps = wrapperDeps({ + readFlags: async () => { + if (fail) throw new Error("db down"); + return { runOpsMintShardSet: "a,b" }; + }, + }); + deps.onReadFailed = () => {}; + + expect(await resolveMintShardWith({ id: "env_1" }, deps)).toBe("new"); + fail = false; + expect(["a", "b"]).toContain(await resolveMintShardWith({ id: "env_1" }, deps)); + expect(deps.reads).toBe(2); + }); + it("agrees with the pure core for the same inputs", async () => { const deps = wrapperDeps(); const viaWrapper = await resolveMintShardWith({ id: "env_1" }, deps); diff --git a/apps/webapp/app/v3/runOpsMigration/mintShardAssignment.ts b/apps/webapp/app/v3/runOpsMigration/mintShardAssignment.ts index 910cd4769cd..a49a1a6a60d 100644 --- a/apps/webapp/app/v3/runOpsMigration/mintShardAssignment.ts +++ b/apps/webapp/app/v3/runOpsMigration/mintShardAssignment.ts @@ -133,11 +133,17 @@ type GlobalShardConfig = { resolution: MintShardSetResolution; override: unknown export type MintShardCache = { value: GlobalShardConfig; expiresAt: number } | undefined; +type MintShardCacheHandle = { + current: MintShardCache; + // The refresh currently in flight, if any. Concurrent misses share it. + inFlight?: Promise; +}; + export type ResolveMintShardDeps = { // Reads the list rows. Injected so the cache and the fail-safe are testable without a // database, the same way computeRunIdMintKind takes its flag reader. readFlags: () => Promise>; - cache: { current: MintShardCache }; + cache: MintShardCacheHandle; nowMs: number; ttlMs: number; graceMs: number; @@ -155,6 +161,20 @@ export type ResolveMintShardDeps = { // // A failed read falls back to gen-1 rather than guessing a list. Guessing would move every // environment's placement for the length of one blip. +async function refreshConfig(deps: ResolveMintShardDeps): Promise { + try { + const flags = await deps.readFlags(); + const config: GlobalShardConfig = { + resolution: readMintShardSetResolution(flags), + override: flags[FEATURE_FLAG.runOpsMintShardOverride], + }; + deps.cache.current = { value: config, expiresAt: deps.nowMs + deps.ttlMs }; + return config; + } finally { + deps.cache.inFlight = undefined; + } +} + export async function resolveMintShardWith( environment: { id: string; orgFeatureFlags?: unknown }, deps: ResolveMintShardDeps @@ -165,16 +185,13 @@ export async function resolveMintShardWith( config = cached.value; } else { try { - const flags = await deps.readFlags(); - config = { - resolution: readMintShardSetResolution(flags), - override: flags[FEATURE_FLAG.runOpsMintShardOverride], - }; + // Single-flight. Without it, two misses both read, and a slower read landing after a + // faster one puts its older snapshot back into the cache for a whole TTL. + config = await (deps.cache.inFlight ??= refreshConfig(deps)); } catch (error) { deps.onReadFailed?.(error); return "new"; } - deps.cache.current = { value: config, expiresAt: deps.nowMs + deps.ttlMs }; } return computeMintShard(environment, { diff --git a/apps/webapp/test/adminFeatureFlagsRouteAction.test.ts b/apps/webapp/test/adminFeatureFlagsRouteAction.test.ts index 1a3575192ae..b5bfd8ea052 100644 --- a/apps/webapp/test/adminFeatureFlagsRouteAction.test.ts +++ b/apps/webapp/test/adminFeatureFlagsRouteAction.test.ts @@ -2,7 +2,7 @@ // bug surface. These drive the real exported action against a real Postgres and assert on the rows // it leaves behind. The only module substituted is the auth wrapper, so the handler can be called // without a super-admin session; the database is the genuine article, injected into db.server. -import { boundedIn } from "@trigger.dev/database"; +import { boundedIn, $transaction as realTransaction } from "@trigger.dev/database"; import type { PrismaClient } from "@trigger.dev/database"; import { postgresTest } from "@internal/testcontainers"; import { describe, expect, vi } from "vitest"; @@ -22,18 +22,23 @@ vi.mock("~/db.server", () => ({ return db.client; }, boundedIn, - // The real helper adds tracing around prisma.$transaction and resolves undefined when it - // swallows an infrastructure error. Neither is under test here, but the transactional - // semantics are, so this stands in with the same shape and a real interactive transaction. - $transaction: async ( + // Delegates to the SAME shared implementation the production helper wraps, so the + // transactional semantics, the nesting case and the retry behaviour are the real ones rather + // than a reimplementation. Only the webapp wrapper's tracing span and its infrastructure-error + // logging are absent, and neither is asserted here. + $transaction: ( client: PrismaClient, nameOrFn: unknown, - fnOrOptions?: unknown - ): Promise => { - const fn = (typeof nameOrFn === "function" ? nameOrFn : fnOrOptions) as ( - tx: PrismaClient - ) => Promise; - return client.$transaction((tx) => fn(tx as unknown as PrismaClient)); + fnOrOptions?: unknown, + options?: unknown + ) => { + const fn = (typeof nameOrFn === "function" ? nameOrFn : fnOrOptions) as Parameters< + typeof realTransaction + >[1]; + const opts = (typeof nameOrFn === "function" ? fnOrOptions : options) as Parameters< + typeof realTransaction + >[3]; + return realTransaction(client, fn, () => {}, opts); }, }));