Skip to content

Commit 7a897ae

Browse files
committed
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
1 parent 07e630e commit 7a897ae

3 files changed

Lines changed: 180 additions & 4 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: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -234,3 +234,129 @@ describe('DbJobAdapter — kernel rebuild (#8362)', () => {
234234
await rebuilt.db.destroy();
235235
});
236236
});
237+
238+
// ─── #9633 — replay() honours `recordRuns`, on all three of its arms ─────────
239+
//
240+
// `recordRuns` had exactly two `startRun` call sites and the gate landed on one
241+
// of them: `wrap`'s per-attempt row was gated, `replay`'s synthetic row was not.
242+
// An operator who switched run history off therefore kept accumulating rows —
243+
// exclusively replay ones, the least representative sample of a job's history,
244+
// with no non-replay rows beside them for context. The carve-out was an
245+
// artifact of `replay` being written to solve a different problem (#5548's
246+
// synthetic row forces the `trigger: 'replay'` tag), never a designed exception,
247+
// so it is closed rather than documented (the ruling on #9633).
248+
//
249+
// ⚠️ Nothing in this package referenced `recordRuns` in ANY direction before
250+
// #9631 — the flag could have stopped being honoured entirely and the suite
251+
// would not have noticed. These cases are written so that cannot happen again:
252+
// each flag-off case asserts the execution REALLY RAN (handler counter plus the
253+
// `sys_job` counters, which the flag deliberately does not gate) alongside the
254+
// "no rows" assertion, so "0 rows" cannot pass for a job that never ran; and
255+
// the flag-on cases assert the synthetic row reached a TERMINAL status, so
256+
// over-gating the three `finishRun` arms — a dangling `running` half-row, worse
257+
// than either original state — goes red instead of silently passing "a row
258+
// exists".
259+
describe('DbJobAdapter — replay() honours recordRuns (#9633)', () => {
260+
let engine: ReturnType<typeof makeFakeEngine>;
261+
const adapters: DbJobAdapter[] = [];
262+
263+
function makeAdapter(options?: { recordRuns?: boolean }) {
264+
const a = new DbJobAdapter({ engine, options });
265+
adapters.push(a);
266+
return a;
267+
}
268+
269+
beforeEach(() => {
270+
engine = makeFakeEngine();
271+
adapters.length = 0;
272+
});
273+
afterEach(async () => {
274+
for (const a of adapters) await a.destroy();
275+
});
276+
277+
const runs = () => engine.tables.get('sys_job_run') ?? [];
278+
const job = () => (engine.tables.get('sys_job') ?? [])[0];
279+
280+
it('recordRuns: false — a replay runs the handler and writes NO sys_job_run row', async () => {
281+
const adapter = makeAdapter({ recordRuns: false });
282+
let ran = 0;
283+
await adapter.schedule('quiet', { type: 'cron', expression: '* * * * *' }, async () => { ran += 1; });
284+
285+
await adapter.replay('quiet');
286+
287+
// The discriminator: neither the synthetic replay row nor the wrapped
288+
// per-attempt row is written. Ungated, this table holds a `trigger:
289+
// 'replay'` row here.
290+
expect(runs()).toHaveLength(0);
291+
// …and the execution really happened, so "0 rows" cannot be passing for a
292+
// job that never ran. `bumpJob` is called outside the row gate by design.
293+
expect(ran).toBe(1);
294+
expect(job().run_count).toBe(1);
295+
expect(job().last_status).toBe('success');
296+
});
297+
298+
it('recordRuns: false — the terminal-status arm writes nothing either, and leaves no dangling row', async () => {
299+
const adapter = makeAdapter({ recordRuns: false });
300+
let ran = 0;
301+
await adapter.schedule('sour', { type: 'cron', expression: '* * * * *' }, async () => {
302+
ran += 1;
303+
throw new Error('replayed and failed');
304+
});
305+
306+
await adapter.replay('sour');
307+
308+
// The arm that reads the terminal status off the inner execution (#7734).
309+
// Half-gating — suppressing `startRun` but not `finishRun`, or the reverse
310+
// — is what would leave a `running` row with no `completed_at`.
311+
expect(runs()).toHaveLength(0);
312+
expect(ran).toBe(1);
313+
expect(job().last_status).toBe('failed');
314+
expect(job().failure_count).toBe(1);
315+
});
316+
317+
it('recordRuns: false — the catch arm writes nothing and still rethrows', async () => {
318+
const adapter = makeAdapter({ recordRuns: false });
319+
await adapter.schedule('boom', { type: 'cron', expression: '* * * * *' }, async () => {});
320+
// `executeJob` swallows a handler throw, so the catch arm is unreachable
321+
// through the handler — the inner call itself has to reject for it to run.
322+
const inner = (adapter as any).inner;
323+
inner.trigger = async () => { throw new Error('inner exploded'); };
324+
325+
await expect(adapter.replay('boom')).rejects.toThrow('inner exploded');
326+
327+
expect(runs()).toHaveLength(0);
328+
});
329+
330+
it('default recordRuns — the synthetic replay row is still written, and SETTLED', async () => {
331+
const adapter = makeAdapter(); // no options at all: the flag defaults to true
332+
await adapter.schedule('loud', { type: 'cron', expression: '* * * * *' }, async () => {});
333+
334+
await adapter.replay('loud');
335+
336+
// The #5548 tag survives the gate: one synthetic replay row beside the
337+
// wrapped run the execution itself produced.
338+
const replayRows = runs().filter((r: any) => r.trigger === 'replay');
339+
expect(replayRows).toHaveLength(1);
340+
// Terminal, not left `running` — this is what goes red if the three
341+
// `finishRun` arms are over-gated along with `startRun`.
342+
expect(replayRows[0].status).toBe('success');
343+
expect(replayRows[0].completed_at).toBeTruthy();
344+
expect(runs().map((r: any) => r.trigger).sort()).toEqual(['replay', 'schedule']);
345+
});
346+
347+
it('recordRuns: true — the replay row still carries the terminal status of the inner execution', async () => {
348+
const adapter = makeAdapter({ recordRuns: true }); // explicit, matching the default
349+
await adapter.schedule('sour', { type: 'cron', expression: '* * * * *' }, async () => {
350+
throw new Error('replayed and failed');
351+
});
352+
353+
await adapter.replay('sour');
354+
355+
const replayRows = runs().filter((r: any) => r.trigger === 'replay');
356+
expect(replayRows).toHaveLength(1);
357+
// #7734: read off the inner execution, not assumed `success`.
358+
expect(replayRows[0].status).toBe('failed');
359+
expect(replayRows[0].error).toBe('replayed and failed');
360+
expect(replayRows[0].completed_at).toBeTruthy();
361+
});
362+
});

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

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -137,14 +137,30 @@ export class DbJobAdapter implements IJobService {
137137
return this.inner.listJobs();
138138
}
139139

