Skip to content

Commit d185ec0

Browse files
TheGreatAxiosTheGreatAxios
authored
Dispose exec children once on every TUI exit path (#846)
* Kill live shell-guard children on plugin dispose * Dispose the toolset inside once-only runtime shutdown * Await runtime shutdown on crash and signal paths * Reap exec children in shutdown integration tests * Format shell-guard dispose tests * Abort TUI quit before awaiting the session-op tail Stop workers first so a hung session-op cannot delay abort and reap. Log shutdown failures at error while still mapping teardown failure to exit 1. * Fail teardown when shell children survive reap A leftover after the two-second backstop must reject dispose so the exit 1 path can fire. Abort already SIGKILLs the process group at abort start. * Surface subagent posix dispose failures * Fail persist close_agent when child dispose throws A leftover child after posix reap must fail persist close and parent toolset dispose instead of looking like a successful shutdown. A wedged close still times out as shutdown. * Fail parent dispose when persist workers leave children * Fail leftover exec dispose without skipping sibling teardown * Reap posix children before waiting on agent close A hung agent.close used to run before process-group reap, so teardown could report success while detached run_shell children were still live. Dispose first, fail a close deadline instead of succeeding, and clear the two-second host timer when dispose wins. * Surface leftover dispose when agent close hangs * Refuse queued run_shell after shell-guard dispose Latch dispose so overlapping calls join one reap, and refuse queued shells that would spawn after the guard is gone.
1 parent c701aff commit d185ec0

34 files changed

Lines changed: 1843 additions & 187 deletions

‎CHANGELOG.md‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,14 @@ parallel copies under `docs/` or `scripts/notes/`. At cut time: rename
4848
owns idle rebuild; delivery generation owns session identity, so interrupt,
4949
/clear, and /new abort the outstanding overlay, skip minting a grant, and
5050
notify the operator instead of delivering into a rebuilt agent.
51+
- TUI quit, crash, and process signals await once-only runtime shutdown so live
52+
shell-guard children are reaped. Teardown failure after a completed session
53+
exits 1; SIGINT, SIGTERM, and SIGHUP still exit 128+n.
54+
- Persist close_agent surfaces leftover-child dispose failure so a worker
55+
that survives reap is not reported as a successful shutdown.
56+
- Leftover exec dispose is reported as a failed run (stderr + status failed), and
57+
parent toolset dispose finishes remaining workers and posix teardown before
58+
surfacing leftover-child failure.
5159

5260
### Changed
5361

‎docs/ARCHITECTURE.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -369,7 +369,7 @@ tool call
369369
- **Secret Guard** (`secret-guard-plugin.ts`) — Hard-denies path-keyed tool calls (`read_file`, `write_file`, …) that would put a sensitive file into (or write it from) the model context. Runs before the permission plugin, so the path-arg deny holds even under `--dangerously-skip-permissions`. Shell commands that _reference_ a sensitive path (tokenized so `cat .env`, `bun --env-file=.env run …`, and quote/env-assignment forms are detected) are not hard-denied here: they require operator approval via the permission gate, and auto mode forces an ask through the auto-shell policy (`sensitive-path` rule). Once the operator approves, the command runs. Shell detection is best-effort: token matching defeats quoting and env-assignment/redirection forms but not dynamic path construction (variable indirection, `printf` assembly). Tool-result secret scrub still redacts credential-shaped output.
370370
- **Authorization** (`run-shell-authz.ts`, wired by `authz-plugin.ts`) — Denies catastrophic shell command patterns by regex, and hard-blocks shell `find`, head-position `rg`, and recursive `grep -r` (they can walk huge trees and OOM the host). Bounded `grep`/`search_files` tools remain practical alternatives (timeout + output caps); the patterns match those three command shapes only — an `ls -R`, `fd`, or scripted `os.walk` is just as unbounded and is not caught, so the block message tells the model not to substitute one. The permission gate’s shell auto-allow path consults the same policy so it never pre-approves a command authz would reject.
371371
- **Permission** (`permission-plugin.ts`) — Delegates consequential calls to the permission gate.
372-
- **Shell Guard** (`shell-guard-plugin.ts`) — Corbits Code-only replacement for stock `run_shell` (interchange stays unpatched): no built-in default timeout (optional per-call or `settings.shell.timeoutMs`; `maxTimeoutMs` clamps only a resolved timeout), 512KB display cap with head+tail retention (the process keeps running when the cap is hit), process-group kill on timeout/abort only, and `background: true` — the call returns a `shell_id` at once (registry in `src/shell/background-shell.ts`), the process group keeps running past the turn, completion is delivered on a later turn via `buildShellBackgroundMessage`, and `shell_collect` retrieves or cancels (schema advertised by `advertiseShellGuardTimeout`; evaluated by the permission chain at start time like any shell call). Also applies a 10s wall-clock budget to `grep`/`search_files`.
372+
- **Shell Guard** (`shell-guard-plugin.ts`) — Corbits Code-only replacement for stock `run_shell` (interchange stays unpatched): no built-in default timeout (optional per-call or `settings.shell.timeoutMs`; `maxTimeoutMs` clamps only a resolved timeout), 512KB display cap with head+tail retention (the process keeps running when the cap is hit), process-group kill on timeout, abort, and plugin dispose (live children tracked in the plugin and reaped by `posixTools.dispose`), and `background: true` — the call returns a `shell_id` at once (registry in `src/shell/background-shell.ts`), the process group keeps running past the turn, completion is delivered on a later turn via `buildShellBackgroundMessage`, and `shell_collect` retrieves or cancels (schema advertised by `advertiseShellGuardTimeout`; evaluated by the permission chain at start time like any shell call). Also applies a 10s wall-clock budget to `grep`/`search_files`. Ripgrep detached spawns are not tracked.
373373
- **Read File Guard** (`read-file-guard-plugin.ts`) — Corbits Code-only short-circuit for `read_file` on real filesystem paths and configured `tool-output://` URIs (interchange stays unpatched): streaming reads that never decode the whole file in one pass, caps model-facing output at 50KB, defaults to 2000 lines, truncates long lines with recovery hints, samples the first chunk to reject binary, and stops at an 8MB scan ceiling. Emits `offset` continuation notices so the model can page without losing file or spill content on disk.
374374
- **Verify** (`verify-plugin.ts`) — Re-reads after `write_file` / `edit_file` and errors on mismatch. Per-path serialization (`file-mutation-lock.ts`) prevents parallel edits on one file from tripping verification.
375375
- **Edit file line range** (`edit-file-line-range-plugin.ts`) — Corbits Code-only short-circuit for `edit_file` mode B (`start_line`/`end_line`/`new_string`), same pattern as shell-guard; schema advertised via `advertiseEditFileLineRange`. Modes are mutually exclusive: a call supplying both `old_string` and `start_line`/`end_line` is rejected with a recoverable error naming which fields to omit (no file-content disambiguation).

‎src/agent/fleet-verbs-mount.test.ts‎

Lines changed: 158 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,164 @@ describe("primary fleet verb mount", () => {
5353
await toolset.dispose();
5454
});
5555

56+
test("createAgentToolset dispose rejects when a fleet closeOne throws leftover children", async () => {
57+
const cwd = mkdtempSync(join(tmpdir(), "corbits-fleet-mount-"));
58+
const { createAgentToolset } = await import("./tools.js");
59+
const permissionGate = {
60+
check: async () => ({ allowed: true }),
61+
getSkipPermissions: () => false,
62+
} as never;
63+
const sessions = createSubAgentSessionStore();
64+
const worker = sessions.start({ description: "d", agentId: "a", brief: "b" });
65+
sessions.markRunning(worker.id);
66+
sessions.registerClose(worker.id, async () => {
67+
throw new Error("1 shell child process still live after 2000ms reap");
68+
});
69+
70+
const toolset = await createAgentToolset({
71+
cwd,
72+
permissionGate,
73+
onOperatorGate: async () => ({ kind: "option", index: 0 }),
74+
subAgent: {
75+
provider: {
76+
providerName: "test",
77+
baseURL: "http://127.0.0.1:0",
78+
model: "test-model",
79+
},
80+
getWorkdirBase: () => cwd,
81+
sessions,
82+
},
83+
});
84+
85+
await expect(toolset.dispose()).rejects.toThrow(/still live after 2000ms reap/);
86+
});
87+
88+
test("createAgentToolset dispose rejects when a retained completed persist worker leaves children", async () => {
89+
const cwd = mkdtempSync(join(tmpdir(), "corbits-fleet-mount-"));
90+
const { createAgentToolset } = await import("./tools.js");
91+
const permissionGate = {
92+
check: async () => ({ allowed: true }),
93+
getSkipPermissions: () => false,
94+
} as never;
95+
const sessions = createSubAgentSessionStore();
96+
const worker = sessions.start({
97+
description: "d",
98+
agentId: "a",
99+
brief: "b",
100+
retained: true,
101+
});
102+
sessions.registerClose(worker.id, async () => {
103+
throw new Error("1 shell child process still live after 2000ms reap");
104+
});
105+
sessions.complete(worker.id, "done", { agentRetained: true });
106+
107+
const toolset = await createAgentToolset({
108+
cwd,
109+
permissionGate,
110+
onOperatorGate: async () => ({ kind: "option", index: 0 }),
111+
subAgent: {
112+
provider: {
113+
providerName: "test",
114+
baseURL: "http://127.0.0.1:0",
115+
model: "test-model",
116+
},
117+
getWorkdirBase: () => cwd,
118+
sessions,
119+
},
120+
});
121+
122+
await expect(toolset.dispose()).rejects.toThrow(/still live after 2000ms reap/);
123+
});
124+
125+
test("createAgentToolset dispose closes remaining retained workers after the first leftover", async () => {
126+
const cwd = mkdtempSync(join(tmpdir(), "corbits-fleet-mount-"));
127+
const { createAgentToolset } = await import("./tools.js");
128+
const permissionGate = {
129+
check: async () => ({ allowed: true }),
130+
getSkipPermissions: () => false,
131+
} as never;
132+
const sessions = createSubAgentSessionStore();
133+
const first = sessions.start({
134+
description: "d1",
135+
agentId: "a",
136+
brief: "b",
137+
retained: true,
138+
});
139+
const second = sessions.start({
140+
description: "d2",
141+
agentId: "a",
142+
brief: "b",
143+
retained: true,
144+
});
145+
let firstCloseCalls = 0;
146+
let secondCloseCalls = 0;
147+
sessions.registerClose(first.id, async () => {
148+
firstCloseCalls += 1;
149+
throw new Error("1 shell child process still live after 2000ms reap");
150+
});
151+
sessions.registerClose(second.id, async () => {
152+
secondCloseCalls += 1;
153+
});
154+
sessions.complete(first.id, "done", { agentRetained: true });
155+
sessions.complete(second.id, "done", { agentRetained: true });
156+
157+
const toolset = await createAgentToolset({
158+
cwd,
159+
permissionGate,
160+
onOperatorGate: async () => ({ kind: "option", index: 0 }),
161+
subAgent: {
162+
provider: {
163+
providerName: "test",
164+
baseURL: "http://127.0.0.1:0",
165+
model: "test-model",
166+
},
167+
getWorkdirBase: () => cwd,
168+
sessions,
169+
},
170+
});
171+
172+
await expect(toolset.dispose()).rejects.toThrow(/still live after 2000ms reap/);
173+
expect(firstCloseCalls).toBe(1);
174+
expect(secondCloseCalls).toBe(1);
175+
});
176+
177+
test("createAgentToolset dispose rejects when a retained running persist worker leaves children", async () => {
178+
const cwd = mkdtempSync(join(tmpdir(), "corbits-fleet-mount-"));
179+
const { createAgentToolset } = await import("./tools.js");
180+
const permissionGate = {
181+
check: async () => ({ allowed: true }),
182+
getSkipPermissions: () => false,
183+
} as never;
184+
const sessions = createSubAgentSessionStore();
185+
const worker = sessions.start({
186+
description: "d",
187+
agentId: "a",
188+
brief: "b",
189+
retained: true,
190+
});
191+
sessions.markRunning(worker.id);
192+
sessions.registerClose(worker.id, async () => {
193+
throw new Error("1 shell child process still live after 2000ms reap");
194+
});
195+
196+
const toolset = await createAgentToolset({
197+
cwd,
198+
permissionGate,
199+
onOperatorGate: async () => ({ kind: "option", index: 0 }),
200+
subAgent: {
201+
provider: {
202+
providerName: "test",
203+
baseURL: "http://127.0.0.1:0",
204+
model: "test-model",
205+
},
206+
getWorkdirBase: () => cwd,
207+
sessions,
208+
},
209+
});
210+
211+
await expect(toolset.dispose()).rejects.toThrow(/still live after 2000ms reap/);
212+
});
213+
56214
test("createAgentToolset omits fleet verbs when subAgent is not set", async () => {
57215
const cwd = mkdtempSync(join(tmpdir(), "corbits-fleet-mount-"));
58216
const { createAgentToolset } = await import("./tools.js");

‎src/agent/tools.ts‎

Lines changed: 27 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,13 @@ export const ASK_OPERATOR_OPTION_MAX_CHARS = 48;
105105
/** Cap on the ask_operator question (UTF-16 code units). */
106106
export const ASK_OPERATOR_QUESTION_MAX_CHARS = 160;
107107

108+
function rethrowToolsetDisposeFailures(failures: unknown[]): void {
109+
const first = failures[0];
110+
if (first === undefined) return;
111+
if (failures.length === 1) throw first;
112+
throw new AggregateError(failures, "toolset leftover dispose failed");
113+
}
114+
108115
const SubmitOutputArgs = type({
109116
"summary?": "string",
110117
"step?": "string",
@@ -982,11 +989,28 @@ export async function createAgentToolset(args: AgentToolsetArgs): Promise<AgentT
982989
disposed = true;
983990
mcpAbortController.abort(new Error("MCP toolset disposed"));
984991
disposal = (async () => {
992+
const failures: unknown[] = [];
993+
// Kill every live background process group before the posix teardown so
994+
// /clear, interrupt, and reload cannot leave orphans behind.
995+
backgroundShells.disposeAll("session closed");
996+
try {
997+
await posixTools.dispose();
998+
} catch (err: unknown) {
999+
failures.push(err);
1000+
}
9851001
const fleetSessions = fleetSessionsForDispose;
9861002
if (fleetSessions !== undefined) {
987-
fleetSessions.cancelAll("parent session closed");
1003+
try {
1004+
await fleetSessions.cancelAll("parent session closed");
1005+
} catch (err: unknown) {
1006+
failures.push(err);
1007+
}
9881008
for (const session of [...fleetSessions.list()].reverse()) {
989-
await fleetSessions.closeOne(session.id, DEFAULT_CLOSE_DEADLINE_MS);
1009+
try {
1010+
await fleetSessions.closeOne(session.id, DEFAULT_CLOSE_DEADLINE_MS);
1011+
} catch (err: unknown) {
1012+
failures.push(err);
1013+
}
9901014
}
9911015
}
9921016
await Promise.allSettled([...inFlightConnections.values()]);
@@ -997,11 +1021,8 @@ export async function createAgentToolset(args: AgentToolsetArgs): Promise<AgentT
9971021
[...connectedClients.values()].map((client) => client.close().catch(() => undefined)),
9981022
);
9991023
connectedClients.clear();
1000-
// Kill every live background process group before the posix teardown so
1001-
// /clear, interrupt, and reload cannot leave orphans behind.
1002-
backgroundShells.disposeAll("session closed");
1003-
await posixTools.dispose();
10041024
await disposeWebSearchClients();
1025+
rethrowToolsetDisposeFailures(failures);
10051026
})();
10061027
return disposal;
10071028
};

0 commit comments

Comments
 (0)