fix(process): skip kill-tree group kill when child wasn't detached (#71662) by hclsys · Pull Request #71681 · openclaw/openclaw

hclsys · 2026-04-25T17:20:52Z

Bug

When the supervisor spawns a child with `detached: false` (service-managed runtime under launchd/systemd), the child shares the gateway's process group. On session abort or SIGKILL, `killProcessTree` unconditionally issued `process.kill(-pid, 'SIGTERM')` — which targets the entire process group (negative pid is POSIX group-kill semantics) and therefore SIGTERMs the gateway parent along with the child.

Reporter saw this on macOS LaunchAgent + KeepAlive=true: aborting a `claude-cli/claude-opus-4-7` session caused the gateway to receive SIGTERM, auto-restart, dropping all in-flight sessions. Switching the primary model to a non-cli provider eliminated it because non-cli paths don't go through this kill-tree call. Did NOT occur on Linux VPS where the gateway runs detached, because there `useDetached === true` and the child gets its own process group.

The trigger is exactly:

`src/process/supervisor/adapters/child.ts:45` — `useDetached = !isServiceManagedRuntime()` → false on launchd/systemd
`src/process/supervisor/adapters/child.ts:290` — `killProcessTree(pid)` called on session abort
`src/process/kill-tree.ts:49` — `process.kill(-pid, 'SIGTERM')` group-kill

Fix

`killProcessTree` now accepts an optional `opts.detached?: boolean`. When `detached: false`, `killProcessTreeUnix` skips the `-pid` group-kill and goes straight to direct-pid SIGTERM/SIGKILL. Default (`detached: true`) is preserved so all existing callers behave exactly as before.

`supervisor/adapters/child.ts:286` now threads the spawn-time `useDetached` flag into `killProcessTree`, so the kill path matches the spawn-time detachment decision.

Tests

