fix(cli,core): harden OOM prevention — idempotent compaction tests, explicit GC, debug log defaults (#4914)

* test(cli): add compactOldItems idempotency regression tests

Cover the scenario fixed in commit 595701096 where already-compacted
tool groups (resultDisplay === UI_COMPACT_CLEARED_MESSAGE) were
incorrectly counted as having real output, causing over-compaction.

Three new test cases:
- Already-compacted groups are not re-compacted; second call is a no-op
- All tool groups already compacted → no-op
- Mixed tool group (some tools real, some cleared) → only groups with
  real output are compacted

* fix(cli,core): enable explicit GC and disable debug log by default

- enableExplicitGC defaults to true, --expose-gc added to start/dev scripts
- isDebugLogFileEnabled() defaults to false (opt-in via QWEN_DEBUG_LOG_FILE=1)
- Add safety tests: trigger_gc only in critical tier, global.gc() only in
  memoryPressureMonitor.ts trigger_gc case

* fix: address R1 review comments for memory pressure monitor

- Replace brittle source-parsing test with behavioral tests for global.gc()
- Export UI_COMPACT_CLEARED_MESSAGE constant and use in tests
- Remove redundant NODE_OPTIONS override from start script
- Add production bin wrapper with --expose-gc for OOM protection
- Remove unused path import from memoryPressureMonitor.test.ts

Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com>

* fix: forward --expose-gc to all deployment modes

Standalone package shims and daemon-spawned sessions (AcpBridge,
httpAcpBridge) were missing --expose-gc, causing explicit GC to
silently fail under critical memory pressure.

Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com>

* fix: forward child process signal in cli-entry wrapper

Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com>

* fix(cli,channels): filter --inspect flags when forwarding execArgv to daemon children

* fix: make cli-entry.js executable (mode 100755)

* fix(core): reject whitespace-only QWEN_DEBUG_LOG_FILE and add QWEN_MEMORY_ENABLE_GC=0 opt-out

* fix(scripts): include cli-entry.js wrapper in dist package for npm publish

* fix(acp-bridge): forward --expose-gc and filter --inspect in spawnChannel

- Add --expose-gc to getAcpMemoryArgs() so daemon-spawned ACP children
  have global.gc() available for critical memory pressure cleanup
- Filter --inspect/-brk flags from process.execArgv to prevent port
  conflicts in multi-session daemon mode
- Update spawnChannel.test.ts for new getAcpMemoryArgs() return shape

This change was previously in httpAcpBridge.ts but lost during the
daemon refactor merge (#4490) that moved spawn logic to acp-bridge.

---------

Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com>
This commit is contained in:
yao 2026-06-14 10:40:53 +08:00 committed by GitHub
parent 800507598c
commit f9080e44fb
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
16 changed files with 392 additions and 42 deletions

View file

@ -23,7 +23,7 @@
"sandboxImageUri": "ghcr.io/qwenlm/qwen-code:0.18.0"
},
"scripts": {
"start": "cross-env node scripts/start.js",
"start": "node scripts/start.js",
"dev": "node scripts/dev.js",
"dev:daemon": "node scripts/daemon-dev.js",
"debug": "cross-env DEBUG=1 node --inspect-brk scripts/start.js",
@ -94,10 +94,11 @@
"@types/react-dom": "^19.2.0"
},
"bin": {
"qwen": "dist/cli.js"
"qwen": "scripts/cli-entry.js"
},
"files": [
"dist/",
"scripts/cli-entry.js",
"README.md",
"LICENSE"
],

View file

@ -247,23 +247,22 @@ describe('scrubChildEnv (defaultSpawnChannelFactory env policy)', () => {
});
describe('getAcpMemoryArgs', () => {
it('returns a valid --max-old-space-size flag or empty array', () => {
it('always includes --expose-gc and optionally --max-old-space-size', () => {
const args = getAcpMemoryArgs();
if (args.length > 0) {
expect(args).toHaveLength(1);
expect(args[0]).toMatch(/^--max-old-space-size=\d+$/);
const sizeMB = Number(args[0]!.split('=')[1]);
expect(args).toContain('--expose-gc');
const heapArg = args.find((a) => a.startsWith('--max-old-space-size='));
if (heapArg) {
const sizeMB = Number(heapArg.split('=')[1]);
expect(sizeMB).toBeGreaterThan(0);
expect(sizeMB).toBeLessThanOrEqual(16_384);
} else {
expect(args).toEqual([]);
}
});
it('respects the 16GB cap', () => {
const args = getAcpMemoryArgs();
if (args.length > 0) {
const sizeMB = Number(args[0]!.split('=')[1]);
const heapArg = args.find((a) => a.startsWith('--max-old-space-size='));
if (heapArg) {
const sizeMB = Number(heapArg.split('=')[1]);
expect(sizeMB).toBeLessThanOrEqual(16_384);
}
});

View file

@ -26,8 +26,10 @@ export function getAcpMemoryArgs(): string[] {
const currentLimitMB = Math.floor(
getHeapStatistics().heap_size_limit / (1024 * 1024),
);
cachedMemoryArgs =
targetMB > currentLimitMB ? [`--max-old-space-size=${targetMB}`] : [];
cachedMemoryArgs = [
...(targetMB > currentLimitMB ? [`--max-old-space-size=${targetMB}`] : []),
'--expose-gc',
];
return cachedMemoryArgs;
}
@ -123,11 +125,18 @@ export function createSpawnChannelFactory(
childEnv['QWEN_CODE_NO_RELAUNCH'] = 'true';
const memoryArgs = getAcpMemoryArgs();
const child = spawn(process.execPath, [...memoryArgs, cliEntry, '--acp'], {
cwd: workspaceCwd,
stdio: ['pipe', 'pipe', 'pipe'],
env: childEnv,
});
const execArgs = process.execArgv.filter(
(a) => !/^--inspect(-brk)?($|=)/.test(a),
);
const child = spawn(
process.execPath,
[...execArgs, ...memoryArgs, cliEntry, '--acp'],
{
cwd: workspaceCwd,
stdio: ['pipe', 'pipe', 'pipe'],
env: childEnv,
},
);
// Forward child stderr to the daemon's stderr line-by-line, with a
// `[serve pid=… cwd=…]` prefix on each line so operators can

View file

@ -53,7 +53,11 @@ export class AcpBridge extends EventEmitter {
async start(): Promise<void> {
const { cliEntryPath, cwd } = this.options;
const args = [cliEntryPath, '--acp'];
const args = [
...process.execArgv.filter((a) => !/^--inspect(-brk)?($|=)/.test(a)),
cliEntryPath,
'--acp',
];
if (this.options.model) {
args.push('--model', this.options.model);
}

View file

@ -6,7 +6,7 @@
import { describe, it, expect, vi, beforeEach } from 'vitest';
import { renderHook, act } from '@testing-library/react';
import { useHistory } from './useHistoryManager.js';
import { useHistory, UI_COMPACT_CLEARED_MESSAGE } from './useHistoryManager.js';
import type { UseHistoryManagerReturn } from './useHistoryManager.js';
import type { HistoryItemWithoutId, HistoryItemToolGroup } from '../types.js';
import { ToolCallStatus } from '../types.js';
@ -314,7 +314,7 @@ describe('useHistoryManager', () => {
const tool = (
result.current.history[0] as unknown as HistoryItemToolGroup
).tools[0];
expect(tool.resultDisplay).toBe('[Old tool result content cleared]');
expect(tool.resultDisplay).toBe(UI_COMPACT_CLEARED_MESSAGE);
// Last 20 (newest) should be untouched
const recentTool = (
result.current.history[24] as unknown as HistoryItemToolGroup
@ -360,7 +360,7 @@ describe('useHistoryManager', () => {
const tool = (
result.current.history[0] as unknown as HistoryItemToolGroup
).tools[0];
expect(tool.resultDisplay).toBe('[Old tool result content cleared]');
expect(tool.resultDisplay).toBe(UI_COMPACT_CLEARED_MESSAGE);
});
it('should return same reference for empty history', () => {
@ -410,7 +410,7 @@ describe('useHistoryManager', () => {
const tool = (
result.current.history[i] as unknown as HistoryItemToolGroup
).tools[0];
expect(tool.resultDisplay).toBe('[Old tool result content cleared]');
expect(tool.resultDisplay).toBe(UI_COMPACT_CLEARED_MESSAGE);
}
// Last 20 (newest) should be untouched
for (let i = 10; i < 30; i++) {
@ -488,7 +488,7 @@ describe('useHistoryManager', () => {
for (let i = 0; i < 10; i++) {
const tool = (remainingToolGroups[i] as unknown as HistoryItemToolGroup)
.tools[0];
expect(tool.resultDisplay).toBe('[Old tool result content cleared]');
expect(tool.resultDisplay).toBe(UI_COMPACT_CLEARED_MESSAGE);
}
// Last 20 tool_groups should be untouched
@ -569,7 +569,7 @@ describe('useHistoryManager', () => {
const tool = (
result.current.history[i] as unknown as HistoryItemToolGroup
).tools[0];
expect(tool.resultDisplay).toBe('[Old tool result content cleared]');
expect(tool.resultDisplay).toBe(UI_COMPACT_CLEARED_MESSAGE);
}
// Last 20 (newest) should be untouched
@ -577,9 +577,7 @@ describe('useHistoryManager', () => {
const tool = (
result.current.history[i] as unknown as HistoryItemToolGroup
).tools[0];
expect(tool.resultDisplay).not.toBe(
'[Old tool result content cleared]',
);
expect(tool.resultDisplay).not.toBe(UI_COMPACT_CLEARED_MESSAGE);
}
});
@ -649,5 +647,177 @@ describe('useHistoryManager', () => {
// Should not compact non-compactable types
expect(result.current.history).toBe(before);
});
it('should not re-compact already-compacted tool groups (idempotent)', () => {
const { result } = renderHook(() => useHistory());
const ts = Date.now();
// 15 already-compacted + 15 fresh tool_groups = 30 total
for (let i = 0; i < 15; i++) {
act(() => {
result.current.addItem(
{
type: 'tool_group',
tools: [
{
callId: String(i),
name: 'read_file',
description: '',
resultDisplay: UI_COMPACT_CLEARED_MESSAGE,
status: ToolCallStatus.Success,
confirmationDetails: undefined,
},
],
} as unknown as HistoryItemWithoutId,
ts + i,
);
});
}
for (let i = 15; i < 30; i++) {
act(() => {
result.current.addItem(
{
type: 'tool_group',
tools: [
{
callId: String(i),
name: 'read_file',
description: '',
resultDisplay: `content-${i}`,
status: ToolCallStatus.Success,
confirmationDetails: undefined,
},
],
} as unknown as HistoryItemWithoutId,
ts + i,
);
});
}
act(() => {
result.current.compactOldItems();
});
// totalToolGroupsWithOutput = 15 (only fresh ones), toolGroupsToCompact = 0
// → no additional compaction, all 30 items kept
expect(result.current.history).toHaveLength(30);
// Already-compacted items should still be the cleared message
for (let i = 0; i < 15; i++) {
const tool = (
result.current.history[i] as unknown as HistoryItemToolGroup
).tools[0];
expect(tool.resultDisplay).toBe(UI_COMPACT_CLEARED_MESSAGE);
}
// Fresh items should remain untouched
for (let i = 15; i < 30; i++) {
const tool = (
result.current.history[i] as unknown as HistoryItemToolGroup
).tools[0];
expect(tool.resultDisplay).toBe(`content-${i}`);
}
// Second call should be a no-op (same reference)
const before = result.current.history;
act(() => {
result.current.compactOldItems();
});
expect(result.current.history).toBe(before);
});
it('should handle all tool groups already compacted', () => {
const { result } = renderHook(() => useHistory());
const ts = Date.now();
for (let i = 0; i < 30; i++) {
act(() => {
result.current.addItem(
{
type: 'tool_group',
tools: [
{
callId: String(i),
name: 'read_file',
description: '',
resultDisplay: UI_COMPACT_CLEARED_MESSAGE,
status: ToolCallStatus.Success,
confirmationDetails: undefined,
},
],
} as unknown as HistoryItemWithoutId,
ts + i,
);
});
}
const before = result.current.history;
act(() => {
result.current.compactOldItems();
});
// totalToolGroupsWithOutput = 0, nothing to compact
expect(result.current.history).toBe(before);
});
it('should handle tool group with mixed output (some tools real, some cleared)', () => {
const { result } = renderHook(() => useHistory());
const ts = Date.now();
// 25 tool_groups, each with 2 tools: one with output, one cleared
for (let i = 0; i < 25; i++) {
act(() => {
result.current.addItem(
{
type: 'tool_group',
tools: [
{
callId: `${i}-a`,
name: 'read_file',
description: '',
resultDisplay: `content-${i}`,
status: ToolCallStatus.Success,
confirmationDetails: undefined,
},
{
callId: `${i}-b`,
name: 'edit',
description: '',
resultDisplay: UI_COMPACT_CLEARED_MESSAGE,
status: ToolCallStatus.Success,
confirmationDetails: undefined,
},
],
} as unknown as HistoryItemWithoutId,
ts + i,
);
});
}
act(() => {
result.current.compactOldItems();
});
// totalToolGroupsWithOutput = 25 (hasOldOutput is true because tool[0] has real output)
// toolGroupsToCompact = max(0, 25 - 20) = 5
// First 5 should be compacted: both tools get resultDisplay replaced
for (let i = 0; i < 5; i++) {
const tools = (
result.current.history[i] as unknown as HistoryItemToolGroup
).tools;
expect(tools[0].resultDisplay).toBe(UI_COMPACT_CLEARED_MESSAGE);
expect(tools[1].resultDisplay).toBe(UI_COMPACT_CLEARED_MESSAGE);
}
// Last 20 should be untouched
for (let i = 5; i < 25; i++) {
const tools = (
result.current.history[i] as unknown as HistoryItemToolGroup
).tools;
expect(tools[0].resultDisplay).toBe(`content-${i}`);
expect(tools[1].resultDisplay).toBe(UI_COMPACT_CLEARED_MESSAGE);
}
});
});
});

View file

@ -16,7 +16,7 @@ type HistoryItemUpdater = (
prevItem: HistoryItem,
) => Partial<HistoryItemWithoutId>;
const UI_COMPACT_CLEARED_MESSAGE = '[Old tool result content cleared]';
export const UI_COMPACT_CLEARED_MESSAGE = '[Old tool result content cleared]';
const UI_COMPACT_KEEP_RECENT = 20;
export interface UseHistoryManagerReturn {

View file

@ -315,7 +315,6 @@ const MEMORY_PRESSURE_ENV_KEYS = [
'QWEN_MEMORY_PRESSURE_SOFT',
'QWEN_MEMORY_PRESSURE_HARD',
'QWEN_MEMORY_PRESSURE_CRITICAL',
'QWEN_MEMORY_ENABLE_GC',
];
vi.mock('../core/baseLlmClient.js');
@ -669,8 +668,7 @@ describe('Server Config (config.ts)', () => {
},
);
it('enables explicit GC when requested by env', async () => {
process.env['QWEN_MEMORY_ENABLE_GC'] = '1';
it('explicit GC is enabled by default', async () => {
const globalWithGc = global as typeof global & { gc?: () => void };
const originalGc = globalWithGc.gc;
const gcSpy = vi.fn();

View file

@ -988,8 +988,12 @@ function loadMemoryPressureConfig(): MemoryPressureConfig {
config.criticalRatio,
);
if (process.env['QWEN_MEMORY_ENABLE_GC'] === '1') {
config.enableExplicitGC = true;
const enableGC = process.env['QWEN_MEMORY_ENABLE_GC'];
if (
enableGC &&
['0', 'false', 'off', 'no'].includes(enableGC.trim().toLowerCase())
) {
config.enableExplicitGC = false;
}
validateMemoryPressureConfig(config);

View file

@ -1668,6 +1668,107 @@ describe('MemoryPressureMonitor', () => {
});
});
describe('global.gc() safety guards', () => {
it('should not include trigger_gc in soft or hard pressure tiers', () => {
const monitor = new MemoryPressureMonitor(
createMockConfig({
fileReadCache: {
clear: vi.fn(),
evictNotAccessedSince: vi.fn(),
},
}),
{
...DEFAULT_PRESSURE_CONFIG,
cleanupCooldownMs: 0,
enableExplicitGC: true,
},
);
// Soft pressure — should NOT trigger GC
setMemUsage(9 * 1024 * 1024 * 1024); // 9/16 = 0.5625 >= 0.5 soft
monitor.performCheck();
expect(mockDebugLogger.debug).not.toHaveBeenCalledWith(
expect.stringContaining('global.gc()'),
);
// Reset for next check
mockDebugLogger.debug.mockClear();
// Hard pressure — should NOT trigger GC
setMemUsage(11 * 1024 * 1024 * 1024); // 11/16 = 0.6875 >= 0.65 hard
monitor.performCheck();
expect(mockDebugLogger.debug).not.toHaveBeenCalledWith(
expect.stringContaining('global.gc()'),
);
});
it('global.gc() is only called under critical pressure with enableExplicitGC: true', async () => {
const gcSpy = vi.fn();
vi.stubGlobal('gc', gcSpy);
const monitor = new MemoryPressureMonitor(
createMockConfig({
fileReadCache: {
clear: vi.fn(),
evictNotAccessedSince: vi.fn(),
},
}),
{
...DEFAULT_PRESSURE_CONFIG,
cleanupCooldownMs: 0,
enableExplicitGC: true,
},
);
// Soft pressure — GC should NOT be called
setMemUsage(9 * 1024 * 1024 * 1024); // 9/16 = 0.5625 >= 0.5 soft
monitor.performCheck();
await drainCleanupMeasurement();
expect(gcSpy).not.toHaveBeenCalled();
gcSpy.mockClear();
// Hard pressure — GC should NOT be called
setMemUsage(11 * 1024 * 1024 * 1024); // 11/16 = 0.6875 >= 0.65 hard
monitor.performCheck();
await drainCleanupMeasurement();
expect(gcSpy).not.toHaveBeenCalled();
gcSpy.mockClear();
// Critical pressure — GC SHOULD be called exactly once
setMemUsage(14 * 1024 * 1024 * 1024); // 14/16 = 0.875 >= 0.8 critical
monitor.performCheck();
await drainCleanupMeasurement();
expect(gcSpy).toHaveBeenCalledTimes(1);
});
it('global.gc() is never called when enableExplicitGC is false', async () => {
const gcSpy = vi.fn();
vi.stubGlobal('gc', gcSpy);
const monitor = new MemoryPressureMonitor(
createMockConfig({
fileReadCache: {
clear: vi.fn(),
evictNotAccessedSince: vi.fn(),
},
}),
{
...DEFAULT_PRESSURE_CONFIG,
cleanupCooldownMs: 0,
enableExplicitGC: false,
},
);
// Critical pressure — GC should NOT be called
setMemUsage(14 * 1024 * 1024 * 1024);
monitor.performCheck();
await drainCleanupMeasurement();
expect(gcSpy).not.toHaveBeenCalled();
});
});
describe('runtime sampling and telemetry', () => {
beforeEach(() => {
setOsTotalmem(16 * 1024 * 1024 * 1024);

View file

@ -209,7 +209,7 @@ export const DEFAULT_PRESSURE_CONFIG: MemoryPressureConfig = {
hardPressureRatio: 0.65,
criticalRatio: 0.8,
cleanupCooldownMs: 5_000,
enableExplicitGC: false,
enableExplicitGC: true,
};
// Validation

View file

@ -37,9 +37,9 @@ const sessionContext = new AsyncLocalStorage<DebugLogSession>();
function isDebugLogFileEnabled(): boolean {
const value = process.env['QWEN_DEBUG_LOG_FILE'];
if (!value) return true;
if (!value) return false;
const normalized = value.trim().toLowerCase();
return !['0', 'false', 'off', 'no'].includes(normalized);
return !['', '0', 'false', 'off', 'no'].includes(normalized);
}
function getActiveSession(): DebugLogSession | null {

37
scripts/cli-entry.js Executable file
View file

@ -0,0 +1,37 @@
#!/usr/bin/env node
/**
* @license
* Copyright 2025 Qwen Code
* SPDX-License-Identifier: Apache-2.0
*/
/**
* Production bin entry wrapper.
*
* Launches dist/cli.js with --expose-gc so that global.gc() is available
* for the memory-pressure monitor's critical-tier cleanup.
*
* --expose-gc only exposes the function; it has zero runtime cost.
* global.gc() is called only when RSS hits the critical threshold (0.80),
* where the 10-200 ms pause is acceptable to avoid an OOM kill.
*/
import { spawnSync } from 'node:child_process';
import { fileURLToPath } from 'node:url';
import { dirname, join } from 'node:path';
const __dirname = dirname(fileURLToPath(import.meta.url));
const cliPath = join(__dirname, '..', 'dist', 'cli.js');
const result = spawnSync(
process.execPath,
['--expose-gc', cliPath, ...process.argv.slice(2)],
{ stdio: 'inherit' },
);
if (result.signal) {
process.kill(process.pid, result.signal);
} else {
process.exit(result.status ?? 1);
}

View file

@ -494,7 +494,7 @@ function writeShims(packageRoot) {
const unixShim = `#!/usr/bin/env sh
set -e
ROOT="$(CDPATH= cd -- "$(dirname -- "$0")/.." && pwd)"
exec "$ROOT/node/bin/node" "$ROOT/lib/cli.js" "$@"
exec "$ROOT/node/bin/node" --expose-gc "$ROOT/lib/cli.js" "$@"
`;
const unixShimPath = path.join(binDir, 'qwen');
fs.writeFileSync(unixShimPath, unixShim);
@ -503,7 +503,7 @@ exec "$ROOT/node/bin/node" "$ROOT/lib/cli.js" "$@"
const windowsShim = `@echo off
setlocal
set "ROOT=%~dp0.."
"%ROOT%\\node\\node.exe" "%ROOT%\\lib\\cli.js" %*
"%ROOT%\\node\\node.exe" --expose-gc "%ROOT%\\lib\\cli.js" %*
`;
fs.writeFileSync(path.join(binDir, 'qwen.cmd'), windowsShim);
}

View file

@ -104,7 +104,7 @@ const env = {
DEV: 'true',
CLI_VERSION: 'dev',
NODE_ENV: 'development',
NODE_OPTIONS: `${existingNodeOptions} ${importFlag}`.trim(),
NODE_OPTIONS: `${existingNodeOptions} --expose-gc ${importFlag}`.trim(),
};
// On Windows, use tsx.cmd; on Unix, use tsx directly

View file

@ -123,6 +123,32 @@ function copyExtensionExamples(rootDir, distDir) {
function writeDistPackageJson(rootDir, distDir) {
console.log('Creating package.json for distribution...');
const cliEntryContent = `#!/usr/bin/env node
import { spawnSync } from 'node:child_process';
import { fileURLToPath } from 'node:url';
import { dirname, join } from 'node:path';
const __dirname = dirname(fileURLToPath(import.meta.url));
const cliPath = join(__dirname, 'cli.js');
const result = spawnSync(
process.execPath,
['--expose-gc', cliPath, ...process.argv.slice(2)],
{ stdio: 'inherit' },
);
if (result.signal) {
process.kill(process.pid, result.signal);
} else {
process.exit(result.status ?? 1);
}
`;
const cliEntryPath = path.join(distDir, 'cli-entry.js');
fs.writeFileSync(cliEntryPath, cliEntryContent, { mode: 0o755 });
console.log('Created dist cli-entry.js wrapper');
const rootPackageJson = JSON.parse(
fs.readFileSync(path.join(rootDir, 'package.json'), 'utf-8'),
);
@ -136,9 +162,10 @@ function writeDistPackageJson(rootDir, distDir) {
type: 'module',
main: 'cli.js',
bin: {
qwen: 'cli.js',
qwen: 'cli-entry.js',
},
files: [
'cli-entry.js',
'cli.js',
// Worker thread entry loaded by FzfWorkerHandle at runtime via
// `resolveBundleDir(import.meta.url)` + `path.join(dir, 'fzfWorker.js')`.

View file

@ -32,7 +32,7 @@ execSync('node ./scripts/check-build-status.js', {
cwd: root,
});
const nodeArgs = [];
const nodeArgs = ['--expose-gc'];
let sandboxCommand = undefined;
try {
sandboxCommand = execSync('node scripts/sandbox_command.js', {