Skip to content

Commit 653dd94

Browse files
committed
fix(run-store): fall back to Postgres when the snapshot store stops answering
Two faults under one cause. The read paths fell back on a miss and on a dangling cycle but not on an error, so a Redis that stopped answering turned an engine read into a throw once the command timed out. Postgres holds every row below redis-only, so falling back is strictly better than failing. At redis-only it still throws, because nothing else holds the rows and an empty history served as real is worse than an error. The second is the cost of the first probe for a run a process has not seen. The residency cache removes the steady-state round trip but not that one, and under a brownout it costs the whole retry budget. A per-process breaker opens after a short run of connectivity failures and refuses later calls locally, so the endpoint takes itself off the run path with no operator and no deploy. Script errors never count toward it. A wrong type or a missing script fails identically on every retry against every node, so counting those would open the circuit on a defect and stop mirroring runs that are healthy.
1 parent 64fbff7 commit 653dd94

5 files changed

Lines changed: 356 additions & 11 deletions

File tree

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,95 @@
1+
// The residency cache removes the steady-state round trip, but a process that has not yet seen a run
2+
// still probes once, and under a brownout that one probe costs the full retry budget (measured: about
3+
// 2.07s, from 4 attempts at a 500ms command timeout plus backoff). The breaker bounds that: after a
4+
// few connectivity failures the store stops trying at all, so a sick Redis takes itself off the run
5+
// path without an operator.
6+
import { describe, expect, it } from "vitest";
7+
import { CircuitBreaker, SnapshotStoreUnavailableError } from "./circuitBreaker.js";
8+
9+
const OPTS = { failureThreshold: 3, openDurationMs: 1_000 };
10+
11+
function connectivityError(message = "Command timed out"): Error {
12+
return new Error(message);
13+
}
14+
15+
describe("CircuitBreaker", () => {
16+
it("passes calls through while closed", async () => {
17+
const breaker = new CircuitBreaker(OPTS);
18+
expect(await breaker.run(async () => "ok")).toBe("ok");
19+
});
20+
21+
it("stays closed while failures are below the threshold", async () => {
22+
const breaker = new CircuitBreaker(OPTS);
23+
for (let i = 0; i < 2; i++) {
24+
await expect(breaker.run(async () => Promise.reject(connectivityError()))).rejects.toThrow();
25+
}
26+
expect(breaker.state).toBe("closed");
27+
});
28+
29+
it("opens on the threshold and then refuses without calling through", async () => {
30+
const breaker = new CircuitBreaker(OPTS);
31+
for (let i = 0; i < 3; i++) {
32+
await expect(breaker.run(async () => Promise.reject(connectivityError()))).rejects.toThrow();
33+
}
34+
expect(breaker.state).toBe("open");
35+
36+
let called = false;
37+
await expect(
38+
breaker.run(async () => {
39+
called = true;
40+
return "ok";
41+
})
42+
).rejects.toBeInstanceOf(SnapshotStoreUnavailableError);
43+
// The point of the breaker: the doomed call is not made, so it costs nothing rather than a
44+
// timeout.
45+
expect(called).toBe(false);
46+
});
47+
48+
it("a success resets the count, so intermittent failures never accumulate to an open", async () => {
49+
const breaker = new CircuitBreaker(OPTS);
50+
for (let i = 0; i < 5; i++) {
51+
await expect(breaker.run(async () => Promise.reject(connectivityError()))).rejects.toThrow();
52+
await breaker.run(async () => "ok");
53+
}
54+
expect(breaker.state).toBe("closed");
55+
});
56+
57+
it("half-opens after the open window and closes on a successful trial", async () => {
58+
const breaker = new CircuitBreaker({ failureThreshold: 3, openDurationMs: 20 });
59+
for (let i = 0; i < 3; i++) {
60+
await expect(breaker.run(async () => Promise.reject(connectivityError()))).rejects.toThrow();
61+
}
62+
expect(breaker.state).toBe("open");
63+
64+
await new Promise((resolve) => setTimeout(resolve, 30));
65+
expect(await breaker.run(async () => "recovered")).toBe("recovered");
66+
expect(breaker.state).toBe("closed");
67+
});
68+
69+
it("re-opens when the trial call fails again", async () => {
70+
const breaker = new CircuitBreaker({ failureThreshold: 3, openDurationMs: 20 });
71+
for (let i = 0; i < 3; i++) {
72+
await expect(breaker.run(async () => Promise.reject(connectivityError()))).rejects.toThrow();
73+
}
74+
await new Promise((resolve) => setTimeout(resolve, 30));
75+
await expect(breaker.run(async () => Promise.reject(connectivityError()))).rejects.toThrow();
76+
expect(breaker.state).toBe("open");
77+
});
78+
79+
it("ignores a script error, because a bug is not an outage", async () => {
80+
// WRONGTYPE and other Lua errors mean the data or the script is wrong, and every retry against
81+
// every node will fail the same way. Counting them would open the breaker on a defect and take
82+
// the mirror down for runs that are perfectly healthy.
83+
const breaker = new CircuitBreaker(OPTS);
84+
for (let i = 0; i < 10; i++) {
85+
await expect(
86+
breaker.run(async () =>
87+
Promise.reject(
88+
new Error("WRONGTYPE Operation against a key holding the wrong kind of value")
89+
)
90+
)
91+
).rejects.toThrow();
92+
}
93+
expect(breaker.state).toBe("closed");
94+
});
95+
});
Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,104 @@
1+
/**
2+
* Thrown instead of attempting a call the breaker has decided will fail. Named so callers can tell
3+
* "Redis is not answering" apart from "Redis answered and said no", which need different handling:
4+
* the first falls back, the second is a real result.
5+
*/
6+
export class SnapshotStoreUnavailableError extends Error {
7+
constructor(message = "snapshot store is unavailable (circuit open)") {
8+
super(message);
9+
this.name = "SnapshotStoreUnavailableError";
10+
}
11+
}
12+
13+
export type CircuitBreakerOptions = {
14+
/** Consecutive connectivity failures that open the circuit. */
15+
failureThreshold?: number;
16+
/** How long the circuit stays open before one trial call is allowed through. */
17+
openDurationMs?: number;
18+
/** Injectable for tests. Defaults to Date.now. */
19+
now?: () => number;
20+
};
21+
22+
export type CircuitState = "closed" | "open" | "half-open";
23+
24+
const DEFAULT_FAILURE_THRESHOLD = 3;
25+
const DEFAULT_OPEN_DURATION_MS = 10_000;
26+
27+
/**
28+
* Connectivity, not correctness.
29+
*
30+
* A Lua error means the script or the data is wrong: every retry, against every node, fails the same
31+
* way, and no amount of waiting helps. Counting those would open the circuit on a defect and stop
32+
* mirroring runs that are perfectly healthy. A timeout, a closed connection or a down cluster is the
33+
* opposite: nothing about the request is wrong, the server is simply not answering.
34+
*/
35+
function isConnectivityFailure(error: unknown): boolean {
36+
const message = error instanceof Error ? error.message : String(error);
37+
if (/WRONGTYPE|NOSCRIPT|ERR |Lua |user_script/i.test(message)) {
38+
return false;
39+
}
40+
return /timed? ?out|ETIMEDOUT|ECONNREFUSED|ECONNRESET|EPIPE|Connection is closed|Stream isn't writeable|CLUSTERDOWN|max retries|Failed to refresh slots/i.test(
41+
message
42+
);
43+
}
44+
45+
/**
46+
* Stops a process paying the retry budget over and over for a Redis that is not answering.
47+
*
48+
* The residency cache already removes the steady-state round trip; this bounds what is left. One
49+
* transition's worth of failed attempts opens the circuit, and every later call returns immediately
50+
* instead of waiting out another timeout. A sick Redis therefore takes ITSELF off the run path,
51+
* which is the property that makes the low dial positions inert under fault without an operator
52+
* setting a second control.
53+
*
54+
* Per process and per client. The sweep holds its own connection and must not trip this one: a long
55+
* scan failing is not evidence that the hot path cannot write.
56+
*/
57+
export class CircuitBreaker {
58+
readonly #failureThreshold: number;
59+
readonly #openDurationMs: number;
60+
readonly #now: () => number;
61+
#consecutiveFailures = 0;
62+
#openedAt?: number;
63+
64+
constructor(options: CircuitBreakerOptions = {}) {
65+
this.#failureThreshold = options.failureThreshold ?? DEFAULT_FAILURE_THRESHOLD;
66+
this.#openDurationMs = options.openDurationMs ?? DEFAULT_OPEN_DURATION_MS;
67+
this.#now = options.now ?? Date.now;
68+
}
69+
70+
get state(): CircuitState {
71+
if (this.#openedAt === undefined) {
72+
return "closed";
73+
}
74+
return this.#now() - this.#openedAt >= this.#openDurationMs ? "half-open" : "open";
75+
}
76+
77+
async run<T>(fn: () => Promise<T>): Promise<T> {
78+
if (this.state === "open") {
79+
throw new SnapshotStoreUnavailableError();
80+
}
81+
82+
// half-open lets exactly this one call through. It either closes the circuit or re-opens it.
83+
try {
84+
const result = await fn();
85+
this.#consecutiveFailures = 0;
86+
this.#openedAt = undefined;
87+
return result;
88+
} catch (error) {
89+
if (!isConnectivityFailure(error)) {
90+
// A success resets the counter and so does an unrelated error: only an unbroken run of
91+
// connectivity failures is evidence the endpoint is gone.
92+
this.#consecutiveFailures = 0;
93+
throw error;
94+
}
95+
96+
this.#consecutiveFailures += 1;
97+
if (this.state === "half-open" || this.#consecutiveFailures >= this.#failureThreshold) {
98+
this.#openedAt = this.#now();
99+
this.#consecutiveFailures = 0;
100+
}
101+
throw error;
102+
}
103+
}
104+
}

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

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import {
77
} from "@internal/redis";
88
import { Logger } from "@trigger.dev/core/logger";
99
import type { CompletedWaitpoint } from "@trigger.dev/core/v3/schemas";
10+
import { CircuitBreaker, type CircuitBreakerOptions } from "./circuitBreaker.js";
1011
import { ResidencyCache } from "./residencyCache.js";
1112