new: `detached: false` skips group kill and uses direct pid SIGTERM only.
new: default behaviour (`detached: true`) still uses group kill (regression guard so the existing test case isn't accidentally weakened).
existing 4 tests unchanged. 6/6 pass locally.

Lint clean: `pnpm oxlint` — 0 warnings, 0 errors.

Out of scope

Other `killProcessTree` callers (mcp-stdio-transport, bash-tools.process, daemon/schtasks, etc.) keep the default group-kill behaviour because those processes are typically detached from the gateway. Only the `supervisor/adapters/child.ts` path threads `detached` through, since it's the path that knows whether the child was actually spawned detached. If other call sites need the same fix, they can adopt `opts.detached` incrementally.

🤖 generated with assistance from Claude Code
Co-authored-by: HCL [email protected]

…penclaw#71662) When the supervisor spawns a child with detached:false (service-managed runtime under launchd/systemd), the child shares the gateway's process group. On session abort or SIGKILL, killProcessTree was unconditionally issuing process.kill(-pid, 'SIGTERM') — which targets the entire process GROUP (negative pid is POSIX group-kill semantics) and therefore SIGTERMs the gateway parent along with the child. Reporter saw this on macOS (LaunchAgent + KeepAlive=true): aborting a claude-cli/claude-opus-4-7 session caused the gateway to receive SIGTERM, then auto-restart, dropping all in-flight sessions. Switching the primary model to a non-cli provider eliminated it because the non-cli paths don't go through this kill-tree call. Did not occur on Linux VPS where the gateway runs detached, because there useDetached === true and the child got its own process group. Fix: - killProcessTree now accepts opts.detached?: boolean. When detached:false, killProcessTreeUnix skips the `-pid` group-kill and goes straight to direct-pid SIGTERM/SIGKILL. Group-kill default (detached:true) is preserved so all existing callers behave exactly as before. - supervisor/adapters/child.ts:286 now threads the spawn-time `useDetached` flag into killProcessTree, so the kill-tree path matches the spawn-time detachment decision (line 45 of the same file already computes useDetached = process.platform !== 'win32' && !isServiceManagedRuntime()). Tests: - new: detached:false skips group kill and uses direct pid SIGTERM only. - new: default behaviour (detached:true) still uses group kill (regression guard so the existing test case isn't accidentally weakened). Existing tests still pass (6/6 in kill-tree.test.ts). Lint clean. Out of scope: other killProcessTree callers (mcp-stdio-transport, bash-tools.process, etc.) keep the default group-kill behaviour because those processes are typically detached from the gateway. Only the supervisor/adapters/child.ts path threads `detached` through, since it's the path that knows whether the child was actually spawned detached.

aisle-research-bot · 2026-04-25T17:20:59Z

🔒 Aisle Security Analysis

We found 1 potential security issue(s) in this PR:

#	Severity	Title
1	🟠 High	Default Unix group-kill in killProcessTree may terminate gateway/other processes when child not detached

1. 🟠 Default Unix group-kill in killProcessTree may terminate gateway/other processes when child not detached

Property	Value
Severity	High
CWE	CWE-400
Location	`src/process/kill-tree.ts:20-36`

Description

killProcessTree now defaults to Unix process-group termination when opts.detached is omitted (useGroupKill = opts?.detached !== false). This causes process.kill(-pid, ...) to be used for SIGTERM/SIGKILL.

If the target process is not actually in its own process group (i.e., was spawned with detached:false, or detachment failed/fell back, or a library spawn doesn’t create a new process group), then -pid can resolve to the caller’s own process group. This can lead to:

Self-termination of the gateway/service (SIGTERM/SIGKILL delivered to its process group)
Termination of unrelated sibling processes in the same group (availability impact)
Potential cross-tenant/process impact when running as a service account (privilege/containment boundary concern)

This is particularly risky because several call sites invoke killProcessTree(pid) without supplying detached:false, so they inherit the new default group-kill behavior.

Vulnerable code (new default):

killProcessTreeUnix(pid, graceMs, opts?.detached !== false);

Recommendation

Make process-group kill opt-in rather than opt-out, and require callers to explicitly assert detachment when safe.

Suggested change:

// default to direct-pid kill unless caller explicitly confirms detachment
killProcessTreeUnix(pid, graceMs, opts?.detached === true);

Additionally:

Audit/adjust all killProcessTree(pid) call sites to pass { detached: true } only when the spawned child is guaranteed to be in its own process group (e.g., successfully spawned with detached:true and no fallback occurred).
For library-spawned processes (e.g., PTYs) where process-group semantics are uncertain, pass { detached: false }.

Analyzed PR: #71681 at commit 9e91299

_{Last updated on: 2026-04-25T19:19:46Z}

greptile-apps · 2026-04-25T17:23:05Z

Greptile Summary

This PR fixes a real bug where killProcessTree was unconditionally group-killing (process.kill(-pid, 'SIGTERM')) even when the child was spawned non-detached (sharing the gateway's process group). The fix adds a detached option to killProcessTree and threads the spawn-time useDetached flag through from child.ts. The logic and tests for the new option are correct.

There is one gap: useDetached captures the intent at spawn time but spawnWithFallback can fall back to detached: false when the detached spawn fails (the no-detach fallback). In that case spawned.usedFallback === true but useDetached is still true, so the group-kill is still issued for a child that shares the gateway's process group. The kill call should use spawned.usedFallback ? false : useDetached.

Confidence Score: 3/5

The fix is largely correct but has a gap in the fallback-spawn path that can still trigger the group-kill bug it intends to eliminate.

A P1 logic defect exists: when spawnWithFallback uses the no-detach fallback, useDetached remains true and the group-kill is still issued against the gateway's own process group. This is the same class of bug the PR is fixing and affects a realistic code path.

src/process/supervisor/adapters/child.ts — the kill closure at line 294 needs to account for spawned.usedFallback.

Prompt To Fix All With AI

This is a comment left during a code review.
Path: src/process/supervisor/adapters/child.ts
Line: 294

Comment:
**`useDetached` may not reflect actual spawn outcome after fallback**

`spawnWithFallback` can fall back to `{ detached: false }` when the initial `detached: true` spawn fails (the `no-detach` fallback at line 64–71). In that case `spawned.usedFallback` will be `true`, but `useDetached` still holds `true`. So `killProcessTree(pid, { detached: true })` fires the group-kill (`-pid`) even though the child actually shares the gateway's process group — re-introducing the exact bug this PR is meant to fix, on non-service-managed runtimes when detached spawn is unavailable.

The fix is to derive the effective detached value from the spawn result:

```ts
const actualDetached = spawned.usedFallback ? false : useDetached;
killProcessTree(pid, { detached: actualDetached });
```

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "fix(process): skip kill-tree group kill ..." | Re-trigger Greptile}

greptile-apps · 2026-04-25T17:23:09Z

+        // this, `killProcessTree` group-kills via `-pid` and (in service-managed
+        // mode where useDetached === false) takes out the gateway's own
+        // process group along with the child. (#71662)
+        killProcessTree(pid, { detached: useDetached });


useDetached may not reflect actual spawn outcome after fallback

spawnWithFallback can fall back to { detached: false } when the initial detached: true spawn fails (the no-detach fallback at line 64–71). In that case spawned.usedFallback will be true, but useDetached still holds true. So killProcessTree(pid, { detached: true }) fires the group-kill (-pid) even though the child actually shares the gateway's process group — re-introducing the exact bug this PR is meant to fix, on non-service-managed runtimes when detached spawn is unavailable.

The fix is to derive the effective detached value from the spawn result:

const actualDetached = spawned.usedFallback ? false : useDetached; killProcessTree(pid, { detached: actualDetached });

Prompt To Fix With AI

This is a comment left during a code review. Path: src/process/supervisor/adapters/child.ts Line: 294 Comment: **`useDetached` may not reflect actual spawn outcome after fallback** `spawnWithFallback` can fall back to `{ detached: false }` when the initial `detached: true` spawn fails (the `no-detach` fallback at line 64–71). In that case `spawned.usedFallback` will be `true`, but `useDetached` still holds `true`. So `killProcessTree(pid, { detached: true })` fires the group-kill (`-pid`) even though the child actually shares the gateway's process group — re-introducing the exact bug this PR is meant to fix, on non-service-managed runtimes when detached spawn is unavailable. The fix is to derive the effective detached value from the spawn result: ```ts const actualDetached = spawned.usedFallback ? false : useDetached; killProcessTree(pid, { detached: actualDetached }); ``` How can I resolve this? If you propose a fix, please make it concise.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 647ec60809

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

chatgpt-codex-connector · 2026-04-25T17:25:37Z

+        // this, `killProcessTree` group-kills via `-pid` and (in service-managed
+        // mode where useDetached === false) takes out the gateway's own
+        // process group along with the child. (#71662)
+        killProcessTree(pid, { detached: useDetached });


Pass the actual detach mode to killProcessTree

killProcessTree now branches on detached, but this call forwards useDetached (the intended mode) rather than the effective mode after spawnWithFallback. In this adapter we already allow a no-detach fallback, so a failed detached spawn can still produce a non-detached child while this line reports detached: true. In that case the Unix path still uses process.kill(-pid, ...), which can terminate the gateway’s process group again instead of only the child. Please derive this flag from the realized spawn attempt (e.g., fallback outcome) so kill behavior matches how the process was actually spawned.

Useful? React with 👍 / 👎.

… fallback (openclaw#71662) Greptile review on the original PR caught a P1 gap: when spawnWithFallback's initial detached spawn fails and it retries with the no-detach fallback (label: "no-detach", options.detached: false), the child runs detached:false but my variable useDetached was still true. The kill closure then passed `detached: useDetached` = true to killProcessTree, which still group-killed the gateway — same bug, just on the fallback path. Compute the actual detachment as `useDetached && !spawned.usedFallback` after spawn returns, and pass that through. This closes the gap: the kill path now correctly skips group-kill in BOTH: 1. Service-managed runtime (useDetached=false from the start, original case) 2. Detached-spawn fallback to no-detach (useDetached=true at intent time but spawned.usedFallback=true) Tests: - existing 'uses process-tree kill for default SIGKILL' updated to assert the new {detached} option is forwarded. - new: passes detached:false to killProcessTree when spawn fell back. - new: passes detached:false in service-managed mode (regression guard for the original fix). 11/11 tests pass in child.test.ts. 6/6 in kill-tree.test.ts.

fabianwilliams

Important fix — process-group SIGTERM with -pid was a footgun for non-detached children: it'd hit the gateway's own group. New detached?: boolean opt is the right shape, defaults to true for behavior preservation. Tests cover both branches. Comments document the why clearly. LGTM, fixes #71662.

…penclaw#71662) (openclaw#71681) * fix(process): skip kill-tree group kill when child wasn't detached (openclaw#71662) When the supervisor spawns a child with detached:false (service-managed runtime under launchd/systemd), the child shares the gateway's process group. On session abort or SIGKILL, killProcessTree was unconditionally issuing process.kill(-pid, 'SIGTERM') — which targets the entire process GROUP (negative pid is POSIX group-kill semantics) and therefore SIGTERMs the gateway parent along with the child. Reporter saw this on macOS (LaunchAgent + KeepAlive=true): aborting a claude-cli/claude-opus-4-7 session caused the gateway to receive SIGTERM, then auto-restart, dropping all in-flight sessions. Switching the primary model to a non-cli provider eliminated it because the non-cli paths don't go through this kill-tree call. Did not occur on Linux VPS where the gateway runs detached, because there useDetached === true and the child got its own process group. Fix: - killProcessTree now accepts opts.detached?: boolean. When detached:false, killProcessTreeUnix skips the `-pid` group-kill and goes straight to direct-pid SIGTERM/SIGKILL. Group-kill default (detached:true) is preserved so all existing callers behave exactly as before. - supervisor/adapters/child.ts:286 now threads the spawn-time `useDetached` flag into killProcessTree, so the kill-tree path matches the spawn-time detachment decision (line 45 of the same file already computes useDetached = process.platform !== 'win32' && !isServiceManagedRuntime()). Tests: - new: detached:false skips group kill and uses direct pid SIGTERM only. - new: default behaviour (detached:true) still uses group kill (regression guard so the existing test case isn't accidentally weakened). Existing tests still pass (6/6 in kill-tree.test.ts). Lint clean. Out of scope: other killProcessTree callers (mcp-stdio-transport, bash-tools.process, etc.) keep the default group-kill behaviour because those processes are typically detached from the gateway. Only the supervisor/adapters/child.ts path threads `detached` through, since it's the path that knows whether the child was actually spawned detached. * fixup(process): also gate kill-tree group-kill on the no-detach spawn fallback (openclaw#71662) Greptile review on the original PR caught a P1 gap: when spawnWithFallback's initial detached spawn fails and it retries with the no-detach fallback (label: "no-detach", options.detached: false), the child runs detached:false but my variable useDetached was still true. The kill closure then passed `detached: useDetached` = true to killProcessTree, which still group-killed the gateway — same bug, just on the fallback path. Compute the actual detachment as `useDetached && !spawned.usedFallback` after spawn returns, and pass that through. This closes the gap: the kill path now correctly skips group-kill in BOTH: 1. Service-managed runtime (useDetached=false from the start, original case) 2. Detached-spawn fallback to no-detach (useDetached=true at intent time but spawned.usedFallback=true) Tests: - existing 'uses process-tree kill for default SIGKILL' updated to assert the new {detached} option is forwarded. - new: passes detached:false to killProcessTree when spawn fell back. - new: passes detached:false in service-managed mode (regression guard for the original fix). 11/11 tests pass in child.test.ts. 6/6 in kill-tree.test.ts.

openclaw-barnacle Bot added the size: S label Apr 25, 2026

greptile-apps Bot reviewed Apr 25, 2026

View reviewed changes

chatgpt-codex-connector Bot reviewed Apr 25, 2026

View reviewed changes

fabianwilliams approved these changes Apr 25, 2026

View reviewed changes

fabianwilliams merged commit 4a72e1b into openclaw:main Apr 25, 2026
77 of 80 checks passed

clawsweeper Bot mentioned this pull request May 2, 2026

feat: Add Apple Silicon optimizations #58552

Closed

25 tasks

clawsweeper Bot mentioned this pull request May 3, 2026

fix(process): default killProcessTree to safe direct-pid kill; require explicit detached:true for group-kill (#76259) #76398

Closed

xydigit-zt mentioned this pull request Jun 18, 2026

fix(kill-tree): verify process group leader before using group kill to prevent gateway SIGTERM (#76259) #94697

Merged

clawsweeper Bot mentioned this pull request Jun 29, 2026

fix(process): add zombie reaper to prevent child process leak #97731

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(process): skip kill-tree group kill when child wasn't detached (#71662)#71681

fix(process): skip kill-tree group kill when child wasn't detached (#71662)#71681
fabianwilliams merged 2 commits into
openclaw:mainfrom
hclsys:fix/kill-tree-respect-detached-71662

hclsys commented Apr 25, 2026

Uh oh!

aisle-research-bot Bot commented Apr 25, 2026 •

edited

Loading

Description

Recommendation

Uh oh!

greptile-apps Bot commented Apr 25, 2026

Uh oh!

greptile-apps Bot Apr 25, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot Apr 25, 2026

Uh oh!

fabianwilliams left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

hclsys commented Apr 25, 2026

Bug

Fix

Tests

Out of scope

Uh oh!

aisle-research-bot Bot commented Apr 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔒 Aisle Security Analysis

Description

Recommendation

Uh oh!

greptile-apps Bot commented Apr 25, 2026

Greptile Summary

Confidence Score: 3/5

Uh oh!

greptile-apps Bot Apr 25, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Apr 25, 2026

Choose a reason for hiding this comment

Uh oh!

fabianwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

aisle-research-bot Bot commented Apr 25, 2026 •

edited

Loading