From dc61fbaee42638b46f2e701d61aa285a7fabcd16 Mon Sep 17 00:00:00 2001 From: os-project-manager Date: Tue, 18 Aug 2026 14:02:08 +0000 Subject: [PATCH] fix(services): the DbJobAdapter class JSDoc stops promising a `sys_job_run` row `recordRuns: false` never writes (#9631) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The class-level "persisted side effects" list said `every execution writes a sys_job_run row`. `wrap()` gates that insert on `recordRuns`: current = { id: this.recordRuns ? await this.startRun(...) : undefined, ... }; and `settle()` only updates a row when one exists, so with `recordRuns: false` no execution writes one. `tsup` emits this comment into the package's built `index.d.ts`, which is the class-level editor tooltip an npm consumer reads — the same "published documentation asserting behaviour the runtime does not have" class as #9611, in the same file and the same emitted declaration. The corrected bullet defers to `DbJobAdapterOptions.recordRuns` with `{@link}` instead of restating the rule. Two true-but-divergent descriptions of one flag is the next version of this defect, and the field's own JSDoc is where the meaning belongs; the class list only says where it shows up. It also names the one row the flag does not govern — `replay()`'s synthetic `trigger: 'replay'` row, written either way, measured rather than assumed. Without that clause the corrected sentence would be false today for exactly the reason #9633 records. The fourth bullet was already true and gains the matching negative: `bumpJob` is called from `settle` OUTSIDE the `if (run.id)` guard, so the `sys_job` counters are updated whether or not a run row exists. Left implicit, a reader carries the `recordRuns` caveat down onto it. No behaviour change. Five cases pin the flag, reusing this file's existing engine double rather than minting a new one. Nothing in the package referenced `recordRuns` in any direction before, so the wording corrected here and the field wording corrected on #9611 were both accurate but unenforced. The discriminator asserts the execution REALLY RAN — handler fired, `sys_job.run_count` bumped — and that no row was written anyway; without that half, "0 rows" would also pass for a job that never fired. The replay case pins today's behaviour because that is what the JSDoc now states, not as an endorsement: #9633 holds the open disposition, and if it lands the case and the bullet it mirrors change together. Co-authored-by: Claude --- .../service-job-class-jsdoc-recordruns.md | 26 ++++++ .../service-job/src/db-job-adapter.test.ts | 86 +++++++++++++++++++ .../service-job/src/db-job-adapter.ts | 10 ++- 3 files changed, 120 insertions(+), 2 deletions(-) create mode 100644 .changeset/service-job-class-jsdoc-recordruns.md diff --git a/.changeset/service-job-class-jsdoc-recordruns.md b/.changeset/service-job-class-jsdoc-recordruns.md new file mode 100644 index 0000000000..f7952fd611 --- /dev/null +++ b/.changeset/service-job-class-jsdoc-recordruns.md @@ -0,0 +1,26 @@ +--- +"@objectstack/service-job": patch +--- + +fix(services): the `DbJobAdapter` class JSDoc stops promising a `sys_job_run` row that `recordRuns: false` never writes (#9631) + +`tsup` emits this comment into `packages/services/service-job/dist/index.d.ts`, so it is +the class-level editor tooltip an npm consumer of `@objectstack/service-job` reads. Its +third "persisted side effects" bullet said **every execution writes a `sys_job_run` row**; +`wrap()` gates that insert on `recordRuns`, which defaults to `true` but writes nothing at +all when set to `false`. The same emitted `index.d.ts` states the truthful field-level rule +for `recordRuns` sixty lines above, so the published declaration disagreed with itself +about one flag — a reader hovering either one got a different answer. + +No runtime behaviour changes. This is a patch because the entire deliverable is text inside +a published package's `.d.ts`: with no version bump the corrected tooltip never reaches npm +and the fix is unmet in the only channel it is about. + +The corrected bullet defers to `DbJobAdapterOptions.recordRuns` via `{@link}` rather than +restating the rule, so the two cannot drift apart again, and it names the one row the flag +does not govern — `replay()`'s synthetic `trigger: 'replay'` row, written either way. The +fourth bullet gains the matching negative: the `sys_job` counters are bumped +unconditionally, `recordRuns` gating only the per-attempt rows. + +Five cases now pin the flag in both directions. Nothing in this package referenced +`recordRuns` before, so both corrected sentences were accurate but unenforced. 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 a1081a7941..111eadcaa4 100644 --- a/packages/services/service-job/src/db-job-adapter.test.ts +++ b/packages/services/service-job/src/db-job-adapter.test.ts @@ -234,3 +234,89 @@ describe('DbJobAdapter — kernel rebuild (#8362)', () => { await rebuilt.db.destroy(); }); }); + +// ─── #9631 — `recordRuns`, the flag two published `.d.ts` comments describe ── +// +// Until this block, NOTHING in this package referenced `recordRuns` in either +// direction: not that `true` writes a row, not that `false` writes none, not +// that the default is `true`. The class JSDoc and the field JSDoc both describe +// the flag to every npm consumer through the emitted `index.d.ts`, and both +// were free to drift from the code — which is exactly what #9611 and #9631 +// each found one of. These cases exist so the sentences stop being unenforced. +// +// The discriminator is case 2: it asserts the execution REALLY RAN (the handler +// fired and `sys_job.run_count` bumped) and that no row was written anyway. +// Without that half, "0 rows" would also pass for a job that never fired, which +// is the way a test like this goes quietly blind. +describe('DbJobAdapter — recordRuns (#9631)', () => { + const adapters: DbJobAdapter[] = []; + const build = (options?: { recordRuns?: boolean }) => { + const engine = makeFakeEngine(); + const adapter = new DbJobAdapter({ engine, options }); + adapters.push(adapter); + return { engine, adapter }; + }; + afterEach(async () => { + while (adapters.length) await adapters.pop()!.destroy(); + }); + + it('defaults to true: a triggered execution writes one sys_job_run row', async () => { + const { engine, adapter } = build(); // no options at all — the documented default + await adapter.schedule('d', { type: 'cron', expression: '* * * * *' }, async () => {}); + await adapter.trigger('d'); + expect(engine.tables.get('sys_job_run') ?? []).toHaveLength(1); + }); + + it('recordRuns: false writes NO sys_job_run row, though the execution really ran', async () => { + const { engine, adapter } = build({ recordRuns: false }); + let ran = 0; + await adapter.schedule('off', { type: 'cron', expression: '* * * * *' }, async () => { ran++; }); + await adapter.trigger('off'); + + expect(ran, 'the handler must actually have run — otherwise "no rows" proves nothing').toBe(1); + expect(engine.tables.get('sys_job_run') ?? []).toHaveLength(0); + }); + + it('recordRuns: false does NOT gate the sys_job counters — only the per-attempt rows', async () => { + // The class JSDoc's fourth bullet: `bumpJob` is called from `settle` + // outside the `if (run.id)` guard, so the job row is updated either way. + const { engine, adapter } = build({ recordRuns: false }); + await adapter.schedule('c', { type: 'cron', expression: '* * * * *' }, async () => { + throw new Error('boom'); + }); + await adapter.trigger('c'); + + const job = (engine.tables.get('sys_job') ?? [])[0]; + expect(job.last_status).toBe('failed'); + expect(job.run_count).toBe(1); + expect(job.failure_count).toBe(1); + expect(engine.tables.get('sys_job_run') ?? []).toHaveLength(0); + }); + + it('recordRuns: true is the same as the default', async () => { + const { engine, adapter } = build({ recordRuns: true }); + await adapter.schedule('on', { type: 'cron', expression: '* * * * *' }, async () => {}); + await adapter.trigger('on'); + const runs = engine.tables.get('sys_job_run') ?? []; + expect(runs).toHaveLength(1); + 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'); + + 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'); + }); +}); diff --git a/packages/services/service-job/src/db-job-adapter.ts b/packages/services/service-job/src/db-job-adapter.ts index 110e4df6c5..dca66ef541 100644 --- a/packages/services/service-job/src/db-job-adapter.ts +++ b/packages/services/service-job/src/db-job-adapter.ts @@ -69,8 +69,14 @@ function uid(prefix: string): string { * Persisted side effects: * - `schedule(name, …)` upserts a `sys_job` row (active=true) * - `cancel(name)` marks the row inactive - * - every execution writes a `sys_job_run` row - * - every execution updates `sys_job.last_run_at / last_status / run_count / failure_count` + * - 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. + * - 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. * * The persistence is best-effort: a DB failure is logged but does not * break job execution. This keeps a healthy job system resilient to