Skip to content

Commit ccbe4e9

Browse files
d-csd-cs
committed
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.
1 parent 60d71da commit ccbe4e9

3 files changed

Lines changed: 175 additions & 38 deletions

File tree

‎apps/webapp/app/routes/admin.feature-flags.tsx‎

Lines changed: 17 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -5,17 +5,18 @@ import { json } from "@remix-run/server-runtime";
55
import { typedjson, useTypedLoaderData } from "remix-typedjson";
66
import { z } from "zod";
77
import { LockClosedIcon } from "@heroicons/react/20/solid";
8-
import { boundedIn, prisma } from "~/db.server";
8+
import { prisma } from "~/db.server";
99
import { env } from "~/env.server";
1010
import { dashboardAction, dashboardLoader } from "~/services/routeBuilders/dashboardBuilder";
1111
import {
1212
FEATURE_FLAG,
1313
GLOBAL_LOCKED_FLAGS,
14+
type FeatureFlagKey,
1415
type FlagControlType,
1516
getAllFlagControlTypes,
1617
validatePartialFeatureFlags,
1718
} from "~/v3/featureFlags";
18-
import { flags as getGlobalFlags } from "~/v3/featureFlags.server";
19+
import { flags as getGlobalFlags, replaceGlobalFeatureFlags } from "~/v3/featureFlags.server";
1920
import { featuresForRequest } from "~/features.server";
2021
import { Button } from "~/components/primitives/Buttons";
2122
import { Callout } from "~/components/primitives/Callout";
@@ -87,7 +88,13 @@ export const action = dashboardAction(
8788
return json({ error: "Invalid JSON body" }, { status: 400 });
8889
}
8990

90-
const payloadSchema = z.object({ flags: z.record(z.unknown()) });
91+
const payloadSchema = z.object({
92+
flags: z.record(z.unknown()),
93+
// The page only submits the flags it is managing, so an omitted key is ambiguous for the
94+
// locked flags: this says whether the admin unlocked them and is therefore authoritative
95+
// over them too.
96+
unlockLockedFlags: z.boolean().optional(),
97+
});
9198
const parsed = payloadSchema.safeParse(body);
9299
if (!parsed.success) {
93100
return json({ error: "Invalid payload" }, { status: 400 });
@@ -116,39 +123,12 @@ export const action = dashboardAction(
116123
);
117124
}
118125

119-
const validatedFlags = validationResult.data as Record<string, unknown>;
120-
const controlTypes = getAllFlagControlTypes();
121-
const catalogKeys = Object.keys(controlTypes);
122-
123-
const keysToDelete: string[] = [];
124-
const upsertOps: ReturnType<typeof prisma.featureFlag.upsert>[] = [];
125-
126-
for (const key of catalogKeys) {
127-
if (key in validatedFlags) {
128-
upsertOps.push(
129-
prisma.featureFlag.upsert({
130-
where: { key },
131-
create: { key, value: validatedFlags[key] as any },
132-
update: { value: validatedFlags[key] as any },
133-
})
134-
);
135-
} else {
136-
// On cloud, never delete locked flags (they're not in the payload
137-
// because the UI doesn't include them). Locally, delete everything
138-
// the user didn't include - full control.
139-
const isProtected = isManagedCloud && GLOBAL_LOCKED_FLAGS.includes(key);
140-
if (!isProtected) {
141-
keysToDelete.push(key);
142-
}
143-
}
144-
}
145-
146-
await prisma.$transaction([
147-
...upsertOps,
148-
...(keysToDelete.length > 0
149-
? [prisma.featureFlag.deleteMany({ where: { key: { in: boundedIn(keysToDelete) } } })]
150-
: []),
151-
]);
126+
await replaceGlobalFeatureFlags(prisma, {
127+
requestedFlags: validationResult.data as Record<string, unknown>,
128+
catalogKeys: Object.keys(getAllFlagControlTypes()) as FeatureFlagKey[],
129+
isManagedCloud,
130+
unlockLockedFlags: parsed.data.unlockLockedFlags ?? false,
131+
});
152132

153133
return json({ success: true });
154134
}
@@ -213,7 +193,7 @@ export default function AdminFeatureFlagsRoute() {
213193
};
214194

