Uh oh!
There was an error while loading. Please reload this page.
fix(service-job): leader-elect interval schedules on multi-replica deployments - #13920
Conversation
…ployments `DbJobAdapter.schedule()` sent `type: 'cron'` to the lock-holding `CronJobAdapter` and `type: 'interval'` to the bare `IntervalJobAdapter`, so every replica armed its own `setInterval` and each tick executed N times. `CronJobAdapter` already handles `type: 'interval'` and fires it through the same leader-elected `runScheduled()`, so interval now takes the same route and inherits the existing `job:` lock rather than growing a second implementation. Both delegated types are registered on the inner adapter through the new `IntervalJobAdapter.register()`, which stores without arming a timer — one process must never hold an elected timer beside an unelected one — keeping `trigger()`, `replay()`, `getExecutions()` and `listJobs()` unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016ZC5rNQj3WEet5HAmmAkMs
The seam-driven form could only show that the fire path was unreachable when the routing is removed; driving both replicas from one `advanceTimersByTime` shows the defect itself — two executions and two run rows for one tick. The winner holds its lease on a gate so the loser's acquire lands while the lock is held, making the count a fact about the lock rather than about how many microtasks the timer flush happened to run. The seam-driven form stays as the separate skip-semantics pin (resolves, does not throw, releases). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016ZC5rNQj3WEet5HAmmAkMs
📓 Docs Drift Check2 anchor(s) derived from 1 changed package(s); no hand-written page names any of them, so this run has nothing to list — not a clean bill of health. This check sees only pages that NAME a derived anchor: one that documents this change in prose, or enumerates it in an authoring dialect, names none and stays invisible to it on every run. What this run could not see
Coarse fallback — 3 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 080b2e35ae916e37191b113cb3b606d556f18472 && git checkout 080b2e35ae916e37191b113cb3b606d556f18472
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 4642f4c64c002f94f1d737bbb2a5fc94ae43ddf3 7e3aee5a03fd18e94850988529add1d96df74157 && git checkout -B drift-repro 4642f4c64c002f94f1d737bbb2a5fc94ae43ddf3 && git merge --no-ff 7e3aee5a03fd18e94850988529add1d96df74157
node scripts/docs-audit/affected-docs.mjs --json 4642f4c64c002f94f1d737bbb2a5fc94ae43ddf3 |
Uh oh!
There was an error while loading. Please reload this page.
Closes#13686
DbJobAdapter— the adapter a production assembly upgrades to — routedtype: 'cron'toCronJobAdapter, which takes a per-fire cluster lock, andtype: 'interval'toIntervalJobAdapter, which has no lock anywhere in the file. So on a multi-replica deployment every replica armed its ownsetIntervaland every tick executed N times. #2219 declared the capability as leader-electing scheduled cron/interval jobs across the cluster; only the cron half enforced it.The reporter's premise, re-verified against the tree first
The card proposed delegating interval to
this.cronbecauseCronJobAdapter"already supports interval and holds the lock". That is the load-bearing claim, and a one-line delegation onto an adapter that only locks its cron branch would have fixed nothing silently. Measured onorigin/mainbefore writing any code:CronJobAdapterschedules an interval registration itselfcron-job-adapter.tsschedule():else if (schedule.type === 'interval' && schedule.intervalMs)armssetInterval(() => { void this.runScheduled(name); }, …)runScheduled()the cron branch's croner callback calls — one method,lock.acquire('job:' + name, { ttlMs, waitMs: 0 }), peers return earlyDbJobAdaptersends interval to the unlocked adapterschedule():if (schedule.type === 'cron')tothis.cron, every other type tothis.inner, which is anIntervalJobAdapterlock/leader/cluster/fencehits ininterval-job-adapter.ts; control: the same grep oncron-job-adapter.tsresolves, so the query worksPremise holds.
cron-job-adapter.leader.test.tsalso already pins the lock semantics using{ type: 'interval' }registrations, so the locked interval limb was covered — just unreachable from the adapter production assembles.Their live evidence (3-replica cluster, traefik → 3 app replicas, shared postgres + redis,
OS_CLUSTER_DRIVER=redis)os:fence:job:ts:*did not move for 100 s; under cron it increments by N (the replica count) per tick.Single-replica is clean for both schedule types — the defect is specific to multi-replica interval.
What changed
db-job-adapter.ts— the routing.intervalnow goes tothis.cronwhen one is assembled, inheriting the existingjob:lock rather than growing a second locking implementation.interval-job-adapter.ts— a newregister(). Delegated types are still registered oninner, but through a seam that stores a registration without arming a timer. This is the part the one-line version of the fix gets wrong:inner.schedule()on an interval registration arms a second, unelectedsetIntervalbeside the elected one, so one process would run the job twice per tick — strictly worse than the across-replicas duplication being fixed.cronwas safe to hand down here only becauseIntervalJobAdapterhappens not to arm a schedule type it cannot run; that inference stops holding the moment the delegated type is one it can run, so it is now said out loud. The cron branch was moved onto the same seam in the same edit — no behaviour change (the inner adapter is constructed without a logger, so its cron warning was already unreachable), but the routing now expresses its intent instead of relying on a coincidence.enableCron: false, or its construction threw) an interval job still fires oninner's timer exactly as before — unelected. Silently dropping a job an assembly can run would not be an improvement, so it runs and says what is missing. Functional degradation,warnper the AGENTS.md level rule.⛔ No new configuration key, per the card: #2219 declared this as the behaviour, and a switch would re-open the same declared-≠-enforced gap on the switch.
Public surface note for review: this adds one method,
IntervalJobAdapter.register(), to an exported class. Additive, no accept-set widening, no spec/authorable surface. The path-derived tier limb does not fire (node scripts/pm/dispatch-gates.mjs --tieron the actual diff: "no path-derived mandate"), and the change alters no contract accept/reject behaviour — but the added method is declared here rather than left for a reviewer to find.What is pinned —
packages/services/service-job/src/db-job-adapter.interval-leader.test.ts, 10 casesinnerwould show as ten). The registration is still visible throughlistJobs().CronJobAdapterand a granting lock, one tick runs the handler once and acquiresjob:sla_escalationwith{ ttlMs: 60000, waitMs: 0 }exactly once; two ticks, twice.advanceTimersByTime. The winner holds its lease on a gate so the loser's acquire lands while the lock is held (making the count a fact about the lock, not about how many microtasks the timer flush ran). Result: one execution, onesys_job_runrow,run_count+1 for one tick.waitMs: 0is the skip spelled structurally — a waiting acquire would let the loser run the same tick a moment later.trigger()still runs on this node while a peer holds the lock (and would throwJob not foundif the delegated registration had leftinner);replay()+getExecutions()still work; thesys_jobrow is still upserted withschedule_type: 'interval'.Ablation
Direction predicted first: reverting only the interval routing branch should redden exactly the four pins that are about the elected path and leave the other six — and all 84 pre-existing package tests — green.
Committed the repair, then mutated the working tree and confirmed the mutation on disk before measuring: anchor marker count 1 → 0; blob hash
f5e5c48d…(HEAD) →40bce718…(worktree);git diff HEAD --statnon-empty (4 deletions). Restore leg proven by state, not by an exit code:git diff HEADempty, worktree blob hash back tof5e5c48d…byte-identical to the HEAD blob, marker count back to 1,git statusclean. The mutation script carried atraprestoring an absolute path, and no rebuild is involved — the suite imports the adapter by a relative specifier inside its own package, so it readssrc/, which the observed reddening itself demonstrates (a suite readingdist/would have stayed green).Measured, 4 failed / 90 passed of 94:
Prediction matched, including the second case's shape: reverting the routing leaves the handler firing once per tick (on
inner's timer), so that pin reddens at the lock assertion rather than at the count — which is precisely why a count-only test would not have caught this defect.trigger(),replay()/getExecutions(), thesys_jobupsert, and the cron-routing control) plus all 84 pre-existing tests in the package. That last one is the point rather than a footnote: nothing already in this package could notice the defect, which is how it shipped.Measured / NOT MEASURED
os:fence:job:ts:*now increments once per tick under interval schedules as it already does under cron, and that the duplicate notification inserts stop. A single-process simulation is not a cluster verification and is not presented as one. @baozhoutao — the confirmation this needs is one run of your existing 60 s-interval configuration against a build carrying this branch, watching that counter and the notification table.check:dual-build-cjs-loadsandcheck:type-check-debtboth require the full workspace build;check:test-completenessrequires a savedturbo run testlog and instructs that a local run record it as NOT MEASURED.service-jobappears in neither the DEBT nor the TEST_DEBT ledger, and the structural halfcheck:type-check-coverageis green, so nothing in this diff can move a ledgered count.Verification (all at
7e3aee5a, the head of this branch, tree clean)pnpm --filter @objectstack/service-job exec vitest run --maxWorkers=2→ 9 files, 93 tests, 0 failed (84 pre-existing + 9 new at that point; 10 new after the contention pin was split, 94 total).pnpm --filter @objectstack/service-job run typecheck→ exit 0.--listFilesconfirms the program really contains 9src/*.test.tsfiles including the new one, so "typecheck clean" is a statement about the new test file and not a vacuous one.pnpm --filter '@objectstack/service-job^...' build→ exit 0 (dependency closure, built before any judgement).node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands(39 runnable): all green except the three prerequisite-bound ones above. Three fired on the new test double and were repaired rather than baselined —check:where-matcher(the double now refuses an operator key instead of reading it as a column),check:objectql-double-limit(the caller's bound applied after the filter, by presence),check:engine-double-contract(update()opens withassertEngineUpdateDispatch, and the new pinned coverage was recorded with--write, a ledger growth).pnpm lint(repo-wideeslint . --no-inline-config) → exit 0. Not narrowed.node scripts/check-nul-bytes.mjs→ exit 0; the changed files also self-scanned clean for raw control bytes.Every exit code above was captured before any pipe.
Out of scope
type: 'once'schedules take the same unlocked limb and duplicate the same way on a multi-replica cluster. Filed as #13918 rather than fixed here — it is not what #2219 declared, the reporter's cluster cannot confirm it, and a one-shot's crash semantics under a lease is a decision rather than a mechanical extension. out of scope: #13918.Generated by Claude Code