Skip to content

fix(services): the DbJobAdapter class JSDoc stops promising a sys_job_run row that recordRuns: false never writes - #9646

Merged
os-project-manager merged 1 commit into
mainfrom
claude/issue-9631-dbjobadapter-class-jsdoc
Aug 18, 2026
Merged

fix(services): the DbJobAdapter class JSDoc stops promising a sys_job_run row that recordRuns: false never writes#9646
os-project-manager merged 1 commit into
mainfrom
claude/issue-9631-dbjobadapter-class-jsdoc

Conversation

@os-project-manager

Copy link
Copy Markdown
Collaborator

Fixes#9631

The DbJobAdapterclass-level JSDoc listed among its persisted side effects:

 * - every execution writes a `sys_job_run` row

wrap() gates that insert on recordRuns, so with recordRuns: false no execution
writes one. tsup emits this comment into packages/services/service-job/dist/index.d.ts,
which makes it the class-level editor tooltip an npm consumer of @objectstack/service-job
reads — the same "published documentation asserting behaviour the runtime does not have"
class as #9611, in the same file and the same emitted declaration. No behaviour changes.

Premise verified before implementing, and one premise that had moved

Re-measured against origin/main (40162f1e2) rather than taken from the card:

The premise that had moved: the dispatch expected PR #9635 (for #9611) to have landed. It has not — it is still an open draft, so main at branch time carries the old field-level comment. I branched from main as instructed, did not branch from its branch, and read the corrected field wording from its head commit 9b07a5e34 to match it. The two diffs touch db-job-adapter.ts about thirty lines apart (its hunk is the DbJobAdapterOptions field comment at lines 31-35; mine is the class JSDoc at 62-77) and share no other file — #9635 touches memory-cache-adapter.ts, memory-cache-adapter.test.ts and its own changeset, while my test edits land in db-job-adapter.test.ts. Either merge order is clean.

The correction, and why it defers instead of restating

The card's sharpest constraint was not to invent a second, differently-worded
description of one flag — two true-but-divergent descriptions is the next version of this
defect. So the corrected bullet points at the field rather than paraphrasing it:

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

Three deliberate choices:

  1. "per attempt", matching Two published .d.ts JSDoc comments in packages/services/* describe behaviour their code does not have — MemoryCacheAdapter's "LRU-style eviction" is insertion-order, and DbJobAdapterOptions.recordRuns is documented as a numeric cap defaulting to none but is a boolean defaulting to true #9611's field wording ("inserted at the start of every attempt"). onAttemptStart fires per attempt, so a retried execution writes more than one row; "every execution writes a row" was loose in that direction too.
  2. The replay clause is load-bearing, not decoration. Writing the obvious sentence — "no row is written when recordRuns is false" — would have been wrong today for exactly the reason DbJobAdapter.replay() writes its synthetic sys_job_run row regardless of recordRuns — an operator who switched run history off still accumulates replay rows #9633 records. The honest sentence has to name the exception, so it does.
  3. The fourth bullet gains its matching negative. It was already true, but left implicit a reader carries the recordRuns caveat down onto it from the bullet above. Named here with its evidence (bumpJob outside the if (run.id) guard); this is the only line I touched that was not itself false.

No issue number went into the published tooltip — #9633 belongs in the test comment, not in what npm renders.

The pin: five cases, and why each discriminates

Nothing in this package referenced recordRuns in any direction before this PR, so both
the field wording #9611 corrected and the class wording corrected here were accurate but
unenforced. The cases reuse this file's existing engine double rather than minting a new
one, so check:engine-double-contract gains nothing to pin (319 pinned, unchanged).

  • recordRuns defaults to true — no options at all, one row written
  • recordRuns: false writes NO row though the execution really ran — the discriminator
  • recordRuns: false does not gate the sys_job counters — pins bullet four
  • explicit recordRuns: true matches the default, with the row's trigger/status shape
  • replay() writes its synthetic row even when the flag is false — pins the exception

The second case asserts the handler fired and sys_job.run_countbumped alongside
the "no rows" assertion. Without that half, "0 rows" would pass just as well for a job that
never ran at all, which is how a test like this goes quietly blind.

Reverse verification — direction predicted before running, and observed. Ablation
applied to the committed state (the recordRuns gate removed from wrap(), so startRun
is unconditional). Predicted: red in exactly cases 2, 3 and 5; 1 and 4 green; all 75
pre-existing tests green. Observed:

 × recordRuns: false writes NO sys_job_run row, though the execution really ran
× recordRuns: false does NOT gate the sys_job counters — only the per-attempt rows
× replay() writes its synthetic row even when recordRuns is false — the exception the JSDoc names
AssertionError: expected [ { …(10) } ] to have a length of +0 but got 1
AssertionError: expected [ 'replay', 'schedule' ] to deeply equal [ 'replay' ]
Tests 3 failed | 77 passed (80)

The second half is the load-bearing part: all 75 pre-existing tests stayed green under the
ablation.
The flag could stop being honoured entirely and this package's suite would not
have noticed — which is precisely how a comment describing it drifted from the code and sat
green through three cards. These are source-level vitest cases, not a dogfood run, so the
ablation needs no rebuild to take effect. Restored with
git checkout claude/issue-9631-dbjobadapter-class-jsdoc -- ...; no marker remains
(grep -c ABLATION-9631 = 0) and the tree is clean at the head below.

Acceptance: checked in the emitted artifact, not the source

Rebuilt the package and read the published declarations. In bothdist/index.d.ts and
its dist/index.d.cts twin, the unqualified bullet counts 0, and the qualified
replacement counts 1:

86: * Persisted side effects:
89: * - every execution writes a `sys_job_run` row per attempt — unless
90: * {@link DbJobAdapterOptions.recordRuns} is `false`, the on/off switch for

Changeset: owed, same reasoning #9611 used

.changeset/service-job-class-jsdoc-recordruns.md, patch on @objectstack/service-job.
AGENTS.md exempts pure bug fixes, but this fix's entire deliverable is text inside a
published package's .d.tswith no version bump the corrected tooltip never reaches
npm
and the card's acceptance is unmet in the only channel it is about. Not breaking, so
no ADR-0087 marker is required.

Verification — union run at dc61fbaee, the final commit

Derived with node scripts/pm/dispatch-gates.mjs against the actual changed paths from
git merge-base (40162f1e2), per #9320 — not origin/main..HEAD. Working tree clean.

gateresult
pnpm --filter @objectstack/service-job test80 passed (8 files) — 75 pre-existing + 5 new
pnpm --filter @objectstack/service-job typechecktsc --noEmit clean
pnpm check:nul-bytesOK — 6177 files, no raw control bytes
pnpm check:changeset-gate-self-testsOK
pnpm check:objectui-changesetOK
pnpm check:test-source-aliasOK — 72 packages
pnpm check:type-source-resolutionOK — 76 packages
node scripts/check-adr-0087-registration.mjsOK — no declared-breaking changeset
node scripts/check-changeset-no-major.mjsOK
node scripts/check-empty-changeset.mjsOK — 1 declaring changeset
node scripts/docs-audit/check-affected-docs.mjsOK — 242 self-test cases
pnpm check:query-options-erasureratchet holds, none new
pnpm check:engine-double-contractOK — 319 pinned, no new double
pnpm check:where-matcherOK — 255 matchers, none new
pnpm check:type-check-coverageOK — 64/77 packages
pnpm check:type-check-debt --re-measureOK — 33 entries re-measured in 230.3s, none above its number

The five convention-triggered families are in that list because this PR adds test code;
the ratchet's --re-measure needs the built workspace closure, so
turbo run build --filter=./packages/* --filter=./packages/*/* ran first (70/70 successful)
— a refusal would have meant NOT MEASURED, not a pass. Every heavy step ran under
flock /tmp/os-heavy-verify.lock.

