fix(scheduler): triggerNow reports a skip instead of claiming success

CHANGELOG.md

@@ -146,6 +146,14 @@
   (21 listed against 22 registered). A new `docs-tool-list.test.ts` guard reads
   the real registry and fails when the README list, the headline tool counts, or
   the documented Node version disagree with the code.
+- **`triggerNow` now reports what actually happened.** It returned a `boolean`
+  that was `false` only when the directory was not in the watch list, and `true`
+  for both "a pass ran" and "a pass was skipped because one was already in
+  flight", so a caller asking whether the request was honoured was told yes
+  when the in-flight guard had dropped the pass. `runOrganization` and
+  `triggerNow` return a `PassOutcome` instead: `ran`, or `skipped` with a
+  `not-watched` or `in-flight` reason and the directory it refers to. The
+  serialization guard itself is unchanged; only what gets reported about it is.
 
 ### Added
 

src/extensions/scheduler/auto-organize.service.ts

@@ -24,6 +24,28 @@ import { historyLogger } from "../../services/history-logger.service.js";
 
 export type ConfigLoader = () => UserConfig;
 
+/**
+ * Why an entry point did not start a pass over a directory.
+ *
+ * A pass can be refused because nothing schedules that directory, or because
+ * one is already running over it. Those are different answers to "why did
+ * nothing happen", so a caller is told which one it got.
+ */
+export type PassSkipReason = "not-watched" | "in-flight";
+
+/**
+ * What one request to organize a directory actually did.
+ *
+ * A boolean carried two of the three outcomes and reported a refused pass as a
+ * success, so a caller asking "did it work?" was told yes when the in-flight
+ * guard had dropped the pass. `directory` is the path the outcome refers to:
+ * validated on the two paths that reach the filesystem, and echoed back
+ * verbatim when the directory was never watched.
+ */
+export type PassOutcome =
+  | { status: "ran"; directory: string }
+  | { status: "skipped"; directory: string; reason: PassSkipReason };
+
 /**
  * How long a file must have sat still before a sweep will touch it. A file
  * still being written to is half a file, and organizing it moves the half.
@@ -223,6 +245,10 @@ export class AutoOrganizeService {
             let attempt = 0;
             while (attempt < maxRetries) {
               try {
+                // Only a thrown pass retries. A pass the in-flight guard
+                // refused is a tick that lost the directory to a manual
+                // trigger or a catchup, and retrying it would just re-check
+                // the same busy directory until the attempt budget ran out.
                 await this.runOrganization(watch);
                 break;
               } catch (error) {
@@ -289,9 +315,10 @@ export class AutoOrganizeService {
    * @see startAutoOrganizeScheduler() - For starting all scheduled tasks
    * @see triggerNow() - For manually triggering a single directory
    * @see getStatus() - For checking which directories are mid-pass
-   * @returns Promise<void>
+   * @returns Whether a pass ran, or was skipped because one was already in
+   *   flight for that directory. Throws when the pass itself fails.
    */
