Skip to content

Commit ed310f5

Browse files
committed
fix(webapp): close a latch bypass, and bound the new configuration
The latch check took its argument optionally so as not to disturb existing callers. One admin route omitted it and therefore skipped the check entirely, which let a per-organisation override be enabled with the latch unset: those runs would be resident with their transitions skipped, and their heads would freeze. An optional safety argument disables the safety at every caller that forgets it, so it is now required and the compiler enumerates the call sites. There was exactly one. The latch is deployment-wide, like the dial and the hard stop, so an organisation save now strips it rather than reporting success for a setting nothing reads from an organisation row. Bounds on the new numeric configuration. A zero cache lifetime or size is a constructor error, a zero sweep budget truncates every pass so rule 2 can never converge, and a zero command timeout fails every command. Jitter may legitimately be zero. The cluster service pins its image by digest, as the repository requires.
1 parent 08f630b commit ed310f5

6 files changed

Lines changed: 61 additions & 17 deletions

File tree

apps/webapp/app/env.server.ts

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1325,11 +1325,15 @@ const EnvironmentSchema = z
13251325
.int()
13261326
.default(2 * 60 * 60 * 1000),
13271327
RUN_ENGINE_SNAPSHOT_STORE_GC_SWEEP_SCHEDULE: z.string().default("0 */6 * * *"),
1328-
RUN_ENGINE_SNAPSHOT_STORE_GC_SWEEP_JITTER_IN_MS: z.coerce.number().int().default(60_000),
1328+
RUN_ENGINE_SNAPSHOT_STORE_GC_SWEEP_JITTER_IN_MS: z.coerce.number().int().min(0).default(60_000),
13291329
// An existing run costs ~4 serial round trips and the orphan-marker clear cannot be batched
13301330
// (cross-slot pipelines are rejected), so a full pass is hours, not minutes. A budget that
13311331
// truncates every pass stops rule 2 converging, because it needs consecutive sightings.
1332-
RUN_ENGINE_SNAPSHOT_STORE_GC_SWEEP_BUDGET_MS: z.coerce.number().int().default(10_800_000),
1332+
RUN_ENGINE_SNAPSHOT_STORE_GC_SWEEP_BUDGET_MS: z.coerce
1333+
.number()
1334+
.int()
1335+
.positive()
1336+
.default(10_800_000),
13331337
/**
13341338
* RETIRED. The hard stop is the snapshotStoreHalt feature flag and nothing else: an environment
13351339
* variable converged over a rolling deploy rather than a flag interval, and during that window a
@@ -1341,8 +1345,16 @@ const EnvironmentSchema = z
13411345
* ignored.
13421346
*/
13431347
RUN_ENGINE_SNAPSHOT_STORE_HALT: z.string().optional(),
1344-
RUN_ENGINE_SNAPSHOT_STORE_ORG_MODE_CACHE_TTL_MS: z.coerce.number().int().default(30_000),
1345-
RUN_ENGINE_SNAPSHOT_STORE_ORG_MODE_CACHE_MAX: z.coerce.number().int().default(10_000),
1348+
RUN_ENGINE_SNAPSHOT_STORE_ORG_MODE_CACHE_TTL_MS: z.coerce
1349+
.number()
1350+
.int()
1351+
.positive()
1352+
.default(30_000),
1353+
RUN_ENGINE_SNAPSHOT_STORE_ORG_MODE_CACHE_MAX: z.coerce
1354+
.number()
1355+
.int()
1356+
.positive()
1357+
.default(10_000),
13461358
// No fallback to REDIS_*: this is a distinct durable endpoint and must be set explicitly, or
13471359
// execution state silently lands on the general-purpose cache.
13481360
RUN_ENGINE_SNAPSHOT_STORE_REDIS_HOST: z.string().optional(),
@@ -1354,7 +1366,11 @@ const EnvironmentSchema = z
13541366
// Fails an append fast rather than letting it wait on an unreachable endpoint. Postgres is
13551367
// authoritative below the final dial position, so a refused append costs a mirrored write; a
13561368
// blocked one costs the request.
1357-
RUN_ENGINE_SNAPSHOT_STORE_REDIS_COMMAND_TIMEOUT_MS: z.coerce.number().int().default(500),
1369+
RUN_ENGINE_SNAPSHOT_STORE_REDIS_COMMAND_TIMEOUT_MS: z.coerce
1370+
.number()
1371+
.int()
1372+
.positive()
1373+
.default(500),
13581374

