Skip to content

Commit 74aa3f0

Browse files
baozhoutaoclaude
andauthored
fix(service-job): migrate early registrations across the placeholder→DbJobAdapter upgrade (#4172)
Business plugins start() before kernel:ready, so their schedules all land on the placeholder IntervalJobAdapter — which silently ignores cron schedules — and the upgrade swapped the service without migrating anything. Default config result: cron jobs never ran at all, while interval timers kept running on the orphaned placeholder, invisible to sys_job. The upgrade now snapshots every early registration, stops the placeholder, and re-schedules them on the DbJobAdapter. The IntervalJobAdapter warns per cron registration it cannot execute, and the no-engine path summarizes the stranded cron jobs. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent 8c2db68 commit 74aa3f0

4 files changed

Lines changed: 225 additions & 3 deletions

File tree

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
---
2+
'@objectstack/service-job': patch
3+
---
4+
5+
Jobs registered before `kernel:ready` now survive the placeholder→DbJobAdapter upgrade.
6+
7+
Business plugins `start()` before the JobServicePlugin's `kernel:ready` hook, so every schedule they register lands on the placeholder IntervalJobAdapter. That placeholder silently ignores `cron` schedules, and the upgrade used `replaceService` without migrating anything — so in the default configuration a plugin's cron jobs never ran at all, while its interval timers kept running on the orphaned placeholder, invisible to `sys_job`. The upgrade now snapshots every early registration, stops the placeholder, and re-schedules them on the DbJobAdapter (whose croner-backed cron routing makes the cron entries actually fire). The IntervalJobAdapter also warns per cron registration, and the no-engine path summarizes stranded cron jobs instead of staying silent.

packages/services/service-job/src/interval-job-adapter.ts

Lines changed: 42 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,25 @@ interface JobRecord {
2121
export interface IntervalJobAdapterOptions {
2222
/** Maximum number of execution records to retain per job (default: 100) */
2323
maxExecutions?: number;
24+
/**
25+
* Logger used to surface schedules this adapter cannot execute. A `cron`
26+
* registration is stored but never fired here, and staying silent about it
27+
* meant "background automation off" with zero diagnostics: every
28+
* plugin that registered its jobs before the DbJobAdapter upgrade saw
29+
* interval jobs run and cron jobs do nothing.
30+
*/
31+
logger?: { warn(message: string, ...args: unknown[]): void };
32+
}
33+
34+
/**
35+
* A job registration as originally supplied to {@link IntervalJobAdapter.schedule},
36+
* exposed so an upgraded adapter can re-schedule it (see JobServicePlugin).
37+
*/
38+
export interface JobRegistration {
39+
name: string;
40+
schedule: JobSchedule;
41+
handler: JobHandler;
42+
options?: JobScheduleOptions;
2443
}
2544

2645
/**
@@ -35,9 +54,11 @@ export interface IntervalJobAdapterOptions {
3554
export class IntervalJobAdapter implements IJobService {
3655
private readonly jobs = new Map<string, JobRecord>();
3756
private readonly maxExecutions: number;
57+
private readonly logger?: IntervalJobAdapterOptions['logger'];
3858

3959
constructor(options: IntervalJobAdapterOptions = {}) {
4060
this.maxExecutions = options.maxExecutions ?? 100;
61+
this.logger = options.logger;
4162
}
4263

4364
async schedule(name: string, schedule: JobSchedule, handler: JobHandler, options?: JobScheduleOptions): Promise<void> {
@@ -57,12 +78,32 @@ export class IntervalJobAdapter implements IJobService {
5778
await this.executeJob(record);
5879
}, delay);
5980
}
81+
} else if (schedule.type === 'cron') {
82+
// Stored but NOT executed — this adapter has no cron engine. Loud on
83+
// purpose: registrations that land here before the DbJobAdapter upgrade
84+
// are re-delivered by JobServicePlugin, but an adapter that KEEPS them
85+
// (explicit `adapter: 'interval'`, or no ObjectQL engine) silently ran
86+
// zero cron jobs.
87+
this.logger?.warn(
88+
`IntervalJobAdapter: cron schedule "${name}" registered but will not run — ` +
89+
'this adapter has no cron engine. Use the db/cron adapter, or an interval schedule.',
90+
);
6091
}
61-
// 'cron' type: stored but not actively scheduled (needs cron library)
6292

6393
this.jobs.set(name, record);
6494
}
6595

96+
/**
97+
* Snapshot of every registration as originally supplied, for handing over to
98+
* a replacement adapter. Does not mutate this adapter's state — callers
99+
* migrate with `getRegistrations()` then `destroy()`.
100+
*/
101+
getRegistrations(): JobRegistration[] {
102+
return [...this.jobs.values()].map(({ name, schedule, handler, options }) => ({
103+
name, schedule, handler, options,
104+
}));
105+
}
106+
66107
async cancel(name: string): Promise<void> {
67108
const record = this.jobs.get(name);
68109
if (record?.timerId) {
Lines changed: 139 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,139 @@
1+
// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* Regression: the placeholder→DbJobAdapter upgrade must not strand
5+
* early registrations.
6+
*
7+
* Business plugins `start()` before `kernel:ready`, so every job they register
8+
* lands on the placeholder IntervalJobAdapter. Before this fix:
9+
* 1. the placeholder silently ignored `cron` schedules (no timer, no error);
10+
* 2. the upgrade swapped the service via `replaceService` WITHOUT migrating
11+
* the already-registered jobs — cron entries were lost for good;
12+
* 3. the placeholder's interval timers kept running on the orphaned adapter
13+
* (which is why interval jobs appeared healthy while cron jobs never
14+
* fired, and none of them showed up in sys_job).
15+
*/
16+
17+
import { describe, it, expect, vi } from 'vitest';
18+
import { JobServicePlugin } from './job-service-plugin';
19+
20+
function makeFakeEngine() {
21+
const tables = new Map<string, any[]>();
22+
return {
23+
tables,
24+
async find(table: string, opts: any = {}) {
25+
const t = tables.get(table) ?? [];
26+
return opts.where
27+
? t.filter((r) => Object.entries(opts.where).every(([k, v]) => r[k] === v))
28+
: [...t];
29+
},
30+
async insert(table: string, data: any) {
31+
const t = tables.get(table) ?? [];
32+
t.push({ ...data });
33+
tables.set(table, t);
34+
return { id: data.id };
35+
},
36+
async update(table: string, patch: any) {
37+
const t = tables.get(table) ?? [];
38+
const r = t.find((x) => x.id === patch.id);
39+
if (!r) throw new Error(`row ${patch.id} not in ${table}`);
40+
Object.assign(r, patch);
41+
return r;
42+
},
43+
};
44+
}
45+
46+
function makeFakeContext(opts: { engine?: any } = {}) {
47+
const services = new Map<string, any>();
48+
const hooks = new Map<string, Array<() => Promise<void> | void>>();
49+
if (opts.engine) services.set('objectql', opts.engine);
50+
const logger = {
51+
info: vi.fn(),
52+
warn: vi.fn(),
53+
error: vi.fn(),
54+
debug: vi.fn(),
55+
};
56+
const ctx = {
57+
logger,
58+
registerService: (name: string, impl: any) => { services.set(name, impl); },
59+
replaceService: (name: string, impl: any) => { services.set(name, impl); },
60+
getService: (name: string) => {
61+
if (!services.has(name)) throw new Error(`service "${name}" not registered`);
62+
return services.get(name);
63+
},
64+
hook: (event: string, cb: () => Promise<void> | void) => {
65+
const list = hooks.get(event) ?? [];
66+
list.push(cb);
67+
hooks.set(event, list);
68+
},
69+
async fire(event: string) {
70+
for (const cb of hooks.get(event) ?? []) await cb();
71+
},
72+
services,
73+
};
74+
return ctx;
75+
}
76+
77+
describe('JobServicePlugin placeholder→DbJobAdapter upgrade', () => {
78+
it('migrates jobs registered before kernel:ready onto the DbJobAdapter', async () => {
79+
const engine = makeFakeEngine();
80+
const ctx = makeFakeContext({ engine });
81+
const plugin = new JobServicePlugin();
82+
await plugin.init(ctx as any);
83+
84+
// Simulate a business plugin registering during its start(): the service
85+
// resolved here is the placeholder IntervalJobAdapter.
86+
const placeholder = ctx.getService('job');
87+
const cronRuns: string[] = [];
88+
await placeholder.schedule(
89+
'nightly-report',
90+
{ type: 'cron', expression: '0 0 * * *' },
91+
async () => { cronRuns.push('ran'); },
92+
);
93+
await placeholder.schedule(
94+
'heartbeat',
95+
{ type: 'interval', intervalMs: 60_000 },
96+
async () => {},
97+
);
98+
99+
await ctx.fire('kernel:ready');
100+
101+
// Service was swapped…
102+
const upgraded = ctx.getService('job');
103+
expect(upgraded).not.toBe(placeholder);
104+
// …and BOTH early registrations followed it (the cron one used to vanish).
105+
expect((await upgraded.listJobs()).sort()).toEqual(['heartbeat', 'nightly-report']);
106+
// The cron job is now persisted where operators can see it.
107+
const sysJobs = engine.tables.get('sys_job') ?? [];
108+
expect(sysJobs.map((r: any) => r.name).sort()).toEqual(['heartbeat', 'nightly-report']);
109+
// The orphaned placeholder no longer owns any jobs (its timers are stopped).
110+
expect(await placeholder.listJobs()).toEqual([]);
111+
// The migrated cron handler still works when triggered through the new adapter.
112+
await upgraded.trigger('nightly-report');
113+
expect(cronRuns).toEqual(['ran']);
114+
115+
await plugin.destroy();
116+
});
117+
118+
it('warns loudly when cron jobs stay stranded on the interval adapter (no engine)', async () => {
119+
const ctx = makeFakeContext();
120+
const plugin = new JobServicePlugin();
121+
await plugin.init(ctx as any);
122+
123+
const job = ctx.getService('job');
124+
await job.schedule('never-runs', { type: 'cron', expression: '*/5 * * * *' }, async () => {});
125+
126+
// Registration itself already warned (the per-schedule diagnostic).
127+
expect(ctx.logger.warn).toHaveBeenCalledWith(expect.stringContaining('never-runs'));
128+
129+
await ctx.fire('kernel:ready');
130+
131+
// And the no-upgrade path summarizes the stranded cron jobs.
132+
const warned = ctx.logger.warn.mock.calls.some(
133+
(c: any[]) => typeof c[0] === 'string' && c[0].includes('will NOT run') && c[0].includes('never-runs'),
134+
);
135+
expect(warned).toBe(true);
136+
137+
await plugin.destroy();
138+
});
139+
});

packages/services/service-job/src/job-service-plugin.ts

Lines changed: 37 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ export class JobServicePlugin implements Plugin {
8383
const choice = this.options.adapter ?? 'auto';
8484

8585
if (choice === 'interval') {
86-
this.intervalAdapter = new IntervalJobAdapter(this.options.interval);
86+
this.intervalAdapter = new IntervalJobAdapter({ ...this.options.interval, logger: ctx.logger });
8787
ctx.registerService('job', this.intervalAdapter);
8888
ctx.logger.info('JobServicePlugin: registered IntervalJobAdapter (in-memory)');
8989
return;
@@ -99,7 +99,7 @@ export class JobServicePlugin implements Plugin {
9999
// 'auto' or 'db' — register a placeholder Interval adapter synchronously
100100
// so callers can `getService('job')` during init, then upgrade in kernel:ready
101101
// when the objectql engine is wired.
102-
this.intervalAdapter = new IntervalJobAdapter(this.options.interval);
102+
this.intervalAdapter = new IntervalJobAdapter({ ...this.options.interval, logger: ctx.logger });
103103
ctx.registerService('job', this.intervalAdapter);
104104

105105
ctx.hook('kernel:ready', async () => {
@@ -113,6 +113,17 @@ export class JobServicePlugin implements Plugin {
113113
} else {
114114
ctx.logger.info('JobServicePlugin: no ObjectQL engine — staying on IntervalJobAdapter');
115115
}
116+
// Jobs stuck on the placeholder include cron schedules that
117+
// will never fire. The per-registration warning already fired, but the
118+
// summary makes "background automation is off" visible in one line.
119+
const stranded = this.intervalAdapter?.getRegistrations()
120+
.filter((r) => r.schedule.type === 'cron')
121+
.map((r) => r.name) ?? [];
122+
if (stranded.length > 0) {
123+
ctx.logger.warn(
124+
`JobServicePlugin: ${stranded.length} cron job(s) will NOT run on IntervalJobAdapter: ${stranded.join(', ')}`,
125+
);
126+
}
116127
return;
117128
}
118129

@@ -138,6 +149,30 @@ export class JobServicePlugin implements Plugin {
138149
ctx.logger.info('JobServicePlugin: upgraded to DbJobAdapter (sys_job + sys_job_run persistence)');
139150
} catch (err) {
140151
ctx.logger.warn('JobServicePlugin: replaceService failed; staying on IntervalJobAdapter', err as any);
152+
return;
153+
}
154+
155+
// Migrate every registration made against the placeholder.
156+
// Business plugins `start()` before this hook runs, so their schedules
157+
// all landed on the IntervalJobAdapter: its cron entries never fired at
158+
// all, and its interval timers would keep running on the orphaned
159+
// placeholder (invisible to sys_job) after the swap. Stop the placeholder
160+
// FIRST — a brief gap beats a double-fire — then re-schedule everything
161+
// on the DbJobAdapter.
162+
const pending = this.intervalAdapter?.getRegistrations() ?? [];
163+
if (this.intervalAdapter) {
164+
await this.intervalAdapter.destroy();
165+
this.intervalAdapter = undefined;
166+
}
167+
for (const r of pending) {
168+
try {
169+
await this.dbAdapter.schedule(r.name, r.schedule, r.handler, r.options);
170+
} catch (err) {
171+
ctx.logger.warn(`JobServicePlugin: failed to migrate job "${r.name}" to DbJobAdapter`, err as any);
172+
}
173+
}
174+
if (pending.length > 0) {
175+
ctx.logger.info(`JobServicePlugin: migrated ${pending.length} early job registration(s) to DbJobAdapter`);
141176
}
142177

143178
// Retention is owned by the platform LifecycleService (ADR-0057):

0 commit comments

Comments
 (0)