Skip to content

Commit 5163a65

Browse files
d-csclaude
andcommitted
refactor(mollifier): drop global FeatureFlag fallback in hot-path resolver
`triggerTask` is the highest-throughput code path in the system and the webapp CLAUDE.md forbids new DB queries there. The previous resolver fell back to `flag()` (a Prisma read against `FeatureFlag`) when the org had no `mollifierEnabled` override, which added a query to every trigger whenever `MOLLIFIER_ENABLED=1`. The fleet-wide kill switch already lives in `MOLLIFIER_ENABLED`; rollout is per-org via `Organization.featureFlags` JSON, matching `canAccessAi`/`hasComputeAccess`/etc. Drop the fallback so the resolver is purely in-memory. Tests no longer need a postgres testcontainer or `makeFlag(prisma)`; the per-org isolation suite now asserts directly on `Organization.featureFlags` shape and adds a regression test for the no-override -> false contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 650f025 commit 5163a65

2 files changed

Lines changed: 110 additions & 161 deletions

File tree

apps/webapp/app/v3/mollifier/mollifierGate.server.ts

Lines changed: 9 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
import { env } from "~/env.server";
22
import { logger } from "~/services/logger.server";
3-
import { flag } from "~/v3/featureFlags.server";
43
import { FEATURE_FLAG, FeatureFlagCatalog } from "~/v3/featureFlags";
54
import { getMollifierBuffer } from "./mollifierBuffer.server";
65
import { createRealTripEvaluator } from "./mollifierTripEvaluator.server";
@@ -102,14 +101,14 @@ function logDivertDecision(
102101
});
103102
}
104103

105-
// Check per-org override in-memory before consulting the DB. `triggerTask`
106-
// is the hot path, so we resolve the common case (org has an explicit
107-
// `mollifierEnabled` value in its `Organization.featureFlags` JSON) without
108-
// a Prisma round-trip. Only orgs with no override fall through to `flag()`,
109-
// which queries the global `FeatureFlag` row.
110-
export function makeResolveMollifierFlag(
111-
flagFn: typeof flag = flag,
112-
): (inputs: GateInputs) => Promise<boolean> {
104+
// Resolve the per-org mollifier flag purely from the in-memory
105+
// `Organization.featureFlags` JSON. No DB query — `triggerTask` is the
106+
// trigger hot path and the webapp CLAUDE.md forbids adding Prisma calls
107+
// there. The fleet-wide kill switch lives in `MOLLIFIER_ENABLED`; rollout
108+
// is per-org via the JSON, matching the pattern used by `canAccessAi`,
109+
// `hasComputeAccess`, etc. There is no global `FeatureFlag` table read
110+
// in this path by design.
111+
export function makeResolveMollifierFlag(): (inputs: GateInputs) => Promise<boolean> {
113112
return (inputs) => {
114113
const override = inputs.orgFeatureFlags?.[FEATURE_FLAG.mollifierEnabled];
115114
if (override !== undefined) {
@@ -118,10 +117,7 @@ export function makeResolveMollifierFlag(
118117
return Promise.resolve(parsed.data);
119118
}
120119
}
121-
return flagFn({
122-
key: FEATURE_FLAG.mollifierEnabled,
123-
defaultValue: false,
124-
});
120+
return Promise.resolve(false);
125121
};
126122
}
127123

apps/webapp/test/mollifierGate.test.ts

Lines changed: 101 additions & 148 deletions
Original file line numberDiff line numberDiff line change
@@ -5,15 +5,11 @@ import { describe, expect, it, vi } from "vitest";
55
// (db.server.ts), so loading it under vitest tries to reach localhost:5432
66
// and surfaces as an unhandled rejection that fails the whole shard — even
77
// though no test in this file actually uses the default prisma client.
8-
// `postgresTest` provides its own container-backed prisma via the fixture.
98
vi.mock("~/db.server", () => ({
109
prisma: {},
1110
$replica: {},
1211
}));
1312

14-
import { postgresTest } from "@internal/testcontainers";
15-
import { FEATURE_FLAG } from "~/v3/featureFlags";
16-
import { makeFlag } from "~/v3/featureFlags.server";
1713
import {
1814
evaluateGate,
1915
makeResolveMollifierFlag,
@@ -23,11 +19,6 @@ import {
2319
} from "~/v3/mollifier/mollifierGate.server";
2420
import type { DecisionOutcome, DecisionReason } from "~/v3/mollifier/mollifierTelemetry.server";
2521

26-
// Each `postgresTest` boots its own Postgres container; the 5s vitest default
27-
// regularly times out on CI just on container start. Match the timeout used by
28-
// other postgresTest suites in this app (e.g. `taskIdentifierRegistry.test.ts`).
29-
vi.setConfig({ testTimeout: 30_000 });
30-
3122
// We deliberately don't use vi.fn here. Per repo policy tests shouldn't lean on
3223
// mock frameworks for behaviours that are pure functions of the inputs — the
3324
// gate is pure decision logic, so a hand-rolled "deps + spy log" wired with
@@ -191,23 +182,13 @@ describe("evaluateGate cascade — exhaustive truth table", () => {
191182
});
192183
});
193184