-  private async runOrganization(watch: WatchConfig): Promise<void> {
+  private async runOrganization(watch: WatchConfig): Promise<PassOutcome> {
     const { directory, rules } = watch;
 
     // Validate directory path before scanning and organizing
@@ -302,7 +329,11 @@ export class AutoOrganizeService {
     // guard keeps them from racing on the same sources/destinations.
     if (this.inFlight.has(validatedDirectory)) {
       logger.debug(`[${validatedDirectory}] Pass already in flight, skipping`);
-      return;
+      return {
+        status: "skipped",
+        directory: validatedDirectory,
+        reason: "in-flight",
+      };
     }
 
     logger.info(`[${validatedDirectory}] Starting scheduled organization`);
@@ -370,6 +401,8 @@ export class AutoOrganizeService {
     } finally {
       this.inFlight.delete(validatedDirectory);
     }
+
+    return { status: "ran", directory: validatedDirectory };
   }
 
   /**
@@ -409,18 +442,22 @@ export class AutoOrganizeService {
 
   /**
    * Manually trigger organization for a directory
+   *
+   * Three outcomes, because "did it work?" has three honest answers here: the
+   * directory is not watched, a pass is already in flight, or a pass ran. The
+   * old `boolean` collapsed the last two, which told a caller its request had
+   * been honoured when the in-flight guard had dropped it.
    */
-  async triggerNow(directory: string): Promise<boolean> {
+  async triggerNow(directory: string): Promise<PassOutcome> {
     const userConfig = this.configLoader();
     const watch = userConfig.watchList?.find((w) => w.directory === directory);
 
     if (!watch) {
       logger.error(`Directory not in watch list: ${directory}`);
-      return false;
+      return { status: "skipped", directory, reason: "not-watched" };
     }
 
-    await this.runOrganization(watch);
-    return true;
+    return await this.runOrganization(watch);
   }
 
   /**

tests/unit/services/auto-organize.test.ts

@@ -232,9 +232,10 @@ describe('AutoOrganizeService', () => {
         },
       );
 
-      await service.triggerNow(tempDir);
+      const result = await service.triggerNow(tempDir);
 
       expect(calls).toHaveLength(1);
+      expect(result).toEqual({ status: 'ran', directory: tempDir });
       expect(calls[0]).toMatchObject({
         directory: tempDir,
         dryRun: false,
@@ -294,7 +295,15 @@ describe('AutoOrganizeService', () => {
       // the gate comes back.
       (service as any).inFlight = new Map([[tempDir, 1]]);
 
-      const runOrgSpy = jest.spyOn(service as any, 'runOrganization').mockResolvedValue(undefined);
+      // What the guard would decide here, so the stub answers the way the real
+      // runOrganization does for a directory already mid-pass.
+      const runOrgSpy = jest
+        .spyOn(service as any, 'runOrganization')
+        .mockResolvedValue({
+          status: 'skipped',
+          directory: tempDir,
+          reason: 'in-flight',
+        });
 
       await service.runMissedSchedules();
 
@@ -342,7 +351,11 @@ describe('AutoOrganizeService', () => {
 
         const runOrgSpy = jest
           .spyOn(service as any, 'runOrganization')
-          .mockResolvedValue(undefined);
+          .mockResolvedValue({
+            status: 'skipped',
+            directory: dir1,
+            reason: 'in-flight',
+          });
 
         await service.runMissedSchedules();
 
@@ -420,13 +433,16 @@ describe('AutoOrganizeService', () => {
         );
 
         let callCount = 0;
+        // Only the call count matters here, so the stub reports a plain
+        // completed pass for whichever directory the catchup handed it.
         const runOrgSpy = jest
           .spyOn(service as any, 'runOrganization')
           .mockImplementation(async () => {
             callCount++;
             if (callCount === 1) {
               throw new Error('Simulated error');
             }
+            return { status: 'ran', directory: tempDir };
           });
 
         await expect(service.runMissedSchedules()).resolves.not.toThrow();
@@ -443,20 +459,34 @@ describe('AutoOrganizeService', () => {
 
   describe('triggerNow', () => {
     let tempDir: string;
+    let originalCustomAllowed: string[];
 
     beforeEach(async () => {
-      tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'trigger-test-'));
+      // Canonicalise once, for the same reason as the catchup describe:
+      // validateStrictPath resolves through realpath, so the key the in-flight
+      // guard uses is the canonical path rather than the mkdtemp spelling.
+      tempDir = await fs.realpath(
+        await fs.mkdtemp(path.join(os.tmpdir(), 'trigger-test-')),
+      );
+      // The skip tests below run the real runOrganization, which validates the
+      // directory against the allow-list. os.tmpdir() is not in the default
+      // roots, so it is whitelisted for the duration of the test.
+      const { CONFIG } = await import('../../../src/core/config/defaults.js');
+      originalCustomAllowed = CONFIG.paths.customAllowed;
+      CONFIG.paths.customAllowed = [...originalCustomAllowed, tempDir];
     });
 
     afterEach(async () => {
+      const { CONFIG } = await import('../../../src/core/config/defaults.js');
+      CONFIG.paths.customAllowed = originalCustomAllowed;
       try {
         await fs.rm(tempDir, { recursive: true, force: true });
       } catch {
         // Ignore cleanup errors
       }
     });
 
-    it('should return false when directory is not in watch list', async () => {
+    it('should report a skip when directory is not in watch list', async () => {
       const { AutoOrganizeService } =
         await import('../../../src/extensions/scheduler/auto-organize.service.js');
 
@@ -465,12 +495,16 @@ describe('AutoOrganizeService', () => {
         () => mockConfig
       );
 
-      const result = await service.triggerNow('/nonexistent');
-      expect(result).toBe(false);
+      const result = await service.triggerNow(tempDir);
+      expect(result).toEqual({
+        status: 'skipped',
+        directory: tempDir,
+        reason: 'not-watched',
+      });
       service.stop();
     });
 
-    it('should return true and run organization when directory is in watch list', async () => {
+    it('should return the pass outcome when directory is in watch list', async () => {
       const { AutoOrganizeService } =
         await import('../../../src/extensions/scheduler/auto-organize.service.js');
 
@@ -486,18 +520,21 @@ describe('AutoOrganizeService', () => {
         () => mockConfig
       );
 
-      const runOrgSpy = jest.spyOn(service as any, 'runOrganization').mockResolvedValue(undefined);
+      const runOrgSpy = jest.spyOn(service as any, 'runOrganization').mockResolvedValue({
+        status: 'ran',
+        directory: tempDir,
+      });
 
       const result = await service.triggerNow(tempDir);
 
-      expect(result).toBe(true);
+      expect(result).toEqual({ status: 'ran', directory: tempDir });
       expect(runOrgSpy).toHaveBeenCalledTimes(1);
 
       runOrgSpy.mockRestore();
       service.stop();
     });
 
-    it('should trigger even if the directory is already in flight', async () => {
+    it('reports the skip when the pass is already in flight', async () => {
       const { AutoOrganizeService } =
         await import('../../../src/extensions/scheduler/auto-organize.service.js');
 
@@ -515,16 +552,84 @@ describe('AutoOrganizeService', () => {
 
       (service as any).inFlight = new Map([[tempDir, 1]]);
 
-      const runOrgSpy = jest.spyOn(service as any, 'runOrganization').mockResolvedValue(undefined);
+      // Stubbed so the test pins triggerNow alone: it must hand back whatever
+      // runOrganization decided instead of reporting success. The version that
+      // stubbed this with `mockResolvedValue(undefined)` asserted `true` for a
+      // pass that never ran, which is the returned-value lie.
+      const runOrgSpy = jest.spyOn(service as any, 'runOrganization').mockResolvedValue({
+        status: 'skipped',
+        directory: tempDir,
+        reason: 'in-flight',
+      });
 
       const result = await service.triggerNow(tempDir);
 
-      expect(result).toBe(true);
+      expect(result).toEqual({
+        status: 'skipped',
+        directory: tempDir,
+        reason: 'in-flight',
+      });
       expect(runOrgSpy).toHaveBeenCalledTimes(1);
 
       (service as any).inFlight = new Map();
       runOrgSpy.mockRestore();
       service.stop();
     });
+
+    it('reports a skip from the real in-flight guard, with no pass behind it', async () => {
+      const { AutoOrganizeService } =
+        await import('../../../src/extensions/scheduler/auto-organize.service.js');
+
+      const watches: WatchConfig[] = [
+        {
+          directory: tempDir,
+          schedule: '0 9 * * *',
+          rules: { auto_organize: true },
+        },
+      ];
+
+      // No stub on runOrganization this time. The sweep is injected so that
+      // "the guard refused it" is observable: a pass that was never handed to
+      // the sweep means the outcome came from the guard and not from a pass.
+      const sweep = jest.fn(
+        async (): Promise<OrganizePassResult> => ({
+          directory: tempDir,
+          dryRun: false,
+          scanned: 0,
+          planned: 0,
+          moved: 0,
+          skipped: 0,
+          statistics: {},
+          errors: [],
+          aborted: false,
+          historyLogged: false,
+        }),
+      );
+      const service = new AutoOrganizeService(
+        () => ({ watchList: watches }),
+        undefined,
+        sweep,
+      );
+
+      // Seeded with the canonical directory, which is the key runOrganization
+      // compares after validateStrictPath resolves it.
+      (service as any).inFlight = new Map([[tempDir, 1]]);
+
+      const result = await service.triggerNow(tempDir);
+
+      expect(result).toEqual({
+        status: 'skipped',
+        directory: tempDir,
+        reason: 'in-flight',
+      });
+      // The setup fired: the pass was refused before the sweep was reached.
+      expect(sweep).not.toHaveBeenCalled();
+      // The seeded counter survives a refusal. A skip that returned after
+      // claiming the directory, or from a finally that ran anyway, would clear
+      // it and un-guard the pass that is genuinely still in flight.
+      expect(service.getStatus().runningDirectories).toEqual([tempDir]);
+
+      service.stop();
+    });
   });
 });