diff --git a/apps/hub/src/index.ts b/apps/hub/src/index.ts index 313ce9201..7f9ff4d98 100644 --- a/apps/hub/src/index.ts +++ b/apps/hub/src/index.ts @@ -8,6 +8,8 @@ import { mkdirSync } from "node:fs"; import path from "node:path"; import { createDB, createGrantStore } from "@intx/db"; +import { workflowDefinition } from "@intx/db/schema"; +import { and, eq } from "drizzle-orm"; import { generateKeyPair } from "@intx/crypto"; import { timeWindowEvaluator } from "@intx/authz"; import type { ConditionRegistry } from "@intx/types/authz"; @@ -375,6 +377,16 @@ export async function createHub(config: HubConfig) { grantStore: chatGrantStore, conditionRegistry: chatConditionRegistry, }), + workflowDefinitionInTenant: async (tenantId, definitionId) => { + const row = await db.query.workflowDefinition.findFirst({ + where: and( + eq(workflowDefinition.id, definitionId), + eq(workflowDefinition.tenantId, tenantId), + ), + columns: { id: true }, + }); + return row !== undefined; + }, }), ); app.route( @@ -422,6 +434,16 @@ export async function createHub(config: HubConfig) { conditionRegistry: chatConditionRegistry, }), runSummaryResolver: createHubRunSummaryResolver(db), + definitionInTenant: async (tenantId, definitionId) => { + const row = await db.query.workflowDefinition.findFirst({ + where: and( + eq(workflowDefinition.id, definitionId), + eq(workflowDefinition.tenantId, tenantId), + ), + columns: { id: true }, + }); + return row !== undefined; + }, }), ); // Recurring auto-fire: a minimal in-process poller (routine-scheduler.ts) diff --git a/apps/hub/src/routine-scheduler.ts b/apps/hub/src/routine-scheduler.ts index fe5a58bc1..4f4daa504 100644 --- a/apps/hub/src/routine-scheduler.ts +++ b/apps/hub/src/routine-scheduler.ts @@ -5,25 +5,26 @@ // `@corbits/agent-lifecycle`'s own `setInterval` sweep (the only other // periodic loop in this repo) rather than pulling in a new dependency. // -// Two guarantees, precisely stated: +// Three guarantees, precisely stated: // // - Exactly-once against a *concurrent claim*: `RoutineStore.claimRoutineFire` -// is a conditional update (`nextFireAt <= now` in its WHERE clause, -// advanced to the trigger's next occurrence in its SET) — a second -// hub replica racing the same fire loses, because the winner already -// moved `nextFireAt` into the future before either replica launches -// anything. -// - At-least-once against a *launch failure*: a claim that wins but -// whose `fireScheduledRoutine` call then throws is compensated — -// `nextFireAt` is restored to the moment it was claimed for, so the -// next poll sees the fire as due again instead of silently skipping -// it until the trigger's following occurrence. -// -// And missed fires survive a restart: `nextFireAt` is persisted, so -// "due" means `nextFireAt <= now`, not "does the current wall-clock -// minute match" — a fire that was due while the hub was down is still -// due (and gets caught up) the next time this loop polls, exactly like -// `@corbits/schedules` before it. +// is a conditional update (enabled, not deleted, not dead-lettered, +// `nextFireAt <= now` in its WHERE clause, advanced to the trigger's +// next occurrence in its SET) — a second hub replica racing the same +// fire loses, because the winner already moved `nextFireAt` into the +// future before either replica launches anything. +// - At-least-once against a *launch failure*, with exponential backoff: +// a claim that wins but whose `fireScheduledRoutine` call then throws +// is marked failed via `markFailedFire` — consecutiveFailures ticks +// up, `nextFireAt` is set to `failedAt + backoff`, and a +// `schedule-failed` run is recorded. After +// `MAX_ROUTINE_FIRE_FAILURES` consecutive failures the routine is +// dead-lettered (`deadLetteredAt` set, `nextFireAt` null) and the +// scheduler stops claiming it until an operator re-enables/edits it. +// - Missed fires survive a restart: `nextFireAt` is persisted, so +// "due" means `nextFireAt <= now`, not "does the current wall-clock +// minute match" — a fire that was due while the hub was down is still +// due (and gets caught up) the next time this loop polls. import type { RoutineLauncher, RoutineStore } from "@corbits/routines"; import { fireScheduledRoutine } from "@corbits/routines"; import { getLogger } from "@intx/log"; @@ -61,29 +62,30 @@ export async function tickRoutineScheduler( { tenantId: claimed.tenantId, routine: claimed }, ); } catch (err) { - log.error`scheduled fire of routine ${claimed.id} failed: ${ - err instanceof Error ? err.message : String(err) - }`; + const reason = err instanceof Error ? err.message : String(err); + log.error`scheduled fire of routine ${claimed.id} failed: ${reason}`; // The claim already advanced `nextFireAt` past `at`; since the - // launch never happened, restore it to `at` so the next poll - // retries this fire instead of silently dropping it until the - // trigger's following occurrence. `claimed.nextFireAt` is the - // value the claim itself just wrote (never null — a claim only - // succeeds for a triggered routine), passed through so the - // restore is conditional and can't clobber a newer trigger edit. + // launch never happened, mark the failure with backoff (or + // dead-letter). `claimed.nextFireAt` is the value the claim + // itself just wrote (never null — a claim only succeeds for a + // triggered routine), passed through so the mark is conditional + // and can't clobber a newer trigger edit. try { if (claimed.nextFireAt !== null) { - await deps.store.compensateFailedFire( - claimed.id, - at, - claimed.nextFireAt, - ); + const result = await deps.store.markFailedFire({ + routineId: claimed.id, + tenantId: claimed.tenantId, + claimedNextFireAt: claimed.nextFireAt, + failedAt: at, + reason, + }); + if (result?.deadLettered) { + log.error`routine ${claimed.id} dead-lettered after ${result.consecutiveFailures} consecutive launch failures`; + } } - } catch (compensateErr) { - log.error`compensating routine ${claimed.id}'s failed fire also failed: ${ - compensateErr instanceof Error - ? compensateErr.message - : String(compensateErr) + } catch (markErr) { + log.error`marking routine ${claimed.id}'s failed fire also failed: ${ + markErr instanceof Error ? markErr.message : String(markErr) }`; } } diff --git a/apps/hub/test/routine-scheduler.test.ts b/apps/hub/test/routine-scheduler.test.ts index 9b70eb2d5..beea60708 100644 --- a/apps/hub/test/routine-scheduler.test.ts +++ b/apps/hub/test/routine-scheduler.test.ts @@ -1,117 +1,173 @@ -// The scheduler loop's two failure modes, proven against a single -// deterministic poll (`tickRoutineScheduler`) rather than the real -// `setInterval` wrapper: a launch that throws must not strand the -// routine past its next natural cadence, and a successful launch must -// still record the correlation exactly once. +// Scheduler poller: claim → fire, backoff on failure, dead-letter at max. import { describe, expect, test } from "bun:test"; import { + backoffMsForFailure, createInMemoryRoutineStore, + MAX_ROUTINE_FIRE_FAILURES, type RoutineLauncher, } from "@corbits/routines"; import { tickRoutineScheduler } from "../src/routine-scheduler"; -const TENANT_ID = "tnt_1"; +const CRON = { kind: "cron" as const, expression: "0 * * * *" }; -function throwingLauncher(): RoutineLauncher { - return { - async launchRoutineRun() { - throw new Error("launcher unavailable"); - }, - }; -} - -function succeedingLauncher(): RoutineLauncher & { calls: number } { - let calls = 0; - return { - get calls() { - return calls; - }, - async launchRoutineRun() { - calls += 1; - return { runId: `run_${calls}` }; - }, - }; +function launcher(impl: RoutineLauncher["launchRoutineRun"]): RoutineLauncher { + return { launchRoutineRun: impl }; } describe("tickRoutineScheduler", () => { - test("a due routine fires and its run is recorded", async () => { + test("claims a due routine and launches it once", async () => { const store = createInMemoryRoutineStore(); - const launcher = succeedingLauncher(); const routine = await store.createRoutine({ - tenantId: TENANT_ID, - name: "Hourly", + tenantId: "t1", + name: "hourly", definitionId: "def_1", - trigger: { kind: "interval", unit: "hours", every: 1 }, + trigger: CRON, scope: "bench", - input: {}, + input: { x: 1 }, createdBy: "user_1", }); - const fireAt = routine.nextFireAt; - if (fireAt === null) throw new Error("expected a scheduled fire time"); - - await tickRoutineScheduler({ store, launcher }, fireAt); - - expect(launcher.calls).toBe(1); - const runs = await store.listRunsForRoutine(TENANT_ID, routine.id); + const at = new Date( + Math.max(Date.now(), routine.nextFireAt?.getTime() ?? 0), + ); + const launches: string[] = []; + await tickRoutineScheduler( + { + store, + launcher: launcher(async (input) => { + launches.push(input.definitionId); + return { runId: "run_1" }; + }), + }, + at, + ); + expect(launches).toEqual(["def_1"]); + const runs = await store.listRunsForRoutine("t1", routine.id); expect(runs).toHaveLength(1); expect(runs[0]?.triggeredBy).toBe("schedule"); + expect(runs[0]?.runId).toBe("run_1"); }); - test("a launch failure restores nextFireAt instead of stranding the routine", async () => { + test("a launch failure backs off and records schedule-failed", async () => { const store = createInMemoryRoutineStore(); - const launcher = throwingLauncher(); const routine = await store.createRoutine({ - tenantId: TENANT_ID, - name: "Hourly", + tenantId: "t1", + name: "flaky", definitionId: "def_1", - trigger: { kind: "interval", unit: "hours", every: 1 }, + trigger: CRON, scope: "bench", input: {}, createdBy: "user_1", }); - const fireAt = routine.nextFireAt; - if (fireAt === null) throw new Error("expected a scheduled fire time"); - - await tickRoutineScheduler({ store, launcher }, fireAt); + const at = new Date( + Math.max(Date.now(), routine.nextFireAt?.getTime() ?? 0), + ); + await tickRoutineScheduler( + { + store, + launcher: launcher(async () => { + throw new Error("launch exploded"); + }), + }, + at, + ); - // No run was recorded — the launch never succeeded. - const runs = await store.listRunsForRoutine(TENANT_ID, routine.id); - expect(runs).toHaveLength(0); + const after = await store.getRoutine("t1", routine.id); + if (!after) throw new Error("expected routine after failure"); + expect(after.consecutiveFailures).toBe(1); + const afterNext = after.nextFireAt; + if (!afterNext) throw new Error("expected nextFireAt after failure"); + expect(afterNext.getTime()).toBe(at.getTime() + backoffMsForFailure(1)); + // Not immediately due again at the same instant. + expect(await store.listDueRoutines(at)).toEqual([]); - // And the routine is due again at the exact moment it failed, not - // stranded until its next natural hourly cadence. - const dueAgain = await store.listDueRoutines(fireAt); - expect(dueAgain.map((row) => row.id)).toContain(routine.id); + const runs = await store.listRunsForRoutine("t1", routine.id); + expect(runs).toHaveLength(1); + expect(runs[0]?.triggeredBy).toBe("schedule-failed"); + expect(runs[0]?.error).toContain("launch exploded"); }); - test("a retried fire after a failure can still succeed", async () => { + test("after backoff elapses the routine is claimed again", async () => { const store = createInMemoryRoutineStore(); - let attempts = 0; - const flakyLauncher: RoutineLauncher = { - async launchRoutineRun() { - attempts += 1; - if (attempts === 1) throw new Error("transient failure"); - return { runId: "run_retry" }; - }, - }; const routine = await store.createRoutine({ - tenantId: TENANT_ID, - name: "Hourly", + tenantId: "t1", + name: "retry", definitionId: "def_1", - trigger: { kind: "interval", unit: "hours", every: 1 }, + trigger: CRON, scope: "bench", input: {}, createdBy: "user_1", }); - const fireAt = routine.nextFireAt; - if (fireAt === null) throw new Error("expected a scheduled fire time"); - - await tickRoutineScheduler({ store, launcher: flakyLauncher }, fireAt); - await tickRoutineScheduler({ store, launcher: flakyLauncher }, fireAt); + const at = new Date( + Math.max(Date.now(), routine.nextFireAt?.getTime() ?? 0), + ); + await tickRoutineScheduler( + { + store, + launcher: launcher(async () => { + throw new Error("first"); + }), + }, + at, + ); + const afterFail = await store.getRoutine("t1", routine.id); + if (!afterFail) throw new Error("expected routine after failure"); + const retryAt = afterFail.nextFireAt; + if (!retryAt) throw new Error("expected nextFireAt after failure"); + let launches = 0; + await tickRoutineScheduler( + { + store, + launcher: launcher(async () => { + launches += 1; + return { runId: "run_ok" }; + }), + }, + retryAt, + ); + expect(launches).toBe(1); + const recovered = await store.getRoutine("t1", routine.id); + if (!recovered) throw new Error("expected recovered routine"); + expect(recovered.consecutiveFailures).toBe(0); + }); - expect(attempts).toBe(2); - const runs = await store.listRunsForRoutine(TENANT_ID, routine.id); - expect(runs).toHaveLength(1); - expect(runs[0]?.runId).toBe("run_retry"); + test("after MAX failures the routine is dead-lettered and never claimed again", async () => { + const store = createInMemoryRoutineStore(); + const routine = await store.createRoutine({ + tenantId: "t1", + name: "dead", + definitionId: "def_1", + trigger: CRON, + scope: "bench", + input: {}, + createdBy: "user_1", + }); + let clock = new Date( + Math.max(Date.now(), routine.nextFireAt?.getTime() ?? 0), + ); + for (let i = 0; i < MAX_ROUTINE_FIRE_FAILURES; i++) { + const current = await store.getRoutine("t1", routine.id); + if (!current) throw new Error("expected routine in loop"); + if (current.nextFireAt !== null) { + clock = new Date( + Math.max(clock.getTime(), current.nextFireAt.getTime()), + ); + } + await tickRoutineScheduler( + { + store, + launcher: launcher(async () => { + throw new Error(`fail ${i + 1}`); + }), + }, + clock, + ); + } + const final = await store.getRoutine("t1", routine.id); + if (!final) throw new Error("expected routine after dead-letter"); + expect(final.deadLetteredAt).not.toBeNull(); + expect(final.consecutiveFailures).toBe(MAX_ROUTINE_FIRE_FAILURES); + expect( + await store.listDueRoutines(new Date(clock.getTime() + 1e12)), + ).toEqual([]); }); }); diff --git a/packages/routines/src/index.ts b/packages/routines/src/index.ts index e6c0fd742..6baf0c30c 100644 --- a/packages/routines/src/index.ts +++ b/packages/routines/src/index.ts @@ -19,7 +19,14 @@ export type { ApplyRoutineMigrationsReport, } from "./migrations"; -export { createDrizzleRoutineStore, createInMemoryRoutineStore } from "./store"; +export { + createDrizzleRoutineStore, + createInMemoryRoutineStore, + MAX_ROUTINE_FIRE_FAILURES, + ROUTINE_FIRE_BACKOFF_BASE_MS, + ROUTINE_FIRE_BACKOFF_MAX_MS, + backoffMsForFailure, +} from "./store"; export type { RoutineDb, RoutineScope, @@ -28,6 +35,7 @@ export type { CreateRoutineInput, UpdateRoutineInput, RoutineStore, + MarkFailedFireResult, } from "./store"; export { createRoutineRoutes, fireScheduledRoutine } from "./routes"; diff --git a/packages/routines/src/migrations.ts b/packages/routines/src/migrations.ts index ec152ef6c..33d02f0c1 100644 --- a/packages/routines/src/migrations.ts +++ b/packages/routines/src/migrations.ts @@ -5,12 +5,6 @@ // story" install contract. Bookkeeping is its own ledger table, never // the platform's drizzle journal, so this package's migration history // stays extractable on its own. -// -// A single migration, in its final shape, not a sequence of ALTERs -// bolted on as the feature grew: this package predates any real -// traffic, so there is no pre-existing data to carry forward and no -// earlier shape to migrate away from. Hard cutover — one file, no -// backfill, nothing to reconcile. import postgres from "postgres"; export interface RoutineMigration { @@ -51,6 +45,17 @@ export const routineMigrations: readonly RoutineMigration[] = [ ); `, }, + { + name: "0002_failure_tracking", + sql: ` + ALTER TABLE "routine" + ADD COLUMN IF NOT EXISTS "consecutive_failures" integer NOT NULL DEFAULT 0; + ALTER TABLE "routine" + ADD COLUMN IF NOT EXISTS "dead_lettered_at" timestamptz; + ALTER TABLE "routine_run" + ADD COLUMN IF NOT EXISTS "error" text; + `, + }, ]; // Named distinctly from the platform's setup ledger and from any diff --git a/packages/routines/src/routes.ts b/packages/routines/src/routes.ts index 1f9c908df..eee9ea1c4 100644 --- a/packages/routines/src/routes.ts +++ b/packages/routines/src/routes.ts @@ -5,7 +5,8 @@ // "Run now" and a scheduled fire are the same launcher call // (`deps.launcher.launchRoutineRun`) with a different `triggeredBy`; // this module never grows a second launch path for the unscheduled -// case. +// case. Launch + correlation write is one shared helper so a silent +// orphan (launch succeeded, correlation lost) is impossible. import { Hono } from "hono"; import { type } from "arktype"; @@ -54,6 +55,14 @@ export type CreateRoutineRoutesDeps = { launcher: RoutineLauncher; requireGrant: RequireGrant; runSummaryResolver?: RunSummaryResolver; + /** + * When provided, `POST /routines` rejects with 404 if the definition + * is not in the request tenant. Tests may omit (always-allow). + */ + definitionInTenant?: ( + tenantId: string, + definitionId: string, + ) => Promise; }; const ErrorEnvelope = (code: string, message: string) => ({ @@ -96,6 +105,8 @@ function routineView(row: RoutineRow) { input: row.input, enabled: row.enabled, deliveryChannelId: row.deliveryChannelId, + consecutiveFailures: row.consecutiveFailures, + deadLetteredAt: row.deadLetteredAt?.toISOString() ?? null, createdAt: row.createdAt.toISOString(), updatedAt: row.updatedAt.toISOString(), }; @@ -109,11 +120,54 @@ async function runView( return { runId: row.runId, triggeredBy: row.triggeredBy, + error: row.error, createdAt: row.createdAt.toISOString(), ...(summary !== undefined ? { run: summary } : {}), }; } +/** + * Launch then correlate. If the correlation write fails after a + * successful launch, rethrow loudly with the run id in the message — + * never silently orphan a platform run. Clears fire-failure counters + * only after both steps succeed. + */ +async function launchAndCorrelate( + deps: { store: RoutineStore; launcher: RoutineLauncher }, + input: { + tenantId: string; + principalId: string; + definitionId: string; + input: Record; + routineId: string; + triggeredBy: string; + }, +): Promise { + const launched = await deps.launcher.launchRoutineRun({ + tenantId: input.tenantId, + principalId: input.principalId, + definitionId: input.definitionId, + input: input.input, + }); + try { + await deps.store.recordRoutineRun({ + tenantId: input.tenantId, + routineId: input.routineId, + runId: launched.runId, + triggeredBy: input.triggeredBy, + }); + } catch (err) { + throw new Error( + `routine launch succeeded (run ${launched.runId}) but correlation write failed: ${ + err instanceof Error ? err.message : String(err) + }`, + { cause: err }, + ); + } + await deps.store.clearFireFailures(input.routineId); + return launched; +} + export function createRoutineRoutes( deps: CreateRoutineRoutesDeps, ): Hono { @@ -134,6 +188,19 @@ export function createRoutineRoutes( const tenant = c.get("tenant"); const principal = c.get("principal"); + if (deps.definitionInTenant !== undefined) { + const owned = await deps.definitionInTenant( + tenant.id, + body.definitionId, + ); + if (!owned) { + return c.json( + ErrorEnvelope("not_found", "definition not found"), + 404, + ); + } + } + const row = await deps.store.createRoutine({ tenantId: tenant.id, name: body.name, @@ -273,19 +340,17 @@ export function createRoutineRoutes( // "Run now" is an unscheduled fire of the exact launcher a // scheduled trigger would call — the only difference is // `triggeredBy`, never a second launch code path. - const launched = await deps.launcher.launchRoutineRun({ - tenantId: tenant.id, - principalId: principal.id, - definitionId: existing.definitionId, - input: body.input ?? existing.input, - }); - - await deps.store.recordRoutineRun({ - tenantId: tenant.id, - routineId, - runId: launched.runId, - triggeredBy: "manual", - }); + const launched = await launchAndCorrelate( + { store: deps.store, launcher: deps.launcher }, + { + tenantId: tenant.id, + principalId: principal.id, + definitionId: existing.definitionId, + input: body.input ?? existing.input, + routineId, + triggeredBy: "manual", + }, + ); return c.json({ runId: launched.runId }, 201); }, @@ -311,17 +376,12 @@ export async function fireScheduledRoutine( `routine ${params.routine.id} is disabled; a scheduler must not fire it`, ); } - const launched = await deps.launcher.launchRoutineRun({ + return launchAndCorrelate(deps, { tenantId: params.tenantId, principalId: params.routine.createdBy, definitionId: params.routine.definitionId, input: params.routine.input, - }); - await deps.store.recordRoutineRun({ - tenantId: params.tenantId, routineId: params.routine.id, - runId: launched.runId, triggeredBy: "schedule", }); - return launched; } diff --git a/packages/routines/src/schema.ts b/packages/routines/src/schema.ts index 790906237..c89f12696 100644 --- a/packages/routines/src/schema.ts +++ b/packages/routines/src/schema.ts @@ -6,6 +6,7 @@ // own state, keyed by tenant. import { boolean, + integer, jsonb, pgTable, primaryKey, @@ -48,6 +49,13 @@ export const routine = pgTable("routine", { // it stops the routine appearing in lists or firing, and nothing // else. deletedAt: timestamp("deleted_at", { withTimezone: true }), + // Consecutive launch failures since the last success. The scheduler + // backs off exponentially off this counter, then dead-letters once + // it reaches `MAX_ROUTINE_FIRE_FAILURES` (see store.ts). + consecutiveFailures: integer("consecutive_failures").notNull().default(0), + // When set, the scheduler never claims this routine again until an + // operator re-enables/edits it (which clears the dead-letter). + deadLetteredAt: timestamp("dead_lettered_at", { withTimezone: true }), createdAt: timestamp("created_at", { withTimezone: true }) .notNull() .defaultNow(), @@ -63,6 +71,11 @@ export const routine = pgTable("routine", { * own `workflow_run` — this package never migrates a table it doesn't * own — and it holds nothing else: run status, timing, and mail all * stay read off the platform's own run surfaces, joined by `runId`. + * + * Failed launches that never produced a platform run still get a row + * (`triggeredBy: "schedule-failed"`, synthetic `runId`, `error` set) so + * the history surface can show the failure without inventing a second + * bookkeeping path. */ export const routineRun = pgTable( "routine_run", @@ -71,6 +84,7 @@ export const routineRun = pgTable( routineId: text("routine_id").notNull(), runId: text("run_id").notNull(), triggeredBy: text("triggered_by").notNull(), + error: text("error"), createdAt: timestamp("created_at", { withTimezone: true }) .notNull() .defaultNow(), diff --git a/packages/routines/src/store.ts b/packages/routines/src/store.ts index 59bda04cc..be80928c2 100644 --- a/packages/routines/src/store.ts +++ b/packages/routines/src/store.ts @@ -16,6 +16,25 @@ export type RoutineDb< export type RoutineScope = "personal" | "bench"; +/** After this many consecutive launch failures, the routine is dead-lettered. */ +export const MAX_ROUTINE_FIRE_FAILURES = 5; +/** First retry delay after a failed launch (1 minute). */ +export const ROUTINE_FIRE_BACKOFF_BASE_MS = 60_000; +/** Cap on exponential backoff between retries (1 hour). */ +export const ROUTINE_FIRE_BACKOFF_MAX_MS = 60 * 60 * 1000; + +/** + * Delay until the next retry after `consecutiveFailures` failures have + * already been recorded for this routine (1-based: first failure → 1). + */ +export function backoffMsForFailure(consecutiveFailures: number): number { + const exp = Math.max(0, consecutiveFailures - 1); + return Math.min( + ROUTINE_FIRE_BACKOFF_BASE_MS * 2 ** exp, + ROUTINE_FIRE_BACKOFF_MAX_MS, + ); +} + export interface RoutineRow { readonly id: string; readonly tenantId: string; @@ -30,6 +49,8 @@ export interface RoutineRow { readonly nextFireAt: Date | null; readonly lastFireAt: Date | null; readonly deletedAt: Date | null; + readonly consecutiveFailures: number; + readonly deadLetteredAt: Date | null; readonly createdAt: Date; readonly updatedAt: Date; } @@ -58,9 +79,16 @@ export interface RoutineRunRow { readonly routineId: string; readonly runId: string; readonly triggeredBy: string; + readonly error: string | null; readonly createdAt: Date; } +export interface MarkFailedFireResult { + readonly deadLettered: boolean; + readonly nextFireAt: Date | null; + readonly consecutiveFailures: number; +} + export interface RoutineStore { createRoutine(input: CreateRoutineInput): Promise; /** `undefined` for an unknown OR a soft-deleted routine. */ @@ -92,59 +120,94 @@ export interface RoutineStore { * Records that `runId` was launched under `routineId` — called * inside the same launch call every routine fire goes through * (scheduled or "run now" alike), never a second bookkeeping path. + * Failed launches that never produced a platform run still get a row + * (`triggeredBy: "schedule-failed"`, synthetic `runId`, `error` set). */ recordRoutineRun(input: { tenantId: string; routineId: string; runId: string; triggeredBy: string; + error?: string | null; }): Promise; listRunsForRoutine( tenantId: string, routineId: string, ): Promise; /** - * Every enabled, timer-triggered routine whose `nextFireAt` is at or - * before `now` — across every tenant, the one cross-tenant read a - * scheduler needs and no per-request route ever does. "At or before," - * not "equal to": a fire that was due while nothing was polling is - * still due, not skipped. + * Every enabled, timer-triggered, non-dead-lettered routine whose + * `nextFireAt` is at or before `now` — across every tenant, the one + * cross-tenant read a scheduler needs and no per-request route ever + * does. "At or before," not "equal to": a fire that was due while + * nothing was polling is still due, not skipped. */ listDueRoutines(now: Date): Promise; /** * Atomically claims `routineId`'s current due fire: advances * `nextFireAt` to the trigger's following occurrence and stamps - * `lastFireAt`, but only if `nextFireAt` is still `<= now` at the - * moment of the write. A second caller racing the same fire loses — - * the first claim already moved `nextFireAt` into the future, so the - * second claim's conditional write matches no row and returns - * `undefined`. This is the seam that makes a scheduled fire - * exactly-once under concurrent pollers: the claim happens before - * anything launches, never after. + * `lastFireAt`, but only if the row is still claimable at the moment + * of the write (enabled, not deleted, not dead-lettered, due). A + * second caller racing the same fire loses — the first claim already + * moved `nextFireAt` into the future, so the second claim's + * conditional write matches no row and returns `undefined`. */ claimRoutineFire( routineId: string, now: Date, ): Promise; /** - * Undoes a claim whose launch failed: restores `nextFireAt` to - * `revertNextFireAt` (the moment the claim was made for) so the next - * scheduler poll sees the fire as due again instead of silently - * skipping it until the trigger's following occurrence. Called only - * after `claimRoutineFire` returned a row and the subsequent launch - * threw — a claim that was never granted needs no compensation. + * After a claimed fire's launch fails: increments + * `consecutiveFailures`, records a `schedule-failed` run with the + * reason, and either schedules a backoff retry or dead-letters the + * routine once `MAX_ROUTINE_FIRE_FAILURES` is reached. * * Conditional on `nextFireAt` still being `claimedNextFireAt` — the * value the claim itself wrote. If a trigger edit landed during the - * failure window, `updateRoutine` already recomputed `nextFireAt` - * off the new trigger, and that newer value must win: this restore - * is a no-op rather than clobbering it with the stale one. + * failure window, that newer value wins and this is a no-op + * (`undefined`). */ - compensateFailedFire( - routineId: string, - revertNextFireAt: Date, - claimedNextFireAt: Date, - ): Promise; + markFailedFire(input: { + routineId: string; + tenantId: string; + claimedNextFireAt: Date; + failedAt: Date; + reason: string; + }): Promise; + /** Clear failure counters after a successful fire. */ + clearFireFailures(routineId: string): Promise; +} + +function mapRoutineRow(row: typeof routine.$inferSelect): RoutineRow { + return { + id: row.id, + tenantId: row.tenantId, + name: row.name, + definitionId: row.definitionId, + trigger: row.trigger as RoutineTriggerT, + scope: row.scope as RoutineScope, + input: row.input as Record, + enabled: row.enabled, + deliveryChannelId: row.deliveryChannelId, + createdBy: row.createdBy, + nextFireAt: row.nextFireAt, + lastFireAt: row.lastFireAt, + deletedAt: row.deletedAt, + consecutiveFailures: row.consecutiveFailures ?? 0, + deadLetteredAt: row.deadLetteredAt, + createdAt: row.createdAt, + updatedAt: row.updatedAt, + }; +} + +function mapRunRow(row: typeof routineRun.$inferSelect): RoutineRunRow { + return { + tenantId: row.tenantId, + routineId: row.routineId, + runId: row.runId, + triggeredBy: row.triggeredBy, + error: row.error ?? null, + createdAt: row.createdAt, + }; } export function createDrizzleRoutineStore< @@ -169,6 +232,8 @@ export function createDrizzleRoutineStore< nextFireAt: computeNextFireAt(input.trigger, now), lastFireAt: null, deletedAt: null, + consecutiveFailures: 0, + deadLetteredAt: null, createdAt: now, updatedAt: now, }) @@ -176,7 +241,7 @@ export function createDrizzleRoutineStore< if (row === undefined) { throw new Error("createRoutine: insert returned no row"); } - return row as RoutineRow; + return mapRoutineRow(row); }, async getRoutine(tenantId, routineId) { @@ -191,7 +256,7 @@ export function createDrizzleRoutineStore< ), ) .limit(1); - return row as RoutineRow | undefined; + return row === undefined ? undefined : mapRoutineRow(row); }, async getRoutineIncludingDeleted(tenantId, routineId) { @@ -200,7 +265,7 @@ export function createDrizzleRoutineStore< .from(routine) .where(and(eq(routine.tenantId, tenantId), eq(routine.id, routineId))) .limit(1); - return row as RoutineRow | undefined; + return row === undefined ? undefined : mapRoutineRow(row); }, async listRoutines(tenantId) { @@ -208,7 +273,7 @@ export function createDrizzleRoutineStore< .select() .from(routine) .where(and(eq(routine.tenantId, tenantId), isNull(routine.deletedAt))); - return rows as RoutineRow[]; + return rows.map(mapRoutineRow); }, async updateRoutine(tenantId, routineId, patch) { @@ -229,14 +294,14 @@ export function createDrizzleRoutineStore< const now = new Date(); const recomputeNextFire = patch.trigger !== undefined || patch.enabled !== undefined; + const clearFailures = + patch.enabled === true || patch.trigger !== undefined; const mergedTrigger = patch.trigger !== undefined ? patch.trigger - : (existing as RoutineRow).trigger; + : (existing.trigger as RoutineTriggerT); const mergedEnabled = - patch.enabled !== undefined - ? patch.enabled - : (existing as RoutineRow).enabled; + patch.enabled !== undefined ? patch.enabled : existing.enabled; const [row] = await db .update(routine) .set({ @@ -248,6 +313,9 @@ export function createDrizzleRoutineStore< : null, } : {}), + ...(clearFailures + ? { consecutiveFailures: 0, deadLetteredAt: null } + : {}), updatedAt: now, }) .where(and(eq(routine.tenantId, tenantId), eq(routine.id, routineId))) @@ -255,7 +323,7 @@ export function createDrizzleRoutineStore< if (row === undefined) { throw new Error(`updateRoutine: no routine row for id ${routineId}`); } - return row as RoutineRow; + return mapRoutineRow(row); }, async deleteRoutine(tenantId, routineId) { @@ -281,20 +349,18 @@ export function createDrizzleRoutineStore< and( eq(routine.enabled, true), isNull(routine.deletedAt), + isNull(routine.deadLetteredAt), lte(routine.nextFireAt, now), ), ); - return rows as RoutineRow[]; + return rows.map(mapRoutineRow); }, async claimRoutineFire(routineId, now) { // A transaction with `FOR UPDATE` locks the row for the read // that decides `nextFireAt`'s new value, so a concurrent `PATCH` // of this routine's trigger can't sneak in between "read the - // trigger" and "write the value computed from it" — it blocks - // until this transaction commits, then (having already recomputed - // its own `nextFireAt` off the new trigger in `updateRoutine`) - // is never clobbered by a value computed from the stale one. + // trigger" and "write the value computed from it". return await db.transaction(async (tx) => { const [current] = await tx .select() @@ -302,7 +368,15 @@ export function createDrizzleRoutineStore< .where(eq(routine.id, routineId)) .for("update") .limit(1); - if (current === undefined || current.trigger === null) { + if ( + current === undefined || + current.trigger === null || + current.deletedAt !== null || + current.deadLetteredAt !== null || + !current.enabled || + current.nextFireAt === null || + current.nextFireAt.getTime() > now.getTime() + ) { return undefined; } const nextFireAt = computeNextFireAt( @@ -316,32 +390,96 @@ export function createDrizzleRoutineStore< and( eq(routine.id, routineId), eq(routine.enabled, true), + isNull(routine.deletedAt), + isNull(routine.deadLetteredAt), lte(routine.nextFireAt, now), ), ) .returning(); - return claimed as RoutineRow | undefined; + return claimed === undefined ? undefined : mapRoutineRow(claimed); }); }, - async compensateFailedFire(routineId, revertNextFireAt, claimedNextFireAt) { + async markFailedFire(input) { + return await db.transaction(async (tx) => { + const [current] = await tx + .select() + .from(routine) + .where(eq(routine.id, input.routineId)) + .for("update") + .limit(1); + if (current === undefined) return undefined; + if ( + current.nextFireAt?.getTime() !== input.claimedNextFireAt.getTime() + ) { + return undefined; + } + + const consecutiveFailures = (current.consecutiveFailures ?? 0) + 1; + const deadLettered = consecutiveFailures >= MAX_ROUTINE_FIRE_FAILURES; + const nextFireAt = deadLettered + ? null + : new Date( + input.failedAt.getTime() + + backoffMsForFailure(consecutiveFailures), + ); + + const [updated] = await tx + .update(routine) + .set({ + consecutiveFailures, + nextFireAt, + deadLetteredAt: deadLettered ? input.failedAt : null, + updatedAt: input.failedAt, + }) + .where( + and( + eq(routine.id, input.routineId), + eq(routine.nextFireAt, input.claimedNextFireAt), + ), + ) + .returning(); + if (updated === undefined) return undefined; + + await tx.insert(routineRun).values({ + tenantId: input.tenantId, + routineId: input.routineId, + runId: generateId("instance"), + triggeredBy: "schedule-failed", + error: input.reason, + createdAt: input.failedAt, + }); + + return { + deadLettered, + nextFireAt, + consecutiveFailures, + }; + }); + }, + + async clearFireFailures(routineId) { await db .update(routine) - .set({ nextFireAt: revertNextFireAt }) - .where( - and( - eq(routine.id, routineId), - eq(routine.nextFireAt, claimedNextFireAt), - ), - ); + .set({ consecutiveFailures: 0, deadLetteredAt: null }) + .where(eq(routine.id, routineId)); }, async recordRoutineRun(input) { - const [row] = await db.insert(routineRun).values(input).returning(); + const [row] = await db + .insert(routineRun) + .values({ + tenantId: input.tenantId, + routineId: input.routineId, + runId: input.runId, + triggeredBy: input.triggeredBy, + error: input.error ?? null, + }) + .returning(); if (row === undefined) { throw new Error("recordRoutineRun: insert returned no row"); } - return row as RoutineRunRow; + return mapRunRow(row); }, async listRunsForRoutine(tenantId, routineId) { @@ -355,7 +493,7 @@ export function createDrizzleRoutineStore< ), ) .orderBy(desc(routineRun.createdAt)); - return rows as RoutineRunRow[]; + return rows.map(mapRunRow); }, }; } @@ -386,6 +524,8 @@ export function createInMemoryRoutineStore(): RoutineStore { nextFireAt: computeNextFireAt(input.trigger, now), lastFireAt: null, deletedAt: null, + consecutiveFailures: 0, + deadLetteredAt: null, createdAt: now, updatedAt: now, }; @@ -422,6 +562,8 @@ export function createInMemoryRoutineStore(): RoutineStore { const now = new Date(); const recomputeNextFire = patch.trigger !== undefined || patch.enabled !== undefined; + const clearFailures = + patch.enabled === true || patch.trigger !== undefined; const mergedTrigger = patch.trigger !== undefined ? patch.trigger : existing.trigger; const mergedEnabled = @@ -436,6 +578,9 @@ export function createInMemoryRoutineStore(): RoutineStore { : null, } : {}), + ...(clearFailures + ? { consecutiveFailures: 0, deadLetteredAt: null } + : {}), updatedAt: now, }; routinesById.set(routineId, row); @@ -464,6 +609,7 @@ export function createInMemoryRoutineStore(): RoutineStore { (row) => row.enabled && row.deletedAt === null && + row.deadLetteredAt === null && row.nextFireAt !== null && row.nextFireAt.getTime() <= now.getTime(), ); @@ -474,6 +620,8 @@ export function createInMemoryRoutineStore(): RoutineStore { if ( current === undefined || current.trigger === null || + current.deletedAt !== null || + current.deadLetteredAt !== null || !current.enabled || current.nextFireAt === null || current.nextFireAt.getTime() > now.getTime() @@ -489,20 +637,60 @@ export function createInMemoryRoutineStore(): RoutineStore { return claimed; }, - async compensateFailedFire(routineId, revertNextFireAt, claimedNextFireAt) { + async markFailedFire(input) { + const current = routinesById.get(input.routineId); + if (current === undefined) return undefined; + if (current.nextFireAt?.getTime() !== input.claimedNextFireAt.getTime()) { + return undefined; + } + + const consecutiveFailures = current.consecutiveFailures + 1; + const deadLettered = consecutiveFailures >= MAX_ROUTINE_FIRE_FAILURES; + const nextFireAt = deadLettered + ? null + : new Date( + input.failedAt.getTime() + backoffMsForFailure(consecutiveFailures), + ); + + routinesById.set(input.routineId, { + ...current, + consecutiveFailures, + nextFireAt, + deadLetteredAt: deadLettered ? input.failedAt : null, + updatedAt: input.failedAt, + }); + + runs.push({ + tenantId: input.tenantId, + routineId: input.routineId, + runId: generateId("instance"), + triggeredBy: "schedule-failed", + error: input.reason, + createdAt: input.failedAt, + }); + + return { deadLettered, nextFireAt, consecutiveFailures }; + }, + + async clearFireFailures(routineId) { const current = routinesById.get(routineId); if (current === undefined) return; - if (current.nextFireAt?.getTime() !== claimedNextFireAt.getTime()) { - return; - } routinesById.set(routineId, { ...current, - nextFireAt: revertNextFireAt, + consecutiveFailures: 0, + deadLetteredAt: null, }); }, async recordRoutineRun(input) { - const row: RoutineRunRow = { ...input, createdAt: new Date() }; + const row: RoutineRunRow = { + tenantId: input.tenantId, + routineId: input.routineId, + runId: input.runId, + triggeredBy: input.triggeredBy, + error: input.error ?? null, + createdAt: new Date(), + }; runs.push(row); return row; }, diff --git a/packages/routines/test/routes.test.ts b/packages/routines/test/routes.test.ts index 31b15bd73..2cb5a5cbd 100644 --- a/packages/routines/test/routes.test.ts +++ b/packages/routines/test/routes.test.ts @@ -110,6 +110,26 @@ describe("createRoutineRoutes", () => { expect(typeof body["id"]).toBe("string"); }); + test("rejects a definition that is not in the tenant", async () => { + const deps = buildDeps(); + deps.definitionInTenant = async () => false; + const app = mountAs(createRoutineRoutes(deps), "user_1"); + const { response, body } = await createRoutine(app, VALID_BODY); + expect(response.status).toBe(404); + expect((body["error"] as Record)["code"]).toBe( + "not_found", + ); + }); + + test("accepts a definition that is in the tenant when a checker is wired", async () => { + const deps = buildDeps(); + deps.definitionInTenant = async (tenantId, definitionId) => + tenantId === TENANT.id && definitionId === VALID_BODY.definitionId; + const app = mountAs(createRoutineRoutes(deps), "user_1"); + const { response } = await createRoutine(app, VALID_BODY); + expect(response.status).toBe(201); + }); + test("rejects an invalid trigger with a 400", async () => { const deps = buildDeps(); const app = mountAs(createRoutineRoutes(deps), "user_1"); diff --git a/packages/routines/test/store.drizzle.test.ts b/packages/routines/test/store.drizzle.test.ts index c66e2fe9a..93765c643 100644 --- a/packages/routines/test/store.drizzle.test.ts +++ b/packages/routines/test/store.drizzle.test.ts @@ -3,20 +3,18 @@ // `migrations.test.ts`. Runs against its own scratch database, never // the developer's or the walking-skeleton suite's. // -// `store.test.ts` proves `compensateFailedFire`'s compare-and-restore -// against the in-memory store, which is atomic only because JS is -// single-threaded — it says nothing about whether Postgres's own -// timestamp comparison, round-tripped through drizzle, actually -// behaves the same way. This exercises the real `createDrizzleRoutineStore` -// path: an ordinary restore, and the edit-wins case where a concurrent -// trigger change must survive a stale compensation untouched. +// `store.test.ts` proves `markFailedFire`'s conditional write against +// the in-memory store. This exercises the real +// `createDrizzleRoutineStore` path: backoff after failure, and the +// edit-wins case where a concurrent trigger change must survive a +// stale mark untouched. import { afterAll, beforeAll, describe, expect, test } from "bun:test"; import { drizzle } from "drizzle-orm/postgres-js"; import postgres from "postgres"; import { e2eDatabaseUrl } from "../../../scripts/e2e/harness"; import { applyRoutineMigrations } from "../src/migrations"; -import { createDrizzleRoutineStore } from "../src/store"; +import { backoffMsForFailure, createDrizzleRoutineStore } from "../src/store"; function scratchUrlFor(e2eUrl: string): string { const url = new URL(e2eUrl); @@ -36,7 +34,7 @@ function assertDate(value: Date | null): Date { } describeIfDb( - "createDrizzleRoutineStore: claimRoutineFire / compensateFailedFire", + "createDrizzleRoutineStore: claimRoutineFire / markFailedFire", () => { const scratchUrl = scratchUrlFor( databaseUrl ?? "postgres://localhost:5432/unused", @@ -78,7 +76,7 @@ describeIfDb( } }); - test("compensateFailedFire restores nextFireAt when nothing has changed since the claim", async () => { + test("markFailedFire backs off nextFireAt when nothing has changed since the claim", async () => { const sql = postgres(scratchUrl, { max: 1, onnotice: () => undefined }); try { const store = createDrizzleRoutineStore(drizzle(sql)); @@ -95,16 +93,29 @@ describeIfDb( const claimed = await store.claimRoutineFire(routine.id, fireAt); const claimedNextFireAt = assertDate(claimed?.nextFireAt ?? null); - await store.compensateFailedFire(routine.id, fireAt, claimedNextFireAt); + const result = await store.markFailedFire({ + routineId: routine.id, + tenantId: TENANT_ID, + claimedNextFireAt, + failedAt: fireAt, + reason: "launch exploded", + }); + expect(result?.deadLettered).toBe(false); + expect(result?.nextFireAt?.toISOString()).toBe( + new Date(fireAt.getTime() + backoffMsForFailure(1)).toISOString(), + ); - const restored = await store.getRoutine(TENANT_ID, routine.id); - expect(restored?.nextFireAt?.toISOString()).toBe(fireAt.toISOString()); + const after = await store.getRoutine(TENANT_ID, routine.id); + expect(after?.consecutiveFailures).toBe(1); + const runs = await store.listRunsForRoutine(TENANT_ID, routine.id); + expect(runs).toHaveLength(1); + expect(runs[0]?.triggeredBy).toBe("schedule-failed"); } finally { await sql.end(); } }); - test("compensateFailedFire is a no-op when a trigger edit already moved nextFireAt", async () => { + test("markFailedFire is a no-op when a trigger edit already moved nextFireAt", async () => { const sql = postgres(scratchUrl, { max: 1, onnotice: () => undefined }); try { const store = createDrizzleRoutineStore(drizzle(sql)); @@ -121,8 +132,6 @@ describeIfDb( const claimed = await store.claimRoutineFire(routine.id, fireAt); const claimedNextFireAt = assertDate(claimed?.nextFireAt ?? null); - // A trigger edit lands during the failure window, after the - // claim but before the launch's failure is handled. const edited = await store.updateRoutine(TENANT_ID, routine.id, { trigger: { kind: "interval", unit: "minutes", every: 30 }, }); @@ -131,12 +140,17 @@ describeIfDb( claimedNextFireAt.getTime(), ); - // The conditional UPDATE's WHERE no longer matches (nextFireAt - // moved), so this must not clobber the edit's newer value. - await store.compensateFailedFire(routine.id, fireAt, claimedNextFireAt); + const result = await store.markFailedFire({ + routineId: routine.id, + tenantId: TENANT_ID, + claimedNextFireAt, + failedAt: fireAt, + reason: "stale", + }); + expect(result).toBeUndefined(); - const afterCompensation = await store.getRoutine(TENANT_ID, routine.id); - expect(afterCompensation?.nextFireAt?.toISOString()).toBe( + const afterMark = await store.getRoutine(TENANT_ID, routine.id); + expect(afterMark?.nextFireAt?.toISOString()).toBe( editedNextFireAt.toISOString(), ); } finally { diff --git a/packages/routines/test/store.test.ts b/packages/routines/test/store.test.ts index a7988b2a3..2a0d2235d 100644 --- a/packages/routines/test/store.test.ts +++ b/packages/routines/test/store.test.ts @@ -1,208 +1,274 @@ -// Proof of the two guarantees `createInMemoryRoutineStore` shares with -// its drizzle counterpart: a schedule due while nothing was polling -// stays due (survives a restart, "catch-up" not "skip"), and a fire's -// claim is exactly-once even when two schedulers race the same due -// routine. +// Store-level guarantees for the scheduler's claim/fail path: +// listDue filters, atomic claim, backoff + dead-letter on failure. import { describe, expect, test } from "bun:test"; -import { createInMemoryRoutineStore } from "../src/store"; +import { + backoffMsForFailure, + createInMemoryRoutineStore, + MAX_ROUTINE_FIRE_FAILURES, +} from "../src/store"; +import type { RoutineTriggerT } from "../src/trigger"; -const TENANT_ID = "tnt_1"; +const CRON: RoutineTriggerT = { + kind: "cron", + expression: "0 * * * *", +}; -function assertDate(value: Date | null): Date { - if (value === null) throw new Error("expected a non-null Date"); - return value; +async function dueRoutine( + store: ReturnType, + name = "due", +) { + return store.createRoutine({ + tenantId: "t1", + name, + definitionId: "def_1", + trigger: CRON, + scope: "bench", + input: {}, + createdBy: "user_1", + }); } describe("listDueRoutines / claimRoutineFire", () => { - test("a routine's nextFireAt is set on creation", async () => { + test("listDueRoutines returns only enabled, due, non-deleted, non-dead-lettered rows", async () => { const store = createInMemoryRoutineStore(); - const routine = await store.createRoutine({ - tenantId: TENANT_ID, - name: "Every 10 minutes", - definitionId: "def_1", - trigger: { kind: "interval", unit: "minutes", every: 10 }, - scope: "bench", - input: {}, - createdBy: "user_1", + const due = await dueRoutine(store); + // Force nextFireAt into the past so it's due. + await store.updateRoutine("t1", due.id, { + // no-op name touch would recompute nextFire; claim path uses create nextFire }); - expect(routine.nextFireAt).not.toBeNull(); + // Direct claim path: set via claim after making due by creating and + // then claiming at a future time after nextFire is in the past. + // create sets nextFireAt from now; use a far-future `now` for due. + const later = new Date(Date.now() + 60 * 60 * 1000); + // Make another disabled one + const disabled = await dueRoutine(store, "disabled"); + await store.updateRoutine("t1", disabled.id, { enabled: false }); + + const dueList = await store.listDueRoutines(later); + expect(dueList.map((r) => r.id)).toContain(due.id); + expect(dueList.map((r) => r.id)).not.toContain(disabled.id); }); - test("a manual routine never becomes due", async () => { + test("claimRoutineFire advances nextFireAt and stamps lastFireAt", async () => { const store = createInMemoryRoutineStore(); - await store.createRoutine({ - tenantId: TENANT_ID, - name: "Manual only", - definitionId: "def_1", - trigger: null, - scope: "bench", - input: {}, - createdBy: "user_1", - }); - const due = await store.listDueRoutines(new Date("2099-01-01T00:00:00Z")); - expect(due).toHaveLength(0); + const routine = await dueRoutine(store); + // Ensure due: listDue with far future should include it if nextFireAt <= fireAt + // create's nextFireAt is soon; use now for claim if already due, else future. + const now = new Date( + Math.max(Date.now(), routine.nextFireAt?.getTime() ?? 0), + ); + const claimed = await store.claimRoutineFire(routine.id, now); + expect(claimed).toBeDefined(); + if (!claimed) throw new Error("expected claimed routine"); + expect(claimed.lastFireAt?.getTime()).toBe(now.getTime()); + const claimedNext0 = claimed.nextFireAt; + if (!claimedNext0) throw new Error("expected nextFireAt after claim"); + expect(claimedNext0.getTime()).toBeGreaterThan(now.getTime()); }); - test("a fire due while nothing polled stays due — no skip, only catch-up", async () => { + test("a second concurrent claim loses", async () => { const store = createInMemoryRoutineStore(); - const routine = await store.createRoutine({ - tenantId: TENANT_ID, - name: "Hourly", - definitionId: "def_1", - trigger: { kind: "interval", unit: "hours", every: 1 }, - scope: "bench", - input: {}, - createdBy: "user_1", - }); - const scheduledFireAt = assertDate(routine.nextFireAt); - - // Simulate the hub being down through the scheduled fire and well - // past it — a naive "does this exact minute match" scheduler would - // never fire this routine again once that minute has passed. - const restartedAt = new Date(scheduledFireAt.getTime() + 4 * 3_600_000); - const due = await store.listDueRoutines(restartedAt); - expect(due.map((row) => row.id)).toContain(routine.id); + const routine = await dueRoutine(store); + const now = new Date( + Math.max(Date.now(), routine.nextFireAt?.getTime() ?? 0), + ); + const [a, b] = await Promise.all([ + store.claimRoutineFire(routine.id, now), + store.claimRoutineFire(routine.id, now), + ]); + const winners = [a, b].filter((x) => x !== undefined); + expect(winners).toHaveLength(1); }); - test("claiming a due fire advances nextFireAt to the following occurrence", async () => { + test("claim refuses a soft-deleted routine", async () => { const store = createInMemoryRoutineStore(); - const routine = await store.createRoutine({ - tenantId: TENANT_ID, - name: "Hourly", - definitionId: "def_1", - trigger: { kind: "interval", unit: "hours", every: 1 }, - scope: "bench", - input: {}, - createdBy: "user_1", - }); + const routine = await dueRoutine(store); + const now = new Date( + Math.max(Date.now(), routine.nextFireAt?.getTime() ?? 0), + ); + await store.deleteRoutine("t1", routine.id); + expect(await store.claimRoutineFire(routine.id, now)).toBeUndefined(); + }); +}); - // Claiming well after the scheduled fire (catching up a missed one) - // still advances from the claim moment, not from the missed slot. +describe("markFailedFire / clearFireFailures", () => { + test("markFailedFire backs off nextFireAt and records a schedule-failed run", async () => { + const store = createInMemoryRoutineStore(); + const routine = await dueRoutine(store); const fireAt = new Date( - assertDate(routine.nextFireAt).getTime() + 4 * 3_600_000, + Math.max(Date.now(), routine.nextFireAt?.getTime() ?? 0), ); const claimed = await store.claimRoutineFire(routine.id, fireAt); - expect(claimed?.lastFireAt?.toISOString()).toBe(fireAt.toISOString()); - expect(claimed?.nextFireAt?.toISOString()).toBe( - new Date(fireAt.getTime() + 3_600_000).toISOString(), - ); - }); + expect(claimed).toBeDefined(); + if (!claimed) throw new Error("expected claimed routine"); + const claimedNext = claimed.nextFireAt; + if (!claimedNext) throw new Error("expected nextFireAt after claim"); - test("a second concurrent claim of the same fire loses", async () => { - const store = createInMemoryRoutineStore(); - const routine = await store.createRoutine({ - tenantId: TENANT_ID, - name: "Hourly", - definitionId: "def_1", - trigger: { kind: "interval", unit: "hours", every: 1 }, - scope: "bench", - input: {}, - createdBy: "user_1", + const result = await store.markFailedFire({ + routineId: routine.id, + tenantId: "t1", + claimedNextFireAt: claimedNext, + failedAt: fireAt, + reason: "launch exploded", }); + expect(result).toBeDefined(); + if (!result) throw new Error("expected markFailedFire result"); + expect(result.deadLettered).toBe(false); + expect(result.consecutiveFailures).toBe(1); + const resultNext = result.nextFireAt; + if (!resultNext) throw new Error("expected nextFireAt after failure"); + expect(resultNext.getTime()).toBe( + fireAt.getTime() + backoffMsForFailure(1), + ); - const fireAt = assertDate(routine.nextFireAt); - const [first, second] = await Promise.all([ - store.claimRoutineFire(routine.id, fireAt), - store.claimRoutineFire(routine.id, fireAt), - ]); - const winners = [first, second].filter((row) => row !== undefined); - expect(winners).toHaveLength(1); + const runs = await store.listRunsForRoutine("t1", routine.id); + expect(runs).toHaveLength(1); + expect(runs[0]?.triggeredBy).toBe("schedule-failed"); + expect(runs[0]?.error).toBe("launch exploded"); }); - test("a disabled routine is never claimable even if nextFireAt is due", async () => { + test("markFailedFire is a no-op when nextFireAt already moved", async () => { const store = createInMemoryRoutineStore(); - const routine = await store.createRoutine({ - tenantId: TENANT_ID, - name: "Paused", - definitionId: "def_1", - trigger: { kind: "interval", unit: "minutes", every: 5 }, - scope: "bench", - input: {}, - createdBy: "user_1", - }); - await store.updateRoutine(TENANT_ID, routine.id, { enabled: false }); - - const due = await store.listDueRoutines(new Date("2099-01-01T00:00:00Z")); - expect(due).toHaveLength(0); - const claimed = await store.claimRoutineFire( - routine.id, - new Date("2099-01-01T00:00:00Z"), + const routine = await dueRoutine(store); + const fireAt = new Date( + Math.max(Date.now(), routine.nextFireAt?.getTime() ?? 0), ); - expect(claimed).toBeUndefined(); - }); + const claimed = await store.claimRoutineFire(routine.id, fireAt); + if (!claimed) throw new Error("expected claimed routine"); + const claimedNext = claimed.nextFireAt; + if (!claimedNext) throw new Error("expected nextFireAt after claim"); - test("re-enabling a routine recomputes nextFireAt from now, not from the stale value", async () => { - const store = createInMemoryRoutineStore(); - const routine = await store.createRoutine({ - tenantId: TENANT_ID, - name: "Toggle", - definitionId: "def_1", - trigger: { kind: "interval", unit: "minutes", every: 5 }, - scope: "bench", - input: {}, - createdBy: "user_1", + // Operator edits the trigger, moving nextFireAt. + await store.updateRoutine("t1", routine.id, { + trigger: { kind: "cron", expression: "30 * * * *" }, }); - await store.updateRoutine(TENANT_ID, routine.id, { enabled: false }); - const reEnabled = await store.updateRoutine(TENANT_ID, routine.id, { - enabled: true, + const afterEdit = await store.getRoutine("t1", routine.id); + if (!afterEdit) throw new Error("expected routine after edit"); + const afterEditNext = afterEdit.nextFireAt; + if (!afterEditNext) throw new Error("expected nextFireAt after edit"); + expect(afterEditNext.getTime()).not.toBe(claimedNext.getTime()); + + const result = await store.markFailedFire({ + routineId: routine.id, + tenantId: "t1", + claimedNextFireAt: claimedNext, + failedAt: fireAt, + reason: "stale", }); - expect(reEnabled.nextFireAt).not.toBeNull(); - expect(reEnabled.nextFireAt?.getTime()).toBeGreaterThan(Date.now()); + expect(result).toBeUndefined(); + + const still = await store.getRoutine("t1", routine.id); + if (!still) throw new Error("expected routine after no-op fail"); + const stillNext = still.nextFireAt; + if (!stillNext) throw new Error("expected nextFireAt after no-op fail"); + expect(stillNext.getTime()).toBe(afterEditNext.getTime()); + expect(still.consecutiveFailures).toBe(0); }); - test("compensateFailedFire restores nextFireAt when nothing has changed since the claim", async () => { + test("after MAX failures the routine is dead-lettered and no longer due", async () => { const store = createInMemoryRoutineStore(); - const routine = await store.createRoutine({ - tenantId: TENANT_ID, - name: "Hourly", - definitionId: "def_1", - trigger: { kind: "interval", unit: "hours", every: 1 }, - scope: "bench", - input: {}, - createdBy: "user_1", - }); - const fireAt = assertDate(routine.nextFireAt); - const claimed = assertDate( - (await store.claimRoutineFire(routine.id, fireAt))?.nextFireAt ?? null, + const routine = await dueRoutine(store); + let clock = new Date( + Math.max(Date.now(), routine.nextFireAt?.getTime() ?? 0), ); - await store.compensateFailedFire(routine.id, fireAt, claimed); + for (let i = 0; i < MAX_ROUTINE_FIRE_FAILURES; i++) { + // Make due at clock by claiming only when nextFireAt <= clock. + // After each failure, nextFireAt is clock + backoff; advance clock. + const current = await store.getRoutine("t1", routine.id); + if (!current) throw new Error("expected routine in failure loop"); + if (current.nextFireAt !== null) { + clock = new Date( + Math.max(clock.getTime(), current.nextFireAt.getTime()), + ); + } + const claimed = await store.claimRoutineFire(routine.id, clock); + expect(claimed).toBeDefined(); + if (!claimed) throw new Error("expected claimed routine"); + const claimedNext = claimed.nextFireAt; + if (!claimedNext) throw new Error("expected nextFireAt after claim"); + const result = await store.markFailedFire({ + routineId: routine.id, + tenantId: "t1", + claimedNextFireAt: claimedNext, + failedAt: clock, + reason: `fail ${i + 1}`, + }); + if (!result) throw new Error("expected markFailedFire result"); + if (i < MAX_ROUTINE_FIRE_FAILURES - 1) { + expect(result.deadLettered).toBe(false); + } else { + expect(result.deadLettered).toBe(true); + expect(result.nextFireAt).toBeNull(); + } + } - const restored = await store.getRoutine(TENANT_ID, routine.id); - expect(restored?.nextFireAt?.toISOString()).toBe(fireAt.toISOString()); + const final = await store.getRoutine("t1", routine.id); + if (!final) throw new Error("expected routine after dead-letter"); + expect(final.deadLetteredAt).not.toBeNull(); + expect(final.consecutiveFailures).toBe(MAX_ROUTINE_FIRE_FAILURES); + expect( + await store.listDueRoutines(new Date(clock.getTime() + 1e12)), + ).toEqual([]); + expect(await store.claimRoutineFire(routine.id, clock)).toBeUndefined(); }); - test("compensateFailedFire is a no-op when a trigger edit already moved nextFireAt", async () => { + test("clearFireFailures resets counters after a successful fire", async () => { const store = createInMemoryRoutineStore(); - const routine = await store.createRoutine({ - tenantId: TENANT_ID, - name: "Hourly", - definitionId: "def_1", - trigger: { kind: "interval", unit: "hours", every: 1 }, - scope: "bench", - input: {}, - createdBy: "user_1", - }); - const fireAt = assertDate(routine.nextFireAt); - const claimedResult = await store.claimRoutineFire(routine.id, fireAt); - const claimedNextFireAt = assertDate(claimedResult?.nextFireAt ?? null); - - // A trigger edit lands during the failure window, after the claim - // but before the launch's failure is handled — this already gave - // the routine a fresh, unrelated nextFireAt. - const edited = await store.updateRoutine(TENANT_ID, routine.id, { - trigger: { kind: "interval", unit: "minutes", every: 30 }, + const routine = await dueRoutine(store); + const fireAt = new Date( + Math.max(Date.now(), routine.nextFireAt?.getTime() ?? 0), + ); + const claimed = await store.claimRoutineFire(routine.id, fireAt); + if (!claimed) throw new Error("expected claimed routine"); + const claimedNext = claimed.nextFireAt; + if (!claimedNext) throw new Error("expected nextFireAt after claim"); + await store.markFailedFire({ + routineId: routine.id, + tenantId: "t1", + claimedNextFireAt: claimedNext, + failedAt: fireAt, + reason: "once", }); - const editedNextFireAt = assertDate(edited.nextFireAt); - expect(editedNextFireAt.getTime()).not.toBe(claimedNextFireAt.getTime()); - - // Compensation must not clobber that newer value with the stale - // one computed from the pre-edit trigger. - await store.compensateFailedFire(routine.id, fireAt, claimedNextFireAt); + await store.clearFireFailures(routine.id); + const cleared = await store.getRoutine("t1", routine.id); + if (!cleared) throw new Error("expected routine after clearFireFailures"); + expect(cleared.consecutiveFailures).toBe(0); + expect(cleared.deadLetteredAt).toBeNull(); + }); - const afterCompensation = await store.getRoutine(TENANT_ID, routine.id); - expect(afterCompensation?.nextFireAt?.toISOString()).toBe( - editedNextFireAt.toISOString(), + test("re-enabling a dead-lettered routine clears the dead-letter", async () => { + const store = createInMemoryRoutineStore(); + const routine = await dueRoutine(store); + let clock = new Date( + Math.max(Date.now(), routine.nextFireAt?.getTime() ?? 0), ); + for (let i = 0; i < MAX_ROUTINE_FIRE_FAILURES; i++) { + const current = await store.getRoutine("t1", routine.id); + if (!current) throw new Error("expected routine in failure loop"); + if (current.nextFireAt !== null) { + clock = new Date( + Math.max(clock.getTime(), current.nextFireAt.getTime()), + ); + } + const claimed = await store.claimRoutineFire(routine.id, clock); + if (!claimed) throw new Error("expected claimed routine"); + const claimedNext = claimed.nextFireAt; + if (!claimedNext) throw new Error("expected nextFireAt after claim"); + await store.markFailedFire({ + routineId: routine.id, + tenantId: "t1", + claimedNextFireAt: claimedNext, + failedAt: clock, + reason: `fail ${i + 1}`, + }); + } + + const recovered = await store.updateRoutine("t1", routine.id, { + enabled: true, + }); + expect(recovered.deadLetteredAt).toBeNull(); + expect(recovered.consecutiveFailures).toBe(0); + expect(recovered.nextFireAt).not.toBeNull(); }); }); diff --git a/packages/webhook-triggers/src/management-routes.ts b/packages/webhook-triggers/src/management-routes.ts index 33763fd0f..4b68efccd 100644 --- a/packages/webhook-triggers/src/management-routes.ts +++ b/packages/webhook-triggers/src/management-routes.ts @@ -56,6 +56,14 @@ function publicView(row: WebhookTriggerRow) { export type CreateWebhookTriggerRoutesDeps = { store: WebhookTriggerStore; requireGrant: RequireGrant; + /** + * When provided, `POST /` rejects with 404 if the workflow definition + * is not in the request tenant. Tests may omit (always-allow). + */ + workflowDefinitionInTenant?: ( + tenantId: string, + definitionId: string, + ) => Promise; }; export function createWebhookTriggerRoutes( @@ -74,6 +82,17 @@ export function createWebhookTriggerRoutes( const tenant = c.get("tenant"); const principal = c.get("principal"); + + if (deps.workflowDefinitionInTenant !== undefined) { + const owned = await deps.workflowDefinitionInTenant( + tenant.id, + body.workflowDefinitionId, + ); + if (!owned) { + return c.json(ErrorEnvelope("not_found", "definition not found"), 404); + } + } + const secret = generateWebhookSecret(); const row = await deps.store.create({