From f4baa267f9915d288defdaf0e99e75e12601af96 Mon Sep 17 00:00:00 2001 From: doudouOUC Date: Sat, 4 Jul 2026 16:03:36 +0800 Subject: [PATCH 1/3] perf(cli): cache LoadedSettings per workspace with stat-based invalidation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ACP child under `qwen serve` is long-lived and re-runs a full loadSettings() on the shared event loop for every session/new, session/load and session/resume: four settings files read, parsed, migration-checked and structuredClone'd, the .env tree walked, home .env re-read, ${VAR} references re-resolved, and all scopes merged. Same-cwd repeat sessions (the typical serve workload) pay full price every time. Add a process-level cache keyed by resolved workspace dir (LRU 64). Freshness is checked deterministically on every access via a fingerprint of every filesystem input: stat signatures (mtimeMs:size:ino) of the four settings files, the re-discovered .env file list with signatures, IDE trust, realpath(cwd) and realpath(homedir). Any change -> full reload; fingerprint errors fail open to a reload; loadSettings() throws propagate uncached. Only the three hot ACP session handlers switch to loadSettingsCached(); all other loadSettings() callers (ext-methods write paths etc.) keep their direct read semantics. Known accepted differences (documented in the module doc): direct process.env mutation without any file change does not re-bake ${VAR} references on a hit; a .env edit racing the miss-path load itself is the usual mtime-cache TOCTOU microsecond window; an in-place overwrite preserving mtime+size+ino is invisible (self-writes go through temp+rename, which changes the inode). Part of the qwen serve multi-session performance work (#6263). 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) --- .../cli/src/acp-integration/acpAgent.test.ts | 8 + packages/cli/src/acp-integration/acpAgent.ts | 7 +- .../acp-integration/acpAgent.worktree.test.ts | 8 + packages/cli/src/config/environment.ts | 6 +- .../cli/src/config/settings-cache.test.ts | 272 ++++++++++++++++++ packages/cli/src/config/settings-cache.ts | 216 ++++++++++++++ 6 files changed, 513 insertions(+), 4 deletions(-) create mode 100644 packages/cli/src/config/settings-cache.test.ts create mode 100644 packages/cli/src/config/settings-cache.ts diff --git a/packages/cli/src/acp-integration/acpAgent.test.ts b/packages/cli/src/acp-integration/acpAgent.test.ts index efc35e7e0d0..4d0a35f40d4 100644 --- a/packages/cli/src/acp-integration/acpAgent.test.ts +++ b/packages/cli/src/acp-integration/acpAgent.test.ts @@ -499,6 +499,14 @@ vi.mock('../config/settings.js', () => ({ loadSettings: vi.fn(), reloadEnvironment: vi.fn(() => ({ updatedKeys: [], removedKeys: [] })), })); +// Passthrough: the real cache would serve the first mockReturnValue to every +// later same-cwd call, breaking tests that re-point loadSettings per call. +vi.mock('../config/settings-cache.js', async () => { + const settings = await import('../config/settings.js'); + return { + loadSettingsCached: (cwd: string) => settings.loadSettings(cwd), + }; +}); vi.mock('../config/loadedSettingsAdapter.js', () => ({ createLoadedSettingsAdapter: vi.fn((settings: unknown) => { (settings as Record)['getValue'] = vi.fn(); diff --git a/packages/cli/src/acp-integration/acpAgent.ts b/packages/cli/src/acp-integration/acpAgent.ts index 415b16a694f..9ce96e70465 100644 --- a/packages/cli/src/acp-integration/acpAgent.ts +++ b/packages/cli/src/acp-integration/acpAgent.ts @@ -143,6 +143,7 @@ import { reloadEnvironment, SettingScope, } from '../config/settings.js'; +import { loadSettingsCached } from '../config/settings-cache.js'; import { buildPermissionSettings, normalizePermissionRules, @@ -2876,7 +2877,7 @@ class QwenAgent implements Agent { // creation from picking up whichever workspace loaded last — Session // persists model changes through this instance, so a mix-up writes to // another workspace's settings.json. - const settings = loadSettings(cwd); + const settings = loadSettingsCached(cwd); this.settings = settings; const config = await this.newSessionConfig(cwd, mcpServers, settings); await this.ensureAuthenticated(config); @@ -2899,7 +2900,7 @@ class QwenAgent implements Agent { // Load per-request settings BEFORE the existence check: the check must // resolve `advanced.runtimeOutputDir` from THIS request's cwd, not from // whichever settings a concurrent handler loaded last. - const settings = loadSettings(params.cwd); + const settings = loadSettingsCached(params.cwd); const exists = await runWithAcpRuntimeOutputDir( settings, params.cwd, @@ -2954,7 +2955,7 @@ class QwenAgent implements Agent { params: ResumeSessionRequest, ): Promise { // Same per-request settings discipline as `loadSession`. - const settings = loadSettings(params.cwd); + const settings = loadSettingsCached(params.cwd); const exists = await runWithAcpRuntimeOutputDir( settings, params.cwd, diff --git a/packages/cli/src/acp-integration/acpAgent.worktree.test.ts b/packages/cli/src/acp-integration/acpAgent.worktree.test.ts index 39fcc5bcb5e..2329ebf8258 100644 --- a/packages/cli/src/acp-integration/acpAgent.worktree.test.ts +++ b/packages/cli/src/acp-integration/acpAgent.worktree.test.ts @@ -233,6 +233,14 @@ vi.mock('../config/settings.js', () => ({ SettingScope: {}, loadSettings: vi.fn(), })); +// Passthrough: the real cache would serve the first mockReturnValue to every +// later same-cwd call, breaking tests that re-point loadSettings per call. +vi.mock('../config/settings-cache.js', async () => { + const settings = await import('../config/settings.js'); + return { + loadSettingsCached: (cwd: string) => settings.loadSettings(cwd), + }; +}); vi.mock('../config/config.js', () => ({ loadCliConfig: vi.fn(), buildDisabledSkillNamesProvider: vi.fn(() => () => new Set()), diff --git a/packages/cli/src/config/environment.ts b/packages/cli/src/config/environment.ts index 7887bb1745c..e8ed3bae6d4 100644 --- a/packages/cli/src/config/environment.ts +++ b/packages/cli/src/config/environment.ts @@ -193,8 +193,12 @@ export function getHomeEnvFallbackVars( * - ~/.qwen/.env * - ~/.env * - /.env (when set) + * + * Exported so `settings-cache.ts` can re-run the exact same discovery when + * validating its fingerprint; keep the discovery semantics in this single + * implementation. */ -function findEnvFiles( +export function findEnvFiles( settings: Settings, startDir: string, userLevelPaths: Set = getUserLevelEnvPaths(), diff --git a/packages/cli/src/config/settings-cache.test.ts b/packages/cli/src/config/settings-cache.test.ts new file mode 100644 index 00000000000..e31853b33de --- /dev/null +++ b/packages/cli/src/config/settings-cache.test.ts @@ -0,0 +1,272 @@ +/** + * @license + * Copyright 2025 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// Mock os.homedir before anything else imports it. The holder is mutable so +// individual tests can relocate "home" (the homeDir fingerprint component). +const mockHome = vi.hoisted(() => ({ dir: '/uninitialized-mock-home' })); + +vi.mock('os', async (importOriginal) => { + const actualOs = await importOriginal(); + return { ...actualOs, homedir: vi.fn(() => mockHome.dir) }; +}); +vi.mock('node:os', async (importOriginal) => { + const actualOs = await importOriginal(); + return { ...actualOs, homedir: vi.fn(() => mockHome.dir) }; +}); + +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { + clearSettingsCacheForTesting, + loadSettingsCached, +} from './settings-cache.js'; +import { + resetEnvironmentTrackingForTesting, + resetHomeEnvBootstrapForTesting, + SettingScope, + SETTINGS_VERSION, + SETTINGS_VERSION_KEY, +} from './settings.js'; +import { resetTrustedFoldersForTesting } from './trustedFolders.js'; + +// Keys written to fixture .env files leak into process.env via +// loadEnvironment (by design); use a unique prefix and clean them up. +const TEST_ENV_PREFIX = 'SETTINGS_CACHE_TEST_'; + +describe('loadSettingsCached', () => { + let tmpRoot: string; + let homeDir: string; + let qwenHome: string; + let workspaceDir: string; + + const userSettingsPath = () => path.join(qwenHome, 'settings.json'); + const workspaceSettingsPath = (ws = workspaceDir) => + path.join(ws, '.qwen', 'settings.json'); + + const writeJson = (filePath: string, value: unknown) => { + fs.mkdirSync(path.dirname(filePath), { recursive: true }); + fs.writeFileSync(filePath, JSON.stringify(value)); + }; + + // All fixtures carry the current settings version so loadSettings never + // rewrites them (a migration self-write would turn the second call into a + // legitimate miss and hide what these tests assert). + const versioned = (settings: Record) => ({ + [SETTINGS_VERSION_KEY]: SETTINGS_VERSION, + ...settings, + }); + + const resetModuleState = () => { + clearSettingsCacheForTesting(); + resetHomeEnvBootstrapForTesting(); + resetEnvironmentTrackingForTesting(); + resetTrustedFoldersForTesting(); + }; + + beforeEach(() => { + tmpRoot = fs.realpathSync( + fs.mkdtempSync(path.join(os.tmpdir(), 'settings-cache-test-')), + ); + homeDir = path.join(tmpRoot, 'home'); + qwenHome = path.join(tmpRoot, 'qwen-home'); + workspaceDir = path.join(tmpRoot, 'project', 'app'); + fs.mkdirSync(homeDir, { recursive: true }); + fs.mkdirSync(qwenHome, { recursive: true }); + fs.mkdirSync(workspaceDir, { recursive: true }); + mockHome.dir = homeDir; + + // Sandbox every settings path inside tmpRoot: user/workspace via + // QWEN_HOME + cwd, system/system-defaults via their test overrides. + vi.stubEnv('QWEN_HOME', qwenHome); + vi.stubEnv( + 'QWEN_CODE_SYSTEM_SETTINGS_PATH', + path.join(tmpRoot, 'system', 'settings.json'), + ); + vi.stubEnv( + 'QWEN_CODE_SYSTEM_DEFAULTS_PATH', + path.join(tmpRoot, 'system', 'system-defaults.json'), + ); + resetModuleState(); + }); + + afterEach(() => { + vi.unstubAllEnvs(); + for (const key of Object.keys(process.env)) { + if (key.startsWith(TEST_ENV_PREFIX)) { + delete process.env[key]; + } + } + fs.rmSync(tmpRoot, { recursive: true, force: true }); + }); + + it('serves the same instance while nothing changed', () => { + writeJson(userSettingsPath(), versioned({ model: { name: 'cached' } })); + + const first = loadSettingsCached(workspaceDir); + const second = loadSettingsCached(workspaceDir); + + // Identity is the strongest possible assertion: loadSettings() always + // constructs a new LoadedSettings, so same instance ⇒ no reload ran. + expect(second).toBe(first); + expect(second.merged.model?.name).toBe('cached'); + }); + + it('reloads when the user settings file changes', () => { + writeJson(userSettingsPath(), versioned({ model: { name: 'before' } })); + const first = loadSettingsCached(workspaceDir); + + writeJson( + userSettingsPath(), + versioned({ model: { name: 'after-edit-longer' } }), + ); + const second = loadSettingsCached(workspaceDir); + + expect(second).not.toBe(first); + expect(second.merged.model?.name).toBe('after-edit-longer'); + }); + + it('reloads when a workspace settings file appears and again when it is deleted', () => { + const first = loadSettingsCached(workspaceDir); + + writeJson( + workspaceSettingsPath(), + versioned({ model: { name: 'from-workspace' } }), + ); + const second = loadSettingsCached(workspaceDir); + expect(second).not.toBe(first); + expect(second.merged.model?.name).toBe('from-workspace'); + + fs.rmSync(workspaceSettingsPath()); + const third = loadSettingsCached(workspaceDir); + expect(third).not.toBe(second); + expect(third.merged.model?.name).toBeUndefined(); + }); + + it('reloads when a .env file appears closer to the workspace', () => { + const first = loadSettingsCached(workspaceDir); + expect(process.env[`${TEST_ENV_PREFIX}CLOSER`]).toBeUndefined(); + + fs.writeFileSync( + path.join(workspaceDir, '.env'), + `${TEST_ENV_PREFIX}CLOSER=yes\n`, + ); + const second = loadSettingsCached(workspaceDir); + + expect(second).not.toBe(first); + expect(process.env[`${TEST_ENV_PREFIX}CLOSER`]).toBe('yes'); + }); + + it('reloads when a discovered home .env file changes', () => { + const homeEnvPath = path.join(qwenHome, '.env'); + fs.writeFileSync(homeEnvPath, `${TEST_ENV_PREFIX}A=1\n`); + + const first = loadSettingsCached(workspaceDir); + expect(process.env[`${TEST_ENV_PREFIX}A`]).toBe('1'); + expect(loadSettingsCached(workspaceDir)).toBe(first); + + fs.writeFileSync( + homeEnvPath, + `${TEST_ENV_PREFIX}A=1\n${TEST_ENV_PREFIX}B=2\n`, + ); + const second = loadSettingsCached(workspaceDir); + + expect(second).not.toBe(first); + expect(process.env[`${TEST_ENV_PREFIX}B`]).toBe('2'); + }); + + it('reloads when QWEN_HOME points at a different directory', () => { + writeJson(userSettingsPath(), versioned({ model: { name: 'home-one' } })); + const first = loadSettingsCached(workspaceDir); + expect(first.merged.model?.name).toBe('home-one'); + + const otherQwenHome = path.join(tmpRoot, 'qwen-home-2'); + writeJson( + path.join(otherQwenHome, 'settings.json'), + versioned({ model: { name: 'home-two' } }), + ); + vi.stubEnv('QWEN_HOME', otherQwenHome); + + const second = loadSettingsCached(workspaceDir); + expect(second).not.toBe(first); + expect(second.merged.model?.name).toBe('home-two'); + }); + + it('reloads when os.homedir() changes even if no settings path moves', () => { + // The R2 corner: QWEN_HOME pins the user/system paths and no .env exists + // anywhere, so only the homeDir fingerprint component can catch this. + const first = loadSettingsCached(workspaceDir); + + const otherHome = path.join(tmpRoot, 'home-2'); + fs.mkdirSync(otherHome, { recursive: true }); + mockHome.dir = otherHome; + + const second = loadSettingsCached(workspaceDir); + expect(second).not.toBe(first); + }); + + it('returns a fresh instance after setValue persisted a change', () => { + writeJson(userSettingsPath(), versioned({ model: { name: 'initial' } })); + const first = loadSettingsCached(workspaceDir); + + first.setValue(SettingScope.User, 'model.name', 'persisted-by-setvalue'); + + const second = loadSettingsCached(workspaceDir); + expect(second).not.toBe(first); + expect(second.merged.model?.name).toBe('persisted-by-setvalue'); + }); + + it('does not cache a load failure and recovers once the file is fixed', () => { + // Valid JSON that is not an object bypasses corruption recovery and + // makes loadSettings throw FatalConfigError. + fs.writeFileSync(userSettingsPath(), '[1]'); + + expect(() => loadSettingsCached(workspaceDir)).toThrow( + /not a valid JSON object/, + ); + + writeJson(userSettingsPath(), versioned({ model: { name: 'fixed' } })); + const recovered = loadSettingsCached(workspaceDir); + expect(recovered.merged.model?.name).toBe('fixed'); + expect(loadSettingsCached(workspaceDir)).toBe(recovered); + }); + + it('keeps independent entries per workspace directory', () => { + const otherWorkspace = path.join(tmpRoot, 'project', 'other'); + fs.mkdirSync(otherWorkspace, { recursive: true }); + writeJson( + workspaceSettingsPath(), + versioned({ model: { name: 'ws-app' } }), + ); + writeJson( + workspaceSettingsPath(otherWorkspace), + versioned({ model: { name: 'ws-other' } }), + ); + + const first = loadSettingsCached(workspaceDir); + const other = loadSettingsCached(otherWorkspace); + + expect(other).not.toBe(first); + expect(first.merged.model?.name).toBe('ws-app'); + expect(other.merged.model?.name).toBe('ws-other'); + expect(loadSettingsCached(workspaceDir)).toBe(first); + expect(loadSettingsCached(otherWorkspace)).toBe(other); + }); + + it('evicts the least recently used entry beyond the cache limit', () => { + const first = loadSettingsCached(workspaceDir); + + for (let i = 0; i < 64; i++) { + const ws = path.join(tmpRoot, 'fleet', `ws-${i}`); + fs.mkdirSync(ws, { recursive: true }); + loadSettingsCached(ws); + } + + // 64 newer entries pushed the first workspace out of the LRU map. + expect(loadSettingsCached(workspaceDir)).not.toBe(first); + }); +}); diff --git a/packages/cli/src/config/settings-cache.ts b/packages/cli/src/config/settings-cache.ts new file mode 100644 index 00000000000..6a6c3a04230 --- /dev/null +++ b/packages/cli/src/config/settings-cache.ts @@ -0,0 +1,216 @@ +/** + * @license + * Copyright 2025 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { ideContextStore, Storage } from '@qwen-code/qwen-code-core'; +import { findEnvFiles, preResolveHomeEnvOverrides } from './environment.js'; +import { + getSystemDefaultsPath, + getSystemSettingsPath, + getUserSettingsPath, + loadSettings, +} from './settings.js'; +import type { LoadedSettings } from './settings.js'; + +/** + * Process-level cache for `loadSettings()`, keyed by workspace directory. + * + * The ACP child under `qwen serve` is long-lived and serves many sessions + * (often for the same cwd). A full `loadSettings()` runs on the shared event + * loop for every `session/new` / `session/load`: four settings files read, + * parsed, migration-checked and structuredClone'd, the `.env` tree walked, + * home `.env` re-read, `${VAR}` references re-resolved, and all scopes merged. + * This wrapper serves a previously loaded `LoadedSettings` instance while a + * stat-based fingerprint of every filesystem input is unchanged. + * + * A cache hit still costs ~4 `statSync` + the `.env` discovery walk + * (`existsSync` per directory level) + 1-2 `realpathSync` — deliberately so: + * invalidation is deterministic (change a file, next call sees it) rather + * than time-based. It just skips the much larger read/parse/clone/merge work. + * + * Known, accepted differences vs. calling `loadSettings()` every time: + * 1. Mutating `process.env` directly (no file change) does not re-resolve + * `${VAR}` references baked into cached settings values. + * 2. A `.env` file modified *while* the miss-path `loadSettings()` is running + * (and never again afterwards) can be served stale — the microsecond + * TOCTOU window shared by every mtime-based cache. + * 3. An in-place overwrite preserving mtime, size *and* inode is invisible. + * Self-writes (`LoadedSettings.setValue`) go through temp-file + rename, + * which changes the inode; on filesystems reporting `ino` as 0 (some + * Windows setups) this degrades to mtime+size detection, same as tsc's + * incremental cache. + */ + +const MAX_CACHE_ENTRIES = 64; + +interface FileSig { + filePath: string; + sig: string; +} + +interface CacheFingerprint { + settingsFiles: FileSig[]; + envFiles: FileSig[]; + ideTrust: boolean | undefined; + realCwd: string; + homeDir: string; +} + +interface CacheEntry { + settings: LoadedSettings; + fingerprint: CacheFingerprint; +} + +const cache = new Map(); + +function statSig(filePath: string): string { + const st = fs.statSync(filePath, { throwIfNoEntry: false }); + return st ? `${st.mtimeMs}:${st.size}:${st.ino}` : 'missing'; +} + +function realpathOr(p: string): string { + try { + return fs.realpathSync(p); + } catch { + // Match loadSettings(): fall back to the resolved path. + return path.resolve(p); + } +} + +/** + * Paths are recomputed on every call (not stored once): they depend on + * QWEN_HOME / HOME and the test-only system-path overrides, so a changed + * environment shows up as a path mismatch and invalidates the entry. + */ +function settingsFileSigs(workspaceDir: string): FileSig[] { + const filePaths = [ + getSystemSettingsPath(), + getSystemDefaultsPath(), + getUserSettingsPath(), + new Storage(workspaceDir).getWorkspaceSettingsPath(), + ]; + return filePaths.map((filePath) => ({ filePath, sig: statSig(filePath) })); +} + +/** + * Re-runs `.env` discovery and signs the result. Discovery only returns + * files that exist, so a new `.env` appearing closer to the workspace (or a + * discovered one disappearing) changes the path list itself, while edits to + * a discovered file change its signature. Trust changes that would alter + * discovery are covered by the settingsFiles / ideTrust components. + */ +function envFileSigs( + settings: LoadedSettings, + workspaceDir: string, +): FileSig[] { + return findEnvFiles(settings.merged, workspaceDir).map((filePath) => ({ + filePath, + sig: statSig(filePath), + })); +} + +function sameFileSigs(a: FileSig[], b: FileSig[]): boolean { + if (a.length !== b.length) { + return false; + } + return a.every( + (sig, i) => sig.filePath === b[i]!.filePath && sig.sig === b[i]!.sig, + ); +} + +function isEntryFresh(key: string, entry: CacheEntry): boolean { + const fp = entry.fingerprint; + return ( + ideContextStore.get()?.workspaceState?.isTrusted === fp.ideTrust && + realpathOr(key) === fp.realCwd && + // homeDir guards the corner where nothing else moves: QWEN_HOME set (so + // settings paths don't follow HOME), no .env files anywhere, and the new + // home equals the workspace dir — which flips workspaceSettingsActive. + realpathOr(os.homedir()) === fp.homeDir && + sameFileSigs(settingsFileSigs(key), fp.settingsFiles) && + sameFileSigs(envFileSigs(entry.settings, key), fp.envFiles) + ); +} + +/** + * Drop-in replacement for `loadSettings(workspaceDir)` on hot paths that use + * its default options. Callers needing `LoadSettingsOptions` must keep using + * `loadSettings()` directly. + */ +export function loadSettingsCached(workspaceDir: string): LoadedSettings { + // Idempotent process-level latch (loadSettings runs it internally too); + // running it up front makes the QWEN_HOME-derived paths below stable from + // the very first call instead of only after the first miss. + preResolveHomeEnvOverrides(); + const key = path.resolve(workspaceDir); + + const entry = cache.get(key); + if (entry) { + let isFresh = false; + try { + isFresh = isEntryFresh(key, entry); + } catch { + // Fail open: any unexpected fingerprint error (EACCES, EIO, ...) is + // treated as a miss so the cache never becomes a new failure source. + } + if (isFresh) { + // Refresh LRU position (Map preserves insertion order). + cache.delete(key); + cache.set(key, entry); + return entry.settings; + } + cache.delete(key); + } + + // Sign the settings files BEFORE loading: if one changes while + // loadSettings() runs, the stored signature is already stale and the next + // call reloads. Conservative — may reload once too often, never serves + // stale data. + let preLoadSigs: FileSig[] | undefined; + try { + preLoadSigs = settingsFileSigs(key); + } catch { + // Fail open: cache nothing this round. + } + + // Load errors (FatalConfigError etc.) propagate unchanged and uncached; + // the stale entry was already dropped above. + const settings = loadSettings(key); + + if (preLoadSigs) { + try { + cache.set(key, { + settings, + fingerprint: { + settingsFiles: preLoadSigs, + // .env discovery needs the merged settings (trust), so it can only + // be signed after the load — see difference (2) in the module doc. + envFiles: envFileSigs(settings, key), + ideTrust: ideContextStore.get()?.workspaceState?.isTrusted, + realCwd: realpathOr(key), + homeDir: realpathOr(os.homedir()), + }, + }); + if (cache.size > MAX_CACHE_ENTRIES) { + const oldest = cache.keys().next().value; + if (oldest !== undefined) { + cache.delete(oldest); + } + } + } catch { + // Fail open: serve the freshly loaded settings uncached. + cache.delete(key); + } + } + + return settings; +} + +export function clearSettingsCacheForTesting(): void { + cache.clear(); +} From 9983b7f63fe4f86f08f052cccf82848a6e42fbe0 Mon Sep 17 00:00:00 2001 From: doudouOUC Date: Sat, 4 Jul 2026 21:55:58 +0800 Subject: [PATCH 2/3] test(cli): add IDE trust flip invalidation test for settings cache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Covers the ideTrust fingerprint component, which is the only trust input that can change within a live process (trustedFolders.json is a permanent singleton, folder-trust toggles live in the settings files). Addresses a Copilot review suggestion to guard against stale-cache trust regressions. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) --- packages/cli/src/config/settings-cache.test.ts | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/packages/cli/src/config/settings-cache.test.ts b/packages/cli/src/config/settings-cache.test.ts index e31853b33de..289a6d5eed1 100644 --- a/packages/cli/src/config/settings-cache.test.ts +++ b/packages/cli/src/config/settings-cache.test.ts @@ -20,6 +20,7 @@ vi.mock('node:os', async (importOriginal) => { import * as fs from 'node:fs'; import * as os from 'node:os'; import * as path from 'node:path'; +import { ideContextStore } from '@qwen-code/qwen-code-core'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { clearSettingsCacheForTesting, @@ -66,6 +67,9 @@ describe('loadSettingsCached', () => { resetHomeEnvBootstrapForTesting(); resetEnvironmentTrackingForTesting(); resetTrustedFoldersForTesting(); + // IDE trust feeds the ideTrust fingerprint component; clear it so state + // never leaks between tests. + ideContextStore.clear(); }; beforeEach(() => { @@ -209,6 +213,19 @@ describe('loadSettingsCached', () => { expect(second).not.toBe(first); }); + it('reloads when IDE trust state flips', () => { + // The ideTrust fingerprint component guards against a stale trust/merge + // result: IDE trust is the one trust input that can change within a live + // process (trustedFolders.json is a permanent singleton, folder-trust + // toggles live in the settings files themselves). + const first = loadSettingsCached(workspaceDir); + + ideContextStore.set({ workspaceState: { isTrusted: true } }); + + const second = loadSettingsCached(workspaceDir); + expect(second).not.toBe(first); + }); + it('returns a fresh instance after setValue persisted a change', () => { writeJson(userSettingsPath(), versioned({ model: { name: 'initial' } })); const first = loadSettingsCached(workspaceDir); From c3186e120512ec6fe52b7b4f683c5a5785f6981c Mon Sep 17 00:00:00 2001 From: doudouOUC Date: Sat, 4 Jul 2026 22:17:51 +0800 Subject: [PATCH 3/3] refactor(cli): harden settings cache observability and fail-open coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses three review suggestions: - Warn comment on settingsFileSigs that the 4-scope path list must stay in sync with loadSettings() (unlike envFileSigs, it is enumerated separately). - Add a createDebugLogger('SETTINGS_CACHE'), matching the SETTINGS / SETTINGS_WATCHER / CONFIG convention in neighbouring config modules, and log hit/miss, each fail-open catch (with the swallowed error), and eviction. - Add a fault-injection test asserting the cache reloads (never throws) when the fingerprint check fails, then recovers once the fault clears. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) --- .../cli/src/config/settings-cache.test.ts | 26 +++++++++++++++ packages/cli/src/config/settings-cache.ts | 33 ++++++++++++++++--- 2 files changed, 55 insertions(+), 4 deletions(-) diff --git a/packages/cli/src/config/settings-cache.test.ts b/packages/cli/src/config/settings-cache.test.ts index 289a6d5eed1..79edbb6f823 100644 --- a/packages/cli/src/config/settings-cache.test.ts +++ b/packages/cli/src/config/settings-cache.test.ts @@ -237,6 +237,32 @@ describe('loadSettingsCached', () => { expect(second.merged.model?.name).toBe('persisted-by-setvalue'); }); + it('falls open to a reload when fingerprint validation throws', () => { + // Fail-open is a core invariant: an unexpected error while checking the + // fingerprint must degrade to a full reload, never surface. isEntryFresh + // reads ideContextStore first, so making that throw exercises the + // hit-path fail-open (and the caching fail-open, since the rebuilt + // fingerprint reads it too). loadSettings itself does not read it here + // (the workspace dir is not process.cwd()), so the reload still succeeds. + writeJson(userSettingsPath(), versioned({ model: { name: 'ok' } })); + const first = loadSettingsCached(workspaceDir); + + const trustSpy = vi.spyOn(ideContextStore, 'get').mockImplementation(() => { + throw new Error('boom'); + }); + const second = loadSettingsCached(workspaceDir); + expect(second).not.toBe(first); // reloaded, not a propagated error + expect(second.merged.model?.name).toBe('ok'); // ...and still correct + + // Recovery: the degraded call could not cache (rebuilding the fingerprint + // also threw), so this first post-recovery call is itself a miss, then + // subsequent calls hit normally. + trustSpy.mockRestore(); + const third = loadSettingsCached(workspaceDir); + expect(third.merged.model?.name).toBe('ok'); + expect(loadSettingsCached(workspaceDir)).toBe(third); + }); + it('does not cache a load failure and recovers once the file is fixed', () => { // Valid JSON that is not an object bypasses corruption recovery and // makes loadSettings throw FatalConfigError. diff --git a/packages/cli/src/config/settings-cache.ts b/packages/cli/src/config/settings-cache.ts index 6a6c3a04230..1c59faf7088 100644 --- a/packages/cli/src/config/settings-cache.ts +++ b/packages/cli/src/config/settings-cache.ts @@ -7,7 +7,11 @@ import * as fs from 'node:fs'; import * as os from 'node:os'; import * as path from 'node:path'; -import { ideContextStore, Storage } from '@qwen-code/qwen-code-core'; +import { + createDebugLogger, + ideContextStore, + Storage, +} from '@qwen-code/qwen-code-core'; import { findEnvFiles, preResolveHomeEnvOverrides } from './environment.js'; import { getSystemDefaultsPath, @@ -48,6 +52,11 @@ import type { LoadedSettings } from './settings.js'; const MAX_CACHE_ENTRIES = 64; +// Matches the SETTINGS / SETTINGS_WATCHER / CONFIG loggers used by the +// neighbouring config modules; enable with the usual DEBUG namespaces to +// trace hit/miss, fail-open degradation and eviction in production. +const debugLogger = createDebugLogger('SETTINGS_CACHE'); + interface FileSig { filePath: string; sig: string; @@ -86,6 +95,13 @@ function realpathOr(p: string): string { * Paths are recomputed on every call (not stored once): they depend on * QWEN_HOME / HOME and the test-only system-path overrides, so a changed * environment shows up as a path mismatch and invalidates the entry. + * + * IMPORTANT: this list MUST stay in sync with the settings files that + * `loadSettings()` reads. Unlike `envFileSigs`, which structurally reuses the + * same `findEnvFiles()` that `loadEnvironment()` calls, these four scopes are + * enumerated independently. If a future PR adds a new settings scope to + * `loadSettings()`, add its path here too — otherwise the cache would serve a + * stale `LoadedSettings` with no test or compile-time signal. */ function settingsFileSigs(workspaceDir: string): FileSig[] { const filePaths = [ @@ -154,18 +170,24 @@ export function loadSettingsCached(workspaceDir: string): LoadedSettings { let isFresh = false; try { isFresh = isEntryFresh(key, entry); - } catch { + } catch (error) { // Fail open: any unexpected fingerprint error (EACCES, EIO, ...) is // treated as a miss so the cache never becomes a new failure source. + debugLogger.warn( + `fingerprint check failed for ${key}; reloading:`, + error, + ); } if (isFresh) { // Refresh LRU position (Map preserves insertion order). cache.delete(key); cache.set(key, entry); + debugLogger.debug(`hit ${key}`); return entry.settings; } cache.delete(key); } + debugLogger.debug(`miss ${key}`); // Sign the settings files BEFORE loading: if one changes while // loadSettings() runs, the stored signature is already stale and the next @@ -174,8 +196,9 @@ export function loadSettingsCached(workspaceDir: string): LoadedSettings { let preLoadSigs: FileSig[] | undefined; try { preLoadSigs = settingsFileSigs(key); - } catch { + } catch (error) { // Fail open: cache nothing this round. + debugLogger.warn(`pre-load signing failed for ${key}; not caching:`, error); } // Load errors (FatalConfigError etc.) propagate unchanged and uncached; @@ -200,11 +223,13 @@ export function loadSettingsCached(workspaceDir: string): LoadedSettings { const oldest = cache.keys().next().value; if (oldest !== undefined) { cache.delete(oldest); + debugLogger.debug(`evicted LRU entry ${oldest}`); } } - } catch { + } catch (error) { // Fail open: serve the freshly loaded settings uncached. cache.delete(key); + debugLogger.warn(`caching failed for ${key}; serving uncached:`, error); } }