diff --git a/src/main/services/infrastructure/TeamTaskWatchRegistry.ts b/src/main/services/infrastructure/TeamTaskWatchRegistry.ts index 64d6ddb5..7dc6874a 100644 --- a/src/main/services/infrastructure/TeamTaskWatchRegistry.ts +++ b/src/main/services/infrastructure/TeamTaskWatchRegistry.ts @@ -71,7 +71,6 @@ export class TeamTaskWatchRegistry { private reconcileTimer: NodeJS.Timeout | null = null; private targets = new Set(); private targetKey = ''; - private initialTargetsCaptured = false; private closed = false; private generation = 0; private reconcileInProgress = false; @@ -164,8 +163,7 @@ export class TeamTaskWatchRegistry { const targets = await this.collectTargets(); const nextKey = targets.join('\n'); if (nextKey !== this.targetKey) { - const addedTargets = targets.filter((target) => !this.targets.has(target)); - await this.rebuildWatcher(targets, nextKey, addedTargets); + await this.applyTargetSet(targets, nextKey); } } catch (error) { if (options.rethrowErrors) { @@ -184,38 +182,57 @@ export class TeamTaskWatchRegistry { } } - private async rebuildWatcher( - targets: string[], - nextKey: string, - addedTargets: string[] - ): Promise { - const generation = this.generation + 1; - this.generation = generation; - - const previousWatcher = this.watcher; - this.watcher = null; - if (previousWatcher) { - await this.closeWatcher(previousWatcher); + private async applyTargetSet(targets: string[], nextKey: string): Promise { + if (this.closed) { + return; } - - if (this.closed || generation !== this.generation) { + // First time: create the watcher with the full target set. ignoreInitial keeps + // the app-startup baseline silent so old files are not replayed. + if (!this.watcher) { + this.createWatcher(targets, nextKey); return; } - const nextWatcher = watch(targets, { + // Incrementally update the existing watcher rather than tearing it down and + // recreating it. A full rebuild re-opens an fd for EVERY watched file (kqueue + // on macOS opens one fd per file), so during a launch that adds dirs in bursts + // it re-opened the entire (large) watched set repeatedly. add()/unwatch() touch + // only the delta. emitExistingFilesForNewTargets still backfills files that + // already exist in newly added dirs, preserving the previous event surface + // (chokidar's own add() scan only re-confirms those same files, idempotently). + const nextSet = new Set(targets); + const addedTargets = targets.filter((target) => !this.targets.has(target)); + const removedTargets = [...this.targets].filter((target) => !nextSet.has(target)); + const generation = this.generation; + + if (removedTargets.length > 0) { + this.watcher.unwatch(removedTargets); + } + if (addedTargets.length > 0) { + this.watcher.add(addedTargets); + } + this.targets = nextSet; + this.targetKey = nextKey; + + if (addedTargets.length > 0) { + await this.emitExistingFilesForNewTargets(addedTargets, generation); + } + } + + private createWatcher(targets: string[], nextKey: string): void { + const generation = this.generation + 1; + this.generation = generation; + + const watcher = watch(targets, { ignoreInitial: true, ignorePermissionErrors: true, followSymlinks: false, depth: 0, }); - this.watcher = nextWatcher; + this.watcher = watcher; this.targets = new Set(targets); this.targetKey = nextKey; - // First registry build is app startup baseline and must not emit old files. - // Later rebuilds can emit existing files only for newly added targets. - const shouldEmitExistingFiles = this.initialTargetsCaptured; - this.initialTargetsCaptured = true; const handleEvent = (eventType: TeamTaskWatchEventType, changedPath?: string): void => { if (this.closed || generation !== this.generation || !changedPath) { @@ -229,7 +246,7 @@ export class TeamTaskWatchRegistry { // addDir/unlinkDir can make the watch target set stale immediately. // Debounced so a burst of dir events (e.g. a team launch) coalesces into one - // rebuild; periodic reconciliation is the backup path if an event is missed. + // reconcile; periodic reconciliation is the backup path if an event is missed. if (this.shouldReconcile(eventType, relativePath)) { this.scheduleReconcile(); } @@ -241,20 +258,16 @@ export class TeamTaskWatchRegistry { this.options.onChange(eventType, relativePath); }; - nextWatcher.on('add', (changedPath) => handleEvent('add', changedPath)); - nextWatcher.on('change', (changedPath) => handleEvent('change', changedPath)); - nextWatcher.on('unlink', (changedPath) => handleEvent('unlink', changedPath)); - nextWatcher.on('addDir', (changedPath) => handleEvent('addDir', changedPath)); - nextWatcher.on('unlinkDir', (changedPath) => handleEvent('unlinkDir', changedPath)); - nextWatcher.on('error', (error) => { + watcher.on('add', (changedPath) => handleEvent('add', changedPath)); + watcher.on('change', (changedPath) => handleEvent('change', changedPath)); + watcher.on('unlink', (changedPath) => handleEvent('unlink', changedPath)); + watcher.on('addDir', (changedPath) => handleEvent('addDir', changedPath)); + watcher.on('unlinkDir', (changedPath) => handleEvent('unlinkDir', changedPath)); + watcher.on('error', (error) => { if (!this.closed && generation === this.generation) { this.options.onError(error); } }); - - if (shouldEmitExistingFiles) { - await this.emitExistingFilesForNewTargets(addedTargets, generation); - } } private async emitExistingFilesForNewTargets( diff --git a/test/main/services/infrastructure/FileWatcher.test.ts b/test/main/services/infrastructure/FileWatcher.test.ts index 15c5d621..fa5a86b7 100644 --- a/test/main/services/infrastructure/FileWatcher.test.ts +++ b/test/main/services/infrastructure/FileWatcher.test.ts @@ -13,6 +13,8 @@ type MockChokidarWatcher = { on: (event: string, handler: (...args: unknown[]) => void) => MockChokidarWatcher; close: ReturnType; emit: (event: string, ...args: unknown[]) => void; + add: (paths: string | string[]) => void; + unwatch: (paths: string | string[]) => void; }; const chokidarMock = vi.hoisted(() => { @@ -29,6 +31,15 @@ const chokidarMock = vi.hoisted(() => { handler(...args); } }, + add(paths: string | string[]) { + for (const p of (Array.isArray(paths) ? paths : [paths]).map((x) => String(x))) { + if (!watcher.targets.includes(p)) watcher.targets.push(p); + } + }, + unwatch(paths: string | string[]) { + const drop = new Set((Array.isArray(paths) ? paths : [paths]).map((x) => String(x))); + watcher.targets = watcher.targets.filter((t) => !drop.has(t)); + }, } as MockChokidarWatcher; watcher.on = vi.fn((event: string, handler: (...args: unknown[]) => void) => { @@ -1570,7 +1581,9 @@ describe('FileWatcher', () => { teamsWatcher.emit('addDir', addedTeamDir); await vi.waitFor(() => { - expect(chokidarMock.watch).toHaveBeenCalledTimes(3); + // Incremental: the existing watcher is updated via add(), not recreated, so + // watch() is still only called twice (teams + tasks). + expect(chokidarMock.watch).toHaveBeenCalledTimes(2); expect(getChokidarWatcherForRoot(teamsDir).targets).toContain(path.normalize(addedTeamDir)); expect(events).toEqual([{ type: 'config', teamName: 'base-2', detail: 'config.json' }]); }); @@ -1579,14 +1592,55 @@ describe('FileWatcher', () => { fs.rmSync(tempDir, { recursive: true, force: true }); }); - it('keeps rebuilding the registry when the previous chokidar close throws synchronously', async () => { - const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'filewatcher-chokidar-close-throw-')); + it('unwatches removed team dirs incrementally without recreating the watcher', async () => { + const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'filewatcher-chokidar-unwatch-')); + setClaudeBasePathOverride(tempDir); + const projectsDir = path.join(tempDir, 'projects'); + const todosDir = path.join(tempDir, 'todos'); + const teamsDir = path.join(tempDir, 'teams'); + const tasksDir = path.join(tempDir, 'tasks'); + const removedTeamDir = path.join(teamsDir, 'base-2'); + fs.mkdirSync(projectsDir, { recursive: true }); + fs.mkdirSync(todosDir, { recursive: true }); + fs.mkdirSync(path.join(teamsDir, 'base-1', 'inboxes'), { recursive: true }); + fs.mkdirSync(path.join(removedTeamDir, 'inboxes'), { recursive: true }); + fs.mkdirSync(path.join(tasksDir, 'base-1'), { recursive: true }); + useRealAccess(); + + const watchMock = vi.mocked(fs.watch); + watchMock.mockImplementation(() => createFakeWatcher()); + + const dataCache = new DataCache(50, 10, false); + const watcher = new FileWatcher(dataCache, projectsDir, todosDir); + watcher.start(); + + await vi.waitFor(() => expect(chokidarMock.watch).toHaveBeenCalledTimes(2)); + const teamsWatcher = getChokidarWatcherForRoot(teamsDir); + expect(teamsWatcher.targets).toContain(path.normalize(removedTeamDir)); + + fs.rmSync(removedTeamDir, { recursive: true, force: true }); + teamsWatcher.emit('unlinkDir', removedTeamDir); + + await vi.waitFor(() => { + expect(getChokidarWatcherForRoot(teamsDir).targets).not.toContain( + path.normalize(removedTeamDir) + ); + }); + // Same persistent watcher instance; no recreate. + expect(chokidarMock.watch).toHaveBeenCalledTimes(2); + expect(getChokidarWatcherForRoot(teamsDir)).toBe(teamsWatcher); + + watcher.stop(); + fs.rmSync(tempDir, { recursive: true, force: true }); + }); + + it('reuses the persistent chokidar watcher across reconciles and keeps handling its events', async () => { + const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'filewatcher-chokidar-persistent-')); setClaudeBasePathOverride(tempDir); const projectsDir = path.join(tempDir, 'projects'); const todosDir = path.join(tempDir, 'todos'); const teamsDir = path.join(tempDir, 'teams'); const tasksDir = path.join(tempDir, 'tasks'); - const addedTeamDir = path.join(teamsDir, 'base-2'); fs.mkdirSync(projectsDir, { recursive: true }); fs.mkdirSync(todosDir, { recursive: true }); fs.mkdirSync(path.join(teamsDir, 'base-1', 'inboxes'), { recursive: true }); @@ -1604,59 +1658,21 @@ describe('FileWatcher', () => { await vi.waitFor(() => expect(chokidarMock.watch).toHaveBeenCalledTimes(2)); const teamsWatcher = getChokidarWatcherForRoot(teamsDir); - teamsWatcher.close.mockImplementationOnce(() => { - throw new Error('close failed'); - }); - - fs.mkdirSync(addedTeamDir, { recursive: true }); - fs.writeFileSync(path.join(addedTeamDir, 'config.json'), '{}', 'utf8'); - teamsWatcher.emit('addDir', addedTeamDir); - - await vi.waitFor(() => { - expect(chokidarMock.watch).toHaveBeenCalledTimes(3); - expect(getChokidarWatcherForRoot(teamsDir).targets).toContain(path.normalize(addedTeamDir)); - expect(events).toEqual([{ type: 'config', teamName: 'base-2', detail: 'config.json' }]); - }); - - watcher.stop(); - fs.rmSync(tempDir, { recursive: true, force: true }); - }); - - it('ignores events from an old chokidar generation after registry rebuild', async () => { - const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'filewatcher-chokidar-generation-')); - setClaudeBasePathOverride(tempDir); - const projectsDir = path.join(tempDir, 'projects'); - const todosDir = path.join(tempDir, 'todos'); - const teamsDir = path.join(tempDir, 'teams'); - const tasksDir = path.join(tempDir, 'tasks'); - fs.mkdirSync(projectsDir, { recursive: true }); - fs.mkdirSync(todosDir, { recursive: true }); - fs.mkdirSync(path.join(teamsDir, 'base-1', 'inboxes'), { recursive: true }); - fs.mkdirSync(path.join(tasksDir, 'base-1'), { recursive: true }); - useRealAccess(); - - const watchMock = vi.mocked(fs.watch); - watchMock.mockImplementation(() => createFakeWatcher()); - - const dataCache = new DataCache(50, 10, false); - const watcher = new FileWatcher(dataCache, projectsDir, todosDir); - const events: unknown[] = []; - watcher.on('team-change', (event) => events.push(event)); - watcher.start(); - - await vi.waitFor(() => expect(chokidarMock.watch).toHaveBeenCalledTimes(2)); - const oldTeamsWatcher = getChokidarWatcherForRoot(teamsDir); fs.mkdirSync(path.join(teamsDir, 'base-2'), { recursive: true }); await vi.advanceTimersByTimeAsync(30_000); - await vi.waitFor(() => expect(chokidarMock.watch).toHaveBeenCalledTimes(3)); - const newTeamsWatcher = getChokidarWatcherForRoot(teamsDir); - expect(newTeamsWatcher).not.toBe(oldTeamsWatcher); + await vi.waitFor(() => + expect(getChokidarWatcherForRoot(teamsDir).targets).toContain( + path.normalize(path.join(teamsDir, 'base-2')) + ) + ); + // The watcher is reused (same instance, no extra watch() call), so there is no + // stale "old generation" to ignore. + expect(chokidarMock.watch).toHaveBeenCalledTimes(2); + expect(getChokidarWatcherForRoot(teamsDir)).toBe(teamsWatcher); - oldTeamsWatcher.emit('change', path.join(teamsDir, 'base-1', 'config.json')); - newTeamsWatcher.emit('change', path.join(teamsDir, 'base-1', 'config.json')); + teamsWatcher.emit('change', path.join(teamsDir, 'base-1', 'config.json')); await vi.advanceTimersByTimeAsync(100); - expect(events).toEqual([{ type: 'config', teamName: 'base-1', detail: 'config.json' }]); watcher.stop(); diff --git a/test/main/services/infrastructure/TeamTaskWatchRegistry.test.ts b/test/main/services/infrastructure/TeamTaskWatchRegistry.test.ts index 07afa05a..d92b6023 100644 --- a/test/main/services/infrastructure/TeamTaskWatchRegistry.test.ts +++ b/test/main/services/infrastructure/TeamTaskWatchRegistry.test.ts @@ -9,6 +9,8 @@ type MockChokidarWatcher = { handlers: Map void>>; on: (event: string, handler: (...args: unknown[]) => void) => MockChokidarWatcher; emit: (event: string, ...args: unknown[]) => void; + add: (paths: string | string[]) => void; + unwatch: (paths: string | string[]) => void; close: ReturnType; }; @@ -23,6 +25,15 @@ const chokidarMock = vi.hoisted(() => { emit(event: string, ...args: unknown[]) { for (const h of watcher.handlers.get(event) ?? []) h(...args); }, + add(paths: string | string[]) { + for (const p of (Array.isArray(paths) ? paths : [paths]).map((x) => String(x))) { + if (!watcher.targets.includes(p)) watcher.targets.push(p); + } + }, + unwatch(paths: string | string[]) { + const drop = new Set((Array.isArray(paths) ? paths : [paths]).map((x) => String(x))); + watcher.targets = watcher.targets.filter((t) => !drop.has(t)); + }, } as MockChokidarWatcher; watcher.on = vi.fn((event: string, handler: (...args: unknown[]) => void) => { const hs = watcher.handlers.get(event) ?? []; @@ -168,7 +179,7 @@ describe('TeamTaskWatchRegistry scoping', () => { expect(targets).toContain(path.normalize(path.join(root, 'beta'))); }); - it('coalesces a burst of addDir events into a single watcher rebuild', async () => { + it('coalesces a burst of addDir events into a single incremental watcher update', async () => { const registry = new TeamTaskWatchRegistry({ kind: 'teams', rootPath: root, @@ -190,8 +201,9 @@ describe('TeamTaskWatchRegistry scoping', () => { const finalTargets = latestTargets(); await registry.close(); - // Exactly one rebuild despite 4 addDir events, and it picked up the new dir. - expect(chokidarMock.instances.length).toBe(instancesAfterStart + 1); + // Coalesced into a single reconcile; the watcher is updated incrementally + // (no teardown/recreate, so no new chokidar instance) and now includes the dir. + expect(chokidarMock.instances.length).toBe(instancesAfterStart); expect(finalTargets).toContain(path.normalize(path.join(root, 'delta'))); }); });