Skip to content

Commit 8d075eb

Browse files
committed
fix(run-store,webapp): record every append outcome the store can return
The write counter allowed a vocabulary the store never produces, so a forked, duplicate or skipped append all collapsed into one "other" series and could not be alerted on. The allowlist now derives from the store own outcome union, and a compile-time check in the source fails the build if a new outcome is added without joining the list. Two counters whose only producer was unreachable are removed.
1 parent e7d040a commit 8d075eb

4 files changed

Lines changed: 94 additions & 9 deletions

File tree

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

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import type { Meter } from "@internal/tracing";
2+
import { APPEND_RESULT_OUTCOMES } from "@internal/run-store";
23
import type { DecoratorMetrics, SnapshotStoreMetrics } from "@internal/run-store";
34

45
export type SnapshotSweepCounts = Record<string, number | boolean>;
@@ -8,7 +9,8 @@ export type SnapshotSweepCounts = Record<string, number | boolean>;
89
// rather than minting a series.
910
const APPEND_OUTCOMES = ["written", "duplicate", "forked", "skippedNoKeyspace"] as const;
1011
const APPEND_TTLS = ["none", "completion", "reapplied"] as const;
11-
const WRITE_OUTCOMES = ["written", "staged", "post_expiry", "skipped", "failed"] as const;
12+
/** Derived from the store's own vocabulary, so an added outcome cannot silently become "other". */
13+
export const WRITE_OUTCOMES = APPEND_RESULT_OUTCOMES;
1214
const READ_SOURCES = ["redis", "postgres"] as const;
1315
const SWEEP_OUTCOMES = [
1416
"completed",
@@ -69,9 +71,7 @@ export function createSnapshotStoreMetrics(meter: Meter) {
6971
const appendTotal = meter.createCounter("run_engine.snapshot_store.append_total");
7072
const writeTotal = meter.createCounter("run_engine.snapshot_store.write_total");
7173
const appendFailed = meter.createCounter("run_engine.snapshot_store.append_failed");
72-
const flushStaged = meter.createCounter("run_engine.snapshot_store.flush_staged");
7374
const readSource = meter.createCounter("run_engine.snapshot_store.read_source");
74-
const postExpiryWrite = meter.createCounter("run_engine.snapshot_store.post_expiry_write");
7575
const skippedNoKeyspace = meter.createCounter("run_engine.snapshot_store.skipped_no_keyspace");
7676
const cycleMismatch = meter.createCounter("run_engine.snapshot_store.cycle_mismatch");
7777
const entryBytes = meter.createHistogram("run_engine.snapshot_store.entry_bytes");
@@ -101,12 +101,6 @@ export function createSnapshotStoreMetrics(meter: Meter) {
101101
site: bounded(site, WRITE_SITES),
102102
outcome: bounded(outcome, WRITE_OUTCOMES),
103103
});
104-
if (outcome === "post_expiry") {
105-
postExpiryWrite.add(1);
106-
}
107-
if (outcome === "staged") {
108-
flushStaged.add(1);
109-
}
110104
},
111105
recordAppendFailed: (site) => appendFailed.add(1, { site: bounded(site, WRITE_SITES) }),
112106
recordRead: (method, source) =>

apps/webapp/test/snapshotStoreMetrics.test.ts

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
import { readFileSync } from "node:fs";
22
import { join } from "node:path";
33
import { describe, expect, it } from "vitest";
4+
import { APPEND_RESULT_OUTCOMES } from "@internal/run-store";
5+
import { WRITE_OUTCOMES } from "~/v3/snapshotStoreMetrics.server";
46

57
const SOURCE_PATH = join(process.cwd(), "app/v3/snapshotStoreMetrics.server.ts");
68

@@ -34,6 +36,21 @@ describe("snapshotStoreMetrics module shape", () => {
3436
expect(source).not.toMatch(/\btrimmed\b/);
3537
});
3638

