From fc701f8608b7570c353fbb1036add6b1940229e6 Mon Sep 17 00:00:00 2001 From: Kagura Date: Mon, 6 Jul 2026 00:02:28 +0800 Subject: [PATCH] perf: memoize skill scans, debounce sleep-inhibitor log, guard IDE readdir (#6155) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * perf: memoize skill scans, debounce sleep-inhibitor log, guard IDE readdir (#6134) Three startup / session performance-noise fixes: 1. **Memoize collectAvailableSkillEntries()**: Add a short-lived (2 s) WeakMap cache keyed by SkillManager instance. Near-simultaneous callers during startup (SkillTool, drainSkillAndCommandReminders, buildAvailableSkillsReminder, coreToolScheduler) now share a single skill scan instead of each re-invoking listSkills(). The cache is explicitly cleared on refreshSkills() so skill-set mutations are picked up immediately. 2. **SleepInhibitor one-time latch**: Add exitedWhileActiveLogged flag so the 'exited while active' debug message fires at most once per run. On Windows the PowerShell inhibitor subprocess may be killed between release()/acquire() cycles, previously logging on every tool call. The flag resets when activeCount drops to 0 or on dispose(). 3. **IDE client ENOENT guard**: In getAllConnectionConfigs(), catch ENOENT from readdir on ~/.qwen/ide silently (return []) instead of logging a debug message on every startup in CLI-only setups without an IDE companion. * test: add coverage for memo cache, ENOENT guard, and dedup latch * fix: correct Config import path in skill-utils test --------- Co-authored-by: 易良 <1204183885@qq.com> --- packages/core/src/ide/ide-client.test.ts | 50 +++++++++++ packages/core/src/ide/ide-client.ts | 8 ++ .../core/src/services/sleepInhibitor.test.ts | 54 +++++++++++ packages/core/src/services/sleepInhibitor.ts | 10 ++- packages/core/src/tools/skill-utils.test.ts | 90 ++++++++++++++++++- packages/core/src/tools/skill-utils.ts | 63 +++++++++++++ packages/core/src/tools/skill.test.ts | 5 ++ packages/core/src/tools/skill.ts | 5 ++ 8 files changed, 282 insertions(+), 3 deletions(-) diff --git a/packages/core/src/ide/ide-client.test.ts b/packages/core/src/ide/ide-client.test.ts index 2e21b1560f..cace1722ca 100644 --- a/packages/core/src/ide/ide-client.test.ts +++ b/packages/core/src/ide/ide-client.test.ts @@ -1440,6 +1440,56 @@ describe('IdeClient', () => { ); }); }); + + describe('getAllConnectionConfigs ENOENT guard', () => { + it('returns empty array silently when readdir rejects with ENOENT', async () => { + const enoent = new Error('ENOENT: no such file or directory'); + (enoent as NodeJS.ErrnoException).code = 'ENOENT'; + ( + vi.mocked(fs.promises.readdir) as Mock< + (path: fs.PathLike) => Promise + > + ).mockRejectedValue(enoent); + mockDebugLogger.debug.mockClear(); + + const ideClient = await IdeClient.getInstance(); + const result = await ( + ideClient as unknown as { + getAllConnectionConfigs: (dir: string) => Promise; + } + ).getAllConnectionConfigs('/some/test/dir'); + + expect(result).toEqual([]); + expect(mockDebugLogger.debug).not.toHaveBeenCalledWith( + 'Failed to read IDE connection directory:', + expect.any(Error), + ); + }); + + it('returns empty array and logs debug when readdir rejects with other error', async () => { + const eperm = new Error('EPERM: operation not permitted'); + (eperm as NodeJS.ErrnoException).code = 'EPERM'; + ( + vi.mocked(fs.promises.readdir) as Mock< + (path: fs.PathLike) => Promise + > + ).mockRejectedValue(eperm); + mockDebugLogger.debug.mockClear(); + + const ideClient = await IdeClient.getInstance(); + const result = await ( + ideClient as unknown as { + getAllConnectionConfigs: (dir: string) => Promise; + } + ).getAllConnectionConfigs('/some/test/dir'); + + expect(result).toEqual([]); + expect(mockDebugLogger.debug).toHaveBeenCalledWith( + 'Failed to read IDE connection directory:', + expect.any(Error), + ); + }); + }); }); describe('getIdeServerHost', () => { diff --git a/packages/core/src/ide/ide-client.ts b/packages/core/src/ide/ide-client.ts index 2f311a0299..c7b4efa1f3 100644 --- a/packages/core/src/ide/ide-client.ts +++ b/packages/core/src/ide/ide-client.ts @@ -743,6 +743,14 @@ export class IdeClient { .map((file) => file.toString()) .filter((file) => fileRegex.test(file)); } catch (e) { + // Silently return empty when the directory simply doesn't exist + // (common in CLI-only setups without an IDE companion extension). + if ( + e instanceof Error && + (e as NodeJS.ErrnoException).code === 'ENOENT' + ) { + return []; + } debugLogger.debug('Failed to read IDE connection directory:', e); return []; } diff --git a/packages/core/src/services/sleepInhibitor.test.ts b/packages/core/src/services/sleepInhibitor.test.ts index 02b2ab3faf..d86ac91219 100644 --- a/packages/core/src/services/sleepInhibitor.test.ts +++ b/packages/core/src/services/sleepInhibitor.test.ts @@ -484,4 +484,58 @@ describe('SleepInhibitor', () => { handle.release(); }); + + describe('exitedWhileActiveLogged dedup latch', () => { + it('logs exit-while-active exactly once per active run', async () => { + const { children, inhibitor, logger } = createHarness('linux'); + + const first = inhibitor.acquire('work'); + await vi.waitFor(() => expect(children).toHaveLength(1)); + children[0]!.emit('exit', 1, null); + + expect(logger.debug).toHaveBeenCalledWith( + 'Sleep inhibitor exited while active: code=1 signal=null', + ); + + const second = inhibitor.acquire('more work'); + await vi.waitFor(() => expect(children).toHaveLength(2)); + children[1]!.emit('exit', 1, null); + + // The latch prevents logging the exit message a second time. + expect( + logger.debug.mock.calls.filter((call) => + String(call[0]).includes('Sleep inhibitor exited while active:'), + ), + ).toHaveLength(1); + + first.release(); + second.release(); + }); + + it('resets the latch after all handles are released', async () => { + const { children, inhibitor, logger } = createHarness('linux'); + + const first = inhibitor.acquire('work'); + await vi.waitFor(() => expect(children).toHaveLength(1)); + children[0]!.emit('exit', 1, null); + + expect(logger.debug).toHaveBeenCalledWith( + 'Sleep inhibitor exited while active: code=1 signal=null', + ); + + // Release all handles → activeCount drops to 0 → latch resets. + first.release(); + logger.debug.mockClear(); + + const second = inhibitor.acquire('new work'); + await vi.waitFor(() => expect(children).toHaveLength(2)); + children[1]!.emit('exit', 2, null); + + expect(logger.debug).toHaveBeenCalledWith( + 'Sleep inhibitor exited while active: code=2 signal=null', + ); + + second.release(); + }); + }); }); diff --git a/packages/core/src/services/sleepInhibitor.ts b/packages/core/src/services/sleepInhibitor.ts index 3972cdeaf9..4774fe7f07 100644 --- a/packages/core/src/services/sleepInhibitor.ts +++ b/packages/core/src/services/sleepInhibitor.ts @@ -64,6 +64,7 @@ export class SleepInhibitor { private activeCount = 0; private child: ChildProcess | undefined; private spawnFailedForCurrentRun = false; + private exitedWhileActiveLogged = false; private readonly platform: NodeJS.Platform; private readonly env: NodeJS.ProcessEnv; private readonly spawn: NonNullable; @@ -119,6 +120,7 @@ export class SleepInhibitor { if (this.activeCount === 0) { this.stop(); this.spawnFailedForCurrentRun = false; + this.exitedWhileActiveLogged = false; } } @@ -209,7 +211,12 @@ export class SleepInhibitor { if (this.child === child) { this.child = undefined; } - if (this.activeCount > 0 && !this.spawnFailedForCurrentRun) { + if ( + this.activeCount > 0 && + !this.spawnFailedForCurrentRun && + !this.exitedWhileActiveLogged + ) { + this.exitedWhileActiveLogged = true; this.logger.debug( `Sleep inhibitor exited while active: code=${String(code)} signal=${String(signal)}`, ); @@ -233,6 +240,7 @@ export class SleepInhibitor { dispose(): void { this.activeCount = 0; this.spawnFailedForCurrentRun = false; + this.exitedWhileActiveLogged = false; this.stop(); } diff --git a/packages/core/src/tools/skill-utils.test.ts b/packages/core/src/tools/skill-utils.test.ts index 5dcccb3b82..82d29cfc72 100644 --- a/packages/core/src/tools/skill-utils.test.ts +++ b/packages/core/src/tools/skill-utils.test.ts @@ -4,9 +4,15 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect, vi } from 'vitest'; -import { applySkillAllowedTools } from './skill-utils.js'; +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { + applySkillAllowedTools, + collectAvailableSkillEntries, + clearCollectedSkillEntriesCache, +} from './skill-utils.js'; import type { PermissionManager } from '../permissions/permission-manager.js'; +import type { SkillManager } from '../skills/skill-manager.js'; +import type { Config } from '../config/config.js'; function mockPermissionManager(): { pm: PermissionManager; @@ -61,3 +67,83 @@ describe('applySkillAllowedTools', () => { expect(addSessionAllowRule).toHaveBeenNthCalledWith(2, 'Read'); }); }); + +describe('collectAvailableSkillEntries memoize cache', () => { + function mockSkillManager(): SkillManager { + return { + listSkills: vi.fn().mockResolvedValue([]), + isSkillActive: vi.fn().mockReturnValue(false), + } as unknown as SkillManager; + } + + function mockConfig(): Config { + return { + getDisabledSkillNames: vi.fn().mockReturnValue(new Set()), + getModelInvocableCommandsProvider: vi.fn().mockReturnValue(null), + } as unknown as Config; + } + + afterEach(() => { + clearCollectedSkillEntriesCache(); + vi.useRealTimers(); + }); + + it('returns the same promise on cache hit within TTL', async () => { + vi.useFakeTimers(); + const sm = mockSkillManager(); + const cfg = mockConfig(); + + const r1 = collectAvailableSkillEntries(sm, cfg); + const r2 = collectAvailableSkillEntries(sm, cfg); + + // The underlying scan should run only once. + expect(sm.listSkills).toHaveBeenCalledTimes(1); + // Both calls resolve to the exact same result object. + const [v1, v2] = await Promise.all([r1, r2]); + expect(v1).toBe(v2); + }); + + it('rescans after TTL expires', async () => { + vi.useFakeTimers(); + const sm = mockSkillManager(); + const cfg = mockConfig(); + + await collectAvailableSkillEntries(sm, cfg); + vi.advanceTimersByTime(2001); + await collectAvailableSkillEntries(sm, cfg); + + expect(sm.listSkills).toHaveBeenCalledTimes(2); + }); + + it('evicts cache entry on rejection so next caller retries', async () => { + vi.useFakeTimers(); + const sm = mockSkillManager(); + const cfg = mockConfig(); + + (sm.listSkills as ReturnType) + .mockRejectedValueOnce(new Error('boom')) + .mockResolvedValueOnce([]); + + const p1 = collectAvailableSkillEntries(sm, cfg); + await expect(p1).rejects.toThrow('boom'); + + // Flush microtask queue so the .catch() eviction handler runs. + await vi.runAllTimersAsync(); + + const p2 = collectAvailableSkillEntries(sm, cfg); + await expect(p2).resolves.toBeDefined(); + expect(sm.listSkills).toHaveBeenCalledTimes(2); + }); + + it('clearCollectedSkillEntriesCache evicts the entry', async () => { + vi.useFakeTimers(); + const sm = mockSkillManager(); + const cfg = mockConfig(); + + await collectAvailableSkillEntries(sm, cfg); + clearCollectedSkillEntriesCache(sm); + await collectAvailableSkillEntries(sm, cfg); + + expect(sm.listSkills).toHaveBeenCalledTimes(2); + }); +}); diff --git a/packages/core/src/tools/skill-utils.ts b/packages/core/src/tools/skill-utils.ts index 5bffbf6d92..df3caee31d 100644 --- a/packages/core/src/tools/skill-utils.ts +++ b/packages/core/src/tools/skill-utils.ts @@ -54,6 +54,39 @@ export interface CollectedAvailableSkills { entries: AvailableSkillEntry[]; } +/** + * Short-lived memo cache for `collectAvailableSkillEntries`. Keyed by + * `SkillManager` instance so independent managers (e.g. in tests) don't + * share results. Each entry stores the in-flight or resolved promise and a + * monotonic timestamp; entries older than `COLLECT_CACHE_TTL_MS` are + * discarded on the next call. + */ +interface CachedCollect { + promise: Promise; + ts: number; +} + +let collectCache = new WeakMap(); + +/** Cache lifetime in milliseconds. */ +const COLLECT_CACHE_TTL_MS = 2_000; + +/** + * Evict any cached result for the given manager, or reset the entire cache + * when called without an argument. Exported for tests and explicit + * invalidation hooks. + */ +export function clearCollectedSkillEntriesCache( + skillManager?: SkillManager, +): void { + if (skillManager) { + collectCache.delete(skillManager); + } else { + // Replace the WeakMap entirely to clear all entries. + collectCache = new WeakMap(); + } +} + /** * Collects the model-facing skill set — active file-based skills + model-invocable * commands — applying the same filtering/dedup rules `SkillTool.refreshSkills` @@ -61,10 +94,40 @@ export interface CollectedAvailableSkills { * returned validation fields and the `entries` list are always consistent, so * the Skill tool, the startup snapshot, and activation reminders share identical * bytes from one source. + * + * Results are memoized for up to 2 s per `SkillManager` instance so that + * near-simultaneous startup callers (SkillTool, drainSkillAndCommandReminders, + * buildAvailableSkillsReminder, coreToolScheduler) share a single scan. */ export async function collectAvailableSkillEntries( skillManager: SkillManager, config: Config, +): Promise { + const cached = collectCache.get(skillManager); + if (cached && Date.now() - cached.ts < COLLECT_CACHE_TTL_MS) { + return cached.promise; + } + + const promise = collectAvailableSkillEntriesUncached(skillManager, config); + collectCache.set(skillManager, { promise, ts: Date.now() }); + + // If the underlying scan fails, evict the cache so the next caller retries + // instead of getting a cached rejection. + promise.catch(() => { + const entry = collectCache.get(skillManager); + if (entry?.promise === promise) { + collectCache.delete(skillManager); + } + }); + + return promise; +} + +/** Uncached implementation — see `collectAvailableSkillEntries` for the + * memoized public API. */ +async function collectAvailableSkillEntriesUncached( + skillManager: SkillManager, + config: Config, ): Promise { // Include a skill only when (a) it is not hidden from the model // (`disable-model-invocation`), (b) it is not user-disabled via diff --git a/packages/core/src/tools/skill.test.ts b/packages/core/src/tools/skill.test.ts index 29bfef7975..be49a9331b 100644 --- a/packages/core/src/tools/skill.test.ts +++ b/packages/core/src/tools/skill.test.ts @@ -16,6 +16,7 @@ import type { ToolResult } from './tools.js'; import { partToString } from '../utils/partUtils.js'; import { collectAvailableSkillEntries, + clearCollectedSkillEntriesCache, renderAvailableSkillsBlock, } from './skill-utils.js'; @@ -82,6 +83,9 @@ describe('SkillTool', () => { mockAddSessionAllowRule = vi.fn(); vi.mocked(recordSkillInvocation).mockClear(); + // Clear skill-entries cache so fake timers don't cause stale hits. + clearCollectedSkillEntriesCache(); + // Create mock config config = { getProjectRoot: vi.fn().mockReturnValue('/test/project'), @@ -138,6 +142,7 @@ describe('SkillTool', () => { afterEach(() => { vi.useRealTimers(); vi.clearAllMocks(); + clearCollectedSkillEntriesCache(mockSkillManager); }); // The skill listing moved out of the tool description into a system-reminder diff --git a/packages/core/src/tools/skill.ts b/packages/core/src/tools/skill.ts index 09ccd170be..6bb1eff689 100644 --- a/packages/core/src/tools/skill.ts +++ b/packages/core/src/tools/skill.ts @@ -36,6 +36,7 @@ import { buildSkillLlmContent, applySkillAllowedTools, collectAvailableSkillEntries, + clearCollectedSkillEntriesCache, } from './skill-utils.js'; /** @@ -170,6 +171,10 @@ export class SkillTool extends BaseDeclarativeTool { */ async refreshSkills(): Promise { try { + // Invalidate the memoization cache so this refresh picks up any + // skill-set mutations (file edits, conditional activations, config + // toggles) that occurred since the last collection. + clearCollectedSkillEntriesCache(this.skillManager); const collected = await collectAvailableSkillEntries( this.skillManager, this.config,