194-
// The gate must opt in single orgs without affecting the others. These tests
195-
// exercise the *real* `resolveOrgFlag` against a real Postgres testcontainer:
196-
// we build it via `makeFlag(prisma)` and let the `Organization.featureFlags`
197-
// blob flow through `flag()`'s overrides path. The global `FeatureFlag` table
198-
// is empty, so the only signal moving outcomes is the per-org JSON.
199185
// Hot-path guard: `triggerTask.server.ts` calls `evaluateGate` on every
200186
// trigger when `MOLLIFIER_ENABLED=1`. The per-org override path must resolve
201187
// without a Prisma round-trip — otherwise the gate adds a DB query to the
202188
// highest-throughput code path in the system (see apps/webapp/CLAUDE.md).
203189
describe("resolveMollifierFlag — hot path", () => {
204-
it("returns override value without calling flag() when override is set", async () => {
205-
let flagCalls = 0;
206-
const flagStub: any = async () => {
207-
flagCalls += 1;
208-
return false;
209-
};
210-
const resolve = makeResolveMollifierFlag(flagStub);
190+
it("returns the per-org override when it's set", async () => {
191+
const resolve = makeResolveMollifierFlag();
211192

212193
const enabled = await resolve({
213194
envId: "e",
@@ -224,16 +205,14 @@ describe("resolveMollifierFlag — hot path", () => {
224205

225206
expect(enabled).toBe(true);
226207
expect(disabled).toBe(false);
227-
expect(flagCalls).toBe(0);
228208
});
229209

230-
it("falls back to flag() when org has no override for the key", async () => {
231-
let flagCalls = 0;
232-
const flagStub: any = async () => {
233-
flagCalls += 1;
234-
return true;
235-
};
236-
const resolve = makeResolveMollifierFlag(flagStub);
210+
it("returns false when the org has no override for the key — no DB query, ever", async () => {
211+
// Regression intent: the resolver MUST NOT call `flag()` (which would
212+
// query `FeatureFlag` via Prisma) on the trigger hot path. Per-org
213+
// rollout via `Organization.featureFlags` JSON is the only enable
214+
// path; the fleet-wide kill switch is `MOLLIFIER_ENABLED`.
215+
const resolve = makeResolveMollifierFlag();
237216

238217
const fromNull = await resolve({
239218
envId: "e",
@@ -248,9 +227,8 @@ describe("resolveMollifierFlag — hot path", () => {
248227
orgFeatureFlags: { hasAiAccess: true },
249228
});
250229

251-
expect(fromNull).toBe(true);
252-
expect(fromUnrelatedKeys).toBe(true);
253-
expect(flagCalls).toBe(2);
230+
expect(fromNull).toBe(false);
231+
expect(fromUnrelatedKeys).toBe(false);
254232
});
255233
});
256234

@@ -330,21 +308,21 @@ describe("evaluateGate — fail open on resolveOrgFlag error", () => {
330308

331309
describe("evaluateGate — per-org isolation via Organization.featureFlags", () => {
332310
function makeIsolationDeps(
333-
realResolveOrgFlag: GateDependencies["resolveOrgFlag"],
311+
resolveOrgFlag: GateDependencies["resolveOrgFlag"],
334312
): { deps: Partial<GateDependencies>; spies: Spies } {
335313
const spies: Spies = {
336314
evaluatorCalls: 0,
337315
logShadowCalls: [],
338316
logMollifiedCalls: [],
339317
recordDecisionCalls: [],
340318
};
341-
// Override lifecycle bits and inject the real DB-backed resolveOrgFlag.
319+
// Override lifecycle bits and inject the production resolveOrgFlag.
342320
// Evaluator returns a fixed tripped decision so the outcome is purely a
343321
// function of the flag resolution (which is what we're isolating on).
344322
const deps: Partial<GateDependencies> = {
345323
isMollifierEnabled: () => true,
346324
isShadowModeOn: () => false,
347-
resolveOrgFlag: realResolveOrgFlag,
325+
resolveOrgFlag,
348326
evaluator: async () => {
349327
spies.evaluatorCalls += 1;
350328
return trippedDecision;
@@ -362,120 +340,95 @@ describe("evaluateGate — per-org isolation via Organization.featureFlags", ()
362340
return { deps, spies };
363341
}
364342

365-
// Build the production resolveOrgFlag wired to the test Prisma client. This
366-
// is exactly the closure `defaultGateDependencies.resolveOrgFlag` runs in
367-
// prod — the only swap is the Prisma instance.
368-
function realResolveOrgFlag(prisma: Parameters<typeof makeFlag>[0]) {
369-
const f = makeFlag(prisma);
370-
return (inputs: GateInputs) =>
371-
f({
372-
key: FEATURE_FLAG.mollifierEnabled,
373-
defaultValue: false,
374-
overrides: inputs.orgFeatureFlags ?? {},
375-
});
376-
}
343+
// The production resolver — purely in-memory, no Prisma. Mirrors
344+
// `defaultGateDependencies.resolveOrgFlag` exactly.
345+
const resolve = makeResolveMollifierFlag();
346+
347+
it("opts in only the org whose featureFlags has mollifierEnabled=true", async () => {
348+
const orgA = { ...inputs, orgId: "org_a", orgFeatureFlags: { mollifierEnabled: true } };
349+
const orgB = { ...inputs, orgId: "org_b", orgFeatureFlags: { mollifierEnabled: false } };
350+
const orgC = { ...inputs, orgId: "org_c", orgFeatureFlags: null };
351+
352+
const a = makeIsolationDeps(resolve);
353+
const b = makeIsolationDeps(resolve);
354+
const c = makeIsolationDeps(resolve);
355+
356+
const [outcomeA, outcomeB, outcomeC] = await Promise.all([
357+
evaluateGate(orgA, a.deps),
358+
evaluateGate(orgB, b.deps),
359+
evaluateGate(orgC, c.deps),
360+
]);
361+
362+
// Only org A's flag is on → only org A mollifies. Orgs B and C never
363+
// reach the evaluator because both flag and shadow-mode are off.
364+
expect(outcomeA.action).toBe("mollify");
365+
expect(outcomeB.action).toBe("pass_through");
366+
expect(outcomeC.action).toBe("pass_through");
367+
368+
expect(a.spies.evaluatorCalls).toBe(1);
369+
expect(b.spies.evaluatorCalls).toBe(0);
370+
expect(c.spies.evaluatorCalls).toBe(0);
371+
372+
expect(a.spies.logMollifiedCalls).toHaveLength(1);
373+
expect(b.spies.logMollifiedCalls).toHaveLength(0);
374+
expect(c.spies.logMollifiedCalls).toHaveLength(0);
375+
});
377376

378-
postgresTest(
379-
"opts in only the org whose featureFlags has mollifierEnabled=true",
380-
async ({ prisma }) => {
381-
const resolve = realResolveOrgFlag(prisma);
382-
const orgA = { ...inputs, orgId: "org_a", orgFeatureFlags: { mollifierEnabled: true } };
383-
const orgB = { ...inputs, orgId: "org_b", orgFeatureFlags: { mollifierEnabled: false } };
384-
const orgC = { ...inputs, orgId: "org_c", orgFeatureFlags: null };
385-
386-
const a = makeIsolationDeps(resolve);
387-
const b = makeIsolationDeps(resolve);
388-
const c = makeIsolationDeps(resolve);
389-
390-
const [outcomeA, outcomeB, outcomeC] = await Promise.all([
391-
evaluateGate(orgA, a.deps),
392-
evaluateGate(orgB, b.deps),
393-
evaluateGate(orgC, c.deps),
394-
]);
395-
396-
// Only org A's flag is on → only org A mollifies. Orgs B and C never
397-
// reach the evaluator because both flag and shadow-mode are off.
398-
expect(outcomeA.action).toBe("mollify");
399-
expect(outcomeB.action).toBe("pass_through");
400-
expect(outcomeC.action).toBe("pass_through");
401-
402-
expect(a.spies.evaluatorCalls).toBe(1);
403-
expect(b.spies.evaluatorCalls).toBe(0);
404-
expect(c.spies.evaluatorCalls).toBe(0);
405-
406-
expect(a.spies.logMollifiedCalls).toHaveLength(1);
407-
expect(b.spies.logMollifiedCalls).toHaveLength(0);
408-
expect(c.spies.logMollifiedCalls).toHaveLength(0);
409-
},
410-
);
377+
it("another org's beta flags must not opt them into mollifier", async () => {
378+
// Org A has mollifier on (plus an unrelated beta).
379+
const orgA = {
380+
...inputs,
381+
orgId: "org_a",
382+
orgFeatureFlags: { mollifierEnabled: true, hasComputeAccess: true },
383+
};
384+
// Org B has *other* betas on but mollifier remains off — keys that gate
385+
// compute/AI/query must not bleed across into the mollifier decision.
386+
const orgB = {
387+
...inputs,
388+
orgId: "org_b",
389+
orgFeatureFlags: { hasComputeAccess: true, hasAiAccess: true },
390+
};
411391

412-
postgresTest(
413-
"another org's beta flags must not opt them into mollifier",
414-
async ({ prisma }) => {
415-
const resolve = realResolveOrgFlag(prisma);
416-
// Org A has mollifier on (plus an unrelated beta).
417-
const orgA = {
418-
...inputs,
419-
orgId: "org_a",
420-
orgFeatureFlags: { mollifierEnabled: true, hasComputeAccess: true },
421-
};
422-
// Org B has *other* betas on but mollifier remains off — keys that gate
423-
// compute/AI/query must not bleed across into the mollifier decision.
424-
const orgB = {
425-
...inputs,
426-
orgId: "org_b",
427-
orgFeatureFlags: { hasComputeAccess: true, hasAiAccess: true },
428-
};
429-
430-
const a = makeIsolationDeps(resolve);
431-
const b = makeIsolationDeps(resolve);
432-
433-
const outcomeA = await evaluateGate(orgA, a.deps);
434-
const outcomeB = await evaluateGate(orgB, b.deps);
435-
436-
expect(outcomeA.action).toBe("mollify");
437-
expect(outcomeB.action).toBe("pass_through");
438-
},
439-
);
392+
const a = makeIsolationDeps(resolve);
393+
const b = makeIsolationDeps(resolve);
440394

441-
postgresTest(
442-
"global FeatureFlag row enables only when an org's overrides don't say otherwise",
443-
async ({ prisma }) => {
444-
// Set the global flag on. The repo-wide `flag()` helper checks
445-
// overrides first, then global, then default. So:
446-
// - org with explicit `mollifierEnabled: false` → stays off.
447-
// - org with no override → picks up the global on.
448-
// - org with explicit `true` → on.
449-
await prisma.featureFlag.create({
450-
data: { key: FEATURE_FLAG.mollifierEnabled, value: true },
451-
});
452-
const resolve = realResolveOrgFlag(prisma);
453-
454-
const orgOptedOut = {
455-
...inputs,
456-
orgId: "org_opted_out",
457-
orgFeatureFlags: { mollifierEnabled: false },
458-
};
459-
const orgInherits = { ...inputs, orgId: "org_inherits", orgFeatureFlags: null };
460-
const orgExplicit = {
461-
...inputs,
462-
orgId: "org_explicit",
463-
orgFeatureFlags: { mollifierEnabled: true },
464-
};
465-
466-
const optedOut = makeIsolationDeps(resolve);
467-
const inherits = makeIsolationDeps(resolve);
468-
const explicit = makeIsolationDeps(resolve);
469-
470-
const [outOptedOut, outInherits, outExplicit] = await Promise.all([
471-
evaluateGate(orgOptedOut, optedOut.deps),
472-
evaluateGate(orgInherits, inherits.deps),
473-
evaluateGate(orgExplicit, explicit.deps),
474-
]);
475-
476-
expect(outOptedOut.action).toBe("pass_through");
477-
expect(outInherits.action).toBe("mollify");
478-
expect(outExplicit.action).toBe("mollify");
479-
},
480-
);
395+
const outcomeA = await evaluateGate(orgA, a.deps);
396+
const outcomeB = await evaluateGate(orgB, b.deps);
397+
398+
expect(outcomeA.action).toBe("mollify");
399+
expect(outcomeB.action).toBe("pass_through");
400+
});
401+
402+
it("orgs without an explicit override stay off — no global FeatureFlag fallback", async () => {
403+
// Regression intent: the resolver MUST NOT consult the global
404+
// `FeatureFlag` table on the hot path. An org with `orgFeatureFlags`
405+
// unset (the default for almost every org during rollout) gets
406+
// pass_through, period. The fleet-wide kill switch lives in
407+
// `MOLLIFIER_ENABLED`, not the FeatureFlag table.
408+
const orgInherits = { ...inputs, orgId: "org_inherits", orgFeatureFlags: null };
409+
const orgEmpty = { ...inputs, orgId: "org_empty", orgFeatureFlags: {} };
410+
const orgUnrelated = {
411+
...inputs,
412+
orgId: "org_unrelated",
413+
orgFeatureFlags: { hasAiAccess: true },
414+
};
415+
416+
const inheritsDeps = makeIsolationDeps(resolve);
417+
const emptyDeps = makeIsolationDeps(resolve);
418+
const unrelatedDeps = makeIsolationDeps(resolve);
419+
420+
const [outInherits, outEmpty, outUnrelated] = await Promise.all([
421+
evaluateGate(orgInherits, inheritsDeps.deps),
422+
evaluateGate(orgEmpty, emptyDeps.deps),
423+
evaluateGate(orgUnrelated, unrelatedDeps.deps),
424+
]);
425+
426+
expect(outInherits.action).toBe("pass_through");
427+
expect(outEmpty.action).toBe("pass_through");
428+
expect(outUnrelated.action).toBe("pass_through");
429+
// None of these reached the evaluator (flag off, shadow off).
430+
expect(inheritsDeps.spies.evaluatorCalls).toBe(0);
431+
expect(emptyDeps.spies.evaluatorCalls).toBe(0);
432+
expect(unrelatedDeps.spies.evaluatorCalls).toBe(0);
433+
});
481434
});

0 commit comments

Comments
 (0)