diff --git a/src/utils/settings/changeDetector.test.ts b/src/utils/settings/changeDetector.test.ts new file mode 100644 index 0000000000..a49e2ca34d --- /dev/null +++ b/src/utils/settings/changeDetector.test.ts @@ -0,0 +1,298 @@ +import { afterEach, beforeEach, describe, expect, mock, test } from 'bun:test' +import { + acquireSharedMutationLock, + releaseSharedMutationLock, +} from '../../test/sharedMutationLock.js' +import type { SettingSource } from './constants.js' + +type SettingsChangeDetectorModule = typeof import('./changeDetector.js') & { + _handleChangeForTesting: (path: string) => void + _handleDeleteForTesting: (path: string) => void + _setDependenciesForTesting: (overrides?: Record) => void +} + +const pathsBySource: Record = { + userSettings: '/tmp/openclaude/user/settings.json', + projectSettings: '/tmp/openclaude/project/.claude/settings.json', + localSettings: '/tmp/openclaude/project/.claude/settings.local.json', + flagSettings: null, + policySettings: '/tmp/openclaude/managed/managed-settings.json', +} + +let resetSettingsCache = mock(() => {}) +let consumeInternalWrite = mock(() => false) +let hookResults: { blocked: boolean }[] = [] +let executeConfigChangeHooksImpl = async () => hookResults +let executeConfigChangeHooks = mock(async () => hookResults) +let activeDetector: SettingsChangeDetectorModule | null = null + +function installMocks(): void { + resetSettingsCache = mock(() => {}) + consumeInternalWrite = mock(() => false) + hookResults = [] + executeConfigChangeHooksImpl = async () => hookResults + executeConfigChangeHooks = mock(() => executeConfigChangeHooksImpl()) +} + +async function importFreshModule(): Promise { + activeDetector = (await import( + `./changeDetector.ts?test=${Date.now()}-${Math.random()}` + )) as SettingsChangeDetectorModule + activeDetector._setDependenciesForTesting({ + clearInternalWrites: mock(() => {}), + consumeInternalWrite, + executeConfigChangeHooks, + getManagedSettingsDropInDir: () => + '/tmp/openclaude/managed/managed-settings.d', + getSettingsFilePathForSource: (source: SettingSource) => + pathsBySource[source], + hasBlockingResult: (results: { blocked: boolean }[]) => + results.some(result => result.blocked), + resetSettingsCache, + }) + return activeDetector +} + +function sleep(ms: number): Promise { + return new Promise(resolve => setTimeout(resolve, ms)) +} + +beforeEach(async () => { + await acquireSharedMutationLock('utils/settings/changeDetector.test.ts') + installMocks() +}) + +afterEach(async () => { + try { + await activeDetector?.resetForTesting() + activeDetector?._setDependenciesForTesting() + activeDetector = null + mock.restore() + } finally { + releaseSharedMutationLock() + } +}) + +describe('settings change detector fanout batching', () => { + test('debounces rapid filesystem changes and emits each source once', async () => { + const detector = await importFreshModule() + await detector.resetForTesting({ settingsDebounce: 5 }) + + const emitted: SettingSource[] = [] + const unsubscribe = detector.subscribe(source => { + emitted.push(source) + }) + + detector._handleChangeForTesting(pathsBySource.userSettings!) + detector._handleChangeForTesting(pathsBySource.userSettings!) + detector._handleChangeForTesting(pathsBySource.projectSettings!) + + await sleep(25) + + expect(resetSettingsCache).toHaveBeenCalledTimes(1) + expect(emitted).toEqual(['userSettings', 'projectSettings']) + + unsubscribe() + await detector.resetForTesting() + }) + + test('debounces accepted deletion fanout after the deletion grace period', async () => { + const detector = await importFreshModule() + await detector.resetForTesting({ + deletionGrace: 1, + settingsDebounce: 5, + }) + + const emitted: SettingSource[] = [] + const unsubscribe = detector.subscribe(source => { + emitted.push(source) + }) + + detector._handleDeleteForTesting(pathsBySource.userSettings!) + detector._handleDeleteForTesting(pathsBySource.projectSettings!) + + await sleep(30) + + expect(resetSettingsCache).toHaveBeenCalledTimes(1) + expect(emitted).toEqual(['userSettings', 'projectSettings']) + + unsubscribe() + await detector.resetForTesting() + }) + + test('does not schedule fanout when a ConfigChange hook blocks a change', async () => { + const detector = await importFreshModule() + await detector.resetForTesting({ settingsDebounce: 5 }) + + const emitted: SettingSource[] = [] + const unsubscribe = detector.subscribe(source => { + emitted.push(source) + }) + + hookResults = [{ blocked: true }] + detector._handleChangeForTesting(pathsBySource.userSettings!) + + await sleep(25) + + expect(resetSettingsCache).not.toHaveBeenCalled() + expect(emitted).toEqual([]) + + unsubscribe() + await detector.resetForTesting() + }) + + test('suppresses a pending fanout when a newer change for the same source is blocked', async () => { + const detector = await importFreshModule() + await detector.resetForTesting({ settingsDebounce: 20 }) + + const emitted: SettingSource[] = [] + const unsubscribe = detector.subscribe(source => { + emitted.push(source) + }) + + hookResults = [] + detector._handleChangeForTesting(pathsBySource.userSettings!) + await sleep(0) + + hookResults = [{ blocked: true }] + detector._handleChangeForTesting(pathsBySource.userSettings!) + await sleep(40) + + expect(resetSettingsCache).not.toHaveBeenCalled() + expect(emitted).toEqual([]) + + unsubscribe() + await detector.resetForTesting() + }) + + test('suppresses a pending fanout when a same-source deletion is pending hooks', async () => { + const detector = await importFreshModule() + await detector.resetForTesting({ + deletionGrace: 30, + settingsDebounce: 5, + }) + + const emitted: SettingSource[] = [] + const unsubscribe = detector.subscribe(source => { + emitted.push(source) + }) + + hookResults = [] + detector._handleChangeForTesting(pathsBySource.userSettings!) + await sleep(0) + + hookResults = [{ blocked: true }] + detector._handleDeleteForTesting(pathsBySource.userSettings!) + await sleep(15) + + expect(resetSettingsCache).not.toHaveBeenCalled() + expect(emitted).toEqual([]) + + await sleep(40) + expect(resetSettingsCache).not.toHaveBeenCalled() + expect(emitted).toEqual([]) + + unsubscribe() + await detector.resetForTesting() + }) + + test('dispose prevents in-flight hook results from scheduling fanout', async () => { + const detector = await importFreshModule() + await detector.resetForTesting({ settingsDebounce: 5 }) + + let releaseHook: (() => void) | undefined + executeConfigChangeHooksImpl = async () => { + await new Promise(resolve => { + releaseHook = resolve + }) + return hookResults + } + + const emitted: SettingSource[] = [] + detector.subscribe(source => { + emitted.push(source) + }) + + detector._handleChangeForTesting(pathsBySource.userSettings!) + await sleep(0) + expect(executeConfigChangeHooks).toHaveBeenCalledTimes(1) + + await detector.dispose() + releaseHook?.() + await sleep(20) + + expect(resetSettingsCache).not.toHaveBeenCalled() + expect(emitted).toEqual([]) + }) + + test('dispose prevents later change and delete events from running hooks', async () => { + const detector = await importFreshModule() + await detector.resetForTesting({ + deletionGrace: 1, + settingsDebounce: 5, + }) + + const emitted: SettingSource[] = [] + detector.subscribe(source => { + emitted.push(source) + }) + + await detector.dispose() + detector._handleChangeForTesting(pathsBySource.userSettings!) + detector._handleDeleteForTesting(pathsBySource.projectSettings!) + await sleep(20) + + expect(executeConfigChangeHooks).not.toHaveBeenCalled() + expect(resetSettingsCache).not.toHaveBeenCalled() + expect(emitted).toEqual([]) + }) + + test('ignores stale same-source hook completions without dropping newer fanout', async () => { + const detector = await importFreshModule() + await detector.resetForTesting({ settingsDebounce: 20 }) + + let releaseFirstHook: (() => void) | undefined + let hookCalls = 0 + executeConfigChangeHooksImpl = async () => { + hookCalls += 1 + if (hookCalls === 1) { + await new Promise(resolve => { + releaseFirstHook = resolve + }) + } + return hookResults + } + + const emitted: SettingSource[] = [] + const unsubscribe = detector.subscribe(source => { + emitted.push(source) + }) + + detector._handleChangeForTesting(pathsBySource.userSettings!) + await sleep(0) + detector._handleChangeForTesting(pathsBySource.userSettings!) + await sleep(0) + + expect(executeConfigChangeHooks).toHaveBeenCalledTimes(2) + releaseFirstHook?.() + await sleep(40) + + expect(resetSettingsCache).toHaveBeenCalledTimes(1) + expect(emitted).toEqual(['userSettings']) + + unsubscribe() + await detector.resetForTesting() + }) + + test('resetForTesting clears pending settings fanout timers', async () => { + const detector = await importFreshModule() + await detector.resetForTesting({ settingsDebounce: 20 }) + + detector._handleChangeForTesting(pathsBySource.userSettings!) + await sleep(0) + await detector.resetForTesting({ settingsDebounce: 5 }) + await sleep(30) + + expect(resetSettingsCache).not.toHaveBeenCalled() + }) +}) diff --git a/src/utils/settings/changeDetector.ts b/src/utils/settings/changeDetector.ts index c83f083581..f34f6f2e1f 100644 --- a/src/utils/settings/changeDetector.ts +++ b/src/utils/settings/changeDetector.ts @@ -62,12 +62,21 @@ const MDM_POLL_INTERVAL_MS = 30 * 60 * 1000 // 30 minutes const DELETION_GRACE_MS = FILE_STABILITY_THRESHOLD_MS + FILE_STABILITY_POLL_INTERVAL_MS + 200 +/** + * Time in milliseconds to batch accepted filesystem settings changes before + * resetting caches and notifying listeners. + */ +const SETTINGS_DEBOUNCE_MS = 500 + let watcher: FSWatcher | null = null let mdmPollTimer: ReturnType | null = null let lastMdmSnapshot: string | null = null let initialized = false let disposed = false const pendingDeletions = new Map>() +let settingsDebounceTimer: ReturnType | null = null +const pendingSettingsSources = new Map() +const settingsSourceGenerations = new Map() const settingsChanged = createSignal<[source: SettingSource]>() // Test overrides for timing constants @@ -76,8 +85,23 @@ let testOverrides: { pollInterval?: number mdmPollInterval?: number deletionGrace?: number + settingsDebounce?: number } | null = null +const defaultDependencies = { + clearInternalWrites, + consumeInternalWrite, + executeConfigChangeHooks, + getManagedSettingsDropInDir, + getSettingsFilePathForSource, + hasBlockingResult, + resetSettingsCache, + stat, + watch: chokidar.watch.bind(chokidar), +} +type SettingsChangeDetectorDependencies = typeof defaultDependencies +let dependencies: SettingsChangeDetectorDependencies = defaultDependencies + /** * Initialize file watching */ @@ -100,7 +124,7 @@ export async function initialize(): Promise { `Watching for changes in setting files ${[...settingsFiles].join(', ')}...${dropInDir ? ` and drop-in directory ${dropInDir}` : ''}`, ) - watcher = chokidar.watch(dirs, { + watcher = dependencies.watch(dirs, { persistent: true, ignoreInitial: true, depth: 0, // Only watch immediate children, not subdirectories @@ -159,8 +183,10 @@ export function dispose(): Promise { } for (const timer of pendingDeletions.values()) clearTimeout(timer) pendingDeletions.clear() + clearSettingsDebounce() + settingsSourceGenerations.clear() lastMdmSnapshot = null - clearInternalWrites() + dependencies.clearInternalWrites() settingsChanged.clear() const w = watcher watcher = null @@ -194,7 +220,7 @@ async function getWatchTargets(): Promise<{ if (source === 'flagSettings') { continue } - const path = getSettingsFilePathForSource(source) + const path = dependencies.getSettingsFilePathForSource(source) if (!path) { continue } @@ -209,7 +235,7 @@ async function getWatchTargets(): Promise<{ // Check if file exists - only watch directories that have at least one existing file try { - const stats = await stat(path) + const stats = await dependencies.stat(path) if (stats.isFile()) { dirsWithExistingFiles.add(dir) } @@ -235,9 +261,9 @@ async function getWatchTargets(): Promise<{ // its immediate children (the .json files). Any .json file inside it maps // to the 'policySettings' source. let dropInDir: string | null = null - const managedDropIn = getManagedSettingsDropInDir() + const managedDropIn = dependencies.getManagedSettingsDropInDir() try { - const stats = await stat(managedDropIn) + const stats = await dependencies.stat(managedDropIn) if (stats.isDirectory()) { dirsWithExistingFiles.add(managedDropIn) dropInDir = managedDropIn @@ -266,6 +292,7 @@ function settingSourceToConfigChangeSource( } function handleChange(path: string): void { + if (disposed) return const source = getSourceForPath(path) if (!source) return @@ -281,7 +308,7 @@ function handleChange(path: string): void { } // Check if this was an internal write - if (consumeInternalWrite(path, INTERNAL_WRITE_WINDOW_MS)) { + if (dependencies.consumeInternalWrite(path, INTERNAL_WRITE_WINDOW_MS)) { return } @@ -289,15 +316,17 @@ function handleChange(path: string): void { // Fire ConfigChange hook first — if blocked (exit code 2 or decision: 'block'), // skip applying the change to the session - void executeConfigChangeHooks( + const generation = nextSettingsSourceGeneration(source) + void dependencies.executeConfigChangeHooks( settingSourceToConfigChangeSource(source), path, ).then(results => { - if (hasBlockingResult(results)) { + if (dependencies.hasBlockingResult(results)) { logForDebugging(`ConfigChange hook blocked change to ${path}`) return } - fanOut(source) + if (disposed) return + scheduleFanOut(source, generation) }) } @@ -306,6 +335,7 @@ function handleChange(path: string): void { * pending deletion grace timer and treats the event as a change. */ function handleAdd(path: string): void { + if (disposed) return const source = getSourceForPath(path) if (!source) return @@ -328,6 +358,7 @@ function handleAdd(path: string): void { * the deletion is cancelled and treated as a normal change instead. */ function handleDelete(path: string): void { + if (disposed) return const source = getSourceForPath(path) if (!source) return @@ -336,25 +367,29 @@ function handleDelete(path: string): void { // If there's already a pending deletion for this path, let it run if (pendingDeletions.has(path)) return + const generation = nextSettingsSourceGeneration(source) const timer = setTimeout( - (p, src) => { + (p, src, gen) => { + if (disposed) return pendingDeletions.delete(p) // Fire ConfigChange hook first — if blocked, skip applying the deletion - void executeConfigChangeHooks( + void dependencies.executeConfigChangeHooks( settingSourceToConfigChangeSource(src), p, ).then(results => { - if (hasBlockingResult(results)) { + if (dependencies.hasBlockingResult(results)) { logForDebugging(`ConfigChange hook blocked deletion of ${p}`) return } - fanOut(src) + if (disposed) return + scheduleFanOut(src, gen) }) }, testOverrides?.deletionGrace ?? DELETION_GRACE_MS, path, source, + generation, ) pendingDeletions.set(path, timer) } @@ -364,13 +399,14 @@ function getSourceForPath(path: string): SettingSource | undefined { const normalizedPath = platformPath.normalize(path) // Check if the path is inside the managed-settings.d/ drop-in directory - const dropInDir = getManagedSettingsDropInDir() + const dropInDir = dependencies.getManagedSettingsDropInDir() if (normalizedPath.startsWith(dropInDir + platformPath.sep)) { return 'policySettings' } return SETTING_SOURCES.find( - source => getSettingsFilePathForSource(source) === normalizedPath, + source => + dependencies.getSettingsFilePathForSource(source) === normalizedPath, ) } @@ -435,10 +471,54 @@ function startMdmPoll(): void { * repopulates; all subsequent listeners hit the cache. */ function fanOut(source: SettingSource): void { - resetSettingsCache() + dependencies.resetSettingsCache() settingsChanged.emit(source) } +function clearSettingsDebounce(): void { + if (settingsDebounceTimer) { + clearTimeout(settingsDebounceTimer) + settingsDebounceTimer = null + } + pendingSettingsSources.clear() +} + +function nextSettingsSourceGeneration(source: SettingSource): number { + const generation = (settingsSourceGenerations.get(source) ?? 0) + 1 + settingsSourceGenerations.set(source, generation) + return generation +} + +function scheduleFanOut(source: SettingSource, generation: number): void { + if (disposed) return + if (settingsSourceGenerations.get(source) !== generation) return + pendingSettingsSources.set(source, generation) + + if (settingsDebounceTimer) { + clearTimeout(settingsDebounceTimer) + } + + settingsDebounceTimer = setTimeout(() => { + settingsDebounceTimer = null + if (disposed) { + pendingSettingsSources.clear() + return + } + + const sources = [...pendingSettingsSources].flatMap(([src, generation]) => + settingsSourceGenerations.get(src) === generation ? [src] : [], + ) + pendingSettingsSources.clear() + + if (sources.length === 0) return + + dependencies.resetSettingsCache() + for (const src of sources) { + settingsChanged.emit(src) + } + }, testOverrides?.settingsDebounce ?? SETTINGS_DEBOUNCE_MS) +} + /** * Manually notify listeners of a settings change. * Used for programmatic settings changes (e.g., remote managed settings refresh) @@ -463,6 +543,7 @@ export function resetForTesting(overrides?: { pollInterval?: number mdmPollInterval?: number deletionGrace?: number + settingsDebounce?: number }): Promise { if (mdmPollTimer) { clearInterval(mdmPollTimer) @@ -470,6 +551,8 @@ export function resetForTesting(overrides?: { } for (const timer of pendingDeletions.values()) clearTimeout(timer) pendingDeletions.clear() + clearSettingsDebounce() + settingsSourceGenerations.clear() lastMdmSnapshot = null initialized = false disposed = false @@ -479,6 +562,15 @@ export function resetForTesting(overrides?: { return w ? w.close() : Promise.resolve() } +export const _handleChangeForTesting = handleChange +export const _handleDeleteForTesting = handleDelete + +export function _setDependenciesForTesting( + overrides: Partial = {}, +): void { + dependencies = { ...defaultDependencies, ...overrides } +} + export const settingsChangeDetector = { initialize, dispose, diff --git a/src/utils/skills/skillChangeDetector.test.ts b/src/utils/skills/skillChangeDetector.test.ts new file mode 100644 index 0000000000..e844471a7b --- /dev/null +++ b/src/utils/skills/skillChangeDetector.test.ts @@ -0,0 +1,318 @@ +import { afterEach, beforeEach, describe, expect, mock, test } from 'bun:test' +import { + acquireSharedMutationLock, + releaseSharedMutationLock, +} from '../../test/sharedMutationLock.js' + +type SkillChangeDetectorModule = typeof import('./skillChangeDetector.js') & { + _scheduleReloadForTesting: (changedPath: string) => void + _setDependenciesForTesting: (overrides?: Record) => void +} + +let clearCommandsCache = mock(() => {}) +let clearCommandMemoizationCaches = mock(() => {}) +let clearSkillCaches = mock(() => {}) +let resetSentSkillNames = mock(() => {}) +let hookResults: { blocked: boolean }[] = [] +let executeConfigChangeHooksImpl = async () => hookResults +let executeConfigChangeHooks = mock(async () => hookResults) +let dynamicSkillsLoadedCallback: (() => void) | undefined +let unregisterDynamicSkillsLoaded = mock(() => {}) +let getSkillsPathImpl = (_source: string, _dir: string) => '' +let statImpl = mock(async (_path: string) => {}) +let chokidarWatch = mock(() => ({ + on: mock(() => {}), + close: mock(async () => {}), +})) +let activeDetector: SkillChangeDetectorModule | null = null + +function installMocks(): void { + clearCommandsCache = mock(() => {}) + clearCommandMemoizationCaches = mock(() => {}) + clearSkillCaches = mock(() => {}) + resetSentSkillNames = mock(() => {}) + hookResults = [] + executeConfigChangeHooksImpl = async () => hookResults + executeConfigChangeHooks = mock(() => executeConfigChangeHooksImpl()) + dynamicSkillsLoadedCallback = undefined + unregisterDynamicSkillsLoaded = mock(() => {}) + getSkillsPathImpl = () => '' + statImpl = mock(async () => {}) + chokidarWatch = mock(() => ({ + on: mock(() => {}), + close: mock(async () => {}), + })) +} + +async function importFreshModule(): Promise { + activeDetector = (await import( + `./skillChangeDetector.ts?test=${Date.now()}-${Math.random()}` + )) as SkillChangeDetectorModule + activeDetector._setDependenciesForTesting({ + clearCommandMemoizationCaches, + clearCommandsCache, + executeConfigChangeHooks, + getFsImplementation: () => ({ + stat: statImpl, + }), + getSkillsPath: (source: string, dir: string) => + getSkillsPathImpl(source, dir), + hasBlockingResult: (results: { blocked: boolean }[]) => + results.some(result => result.blocked), + onDynamicSkillsLoaded: (callback: () => void) => { + dynamicSkillsLoadedCallback = callback + return unregisterDynamicSkillsLoaded + }, + resetSentSkillNames, + watch: chokidarWatch, + }) + return activeDetector +} + +function sleep(ms: number): Promise { + return new Promise(resolve => setTimeout(resolve, ms)) +} + +beforeEach(async () => { + await acquireSharedMutationLock('utils/skills/skillChangeDetector.test.ts') + installMocks() +}) + +afterEach(async () => { + try { + await activeDetector?.resetForTesting() + activeDetector?._setDependenciesForTesting() + activeDetector = null + mock.restore() + } finally { + releaseSharedMutationLock() + } +}) + +describe('skillChangeDetector reload batching', () => { + test('dispose unregisters and guards dynamic skill callbacks', async () => { + const detector = await importFreshModule() + + let notifications = 0 + detector.subscribe(() => { + notifications += 1 + }) + + await detector.initialize() + expect(dynamicSkillsLoadedCallback).toBeDefined() + + await detector.dispose() + dynamicSkillsLoadedCallback?.() + + expect(unregisterDynamicSkillsLoaded).toHaveBeenCalledTimes(1) + expect(clearCommandMemoizationCaches).not.toHaveBeenCalled() + expect(notifications).toBe(0) + }) + + test('dispose during initialize prevents watcher creation after path lookup resolves', async () => { + const detector = await importFreshModule() + getSkillsPathImpl = (source, dir) => + source === 'userSettings' && dir === 'skills' ? '/tmp/skills' : '' + + let resolveStat: (() => void) | undefined + statImpl = mock( + async () => + await new Promise(resolve => { + resolveStat = resolve + }), + ) + + const initializePromise = detector.initialize() + await sleep(0) + await detector.dispose() + resolveStat?.() + await initializePromise + + expect(chokidarWatch).not.toHaveBeenCalled() + }) + + test('batches rapid reload requests into one hook/cache clear/notification', async () => { + const detector = await importFreshModule() + await detector.resetForTesting({ reloadDebounce: 5, reloadCooldown: 20 }) + + let notifications = 0 + const unsubscribe = detector.subscribe(() => { + notifications += 1 + }) + + detector._scheduleReloadForTesting('/tmp/skills/a/SKILL.md') + detector._scheduleReloadForTesting('/tmp/skills/b/SKILL.md') + detector._scheduleReloadForTesting('/tmp/skills/c/SKILL.md') + + await sleep(30) + + expect(executeConfigChangeHooks).toHaveBeenCalledTimes(1) + expect(clearCommandsCache).toHaveBeenCalledTimes(1) + expect(clearSkillCaches).not.toHaveBeenCalled() + expect(resetSentSkillNames).toHaveBeenCalledTimes(1) + expect(notifications).toBe(1) + + unsubscribe() + await detector.resetForTesting() + }) + + test('delays a second burst until the reload cooldown has elapsed', async () => { + const detector = await importFreshModule() + await detector.resetForTesting({ reloadDebounce: 5, reloadCooldown: 60 }) + + let notifications = 0 + const unsubscribe = detector.subscribe(() => { + notifications += 1 + }) + + detector._scheduleReloadForTesting('/tmp/skills/first/SKILL.md') + await sleep(20) + expect(notifications).toBe(1) + + detector._scheduleReloadForTesting('/tmp/skills/second/SKILL.md') + await sleep(20) + expect(notifications).toBe(1) + + await sleep(60) + expect(notifications).toBe(2) + expect(executeConfigChangeHooks).toHaveBeenCalledTimes(2) + + unsubscribe() + await detector.resetForTesting() + }) + + test('does not start cooldown when a ConfigChange hook blocks reload', async () => { + const detector = await importFreshModule() + await detector.resetForTesting({ reloadDebounce: 5, reloadCooldown: 500 }) + + let notifications = 0 + const unsubscribe = detector.subscribe(() => { + notifications += 1 + }) + + hookResults = [{ blocked: true }] + detector._scheduleReloadForTesting('/tmp/skills/blocked/SKILL.md') + await sleep(20) + expect(notifications).toBe(0) + expect(clearCommandsCache).not.toHaveBeenCalled() + + hookResults = [] + detector._scheduleReloadForTesting('/tmp/skills/allowed/SKILL.md') + await sleep(20) + + expect(notifications).toBe(1) + expect(clearCommandsCache).toHaveBeenCalledTimes(1) + + unsubscribe() + await detector.resetForTesting() + }) + + test('queues events during an in-flight reload before cache clear and notification', async () => { + const detector = await importFreshModule() + await detector.resetForTesting({ reloadDebounce: 5, reloadCooldown: 60 }) + + let releaseFirstHook: (() => void) | undefined + let releaseSecondHook: (() => void) | undefined + let hookCalls = 0 + executeConfigChangeHooksImpl = async () => { + hookCalls += 1 + if (hookCalls === 1) { + await new Promise(resolve => { + releaseFirstHook = resolve + }) + } else if (hookCalls === 2) { + await new Promise(resolve => { + releaseSecondHook = resolve + }) + } + return hookResults + } + + let notifications = 0 + const unsubscribe = detector.subscribe(() => { + notifications += 1 + }) + + detector._scheduleReloadForTesting('/tmp/skills/first/SKILL.md') + await sleep(20) + expect(executeConfigChangeHooks).toHaveBeenCalledTimes(1) + + detector._scheduleReloadForTesting('/tmp/skills/second/SKILL.md') + await sleep(30) + expect(executeConfigChangeHooks).toHaveBeenCalledTimes(1) + + releaseFirstHook?.() + await sleep(20) + expect(notifications).toBe(0) + expect(clearCommandsCache).not.toHaveBeenCalled() + expect(executeConfigChangeHooks).toHaveBeenCalledTimes(2) + + releaseSecondHook?.() + await sleep(30) + expect(notifications).toBe(1) + expect(executeConfigChangeHooks).toHaveBeenCalledTimes(2) + + unsubscribe() + await detector.resetForTesting() + }) + + test('dispose prevents an in-flight reload from clearing caches after hook resolution', async () => { + const detector = await importFreshModule() + await detector.resetForTesting({ reloadDebounce: 5, reloadCooldown: 20 }) + + let releaseHook: (() => void) | undefined + executeConfigChangeHooksImpl = async () => { + await new Promise(resolve => { + releaseHook = resolve + }) + return hookResults + } + + let notifications = 0 + detector.subscribe(() => { + notifications += 1 + }) + + detector._scheduleReloadForTesting('/tmp/skills/in-flight/SKILL.md') + await sleep(20) + expect(executeConfigChangeHooks).toHaveBeenCalledTimes(1) + + await detector.dispose() + releaseHook?.() + await sleep(20) + + expect(clearCommandsCache).not.toHaveBeenCalled() + expect(resetSentSkillNames).not.toHaveBeenCalled() + expect(notifications).toBe(0) + }) + + test('dispose prevents later reload scheduling from running hooks', async () => { + const detector = await importFreshModule() + await detector.resetForTesting({ reloadDebounce: 5, reloadCooldown: 20 }) + + let notifications = 0 + detector.subscribe(() => { + notifications += 1 + }) + + await detector.dispose() + detector._scheduleReloadForTesting('/tmp/skills/after-dispose/SKILL.md') + await sleep(20) + + expect(executeConfigChangeHooks).not.toHaveBeenCalled() + expect(clearCommandsCache).not.toHaveBeenCalled() + expect(notifications).toBe(0) + }) + + test('resetForTesting clears pending reload timers', async () => { + const detector = await importFreshModule() + await detector.resetForTesting({ reloadDebounce: 20, reloadCooldown: 20 }) + + detector._scheduleReloadForTesting('/tmp/skills/pending/SKILL.md') + await detector.resetForTesting({ reloadDebounce: 5, reloadCooldown: 5 }) + await sleep(30) + + expect(executeConfigChangeHooks).not.toHaveBeenCalled() + expect(clearCommandsCache).not.toHaveBeenCalled() + }) +}) diff --git a/src/utils/skills/skillChangeDetector.ts b/src/utils/skills/skillChangeDetector.ts index f14f7c2eeb..bc33e1d65d 100644 --- a/src/utils/skills/skillChangeDetector.ts +++ b/src/utils/skills/skillChangeDetector.ts @@ -10,7 +10,6 @@ import { logEvent, } from '../../services/analytics/index.js' import { - clearSkillCaches, getSkillsPath, onDynamicSkillsLoaded, } from '../../skills/loadSkillsDir.js' @@ -34,12 +33,17 @@ const FILE_STABILITY_POLL_INTERVAL_MS = 500 /** * Time in milliseconds to debounce rapid skill change events into a single * reload. Prevents cascading reloads when many skill files change at once - * (e.g. during auto-update or when another session modifies skill directories). - * Without this, each file change triggers a full clearSkillCaches() + - * clearCommandsCache() + listener notification cycle, which can deadlock the - * event loop when dozens of events fire in rapid succession. + * (e.g. during auto-update, folder moves/renames, or when another session + * modifies skill directories). */ -const RELOAD_DEBOUNCE_MS = 300 +const RELOAD_DEBOUNCE_MS = 3000 + +/** + * Minimum spacing between completed skill reloads. Some filesystem operations + * emit multiple event waves; this prevents a second wave from causing an + * immediate back-to-back reload after the first batch finishes. + */ +const RELOAD_COOLDOWN_MS = 5000 /** * Polling interval for chokidar when usePolling is enabled. @@ -64,9 +68,12 @@ const USE_POLLING = typeof Bun !== 'undefined' let watcher: FSWatcher | null = null let reloadTimer: ReturnType | null = null const pendingChangedPaths = new Set() +let lastReloadTime = 0 +let reloadInProgress = false let initialized = false let disposed = false let dynamicSkillsCallbackRegistered = false +let unregisterDynamicSkillsCallback: (() => void) | null = null let unregisterCleanup: (() => void) | null = null const skillsChanged = createSignal() @@ -75,10 +82,25 @@ let testOverrides: { stabilityThreshold?: number pollInterval?: number reloadDebounce?: number + reloadCooldown?: number /** Chokidar fs.stat polling interval when USE_POLLING is active. */ chokidarInterval?: number } | null = null +const defaultDependencies = { + clearCommandMemoizationCaches, + clearCommandsCache, + executeConfigChangeHooks, + getFsImplementation, + getSkillsPath, + hasBlockingResult, + onDynamicSkillsLoaded, + resetSentSkillNames, + watch: chokidar.watch.bind(chokidar), +} +type SkillChangeDetectorDependencies = typeof defaultDependencies +let dependencies: SkillChangeDetectorDependencies = defaultDependencies + /** * Initialize file watching for skill directories */ @@ -86,28 +108,36 @@ export async function initialize(): Promise { if (initialized || disposed) return initialized = true + // Register cleanup before the first await so dispose() can win races during + // async path discovery. + unregisterCleanup = registerCleanup(async () => { + await dispose() + }) + // Register callback for when dynamic skills are loaded (only once) if (!dynamicSkillsCallbackRegistered) { dynamicSkillsCallbackRegistered = true - onDynamicSkillsLoaded(() => { + unregisterDynamicSkillsCallback = dependencies.onDynamicSkillsLoaded(() => { + if (disposed) return // Clear memoization caches so new skills are picked up // Note: we use clearCommandMemoizationCaches (not clearCommandsCache) // because clearCommandsCache would call clearSkillCaches which // wipes out the dynamic skills we just loaded - clearCommandMemoizationCaches() + dependencies.clearCommandMemoizationCaches() // Notify listeners that skills changed skillsChanged.emit() }) } const paths = await getWatchablePaths() + if (disposed) return if (paths.length === 0) return logForDebugging( `Watching for changes in skill/command directories: ${paths.join(', ')}...`, ) - watcher = chokidar.watch(paths, { + watcher = dependencies.watch(paths, { persistent: true, ignoreInitial: true, depth: 2, // Skills use skill-name/SKILL.md format @@ -133,11 +163,6 @@ export async function initialize(): Promise { watcher.on('add', handleChange) watcher.on('change', handleChange) watcher.on('unlink', handleChange) - - // Register cleanup to properly dispose of the file watcher during graceful shutdown - unregisterCleanup = registerCleanup(async () => { - await dispose() - }) } /** @@ -149,6 +174,11 @@ export function dispose(): Promise { unregisterCleanup() unregisterCleanup = null } + if (unregisterDynamicSkillsCallback) { + unregisterDynamicSkillsCallback() + unregisterDynamicSkillsCallback = null + dynamicSkillsCallbackRegistered = false + } let closePromise: Promise = Promise.resolve() if (watcher) { closePromise = watcher.close() @@ -159,6 +189,8 @@ export function dispose(): Promise { reloadTimer = null } pendingChangedPaths.clear() + lastReloadTime = 0 + reloadInProgress = false skillsChanged.clear() return closePromise } @@ -169,11 +201,11 @@ export function dispose(): Promise { export const subscribe = skillsChanged.subscribe async function getWatchablePaths(): Promise { - const fs = getFsImplementation() + const fs = dependencies.getFsImplementation() const paths: string[] = [] // User skills directory (~/.openclaude/skills) - const userSkillsPath = getSkillsPath('userSettings', 'skills') + const userSkillsPath = dependencies.getSkillsPath('userSettings', 'skills') if (userSkillsPath) { try { await fs.stat(userSkillsPath) @@ -184,7 +216,10 @@ async function getWatchablePaths(): Promise { } // User commands directory (~/.openclaude/commands) - const userCommandsPath = getSkillsPath('userSettings', 'commands') + const userCommandsPath = dependencies.getSkillsPath( + 'userSettings', + 'commands', + ) if (userCommandsPath) { try { await fs.stat(userCommandsPath) @@ -195,7 +230,10 @@ async function getWatchablePaths(): Promise { } // Project skills directory (.claude/skills) - const projectSkillsPath = getSkillsPath('projectSettings', 'skills') + const projectSkillsPath = dependencies.getSkillsPath( + 'projectSettings', + 'skills', + ) if (projectSkillsPath) { try { // For project settings, resolve to absolute path @@ -208,7 +246,10 @@ async function getWatchablePaths(): Promise { } // Project commands directory (.claude/commands) - const projectCommandsPath = getSkillsPath('projectSettings', 'commands') + const projectCommandsPath = dependencies.getSkillsPath( + 'projectSettings', + 'commands', + ) if (projectCommandsPath) { try { // For project settings, resolve to absolute path @@ -235,6 +276,7 @@ async function getWatchablePaths(): Promise { } function handleChange(path: string): void { + if (disposed) return logForDebugging(`Detected skill change: ${path}`) logEvent('tengu_skill_file_changed', { source: @@ -248,34 +290,65 @@ function handleChange(path: string): void { * Debounce rapid skill changes into a single reload. When many skill files * change at once (e.g. auto-update installs a new binary and a new session * touches skill directories), each file fires its own chokidar event. Without - * debouncing, each event triggers clearSkillCaches() + clearCommandsCache() + - * listener notification — 30 events means 30 full reload cycles, which can - * deadlock the Bun event loop via rapid FSWatcher watch/unwatch churn. + * debouncing, each event triggers clearCommandsCache() + listener notification + * — 30 events means 30 full reload cycles, which can deadlock the Bun event + * loop via rapid FSWatcher watch/unwatch churn. */ function scheduleReload(changedPath: string): void { + if (disposed) return pendingChangedPaths.add(changedPath) + if (reloadInProgress) return + scheduleReloadTimer() +} + +function scheduleReloadTimer(): void { if (reloadTimer) clearTimeout(reloadTimer) + const debounceMs = testOverrides?.reloadDebounce ?? RELOAD_DEBOUNCE_MS + const reloadCooldownMs = testOverrides?.reloadCooldown ?? RELOAD_COOLDOWN_MS + const cooldownRemaining = lastReloadTime + reloadCooldownMs - Date.now() + const delay = Math.max(debounceMs, cooldownRemaining) reloadTimer = setTimeout(async () => { reloadTimer = null + if (disposed) return const paths = [...pendingChangedPaths] pendingChangedPaths.clear() - // Fire ConfigChange hook once for the batch — the hook query is always - // 'skills' so firing per-path (which can be hundreds during a git - // operation) just spams the hook matcher with identical queries. Pass the - // first path as a representative; hooks can inspect all paths via the - // skills directory if they need the full set. - const results = await executeConfigChangeHooks('skills', paths[0]!) - if (hasBlockingResult(results)) { - logForDebugging( - `ConfigChange hook blocked skill reload (${paths.length} paths)`, + if (paths.length === 0) return + + reloadInProgress = true + try { + // Fire ConfigChange hook once for the batch — the hook query is always + // 'skills' so firing per-path (which can be hundreds during a git + // operation) just spams the hook matcher with identical queries. Pass the + // first path as a representative; hooks can inspect all paths via the + // skills directory if they need the full set. + const results = await dependencies.executeConfigChangeHooks( + 'skills', + paths[0]!, ) - return + if (dependencies.hasBlockingResult(results)) { + logForDebugging( + `ConfigChange hook blocked skill reload (${paths.length} paths)`, + ) + return + } + if (disposed) return + if (pendingChangedPaths.size > 0) { + logForDebugging( + `Deferring skill reload because ${pendingChangedPaths.size} newer paths arrived during hooks`, + ) + return + } + dependencies.clearCommandsCache() + dependencies.resetSentSkillNames() + lastReloadTime = Date.now() + skillsChanged.emit() + } finally { + reloadInProgress = false + if (!disposed && pendingChangedPaths.size > 0) { + scheduleReloadTimer() + } } - clearSkillCaches() - clearCommandsCache() - resetSentSkillNames() - skillsChanged.emit() - }, testOverrides?.reloadDebounce ?? RELOAD_DEBOUNCE_MS) + }, delay) } /** @@ -285,6 +358,7 @@ export async function resetForTesting(overrides?: { stabilityThreshold?: number pollInterval?: number reloadDebounce?: number + reloadCooldown?: number chokidarInterval?: number }): Promise { // Clean up existing watcher if present to avoid resource leaks @@ -292,17 +366,36 @@ export async function resetForTesting(overrides?: { await watcher.close() watcher = null } + if (unregisterCleanup) { + unregisterCleanup() + unregisterCleanup = null + } + if (unregisterDynamicSkillsCallback) { + unregisterDynamicSkillsCallback() + unregisterDynamicSkillsCallback = null + dynamicSkillsCallbackRegistered = false + } if (reloadTimer) { clearTimeout(reloadTimer) reloadTimer = null } pendingChangedPaths.clear() + lastReloadTime = 0 + reloadInProgress = false skillsChanged.clear() initialized = false disposed = false testOverrides = overrides ?? null } +export const _scheduleReloadForTesting = scheduleReload + +export function _setDependenciesForTesting( + overrides: Partial = {}, +): void { + dependencies = { ...defaultDependencies, ...overrides } +} + export const skillChangeDetector = { initialize, dispose,