Skip to content

Commit 226cb71

Browse files
Jack Qclaude
andauthored
test(service-job): schedule exact-count cases on an inert cron fixture (#8748) (#8774)
Seven cases in this package asserted an exact call/execution count while `CronJobAdapter.schedule()` had registered a REAL croner job on a firing expression. Croner fired that registration on its own schedule alongside the explicit `trigger()`, so every one of those assertions was a claim about the wall clock rather than about the adapter. Reproduced 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` 1 -> 2 execution rows, `retries a failing handler` 3 -> 4 calls (the self-fire re-enters a handler whose retry counter is already spent, so it succeeds on the first attempt), 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 stopped reproducing — croner had already passed the instant by the time it computed `nextRun()` — which is what makes this a positional flake rather than a deterministic failure. Under the fixture all three hold at every lead. Four cases used '* * * * *' (hazard window once per minute) and three used a daily expression (same defect, ~1440x rarer). All seven now schedule on the inert `NEVER_FIRES` fixture ('0 0 30 2 *' — February 30th never occurs, so `nextRun()` is null and the registration owns no schedule of its own). The counts themselves are unchanged: they are the only thing here that can catch a genuine double-scheduling regression. `NEVER_FIRES` moves out of db-job-adapter.timeout.test.ts into a shared test-support module so a later tidy-up cannot revive the hazard in one file only, and both halves of the pin (the fixture cannot fire; the registration the case actually made cannot fire) are shared assertions rather than a shape each file re-spells by hand. Claude-Session: https://claude.ai/code/session_01NaS1PAHJcPfAA2acnV53Tn Co-authored-by: Claude <noreply@anthropic.com>
1 parent 79394d7 commit 226cb71

4 files changed

Lines changed: 157 additions & 37 deletions

File tree

packages/services/service-job/src/cron-job-adapter.test.ts

Lines changed: 40 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -3,16 +3,32 @@
33
import { describe, it, expect, afterEach } from 'vitest';
44
import { Cron, scheduledJobs } from 'croner';
55
import { CronJobAdapter } from './cron-job-adapter.js';
6-
6+
import {
7+
NEVER_FIRES_SCHEDULE as CRON,
8+
expectFixtureCannotFire,
9+
expectInertRegistration,
10+
} from './never-fires.fixture.js';
11+
12+
/**
13+
* Every case below that asserts an EXACT call/execution count schedules on the
14+
* inert `NEVER_FIRES` fixture, and pins that its own registration cannot fire.
15+
* The rationale, and what the firing spellings cost, is in
16+
* `never-fires.fixture.ts`.
17+
*/
718
describe('CronJobAdapter', () => {
819
let adapter: CronJobAdapter;
920
afterEach(async () => { await adapter?.destroy(); });
1021

22+
it('the shared cron fixture cannot fire on its own', () => {
23+
expectFixtureCannotFire();
24+
});
25+
1126
it('schedules and triggers a cron job', async () => {
1227
adapter = new CronJobAdapter();
1328
let calls = 0;
14-
await adapter.schedule('daily', { type: 'cron', expression: '0 0 * * *' }, async () => { calls++; });
29+
await adapter.schedule('daily', CRON, async () => { calls++; });
1530
expect(await adapter.listJobs()).toEqual(['daily']);
31+
expectInertRegistration(adapter, 'daily');
1632

1733
await adapter.trigger('daily');
1834
expect(calls).toBe(1);
@@ -35,7 +51,8 @@ describe('CronJobAdapter', () => {
3551

3652
it('records executions', async () => {
3753
adapter = new CronJobAdapter();
38-
await adapter.schedule('tracked', { type: 'cron', expression: '* * * * *' }, async () => {});
54+
await adapter.schedule('tracked', CRON, async () => {});
55+
expectInertRegistration(adapter, 'tracked');
3956
await adapter.trigger('tracked');
4057
const execs = await adapter.getExecutions('tracked');
4158
expect(execs).toHaveLength(1);
@@ -76,13 +93,14 @@ describe('CronJobAdapter retryPolicy / timeout (#3494)', () => {
7693
let calls = 0;
7794
await adapter.schedule(
7895
'flaky',
79-
{ type: 'cron', expression: '* * * * *' },
96+
CRON,
8097
async () => {
8198
calls++;
8299
if (calls < 3) throw new Error(`attempt ${calls} boom`);
83100
},
84101
{ retryPolicy: { maxRetries: 3, backoffMs: 1, backoffMultiplier: 1 } },
85102
);
103+
expectInertRegistration(adapter, 'flaky');
86104
await adapter.trigger('flaky');
87105
expect(calls).toBe(3);
88106
const execs = await adapter.getExecutions('flaky');
@@ -95,10 +113,11 @@ describe('CronJobAdapter retryPolicy / timeout (#3494)', () => {
95113
let calls = 0;
96114
await adapter.schedule(
97115
'doomed',
98-
{ type: 'cron', expression: '* * * * *' },
116+
CRON,
99117
async () => { calls++; throw new Error('always boom'); },
100118
{ retryPolicy: { maxRetries: 2, backoffMs: 1 } },
101119
);
120+
expectInertRegistration(adapter, 'doomed');
102121
await adapter.trigger('doomed');
103122
expect(calls).toBe(3); // initial + 2 retries
104123
const execs = await adapter.getExecutions('doomed');
@@ -109,10 +128,11 @@ describe('CronJobAdapter retryPolicy / timeout (#3494)', () => {
109128
it('does not retry when no retryPolicy is given (legacy behavior)', async () => {
110129
adapter = new CronJobAdapter();
111130
let calls = 0;
112-
await adapter.schedule('legacy', { type: 'cron', expression: '* * * * *' }, async () => {
131+
await adapter.schedule('legacy', CRON, async () => {
113132
calls++;
114133
throw new Error('boom');
115134
});
135+
expectInertRegistration(adapter, 'legacy');
116136
await adapter.trigger('legacy');
117137
expect(calls).toBe(1);
118138
const execs = await adapter.getExecutions('legacy');
@@ -168,20 +188,27 @@ describe('CronJobAdapter — process-global croner name registry (#8362)', () =>
168188
const registeredFor = (jobName: string) =>
169189
scheduledJobs.filter((j) => (j.name ?? '').endsWith(jobName));
170190

171-
const DAILY = { type: 'cron', expression: '0 8 * * *' } as const;
191+
// These cases scheduled on a CRON expression, hazardous for the same reason
192+
// on a window one instant wide per day rather than per minute: `fired` and
193+
// `calls` below are exact counts, and a self-fire at 08:00 UTC adds to them.
194+
it('the shared cron fixture cannot fire on its own', () => {
195+
expectFixtureCannotFire(CRON.expression);
196+
});
172197

173198
it('lets two live adapters hold the SAME job name — two environments, one container', async () => {
174199
const NAME = 'flow-time-relative:contract_expiry_reminder_flow';
175200
const fired: string[] = [];
176201

177202
const envA = make();
178-
await envA.schedule(NAME, DAILY, async () => { fired.push('A'); });
203+
await envA.schedule(NAME, CRON, async () => { fired.push('A'); });
179204
// The FIRST bind must really have entered the named registry: a rebind pin
180205
// whose first bind registered nothing passes for the wrong reason.
181206
expect(registeredFor(NAME)).toHaveLength(1);
182207

183208
const envB = make();
184-
await envB.schedule(NAME, DAILY, async () => { fired.push('B'); });
209+
await envB.schedule(NAME, CRON, async () => { fired.push('B'); });
210+
expectInertRegistration(envA, NAME);
211+
expectInertRegistration(envB, NAME);
185212

186213
expect(registeredFor(NAME)).toHaveLength(2);
187214

@@ -193,7 +220,7 @@ describe('CronJobAdapter — process-global croner name registry (#8362)', () =>
193220
it('frees the process-global name on destroy() — the job is STOPPED, not renamed around', async () => {
194221
const NAME = 'flow-schedule:nightly_rollup';
195222
const adapterA = make();
196-
await adapterA.schedule(NAME, DAILY, async () => {});
223+
await adapterA.schedule(NAME, CRON, async () => {});
197224

198225
const [job] = registeredFor(NAME);
199226
expect(job).toBeDefined();
@@ -213,10 +240,11 @@ describe('CronJobAdapter — process-global croner name registry (#8362)', () =>
213240
// Somebody else already holds the exact name this adapter will register
214241
// under — the residual shape once per-instance namespacing rules out our
215242
// own collisions. Replace semantics: the holder is stopped, not tolerated.
216-
const squatter = new Cron(DAILY.expression, { name: adapterA.cronRegistryName(NAME) }, () => {});
243+
const squatter = new Cron(CRON.expression, { name: adapterA.cronRegistryName(NAME) }, () => {});
217244
expect(registeredFor(NAME)).toHaveLength(1);
218245

219-
await adapterA.schedule(NAME, DAILY, async () => { calls++; });
246+
await adapterA.schedule(NAME, CRON, async () => { calls++; });
247+
expectInertRegistration(adapterA, NAME);
220248

221249
expect(squatter.isStopped()).toBe(true);
222250
const held = registeredFor(NAME);

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

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,11 @@ import { describe, it, expect, beforeEach, afterEach } from 'vitest';
44
import { scheduledJobs } from 'croner';
55
import { DbJobAdapter } from './db-job-adapter.js';
66
import { CronJobAdapter } from './cron-job-adapter.js';
7+
import {
8+
NEVER_FIRES_SCHEDULE as CRON,
9+
expectFixtureCannotFire,
10+
expectInertRegistration,
11+
} from './never-fires.fixture.js';
712

813
function makeFakeEngine() {
914
const tables = new Map<string, any[]>();
@@ -167,7 +172,16 @@ describe('DbJobAdapter — kernel rebuild (#8362)', () => {
167172
const registeredFor = (jobName: string) =>
168173
scheduledJobs.filter((j) => (j.name ?? '').endsWith(jobName));
169174

170-
const DAILY = { type: 'cron', expression: '0 8 * * *' } as const;
175+
/**
176+
* These cases inject a REAL CronJobAdapter, so `schedule()` builds a REAL
177+
* croner job. They scheduled on a daily expression, whose one instant a day
178+
* is a window the exact-count assertion below (`fired`) straddles just as an
179+
* every-minute expression's is — 1440x rarer, same defect. The inert fixture
180+
* removes the schedule entirely; see `never-fires.fixture.ts`.
181+
*/
182+
it('the shared cron fixture cannot fire on its own', () => {
183+
expectFixtureCannotFire(CRON.expression);
184+
});
171185

172186
/** One kernel's job-service wiring: the pair JobServicePlugin builds. */
173187
function kernel() {
@@ -178,7 +192,8 @@ describe('DbJobAdapter — kernel rebuild (#8362)', () => {
178192
it('destroy() destroys the CRON adapter too, freeing the process-global name', async () => {
179193
const NAME = 'flow-time-relative:xqao_contract_expiry_reminder_flow';
180194
const k = kernel();
181-
await k.db.schedule(NAME, DAILY, async () => {});
195+
await k.db.schedule(NAME, CRON, async () => {});
196+
expectInertRegistration(k.cron, NAME);
182197

183198
const [job] = registeredFor(NAME);
184199
expect(job, 'the first bind must register a REAL croner named job').toBeDefined();
@@ -197,15 +212,17 @@ describe('DbJobAdapter — kernel rebuild (#8362)', () => {
197212
const fired: string[] = [];
198213

199214
const old = kernel();
200-
await old.db.schedule(NAME, DAILY, async () => { fired.push('old-kernel'); });
215+
await old.db.schedule(NAME, CRON, async () => { fired.push('old-kernel'); });
216+
expectInertRegistration(old.cron, NAME);
201217
// Assert the FIRST bind landed before asserting anything about the second.
202218
expect(registeredFor(NAME)).toHaveLength(1);
203219
const oldJob = registeredFor(NAME)[0];
204220

205221
await old.db.destroy(); // kernel evicted by the freshness probe
206222

207223
const rebuilt = kernel();
208-
await rebuilt.db.schedule(NAME, DAILY, async () => { fired.push('new-kernel'); });
224+
await rebuilt.db.schedule(NAME, CRON, async () => { fired.push('new-kernel'); });
225+
expectInertRegistration(rebuilt.cron, NAME);
209226

210227
const held = registeredFor(NAME);
211228
expect(held).toHaveLength(1); // exactly once — not one live + one zombie

packages/services/service-job/src/db-job-adapter.timeout.test.ts

Lines changed: 11 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,14 @@
11
// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
22

33
import { describe, it, expect, beforeEach, afterEach } from 'vitest';
4-
import { Cron, scheduledJobs } from 'croner';
54
import { assertEngineUpdateDispatch } from '@objectstack/metadata-core';
65
import { DbJobAdapter } from './db-job-adapter.js';
76
import { CronJobAdapter } from './cron-job-adapter.js';
7+
import {
8+
NEVER_FIRES_SCHEDULE,
9+
expectFixtureCannotFire,
10+
expectInertRegistration,
11+
} from './never-fires.fixture.js';
812

913
/**
1014
* #7734 — a job that blows its `timeout` must say so in the DURABLE record.
@@ -47,26 +51,14 @@ function makeFakeEngine() {
4751
}
4852

4953
/**
50-
* A cron expression croner PARSES but can never fire: February 30th does not
51-
* exist, so `nextRun()` is `null` and the registration carries no schedule of
52-
* its own. The explicit `trigger()` in each case below is therefore the ONLY
53-
* writer of `sys_job_run`, which is what entitles these cases to assert an
54-
* EXACT row count.
55-
*
56-
* This was `'* * * * *'`, and under that spelling the exact-count assertions
57-
* were a claim about the wall clock rather than about the adapter: when a run
58-
* straddled a minute boundary croner fired the registration on its own, a
59-
* second `sys_job_run` row landed, and CI reddened on a package the offending
60-
* PR had usually not touched (#8628).
61-
*
62-
* `'0 0 29 2 *'` is NOT a substitute — croner resolves Feb 29 to the next leap
63-
* year and it would fire there. Only a date that never occurs is inert.
54+
* The inert cron fixture, now shared with the sibling job suites rather than
55+
* spelled a second time here (#8628, #8748): why February 30th, and why
56+
* `'0 0 29 2 *'` is NOT a substitute, live in `never-fires.fixture.ts`.
6457
*
6558
* Nothing here depends on the registration being schedulable: `trigger()`
6659
* executes the stored record directly and never consults the schedule.
6760
*/
68-
const NEVER_FIRES = '0 0 30 2 *';
69-
const CRON = { type: 'cron', expression: NEVER_FIRES } as const;
61+
const CRON = NEVER_FIRES_SCHEDULE;
7062
const TIMEOUT_MS = 20;
7163
const HANDLER_MS = 300;
7264

@@ -233,7 +225,7 @@ describe('the timeout policy still applies through an injected cron adapter (#77
233225
* machine and reds CI only when a run happens to straddle `:00`.
234226
*/
235227
it('the shared CRON fixture cannot fire on its own', () => {
236-
expect(new Cron(CRON.expression, { timezone: 'UTC' }).nextRun()).toBeNull();
228+
expectFixtureCannotFire(CRON.expression);
237229
});
238230

239231
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
251243
// holds only while that registration owns no schedule of its own. Pin both
252244
// halves: the job is genuinely registered (the case still exercises the
253245
// real adapter) and it will never fire itself (#8628).
254-
const registered = scheduledJobs.find((j) => j.name === cron.cronRegistryName('cronic'));
255-
expect(registered, 'the case must register a REAL croner job').toBeDefined();
256-
expect(registered!.nextRun()).toBeNull();
246+
expectInertRegistration(cron, 'cronic');
257247

258248
await cron.trigger('cronic'); // fire the copy the cron adapter holds
259249

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* The inert cron fixture every exact-count job test in this package schedules
5+
* on, and the two assertions that keep it inert.
6+
*
7+
* Test-only support module. Nothing in `src/index.ts` re-exports it and the
8+
* package's tsup entry is `src/index.ts` alone, so it is type-checked with the
9+
* rest of `src` and shipped in nothing.
10+
*/
11+
12+
import { expect } from 'vitest';
13+
import { Cron, scheduledJobs } from 'croner';
14+
import type { CronJobAdapter } from './cron-job-adapter.js';
15+
16+
/**
17+
* A cron expression croner PARSES but can never fire: February 30th does not
18+
* exist, so `nextRun()` is `null` and a registration on it carries no schedule
19+
* of its own. An explicit `trigger()` is then the ONLY thing that can run the
20+
* handler, which is what entitles a case to assert an EXACT execution count.
21+
*
22+
* These fixtures were spelled `'* * * * *'` (and, in the rarer cases, a real
23+
* daily expression). Under those spellings every exact-count assertion in the
24+
* package was a claim about the wall clock rather than about the adapter:
25+
* `CronJobAdapter.schedule()` builds a REAL croner job, so when a run straddled
26+
* the expression's own instant croner fired the registration alongside the
27+
* explicit `trigger()`, a second execution landed, and CI reddened on a package
28+
* the offending PR had usually not touched.
29+
*
30+
* Measured (#8748) by faking ONLY `Date` — real timers, croner's real
31+
* scheduling path — with the registration placed 2/5/10/15 ms before the
32+
* expression's own instant. Under the firing spellings each case gained
33+
* exactly one extra handler run: `records executions` went 1 → 2 execution
34+
* rows, `retries a failing handler` went 3 → 4 calls (the self-fire re-enters
35+
* a handler whose retry counter is already spent, so it succeeds first try),
36+
* and the daily kernel-rebuild case gained an unasked-for `fired` entry at
37+
* 08:00 UTC. At a 2 ms lead two of the three no longer reproduced — croner had
38+
* already passed the instant by the time it computed `nextRun()` — which is
39+
* exactly why this is a positional flake rather than a deterministic failure.
40+
* Under this fixture all three stay put at every lead.
41+
*
42+
* ⛔ `'0 0 29 2 *'` is NOT a substitute — croner resolves Feb 29 forward to the
43+
* next leap year (measured: `2028-02-29T00:00:00.000Z`) and it would fire
44+
* there. Only a date that never occurs at all is inert.
45+
*
46+
* ⛔ And the remedy is never to loosen the counts. An exact count is the only
47+
* thing in this package that can catch a genuine double-scheduling regression,
48+
* which is precisely what these suites exist to catch.
49+
*/
50+
export const NEVER_FIRES = '0 0 30 2 *';
51+
52+
/** The inert fixture as a `JobSchedule`, ready to hand to `schedule()`. */
53+
export const NEVER_FIRES_SCHEDULE = { type: 'cron', expression: NEVER_FIRES } as const;
54+
55+
/**
56+
* Pin that the FIXTURE itself is inert.
57+
*
58+
* Stated as an assertion and not a comment because the failure it prevents is
59+
* invisible locally: restoring a firing spelling passes on a developer machine
60+
* and reds CI only when a run happens to straddle the expression's instant.
61+
*/
62+
export function expectFixtureCannotFire(expression: string = NEVER_FIRES, timezone = 'UTC'): void {
63+
expect(
64+
new Cron(expression, { timezone }).nextRun(),
65+
`cron fixture "${expression}" must have no next run — otherwise every exact-count assertion in this file is a claim about the wall clock`,
66+
).toBeNull();
67+
}
68+
69+
/**
70+
* Pin that the registration a case ACTUALLY made is inert — both halves.
71+
*
72+
* The fixture pin above is not sufficient on its own: a case can only assert an
73+
* exact count if the job it really registered owns no schedule, and a case that
74+
* registered nothing at all would pass a one-sided check for the wrong reason
75+
* (it would no longer be exercising the real adapter). So: registered, and
76+
* unable to fire.
77+
*/
78+
export function expectInertRegistration(adapter: CronJobAdapter, jobName: string): void {
79+
const registered = scheduledJobs.find((job) => job.name === adapter.cronRegistryName(jobName));
80+
expect(registered, `"${jobName}": the case must register a REAL croner job`).toBeDefined();
81+
expect(
82+
registered!.nextRun(),
83+
`"${jobName}": the registration must own no schedule of its own, or it can fire alongside trigger()`,
84+
).toBeNull();
85+
}

0 commit comments

Comments
 (0)