diff --git a/.changeset/service-job-replay-honours-recordruns.md b/.changeset/service-job-replay-honours-recordruns.md new file mode 100644 index 0000000000..329dbebc1e --- /dev/null +++ b/.changeset/service-job-replay-honours-recordruns.md @@ -0,0 +1,34 @@ +--- +"@objectstack/service-job": patch +--- + +fix(services): `DbJobAdapter.replay()` honours `recordRuns` — an operator who switched run history off stops accumulating replay rows (#9633) + +`recordRuns` is the on/off switch for `sys_job_run` history, and it had exactly +two `startRun` call sites. The gate landed on one of them: `wrap()`'s +per-attempt row was gated, `replay()`'s synthetic row was not. So a deployment +that set `recordRuns: false` wrote nothing for any scheduled or triggered +execution and **one complete row for every replay** — a table the operator +believes is switched off, filling slowly and exclusively with `trigger: 'replay'` +rows, the least representative sample of a job's history and one with no +non-replay rows beside it for context. + +The carve-out was never designed. `replay()`'s synthetic row exists to force the +`trigger: 'replay'` tag that `IJobService.trigger` cannot carry back; the flag +simply arrived later and landed on one of the two writers. It is closed rather +than documented: one flag, one meaning, no second de-facto rule at a call site. +If an operator-initiated replay needs to be auditable when routine history is +off, the principled home for that is `sys_audit_log` — which has its own opt-in, +writer and retention — not an exception to a history switch. + +**Behaviour change, user-visible:** with `recordRuns: false`, `replay()` now +writes no `sys_job_run` row. The handler still executes, and `sys_job`'s own +`last_run_at` / `last_status` / `run_count` / `failure_count` counters still +update — the flag has never gated those. With the default (`true`) nothing +changes: the synthetic row is still written, still tagged `trigger: 'replay'`, +and still carries the terminal status read off the inner execution. + +All three of `replay()`'s arms are gated, not just the insert — the terminal +status arm, the success arm and the catch arm — so the flag cannot leave a +dangling `running` half-row with no `completed_at`, which would be worse than +either original behaviour. diff --git a/packages/services/service-job/src/db-job-adapter.test.ts b/packages/services/service-job/src/db-job-adapter.test.ts index 111eadcaa4..c21b6ce2b8 100644 --- a/packages/services/service-job/src/db-job-adapter.test.ts +++ b/packages/services/service-job/src/db-job-adapter.test.ts @@ -302,21 +302,145 @@ describe('DbJobAdapter — recordRuns (#9631)', () => { expect(runs[0]).toMatchObject({ job_name: 'on', trigger: 'schedule', status: 'success' }); }); - it("replay() writes its synthetic row even when recordRuns is false — the exception the JSDoc names", async () => { - // This pins TODAY'S behaviour, which is what the class JSDoc now states; - // it is not an endorsement of it. #9633 holds the open disposition on - // whether `replay()` should honour the flag. If that lands, this case and - // the class-JSDoc bullet it mirrors change together — which is the whole - // point of writing it down here: the sentence cannot go stale in silence - // again. - const { engine, adapter } = build({ recordRuns: false }); - await adapter.schedule('rp', { type: 'cron', expression: '* * * * *' }, async () => {}); - await adapter.replay('rp'); + // ─── the fifth case that stood here is GONE, by design ─── + // It pinned "replay() writes its synthetic row even when recordRuns is + // false" — today's behaviour as the corrected JSDoc then stated it, never an + // endorsement — and it carried a comment saying it would change together + // with that JSDoc if #9633 ruled the carve-out shut. It did: the exception + // was an artifact of the gate landing on one of two `startRun` call sites, + // so `replay()` now honours the flag and this assertion became false. + // Deleting it IS the coupling that case was written to force. Its + // replacement is the block below, pinning the opposite direction on all + // three of replay()'s arms. +}); - const runs = engine.tables.get('sys_job_run') ?? []; - // Exactly one: the synthetic replay row. The wrapped execution the replay - // drives underneath it is gated by the flag and writes nothing. - expect(runs.map((r) => r.trigger)).toEqual(['replay']); - expect(runs[0].status).toBe('success'); +// ─── #9633 — replay() honours `recordRuns`, on all three of its arms ───────── +// +// `recordRuns` had exactly two `startRun` call sites and the gate landed on one +// of them: `wrap`'s per-attempt row was gated, `replay`'s synthetic row was not. +// An operator who switched run history off therefore kept accumulating rows — +// exclusively replay ones, the least representative sample of a job's history, +// with no non-replay rows beside them for context. The carve-out was an +// artifact of `replay` being written to solve a different problem (#5548's +// synthetic row forces the `trigger: 'replay'` tag), never a designed exception, +// so it is closed rather than documented (the ruling on #9633). +// +// ⚠️ Nothing in this package referenced `recordRuns` in ANY direction before +// #9631 — the flag could have stopped being honoured entirely and the suite +// would not have noticed. These cases are written so that cannot happen again: +// each flag-off case asserts the execution REALLY RAN (handler counter plus the +// `sys_job` counters, which the flag deliberately does not gate) alongside the +// "no rows" assertion, so "0 rows" cannot pass for a job that never ran; and +// the flag-on cases assert the synthetic row reached a TERMINAL status, so +// over-gating the three `finishRun` arms — a dangling `running` half-row, worse +// than either original state — goes red instead of silently passing "a row +// exists". +describe('DbJobAdapter — replay() honours recordRuns (#9633)', () => { + let engine: ReturnType; + const adapters: DbJobAdapter[] = []; + + function makeAdapter(options?: { recordRuns?: boolean }) { + const a = new DbJobAdapter({ engine, options }); + adapters.push(a); + return a; + } + + beforeEach(() => { + engine = makeFakeEngine(); + adapters.length = 0; + }); + afterEach(async () => { + for (const a of adapters) await a.destroy(); + }); + + const runs = () => engine.tables.get('sys_job_run') ?? []; + const job = () => (engine.tables.get('sys_job') ?? [])[0]; + + it('recordRuns: false — a replay runs the handler and writes NO sys_job_run row', async () => { + const adapter = makeAdapter({ recordRuns: false }); + let ran = 0; + await adapter.schedule('quiet', { type: 'cron', expression: '* * * * *' }, async () => { ran += 1; }); + + await adapter.replay('quiet'); + + // The discriminator: neither the synthetic replay row nor the wrapped + // per-attempt row is written. Ungated, this table holds a `trigger: + // 'replay'` row here. + expect(runs()).toHaveLength(0); + // …and the execution really happened, so "0 rows" cannot be passing for a + // job that never ran. `bumpJob` is called outside the row gate by design. + expect(ran).toBe(1); + expect(job().run_count).toBe(1); + expect(job().last_status).toBe('success'); + // README.md's options table — "`false` keeps the in-memory history only" — + // was falsified by exactly the replay row this case now forbids. Pinned + // here rather than restated in prose: the durable table is empty and the + // in-memory history the sentence promises is still there. + expect((await adapter.getExecutions('quiet')).length).toBeGreaterThan(0); + }); + + it('recordRuns: false — the terminal-status arm writes nothing either, and leaves no dangling row', async () => { + const adapter = makeAdapter({ recordRuns: false }); + let ran = 0; + await adapter.schedule('sour', { type: 'cron', expression: '* * * * *' }, async () => { + ran += 1; + throw new Error('replayed and failed'); + }); + + await adapter.replay('sour'); + + // The arm that reads the terminal status off the inner execution (#7734). + // Half-gating — suppressing `startRun` but not `finishRun`, or the reverse + // — is what would leave a `running` row with no `completed_at`. + expect(runs()).toHaveLength(0); + expect(ran).toBe(1); + expect(job().last_status).toBe('failed'); + expect(job().failure_count).toBe(1); + }); + + it('recordRuns: false — the catch arm writes nothing and still rethrows', async () => { + const adapter = makeAdapter({ recordRuns: false }); + await adapter.schedule('boom', { type: 'cron', expression: '* * * * *' }, async () => {}); + // `executeJob` swallows a handler throw, so the catch arm is unreachable + // through the handler — the inner call itself has to reject for it to run. + const inner = (adapter as any).inner; + inner.trigger = async () => { throw new Error('inner exploded'); }; + + await expect(adapter.replay('boom')).rejects.toThrow('inner exploded'); + + expect(runs()).toHaveLength(0); + }); + + it('default recordRuns — the synthetic replay row is still written, and SETTLED', async () => { + const adapter = makeAdapter(); // no options at all: the flag defaults to true + await adapter.schedule('loud', { type: 'cron', expression: '* * * * *' }, async () => {}); + + await adapter.replay('loud'); + + // The #5548 tag survives the gate: one synthetic replay row beside the + // wrapped run the execution itself produced. + const replayRows = runs().filter((r: any) => r.trigger === 'replay'); + expect(replayRows).toHaveLength(1); + // Terminal, not left `running` — this is what goes red if the three + // `finishRun` arms are over-gated along with `startRun`. + expect(replayRows[0].status).toBe('success'); + expect(replayRows[0].completed_at).toBeTruthy(); + expect(runs().map((r: any) => r.trigger).sort()).toEqual(['replay', 'schedule']); + }); + + it('recordRuns: true — the replay row still carries the terminal status of the inner execution', async () => { + const adapter = makeAdapter({ recordRuns: true }); // explicit, matching the default + await adapter.schedule('sour', { type: 'cron', expression: '* * * * *' }, async () => { + throw new Error('replayed and failed'); + }); + + await adapter.replay('sour'); + + const replayRows = runs().filter((r: any) => r.trigger === 'replay'); + expect(replayRows).toHaveLength(1); + // #7734: read off the inner execution, not assumed `success`. + expect(replayRows[0].status).toBe('failed'); + expect(replayRows[0].error).toBe('replayed and failed'); + expect(replayRows[0].completed_at).toBeTruthy(); }); }); diff --git a/packages/services/service-job/src/db-job-adapter.ts b/packages/services/service-job/src/db-job-adapter.ts index 9185dccd30..7ae0928ca8 100644 --- a/packages/services/service-job/src/db-job-adapter.ts +++ b/packages/services/service-job/src/db-job-adapter.ts @@ -37,12 +37,11 @@ export interface DbJobAdapterOptions { * when that attempt settles. Default **`true`**. * * This is an on/off switch for run history, NOT a retention cap: setting it to - * `false` means no per-attempt rows are written at all, so `sys_job_run` holds - * nothing for these executions and `listExecutionsByStatus` has nothing to - * read. Two things are unaffected either way — the `sys_job` row's own - * `last_status` / `run_count` / `failure_count` counters, and - * {@link DbJobAdapter.replay}, which writes its synthetic `trigger: 'replay'` - * row regardless of this flag. + * `false` means no rows are written at all — not the per-attempt rows above, + * and not {@link DbJobAdapter.replay}'s synthetic `trigger: 'replay'` row — + * so `sys_job_run` stays empty for this adapter and `listExecutionsByStatus` + * has nothing to read. The one thing unaffected either way is the `sys_job` + * row's own `last_status` / `run_count` / `failure_count` counters. */ recordRuns?: boolean; } @@ -83,9 +82,8 @@ function uid(prefix: string): string { * - `cancel(name)` marks the row inactive * - every execution writes a `sys_job_run` row per attempt — unless * {@link DbJobAdapterOptions.recordRuns} is `false`, the on/off switch for - * run history, which writes none of them. The one row it does not govern is - * {@link DbJobAdapter.replay}'s synthetic `trigger: 'replay'` row, written - * either way. + * run history, which writes none of them, {@link DbJobAdapter.replay}'s + * synthetic `trigger: 'replay'` row included. * - every execution updates `sys_job.last_run_at / last_status / run_count / * failure_count` — unconditionally: `recordRuns` gates the per-attempt rows * above, never these counters. @@ -155,6 +153,16 @@ export class DbJobAdapter implements IJobService { return this.inner.listJobs(); } + /** + * Replay a job's most recent execution, tagging its run `trigger: 'replay'`. + * + * The synthetic `sys_job_run` row this writes is governed by + * {@link DbJobAdapterOptions.recordRuns} exactly as every other run row is: + * with the flag `false` the handler still runs and no row is written — not + * this synthetic one, and not the per-attempt row the execution itself + * would produce. `sys_job_run` is run history, not an audit trail; an + * operator who switched history off gets nothing durable from this path. + */ async replay(name: string, data?: unknown): Promise { // Same execution path as trigger but tag the run as 'replay'. const handlers = (this.inner as any).jobs?.get?.(name); @@ -162,7 +170,13 @@ export class DbJobAdapter implements IJobService { // Reuse trigger; the wrap function uses a closure flag — simpler: // expose by calling inner.trigger with a marker via data is intrusive, // so we record a synthetic run row before/after to ensure 'replay' tag. - const runId = await this.startRun(name, 'replay'); + // + // Gated exactly as `wrap`'s per-attempt row is: `recordRuns` is the on/off + // switch for run history and this row is run history, so an operator who + // turned it off gets no replay rows either. Ungated, this was the one write + // that ignored the flag — the artifact of the gate landing on one of two + // `startRun` call sites, never a designed carve-out for replay. + const runId = this.recordRuns ? await this.startRun(name, 'replay') : undefined; try { await this.inner.trigger(name, data); // The wrap already recorded a run; settle our synthetic row the same way @@ -179,12 +193,12 @@ export class DbJobAdapter implements IJobService { const [last] = await this.inner.getExecutions(name, 1); const status = last?.status; if (status === 'degraded' || status === 'timeout' || status === 'failed') { - await this.finishRun(runId, status, last.error); + if (runId) await this.finishRun(runId, status, last.error); } else { - await this.finishRun(runId, 'success'); + if (runId) await this.finishRun(runId, 'success'); } } catch (err) { - await this.finishRun(runId, 'failed', err instanceof Error ? err.message : String(err)); + if (runId) await this.finishRun(runId, 'failed', err instanceof Error ? err.message : String(err)); throw err; } }