From 8b3cec8013e1c676b5fec0428ab9940d5f50a496 Mon Sep 17 00:00:00 2001 From: 777genius Date: Sat, 30 May 2026 12:05:23 +0300 Subject: [PATCH] perf: update team watcher incrementally instead of full rebuild A team launch repeatedly changes the watched target set (new dirs appear), and each change tore down the chokidar watcher and recreated it over the full target set. On macOS chokidar uses kqueue with one fd per watched file, so every rebuild re-opened an fd for EVERY watched file (the large always-watched inbox set plus scoped dirs). Profiling a 6-member mixed launch showed ~54k open() syscalls dominated by these rebuilds. Keep one persistent watcher and apply target-set changes with add()/unwatch() on the delta only, so a reconcile opens fds for just the newly added dirs. The initial watcher still uses ignoreInitial for a silent startup baseline, and emitExistingFilesForNewTargets still backfills files already present in newly added dirs, so the emitted event surface is unchanged. Because the watcher is no longer recreated per reconcile, the stale-old-generation and close-throws-during-rebuild failure modes are gone; their tests are replaced with incremental add/unwatch and persistent-watcher coverage. All 69 watcher tests pass. --- .../infrastructure/TeamTaskWatchRegistry.ts | 81 +++++++----- .../infrastructure/FileWatcher.test.ts | 120 ++++++++++-------- .../TeamTaskWatchRegistry.test.ts | 18 ++- 3 files changed, 130 insertions(+), 89 deletions(-) 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'))); }); });