Skip to content

Commit ad9e703

Browse files
d-csclaude
andcommitted
fix(webapp,run-store): make run-ops sharding failures visible and unroutable ids a 404
Four QA findings from the N-way run-ops sharding pass: - A replication source whose publication carries no tables replicated nothing while boot passed and the service looked healthy. The client now emits a typed PublicationMisconfiguredError and the webapp counts it per source (runs_replication_publication_misconfigured_total), so it is alarmable. - POST /api/v1/waitpoints/tokens/:id/complete (and the HTTP-callback route) answered 500 for an id naming an unconfigured shard; both now answer 404 like the run routes. - An unparseable runOpsMintShardSet still degrades to gen-1 minting, but the parse failure is now reported through the caller's logger instead of vanishing. - Per-shard observability: runops_shard_routed_total{shard} from the router and runops_read_through_source_total{source,shard} from the read-through layer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent fe94700 commit ad9e703

24 files changed

Lines changed: 699 additions & 22 deletions

apps/webapp/app/routes/api.v1.waitpoints.tokens.$waitpointFriendlyId.callback.$hash.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import { env } from "~/env.server";
66
import { processWaitpointCompletionPacket } from "~/runEngine/concerns/waitpointCompletionPacket.server";
77
import { verifyHttpCallbackHash } from "~/services/httpCallback.server";
88
import { logger } from "~/services/logger.server";
9+
import { unroutableIdResponse } from "~/services/routeBuilders/unroutableId.server";
910
import { controlPlaneResolver } from "~/v3/runOpsMigration/controlPlaneResolver.server";
1011
import { engine } from "~/v3/runEngine.server";
1112
import { runStore } from "~/v3/runStore.server";
@@ -102,6 +103,12 @@ export async function action({ request, params }: ActionFunctionArgs) {
102103
{ status: 200 }
103104
);
104105
} catch (error) {
106+
// Same as the complete route: the waitpoint id comes off the URL, so an unconfigured shard
107+
// key is caller-supplied input and must answer 404 rather than 500. This route is a bare
108+
// Remix action, so the api-builder boundary never sees the error — answer it here.
109+
const unroutable = unroutableIdResponse(error);
110+
if (unroutable) return unroutable;
111+
105112
logger.error("Failed to complete HTTP callback", { error });
106113
throw json({ error: "Failed to complete HTTP callback" }, { status: 500 });
107114
}

apps/webapp/app/routes/api.v1.waitpoints.tokens.$waitpointFriendlyId.complete.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import { env } from "~/env.server";
1010
import { logger } from "~/services/logger.server";
1111
import { processWaitpointCompletionPacket } from "~/runEngine/concerns/waitpointCompletionPacket.server";
1212
import { createActionApiRoute } from "~/services/routeBuilders/apiBuilder.server";
13+
import { unroutableIdResponse } from "~/services/routeBuilders/unroutableId.server";
1314
import { engine } from "~/v3/runEngine.server";
1415
import { runStore } from "~/v3/runStore.server";
1516

