From 18ba27d8d2577f01d1574560454c0f973a603928 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20My=C5=9Bliwiec?= Date: Thu, 22 Oct 2026 08:59:21 +0200 Subject: [PATCH] fix: key inherited jobs by the provider's class A method decorated with @OnOneInstance() or @WithoutOverlapping() on a base class took its default key from the class that declares it, so every subclass shared one run lock and one lease: one subclass's runs were skipped while another's ran, and the startup check, which compared job definitions, never saw two jobs. The default key, the lease and the log lines now follow the instance's class (SalesReport.run, StockReport.run), and the collision check is per instance, so one class provided twice without a key fails at startup too. A `useExisting` alias of the same instance is not a second job. Closes #38 --- lib/services/scheduled-jobs.service.ts | 38 ++++++++++++--------- lib/utils/job-decorator.util.ts | 9 +++-- lib/utils/job-registry.util.ts | 16 ++++++--- tests/scheduling.spec.ts | 46 ++++++++++++++++++++++++++ 4 files changed, 85 insertions(+), 24 deletions(-) diff --git a/lib/services/scheduled-jobs.service.ts b/lib/services/scheduled-jobs.service.ts index 1c000bd..af3a923 100644 --- a/lib/services/scheduled-jobs.service.ts +++ b/lib/services/scheduled-jobs.service.ts @@ -83,8 +83,11 @@ export class ScheduledJobs implements OnModuleInit, OnModuleDestroy { ) {} onModuleInit() { - /** Which job each key came from: two different jobs may share a key only if both set it. */ - const owners = new Map(); + /** + * Which job each key came from: two different jobs (another method, or the same one on + * another instance) may share a key only if both set it. + */ + const owners = new Map(); /** The jobs on each key, for the collision check against the other keys and the elections. */ const jobs = new Map(); @@ -98,27 +101,33 @@ export class ScheduledJobs implements OnModuleInit, OnModuleDestroy { if (!definition) { continue; } + // Named after the instance's class, not the one declaring the method: each subclass + // that inherits a decorated method is a job of its own. + const job = describeJob(definition, instance); if (!wrapper.isDependencyTreeStatic()) { this.logger.warn( - `${describeJob(definition)} is decorated with ${decoratorsOf(definition)}, but ${wrapper.name as string} is ` + + `${job} is decorated with ${decoratorsOf(definition)}, but ${wrapper.name as string} is ` + 'not a singleton provider (it is request-scoped or transient, or depends on one), so its calls will fail.', ); continue; } - const key = definition.key ?? definition.derivedKey; + const key = definition.key ?? job; const previous = owners.get(key); - if (previous && previous !== definition && (previous.key === undefined || definition.key === undefined)) { + if (previous && previous.instance === instance && previous.definition === definition) { + continue; // the same provider under another token (`useExisting`) + } + if (previous && (previous.definition.key === undefined || definition.key === undefined)) { throw new Error( - `LocksModule: ${describeJob(previous)} and ${describeJob(definition)} both use the lock key "${key}". ` + + `LocksModule: ${previous.job} and ${job} both use the lock key "${key}". ` + 'Two jobs share a key only when both set it on purpose (to exclude each other); set `key` on ' + - `${decoratorsOf(definition.key === undefined ? definition : previous)} to tell them apart.`, + `${decoratorsOf(definition.key === undefined ? definition : previous.definition)} to tell them apart.`, ); } - owners.set(key, definition); - if (!jobs.get(key)?.includes(describeJob(definition))) { - jobs.set(key, [...(jobs.get(key) ?? []), describeJob(definition)]); + owners.set(key, { instance, definition, job }); + if (!jobs.get(key)?.includes(job)) { + jobs.set(key, [...(jobs.get(key) ?? []), job]); } - this.register(instance, definition, key); + this.register(instance, definition, key, job); } } // A job holds `key` (its run lock) and `key:owner` (its lease): neither may be another @@ -154,13 +163,13 @@ export class ScheduledJobs implements OnModuleInit, OnModuleDestroy { ); } - private register(instance: object, definition: JobDefinition, key: string) { + private register(instance: object, definition: JobDefinition, key: string, job: string) { const internals = this.locks[LOCKS_INTERNALS]; let state = this.states.get(key); if (!state) { this.states.set(key, (state = new KeyState(key, internals, this.logger))); } - const runner = this.runner(definition, state, internals); + const runner = this.runner(definition, job, state, internals); let runners = jobRunners.get(instance); if (!runners) { jobRunners.set(instance, (runners = new Map())); @@ -168,8 +177,7 @@ export class ScheduledJobs implements OnModuleInit, OnModuleDestroy { runners.set(definition, runner); } - private runner(definition: JobDefinition, state: KeyState, internals: LocksInternals): JobRunner { - const job = describeJob(definition); + private runner(definition: JobDefinition, job: string, state: KeyState, internals: LocksInternals): JobRunner { const skip = (why: string) => { this.logger.debug(`Skipped ${job}: ${why}`); return undefined; diff --git a/lib/utils/job-decorator.util.ts b/lib/utils/job-decorator.util.ts index f5a9d6a..df5f179 100644 --- a/lib/utils/job-decorator.util.ts +++ b/lib/utils/job-decorator.util.ts @@ -1,6 +1,6 @@ import { JOB_DEFINITION } from '../locks.constants.js'; import type { OnOneInstanceOptions, WithoutOverlappingOptions } from '../interfaces/job-options.interface.js'; -import { decoratorsOf, describeJob, jobRunners, type JobDefinition } from './job-registry.util.js'; +import { decoratorsOf, describeJob, jobClassName, jobRunners, type JobDefinition } from './job-registry.util.js'; import { ttlMs } from './ttl.util.js'; type Kind = 'oneInstance' | 'withoutOverlapping'; @@ -64,15 +64,14 @@ function wrap(method: (...args: unknown[]) => unknown, target: object, propertyK const definition: JobDefinition = { className: target.constructor.name, methodName: String(propertyKey), - derivedKey: `${target.constructor.name}.${String(propertyKey)}`, }; const wrapper: Wrapped = async function (this: object, ...args: unknown[]) { const runner = jobRunners.get(this)?.get(definition); if (!runner) { throw new Error( - `${describeJob(definition)} is decorated with ${decoratorsOf(definition)}, but LocksModule doesn't know this ` + - `instance, so it can't tell whether this call may run. Import LocksModule in the application, and provide ` + - `${definition.className} as a singleton provider (not request-scoped or transient, not created with \`new\`).`, + `${describeJob(definition, this)} is decorated with ${decoratorsOf(definition)}, but LocksModule doesn't know ` + + `this instance, so it can't tell whether this call may run. Import LocksModule in the application, and provide ` + + `${jobClassName(definition, this)} as a singleton provider (not request-scoped or transient, not created with \`new\`).`, ); } diff --git a/lib/utils/job-registry.util.ts b/lib/utils/job-registry.util.ts index ed5e65c..6a9d288 100644 --- a/lib/utils/job-registry.util.ts +++ b/lib/utils/job-registry.util.ts @@ -1,9 +1,8 @@ /** What `@OnOneInstance()` and `@WithoutOverlapping()` record on the method they wrap. */ export interface JobDefinition { + /** The class that declares the method: its subclasses inherit the job, each with a key of its own. */ className: string; methodName: string; - /** `ClassName.methodName`: the key when neither decorator sets one. */ - derivedKey: string; /** Set by either decorator. */ key?: string; /** `@OnOneInstance()`: the ownership lease's ttl in ms, or the module's. */ @@ -24,8 +23,17 @@ export interface JobRunner { */ export const jobRunners = new WeakMap>(); -export function describeJob(definition: JobDefinition): string { - return `${definition.className}.${definition.methodName}`; +/** + * `ClassName.methodName`, named after the instance's class when given one: the key when neither + * decorator sets one. A subclass that inherits a decorated method is a job of its own. + */ +export function describeJob(definition: JobDefinition, instance?: object): string { + return `${jobClassName(definition, instance)}.${definition.methodName}`; +} + +/** The instance's class when given one, else the class that declares the method. */ +export function jobClassName(definition: JobDefinition, instance?: object): string { + return (instance && (instance.constructor as { name?: string } | undefined)?.name) || definition.className; } export function decoratorsOf(definition: JobDefinition): string { diff --git a/tests/scheduling.spec.ts b/tests/scheduling.spec.ts index 11b82f5..6b157a2 100644 --- a/tests/scheduling.spec.ts +++ b/tests/scheduling.spec.ts @@ -403,6 +403,52 @@ describe('@OnOneInstance() and @WithoutOverlapping()', () => { await exporting; }); + it('gives each subclass that inherits a decorated method a key of its own', async () => { + abstract class ReportJob { + @OnOneInstance() + @WithoutOverlapping() + async run() { + runs.push({ job: this.constructor.name, instance: 'a' }); + await gate; + } + } + @Injectable() + class SalesReport extends ReportJob {} + @Injectable() + class StockReport extends ReportJob {} + const app = await startInstance({ providers: [SalesReport, StockReport], store, locks: { clock }, logger }); + apps.push(app); + const hold = deferred(); + gate = hold.promise; + const sales = app.get(SalesReport).run(); + const stock = app.get(StockReport).run(); + await until(() => runs.length === 2); // neither excludes the other + expect(await app.get(SalesReport).run()).toBeUndefined(); // but each still excludes itself + expect(logger.matching('DEBUG Skipped SalesReport.run: a run is still in progress')).toHaveLength(1); + hold.resolve(); + await Promise.all([sales, stock]); + expect(runs.map((r) => r.job).sort()).toEqual(['SalesReport', 'StockReport']); + expect(logger.matching(/runs on this instance \(lease "(Sales|Stock)Report\.run:owner"/)).toHaveLength(2); + }); + + it('refuses one job class provided twice under its default key, but not an alias of the same instance', async () => { + @Injectable() + class Sync { + @WithoutOverlapping() + run() {} + } + const twice = await Test.createTestingModule({ + imports: [LocksModule.forRoot()], + providers: [Sync, { provide: 'second', useClass: Sync }], + }).compile(); + twice.useLogger(false); + await expect(twice.init()).rejects.toThrow('LocksModule: Sync.run and Sync.run both use the lock key "Sync.run".'); + + const app = await startInstance({ providers: [Sync, { provide: 'alias', useExisting: Sync }], store, locks: { clock }, logger }); + apps.push(app); + expect(app.get('alias')).toBe(app.get(Sync)); + }); + it("refuses a job whose key is another job's lease (`:owner`)", async () => { @Injectable() class Nightly {