fix: key inherited jobs by the provider's class

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<string, JobDefinition>();
+    /**
+     * 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<string, { instance: object; definition: JobDefinition; job: string }>();
     /** The jobs on each key, for the collision check against the other keys and the elections. */
     const jobs = new Map<string, string[]>();
 
@@ -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,22 +163,21 @@ 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()));
     }
     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;

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\`).`,
       );
     }
 

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<object, Map<JobDefinition, JobRunner>>();
 
-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 {

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 (`<key>:owner`)", async () => {
       @Injectable()
       class Nightly {