fix(worker): marked-command survives tail-exec; kill script excludes itself
This commit is contained in:
Gabriel Brown
@@ -393,28 +393,75 @@ export const ensureUserContainer = async (args: {
|
||||
|
||||
const shellQuote = (value: string) => `'${value.replaceAll("'", "'\\''")}'`;
|
||||
|
||||
// The marker is embedded literally into a `# comment` line and into a `pgrep`
|
||||
// pattern, so it must not contain shell metacharacters or a newline (either could
|
||||
// break out of the comment or the pattern). Callers generate it, but validate
|
||||
// defensively so a malformed marker fails loudly instead of silently corrupting
|
||||
// the script.
|
||||
const MARKER_PATTERN = /^[A-Za-z0-9_-]+$/;
|
||||
const assertMarker = (marker: string) => {
|
||||
if (!MARKER_PATTERN.test(marker)) {
|
||||
throw new Error(
|
||||
`Invalid process marker ${JSON.stringify(marker)}: must match ${String(MARKER_PATTERN)}.`,
|
||||
);
|
||||
}
|
||||
};
|
||||
|
||||
// Wraps a CLI argv so the process runs as a new session/process-group leader whose
|
||||
// bash parent carries the marker in its argv (matchable by `pgrep -f`). We do NOT
|
||||
// `exec` (bash must survive so the marker stays visible); stdout/stderr fds are
|
||||
// inherited by the CLI so streaming still works.
|
||||
// `exec` the CLI: bash tail-exec's a final simple command, which would replace the
|
||||
// marker-carrying bash with the CLI and make `pgrep -f <marker>` miss the running
|
||||
// turn. Keeping a trailing `rc=$?; exit "$rc"` after the CLI means the CLI is no
|
||||
// longer the script's final statement, so bash survives as the group leader while
|
||||
// still propagating the CLI's exit code (downstream `normalizeRunResult` relies on
|
||||
// it — a bare trailing `wait` would mask a non-zero CLI failure as 0). The CLI runs
|
||||
// in the same process group as this bash, so the kill helper's group signal reaches
|
||||
// it. stdout/stderr fds are inherited by the CLI so streaming still works.
|
||||
export const buildMarkedCommand = (
|
||||
marker: string,
|
||||
command: string[],
|
||||
): string[] => {
|
||||
const script = `# ${marker}\nexec 0</dev/null\n${command.map(shellQuote).join(' ')}`;
|
||||
assertMarker(marker);
|
||||
const script = [
|
||||
`# ${marker}`,
|
||||
'exec 0</dev/null',
|
||||
command.map(shellQuote).join(' '),
|
||||
'rc=$?',
|
||||
'exit "$rc"',
|
||||
].join('\n');
|
||||
return ['setsid', 'bash', '-lc', script];
|
||||
};
|
||||
|
||||
// Kills every process group whose bash parent matches the marker, TERM then KILL.
|
||||
// The negative pid (`-"$pid"`) targets the whole process group, so the CLI and any
|
||||
// children it spawned die together.
|
||||
export const buildKillScript = (marker: string): string =>
|
||||
[
|
||||
// The negative pid (`kill -TERM -"$pgid"`) targets the whole process group, so the
|
||||
// CLI and any children it spawned die together.
|
||||
//
|
||||
// pgrep self-match: this kill script runs via `bash -lc <script>`, whose OWN argv
|
||||
// contains the marker literal, so `pgrep -f <marker>` matches the kill script (and
|
||||
// its command-substitution subshells) too. Left unguarded it would `kill -TERM` its
|
||||
// own process group and die on iteration one, leaking the real target. So we compute
|
||||
// this script's pgid up front and skip any match sharing it, killing only the
|
||||
// target's process group.
|
||||
export const buildKillScript = (marker: string): string => {
|
||||
assertMarker(marker);
|
||||
return [
|
||||
`self_pgid=$(ps -o pgid= -p $$ | tr -d ' ')`,
|
||||
`pids=$(pgrep -f ${shellQuote(marker)} || true)`,
|
||||
`for pid in $pids; do kill -TERM -"$pid" 2>/dev/null || true; done`,
|
||||
`for pid in $pids; do`,
|
||||
` pgid=$(ps -o pgid= -p "$pid" | tr -d ' ')`,
|
||||
` [ -z "$pgid" ] && continue`,
|
||||
` [ "$pgid" = "$self_pgid" ] && continue`,
|
||||
` kill -TERM -"$pgid" 2>/dev/null || true`,
|
||||
`done`,
|
||||
`sleep 2`,
|
||||
`for pid in $pids; do kill -KILL -"$pid" 2>/dev/null || true; done`,
|
||||
`for pid in $pids; do`,
|
||||
` pgid=$(ps -o pgid= -p "$pid" | tr -d ' ')`,
|
||||
` [ -z "$pgid" ] && continue`,
|
||||
` [ "$pgid" = "$self_pgid" ] && continue`,
|
||||
` kill -KILL -"$pgid" 2>/dev/null || true`,
|
||||
`done`,
|
||||
].join('\n');
|
||||
};
|
||||
|
||||
export const killBoxProcessesByMarker = async (args: {
|
||||
containerName: string;
|
||||
|
||||
@@ -22,14 +22,54 @@ describe('in-box process-group marker + kill helpers', () => {
|
||||
expect(argv[2]).toBe('-lc');
|
||||
const script = argv[3] ?? '';
|
||||
expect(script).toContain('# spoon-turn-abc');
|
||||
expect(script.split('\n').at(-1)).toBe("'codex' 'exec' '--json' 'hi'");
|
||||
expect(script).toContain("'codex' 'exec' '--json' 'hi'");
|
||||
});
|
||||
|
||||
test('buildMarkedCommand keeps a trailing exit after the CLI so bash survives tail-exec while propagating the exit code (C1 guard)', async () => {
|
||||
const { buildMarkedCommand } = await loadDocker();
|
||||
const script = buildMarkedCommand('spoon-turn-abc', ['codex', 'run'])[3] ?? '';
|
||||
// The CLI must NOT be the final simple command (bash would tail-exec it,
|
||||
// replacing the marker-carrying bash). The exit code must still propagate.
|
||||
expect(script).toContain("'codex' 'run'\nrc=$?\nexit \"$rc\"");
|
||||
expect(script.split('\n').at(-1)).toBe('exit "$rc"');
|
||||
expect(script.split('\n').at(-1)).not.toBe("'codex' 'run'");
|
||||
});
|
||||
|
||||
test('buildMarkedCommand rejects a marker that could break out of the comment', async () => {
|
||||
const { buildMarkedCommand } = await loadDocker();
|
||||
expect(() => buildMarkedCommand('bad\nmarker', ['codex'])).toThrow();
|
||||
});
|
||||
|
||||
test('buildKillScript TERM-then-KILLs every group matching the marker', async () => {
|
||||
const { buildKillScript } = await loadDocker();
|
||||
const script = buildKillScript('spoon-turn-abc');
|
||||
expect(script).toContain("pgrep -f 'spoon-turn-abc'");
|
||||
expect(script).toContain('kill -TERM -"$pid"');
|
||||
expect(script).toContain('kill -KILL -"$pid"');
|
||||
expect(script).toContain('kill -TERM -"$pgid"');
|
||||
expect(script).toContain('kill -KILL -"$pgid"');
|
||||
});
|
||||
|
||||
test('buildKillScript excludes its own process group so it does not kill itself (C2 guard)', async () => {
|
||||
const { buildKillScript } = await loadDocker();
|
||||
const script = buildKillScript('spoon-turn-abc');
|
||||
// Computes its own pgid and skips any match sharing it (the kill script's
|
||||
// own bash + its command-substitution subshells all carry the marker).
|
||||
expect(script).toContain('self_pgid=$(ps -o pgid= -p $$ | tr -d \' \')');
|
||||
expect(script).toContain('[ "$pgid" = "$self_pgid" ] && continue');
|
||||
});
|
||||
|
||||
test('buildKillScript escalates TERM -> KILL with a sleep 2 in between (order matters)', async () => {
|
||||
const { buildKillScript } = await loadDocker();
|
||||
const script = buildKillScript('spoon-turn-abc');
|
||||
const termIdx = script.indexOf('kill -TERM -"$pgid"');
|
||||
const sleepIdx = script.indexOf('\nsleep 2\n');
|
||||
const killIdx = script.indexOf('kill -KILL -"$pgid"');
|
||||
expect(termIdx).toBeGreaterThanOrEqual(0);
|
||||
expect(sleepIdx).toBeGreaterThan(termIdx);
|
||||
expect(killIdx).toBeGreaterThan(sleepIdx);
|
||||
});
|
||||
|
||||
test('buildKillScript rejects a marker that could break out of the pgrep pattern', async () => {
|
||||
const { buildKillScript } = await loadDocker();
|
||||
expect(() => buildKillScript('bad\nmarker')).toThrow();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -131,23 +131,50 @@ export const getAdapter = (name: AgentRuntimeName): AgentRuntime => {
|
||||
```ts
|
||||
// Wraps a CLI argv so the process runs as a new session/process-group leader whose
|
||||
// bash parent carries the marker in its argv (matchable by `pgrep -f`). We do NOT
|
||||
// `exec` (bash must survive so the marker stays visible); stdout/stderr fds are
|
||||
// inherited by the CLI so streaming still works.
|
||||
// `exec` the CLI: bash tail-exec's a final simple command, so an `exec`-or-trailing
|
||||
// CLI would replace the marker-carrying bash and `pgrep -f <marker>` would miss the
|
||||
// running turn. Keep a trailing `rc=$?; exit "$rc"` so the CLI is no longer the final
|
||||
// statement (bash survives as group leader) while still propagating its exit code
|
||||
// (downstream `normalizeRunResult` relies on it; a bare `wait` would mask failures).
|
||||
export const buildMarkedCommand = (marker: string, command: string[]): string[] => {
|
||||
const script = `# ${marker}\nexec 0</dev/null\n${command.map(shellQuote).join(' ')}`;
|
||||
// assertMarker(marker) — reject anything but /^[A-Za-z0-9_-]+$/ so a newline can't
|
||||
// break out of the `# comment` line.
|
||||
const script = [
|
||||
`# ${marker}`,
|
||||
'exec 0</dev/null',
|
||||
command.map(shellQuote).join(' '),
|
||||
'rc=$?',
|
||||
'exit "$rc"',
|
||||
].join('\n');
|
||||
return ['setsid', 'bash', '-lc', script];
|
||||
};
|
||||
|
||||
// Kills every process group whose bash parent matches the marker, TERM then KILL.
|
||||
// pgrep self-match: the kill script runs via `bash -lc <script>` whose own argv holds
|
||||
// the marker literal, so pgrep matches the kill script (and its command-substitution
|
||||
// subshells) too. Exclude its own pgid or it SIGTERMs itself on iteration one and
|
||||
// leaks the target; kill the TARGET's pgid instead.
|
||||
export const killBoxProcessesByMarker = async (args: {
|
||||
containerName: string;
|
||||
marker: string;
|
||||
}): Promise<void> => {
|
||||
// assertMarker(args.marker) — same /^[A-Za-z0-9_-]+$/ guard for the pgrep pattern.
|
||||
const script = [
|
||||
`self_pgid=$(ps -o pgid= -p $$ | tr -d ' ')`,
|
||||
`pids=$(pgrep -f ${shellQuote(args.marker)} || true)`,
|
||||
`for pid in $pids; do kill -TERM -"$pid" 2>/dev/null || true; done`,
|
||||
`for pid in $pids; do`,
|
||||
` pgid=$(ps -o pgid= -p "$pid" | tr -d ' ')`,
|
||||
` [ -z "$pgid" ] && continue`,
|
||||
` [ "$pgid" = "$self_pgid" ] && continue`,
|
||||
` kill -TERM -"$pgid" 2>/dev/null || true`,
|
||||
`done`,
|
||||
`sleep 2`,
|
||||
`for pid in $pids; do kill -KILL -"$pid" 2>/dev/null || true; done`,
|
||||
`for pid in $pids; do`,
|
||||
` pgid=$(ps -o pgid= -p "$pid" | tr -d ' ')`,
|
||||
` [ -z "$pgid" ] && continue`,
|
||||
` [ "$pgid" = "$self_pgid" ] && continue`,
|
||||
` kill -KILL -"$pgid" 2>/dev/null || true`,
|
||||
`done`,
|
||||
].join('\n');
|
||||
await execa(containerRuntime(), ['exec', args.containerName, 'bash', '-lc', script], {
|
||||
reject: false, stdin: 'ignore',
|
||||
@@ -159,7 +186,7 @@ Add a local `const shellQuote = (v: string) => `'${v.replaceAll("'", "'\\''")}'`
|
||||
*Consumes:* `execa`, `containerRuntime()` (already in file).
|
||||
|
||||
**Steps:**
|
||||
- [ ] Write failing `apps/agent-worker/tests/unit/box-process-kill.test.ts`: import `buildMarkedCommand`. Assert `buildMarkedCommand('spoon-turn-abc', ['codex', 'exec', '--json', 'hi'])` returns `['setsid', 'bash', '-lc', expect.stringContaining('# spoon-turn-abc')]` and the script's last line contains `codex exec --json 'hi'`. (Do not unit-test `killBoxProcessesByMarker` end-to-end — it shells out; assert only the argv via a spy if you refactor it to a pure `buildKillScript(marker)` helper. **Recommended:** extract `export const buildKillScript = (marker: string): string` and test it contains `pgrep -f 'spoon-turn-abc'`, `kill -TERM -"$pid"`, and `kill -KILL -"$pid"`.)
|
||||
- [ ] Write failing `apps/agent-worker/tests/unit/box-process-kill.test.ts`: import `buildMarkedCommand`. Assert `buildMarkedCommand('spoon-turn-abc', ['codex', 'exec', '--json', 'hi'])` returns `['setsid', 'bash', '-lc', expect.stringContaining('# spoon-turn-abc')]` and the script's last line contains `codex exec --json 'hi'`. (Do not unit-test `killBoxProcessesByMarker` end-to-end — it shells out; assert only the argv via a spy if you refactor it to a pure `buildKillScript(marker)` helper. **Recommended:** extract `export const buildKillScript = (marker: string): string` and test it contains `pgrep -f 'spoon-turn-abc'`, `kill -TERM -"$pgid"`, and `kill -KILL -"$pgid"`, plus the self-pgid exclusion (`[ "$pgid" = "$self_pgid" ] && continue`).)
|
||||
- [ ] Run `bun run test:unit` → FAIL.
|
||||
- [ ] Implement `buildMarkedCommand`, `buildKillScript`, and `killBoxProcessesByMarker` in `docker.ts`.
|
||||
- [ ] Run `bun run test:unit` → PASS. `bun run typecheck` → PASS.
|
||||
|
||||
Reference in New Issue
Block a user