diff --git a/apps/webapp/app/components/admin/flagChangeList.ts b/apps/webapp/app/components/admin/flagChangeList.ts new file mode 100644 index 000000000..4cc1c9c6d --- /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 2ba402bf2..043cd86ff 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 0054f1314..7760df8fe 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 e63547641..3a88beb54 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 000000000..0db0e64f0 --- /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( + {} + ); + }); +});