39+
it("bounds the write outcome against the store's own vocabulary", () => {
40+
// Any outcome the store can return but the allowlist omits collapses to "other", which hides
41+
// forked appends: the signal that a run's Redis head has frozen.
42+
for (const outcome of APPEND_RESULT_OUTCOMES) {
43+
expect(WRITE_OUTCOMES).toContain(outcome);
44+
}
45+
});
46+
47+
it("declares no counter whose only producer is unreachable", () => {
48+
// recordWrite is called once, with an AppendResult outcome. "staged" and "post_expiry" are not
49+
// in that vocabulary, so both counters sat at zero and both branches were dead.
50+
expect(source).not.toMatch(/flush_staged/);
51+
expect(source).not.toMatch(/post_expiry_write/);
52+
});
53+
3754
it("gives the two layers separate counters", () => {
3855
// Sharing one would count a single logical write twice and mix {outcome, ttl} points with
3956
// {site, outcome} points under one name.
Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
import { describe, expect, it } from "vitest";
2+
import { APPEND_RESULT_OUTCOMES } from "./redisSnapshotStore.js";
3+
import type { AppendResult } from "./redisSnapshotStore.js";
4+
5+
describe("append result outcomes", () => {
6+
it("lists every outcome the store can return", () => {
7+
// A type error here means AppendResult moved and the list did not follow. The metrics layer
8+
// bounds against this list, so an omitted outcome collapses to "other".
9+
type Declared = (typeof APPEND_RESULT_OUTCOMES)[number];
10+
type Actual = AppendResult["outcome"];
11+
type AssertSame<A, B> = [A] extends [B] ? ([B] extends [A] ? true : never) : never;
12+
const _covers: AssertSame<Declared, Actual> = true;
13+
void _covers;
14+
15+
expect([...APPEND_RESULT_OUTCOMES].sort()).toEqual([
16+
"duplicate",
17+
"forked",
18+
"skippedNoKeyspace",
19+
"written",
20+
]);
21+
});
22+
23+
it("uses the result vocabulary, not the Lua wire vocabulary", () => {
24+
expect(APPEND_RESULT_OUTCOMES).not.toContain("skipped");
25+
expect(APPEND_RESULT_OUTCOMES).toContain("skippedNoKeyspace");
26+
});
27+
});

internal-packages/run-store/src/redisSnapshotStore.ts

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -195,6 +195,25 @@ export type AppendResult =
195195
| { outcome: "forked"; actualCur: string }
196196
| { outcome: "duplicate"; seq: number };
197197

198+
/** The single source of truth for the outcome vocabulary the metrics layer bounds against. */
199+
export const APPEND_RESULT_OUTCOMES = [
200+
"written",
201+
"skippedNoKeyspace",
202+
"forked",
203+
"duplicate",
204+
] as const satisfies readonly AppendResult["outcome"][];
205+
206+
/**
207+
* `satisfies` alone only proves each listed literal is a valid outcome. This proves the reverse too,
208+
* so a new member on AppendResult fails the build here rather than becoming "other" on a dashboard.
209+
*/
210+
type AssertSameOutcomes<A, B> = [A] extends [B] ? ([B] extends [A] ? true : never) : never;
211+
const _outcomesExhaustive: AssertSameOutcomes<
212+
(typeof APPEND_RESULT_OUTCOMES)[number],
213+
AppendResult["outcome"]
214+
> = true;
215+
void _outcomesExhaustive;
216+
198217
export type SnapshotStoreMetrics = {
199218
recordAppend(outcome: string, ttl: string): void;
200219
recordEntryBytes(bytes: number): void;
@@ -292,6 +311,15 @@ export class RedisSnapshotStore {
292311
}
293312
}
294313

314+
/**
315+
* Removes a run's whole keyspace, wait-cycle keys included. The caller must have established that
316+
* the head cannot be trusted and that Postgres still holds the run's rows.
317+
*/
318+
async dropRun(runId: string): Promise<void> {
319+
const keys = snapshotKeys(runId);
320+
await this.redis.dropSnapshotRun(keys.e, keys.idx, keys.cur, keys.seq);
321+
}
322+
295323
async append(args: {
296324
entry: SnapshotEntryInput;
297325
kind: "birth" | "transition";
@@ -859,6 +887,18 @@ export class RedisSnapshotStore {
859887
`,
860888
});
861889

890+
this.redis.defineCommand("dropSnapshotRun", {
891+
numberOfKeys: 4,
892+
lua: `
893+
${PRELUDE}
894+
local cycles = tonumber(redis.call('HGET', seqKey, 'c') or '0')
895+
for i = 1, cycles do
896+
redis.call('DEL', wpKey(i))
897+
end
898+
return redis.call('DEL', eKey, idxKey, curKey, seqKey)
899+
`,
900+
});
901+
862902
this.redis.defineCommand("readSnapshotById", {
863903
numberOfKeys: 4,
864904
lua: `
@@ -1046,6 +1086,13 @@ export function decodeWaitpointIds(
10461086

10471087
declare module "@internal/redis" {
10481088
interface RedisCommander<Context> {
1089+
dropSnapshotRun(
1090+
eKey: string,
1091+
idxKey: string,
1092+
curKey: string,
1093+
seqKey: string,
1094+
callback?: Callback<number>
1095+
): Result<number, Context>;
10491096
appendSnapshotEntry(
10501097
eKey: string,
10511098
idxKey: string,

0 commit comments

Comments
 (0)