fix(services): DbJobAdapter.replay() honours recordRuns — run history off stops accumulating replay rows

.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.

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<typeof makeFakeEngine>;
+ 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();
  });
 });

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,14 +153,30 @@ 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<void> {
  // Same execution path as trigger but tag the run as 'replay'.
  const handlers = (this.inner as any).jobs?.get?.(name);
  if (!handlers) throw new Error(`Job "${name}" not found`);
  // 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;
  }
  }