215195
const handleSave = () => {
216-
saveFetcher.submit(JSON.stringify({ flags: values }), {
196+
saveFetcher.submit(JSON.stringify({ flags: values, unlockLockedFlags: unlocked }), {
217197
method: "POST",
218198
encType: "application/json",
219199
});

‎apps/webapp/app/v3/featureFlags.server.ts‎

Lines changed: 46 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,12 @@
11
import { type z } from "zod";
22
import type { PrismaClient } from "@trigger.dev/database";
3-
import { prisma, type PrismaClientOrTransaction } from "~/db.server";
3+
import { boundedIn, prisma, type PrismaClientOrTransaction } from "~/db.server";
44
import {
55
FEATURE_FLAG,
66
type FeatureFlagCatalogSchema,
77
type FeatureFlagKey,
88
FeatureFlagCatalog,
9+
GLOBAL_LOCKED_FLAGS,
910
} from "~/v3/featureFlags";
1011
import { stampMintKindFlip } from "~/v3/runOpsMigration/mintFlipGrace";
1112

@@ -220,3 +221,47 @@ export async function applyGlobalMintKindFlip(
220221
return makeSetMultipleFlags(tx)(stamped);
221222
});
222223
}
224+
225+
/**
226+
* Replace-semantics write for the global admin flags page: catalog keys present in
227+
* `requestedFlags` are upserted, catalog keys absent from it are deleted.
228+
*
229+
* A locked flag absent from the payload means the page never offered it for editing, not that
230+
* the admin unset it, so it survives the sweep. Only a self-hosted page that says it unlocked
231+
* them can delete one.
232+
*/
233+
export async function replaceGlobalFeatureFlags(
234+
client: PrismaClient,
235+
params: {
236+
requestedFlags: Record<string, unknown>;
237+
catalogKeys: FeatureFlagKey[];
238+
isManagedCloud: boolean;
239+
unlockLockedFlags: boolean;
240+
}
241+
): Promise<void> {
242+
const canDeleteLocked = params.unlockLockedFlags && !params.isManagedCloud;
243+
const upsertOps: ReturnType<typeof client.featureFlag.upsert>[] = [];
244+
const keysToDelete: string[] = [];
245+
246+
for (const key of params.catalogKeys) {
247+
if (key in params.requestedFlags) {
248+
const value = params.requestedFlags[key];
249+
upsertOps.push(
250+
client.featureFlag.upsert({
251+
where: { key },
252+
create: { key, value: value as any },
253+
update: { value: value as any },
254+
})
255+
);
256+
} else if (canDeleteLocked || !GLOBAL_LOCKED_FLAGS.includes(key)) {
257+
keysToDelete.push(key);
258+
}
259+
}
260+
261+
await client.$transaction([
262+
...upsertOps,
263+
...(keysToDelete.length > 0
264+
? [client.featureFlag.deleteMany({ where: { key: { in: boundedIn(keysToDelete) } } })]
265+
: []),
266+
]);
267+
}

‎apps/webapp/test/globalFeatureFlagsLockedFlags.test.ts‎

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,112 @@
1+
// With "Unlock read-only flags" off, the page strips GLOBAL_LOCKED_FLAGS from its payload, so an
2+
// omitted locked key means "the UI never offered it", not "the admin unset it".
3+
import type { PrismaClient } from "@trigger.dev/database";
4+
import { postgresTest } from "@internal/testcontainers";
5+
import { describe, expect, vi } from "vitest";
6+
import { FEATURE_FLAG, FeatureFlagCatalog, type FeatureFlagKey } from "~/v3/featureFlags";
7+
import { makeSetMultipleFlags, replaceGlobalFeatureFlags } from "~/v3/featureFlags.server";
8+
9+
vi.setConfig({ testTimeout: 60_000 });
10+
11+
const CATALOG_KEYS = Object.keys(FeatureFlagCatalog) as FeatureFlagKey[];
12+
const WORKER_GROUP_ID = "clwg000000000000000000000";
13+
14+
async function readFlag(prisma: PrismaClient, key: FeatureFlagKey): Promise<unknown> {
15+
const row = await prisma.featureFlag.findFirst({ where: { key }, select: { value: true } });
16+
return row?.value;
17+
}
18+
19+
describe("replaceGlobalFeatureFlags — locked flags the UI never submitted", () => {
20+
postgresTest(
21+
"keeps defaultWorkerInstanceGroupId when a locked flag is absent from the payload",
22+
async ({ prisma }) => {
23+
await makeSetMultipleFlags(prisma)({
24+
[FEATURE_FLAG.defaultWorkerInstanceGroupId]: WORKER_GROUP_ID,
25+
[FEATURE_FLAG.mollifierEnabled]: true,
26+
});
27+
28+
// What the page posts when an admin unsets mollifierEnabled on a self-hosted instance.
29+
await replaceGlobalFeatureFlags(prisma, {
30+
requestedFlags: {},
31+
catalogKeys: CATALOG_KEYS,
32+
isManagedCloud: false,
33+
unlockLockedFlags: false,
34+
});
35+
36+
expect(await readFlag(prisma, FEATURE_FLAG.defaultWorkerInstanceGroupId)).toBe(
37+
WORKER_GROUP_ID
38+
);
39+
expect(await readFlag(prisma, FEATURE_FLAG.mollifierEnabled)).toBeUndefined();
40+
}
41+
);
42+
43+
postgresTest("an unlocked self-hosted page can still unset a locked flag", async ({ prisma }) => {
44+
await makeSetMultipleFlags(prisma)({
45+
[FEATURE_FLAG.defaultWorkerInstanceGroupId]: WORKER_GROUP_ID,
46+
});
47+
48+
await replaceGlobalFeatureFlags(prisma, {
49+
requestedFlags: {},
50+
catalogKeys: CATALOG_KEYS,
51+
isManagedCloud: false,
52+
unlockLockedFlags: true,
53+
});
54+
55+
expect(await readFlag(prisma, FEATURE_FLAG.defaultWorkerInstanceGroupId)).toBeUndefined();
56+
});
57+
58+
postgresTest(
59+
"managed cloud keeps locked flags even when unlocking is claimed",
60+
async ({ prisma }) => {
61+
await makeSetMultipleFlags(prisma)({
62+
[FEATURE_FLAG.defaultWorkerInstanceGroupId]: WORKER_GROUP_ID,
63+
});
64+
65+
await replaceGlobalFeatureFlags(prisma, {
66+
requestedFlags: {},
67+
catalogKeys: CATALOG_KEYS,
68+
isManagedCloud: true,
69+
unlockLockedFlags: true,
70+
});
71+
72+
expect(await readFlag(prisma, FEATURE_FLAG.defaultWorkerInstanceGroupId)).toBe(
73+
WORKER_GROUP_ID
74+
);
75+
}
76+
);
77+
78+
postgresTest("managed cloud still sweeps ordinary flags it was not sent", async ({ prisma }) => {
79+
await makeSetMultipleFlags(prisma)({
80+
[FEATURE_FLAG.defaultWorkerInstanceGroupId]: WORKER_GROUP_ID,
81+
[FEATURE_FLAG.hasAiAccess]: true,
82+
});
83+
84+
await replaceGlobalFeatureFlags(prisma, {
85+
requestedFlags: { [FEATURE_FLAG.mollifierEnabled]: true },
86+
catalogKeys: CATALOG_KEYS,
87+
isManagedCloud: true,
88+
unlockLockedFlags: false,
89+
});
90+
91+
expect(await readFlag(prisma, FEATURE_FLAG.defaultWorkerInstanceGroupId)).toBe(WORKER_GROUP_ID);
92+
expect(await readFlag(prisma, FEATURE_FLAG.hasAiAccess)).toBeUndefined();
93+
expect(await readFlag(prisma, FEATURE_FLAG.mollifierEnabled)).toBe(true);
94+
});
95+
96+
postgresTest("submitted flags are upserted and omitted ones swept", async ({ prisma }) => {
97+
await makeSetMultipleFlags(prisma)({
98+
[FEATURE_FLAG.mollifierEnabled]: true,
99+
[FEATURE_FLAG.hasAiAccess]: true,
100+
});
101+
102+
await replaceGlobalFeatureFlags(prisma, {
103+
requestedFlags: { [FEATURE_FLAG.mollifierEnabled]: false },
104+
catalogKeys: CATALOG_KEYS,
105+
isManagedCloud: false,
106+
unlockLockedFlags: false,
107+
});
108+
109+
expect(await readFlag(prisma, FEATURE_FLAG.mollifierEnabled)).toBe(false);
110+
expect(await readFlag(prisma, FEATURE_FLAG.hasAiAccess)).toBeUndefined();
111+
});
112+
});

0 commit comments

Comments
 (0)