1213
export type SnapshotKeys = { e: string; idx: string; cur: string; seq: string };
@@ -242,6 +243,8 @@ export type RedisSnapshotStoreOptions = RedisSnapshotStoreConnection & {
242243
completedTtlMs: number;
243244
/** Entries in the per-process residency cache. See {@link ResidencyCache}. */
244245
residencyCacheMax?: number;
246+
/** Tuning for the per-process breaker. See {@link CircuitBreaker}. */
247+
breaker?: CircuitBreakerOptions;
245248
sinceLimit?: number;
246249
highWater?: { entryBytes?: number; cycleKeyBytes?: number; cycleCount?: number };
247250
metrics?: SnapshotStoreMetrics;
@@ -279,6 +282,12 @@ export class RedisSnapshotStore {
279282
* replies that are the cache's only ground truth.
280283
*/
281284
readonly #residency: ResidencyCache;
285+
/**
286+
* Bounds what the residency cache cannot: the first probe for a run this process has not seen,
287+
* which under a brownout costs the whole retry budget. After a few connectivity failures the store
288+
* stops calling out at all, so a sick Redis removes itself from the run path with no operator.
289+
*/
290+
readonly #breaker: CircuitBreaker;
282291
#quit?: Promise<void>;
283292

284293
constructor(options: RedisSnapshotStoreOptions) {
@@ -290,6 +299,7 @@ export class RedisSnapshotStore {
290299
this.#residency = new ResidencyCache({
291300
...(options.residencyCacheMax !== undefined && { max: options.residencyCacheMax }),
292301
});
302+
this.#breaker = new CircuitBreaker(options.breaker ?? {});
293303
this.ownsClient = options.client === undefined;
294304
this.redis =
295305
options.client ??
@@ -315,15 +325,25 @@ export class RedisSnapshotStore {
315325
await this.#quit;
316326
}
317327

328+
/**
329+
* Every command goes through here, so the breaker sits on the one seam rather than on each method.
330+
* Latency is still recorded for a refused call: a call that cost nothing because the circuit was
331+
* open is exactly the thing an operator wants to see in the latency series.
332+
*/
318333
async #timed<T>(op: string, fn: () => Promise<T>): Promise<T> {
319334
const started = Date.now();
320335
try {
321-
return await fn();
336+
return await this.#breaker.run(fn);
322337
} finally {
323338
this.metrics?.recordLatency(op, Date.now() - started);
324339
}
325340
}
326341

342+
/** Test seam. */
343+
get breakerState(): "closed" | "open" | "half-open" {
344+
return this.#breaker.state;
345+
}
346+
327347
/** Test seam. */
328348
residencyFor(runId: string): "resident" | "non-resident" | undefined {
329349
return this.#residency.get(runId);

‎internal-packages/run-store/src/taskRunExecutionSnapshotStore.hotPath.test.ts‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -184,3 +184,71 @@ describe("Redis stays off the hot path for a non-resident run", () => {
184184
}
185185
);
186186
});
187+
188+
describe("a Redis that stops answering", () => {
189+
containerTest(
190+
"serves reads from Postgres instead of throwing into the engine",
191+
async ({ prisma, redisOptions }) => {
192+
// The read paths fell back on a miss and on a dangling cycle, but not on an ERROR, so a
193+
// brownout at redis-read turned an engine read into a throw once the command timed out.
194+
const redis = new RedisSnapshotStore({ redisOptions, completedTtlMs: COMPLETED_TTL_MS });
195+
const decorated = new TaskRunExecutionSnapshotStore(
196+
new PostgresRunStore({ prisma, readOnlyPrisma: prisma }) as unknown as RunStore,
197+
{ store: redis, mode: "redis-read", readPercent: 100 }
198+
);
199+
try {
200+
const env = await seedSnapshotEnvironment(prisma);
201+
const runId = generateInternalId();
202+
await decorated.createRun({
203+
data: buildCreateRunData(runId, env),
204+
snapshot: birthSnapshot(env),
205+
});
206+
207+
// Reads work while Redis answers.
208+
expect(await decorated.findLatestExecutionSnapshot(runId)).not.toBeNull();
209+
210+
// Now take Redis away underneath it, the way a brownout does.
211+
await redis.quit();
212+
213+
const served = await decorated.findLatestExecutionSnapshot(runId);
214+
expect(served).not.toBeNull();
215+
expect(served!.runId).toBe(runId);
216+
} finally {
217+
await redis.quit();
218+
}
219+
}
220+
);
221+
222+
containerTest("stops calling out once the circuit opens", async ({ prisma, redisOptions }) => {
223+
const redis = new RedisSnapshotStore({
224+
redisOptions,
225+
completedTtlMs: COMPLETED_TTL_MS,
226+
breaker: { failureThreshold: 2, openDurationMs: 60_000 },
227+
});
228+
const decorated = new TaskRunExecutionSnapshotStore(
229+
new PostgresRunStore({ prisma, readOnlyPrisma: prisma }) as unknown as RunStore,
230+
{ store: redis, mode: "redis-read", readPercent: 100 }
231+
);
232+
try {
233+
const env = await seedSnapshotEnvironment(prisma);
234+
const runId = generateInternalId();
235+
await decorated.createRun({
236+
data: buildCreateRunData(runId, env),
237+
snapshot: birthSnapshot(env),
238+
});
239+
240+
await redis.quit();
241+
expect(redis.breakerState).toBe("closed");
242+
243+
for (let i = 0; i < 4; i++) {
244+
expect(await decorated.findLatestExecutionSnapshot(runId)).not.toBeNull();
245+
}
246+
247+
// Open, so every later call is refused locally rather than waiting out another timeout. This
248+
// is what bounds the cold-cache probe under a brownout.
249+
expect(redis.breakerState).toBe("open");
250+
} finally {
251+
await redis.quit();
252+
}
253+
});
254+
});

0 commit comments

Comments
 (0)