From 90c87ff1f9b920a726d26ba01a720c7b9d9e9cd5 Mon Sep 17 00:00:00 2001 From: Kriday Dave Date: Tue, 13 Oct 2026 21:51:05 +0530 Subject: [PATCH] fix(scheduler): triggerNow reports a skip instead of claiming success `triggerNow` 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". The second case became reachable when #109 put the per-directory guard in front of every entry point, so a caller asking whether its request was honoured was told yes when nothing ran. `runOrganization` now returns a `PassOutcome`: ran, or skipped with a `not-watched` or `in-flight` reason and the directory the outcome refers to. `triggerNow` returns that directly rather than collapsing it back to a boolean. A discriminated union rather than a string, so a caller cannot read an outcome the code did not produce. The guard is untouched. The cron retry loop gained a comment saying why a skip is not retried: it is a tick that lost the directory, and re-checking a busy directory just spends the attempt budget. Two tests cover it. One corrects the existing "should trigger even if the directory is already in flight", which stubbed `runOrganization` and so never reached the guard it named while pinning `true`. The second exercises the real guard with no stub and asserts the sweep was not called behind the skip. Mutation-checked by restoring the unconditional `true` in `triggerNow`: both go red. Commit made by Step 5 Preview Free in OpenCode on behalf of Kriday. --- CHANGELOG.md | 8 ++ .../scheduler/auto-organize.service.ts | 51 ++++++- tests/unit/services/auto-organize.test.ts | 131 ++++++++++++++++-- 3 files changed, 170 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 99fedb0..3b556dd 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/extensions/scheduler/auto-organize.service.ts b/src/extensions/scheduler/auto-organize.service.ts index e82c96e..2ac1fc1 100644 --- a/src/extensions/scheduler/auto-organize.service.ts +++ b/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 + * @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 { + private async runOrganization(watch: WatchConfig): Promise { 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 { + async triggerNow(directory: string): Promise { 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); } /** diff --git a/tests/unit/services/auto-organize.test.ts b/tests/unit/services/auto-organize.test.ts index fa8ad88..b86f6f8 100644 --- a/tests/unit/services/auto-organize.test.ts +++ b/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,6 +433,8 @@ 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 () => { @@ -427,6 +442,7 @@ describe('AutoOrganizeService', () => { if (callCount === 1) { throw new Error('Simulated error'); } + return { status: 'ran', directory: tempDir }; }); await expect(service.runMissedSchedules()).resolves.not.toThrow(); @@ -443,12 +459,26 @@ 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 { @@ -456,7 +486,7 @@ describe('AutoOrganizeService', () => { } }); - 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 => ({ + 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(); + }); }); });