@@ -87,6 +88,11 @@ const { action, loader } = createActionApiRoute(
8788
// client gets the correct status code instead of a 500, and we don't log them as errors.
8889
if (error instanceof Response) throw error;
8990

91+
// A caller-supplied id naming a shard this topology has no store for cannot be routed,
92+
// so it is a 404 like an absent token — not the 500 this catch would otherwise answer.
93+
const unroutable = unroutableIdResponse(error);
94+
if (unroutable) throw unroutable;
95+
9096
logger.error("Failed to complete waitpoint token", {
9197
error:
9298
error instanceof Error

apps/webapp/app/services/runsReplicationInstance.server.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import {
99
setRunsReplicationConfiguredSources,
1010
setRunsReplicationGlobal,
1111
} from "./runsReplicationGlobal.server";
12+
import { runsReplicationSourceMetrics } from "./runsReplicationMetrics.server";
1213
import {
1314
RunsReplicationService,
1415
type RunsReplicationSource,
@@ -249,6 +250,9 @@ function initializeRunsReplicationInstance() {
249250
insertStrategy: env.RUN_REPLICATION_INSERT_STRATEGY,
250251
disablePayloadInsert: env.RUN_REPLICATION_DISABLE_PAYLOAD_INSERT === "1",
251252
disableErrorFingerprinting: env.RUN_REPLICATION_DISABLE_ERROR_FINGERPRINTING === "1",
253+
// A source whose publication carries no usable table logs every 30s and replicates nothing.
254+
// Boot cannot see it (the source IS configured), so the counter is the alarmable signal.
255+
onSourceError: runsReplicationSourceMetrics.recordSourceError,
252256
};
253257

254258
// Construct the SINGLE legacy source synchronously (the split gate has not resolved
Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
/**
2+
* A replication source whose publication carries no usable table replicates nothing while the
3+
* service stays up and healthy: boot passes, `assertReplicationCoversSplit` only checks that a
4+
* source is CONFIGURED, and the client's retry loop logs every 30s. Counting it is what makes it
5+
* alarmable — a non-zero rate here means that source's runs are not reaching ClickHouse.
6+
*/
7+
import { PublicationMisconfiguredError } from "@internal/replication";
8+
import { Counter, type Registry, type RegistryContentType } from "prom-client";
9+
import { metricsRegister } from "~/metrics.server";
10+
import { singleton } from "~/utils/singleton";
11+
12+
export type RunsReplicationSourceMetrics = {
13+
recordSourceError(info: { sourceId: string; error: unknown }): void;
14+
};
15+
16+
export function buildRunsReplicationSourceMetrics(
17+
register: Registry<RegistryContentType>
18+
): RunsReplicationSourceMetrics {
19+
const publicationMisconfigured = new Counter({
20+
name: "runs_replication_publication_misconfigured_total",
21+
help: "A replication source's publication does not carry the replicated table, so that source replicates nothing.",
22+
labelNames: ["source"],
23+
registers: [register],
24+
});
25+
26+
return {
27+
recordSourceError: ({ sourceId, error }) => {
28+
if (error instanceof PublicationMisconfiguredError) {
29+
publicationMisconfigured.inc({ source: sourceId });
30+
}
31+
},
32+
};
33+
}
34+
35+
// singleton: module-scope Counter registration double-registers under dev HMR.
36+
export const runsReplicationSourceMetrics = singleton("runsReplicationSourceMetrics", () =>
37+
buildRunsReplicationSourceMetrics(metricsRegister)
38+
);

apps/webapp/app/services/runsReplicationService.server.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,12 @@ export type RunsReplicationServiceOptions = {
116116
disablePayloadInsert?: boolean;
117117
disableErrorFingerprinting?: boolean;
118118
maxPoisonStripsPerBatch?: number;
119+
/**
120+
* Per-source client error hook. A client error does not stop the service — a misconfigured
121+
* publication just replicates nothing while the retry loop logs — so the owner needs a seam to
122+
* count it on.
123+
*/
124+
onSourceError?: (info: { sourceId: string; error: unknown }) => void;
119125
};
120126

121127
type PostgresTaskRun = TaskRun & { masterQueue: string };
@@ -470,6 +476,7 @@ export class RunsReplicationService {
470476
sourceId: source.id,
471477
error,
472478
});
479+
this.options.onSourceError?.({ sourceId: source.id, error });
473480
});
474481

475482
client.events.on("start", () => {

apps/webapp/app/v3/runOpsMigration/mintShardAssignment.test.ts

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -314,6 +314,39 @@ describe("resolveMintShardWith — cache, read failure and fail-safe", () => {
314314
expect(["a", "b"]).toContain(await resolveMintShardWith({ id: "env_1" }, deps));
315315
});
316316

317+
it("reports an unparseable stored list while still degrading to gen-1", async () => {
318+
// The observed failure: `runOpsMintShardSet` saved as "A,B" (uppercase) reverted the whole
319+
// fleet to gen-1 minting with ZERO log lines, because the parse throw was swallowed and
320+
// `onReadFailed` never fires for it — the read succeeded. The degrade is correct; silence is not.
321+
const deps = wrapperDeps({ readFlags: async () => ({ runOpsMintShardSet: "A,B" }) });
322+
const readFailures: unknown[] = [];
323+
const parseFailures: Array<{ key: string; value: string }> = [];
324+
deps.onReadFailed = (error) => readFailures.push(error);
325+
deps.onSetParseFailed = ({ key, value }) => parseFailures.push({ key, value });
326+
327+
expect(await resolveMintShardWith({ id: "env_1" }, deps)).toBe("new");
328+
expect(readFailures).toEqual([]);
329+
expect(parseFailures).toEqual([{ key: "runOpsMintShardSet", value: "A,B" }]);
330+
});
331+
332+
it("reports a reserved key in the stored list too", async () => {
333+
const deps = wrapperDeps({ readFlags: async () => ({ runOpsMintShardSet: "a,legacy" }) });
334+
const parseFailures: Array<{ key: string; value: string }> = [];
335+
deps.onSetParseFailed = ({ key, value }) => parseFailures.push({ key, value });
336+
337+
expect(await resolveMintShardWith({ id: "env_1" }, deps)).toBe("new");
338+
expect(parseFailures).toEqual([{ key: "runOpsMintShardSet", value: "a,legacy" }]);
339+
});
340+
341+
it("stays silent for a stored list that parses", async () => {
342+
const deps = wrapperDeps();
343+
const parseFailures: unknown[] = [];
344+
deps.onSetParseFailed = (failure) => parseFailures.push(failure);
345+
346+
expect(["a", "b"]).toContain(await resolveMintShardWith({ id: "env_1" }, deps));
347+
expect(parseFailures).toEqual([]);
348+
});
349+
317350
it("returns gen-1 when the stored list is empty", async () => {
318351
const deps = wrapperDeps({ readFlags: async () => ({ runOpsMintShardSet: "" }) });
319352
expect(await resolveMintShardWith({ id: "env_1" }, deps)).toBe("new");

apps/webapp/app/v3/runOpsMigration/mintShardAssignment.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import {
88
GEN_1_PIN_VALUE,
99
isValidPinValue,
1010
readMintShardSetResolution,
11+
type MintShardSetParseFailure,
1112
type MintShardSetResolution,
1213
} from "./mintShardGrace";
1314

@@ -166,6 +167,9 @@ export type ResolveMintShardDeps = {
166167
onPinRejected?: (info: { environmentId: string; pin: string; activeSet: string[] }) => void;
167168
onOverrideRejected?: (info: { override: string; activeSet: string[] }) => void;
168169
onReadFailed?: (error: unknown) => void;
170+
// A stored set that PARSED badly, which is not a read failure: onReadFailed never fires for it,
171+
// yet the fleet reverts to gen-1 minting. Reported here so the operator sees the degrade.
172+
onSetParseFailed?: (failure: MintShardSetParseFailure) => void;
169173
};
170174

171175
// The live list is org-independent, so one process-wide entry serves every mint: one query per
@@ -180,7 +184,7 @@ async function refreshConfig(deps: ResolveMintShardDeps): Promise<GlobalShardCon
180184
try {
181185
const flags = await deps.readFlags();
182186
const config: GlobalShardConfig = {
183-
resolution: readMintShardSetResolution(flags),
187+
resolution: readMintShardSetResolution(flags, deps.onSetParseFailed),
184188
override: flags[FEATURE_FLAG.runOpsMintShardOverride],
185189
};
186190
deps.cache.current = { value: config, expiresAt: deps.nowMs + deps.ttlMs };

apps/webapp/app/v3/runOpsMigration/mintShardGrace.ts

Lines changed: 23 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -67,15 +67,27 @@ export function effectiveMintShardSet(
6767
return nowMs < r.flippedAtMs + graceMs ? r.prevSet : r.set;
6868
}
6969

70+
/** Reports a stored value that could not be parsed. The module stays pure; the caller logs. */
71+
export type MintShardSetParseFailure = { key: string; value: string; error: unknown };
72+
7073
// The active set lives in the control-plane database, not in the environment. A deploy rolls
7174
// for hours, so two pods can hold different environment values at the same time; only a shared
7275
// row lets every pod agree on one set. Boot may reject a bad environment value, but the mint
7376
// path must never throw on a bad stored value, so an unreadable list degrades to empty.
74-
function readStoredCsv(value: unknown): string[] {
77+
//
78+
// Degrading is right; degrading SILENTLY is not. An unparseable value (a `"A,B"` typed in
79+
// uppercase) reverts the whole fleet to gen-1 minting, so the failure is reported to the caller,
80+
// which owns the logger. The reserved-key and alphabet rejections are the same failure.
81+
function readStoredCsv(
82+
value: unknown,
83+
key: string,
84+
onInvalid?: (failure: MintShardSetParseFailure) => void
85+
): string[] {
7586
if (typeof value !== "string") return [];
7687
try {
7788
return parseShardCsv(value);
78-
} catch {
89+
} catch (error) {
90+
onInvalid?.({ key, value, error });
7991
return [];
8092
}
8193
}
@@ -84,16 +96,20 @@ function readStoredCsv(value: unknown): string[] {
8496
// timestamp can never apply, so it is dropped. A timestamp with an EMPTY prevSet is meaningful:
8597
// it graces a first activation, serving no shards for the window.
8698
export function readMintShardSetResolution(
87-
flags: Record<string, unknown> | null | undefined
99+
flags: Record<string, unknown> | null | undefined,
100+
onInvalid?: (failure: MintShardSetParseFailure) => void
88101
): MintShardSetResolution {
89102
const source = flags ?? {};
90103
const flippedAtRaw = source[SET_FLIPPED_AT_KEY];
91104
const parsed = typeof flippedAtRaw === "string" ? Date.parse(flippedAtRaw) : NaN;
92105
const flippedAtMs = Number.isNaN(parsed) ? undefined : parsed;
93106

94107
return {
95-
set: readStoredCsv(source[SET_KEY]),
96-
prevSet: flippedAtMs === undefined ? undefined : readStoredCsv(source[SET_PREV_KEY]),
108+
set: readStoredCsv(source[SET_KEY], SET_KEY, onInvalid),
109+
prevSet:
110+
flippedAtMs === undefined
111+
? undefined
112+
: readStoredCsv(source[SET_PREV_KEY], SET_PREV_KEY, onInvalid),
97113
flippedAtMs,
98114
};
99115
}
@@ -113,8 +129,8 @@ export function stampMintShardSetFlip(
113129
}
114130

115131
const existing = existingFlags ?? {};
116-
const outgoingSet = readStoredCsv(outgoingFlags[SET_KEY]);
117-
const storedSet = readStoredCsv(existing[SET_KEY]);
132+
const outgoingSet = readStoredCsv(outgoingFlags[SET_KEY], SET_KEY);
133+
const storedSet = readStoredCsv(existing[SET_KEY], SET_KEY);
118134

119135
if (outgoingSet.join(",") !== storedSet.join(",")) {
120136
const effective = effectiveMintShardSet(readMintShardSetResolution(existing), nowMs, graceMs);

apps/webapp/app/v3/runOpsMigration/readThrough.server.test.ts

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -309,4 +309,70 @@ describe("readThroughRun (legacy replica + new DB)", () => {
309309
expect(throwingLegacy).not.toHaveBeenCalled();
310310
}
311311
);
312+
// Which store served a read was a return value only — never emitted — so during a cohort ramp
313+
// there was no way to see from outside the process where reads were landing.
314+
heteroPostgresTest(
315+
"emits the serving source for a gen-2 shard, the gen-1 new store and the legacy replica",
316+
async ({ prisma14, prisma17 }) => {
317+
const emitted: string[] = [];
318+
const deps = {
319+
splitEnabled: true,
320+
newClient: prisma17 as unknown as PrismaReplicaClient,
321+
legacyReplica: prisma14 as unknown as PrismaReplicaClient,
322+
shardReplicas: new Map([["a", prisma17 as unknown as PrismaReplicaClient]]),
323+
onSource: (source: string) => emitted.push(source),
324+
};
325+
326+
await readThroughRun({
327+
id: SHARD_A_RUN_ID,
328+
idKind: "run",
329+
environmentId: "env_1",
330+
readNew: (c) => realRead(c, true),
331+
readLegacy: (c) => realRead(c, false),
332+
deps,
333+
});
334+
await readThroughRun({
335+
id: NEW_RUN_ID,
336+
idKind: "run",
337+
environmentId: "env_1",
338+
readNew: (c) => realRead(c, true),
339+
readLegacy: (c) => realRead(c, false),
340+
deps,
341+
});
342+
await readThroughRun({
343+
id: LEGACY_RUN_ID,
344+
idKind: "run",
345+
environmentId: "env_1",
346+
readNew: (c) => realRead(c, false),
347+
readLegacy: (c) => realRead(c, true),
348+
deps,
349+
});
350+
351+
expect(emitted).toEqual(["shard:a", "new", "legacy-replica"]);
352+
}
353+
);
354+
355+
heteroPostgresTest(
356+
"emits nothing for a miss, so a not-found cannot look like a hit",
357+
async ({ prisma14, prisma17 }) => {
358+
const emitted: string[] = [];
359+
360+
const result = await readThroughRun({
361+
id: LEGACY_RUN_ID,
362+
idKind: "run",
363+
environmentId: "env_1",
364+
readNew: (c) => realRead(c, false),
365+
readLegacy: (c) => realRead(c, false),
366+
deps: {
367+
splitEnabled: true,
368+
newClient: prisma17 as unknown as PrismaReplicaClient,
369+
legacyReplica: prisma14 as unknown as PrismaReplicaClient,
370+
onSource: (source: string) => emitted.push(source),
371+
},
372+
});
373+
374+
expect(result.found).toBe(false);
375+
expect(emitted).toEqual([]);
376+
}
377+
);
312378
});

0 commit comments

Comments
 (0)