diff --git a/apps/webapp/app/routes/admin.feature-flags.tsx b/apps/webapp/app/routes/admin.feature-flags.tsx index ef8caec4b..197800c7e 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"; @@ -38,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 }) => { @@ -87,7 +94,16 @@ export const action = dashboardAction( return json({ error: "Invalid JSON body" }, { status: 400 }); } - const payloadSchema = z.object({ flags: z.record(z.unknown()) }); + // 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 + // 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 +132,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 +202,8 @@ export default function AdminFeatureFlagsRoute() { }; const handleSave = () => { - saveFetcher.submit(JSON.stringify({ flags: values }), { + const body: SaveFlagsBody = { flags: values, unlockLockedFlags: unlocked }; + saveFetcher.submit(JSON.stringify(body), { method: "POST", encType: "application/json", }); diff --git a/apps/webapp/app/v3/featureFlags.server.ts b/apps/webapp/app/v3/featureFlags.server.ts index fdd302ede..dd1fb125b 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, validatePartialFeatureFlags, } from "~/v3/featureFlags"; import { env } from "~/env.server"; @@ -223,6 +224,50 @@ export async function applyGlobalMintKindFlip( }); } +/** + * 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) } } })] + : []), + ]); +} + /** The global flag set, with the env-var defaults this app applies. */ export async function globalFeatureFlags() { return flags({ diff --git a/apps/webapp/test/adminFeatureFlagsRouteAction.test.ts b/apps/webapp/test/adminFeatureFlagsRouteAction.test.ts new file mode 100644 index 000000000..a510fbe0f --- /dev/null +++ b/apps/webapp/test/adminFeatureFlagsRouteAction.test.ts @@ -0,0 +1,137 @@ +// 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"; + +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("~/db.server", () => ({ + get prisma() { + return db.client; + }, + boundedIn, +})); + +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`, { + method: "POST", + body: JSON.stringify(body), + headers: { "content-type": "application/json" }, + }); + 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", () => { + 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(await readFlag(prisma, FEATURE_FLAG.defaultWorkerInstanceGroupId)).toBe(WORKER_GROUP_ID); + expect(await readFlag(prisma, FEATURE_FLAG.mollifierEnabled)).toBeUndefined(); + }); + + 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(response.status).toBe(200); + expect(await readFlag(prisma, FEATURE_FLAG.defaultWorkerInstanceGroupId)).toBe(WORKER_GROUP_ID); + }); + + postgresTest("deletes the locked flag when the page unlocked it", async ({ prisma }) => { + await seed(prisma); + + await post("localhost:3030", { flags: {}, unlockLockedFlags: true }); + + expect(await readFlag(prisma, FEATURE_FLAG.defaultWorkerInstanceGroupId)).toBeUndefined(); + }); + + postgresTest( + "keeps the locked flag on managed cloud despite the unlock claim", + async ({ prisma }) => { + await seed(prisma); + + 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); + + const response = await post("localhost:3030", { + flags: { [FEATURE_FLAG.realtimeBackend]: "not-a-backend" }, + }); + + expect(response.status).toBe(400); + expect(await readFlag(prisma, FEATURE_FLAG.mollifierEnabled)).toBe(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, + }); + + 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); + }); +}); diff --git a/apps/webapp/test/globalFeatureFlagsLockedFlags.test.ts b/apps/webapp/test/globalFeatureFlagsLockedFlags.test.ts new file mode 100644 index 000000000..ce18ef800 --- /dev/null +++ b/apps/webapp/test/globalFeatureFlagsLockedFlags.test.ts @@ -0,0 +1,150 @@ +// 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); + }); + + // 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, + [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(); + }); +});