Out of scope


Generated by Claude Code

…b_run` row `recordRuns: false` never writes (#9631)
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 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

Nothing in this diff resolved to a documentable surface (no symbol, route or SDK anchor derived from 1 changed package(s)), so this run has no opinion about the docs.

What this run could not see
  • 1 changed file(s) yielded no anchor (packages/services/service-job/src/db-job-adapter.ts) — pages documenting those are invisible to this run

Coarse fallback — 3 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 07e630e5830c633d069798975f1b7883e7749bdbpackageMentionDocs.

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 18, 2026
@os-project-manager
os-project-manager marked this pull request as ready for review August 18, 2026 14:37
@os-project-manager
os-project-manager added this pull request to the merge queueAug 18, 2026
Merged via the queue into main with commit 73010f1Aug 18, 2026
26 checks passed
@os-project-manager
os-project-manager deleted the claude/issue-9631-dbjobadapter-class-jsdoc branch August 18, 2026 14:59
os-project-manager pushed a commit that referenced this pull request Aug 18, 2026
… landed
Merged rather than rebased: rebasing a pushed branch needs a force-push, which
the dev contract forbids outright. Same outcome, no forced history.
Both blockers are now on main (#9635 at c07d6e8, #9646 at 73010f1), so the
three points this card was holding are resolved here:
1. Field JSDoc — the clause naming replay as an exception is gone. Not a
mechanical delete: it left "Two things are unaffected either way" counting
wrong, so the sentence is rewritten to say `false` writes no rows at all,
replay included, with the sys_job counters as the one exception.
2. Class JSDoc — the "one row it does not govern" sentence is gone. The bullet
keeps its single {@link DbJobAdapterOptions.recordRuns} pointer rather than
paraphrasing the flag a second time, per the constraint #9631 was built on.
3. #9646's fifth test case, which pinned "replay writes anyway", is deleted.
It carried a comment saying it would change together with the class JSDoc if
this card ruled the carve-out shut. It did. A note stands where it was.
Test-file conflict was add/add — both describe blocks appended at the end. Both
are kept; they cover different paths (#9631's the wrap() path, this card's the
replay() path).
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y26DJEHSBhhAQ6wwfsHNza
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/mteststooling

Projects

None yet

1 participant

@os-project-manager