Skip to content

Commit 52182a6

Browse files
fix(services): DbJobAdapter.replay() honours recordRuns — run history off stops accumulating replay rows (#9675)
* fix(services): DbJobAdapter.replay() honours recordRuns (#9633) `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 still accumulated one complete row per replay. All three of `replay()`'s `finishRun` arms are gated alongside the insert — the terminal-status arm, the success arm and the catch arm — so the flag cannot leave a dangling `running` half-row. Five pins added; the package referenced `recordRuns` in no direction before this. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y26DJEHSBhhAQ6wwfsHNza * test(services): pin the README's in-memory-history sentence on the replay path (#9633) Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y26DJEHSBhhAQ6wwfsHNza --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 96f397a commit 52182a6

3 files changed

Lines changed: 200 additions & 28 deletions

File tree

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
---
2+
"@objectstack/service-job": patch
3+
---
4+
5+
fix(services): `DbJobAdapter.replay()` honours `recordRuns` — an operator who switched run history off stops accumulating replay rows (#9633)
6+
7+
`recordRuns` is the on/off switch for `sys_job_run` history, and it had exactly
8+
two `startRun` call sites. The gate landed on one of them: `wrap()`'s
9+
per-attempt row was gated, `replay()`'s synthetic row was not. So a deployment
10+
that set `recordRuns: false` wrote nothing for any scheduled or triggered
11+
execution and **one complete row for every replay** — a table the operator
12+
believes is switched off, filling slowly and exclusively with `trigger: 'replay'`
13+
rows, the least representative sample of a job's history and one with no
14+
non-replay rows beside it for context.
15+
16+
The carve-out was never designed. `replay()`'s synthetic row exists to force the
17+
`trigger: 'replay'` tag that `IJobService.trigger` cannot carry back; the flag
18+
simply arrived later and landed on one of the two writers. It is closed rather
19+
than documented: one flag, one meaning, no second de-facto rule at a call site.
20+
If an operator-initiated replay needs to be auditable when routine history is
21+
off, the principled home for that is `sys_audit_log` — which has its own opt-in,
22+
writer and retention — not an exception to a history switch.
23+
24+
**Behaviour change, user-visible:** with `recordRuns: false`, `replay()` now
25+
writes no `sys_job_run` row. The handler still executes, and `sys_job`'s own
26+
`last_run_at` / `last_status` / `run_count` / `failure_count` counters still
27+
update — the flag has never gated those. With the default (`true`) nothing
28+
changes: the synthetic row is still written, still tagged `trigger: 'replay'`,
29+
and still carries the terminal status read off the inner execution.
30+
31+
All three of `replay()`'s arms are gated, not just the insert — the terminal
32+
status arm, the success arm and the catch arm — so the flag cannot leave a
33+
dangling `running` half-row with no `completed_at`, which would be worse than
34+
either original behaviour.

packages/services/service-job/src/db-job-adapter.test.ts

Lines changed: 139 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -302,21 +302,145 @@ describe('DbJobAdapter — recordRuns (#9631)', () => {
302302
expect(runs[0]).toMatchObject({ job_name: 'on', trigger: 'schedule', status: 'success' });
303303
});
304304

305-
it("replay() writes its synthetic row even when recordRuns is false — the exception the JSDoc names", async () => {
306-
// This pins TODAY'S behaviour, which is what the class JSDoc now states;
307-
// it is not an endorsement of it. #9633 holds the open disposition on
308-
// whether `replay()` should honour the flag. If that lands, this case and
309-
// the class-JSDoc bullet it mirrors change together — which is the whole
310-
// point of writing it down here: the sentence cannot go stale in silence
311-
// again.
312-
const { engine, adapter } = build({ recordRuns: false });
313-
await adapter.schedule('rp', { type: 'cron', expression: '* * * * *' }, async () => {});
314-
await adapter.replay('rp');
305+
// ─── the fifth case that stood here is GONE, by design ───
306+
// It pinned "replay() writes its synthetic row even when recordRuns is
307+
// false" — today's behaviour as the corrected JSDoc then stated it, never an
308+
// endorsement — and it carried a comment saying it would change together
309+
// with that JSDoc if #9633 ruled the carve-out shut. It did: the exception
310+
// was an artifact of the gate landing on one of two `startRun` call sites,
311+
// so `replay()` now honours the flag and this assertion became false.
312+
// Deleting it IS the coupling that case was written to force. Its
313+
// replacement is the block below, pinning the opposite direction on all
314+
// three of replay()'s arms.
315+
});
315316

316-
const runs = engine.tables.get('sys_job_run') ?? [];
317-
// Exactly one: the synthetic replay row. The wrapped execution the replay
318-
// drives underneath it is gated by the flag and writes nothing.
319-
expect(runs.map((r) => r.trigger)).toEqual(['replay']);
320-
expect(runs[0].status).toBe('success');
317+
// ─── #9633 — replay() honours `recordRuns`, on all three of its arms ─────────
318+
//
319+
// `recordRuns` had exactly two `startRun` call sites and the gate landed on one
320+
// of them: `wrap`'s per-attempt row was gated, `replay`'s synthetic row was not.
321+
// An operator who switched run history off therefore kept accumulating rows —
322+
// exclusively replay ones, the least representative sample of a job's history,
323+
// with no non-replay rows beside them for context. The carve-out was an
324+
// artifact of `replay` being written to solve a different problem (#5548's
325+
// synthetic row forces the `trigger: 'replay'` tag), never a designed exception,
326+
// so it is closed rather than documented (the ruling on #9633).
327+
//
328+
// ⚠️ Nothing in this package referenced `recordRuns` in ANY direction before
329+
// #9631 — the flag could have stopped being honoured entirely and the suite
330+
// would not have noticed. These cases are written so that cannot happen again:
331+
// each flag-off case asserts the execution REALLY RAN (handler counter plus the
332+
// `sys_job` counters, which the flag deliberately does not gate) alongside the
333+
// "no rows" assertion, so "0 rows" cannot pass for a job that never ran; and
334+
// the flag-on cases assert the synthetic row reached a TERMINAL status, so
335+
// over-gating the three `finishRun` arms — a dangling `running` half-row, worse
336+
// than either original state — goes red instead of silently passing "a row
337+
// exists".
338+
describe('DbJobAdapter — replay() honours recordRuns (#9633)', () => {
339+
let engine: ReturnType<typeof makeFakeEngine>;
340+
const adapters: DbJobAdapter[] = [];
341+
342+
function makeAdapter(options?: { recordRuns?: boolean }) {
343+
const a = new DbJobAdapter({ engine, options });
344+
adapters.push(a);
345+
return a;
346+
}
347+
348+
beforeEach(() => {
349+
engine = makeFakeEngine();
350+
adapters.length = 0;
351+
});
352+
afterEach(async () => {
353+
for (const a of adapters) await a.destroy();
354+
});
355+
356+
const runs = () => engine.tables.get('sys_job_run') ?? [];
357+
const job = () => (engine.tables.get('sys_job') ?? [])[0];
358+
359+
it('recordRuns: false — a replay runs the handler and writes NO sys_job_run row', async () => {
360+
const adapter = makeAdapter({ recordRuns: false });
361+
let ran = 0;
362+
await adapter.schedule('quiet', { type: 'cron', expression: '* * * * *' }, async () => { ran += 1; });
363+
364+
await adapter.replay('quiet');
365+
366+
// The discriminator: neither the synthetic replay row nor the wrapped
367+
// per-attempt row is written. Ungated, this table holds a `trigger:
368+
// 'replay'` row here.
369+
expect(runs()).toHaveLength(0);
370+
// …and the execution really happened, so "0 rows" cannot be passing for a
371+
// job that never ran. `bumpJob` is called outside the row gate by design.
372+
expect(ran).toBe(1);
373+
expect(job().run_count).toBe(1);
374+
expect(job().last_status).toBe('success');
375+
// README.md's options table — "`false` keeps the in-memory history only" —
376+
// was falsified by exactly the replay row this case now forbids. Pinned
377+
// here rather than restated in prose: the durable table is empty and the
378+
// in-memory history the sentence promises is still there.
379+
expect((await adapter.getExecutions('quiet')).length).toBeGreaterThan(0);
380+
});
381+
382+
it('recordRuns: false — the terminal-status arm writes nothing either, and leaves no dangling row', async () => {
383+
const adapter = makeAdapter({ recordRuns: false });
384+
let ran = 0;
385+
await adapter.schedule('sour', { type: 'cron', expression: '* * * * *' }, async () => {
386+
ran += 1;
387+
throw new Error('replayed and failed');
388+
});
389+
390+
await adapter.replay('sour');
391+
392+
// The arm that reads the terminal status off the inner execution (#7734).
393+
// Half-gating — suppressing `startRun` but not `finishRun`, or the reverse
394+
// — is what would leave a `running` row with no `completed_at`.
395+
expect(runs()).toHaveLength(0);
396+
expect(ran).toBe(1);
397+
expect(job().last_status).toBe('failed');
398+
expect(job().failure_count).toBe(1);
399+
});
400+
401+
it('recordRuns: false — the catch arm writes nothing and still rethrows', async () => {
402+
const adapter = makeAdapter({ recordRuns: false });
403+
await adapter.schedule('boom', { type: 'cron', expression: '* * * * *' }, async () => {});
404+
// `executeJob` swallows a handler throw, so the catch arm is unreachable
405+
// through the handler — the inner call itself has to reject for it to run.
406+
const inner = (adapter as any).inner;
407+
inner.trigger = async () => { throw new Error('inner exploded'); };
408+
409+
await expect(adapter.replay('boom')).rejects.toThrow('inner exploded');
410+
411+
expect(runs()).toHaveLength(0);
412+
});
413+
414+
it('default recordRuns — the synthetic replay row is still written, and SETTLED', async () => {
415+
const adapter = makeAdapter(); // no options at all: the flag defaults to true
416+
await adapter.schedule('loud', { type: 'cron', expression: '* * * * *' }, async () => {});
417+
418+
await adapter.replay('loud');
419+
420+
// The #5548 tag survives the gate: one synthetic replay row beside the
421+
// wrapped run the execution itself produced.
422+
const replayRows = runs().filter((r: any) => r.trigger === 'replay');
423+
expect(replayRows).toHaveLength(1);
424+
// Terminal, not left `running` — this is what goes red if the three
425+
// `finishRun` arms are over-gated along with `startRun`.
426+
expect(replayRows[0].status).toBe('success');
427+
expect(replayRows[0].completed_at).toBeTruthy();
428+
expect(runs().map((r: any) => r.trigger).sort()).toEqual(['replay', 'schedule']);
429+
});
430+
431+
it('recordRuns: true — the replay row still carries the terminal status of the inner execution', async () => {
432+
const adapter = makeAdapter({ recordRuns: true }); // explicit, matching the default
433+
await adapter.schedule('sour', { type: 'cron', expression: '* * * * *' }, async () => {
434+
throw new Error('replayed and failed');
435+
});
436+
437+
await adapter.replay('sour');
438+
439+
const replayRows = runs().filter((r: any) => r.trigger === 'replay');
440+
expect(replayRows).toHaveLength(1);
441+
// #7734: read off the inner execution, not assumed `success`.
442+
expect(replayRows[0].status).toBe('failed');
443+
expect(replayRows[0].error).toBe('replayed and failed');
444+
expect(replayRows[0].completed_at).toBeTruthy();
321445
});
322446
});

packages/services/service-job/src/db-job-adapter.ts

Lines changed: 27 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -37,12 +37,11 @@ export interface DbJobAdapterOptions {
3737
* when that attempt settles. Default **`true`**.
3838
*
3939
* This is an on/off switch for run history, NOT a retention cap: setting it to
40-
* `false` means no per-attempt rows are written at all, so `sys_job_run` holds
41-
* nothing for these executions and `listExecutionsByStatus` has nothing to
42-
* read. Two things are unaffected either way — the `sys_job` row's own
43-
* `last_status` / `run_count` / `failure_count` counters, and
44-
* {@link DbJobAdapter.replay}, which writes its synthetic `trigger: 'replay'`
45-
* row regardless of this flag.
40+
* `false` means no rows are written at all — not the per-attempt rows above,
41+
* and not {@link DbJobAdapter.replay}'s synthetic `trigger: 'replay'` row —
42+
* so `sys_job_run` stays empty for this adapter and `listExecutionsByStatus`
43+
* has nothing to read. The one thing unaffected either way is the `sys_job`
44+
* row's own `last_status` / `run_count` / `failure_count` counters.
4645
*/
4746
recordRuns?: boolean;
4847
}
@@ -83,9 +82,8 @@ function uid(prefix: string): string {
8382
* - `cancel(name)` marks the row inactive
8483
* - every execution writes a `sys_job_run` row per attempt — unless
8584
* {@link DbJobAdapterOptions.recordRuns} is `false`, the on/off switch for
86-
* run history, which writes none of them. The one row it does not govern is
87-
* {@link DbJobAdapter.replay}'s synthetic `trigger: 'replay'` row, written
88-
* either way.
85+
* run history, which writes none of them, {@link DbJobAdapter.replay}'s
86+
* synthetic `trigger: 'replay'` row included.
8987
* - every execution updates `sys_job.last_run_at / last_status / run_count /
9088
* failure_count` — unconditionally: `recordRuns` gates the per-attempt rows
9189
* above, never these counters.
@@ -155,14 +153,30 @@ export class DbJobAdapter implements IJobService {
155153
return this.inner.listJobs();
156154
}
157155

156+
/**
157+
* Replay a job's most recent execution, tagging its run `trigger: 'replay'`.
158+
*
159+
* The synthetic `sys_job_run` row this writes is governed by
160+
* {@link DbJobAdapterOptions.recordRuns} exactly as every other run row is:
161+
* with the flag `false` the handler still runs and no row is written — not
162+
* this synthetic one, and not the per-attempt row the execution itself
163+
* would produce. `sys_job_run` is run history, not an audit trail; an
164+
* operator who switched history off gets nothing durable from this path.
165+
*/
158166
async replay(name: string, data?: unknown): Promise<void> {
159167
// Same execution path as trigger but tag the run as 'replay'.
160168
const handlers = (this.inner as any).jobs?.get?.(name);
161169
if (!handlers) throw new Error(`Job "${name}" not found`);
162170
// Reuse trigger; the wrap function uses a closure flag — simpler:
163171
// expose by calling inner.trigger with a marker via data is intrusive,
164172
// so we record a synthetic run row before/after to ensure 'replay' tag.
165-
const runId = await this.startRun(name, 'replay');
173+
//
174+
// Gated exactly as `wrap`'s per-attempt row is: `recordRuns` is the on/off
175+
// switch for run history and this row is run history, so an operator who
176+
// turned it off gets no replay rows either. Ungated, this was the one write
177+
// that ignored the flag — the artifact of the gate landing on one of two
178+
// `startRun` call sites, never a designed carve-out for replay.
179+
const runId = this.recordRuns ? await this.startRun(name, 'replay') : undefined;
166180
try {
167181
await this.inner.trigger(name, data);
168182
// The wrap already recorded a run; settle our synthetic row the same way
@@ -179,12 +193,12 @@ export class DbJobAdapter implements IJobService {
179193
const [last] = await this.inner.getExecutions(name, 1);
180194
const status = last?.status;
181195
if (status === 'degraded' || status === 'timeout' || status === 'failed') {
182-
await this.finishRun(runId, status, last.error);
196+
if (runId) await this.finishRun(runId, status, last.error);
183197
} else {
184-
await this.finishRun(runId, 'success');
198+
if (runId) await this.finishRun(runId, 'success');
185199
}
186200
} catch (err) {
187-
await this.finishRun(runId, 'failed', err instanceof Error ? err.message : String(err));
201+
if (runId) await this.finishRun(runId, 'failed', err instanceof Error ? err.message : String(err));
188202
throw err;
189203
}
190204
}

0 commit comments

Comments
 (0)