140+
/**
141+
* Replay a job's most recent execution, tagging its run `trigger: 'replay'`.
142+
*
143+
* The synthetic `sys_job_run` row this writes is governed by
144+
* {@link DbJobAdapterOptions.recordRuns} exactly as every other run row is:
145+
* with the flag `false` the handler still runs and no row is written — not
146+
* this synthetic one, and not the per-attempt row the execution itself
147+
* would produce. `sys_job_run` is run history, not an audit trail; an
148+
* operator who switched history off gets nothing durable from this path.
149+
*/
140150
async replay(name: string, data?: unknown): Promise<void> {
141151
// Same execution path as trigger but tag the run as 'replay'.
142152
const handlers = (this.inner as any).jobs?.get?.(name);
143153
if (!handlers) throw new Error(`Job "${name}" not found`);
144154
// Reuse trigger; the wrap function uses a closure flag — simpler:
145155
// expose by calling inner.trigger with a marker via data is intrusive,
146156
// so we record a synthetic run row before/after to ensure 'replay' tag.
147-
const runId = await this.startRun(name, 'replay');
157+
//
158+
// Gated exactly as `wrap`'s per-attempt row is: `recordRuns` is the on/off
159+
// switch for run history and this row is run history, so an operator who
160+
// turned it off gets no replay rows either. Ungated, this was the one write
161+
// that ignored the flag — the artifact of the gate landing on one of two
162+
// `startRun` call sites, never a designed carve-out for replay.
163+
const runId = this.recordRuns ? await this.startRun(name, 'replay') : undefined;
148164
try {
149165
await this.inner.trigger(name, data);
150166
// The wrap already recorded a run; settle our synthetic row the same way
@@ -161,12 +177,12 @@ export class DbJobAdapter implements IJobService {
161177
const [last] = await this.inner.getExecutions(name, 1);
162178
const status = last?.status;
163179
if (status === 'degraded' || status === 'timeout' || status === 'failed') {
164-
await this.finishRun(runId, status, last.error);
180+
if (runId) await this.finishRun(runId, status, last.error);
165181
} else {
166-
await this.finishRun(runId, 'success');
182+
if (runId) await this.finishRun(runId, 'success');
167183
}
168184
} catch (err) {
169-
await this.finishRun(runId, 'failed', err instanceof Error ? err.message : String(err));
185+
if (runId) await this.finishRun(runId, 'failed', err instanceof Error ? err.message : String(err));
170186
throw err;
171187
}
172188
}

0 commit comments

Comments
 (0)