diff --git a/packages/services/service-job/src/cron-job-adapter.test.ts b/packages/services/service-job/src/cron-job-adapter.test.ts index ce943229e3..5667017045 100644 --- a/packages/services/service-job/src/cron-job-adapter.test.ts +++ b/packages/services/service-job/src/cron-job-adapter.test.ts @@ -3,16 +3,32 @@ import { describe, it, expect, afterEach } from 'vitest'; import { Cron, scheduledJobs } from 'croner'; import { CronJobAdapter } from './cron-job-adapter.js'; - +import { + NEVER_FIRES_SCHEDULE as CRON, + expectFixtureCannotFire, + expectInertRegistration, +} from './never-fires.fixture.js'; + +/** + * Every case below that asserts an EXACT call/execution count schedules on the + * inert `NEVER_FIRES` fixture, and pins that its own registration cannot fire. + * The rationale, and what the firing spellings cost, is in + * `never-fires.fixture.ts`. + */ describe('CronJobAdapter', () => { let adapter: CronJobAdapter; afterEach(async () => { await adapter?.destroy(); }); + it('the shared cron fixture cannot fire on its own', () => { + expectFixtureCannotFire(); + }); + it('schedules and triggers a cron job', async () => { adapter = new CronJobAdapter(); let calls = 0; - await adapter.schedule('daily', { type: 'cron', expression: '0 0 * * *' }, async () => { calls++; }); + await adapter.schedule('daily', CRON, async () => { calls++; }); expect(await adapter.listJobs()).toEqual(['daily']); + expectInertRegistration(adapter, 'daily'); await adapter.trigger('daily'); expect(calls).toBe(1); @@ -35,7 +51,8 @@ describe('CronJobAdapter', () => { it('records executions', async () => { adapter = new CronJobAdapter(); - await adapter.schedule('tracked', { type: 'cron', expression: '* * * * *' }, async () => {}); + await adapter.schedule('tracked', CRON, async () => {}); + expectInertRegistration(adapter, 'tracked'); await adapter.trigger('tracked'); const execs = await adapter.getExecutions('tracked'); expect(execs).toHaveLength(1); @@ -76,13 +93,14 @@ describe('CronJobAdapter retryPolicy / timeout (#3494)', () => { let calls = 0; await adapter.schedule( 'flaky', - { type: 'cron', expression: '* * * * *' }, + CRON, async () => { calls++; if (calls < 3) throw new Error(`attempt ${calls} boom`); }, { retryPolicy: { maxRetries: 3, backoffMs: 1, backoffMultiplier: 1 } }, ); + expectInertRegistration(adapter, 'flaky'); await adapter.trigger('flaky'); expect(calls).toBe(3); const execs = await adapter.getExecutions('flaky'); @@ -95,10 +113,11 @@ describe('CronJobAdapter retryPolicy / timeout (#3494)', () => { let calls = 0; await adapter.schedule( 'doomed', - { type: 'cron', expression: '* * * * *' }, + CRON, async () => { calls++; throw new Error('always boom'); }, { retryPolicy: { maxRetries: 2, backoffMs: 1 } }, ); + expectInertRegistration(adapter, 'doomed'); await adapter.trigger('doomed'); expect(calls).toBe(3); // initial + 2 retries const execs = await adapter.getExecutions('doomed'); @@ -109,10 +128,11 @@ describe('CronJobAdapter retryPolicy / timeout (#3494)', () => { it('does not retry when no retryPolicy is given (legacy behavior)', async () => { adapter = new CronJobAdapter(); let calls = 0; - await adapter.schedule('legacy', { type: 'cron', expression: '* * * * *' }, async () => { + await adapter.schedule('legacy', CRON, async () => { calls++; throw new Error('boom'); }); + expectInertRegistration(adapter, 'legacy'); await adapter.trigger('legacy'); expect(calls).toBe(1); const execs = await adapter.getExecutions('legacy'); @@ -168,20 +188,27 @@ describe('CronJobAdapter — process-global croner name registry (#8362)', () => const registeredFor = (jobName: string) => scheduledJobs.filter((j) => (j.name ?? '').endsWith(jobName)); - const DAILY = { type: 'cron', expression: '0 8 * * *' } as const; + // These cases scheduled on a CRON expression, hazardous for the same reason + // on a window one instant wide per day rather than per minute: `fired` and + // `calls` below are exact counts, and a self-fire at 08:00 UTC adds to them. + it('the shared cron fixture cannot fire on its own', () => { + expectFixtureCannotFire(CRON.expression); + }); it('lets two live adapters hold the SAME job name — two environments, one container', async () => { const NAME = 'flow-time-relative:contract_expiry_reminder_flow'; const fired: string[] = []; const envA = make(); - await envA.schedule(NAME, DAILY, async () => { fired.push('A'); }); + await envA.schedule(NAME, CRON, async () => { fired.push('A'); }); // The FIRST bind must really have entered the named registry: a rebind pin // whose first bind registered nothing passes for the wrong reason. expect(registeredFor(NAME)).toHaveLength(1); const envB = make(); - await envB.schedule(NAME, DAILY, async () => { fired.push('B'); }); + await envB.schedule(NAME, CRON, async () => { fired.push('B'); }); + expectInertRegistration(envA, NAME); + expectInertRegistration(envB, NAME); expect(registeredFor(NAME)).toHaveLength(2); @@ -193,7 +220,7 @@ describe('CronJobAdapter — process-global croner name registry (#8362)', () => it('frees the process-global name on destroy() — the job is STOPPED, not renamed around', async () => { const NAME = 'flow-schedule:nightly_rollup'; const adapterA = make(); - await adapterA.schedule(NAME, DAILY, async () => {}); + await adapterA.schedule(NAME, CRON, async () => {}); const [job] = registeredFor(NAME); expect(job).toBeDefined(); @@ -213,10 +240,11 @@ describe('CronJobAdapter — process-global croner name registry (#8362)', () => // Somebody else already holds the exact name this adapter will register // under — the residual shape once per-instance namespacing rules out our // own collisions. Replace semantics: the holder is stopped, not tolerated. - const squatter = new Cron(DAILY.expression, { name: adapterA.cronRegistryName(NAME) }, () => {}); + const squatter = new Cron(CRON.expression, { name: adapterA.cronRegistryName(NAME) }, () => {}); expect(registeredFor(NAME)).toHaveLength(1); - await adapterA.schedule(NAME, DAILY, async () => { calls++; }); + await adapterA.schedule(NAME, CRON, async () => { calls++; }); + expectInertRegistration(adapterA, NAME); expect(squatter.isStopped()).toBe(true); const held = registeredFor(NAME); 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 58a478b7de..a1081a7941 100644 --- a/packages/services/service-job/src/db-job-adapter.test.ts +++ b/packages/services/service-job/src/db-job-adapter.test.ts @@ -4,6 +4,11 @@ import { describe, it, expect, beforeEach, afterEach } from 'vitest'; import { scheduledJobs } from 'croner'; import { DbJobAdapter } from './db-job-adapter.js'; import { CronJobAdapter } from './cron-job-adapter.js'; +import { + NEVER_FIRES_SCHEDULE as CRON, + expectFixtureCannotFire, + expectInertRegistration, +} from './never-fires.fixture.js'; function makeFakeEngine() { const tables = new Map(); @@ -167,7 +172,16 @@ describe('DbJobAdapter — kernel rebuild (#8362)', () => { const registeredFor = (jobName: string) => scheduledJobs.filter((j) => (j.name ?? '').endsWith(jobName)); - const DAILY = { type: 'cron', expression: '0 8 * * *' } as const; + /** + * These cases inject a REAL CronJobAdapter, so `schedule()` builds a REAL + * croner job. They scheduled on a daily expression, whose one instant a day + * is a window the exact-count assertion below (`fired`) straddles just as an + * every-minute expression's is — 1440x rarer, same defect. The inert fixture + * removes the schedule entirely; see `never-fires.fixture.ts`. + */ + it('the shared cron fixture cannot fire on its own', () => { + expectFixtureCannotFire(CRON.expression); + }); /** One kernel's job-service wiring: the pair JobServicePlugin builds. */ function kernel() { @@ -178,7 +192,8 @@ describe('DbJobAdapter — kernel rebuild (#8362)', () => { it('destroy() destroys the CRON adapter too, freeing the process-global name', async () => { const NAME = 'flow-time-relative:xqao_contract_expiry_reminder_flow'; const k = kernel(); - await k.db.schedule(NAME, DAILY, async () => {}); + await k.db.schedule(NAME, CRON, async () => {}); + expectInertRegistration(k.cron, NAME); const [job] = registeredFor(NAME); expect(job, 'the first bind must register a REAL croner named job').toBeDefined(); @@ -197,7 +212,8 @@ describe('DbJobAdapter — kernel rebuild (#8362)', () => { const fired: string[] = []; const old = kernel(); - await old.db.schedule(NAME, DAILY, async () => { fired.push('old-kernel'); }); + await old.db.schedule(NAME, CRON, async () => { fired.push('old-kernel'); }); + expectInertRegistration(old.cron, NAME); // Assert the FIRST bind landed before asserting anything about the second. expect(registeredFor(NAME)).toHaveLength(1); const oldJob = registeredFor(NAME)[0]; @@ -205,7 +221,8 @@ describe('DbJobAdapter — kernel rebuild (#8362)', () => { await old.db.destroy(); // kernel evicted by the freshness probe const rebuilt = kernel(); - await rebuilt.db.schedule(NAME, DAILY, async () => { fired.push('new-kernel'); }); + await rebuilt.db.schedule(NAME, CRON, async () => { fired.push('new-kernel'); }); + expectInertRegistration(rebuilt.cron, NAME); const held = registeredFor(NAME); expect(held).toHaveLength(1); // exactly once — not one live + one zombie diff --git a/packages/services/service-job/src/db-job-adapter.timeout.test.ts b/packages/services/service-job/src/db-job-adapter.timeout.test.ts index 35256042ca..fb3e76fafa 100644 --- a/packages/services/service-job/src/db-job-adapter.timeout.test.ts +++ b/packages/services/service-job/src/db-job-adapter.timeout.test.ts @@ -1,10 +1,14 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import { describe, it, expect, beforeEach, afterEach } from 'vitest'; -import { Cron, scheduledJobs } from 'croner'; import { assertEngineUpdateDispatch } from '@objectstack/metadata-core'; import { DbJobAdapter } from './db-job-adapter.js'; import { CronJobAdapter } from './cron-job-adapter.js'; +import { + NEVER_FIRES_SCHEDULE, + expectFixtureCannotFire, + expectInertRegistration, +} from './never-fires.fixture.js'; /** * #7734 — a job that blows its `timeout` must say so in the DURABLE record. @@ -47,26 +51,14 @@ function makeFakeEngine() { } /** - * A cron expression croner PARSES but can never fire: February 30th does not - * exist, so `nextRun()` is `null` and the registration carries no schedule of - * its own. The explicit `trigger()` in each case below is therefore the ONLY - * writer of `sys_job_run`, which is what entitles these cases to assert an - * EXACT row count. - * - * This was `'* * * * *'`, and under that spelling the exact-count assertions - * were a claim about the wall clock rather than about the adapter: when a run - * straddled a minute boundary croner fired the registration on its own, a - * second `sys_job_run` row landed, and CI reddened on a package the offending - * PR had usually not touched (#8628). - * - * `'0 0 29 2 *'` is NOT a substitute — croner resolves Feb 29 to the next leap - * year and it would fire there. Only a date that never occurs is inert. + * The inert cron fixture, now shared with the sibling job suites rather than + * spelled a second time here (#8628, #8748): why February 30th, and why + * `'0 0 29 2 *'` is NOT a substitute, live in `never-fires.fixture.ts`. * * Nothing here depends on the registration being schedulable: `trigger()` * executes the stored record directly and never consults the schedule. */ -const NEVER_FIRES = '0 0 30 2 *'; -const CRON = { type: 'cron', expression: NEVER_FIRES } as const; +const CRON = NEVER_FIRES_SCHEDULE; const TIMEOUT_MS = 20; const HANDLER_MS = 300; @@ -233,7 +225,7 @@ describe('the timeout policy still applies through an injected cron adapter (#77 * machine and reds CI only when a run happens to straddle `:00`. */ it('the shared CRON fixture cannot fire on its own', () => { - expect(new Cron(CRON.expression, { timezone: 'UTC' }).nextRun()).toBeNull(); + expectFixtureCannotFire(CRON.expression); }); it('a cron-scheduled run lands a timeout row even though the adapter no longer sees the policy', async () => { @@ -251,9 +243,7 @@ describe('the timeout policy still applies through an injected cron adapter (#77 // holds only while that registration owns no schedule of its own. Pin both // halves: the job is genuinely registered (the case still exercises the // real adapter) and it will never fire itself (#8628). - const registered = scheduledJobs.find((j) => j.name === cron.cronRegistryName('cronic')); - expect(registered, 'the case must register a REAL croner job').toBeDefined(); - expect(registered!.nextRun()).toBeNull(); + expectInertRegistration(cron, 'cronic'); await cron.trigger('cronic'); // fire the copy the cron adapter holds diff --git a/packages/services/service-job/src/never-fires.fixture.ts b/packages/services/service-job/src/never-fires.fixture.ts new file mode 100644 index 0000000000..919a2e444f --- /dev/null +++ b/packages/services/service-job/src/never-fires.fixture.ts @@ -0,0 +1,85 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The inert cron fixture every exact-count job test in this package schedules + * on, and the two assertions that keep it inert. + * + * Test-only support module. Nothing in `src/index.ts` re-exports it and the + * package's tsup entry is `src/index.ts` alone, so it is type-checked with the + * rest of `src` and shipped in nothing. + */ + +import { expect } from 'vitest'; +import { Cron, scheduledJobs } from 'croner'; +import type { CronJobAdapter } from './cron-job-adapter.js'; + +/** + * A cron expression croner PARSES but can never fire: February 30th does not + * exist, so `nextRun()` is `null` and a registration on it carries no schedule + * of its own. An explicit `trigger()` is then the ONLY thing that can run the + * handler, which is what entitles a case to assert an EXACT execution count. + * + * These fixtures were spelled `'* * * * *'` (and, in the rarer cases, a real + * daily expression). Under those spellings every exact-count assertion in the + * package was a claim about the wall clock rather than about the adapter: + * `CronJobAdapter.schedule()` builds a REAL croner job, so when a run straddled + * the expression's own instant croner fired the registration alongside the + * explicit `trigger()`, a second execution landed, and CI reddened on a package + * the offending PR had usually not touched. + * + * Measured (#8748) by faking ONLY `Date` — real timers, croner's real + * scheduling path — with the registration placed 2/5/10/15 ms before the + * expression's own instant. Under the firing spellings each case gained + * exactly one extra handler run: `records executions` went 1 → 2 execution + * rows, `retries a failing handler` went 3 → 4 calls (the self-fire re-enters + * a handler whose retry counter is already spent, so it succeeds first try), + * and the daily kernel-rebuild case gained an unasked-for `fired` entry at + * 08:00 UTC. At a 2 ms lead two of the three no longer reproduced — croner had + * already passed the instant by the time it computed `nextRun()` — which is + * exactly why this is a positional flake rather than a deterministic failure. + * Under this fixture all three stay put at every lead. + * + * ⛔ `'0 0 29 2 *'` is NOT a substitute — croner resolves Feb 29 forward to the + * next leap year (measured: `2028-02-29T00:00:00.000Z`) and it would fire + * there. Only a date that never occurs at all is inert. + * + * ⛔ And the remedy is never to loosen the counts. An exact count is the only + * thing in this package that can catch a genuine double-scheduling regression, + * which is precisely what these suites exist to catch. + */ +export const NEVER_FIRES = '0 0 30 2 *'; + +/** The inert fixture as a `JobSchedule`, ready to hand to `schedule()`. */ +export const NEVER_FIRES_SCHEDULE = { type: 'cron', expression: NEVER_FIRES } as const; + +/** + * Pin that the FIXTURE itself is inert. + * + * Stated as an assertion and not a comment because the failure it prevents is + * invisible locally: restoring a firing spelling passes on a developer machine + * and reds CI only when a run happens to straddle the expression's instant. + */ +export function expectFixtureCannotFire(expression: string = NEVER_FIRES, timezone = 'UTC'): void { + expect( + new Cron(expression, { timezone }).nextRun(), + `cron fixture "${expression}" must have no next run — otherwise every exact-count assertion in this file is a claim about the wall clock`, + ).toBeNull(); +} + +/** + * Pin that the registration a case ACTUALLY made is inert — both halves. + * + * The fixture pin above is not sufficient on its own: a case can only assert an + * exact count if the job it really registered owns no schedule, and a case that + * registered nothing at all would pass a one-sided check for the wrong reason + * (it would no longer be exercising the real adapter). So: registered, and + * unable to fire. + */ +export function expectInertRegistration(adapter: CronJobAdapter, jobName: string): void { + const registered = scheduledJobs.find((job) => job.name === adapter.cronRegistryName(jobName)); + expect(registered, `"${jobName}": the case must register a REAL croner job`).toBeDefined(); + expect( + registered!.nextRun(), + `"${jobName}": the registration must own no schedule of its own, or it can fire alongside trigger()`, + ).toBeNull(); +}