Skip to content

Commit 40dd4c7

Browse files
849261680849261680
committed
fix(cron): persist failure alert delivery status
1 parent 0a1ce14 commit 40dd4c7

7 files changed

Lines changed: 347 additions & 76 deletions

File tree

‎src/cron/service.persists-delivered-status.test.ts‎

Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,17 @@ function buildFailureDestinationOnlyJob(name: string): CronAddInput {
6262
};
6363
}
6464

65+
function buildFailureAlertOnlyJob(name: string): CronAddInput {
66+
return {
67+
...buildIsolatedAgentTurnJob(name),
68+
failureAlert: {
69+
after: 1,
70+
channel: "forum",
71+
to: "123",
72+
},
73+
};
74+
}
75+
6576
function buildBestEffortFailureDestinationOnlyJob(name: string): CronAddInput {
6677
return {
6778
...buildFailureDestinationOnlyJob(name),
@@ -92,6 +103,7 @@ function createIsolatedCronWithFinishedBarrier(params: {
92103
status?: "ok" | "error";
93104
delivered?: boolean;
94105
error?: string;
106+
sendCronFailureAlert?: ConstructorParameters<typeof CronService>[0]["sendCronFailureAlert"];
95107
onFinished?: (evt: {
96108
jobId: string;
97109
delivered?: boolean;
@@ -116,6 +128,9 @@ function createIsolatedCronWithFinishedBarrier(params: {
116128
...(params.error === undefined ? {} : { error: params.error }),
117129
...(params.delivered === undefined ? {} : { delivered: params.delivered }),
118130
})),
131+
...(params.sendCronFailureAlert === undefined
132+
? {}
133+
: { sendCronFailureAlert: params.sendCronFailureAlert }),
119134
onEvent: (evt) => {
120135
if (evt.action === "finished") {
121136
params.onFinished?.({
@@ -190,6 +205,7 @@ async function runIsolatedJobAndReadState(params: {
190205
status?: "ok" | "error";
191206
delivered?: boolean;
192207
error?: string;
208+
sendCronFailureAlert?: ConstructorParameters<typeof CronService>[0]["sendCronFailureAlert"];
193209
onFinished?: (evt: {
194210
jobId: string;
195211
delivered?: boolean;
@@ -208,6 +224,9 @@ async function runIsolatedJobAndReadState(params: {
208224
...(params.status !== undefined ? { status: params.status } : {}),
209225
...(params.delivered !== undefined ? { delivered: params.delivered } : {}),
210226
...(params.error !== undefined ? { error: params.error } : {}),
227+
...(params.sendCronFailureAlert !== undefined
228+
? { sendCronFailureAlert: params.sendCronFailureAlert }
229+
: {}),
211230
onFinished: (evt) => {
212231
params.onFinished?.(evt);
213232
finishedEvents.get(evt.jobId)?.(evt);
@@ -326,6 +345,102 @@ describe("CronService persists delivered status", () => {
326345
expect(capturedEvent?.failureNotificationDelivery).toEqual({ status: "unknown" });
327346
});
328347

348+
it("persists delivered status for requested per-job failureAlert", async () => {
349+
const sendCronFailureAlert = vi.fn(async () => undefined);
350+
let capturedEvent:
351+
| {
352+
delivered?: boolean;
353+
deliveryStatus?: string;
354+
failureNotificationDelivery?: {
355+
delivered?: boolean;
356+
status: string;
357+
error?: string;
358+
};
359+
}
360+
| undefined;
361+
const updated = await runIsolatedJobAndReadState({
362+
job: buildFailureAlertOnlyJob("failure-alert-only"),
363+
status: "error",
364+
error: "Agent couldn't generate a response.",
365+
sendCronFailureAlert,
366+
onFinished: (evt) => {
367+
capturedEvent = evt;
368+
},
369+
});
370+
371+
expect(sendCronFailureAlert).toHaveBeenCalledTimes(1);
372+
expect(updated?.state.lastRunStatus).toBe("error");
373+
expect(updated?.state.lastDeliveryStatus).toBe("not-requested");
374+
expect(updated?.state.lastFailureNotificationDelivered).toBe(true);
375+
expect(updated?.state.lastFailureNotificationDeliveryStatus).toBe("delivered");
376+
expect(updated?.state.lastFailureNotificationDeliveryError).toBeUndefined();
377+
expect(capturedEvent?.deliveryStatus).toBe("not-requested");
378+
expect(capturedEvent?.failureNotificationDelivery).toEqual({
379+
delivered: true,
380+
status: "delivered",
381+
});
382+
});
383+
384+
it("persists not-delivered status when per-job failureAlert send fails", async () => {
385+
const sendCronFailureAlert = vi.fn(async () => {
386+
throw new Error("channel offline");
387+
});
388+
let capturedEvent:
389+
| {
390+
failureNotificationDelivery?: {
391+
delivered?: boolean;
392+
status: string;
393+
error?: string;
394+
};
395+
}
396+
| undefined;
397+
const updated = await runIsolatedJobAndReadState({
398+
job: buildFailureAlertOnlyJob("failure-alert-send-fails"),
399+
status: "error",
400+
error: "Agent couldn't generate a response.",
401+
sendCronFailureAlert,
402+
onFinished: (evt) => {
403+
capturedEvent = evt;
404+
},
405+
});
406+
407+
expect(sendCronFailureAlert).toHaveBeenCalledTimes(1);
408+
expect(updated?.state.lastRunStatus).toBe("error");
409+
expect(updated?.state.lastDeliveryStatus).toBe("not-requested");
410+
expect(updated?.state.lastFailureNotificationDelivered).toBe(false);
411+
expect(updated?.state.lastFailureNotificationDeliveryStatus).toBe("not-delivered");
412+
expect(updated?.state.lastFailureNotificationDeliveryError).toBe("Error: channel offline");
413+
expect(capturedEvent?.failureNotificationDelivery).toEqual({
414+
delivered: false,
415+
status: "not-delivered",
416+
error: "Error: channel offline",
417+
});
418+
});
419+
420+
it("persists unknown status when per-job failureAlert send times out", async () => {
421+
const sendCronFailureAlert = vi.fn(() => new Promise<never>(() => {}));
422+
const updatedPromise = runIsolatedJobAndReadState({
423+
job: buildFailureAlertOnlyJob("failure-alert-send-times-out"),
424+
status: "error",
425+
error: "Agent couldn't generate a response.",
426+
sendCronFailureAlert,
427+
});
428+
429+
await vi.runOnlyPendingTimersAsync();
430+
await vi.waitFor(() => expect(sendCronFailureAlert).toHaveBeenCalledTimes(1));
431+
await vi.advanceTimersByTimeAsync(10_000);
432+
433+
const updated = await updatedPromise;
434+
expect(sendCronFailureAlert).toHaveBeenCalledTimes(1);
435+
expect(updated?.state.lastRunStatus).toBe("error");
436+
expect(updated?.state.lastDeliveryStatus).toBe("not-requested");
437+
expect(updated?.state.lastFailureNotificationDelivered).toBeUndefined();
438+
expect(updated?.state.lastFailureNotificationDeliveryStatus).toBe("unknown");
439+
expect(updated?.state.lastFailureNotificationDeliveryError).toBe(
440+
"failure alert delivery timed out after 10000ms",
441+
);
442+
});
443+
329444
it("does not treat primary error delivery as alternate failure-destination delivery", async () => {
330445
let capturedEvent:
331446
| {

‎src/cron/service/failure-alerts.ts‎

Lines changed: 60 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import type { CronServiceState } from "./state.js";
66

77
const DEFAULT_FAILURE_ALERT_AFTER = 2;
88
const DEFAULT_FAILURE_ALERT_COOLDOWN_MS = 60 * 60_000; // 1 hour
9+
const FAILURE_ALERT_DELIVERY_TIMEOUT_MS = 10_000;
910

1011
type ResolvedFailureAlert = {
1112
after: number;
@@ -98,7 +99,16 @@ export function resolveFailureAlert(
9899
};
99100
}
100101

101-
function emitFailureAlert(
102+
function setFailureNotificationDelivery(
103+
job: CronJob,
104+
delivery: CronFailureNotificationDelivery,
105+
): void {
106+
job.state.lastFailureNotificationDelivered = delivery.delivered;
107+
job.state.lastFailureNotificationDeliveryStatus = delivery.status;
108+
job.state.lastFailureNotificationDeliveryError = delivery.error;
109+
}
110+
111+
async function emitFailureAlert(
102112
state: CronServiceState,
103113
params: {
104114
job: CronJob;
@@ -111,7 +121,7 @@ function emitFailureAlert(
111121
status: "error" | "skipped";
112122
provider?: string;
113123
},
114-
) {
124+
): Promise<CronFailureNotificationDelivery> {
115125
const safeJobName = params.job.name || params.job.id;
116126
const truncatedError = (params.error?.trim() || "unknown reason").slice(0, 200);
117127
const errorReason =
@@ -129,22 +139,48 @@ function emitFailureAlert(
129139
].join("\n");
130140

131141
if (state.deps.sendCronFailureAlert) {
132-
void state.deps
133-
.sendCronFailureAlert({
134-
job: params.job,
135-
text,
136-
channel: params.channel,
137-
to: params.to,
138-
mode: params.mode,
139-
accountId: params.accountId,
140-
})
141-
.catch((err: unknown) => {
142-
state.deps.log.warn(
143-
{ jobId: params.job.id, err: String(err) },
144-
"cron: failure alert delivery failed",
145-
);
146-
});
147-
return;
142+
const deliveryPromise = state.deps.sendCronFailureAlert({
143+
job: params.job,
144+
text,
145+
channel: params.channel,
146+
to: params.to,
147+
mode: params.mode,
148+
accountId: params.accountId,
149+
});
150+
const timeoutToken = Symbol("failure-alert-timeout");
151+
let timeout: ReturnType<typeof setTimeout> | undefined;
152+
try {
153+
const delivery = await Promise.race([
154+
deliveryPromise,
155+
new Promise<typeof timeoutToken>((resolve) => {
156+
timeout = setTimeout(() => resolve(timeoutToken), FAILURE_ALERT_DELIVERY_TIMEOUT_MS);
157+
}),
158+
]);
159+
if (delivery === timeoutToken) {
160+
void deliveryPromise.catch((err: unknown) => {
161+
state.deps.log.warn(
162+
{ jobId: params.job.id, err: String(err) },
163+
"cron: failure alert delivery failed after timeout",
164+
);
165+
});
166+
return {
167+
status: "unknown",
168+
error: `failure alert delivery timed out after ${FAILURE_ALERT_DELIVERY_TIMEOUT_MS}ms`,
169+
};
170+
}
171+
return delivery ?? { delivered: true, status: "delivered" };
172+
} catch (err: unknown) {
173+
const error = String(err);
174+
state.deps.log.warn(
175+
{ jobId: params.job.id, err: error },
176+
"cron: failure alert delivery failed",
177+
);
178+
return { delivered: false, status: "not-delivered", error };
179+
} finally {
180+
if (timeout) {
181+
clearTimeout(timeout);
182+
}
183+
}
148184
}
149185

150186
state.deps.enqueueSystemEvent(text, { agentId: params.job.agentId });
@@ -155,10 +191,11 @@ function emitFailureAlert(
155191
reason: `cron:${params.job.id}:failure-alert`,
156192
});
157193
}
194+
return { status: "unknown" };
158195
}
159196

160197
/** Emits a failure alert when threshold, best-effort, and cooldown policy allow it. */
161-
export function maybeEmitFailureAlert(
198+
export async function maybeEmitFailureAlert(
162199
state: CronServiceState,
163200
params: {
164201
job: CronJob;
@@ -168,7 +205,7 @@ export function maybeEmitFailureAlert(
168205
provider?: string;
169206
consecutiveCount: number;
170207
},
171-
) {
208+
): Promise<void> {
172209
if (!params.alertConfig || params.consecutiveCount < params.alertConfig.after) {
173210
return;
174211
}
@@ -185,7 +222,8 @@ export function maybeEmitFailureAlert(
185222
if (inCooldown) {
186223
return;
187224
}
188-
emitFailureAlert(state, {
225+
params.job.state.lastFailureAlertAtMs = now;
226+
const delivery = await emitFailureAlert(state, {
189227
job: params.job,
190228
error: params.error,
191229
consecutiveErrors: params.consecutiveCount,
@@ -196,5 +234,5 @@ export function maybeEmitFailureAlert(
196234
status: params.status,
197235
provider: params.provider,
198236
});
199-
params.job.state.lastFailureAlertAtMs = now;
237+
setFailureNotificationDelivery(params.job, delivery);
200238
}

‎src/cron/service/ops.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ import {
5353
armTimer,
5454
emit,
5555
executeJobCoreWithTimeout,
56+
maybeEmitPostRunFailureAlert,
5657
type IsolatedAgentSetupTimeoutSignal,
5758
maybeNotifyIsolatedAgentSetupTimeout,
5859
runMissedJobs,
@@ -661,6 +662,10 @@ async function skipInvalidPersistedManualRun(params: {
661662
},
662663
{ preserveSchedule: params.mode === "force" },
663664
);
665+
await maybeEmitPostRunFailureAlert(params.state, params.job, {
666+
status: "skipped",
667+
error: errorText,
668+
});
664669

665670
emit(params.state, {
666671
jobId: params.job.id,
@@ -947,6 +952,7 @@ async function finishPreparedManualRun(
947952
},
948953
{ preserveSchedule: mode === "force" },
949954
);
955+
await maybeEmitPostRunFailureAlert(state, job, coreResult);
950956

951957
emit(state, {
952958
jobId: job.id,

‎src/cron/service/state.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -179,7 +179,7 @@ export type CronServiceDeps = {
179179
to?: string;
180180
mode?: "announce" | "webhook";
181181
accountId?: string;
182-
}) => Promise<void>;
182+
}) => Promise<CronFailureNotificationDelivery | void>;
183183
onEvent?: (evt: CronEvent) => void;
184184
};
185185

0 commit comments

Comments
 (0)