13591375
RUN_ENGINE_DEV_PRESENCE_REDIS_HOST: z
13601376
.string()

apps/webapp/app/routes/admin.api.v2.orgs.$organizationId.feature-flags.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import { env } from "~/env.server";
66
import { prisma } from "~/db.server";
77
import { requireUser } from "~/services/session.server";
88
import { controlPlaneResolver } from "~/v3/runOpsMigration/controlPlaneResolver.server";
9+
import { globalFlagsRegistry } from "~/v3/globalFlagsRegistry.server";
910
import { snapshotStoreFlagSaveError } from "~/v3/snapshotStoreFlagGuard.server";
1011
import { invalidateSnapshotStoreOrgMode } from "~/v3/snapshotStoreMode.server";
1112
import { selectMintBaselineSource, stampMintKindFlip } from "~/v3/runOpsMigration/mintFlipGrace";
@@ -149,6 +150,9 @@ export async function action({ request, params }: ActionFunctionArgs) {
149150

150151
const snapshotStoreError = snapshotStoreFlagSaveError(requestedFlags, {
151152
redisHostConfigured: !!env.RUN_ENGINE_SNAPSHOT_STORE_REDIS_HOST,
153+
// Read from the live registry, not the payload: the latch must ALREADY be true before anything
154+
// can be enabled, or a run born in the gap is resident with its transitions skipped.
155+
everEnabled: globalFlagsRegistry.current()?.[FEATURE_FLAG.snapshotStoreEverEnabled] === true,
152156
});
153157
if (snapshotStoreError) {
154158
return json({ error: snapshotStoreError }, { status: 400 });

apps/webapp/app/v3/featureFlags.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -245,7 +245,13 @@ export const ORG_LOCKED_FLAGS: FeatureFlagKey[] = [
245245
* consults it, so the line is held here — the same way the mint grace stamps are stripped.
246246
*/
247247
export function withoutOrgForbiddenSnapshotKeys<T extends Record<string, unknown>>(values: T): T {
248-
const forbidden = [FEATURE_FLAG.snapshotStoreMode, FEATURE_FLAG.snapshotStoreHalt] as const;
248+
const forbidden = [
249+
FEATURE_FLAG.snapshotStoreMode,
250+
FEATURE_FLAG.snapshotStoreHalt,
251+
// Deployment-wide, like the other two. Nothing reads it from an organisation row, so accepting
252+
// it on an organisation save reports success for a setting that does nothing.
253+
FEATURE_FLAG.snapshotStoreEverEnabled,
254+
] as const;
249255
if (!forbidden.some((key) => key in values)) return values;
250256

251257
const rest = { ...values };

apps/webapp/app/v3/snapshotStoreFlagGuard.server.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,10 @@ import { FEATURE_FLAG } from "~/v3/featureFlags";
77
*/
88
export function snapshotStoreFlagSaveError(
99
requested: Record<string, unknown>,
10-
opts: { redisHostConfigured: boolean; everEnabled?: boolean }
10+
// Both REQUIRED. `everEnabled` was optional so as not to disturb existing callers, and a route
11+
// that omitted it silently skipped the latch check: an optional safety argument disables the
12+
// safety at every caller that forgets it. Required means the compiler enumerates them.
13+
opts: { redisHostConfigured: boolean; everEnabled: boolean }
1114
): string | undefined {
1215
// Both keys, because either one past `off` is equally silent without a connection, and either one
1316
// equally makes a run resident once there is one.
@@ -26,7 +29,7 @@ export function snapshotStoreFlagSaveError(
2629
// entirely while it is unset, so a run born after the dial moved but before the latch landed would
2730
// be resident with its transitions skipped, and its head would freeze while Postgres moved on.
2831
// Refusing here makes that ordering impossible to get wrong rather than merely documented.
29-
if (opts.everEnabled === false) {
32+
if (!opts.everEnabled) {
3033
for (const { key, value } of enabling) {
3134
return `Cannot set ${key} to "${String(value)}" before ${FEATURE_FLAG.snapshotStoreEverEnabled} is true. Set that flag first: until it is, transitions skip the store entirely, so a run born now would be resident with its transitions skipped and its head would freeze.`;
3235
}

apps/webapp/test/snapshotStoreFlagGuard.test.ts

Lines changed: 23 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ describe("snapshotStoreFlagSaveError", () => {
1515
expect(
1616
snapshotStoreFlagSaveError(
1717
{ snapshotStoreMode: "dual-write" },
18-
{ redisHostConfigured: false }
18+
{ redisHostConfigured: false, everEnabled: true }
1919
)
2020
).toMatch(/RUN_ENGINE_SNAPSHOT_STORE_REDIS_HOST/);
2121
});
@@ -24,32 +24,44 @@ describe("snapshotStoreFlagSaveError", () => {
2424
expect(
2525
snapshotStoreFlagSaveError(
2626
{ snapshotStoreMode: "redis-read" },
27-
{ redisHostConfigured: false }
27+
{ redisHostConfigured: false, everEnabled: true }
2828
)
2929
).toMatch(/redis-read/);
3030
});
3131

3232
it("allows a flip past off once the host is configured", () => {
3333
expect(
34-
snapshotStoreFlagSaveError({ snapshotStoreMode: "dual-write" }, { redisHostConfigured: true })
34+
snapshotStoreFlagSaveError(
35+
{ snapshotStoreMode: "dual-write" },
36+
{ redisHostConfigured: true, everEnabled: true }
37+
)
3538
).toBeUndefined();
3639
});
3740

3841
it("allows off with no host, because that is the default state", () => {
3942
expect(
40-
snapshotStoreFlagSaveError({ snapshotStoreMode: "off" }, { redisHostConfigured: false })
43+
snapshotStoreFlagSaveError(
44+
{ snapshotStoreMode: "off" },
45+
{ redisHostConfigured: false, everEnabled: true }
46+
)
4147
).toBeUndefined();
4248
});
4349

4450
it("ignores a payload that does not mention the dial", () => {
4551
expect(
46-
snapshotStoreFlagSaveError({ runOpsMintKind: "cuid" }, { redisHostConfigured: false })
52+
snapshotStoreFlagSaveError(
53+
{ runOpsMintKind: "cuid" },
54+
{ redisHostConfigured: false, everEnabled: true }
55+
)
4756
).toBeUndefined();
4857
});
4958

5059
it("ignores a non-string dial value and leaves it to schema validation", () => {
5160
expect(
52-
snapshotStoreFlagSaveError({ snapshotStoreMode: 3 }, { redisHostConfigured: false })
61+
snapshotStoreFlagSaveError(
62+
{ snapshotStoreMode: 3 },
63+
{ redisHostConfigured: false, everEnabled: true }
64+
)
5365
).toBeUndefined();
5466
});
5567

@@ -58,14 +70,17 @@ describe("snapshotStoreFlagSaveError", () => {
5870
expect(
5971
snapshotStoreFlagSaveError(
6072
{ snapshotStoreOrgMode: "dual-write" },
61-
{ redisHostConfigured: false }
73+
{ redisHostConfigured: false, everEnabled: true }
6274
)
6375
).toMatch(/RUN_ENGINE_SNAPSHOT_STORE_REDIS_HOST/);
6476
});
6577

6678
it("allows a per-organisation off with no host", () => {
6779
expect(
68-
snapshotStoreFlagSaveError({ snapshotStoreOrgMode: "off" }, { redisHostConfigured: false })
80+
snapshotStoreFlagSaveError(
81+
{ snapshotStoreOrgMode: "off" },
82+
{ redisHostConfigured: false, everEnabled: true }
83+
)
6984
).toBeUndefined();
7085
});
7186
});

docker/docker-compose.extras.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,7 @@ services:
136136
# ioredis discovers the other five nodes from that one seed.
137137
redis-cluster:
138138
container_name: ${CONTAINER_PREFIX:-}redis-cluster
139-
image: redis:7.2
139+
image: redis:7.2@sha256:74566c6910d13ae61e7ce73ebd3127438a1fe805b309b097c323142719ec8a5b
140140
restart: always
141141
volumes:
142142
- ./config/redis-cluster-init.sh:/init.sh:ro

0 commit comments

Comments
 (0)