mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-07-09 17:19:02 +00:00
perf: memoize skill scans, debounce sleep-inhibitor log, guard IDE readdir (#6155)
* 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>
This commit is contained in:
parent
1b58ede8e7
commit
fc701f8608
8 changed files with 282 additions and 3 deletions
|
|
@ -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<string[]>
|
||||
>
|
||||
).mockRejectedValue(enoent);
|
||||
mockDebugLogger.debug.mockClear();
|
||||
|
||||
const ideClient = await IdeClient.getInstance();
|
||||
const result = await (
|
||||
ideClient as unknown as {
|
||||
getAllConnectionConfigs: (dir: string) => Promise<unknown[]>;
|
||||
}
|
||||
).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<string[]>
|
||||
>
|
||||
).mockRejectedValue(eperm);
|
||||
mockDebugLogger.debug.mockClear();
|
||||
|
||||
const ideClient = await IdeClient.getInstance();
|
||||
const result = await (
|
||||
ideClient as unknown as {
|
||||
getAllConnectionConfigs: (dir: string) => Promise<unknown[]>;
|
||||
}
|
||||
).getAllConnectionConfigs('/some/test/dir');
|
||||
|
||||
expect(result).toEqual([]);
|
||||
expect(mockDebugLogger.debug).toHaveBeenCalledWith(
|
||||
'Failed to read IDE connection directory:',
|
||||
expect.any(Error),
|
||||
);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe('getIdeServerHost', () => {
|
||||
|
|
|
|||
|
|
@ -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 [];
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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<SleepInhibitorConfig['spawn']>;
|
||||
|
|
@ -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();
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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<string>()),
|
||||
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<typeof vi.fn>)
|
||||
.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);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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<CollectedAvailableSkills>;
|
||||
ts: number;
|
||||
}
|
||||
|
||||
let collectCache = new WeakMap<SkillManager, CachedCollect>();
|
||||
|
||||
/** 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<CollectedAvailableSkills> {
|
||||
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<CollectedAvailableSkills> {
|
||||
// Include a skill only when (a) it is not hidden from the model
|
||||
// (`disable-model-invocation`), (b) it is not user-disabled via
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -36,6 +36,7 @@ import {
|
|||
buildSkillLlmContent,
|
||||
applySkillAllowedTools,
|
||||
collectAvailableSkillEntries,
|
||||
clearCollectedSkillEntriesCache,
|
||||
} from './skill-utils.js';
|
||||
|
||||
/**
|
||||
|
|
@ -170,6 +171,10 @@ export class SkillTool extends BaseDeclarativeTool<SkillParams, ToolResult> {
|
|||
*/
|
||||
async refreshSkills(): Promise<void> {
|
||||
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,
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue