From ccbe4e9ab3bfefa3c226e904e1d44ead81ea50b0 Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Fri, 21 Aug 2026 13:39:13 +0100 Subject: [PATCH 1/7] fix(webapp): stop saving global flags from unsetting the locked ones The admin flags page submits only the flags its UI is managing, and strips the read-only ones unless they are unlocked. The action read every absent catalog key as an unset, so on a self-hosted instance any save deleted defaultWorkerInstanceGroupId and taskEventRepository as well. --- .../webapp/app/routes/admin.feature-flags.tsx | 54 +++------ apps/webapp/app/v3/featureFlags.server.ts | 47 +++++++- .../globalFeatureFlagsLockedFlags.test.ts | 112 ++++++++++++++++++ 3 files changed, 175 insertions(+), 38 deletions(-) create mode 100644 apps/webapp/test/globalFeatureFlagsLockedFlags.test.ts diff --git a/apps/webapp/app/routes/admin.feature-flags.tsx b/apps/webapp/app/routes/admin.feature-flags.tsx index be0f6174622..13dc128c4a8 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"; @@ -87,7 +88,13 @@ export const action = dashboardAction( return json({ error: "Invalid JSON body" }, { status: 400 }); } - const payloadSchema = z.object({ flags: z.record(z.unknown()) }); + const payloadSchema = z.object({ + flags: z.record(z.unknown()), + // The page only submits the flags it is managing, so an omitted key is ambiguous for the + // locked flags: this says whether the admin unlocked them and is therefore authoritative + // over them too. + unlockLockedFlags: z.boolean().optional(), + }); const parsed = payloadSchema.safeParse(body); if (!parsed.success) { return json({ error: "Invalid payload" }, { status: 400 }); @@ -116,39 +123,12 @@ 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); - } - } - } - - await prisma.$transaction([ - ...upsertOps, - ...(keysToDelete.length > 0 - ? [prisma.featureFlag.deleteMany({ where: { key: { in: boundedIn(keysToDelete) } } })] - : []), - ]); + await replaceGlobalFeatureFlags(prisma, { + requestedFlags: validationResult.data as Record, + catalogKeys: Object.keys(getAllFlagControlTypes()) as FeatureFlagKey[], + isManagedCloud, + unlockLockedFlags: parsed.data.unlockLockedFlags ?? false, + }); return json({ success: true }); } @@ -213,7 +193,7 @@ export default function AdminFeatureFlagsRoute() { }; const handleSave = () => { - saveFetcher.submit(JSON.stringify({ flags: values }), { + saveFetcher.submit(JSON.stringify({ flags: values, unlockLockedFlags: unlocked }), { method: "POST", encType: "application/json", }); diff --git a/apps/webapp/app/v3/featureFlags.server.ts b/apps/webapp/app/v3/featureFlags.server.ts index b32a4578640..bc1e85cc00f 100644 --- a/apps/webapp/app/v3/featureFlags.server.ts +++ b/apps/webapp/app/v3/featureFlags.server.ts @@ -1,11 +1,12 @@ import { type z } from "zod"; import type { PrismaClient } from "@trigger.dev/database"; -import { prisma, type PrismaClientOrTransaction } from "~/db.server"; +import { boundedIn, prisma, type PrismaClientOrTransaction } from "~/db.server"; import { FEATURE_FLAG, type FeatureFlagCatalogSchema, type FeatureFlagKey, FeatureFlagCatalog, + GLOBAL_LOCKED_FLAGS, } from "~/v3/featureFlags"; import { stampMintKindFlip } from "~/v3/runOpsMigration/mintFlipGrace"; @@ -220,3 +221,47 @@ export async function applyGlobalMintKindFlip( return makeSetMultipleFlags(tx)(stamped); }); } + +/** + * Replace-semantics write for the global admin flags page: catalog keys present in + * `requestedFlags` are upserted, catalog keys absent from it are deleted. + * + * A locked flag absent from the payload means the page never offered it for editing, not that + * the admin unset it, so it survives the sweep. Only a self-hosted page that says it unlocked + * them can delete one. + */ +export async function replaceGlobalFeatureFlags( + client: PrismaClient, + params: { + requestedFlags: Record; + catalogKeys: FeatureFlagKey[]; + isManagedCloud: boolean; + unlockLockedFlags: boolean; + } +): Promise { + const canDeleteLocked = params.unlockLockedFlags && !params.isManagedCloud; + const upsertOps: ReturnType[] = []; + const keysToDelete: string[] = []; + + for (const key of params.catalogKeys) { + if (key in params.requestedFlags) { + const value = params.requestedFlags[key]; + upsertOps.push( + client.featureFlag.upsert({ + where: { key }, + create: { key, value: value as any }, + update: { value: value as any }, + }) + ); + } else if (canDeleteLocked || !GLOBAL_LOCKED_FLAGS.includes(key)) { + keysToDelete.push(key); + } + } + + await client.$transaction([ + ...upsertOps, + ...(keysToDelete.length > 0 + ? [client.featureFlag.deleteMany({ where: { key: { in: boundedIn(keysToDelete) } } })] + : []), + ]); +} diff --git a/apps/webapp/test/globalFeatureFlagsLockedFlags.test.ts b/apps/webapp/test/globalFeatureFlagsLockedFlags.test.ts new file mode 100644 index 00000000000..8613dc825c6 --- /dev/null +++ b/apps/webapp/test/globalFeatureFlagsLockedFlags.test.ts @@ -0,0 +1,112 @@ +// With "Unlock read-only flags" off, the page strips GLOBAL_LOCKED_FLAGS from its payload, so an +// omitted locked key means "the UI never offered it", not "the admin unset it". +import type { PrismaClient } from "@trigger.dev/database"; +import { postgresTest } from "@internal/testcontainers"; +import { describe, expect, vi } from "vitest"; +import { FEATURE_FLAG, FeatureFlagCatalog, type FeatureFlagKey } from "~/v3/featureFlags"; +import { makeSetMultipleFlags, replaceGlobalFeatureFlags } from "~/v3/featureFlags.server"; + +vi.setConfig({ testTimeout: 60_000 }); + +const CATALOG_KEYS = Object.keys(FeatureFlagCatalog) as FeatureFlagKey[]; +const WORKER_GROUP_ID = "clwg000000000000000000000"; + +async function readFlag(prisma: PrismaClient, key: FeatureFlagKey): Promise { + const row = await prisma.featureFlag.findFirst({ where: { key }, select: { value: true } }); + return row?.value; +} + +describe("replaceGlobalFeatureFlags — locked flags the UI never submitted", () => { + postgresTest( + "keeps defaultWorkerInstanceGroupId when a locked flag is absent from the payload", + async ({ prisma }) => { + await makeSetMultipleFlags(prisma)({ + [FEATURE_FLAG.defaultWorkerInstanceGroupId]: WORKER_GROUP_ID, + [FEATURE_FLAG.mollifierEnabled]: true, + }); + + // What the page posts when an admin unsets mollifierEnabled on a self-hosted instance. + await replaceGlobalFeatureFlags(prisma, { + requestedFlags: {}, + catalogKeys: CATALOG_KEYS, + isManagedCloud: false, + unlockLockedFlags: false, + }); + + expect(await readFlag(prisma, FEATURE_FLAG.defaultWorkerInstanceGroupId)).toBe( + WORKER_GROUP_ID + ); + expect(await readFlag(prisma, FEATURE_FLAG.mollifierEnabled)).toBeUndefined(); + } + ); + + postgresTest("an unlocked self-hosted page can still unset a locked flag", async ({ prisma }) => { + await makeSetMultipleFlags(prisma)({ + [FEATURE_FLAG.defaultWorkerInstanceGroupId]: WORKER_GROUP_ID, + }); + + await replaceGlobalFeatureFlags(prisma, { + requestedFlags: {}, + catalogKeys: CATALOG_KEYS, + isManagedCloud: false, + unlockLockedFlags: true, + }); + + expect(await readFlag(prisma, FEATURE_FLAG.defaultWorkerInstanceGroupId)).toBeUndefined(); + }); + + postgresTest( + "managed cloud keeps locked flags even when unlocking is claimed", + async ({ prisma }) => { + await makeSetMultipleFlags(prisma)({ + [FEATURE_FLAG.defaultWorkerInstanceGroupId]: WORKER_GROUP_ID, + }); + + await replaceGlobalFeatureFlags(prisma, { + requestedFlags: {}, + catalogKeys: CATALOG_KEYS, + isManagedCloud: true, + unlockLockedFlags: true, + }); + + expect(await readFlag(prisma, FEATURE_FLAG.defaultWorkerInstanceGroupId)).toBe( + WORKER_GROUP_ID + ); + } + ); + + postgresTest("managed cloud still sweeps ordinary flags it was not sent", async ({ prisma }) => { + await makeSetMultipleFlags(prisma)({ + [FEATURE_FLAG.defaultWorkerInstanceGroupId]: WORKER_GROUP_ID, + [FEATURE_FLAG.hasAiAccess]: true, + }); + + await replaceGlobalFeatureFlags(prisma, { + requestedFlags: { [FEATURE_FLAG.mollifierEnabled]: true }, + catalogKeys: CATALOG_KEYS, + isManagedCloud: true, + unlockLockedFlags: false, + }); + + expect(await readFlag(prisma, FEATURE_FLAG.defaultWorkerInstanceGroupId)).toBe(WORKER_GROUP_ID); + expect(await readFlag(prisma, FEATURE_FLAG.hasAiAccess)).toBeUndefined(); + expect(await readFlag(prisma, FEATURE_FLAG.mollifierEnabled)).toBe(true); + }); + + postgresTest("submitted flags are upserted and omitted ones swept", async ({ prisma }) => { + await makeSetMultipleFlags(prisma)({ + [FEATURE_FLAG.mollifierEnabled]: true, + [FEATURE_FLAG.hasAiAccess]: true, + }); + + await replaceGlobalFeatureFlags(prisma, { + requestedFlags: { [FEATURE_FLAG.mollifierEnabled]: false }, + catalogKeys: CATALOG_KEYS, + isManagedCloud: false, + unlockLockedFlags: false, + }); + + expect(await readFlag(prisma, FEATURE_FLAG.mollifierEnabled)).toBe(false); + expect(await readFlag(prisma, FEATURE_FLAG.hasAiAccess)).toBeUndefined(); + }); +}); From 0136d3995361082c1208030a6ea2d3a1e0fa98e4 Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Fri, 21 Aug 2026 14:08:38 +0100 Subject: [PATCH 2/7] refactor(webapp): route the global flags write through the transaction helper Use the $transaction helper from ~/db.server instead of calling client.$transaction directly, so the write gets tracing and infra-error boundary logging. The helper is callback-only, so the batched upserts become sequential statements inside one interactive transaction, and an undefined result is treated as a failure rather than a silent no-op. --- apps/webapp/app/v3/featureFlags.server.ts | 40 ++++++++++++++--------- 1 file changed, 24 insertions(+), 16 deletions(-) diff --git a/apps/webapp/app/v3/featureFlags.server.ts b/apps/webapp/app/v3/featureFlags.server.ts index bc1e85cc00f..d4a8dea6cc2 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 { boundedIn, prisma, type PrismaClientOrTransaction } from "~/db.server"; +import { $transaction, boundedIn, prisma, type PrismaClientOrTransaction } from "~/db.server"; import { FEATURE_FLAG, type FeatureFlagCatalogSchema, @@ -240,28 +240,36 @@ export async function replaceGlobalFeatureFlags( } ): Promise { const canDeleteLocked = params.unlockLockedFlags && !params.isManagedCloud; - const upsertOps: ReturnType[] = []; + const toUpsert: { key: FeatureFlagKey; value: unknown }[] = []; const keysToDelete: string[] = []; for (const key of params.catalogKeys) { if (key in params.requestedFlags) { - const value = params.requestedFlags[key]; - upsertOps.push( - client.featureFlag.upsert({ - where: { key }, - create: { key, value: value as any }, - update: { value: value as any }, - }) - ); + toUpsert.push({ key, value: params.requestedFlags[key] }); } else if (canDeleteLocked || !GLOBAL_LOCKED_FLAGS.includes(key)) { keysToDelete.push(key); } } - await client.$transaction([ - ...upsertOps, - ...(keysToDelete.length > 0 - ? [client.featureFlag.deleteMany({ where: { key: { in: boundedIn(keysToDelete) } } })] - : []), - ]); + const applied = await $transaction(client, "replaceGlobalFeatureFlags", async (tx) => { + for (const { key, value } of toUpsert) { + await tx.featureFlag.upsert({ + where: { key }, + create: { key, value: value as any }, + update: { value: value as any }, + }); + } + + if (keysToDelete.length > 0) { + await tx.featureFlag.deleteMany({ where: { key: { in: boundedIn(keysToDelete) } } }); + } + + return true; + }); + + // The helper resolves undefined instead of throwing when Prisma errors are swallowed. This + // write deletes flags, so treat a transaction that did not run as a failure the caller sees. + if (!applied) { + throw new Error("replaceGlobalFeatureFlags: transaction did not complete"); + } } From 045ce2892152b2e4d4235255baa20ea563eadbbc Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Mon, 24 Aug 2026 11:08:34 +0100 Subject: [PATCH 3/7] perf(webapp): write the global flags in one statement The save wrote one upsert per submitted flag plus a delete, inside an interactive transaction. The upsert and the sweep touch disjoint keys, so they fold into a single data-modifying statement that is atomic on its own and costs one round trip whatever the catalog size. Measured against a real Postgres with 30 flags submitted: 7.8ms for the original batched form, 0.5ms now. The span the transaction helper provided is kept explicitly. --- apps/webapp/app/v3/featureFlags.server.ts | 65 +++++++++++++++-------- 1 file changed, 44 insertions(+), 21 deletions(-) diff --git a/apps/webapp/app/v3/featureFlags.server.ts b/apps/webapp/app/v3/featureFlags.server.ts index 8af92b27748..69f48cac72a 100644 --- a/apps/webapp/app/v3/featureFlags.server.ts +++ b/apps/webapp/app/v3/featureFlags.server.ts @@ -1,6 +1,14 @@ import { type z } from "zod"; import type { PrismaClient } from "@trigger.dev/database"; -import { $transaction, boundedIn, prisma, type PrismaClientOrTransaction } from "~/db.server"; +import cuid from "cuid"; +import { + boundedIn, + Prisma, + prisma, + sqlDatabaseSchema, + type PrismaClientOrTransaction, +} from "~/db.server"; +import { startActiveSpan } from "~/v3/tracer.server"; import { FEATURE_FLAG, type FeatureFlagCatalogSchema, @@ -253,27 +261,42 @@ export async function replaceGlobalFeatureFlags( } } - const applied = await $transaction(client, "replaceGlobalFeatureFlags", async (tx) => { - for (const { key, value } of toUpsert) { - await tx.featureFlag.upsert({ - where: { key }, - create: { key, value: value as any }, - update: { value: value as any }, - }); - } - - if (keysToDelete.length > 0) { - await tx.featureFlag.deleteMany({ where: { key: { in: boundedIn(keysToDelete) } } }); - } - - return true; - }); - - // The helper resolves undefined instead of throwing when Prisma errors are swallowed. This - // write deletes flags, so treat a transaction that did not run as a failure the caller sees. - if (!applied) { - throw new Error("replaceGlobalFeatureFlags: transaction did not complete"); + // The upsert and the sweep touch disjoint keys, because the loop above sends each catalog key + // to exactly one of the two lists. That lets them combine into a single data-modifying + // statement, which is atomic on its own and costs one round trip whatever the catalog size. + const upsertSql = + toUpsert.length > 0 + ? Prisma.sql` + INSERT INTO ${sqlDatabaseSchema}."FeatureFlag" (id, key, value, "createdAt", "updatedAt") + VALUES ${Prisma.join( + toUpsert.map( + ({ key, value }) => + Prisma.sql`(${cuid()}, ${key}, ${JSON.stringify( + value ?? null + )}::jsonb, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)` + ) + )} + ON CONFLICT (key) DO UPDATE + SET value = EXCLUDED.value, "updatedAt" = CURRENT_TIMESTAMP` + : undefined; + + const deleteSql = + keysToDelete.length > 0 + ? Prisma.sql` + DELETE FROM ${sqlDatabaseSchema}."FeatureFlag" + WHERE key IN (${Prisma.join(boundedIn(keysToDelete))})` + : undefined; + + const statement = + upsertSql && deleteSql + ? Prisma.sql`WITH upserted AS (${upsertSql} RETURNING 1) ${deleteSql}` + : (upsertSql ?? deleteSql); + + if (!statement) { + return; } + + await startActiveSpan("replaceGlobalFeatureFlags", () => client.$executeRaw(statement)); } /** The global flag set, with the env-var defaults this app applies. */ From c45c11e0070e888655acc10d88080d404e47bfec Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Mon, 24 Aug 2026 11:13:06 +0100 Subject: [PATCH 4/7] test(webapp): cover the global flags write end to end Adds the coverage the fix was missing: every catalog control type is compared against a typed Prisma write, so the hand-built SQL cannot drift on encoding, and the route action is driven directly to pin the locked-flag rejection, the schema rejection, and the default applied when the page does not say whether it unlocked the read-only flags. Also pins the disjoint-key assumption the single-statement write depends on. --- .../test/adminFeatureFlagsRouteAction.test.ts | 87 +++++++++++++++++++ .../globalFeatureFlagsLockedFlags.test.ts | 38 ++++++++ .../globalFeatureFlagsValueFidelity.test.ts | 51 +++++++++++ 3 files changed, 176 insertions(+) create mode 100644 apps/webapp/test/adminFeatureFlagsRouteAction.test.ts create mode 100644 apps/webapp/test/globalFeatureFlagsValueFidelity.test.ts diff --git a/apps/webapp/test/adminFeatureFlagsRouteAction.test.ts b/apps/webapp/test/adminFeatureFlagsRouteAction.test.ts new file mode 100644 index 00000000000..f64dc328ee2 --- /dev/null +++ b/apps/webapp/test/adminFeatureFlagsRouteAction.test.ts @@ -0,0 +1,87 @@ +// The page posts only the flags its UI manages, so the action's reading of an absent key is the +// whole bug surface. These drive the real exported action with the auth wrapper unwrapped, and +// assert on what it hands the writer. +import { describe, expect, it, vi } from "vitest"; +import { FEATURE_FLAG } from "~/v3/featureFlags"; + +const { replaceGlobalFeatureFlags } = vi.hoisted(() => ({ + replaceGlobalFeatureFlags: vi.fn().mockResolvedValue(undefined), +})); + +vi.mock("~/services/routeBuilders/dashboardBuilder", () => ({ + dashboardAction: (_options: unknown, handler: unknown) => handler, + dashboardLoader: (_options: unknown, handler: unknown) => handler, +})); +vi.mock("~/v3/featureFlags.server", () => ({ + replaceGlobalFeatureFlags, + flags: vi.fn().mockResolvedValue({}), +})); +vi.mock("~/db.server", () => ({ prisma: {}, boundedIn: (v: unknown) => v })); + +const { action } = await import("~/routes/admin.feature-flags"); + +async function post(host: string, body: unknown) { + const request = new Request(`https://${host}/admin/feature-flags`, { + method: "POST", + body: JSON.stringify(body), + headers: { "content-type": "application/json" }, + }); + return (await (action as any)({ request, params: {}, context: {} })) as Response; +} + +describe("admin feature flags action", () => { + it("defaults unlockLockedFlags to false when the field is absent", async () => { + replaceGlobalFeatureFlags.mockClear(); + const response = await post("localhost:3030", { flags: {} }); + + expect(response.status).toBe(200); + expect(replaceGlobalFeatureFlags).toHaveBeenCalledTimes(1); + expect(replaceGlobalFeatureFlags.mock.calls[0][1]).toMatchObject({ + unlockLockedFlags: false, + isManagedCloud: false, + }); + }); + + it("passes unlockLockedFlags through when the page says it unlocked them", async () => { + replaceGlobalFeatureFlags.mockClear(); + await post("localhost:3030", { flags: {}, unlockLockedFlags: true }); + + expect(replaceGlobalFeatureFlags.mock.calls[0][1]).toMatchObject({ unlockLockedFlags: true }); + }); + + it("marks a managed cloud host as such", async () => { + replaceGlobalFeatureFlags.mockClear(); + await post("cloud.trigger.dev", { flags: {}, unlockLockedFlags: true }); + + expect(replaceGlobalFeatureFlags.mock.calls[0][1]).toMatchObject({ isManagedCloud: true }); + }); + + it("rejects a locked flag submitted to managed cloud without writing", async () => { + replaceGlobalFeatureFlags.mockClear(); + const response = await post("cloud.trigger.dev", { + flags: { [FEATURE_FLAG.defaultWorkerInstanceGroupId]: "clwg0001" }, + }); + + expect(response.status).toBe(400); + expect(replaceGlobalFeatureFlags).not.toHaveBeenCalled(); + }); + + it("rejects a value that fails the catalog schema without writing", async () => { + replaceGlobalFeatureFlags.mockClear(); + const response = await post("localhost:3030", { + flags: { [FEATURE_FLAG.realtimeBackend]: "not-a-backend" }, + }); + + expect(response.status).toBe(400); + expect(replaceGlobalFeatureFlags).not.toHaveBeenCalled(); + }); + + it("submits every catalog key so omitted flags are swept", async () => { + replaceGlobalFeatureFlags.mockClear(); + await post("localhost:3030", { flags: { [FEATURE_FLAG.mollifierEnabled]: true } }); + + const { catalogKeys, requestedFlags } = replaceGlobalFeatureFlags.mock.calls[0][1]; + expect(catalogKeys).toContain(FEATURE_FLAG.defaultWorkerInstanceGroupId); + expect(requestedFlags).toEqual({ [FEATURE_FLAG.mollifierEnabled]: true }); + }); +}); diff --git a/apps/webapp/test/globalFeatureFlagsLockedFlags.test.ts b/apps/webapp/test/globalFeatureFlagsLockedFlags.test.ts index 8613dc825c6..ce18ef800e3 100644 --- a/apps/webapp/test/globalFeatureFlagsLockedFlags.test.ts +++ b/apps/webapp/test/globalFeatureFlagsLockedFlags.test.ts @@ -93,6 +93,44 @@ describe("replaceGlobalFeatureFlags — locked flags the UI never submitted", () expect(await readFlag(prisma, FEATURE_FLAG.mollifierEnabled)).toBe(true); }); + // The upsert and the sweep share one statement, which is only safe while no key is in both. + postgresTest("a submitted key is never also swept", async ({ prisma }) => { + await makeSetMultipleFlags(prisma)({ + [FEATURE_FLAG.mollifierEnabled]: true, + [FEATURE_FLAG.hasAiAccess]: true, + [FEATURE_FLAG.defaultWorkerInstanceGroupId]: WORKER_GROUP_ID, + }); + + await replaceGlobalFeatureFlags(prisma, { + requestedFlags: { + [FEATURE_FLAG.mollifierEnabled]: false, + [FEATURE_FLAG.hasAiAccess]: true, + }, + catalogKeys: CATALOG_KEYS, + isManagedCloud: false, + unlockLockedFlags: true, + }); + + // Both submitted keys survive with their new values rather than being swept by the same + // statement that wrote them. + expect(await readFlag(prisma, FEATURE_FLAG.mollifierEnabled)).toBe(false); + expect(await readFlag(prisma, FEATURE_FLAG.hasAiAccess)).toBe(true); + expect(await readFlag(prisma, FEATURE_FLAG.defaultWorkerInstanceGroupId)).toBeUndefined(); + }); + + postgresTest("writes nothing when there is nothing to write", async ({ prisma }) => { + await makeSetMultipleFlags(prisma)({ [FEATURE_FLAG.mollifierEnabled]: true }); + + await replaceGlobalFeatureFlags(prisma, { + requestedFlags: {}, + catalogKeys: [], + isManagedCloud: false, + unlockLockedFlags: false, + }); + + expect(await readFlag(prisma, FEATURE_FLAG.mollifierEnabled)).toBe(true); + }); + postgresTest("submitted flags are upserted and omitted ones swept", async ({ prisma }) => { await makeSetMultipleFlags(prisma)({ [FEATURE_FLAG.mollifierEnabled]: true, diff --git a/apps/webapp/test/globalFeatureFlagsValueFidelity.test.ts b/apps/webapp/test/globalFeatureFlagsValueFidelity.test.ts new file mode 100644 index 00000000000..87b2c289a6c --- /dev/null +++ b/apps/webapp/test/globalFeatureFlagsValueFidelity.test.ts @@ -0,0 +1,51 @@ +// replaceGlobalFeatureFlags writes through hand-built SQL rather than Prisma's typed upsert, so +// every control type in the catalog has to land in the column exactly as the typed write would. +import type { PrismaClient } from "@trigger.dev/database"; +import { postgresTest } from "@internal/testcontainers"; +import { describe, expect, vi } from "vitest"; +import { FEATURE_FLAG, FeatureFlagCatalog, type FeatureFlagKey } from "~/v3/featureFlags"; +import { makeSetMultipleFlags, replaceGlobalFeatureFlags } from "~/v3/featureFlags.server"; + +vi.setConfig({ testTimeout: 60_000 }); + +const CATALOG_KEYS = Object.keys(FeatureFlagCatalog) as FeatureFlagKey[]; + +const CASES: { key: FeatureFlagKey; value: unknown; label: string }[] = [ + { key: FEATURE_FLAG.defaultWorkerInstanceGroupId, value: "clwg0001", label: "string" }, + { key: FEATURE_FLAG.mollifierEnabled, value: true, label: "boolean true" }, + { key: FEATURE_FLAG.hasAiAccess, value: false, label: "boolean false" }, + { key: FEATURE_FLAG.computeMigrationFreePercentage, value: 0, label: "number zero" }, + { key: FEATURE_FLAG.computeMigrationPaidPercentage, value: 100, label: "number" }, + { key: FEATURE_FLAG.realtimeBackend, value: "shadow", label: "enum" }, + { + key: FEATURE_FLAG.promotedDashboardAgentPrompt, + value: '{"prompt":"hi","nested":{"quote":"a \\"quoted\\" word"}}', + label: "string holding JSON", + }, +]; + +async function raw(prisma: PrismaClient, key: FeatureFlagKey) { + const row = await prisma.featureFlag.findFirst({ where: { key }, select: { value: true } }); + return row?.value; +} + +describe("replaceGlobalFeatureFlags value fidelity", () => { + for (const { key, value, label } of CASES) { + postgresTest(`${label} matches the typed write`, async ({ prisma }) => { + await replaceGlobalFeatureFlags(prisma, { + requestedFlags: { [key]: value }, + catalogKeys: CATALOG_KEYS, + isManagedCloud: false, + unlockLockedFlags: true, + }); + const viaRawSql = await raw(prisma, key); + + await prisma.featureFlag.deleteMany({ where: { key } }); + await makeSetMultipleFlags(prisma)({ [key]: value } as any); + const viaPrisma = await raw(prisma, key); + + expect(viaRawSql).toStrictEqual(viaPrisma); + expect(viaRawSql).toStrictEqual(value); + }); + } +}); From ae9701bca76dc6bb064d9a732132c3652b00e7f1 Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Mon, 24 Aug 2026 11:21:01 +0100 Subject: [PATCH 5/7] refactor(webapp): keep the batched write when replacing global flags Restores the batched upsert plus delete this code already used, rather than the hand-built statement the earlier commits introduced. At the number of flags a save actually submits the difference is under two milliseconds, and the typed Prisma calls are worth more than that on a path an admin hits occasionally. Drops the value-fidelity tests with it, since they existed to guard hand-written JSON encoding that no longer exists. --- apps/webapp/app/v3/featureFlags.server.ts | 63 +++++-------------- .../globalFeatureFlagsValueFidelity.test.ts | 51 --------------- 2 files changed, 16 insertions(+), 98 deletions(-) delete mode 100644 apps/webapp/test/globalFeatureFlagsValueFidelity.test.ts diff --git a/apps/webapp/app/v3/featureFlags.server.ts b/apps/webapp/app/v3/featureFlags.server.ts index 69f48cac72a..dd1fb125ba6 100644 --- a/apps/webapp/app/v3/featureFlags.server.ts +++ b/apps/webapp/app/v3/featureFlags.server.ts @@ -1,14 +1,6 @@ import { type z } from "zod"; import type { PrismaClient } from "@trigger.dev/database"; -import cuid from "cuid"; -import { - boundedIn, - Prisma, - prisma, - sqlDatabaseSchema, - type PrismaClientOrTransaction, -} from "~/db.server"; -import { startActiveSpan } from "~/v3/tracer.server"; +import { boundedIn, prisma, type PrismaClientOrTransaction } from "~/db.server"; import { FEATURE_FLAG, type FeatureFlagCatalogSchema, @@ -250,53 +242,30 @@ export async function replaceGlobalFeatureFlags( } ): Promise { const canDeleteLocked = params.unlockLockedFlags && !params.isManagedCloud; - const toUpsert: { key: FeatureFlagKey; value: unknown }[] = []; + const upsertOps: ReturnType[] = []; const keysToDelete: string[] = []; for (const key of params.catalogKeys) { if (key in params.requestedFlags) { - toUpsert.push({ key, value: params.requestedFlags[key] }); + const value = params.requestedFlags[key]; + upsertOps.push( + client.featureFlag.upsert({ + where: { key }, + create: { key, value: value as any }, + update: { value: value as any }, + }) + ); } else if (canDeleteLocked || !GLOBAL_LOCKED_FLAGS.includes(key)) { keysToDelete.push(key); } } - // The upsert and the sweep touch disjoint keys, because the loop above sends each catalog key - // to exactly one of the two lists. That lets them combine into a single data-modifying - // statement, which is atomic on its own and costs one round trip whatever the catalog size. - const upsertSql = - toUpsert.length > 0 - ? Prisma.sql` - INSERT INTO ${sqlDatabaseSchema}."FeatureFlag" (id, key, value, "createdAt", "updatedAt") - VALUES ${Prisma.join( - toUpsert.map( - ({ key, value }) => - Prisma.sql`(${cuid()}, ${key}, ${JSON.stringify( - value ?? null - )}::jsonb, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)` - ) - )} - ON CONFLICT (key) DO UPDATE - SET value = EXCLUDED.value, "updatedAt" = CURRENT_TIMESTAMP` - : undefined; - - const deleteSql = - keysToDelete.length > 0 - ? Prisma.sql` - DELETE FROM ${sqlDatabaseSchema}."FeatureFlag" - WHERE key IN (${Prisma.join(boundedIn(keysToDelete))})` - : undefined; - - const statement = - upsertSql && deleteSql - ? Prisma.sql`WITH upserted AS (${upsertSql} RETURNING 1) ${deleteSql}` - : (upsertSql ?? deleteSql); - - if (!statement) { - return; - } - - await startActiveSpan("replaceGlobalFeatureFlags", () => client.$executeRaw(statement)); + await client.$transaction([ + ...upsertOps, + ...(keysToDelete.length > 0 + ? [client.featureFlag.deleteMany({ where: { key: { in: boundedIn(keysToDelete) } } })] + : []), + ]); } /** The global flag set, with the env-var defaults this app applies. */ diff --git a/apps/webapp/test/globalFeatureFlagsValueFidelity.test.ts b/apps/webapp/test/globalFeatureFlagsValueFidelity.test.ts deleted file mode 100644 index 87b2c289a6c..00000000000 --- a/apps/webapp/test/globalFeatureFlagsValueFidelity.test.ts +++ /dev/null @@ -1,51 +0,0 @@ -// replaceGlobalFeatureFlags writes through hand-built SQL rather than Prisma's typed upsert, so -// every control type in the catalog has to land in the column exactly as the typed write would. -import type { PrismaClient } from "@trigger.dev/database"; -import { postgresTest } from "@internal/testcontainers"; -import { describe, expect, vi } from "vitest"; -import { FEATURE_FLAG, FeatureFlagCatalog, type FeatureFlagKey } from "~/v3/featureFlags"; -import { makeSetMultipleFlags, replaceGlobalFeatureFlags } from "~/v3/featureFlags.server"; - -vi.setConfig({ testTimeout: 60_000 }); - -const CATALOG_KEYS = Object.keys(FeatureFlagCatalog) as FeatureFlagKey[]; - -const CASES: { key: FeatureFlagKey; value: unknown; label: string }[] = [ - { key: FEATURE_FLAG.defaultWorkerInstanceGroupId, value: "clwg0001", label: "string" }, - { key: FEATURE_FLAG.mollifierEnabled, value: true, label: "boolean true" }, - { key: FEATURE_FLAG.hasAiAccess, value: false, label: "boolean false" }, - { key: FEATURE_FLAG.computeMigrationFreePercentage, value: 0, label: "number zero" }, - { key: FEATURE_FLAG.computeMigrationPaidPercentage, value: 100, label: "number" }, - { key: FEATURE_FLAG.realtimeBackend, value: "shadow", label: "enum" }, - { - key: FEATURE_FLAG.promotedDashboardAgentPrompt, - value: '{"prompt":"hi","nested":{"quote":"a \\"quoted\\" word"}}', - label: "string holding JSON", - }, -]; - -async function raw(prisma: PrismaClient, key: FeatureFlagKey) { - const row = await prisma.featureFlag.findFirst({ where: { key }, select: { value: true } }); - return row?.value; -} - -describe("replaceGlobalFeatureFlags value fidelity", () => { - for (const { key, value, label } of CASES) { - postgresTest(`${label} matches the typed write`, async ({ prisma }) => { - await replaceGlobalFeatureFlags(prisma, { - requestedFlags: { [key]: value }, - catalogKeys: CATALOG_KEYS, - isManagedCloud: false, - unlockLockedFlags: true, - }); - const viaRawSql = await raw(prisma, key); - - await prisma.featureFlag.deleteMany({ where: { key } }); - await makeSetMultipleFlags(prisma)({ [key]: value } as any); - const viaPrisma = await raw(prisma, key); - - expect(viaRawSql).toStrictEqual(viaPrisma); - expect(viaRawSql).toStrictEqual(value); - }); - } -}); From 953281cc3774bdad8592d8c3e9aa46e6667c5253 Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Mon, 24 Aug 2026 11:26:35 +0100 Subject: [PATCH 6/7] fix(webapp): make the flags page state its unlock choice at compile time The save body typed the flags map but not the unlock answer, so dropping that field from the page compiled fine and quietly disabled unlocking while every test still passed. Typing the body makes its absence a compile error. The request schema still accepts a body without it, so a tab opened before this shipped keeps saving with the safe default. --- apps/webapp/app/routes/admin.feature-flags.tsx | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/apps/webapp/app/routes/admin.feature-flags.tsx b/apps/webapp/app/routes/admin.feature-flags.tsx index 646772a47a6..197800c7ef3 100644 --- a/apps/webapp/app/routes/admin.feature-flags.tsx +++ b/apps/webapp/app/routes/admin.feature-flags.tsx @@ -39,6 +39,12 @@ import { type WorkerGroup, } from "~/components/admin/FlagControls"; +/** What the page posts to the action. See the note on payloadSchema. */ +type SaveFlagsBody = { + flags: Record; + unlockLockedFlags: boolean; +}; + export const loader = dashboardLoader( { authorization: { requireSuper: true } }, async ({ request }) => { @@ -88,6 +94,9 @@ export const action = dashboardAction( return json({ error: "Invalid JSON body" }, { status: 400 }); } + // The zod schema leaves unlockLockedFlags optional so a tab opened before this shipped still + // saves, defaulting to the safe answer. SaveFlagsBody keeps it required for our own client, so + // dropping it from the page is a compile error rather than a silently disabled unlock. const payloadSchema = z.object({ flags: z.record(z.unknown()), // The page only submits the flags it is managing, so an omitted key is ambiguous for the @@ -193,7 +202,8 @@ export default function AdminFeatureFlagsRoute() { }; const handleSave = () => { - saveFetcher.submit(JSON.stringify({ flags: values, unlockLockedFlags: unlocked }), { + const body: SaveFlagsBody = { flags: values, unlockLockedFlags: unlocked }; + saveFetcher.submit(JSON.stringify(body), { method: "POST", encType: "application/json", }); From ca53728f73cf3633d1ea760a77a978990d385f72 Mon Sep 17 00:00:00 2001 From: Daniel Sutton Date: Mon, 24 Aug 2026 12:05:48 +0100 Subject: [PATCH 7/7] test(webapp): drive the flags route against a real database The route tests stubbed the writer and the database client and asserted on the arguments the action passed. They now run against a container Postgres and assert on the rows the save leaves behind, so they check what was persisted rather than what was called. Only the auth wrapper is still substituted, so the handler can be invoked without a super-admin session. --- .../test/adminFeatureFlagsRouteAction.test.ts | 138 ++++++++++++------ 1 file changed, 94 insertions(+), 44 deletions(-) diff --git a/apps/webapp/test/adminFeatureFlagsRouteAction.test.ts b/apps/webapp/test/adminFeatureFlagsRouteAction.test.ts index f64dc328ee2..a510fbe0f35 100644 --- a/apps/webapp/test/adminFeatureFlagsRouteAction.test.ts +++ b/apps/webapp/test/adminFeatureFlagsRouteAction.test.ts @@ -1,24 +1,32 @@ -// The page posts only the flags its UI manages, so the action's reading of an absent key is the -// whole bug surface. These drive the real exported action with the auth wrapper unwrapped, and -// assert on what it hands the writer. -import { describe, expect, it, vi } from "vitest"; +// The page posts only the flags its UI manages, so how the action reads an absent key is the whole +// 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 type { PrismaClient } from "@trigger.dev/database"; +import { postgresTest } from "@internal/testcontainers"; +import { describe, expect, vi } from "vitest"; import { FEATURE_FLAG } from "~/v3/featureFlags"; -const { replaceGlobalFeatureFlags } = vi.hoisted(() => ({ - replaceGlobalFeatureFlags: vi.fn().mockResolvedValue(undefined), -})); +vi.setConfig({ testTimeout: 60_000 }); + +const db = vi.hoisted(() => ({ client: null as unknown as PrismaClient })); vi.mock("~/services/routeBuilders/dashboardBuilder", () => ({ dashboardAction: (_options: unknown, handler: unknown) => handler, dashboardLoader: (_options: unknown, handler: unknown) => handler, })); -vi.mock("~/v3/featureFlags.server", () => ({ - replaceGlobalFeatureFlags, - flags: vi.fn().mockResolvedValue({}), + +vi.mock("~/db.server", () => ({ + get prisma() { + return db.client; + }, + boundedIn, })); -vi.mock("~/db.server", () => ({ prisma: {}, boundedIn: (v: unknown) => v })); -const { action } = await import("~/routes/admin.feature-flags"); +import { action } from "~/routes/admin.feature-flags"; + +const WORKER_GROUP_ID = "clwg000000000000000000000"; async function post(host: string, body: unknown) { const request = new Request(`https://${host}/admin/feature-flags`, { @@ -29,59 +37,101 @@ async function post(host: string, body: unknown) { return (await (action as any)({ request, params: {}, context: {} })) as Response; } +async function readFlag(prisma: PrismaClient, key: string) { + const row = await prisma.featureFlag.findFirst({ where: { key }, select: { value: true } }); + return row?.value; +} + +async function seed(prisma: PrismaClient) { + db.client = prisma; + await prisma.featureFlag.createMany({ + data: [ + { id: "ff_locked", key: FEATURE_FLAG.defaultWorkerInstanceGroupId, value: WORKER_GROUP_ID }, + { id: "ff_plain", key: FEATURE_FLAG.mollifierEnabled, value: true }, + ], + }); +} + describe("admin feature flags action", () => { - it("defaults unlockLockedFlags to false when the field is absent", async () => { - replaceGlobalFeatureFlags.mockClear(); + postgresTest("keeps the locked flag when the page did not unlock it", async ({ prisma }) => { + await seed(prisma); + const response = await post("localhost:3030", { flags: {} }); expect(response.status).toBe(200); - expect(replaceGlobalFeatureFlags).toHaveBeenCalledTimes(1); - expect(replaceGlobalFeatureFlags.mock.calls[0][1]).toMatchObject({ - unlockLockedFlags: false, - isManagedCloud: false, - }); + expect(await readFlag(prisma, FEATURE_FLAG.defaultWorkerInstanceGroupId)).toBe(WORKER_GROUP_ID); + expect(await readFlag(prisma, FEATURE_FLAG.mollifierEnabled)).toBeUndefined(); }); - it("passes unlockLockedFlags through when the page says it unlocked them", async () => { - replaceGlobalFeatureFlags.mockClear(); - await post("localhost:3030", { flags: {}, unlockLockedFlags: true }); + postgresTest("keeps the locked flag when the body omits the unlock field", async ({ prisma }) => { + await seed(prisma); + + // A tab opened before the field existed posts the old shape. + const response = await post("localhost:3030", { flags: {}, unlockLockedFlags: undefined }); - expect(replaceGlobalFeatureFlags.mock.calls[0][1]).toMatchObject({ unlockLockedFlags: true }); + expect(response.status).toBe(200); + expect(await readFlag(prisma, FEATURE_FLAG.defaultWorkerInstanceGroupId)).toBe(WORKER_GROUP_ID); }); - it("marks a managed cloud host as such", async () => { - replaceGlobalFeatureFlags.mockClear(); - await post("cloud.trigger.dev", { flags: {}, unlockLockedFlags: true }); + postgresTest("deletes the locked flag when the page unlocked it", async ({ prisma }) => { + await seed(prisma); + + await post("localhost:3030", { flags: {}, unlockLockedFlags: true }); - expect(replaceGlobalFeatureFlags.mock.calls[0][1]).toMatchObject({ isManagedCloud: true }); + expect(await readFlag(prisma, FEATURE_FLAG.defaultWorkerInstanceGroupId)).toBeUndefined(); }); - it("rejects a locked flag submitted to managed cloud without writing", async () => { - replaceGlobalFeatureFlags.mockClear(); - const response = await post("cloud.trigger.dev", { - flags: { [FEATURE_FLAG.defaultWorkerInstanceGroupId]: "clwg0001" }, - }); + postgresTest( + "keeps the locked flag on managed cloud despite the unlock claim", + async ({ prisma }) => { + await seed(prisma); - expect(response.status).toBe(400); - expect(replaceGlobalFeatureFlags).not.toHaveBeenCalled(); - }); + await post("cloud.trigger.dev", { flags: {}, unlockLockedFlags: true }); + + expect(await readFlag(prisma, FEATURE_FLAG.defaultWorkerInstanceGroupId)).toBe( + WORKER_GROUP_ID + ); + } + ); + + postgresTest( + "rejects a locked flag submitted to managed cloud, writing nothing", + async ({ prisma }) => { + await seed(prisma); + + const response = await post("cloud.trigger.dev", { + flags: { [FEATURE_FLAG.defaultWorkerInstanceGroupId]: "clwg999" }, + }); + + expect(response.status).toBe(400); + expect(await readFlag(prisma, FEATURE_FLAG.defaultWorkerInstanceGroupId)).toBe( + WORKER_GROUP_ID + ); + expect(await readFlag(prisma, FEATURE_FLAG.mollifierEnabled)).toBe(true); + } + ); + + postgresTest("rejects a value the catalog refuses, writing nothing", async ({ prisma }) => { + await seed(prisma); - it("rejects a value that fails the catalog schema without writing", async () => { - replaceGlobalFeatureFlags.mockClear(); const response = await post("localhost:3030", { flags: { [FEATURE_FLAG.realtimeBackend]: "not-a-backend" }, }); expect(response.status).toBe(400); - expect(replaceGlobalFeatureFlags).not.toHaveBeenCalled(); + expect(await readFlag(prisma, FEATURE_FLAG.mollifierEnabled)).toBe(true); }); - it("submits every catalog key so omitted flags are swept", async () => { - replaceGlobalFeatureFlags.mockClear(); - await post("localhost:3030", { flags: { [FEATURE_FLAG.mollifierEnabled]: true } }); + postgresTest("upserts what was submitted and sweeps what was not", async ({ prisma }) => { + await seed(prisma); + + await post("localhost:3030", { + flags: { [FEATURE_FLAG.hasAiAccess]: true }, + unlockLockedFlags: false, + }); - const { catalogKeys, requestedFlags } = replaceGlobalFeatureFlags.mock.calls[0][1]; - expect(catalogKeys).toContain(FEATURE_FLAG.defaultWorkerInstanceGroupId); - expect(requestedFlags).toEqual({ [FEATURE_FLAG.mollifierEnabled]: true }); + expect(await readFlag(prisma, FEATURE_FLAG.hasAiAccess)).toBe(true); + expect(await readFlag(prisma, FEATURE_FLAG.mollifierEnabled)).toBeUndefined(); + expect(await readFlag(prisma, FEATURE_FLAG.defaultWorkerInstanceGroupId)).toBe(WORKER_GROUP_ID); }); });