vrr/qwen-code
2
0
Fork 0
mirror of https://github.com/QwenLM/qwen-code.git synced 2026-07-12 02:29:00 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 10

fix(core): pr #4996 review round 1 — leak fixes via explicit dispose contract

Browse source 
Reviewer flagged two Criticals + one Suggestion + one Nice-to-have. The two
Criticals share a root cause (subagent execute()'s inner try/finally
doesn't fire on every exit path), so they fold into a single API change.

## [Critical] Hook cleanup leak on AgentHeadless.execute() early exits

`wrapAgentHooksForCleanup` relied on `onStop` firing inside execute()'s
inner try/finally. Two early-exit paths bypass that finally:

1. `createChat()` returning null at agent-headless.ts:224-226 — returns
   before the outer `try` at 233 is even entered.
2. `prepareTools()` throwing at 234 — propagates through the outer
   `finally` at 335, which only calls `abortController.abort()` and
   never reaches the inner finally that fires `onStop`.

The pre-fix `catch` block only guarded `AgentHeadless.create()`, not
`execute()`. Leaked HookRegistry entries fire globally for every matching
event in the session, polluting unrelated tool calls.

## [Critical] Per-agent MCP server processes leak after every spawn

`discoverToolsForServer` connects real MCP clients (stdio child
processes, HTTP/SSE sockets) in the force-rebuilt subagent ToolRegistry.
Nothing stopped that registry: `Config.shutdown` only reaches the root's
`this.toolRegistry`, and AgentTool's existing `agentConfig.getToolRegistry()
.stop()` (fg + bg + resume finally blocks) only stops the parent's
registry, not the override's distinct fresh one. Every subagent
invocation that declared `mcpServers` orphaned a child process for the
rest of the host process's lifetime.

## Shared fix — caller-driven `dispose` contract

`SubagentManager.createAgentHeadless` now returns
`{ subagent, dispose }`. Callers MUST invoke `dispose()` in the same
`finally` block that wraps `subagent.execute()`. That `finally` lives
in the caller's scope (AgentTool fg/bg, BackgroundAgentResumeService),
which is reachable on every execute() exit — including the two early-
exit paths the previous `onStop` hook never reached.

`dispose` is a single closure that calls the previously-separate
cleanup callbacks in order:

1. `unregisterAgentHooks` returned from `HookRegistry.addAgentHooks`
   (when per-agent hooks were registered).
2. `disposeRegistry` returned alongside the new `buildSubagentContextOverride`
   return shape `{ context, disposeRegistry }` (set only when this call
   force-rebuilt the registry for `mcpServers`).

Both cleanups are wrapped in `try/finally` that logs and re-arms so an
exception in one path doesn't block the other and doesn't double-fire.
The pre-existing constructor-failure catch in `createAgentHeadless` now
runs the same closure directly — the caller never received the return
value, so it cannot fire `dispose` itself.

The three callers gain one variable + one `void dispose?.().catch()`
inside their existing finally:

- `agent.ts:2039` (foreground) — finally at `agent.ts:2904`
- `agent.ts:2131` (background) — finally at `agent.ts:2502`
- `background-agent-resume.ts:630` (resume) — finally at `:852`

Fork subagents share the parent's lifecycle; their `dispose` stays
undefined and the `?.()` is a no-op.

`wrapAgentHooksForCleanup` is removed — it was load-bearing only for
the happy + inner-reasoning-loop-failure paths and is now obsolete.

## [Suggestion] Repeated guard condition

`config.hooks && Object.keys(config.hooks).length > 0` no longer appears
in both `if` / `else if` arms. Single outer guard + nested branch on
`hookRegistry`.

## [Nice-to-have] claude-converter.ts:290 missing `.trim()`

`stringifyYaml(newFrontmatter).trim()` brings the converter into line
with `subagent-manager.ts:651`. Without trim, eemeli/yaml's trailing
newline produced an extra blank line before the closing `---`
delimiter — cosmetic (both readers tolerate it) but the asymmetry
between the two writers was a real consistency bug.

## Tests

3 new RED-first tests in `subagent-manager.test.ts` pin the dispose
contract:

1. `returns { subagent, dispose }; dispose unregisters per-agent hooks`
2. `dispose unregisters even when execute() never runs (early-exit leak fix)`
   — the case where `createChat()` → null or `prepareTools()` throws
3. `dispose is a safe no-op when neither hooks nor mcpServers are declared`

Override test helper updated to destructure the new
`buildSubagentContextOverride` return shape. AgentTool + BackgroundAgent
test mocks updated to return `{ subagent, dispose }`. All 2087 in-scope
tests pass.
This commit is contained in:
LaZzyMan 2026-06-12 10:38:11 +08:00
parent 791b38bf5a
commit 720f0e4a1f
8 changed files with 368 additions and 123 deletions
Download patch file Download diff file Expand all files Collapse all files

71
packages/core/src/agents/background-agent-resume.test.ts
View file

@ -430,7 +430,10 @@ describe('BackgroundAgentResumeService', () => {
};
const { service, subagentManager, hookSystem } = createService();
subagentManager.createAgentHeadless.mockResolvedValue(subagent);
subagentManager.createAgentHeadless.mockResolvedValue({
subagent,
dispose: vi.fn().mockResolvedValue(undefined),
});
hookSystem.fireSubagentStartEvent.mockResolvedValue({
getAdditionalContext: () => 'resume-context',
});
@ -518,7 +521,10 @@ describe('BackgroundAgentResumeService', () => {
};
const { service, subagentManager } = createService();
subagentManager.createAgentHeadless.mockResolvedValue(subagent);
subagentManager.createAgentHeadless.mockResolvedValue({
subagent,
dispose: vi.fn().mockResolvedValue(undefined),
});
const resumed = await service.resumeBackgroundAgent(agentId, 'continue');
@ -653,7 +659,10 @@ describe('BackgroundAgentResumeService', () => {
};
const { service, subagentManager, hookSystem } = createService();
subagentManager.createAgentHeadless.mockResolvedValue(subagent);
subagentManager.createAgentHeadless.mockResolvedValue({
subagent,
dispose: vi.fn().mockResolvedValue(undefined),
});
const resumed = await service.resumeBackgroundAgent(agentId, 'continue');
@ -734,7 +743,10 @@ describe('BackgroundAgentResumeService', () => {
const { service, subagentManager, hookSystem } = createService({
stopHookBlockingCap: 2,
});
subagentManager.createAgentHeadless.mockResolvedValue(subagent);
subagentManager.createAgentHeadless.mockResolvedValue({
subagent,
dispose: vi.fn().mockResolvedValue(undefined),
});
hookSystem.fireSubagentStopEvent.mockResolvedValue(stopOutput);
const resumed = await service.resumeBackgroundAgent(agentId, 'continue');
@ -801,15 +813,18 @@ describe('BackgroundAgentResumeService', () => {
});
const createAgentHeadless = vi.fn().mockResolvedValue({
execute: vi.fn(async () => undefined),
setExternalMessageProvider: vi.fn(),
getCore: () => ({ getEventEmitter: () => new AgentEventEmitter() }),
getExecutionSummary: () => ({
totalTokens: 0,
totalDurationMs: 0,
}),
getTerminateMode: () => AgentTerminateMode.GOAL,
getFinalText: () => 'done',
subagent: {
execute: vi.fn(async () => undefined),
setExternalMessageProvider: vi.fn(),
getCore: () => ({ getEventEmitter: () => new AgentEventEmitter() }),
getExecutionSummary: () => ({
totalTokens: 0,
totalDurationMs: 0,
}),
getTerminateMode: () => AgentTerminateMode.GOAL,
getFinalText: () => 'done',
},
dispose: vi.fn().mockResolvedValue(undefined),
});
const { service, subagentManager } = createService();
@ -890,7 +905,10 @@ describe('BackgroundAgentResumeService', () => {
};
const { service, subagentManager } = createService();
subagentManager.createAgentHeadless.mockResolvedValue(subagent);
subagentManager.createAgentHeadless.mockResolvedValue({
subagent,
dispose: vi.fn().mockResolvedValue(undefined),
});
const first = service.resumeBackgroundAgent(agentId, 'first message');
const second = service.resumeBackgroundAgent(agentId, 'second message');
@ -971,7 +989,10 @@ describe('BackgroundAgentResumeService', () => {
getFinalText: () => 'done',
};
const { service, subagentManager, monitorRegistry } = createService();
subagentManager.createAgentHeadless.mockResolvedValue(subagent);
subagentManager.createAgentHeadless.mockResolvedValue({
subagent,
dispose: vi.fn().mockResolvedValue(undefined),
});
const resume = service.resumeBackgroundAgent(agentId, 'continue');
await vi.waitFor(() => {
@ -1080,7 +1101,10 @@ describe('BackgroundAgentResumeService', () => {
getFinalText: () => 'done',
};
const { service, subagentManager, monitorRegistry } = createService();
subagentManager.createAgentHeadless.mockResolvedValue(subagent);
subagentManager.createAgentHeadless.mockResolvedValue({
subagent,
dispose: vi.fn().mockResolvedValue(undefined),
});
await expect(
service.resumeBackgroundAgent(agentId, 'continue'),
@ -1484,7 +1508,10 @@ describe('BackgroundAgentResumeService', () => {
};
const { service, subagentManager } = createService();
subagentManager.createAgentHeadless.mockResolvedValue(subagent);
subagentManager.createAgentHeadless.mockResolvedValue({
subagent,
dispose: vi.fn().mockResolvedValue(undefined),
});
const resumed = await service.resumeBackgroundAgent(agentId, 'continue');
expect(resumed).toBeDefined();
@ -1559,7 +1586,10 @@ describe('BackgroundAgentResumeService', () => {
};
const { service, subagentManager } = createService();
subagentManager.createAgentHeadless.mockResolvedValue(subagent);
subagentManager.createAgentHeadless.mockResolvedValue({
subagent,
dispose: vi.fn().mockResolvedValue(undefined),
});
const resumed = await service.resumeBackgroundAgent(agentId, 'continue');
expect(resumed).toBeDefined();
@ -1654,7 +1684,10 @@ describe('BackgroundAgentResumeService', () => {
};
const { service, subagentManager } = createService();
subagentManager.createAgentHeadless.mockResolvedValue(subagent);
subagentManager.createAgentHeadless.mockResolvedValue({
subagent,
dispose: vi.fn().mockResolvedValue(undefined),
});
await service.resumeBackgroundAgent(agentId, 'continue work');

46
packages/core/src/agents/background-agent-resume.ts
View file

@ -618,21 +618,32 @@ export class BackgroundAgentResumeService {
}
const bgEventEmitter = new AgentEventEmitter();
const subagent = target.isFork
? await this.createResumedForkSubagent(
bgConfig as Config,
bgEventEmitter,
resumeHistory ?? [],
recovery.forkBootstrap!,
)
: await this.config
.getSubagentManager()
.createAgentHeadless(target.subagentConfig!, bgConfig as Config, {
eventEmitter: bgEventEmitter,
promptConfigOverrides: {
initialMessages: resumeHistory,
},
});
// Per-spawn cleanup from `SubagentManager.createAgentHeadless` —
// the resume `finally` invokes this so per-agent hook entries and
// the force-rebuilt ToolRegistry don't leak across the resume
// boundary. Stays undefined on the fork-resume path (forks share
// the parent's registry + hook lifecycle).
let subagentDispose: (() => Promise<void>) | undefined;
let subagent: AgentHeadless;
if (target.isFork) {
subagent = await this.createResumedForkSubagent(
bgConfig as Config,
bgEventEmitter,
resumeHistory ?? [],
recovery.forkBootstrap!,
);
} else {
const result = await this.config
.getSubagentManager()
.createAgentHeadless(target.subagentConfig!, bgConfig as Config, {
eventEmitter: bgEventEmitter,
promptConfigOverrides: {
initialMessages: resumeHistory,
},
});
subagent = result.subagent;
subagentDispose = result.dispose;
}
const projectRoot = this.config.getProjectRoot();
cleanupJsonl = attachJsonlTranscriptWriter(bgEventEmitter, outputFile, {
@ -840,6 +851,11 @@ export class BackgroundAgentResumeService {
.getToolRegistry()
.stop()
.catch(() => {});
// Per-spawn cleanup from `createAgentHeadless`: releases agent-
// scope hook entries and stops the per-agent ToolRegistry that
// the force rebuild created for `mcpServers`. Distinct from the
// parent registry above (no-op when target.isFork).
void subagentDispose?.().catch(() => {});
// Restore parent PermissionManager's dangerous allow rules if
// this override stripped them. See createApprovalModeOverride
// strip-lifecycle comment in agent.ts.

8
packages/core/src/extension/claude-converter.ts
View file

@ -286,8 +286,12 @@ async function convertAgentFiles(agentsDir: string): Promise<void> {
}
}
// Write converted content back
const newYaml = stringifyYaml(newFrontmatter);
// Write converted content back. Trim to drop the trailing newline
// `yaml.stringify` appends so the assembled file has the same single
// blank line between the closing `---` and the body that
// `subagent-manager.ts:serializeSubagent` produces — without `.trim()`
// the converter emits an extra blank line before the closing `---`.
const newYaml = stringifyYaml(newFrontmatter).trim();
const systemPrompt = (qwenAgent['systemPrompt'] as string) || body.trim();
const newContent = `---
${newYaml}

10
packages/core/src/subagents/subagent-manager-override.test.ts
View file

@ -40,7 +40,7 @@ describe('SubagentManager.buildSubagentContextOverride bound-tool isolation', ()
// The method is `private`. Cast via `unknown` to invoke it directly —
// testing through the public `createAgentHeadless` pathway would also
// work but pulls in a much larger graph (file IO, hooks, etc.).
function callBuildOverride(
async function callBuildOverride(
manager: SubagentManager,
base: Config,
config?: Partial<SubagentConfig>,
@ -50,7 +50,10 @@ describe('SubagentManager.buildSubagentContextOverride bound-tool isolation', ()
buildSubagentContextOverride: (
b: Config,
c: SubagentConfig,
) => Promise<Config>;
) => Promise<{
context: Config;
disposeRegistry?: () => Promise<void>;
}>;
}
).buildSubagentContextOverride.bind(manager);
const fullConfig: SubagentConfig = {
@ -60,7 +63,8 @@ describe('SubagentManager.buildSubagentContextOverride bound-tool isolation', ()
level: 'session',
...config,
};
return fn(base, fullConfig);
const result = await fn(base, fullConfig);
return result.context;
}
it('returns a Config whose registry is distinct from the parent and binds Edit/Read to the override', async () => {

119
packages/core/src/subagents/subagent-manager.test.ts
View file

@ -1955,5 +1955,124 @@ System prompt 3`);
expect(runtimeView).toBeUndefined();
});
});
describe('createAgentHeadless — caller-driven dispose contract', () => {
// Regression for self-inflicted leaks (review #4996 round 1):
// 1. `wrapAgentHooksForCleanup` relied on `AgentHeadless.execute()`'s
// inner finally firing `onStop`. Two execute() early-exit paths
// (`createChat()` → null and `prepareTools()` throwing) bypass
// that finally, so ephemeral hook entries leaked into the global
// registry for the rest of the session.
// 2. The forced tool-registry rebuild for per-agent `mcpServers`
// spawned real MCP client connections (stdio child processes,
// sockets) on a registry distinct from the parent's, but nothing
// stopped it — every subagent invocation declaring `mcpServers`
// orphaned its server processes.
//
// The unified fix is to return `{ subagent, dispose }` from
// `createAgentHeadless` and have callers run `dispose()` in a
// `finally` that they already own around `subagent.execute()`. These
// tests assert that contract.
const baseConfig: SubagentConfig = {
name: 'cleanup-agent',
description: 'dispose contract test',
systemPrompt: 'You are a test agent.',
level: 'session' as const,
};
beforeEach(() => {
mockAgentHeadlessCreate.mockResolvedValue({
execute: vi.fn(),
getResult: vi.fn(),
});
});
afterEach(() => {
mockAgentHeadlessCreate.mockReset();
});
it('returns { subagent, dispose }; dispose unregisters per-agent hooks', async () => {
const unregisterSpy = vi.fn();
const addAgentHooksSpy = vi.fn().mockReturnValue(unregisterSpy);
vi.spyOn(mockConfig, 'getHookSystem').mockReturnValue({
getRegistry: () => ({ addAgentHooks: addAgentHooksSpy }),
} as unknown as ReturnType<Config['getHookSystem']>);
const result = await manager.createAgentHeadless(
{
...baseConfig,
hooks: {
PreToolUse: [
{
matcher: 'Bash',
hooks: [{ type: 'command', command: 'echo' }],
},
],
},
},
mockConfig,
);
// The whole point: callers need an explicit cleanup handle they can
// invoke from the outer `finally`. A return shape of just
// `AgentHeadless` (the pre-fix contract) gives them no way to do
// that, because the inner onStop wrap doesn't fire on every
// execute() exit path.
expect(result).toHaveProperty('subagent');
expect(result).toHaveProperty('dispose');
expect(typeof result.dispose).toBe('function');
expect(addAgentHooksSpy).toHaveBeenCalledTimes(1);
expect(unregisterSpy).not.toHaveBeenCalled();
await result.dispose();
expect(unregisterSpy).toHaveBeenCalledTimes(1);
});
it('dispose unregisters even when execute() never runs (early-exit leak fix)', async () => {
// Caller pattern:
// const { subagent, dispose } = await createAgentHeadless(...);
// try { await subagent.execute(...); } finally { await dispose(); }
// We never call execute() in this test — that simulates the
// createChat-returns-null and prepareTools-throws paths where the
// pre-fix `onStop` wrapping never fired its cleanup.
const unregisterSpy = vi.fn();
vi.spyOn(mockConfig, 'getHookSystem').mockReturnValue({
getRegistry: () => ({
addAgentHooks: vi.fn().mockReturnValue(unregisterSpy),
}),
} as unknown as ReturnType<Config['getHookSystem']>);
const { dispose } = await manager.createAgentHeadless(
{
...baseConfig,
hooks: {
PreToolUse: [
{
matcher: '*',
hooks: [{ type: 'command', command: 'echo' }],
},
],
},
},
mockConfig,
);
await dispose();
expect(unregisterSpy).toHaveBeenCalledTimes(1);
});
it('dispose is a safe no-op when neither hooks nor mcpServers are declared', async () => {
const result = await manager.createAgentHeadless(
baseConfig,
mockConfig,
);
expect(typeof result.dispose).toBe('function');
// Must not throw — the caller's `finally` always invokes dispose,
// even for agents that triggered no cleanup-bearing setup.
await expect(result.dispose()).resolves.toBeUndefined();
});
});
});
});

168
packages/core/src/subagents/subagent-manager.ts
View file

@ -655,11 +655,29 @@ export class SubagentManager {
}
/**
* Creates an AgentHeadless from a subagent configuration.
* Creates an AgentHeadless from a subagent configuration and returns a
* `dispose` callback that releases the per-spawn cleanup-bearing resources
* (ephemeral hook entries registered against the session's HookRegistry,
* the per-agent tool registry created when `mcpServers` triggers a force
* rebuild and the MCP child processes / sockets it owns).
*
* Callers MUST invoke `dispose` in a `finally` block around the
* `subagent.execute()` call. This is the only reliable way to clean up
* across every execute() exit path: the inner try/finally inside
* `AgentHeadless.execute()` does not fire `onStop` on the early-exit
* paths (`createChat()` returning null, `prepareTools()` throwing), and a
* leaked HookRegistry entry would fire globally for every matching event
* for the rest of the session; a leaked ToolRegistry would leave stdio
* child processes alive until process exit.
*
* `dispose` is idempotent — calling it twice is safe (the unregister
* callback filters by `agentScope` and is a no-op the second time; the
* registry's `stop()` is itself documented idempotent).
*
* @param config - Subagent configuration
* @param runtimeContext - Runtime context
* @returns Promise resolving to AgentHeadless
* @returns the AgentHeadless and a `dispose` callback to run in the
* caller's `finally` block.
*/
async createAgentHeadless(
config: SubagentConfig,
@ -672,7 +690,38 @@ export class SubagentManager {
runConfigOverrides?: Partial<RunConfig>;
toolConfigOverride?: ToolConfig;
},
): Promise<AgentHeadless> {
): Promise<{ subagent: AgentHeadless; dispose: () => Promise<void> }> {
// Track per-spawn cleanup callbacks declared outside the inner
// `try/catch` so the catch can fire them on a constructor failure
// before the caller ever receives the return value. The successful
// path puts the same callbacks behind `dispose`.
let unregisterAgentHooks: (() => void) | undefined;
let disposeSubagentRegistry: (() => Promise<void>) | undefined;
const runCleanup = async (): Promise<void> => {
if (unregisterAgentHooks) {
try {
unregisterAgentHooks();
} catch (error) {
debugLogger.warn(
`Subagent "${config.name}": failed to unregister per-agent hooks: ${error instanceof Error ? error.message : String(error)}`,
);
} finally {
unregisterAgentHooks = undefined;
}
}
if (disposeSubagentRegistry) {
try {
await disposeSubagentRegistry();
} catch (error) {
debugLogger.warn(
`Subagent "${config.name}": failed to stop per-agent ToolRegistry: ${error instanceof Error ? error.message : String(error)}`,
);
} finally {
disposeSubagentRegistry = undefined;
}
}
};
try {
const runtimeConfig = await this.convertToRuntimeConfig(
config,
@ -702,46 +751,38 @@ export class SubagentManager {
runtimeContext,
);
const subagentContext = await this.buildSubagentContextOverride(
runtimeContext,
config,
);
const { context: subagentContext, disposeRegistry } =
await this.buildSubagentContextOverride(runtimeContext, config);
disposeSubagentRegistry = disposeRegistry;
// Register per-agent frontmatter hooks. Returns an unregister callback
// that the wrapped onStop hook below invokes when the agent terminates;
// a synchronous failure path also fires it via the catch block. v1
// limitation: while the entries live in the registry they fire for
// every event of their declared type, regardless of which agent is
// currently active — proper per-agent scope filtering is deferred.
// Register per-agent frontmatter hooks. The returned unregister callback
// is invoked from `dispose` (and from the catch block below on a
// constructor failure). v1 limitation: while the entries live in the
// registry they fire for every event of their declared type, regardless
// of which agent is currently active — proper per-agent scope filtering
// is deferred.
const hookSystem = runtimeContext.getHookSystem();
const hookRegistry = hookSystem?.getRegistry();
let unregisterAgentHooks: (() => void) | undefined;
if (
config.hooks &&
Object.keys(config.hooks).length > 0 &&
hookRegistry
) {
const agentScope = `agent:${config.name}:${randomUUID()}`;
unregisterAgentHooks = hookRegistry.addAgentHooks(
config.hooks as { [K in HookEventName]?: HookDefinition[] },
agentScope,
);
} else if (
config.hooks &&
Object.keys(config.hooks).length > 0 &&
!hookRegistry
) {
debugLogger.warn(
`Subagent "${config.name}" declares hooks but the host has no HookSystem; ignoring per-agent hooks.`,
);
if (config.hooks && Object.keys(config.hooks).length > 0) {
if (hookRegistry) {
const agentScope = `agent:${config.name}:${randomUUID()}`;
unregisterAgentHooks = hookRegistry.addAgentHooks(
config.hooks as { [K in HookEventName]?: HookDefinition[] },
agentScope,
);
} else {
// Single outer guard; nested branch on hookRegistry. The pre-fix
// structure repeated the `config.hooks && Object.keys(...).length`
// predicate across two `if`/`else if` arms, which made it easy to
// drift one side during future edits.
debugLogger.warn(
`Subagent "${config.name}" declares hooks but the host has no HookSystem; ignoring per-agent hooks.`,
);
}
}
try {
const wrappedHooks: AgentHooks | undefined = unregisterAgentHooks
? wrapAgentHooksForCleanup(options?.hooks, unregisterAgentHooks)
: options?.hooks;
return await AgentHeadless.create(
const subagent = await AgentHeadless.create(
config.name,
subagentContext,
promptConfig,
@ -749,11 +790,16 @@ export class SubagentManager {
runConfig,
toolConfig,
options?.eventEmitter,
wrappedHooks,
options?.hooks,
runtimeView,
);
return { subagent, dispose: runCleanup };
} catch (innerError) {
unregisterAgentHooks?.();
// The caller never received the return value — `dispose` cannot
// possibly fire. Run the cleanup ourselves so the registered hook
// entries and the rebuilt ToolRegistry don't leak past this
// constructor failure.
await runCleanup();
throw innerError;
}
} catch (error) {
@ -792,7 +838,17 @@ export class SubagentManager {
private async buildSubagentContextOverride(
runtimeContext: Config,
config: SubagentConfig,
): Promise<Config> {
): Promise<{
context: Config;
/**
* Set only when this call force-rebuilt the registry to land per-agent
* MCP server connections. The freshly built registry owns stdio child
* processes / sockets that the parent's `Config.shutdown` cannot reach,
* so the caller (`createAgentHeadless`) carries this callback through
* to its `dispose` closure and runs it when the subagent terminates.
*/
disposeRegistry?: () => Promise<void>;
}> {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const subagentContext = Object.create(runtimeContext) as any as Config;
@ -862,8 +918,12 @@ export class SubagentManager {
);
}
}
return {
context: subagentContext,
disposeRegistry: () => subagentRegistry.stop(),
};
}
return subagentContext;
return { context: subagentContext };
}
/**
@ -1431,29 +1491,3 @@ function warnInvalidSubagentFile(filePath: string, error: unknown): void {
const message = error instanceof Error ? error.message : String(error);
debugLogger.debug(`Skipped invalid file ${filePath}: ${message}`);
}
/**
* Wrap an optional {@link AgentHooks} so the supplied `cleanup` callback is
* invoked when the agent's `onStop` fires, regardless of whether the caller
* supplied their own `onStop`. The original handler still runs first; the
* cleanup runs in a try/finally so a throwing user handler does not leak
* the ephemeral hook entries the cleanup is responsible for removing.
*/
function wrapAgentHooksForCleanup(
inner: AgentHooks | undefined,
cleanup: () => void,
): AgentHooks {
const innerOnStop = inner?.onStop;
return {
...inner,
onStop: async (payload) => {
try {
if (innerOnStop) {
await innerOnStop(payload);
}
} finally {
cleanup();
}
},
};
}

35
packages/core/src/tools/agent/agent.test.ts
View file

@ -648,9 +648,10 @@ describe('AgentTool', () => {
vi.mocked(mockSubagentManager.loadSubagent).mockResolvedValue(
mockSubagents[0],
);
vi.mocked(mockSubagentManager.createAgentHeadless).mockResolvedValue(
mockAgent,
);
vi.mocked(mockSubagentManager.createAgentHeadless).mockResolvedValue({
subagent: mockAgent,
dispose: vi.fn().mockResolvedValue(undefined),
});
});
it('should execute subagent successfully', async () => {
@ -1437,9 +1438,10 @@ describe('AgentTool', () => {
vi.mocked(mockSubagentManager.loadSubagent).mockResolvedValue(
mockSubagents[0],
);
vi.mocked(mockSubagentManager.createAgentHeadless).mockResolvedValue(
mockAgent,
);
vi.mocked(mockSubagentManager.createAgentHeadless).mockResolvedValue({
subagent: mockAgent,
dispose: vi.fn().mockResolvedValue(undefined),
});
mockHookSystem = {
fireSubagentStartEvent: vi.fn().mockResolvedValue(undefined),
@ -1619,9 +1621,10 @@ describe('AgentTool', () => {
vi.mocked(mockSubagentManager.loadSubagent).mockResolvedValue(
mockSubagents[0],
);
vi.mocked(mockSubagentManager.createAgentHeadless).mockResolvedValue(
mockAgent,
);
vi.mocked(mockSubagentManager.createAgentHeadless).mockResolvedValue({
subagent: mockAgent,
dispose: vi.fn().mockResolvedValue(undefined),
});
mockHookSystem = {
fireSubagentStartEvent: vi.fn().mockResolvedValue(undefined),
@ -1952,9 +1955,10 @@ describe('AgentTool', () => {
emitDuringExecute(capturedInvocation.eventEmitter);
});
vi.mocked(mockSubagentManager.createAgentHeadless).mockResolvedValue(
mockAgent,
);
vi.mocked(mockSubagentManager.createAgentHeadless).mockResolvedValue({
subagent: mockAgent,
dispose: vi.fn().mockResolvedValue(undefined),
});
const params: AgentParams = {
description: 'Edit files',
@ -2265,9 +2269,10 @@ describe('AgentTool', () => {
] = vi.fn();
vi.mocked(mockSubagentManager.loadSubagent).mockResolvedValue(bgSubagent);
vi.mocked(mockSubagentManager.createAgentHeadless).mockResolvedValue(
mockAgent,
);
vi.mocked(mockSubagentManager.createAgentHeadless).mockResolvedValue({
subagent: mockAgent,
dispose: vi.fn().mockResolvedValue(undefined),
});
});
it('should run in background when agent definition has background: true', async () => {

34
packages/core/src/tools/agent/agent.ts
View file

@ -2031,16 +2031,26 @@ class AgentToolInvocation extends BaseToolInvocation<AgentParams, ToolResult> {
let subagent: AgentHeadless;
let taskPrompt: string;
// Per-spawn cleanup the subagent manager returns. The caller MUST
// invoke this in the same `finally` block that wraps `execute()` —
// see SubagentManager.createAgentHeadless's JSDoc for the leak
// scenarios it covers (ephemeral HookRegistry entries, force-rebuilt
// ToolRegistry owning per-agent MCP child processes / sockets).
// Fork subagents share the parent's lifecycle and need no per-spawn
// dispose, so this stays undefined on the fork path.
let subagentDispose: (() => Promise<void>) | undefined;
if (isFork) {
const fork = await this.createForkSubagent(agentConfig);
subagent = fork.subagent;
taskPrompt = fork.taskPrompt;
} else {
subagent = await this.subagentManager.createAgentHeadless(
const result = await this.subagentManager.createAgentHeadless(
subagentConfig,
agentConfig,
{ eventEmitter: this.eventEmitter },
);
subagent = result.subagent;
subagentDispose = result.dispose;
taskPrompt = this.params.prompt;
}
@ -2117,6 +2127,11 @@ class AgentToolInvocation extends BaseToolInvocation<AgentParams, ToolResult> {
let bgTaskPrompt: string;
let bgPromptConfig: PromptConfig | undefined;
let bgToolConfig: ToolConfig | undefined;
// Per-spawn cleanup from `createAgentHeadless` (background path).
// The bg `finally` below invokes this alongside the existing
// parent-registry stop; see the foreground call site for the leak
// scenarios it covers.
let bgSubagentDispose: (() => Promise<void>) | undefined;
if (isFork) {
const fork = await this.createForkSubagent(
bgConfig as Config,
@ -2128,11 +2143,13 @@ class AgentToolInvocation extends BaseToolInvocation<AgentParams, ToolResult> {
bgPromptConfig = fork.promptConfig;
bgToolConfig = fork.toolConfig;
} else {
bgSubagent = await this.subagentManager.createAgentHeadless(
const bgResult = await this.subagentManager.createAgentHeadless(
subagentConfig,
bgConfig as Config,
{ eventEmitter: bgEventEmitter },
);
bgSubagent = bgResult.subagent;
bgSubagentDispose = bgResult.dispose;
bgTaskPrompt = this.params.prompt;
}
@ -2486,6 +2503,12 @@ class AgentToolInvocation extends BaseToolInvocation<AgentParams, ToolResult> {
.getToolRegistry()
.stop()
.catch(() => {});
// Per-spawn cleanup from `SubagentManager.createAgentHeadless`
// (background path). Mirrors the foreground finally: releases
// agent-scope hook entries and stops the per-agent ToolRegistry
// owning MCP child processes; not redundant with the parent
// registry stop above.
void bgSubagentDispose?.().catch(() => {});
// Restore parent PermissionManager's dangerous allow rules
// if this AUTO override stripped them. Background path:
// restore fires when the bg agent terminates (complete /
@ -2891,6 +2914,13 @@ class AgentToolInvocation extends BaseToolInvocation<AgentParams, ToolResult> {
.getToolRegistry()
.stop()
.catch(() => {});
// Per-spawn cleanup from `SubagentManager.createAgentHeadless`:
// releases the agent-scope hook entries registered for this
// invocation and stops the per-agent ToolRegistry that the force
// rebuild created to land `mcpServers` discovery. The parent
// `getToolRegistry().stop()` above only reaches the parent's
// registry — the per-agent one is distinct.
void subagentDispose?.().catch(() => {});
// Restore parent PermissionManager's dangerous allow rules if
// this AUTO override stripped them on creation. No-op for non-
// AUTO overrides and for AUTO overrides when parent was already