fix(daemon): use KeepAlive SuccessfulExit=false to prevent launchd crash-loop#73849
fix(daemon): use KeepAlive SuccessfulExit=false to prevent launchd crash-loop#73849kaseonedge wants to merge 5 commits into
Conversation
ensureDockerImage now tries to pull custom images and gracefully returns when the Docker daemon is down, instead of throwing 'Sandbox image not found'. This closes the gap identified in the PR review where only DEFAULT_SANDBOX_IMAGE was guarded.
…port - Export from docker.ts, import in browser.ts and doctor-sandbox.ts - Eliminates byte-for-byte duplicated logic across three files - Addresses codex review feedback on PR #73671
Introduced during isDockerDaemonUnavailable consolidation refactor. Fixes CI build failures from duplicate identifier declarations.
…ash-loop Change macOS LaunchAgent plist from KeepAlive=true to KeepAlive with SuccessfulExit=false. This prevents launchd from restarting the gateway after clean exits, making crash debugging much easier while still restarting on actual failures. Also update the service audit check to recognize both KeepAlive forms.
openclaw-barnacle
Bot
added
gateway
Greptile SummaryThis PR fixes the launchd crash-loop by replacing
Confidence Score: 3/5Safe to merge the launchd fix; the Docker image fallback logic in docker.ts introduces a silent behavioral regression that should be addressed first. The core launchd plist change, test update, and audit regex update are correct and well-scoped. However, the Docker-related changes include a P1 issue: ensureDockerImage now silently falls back to debian:bookworm-slim even when Docker is fully operational, which was explicitly disallowed in the original code and degrades sandbox functionality without any user-visible warning. src/agents/sandbox/docker.ts — the ensureDockerImage fallback logic Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/agents/sandbox/docker.ts
Line: 315-327
Comment:
**Silent fallback contradicts original design intent**
When Docker is running but the custom sandbox image is simply missing, this code silently pulls and tags `debian:bookworm-slim` as `DEFAULT_SANDBOX_IMAGE`. The original error message (still present in the failure branch on line 323) explicitly stated "OpenClaw will not substitute plain debian:bookworm-slim" — because the default image includes python3 and other sandbox helpers that `bookworm-slim` lacks. A user with Docker running who forgot to build the image will now silently get a degraded environment with no warning, and the error message they do see if the pull fails still says the substitution won't happen, which is now untrue.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/commands/doctor-sandbox.ts
Line: 89-90
Comment:
**Unused variable `lower`**
`lower` is assigned but never read — `isDockerDaemonUnavailable` is called with the original `stderr` string. Either pass `lower` to the function or remove the assignment.
```suggestion
if (isDockerDaemonUnavailable(stderr)) {
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/agents/sandbox/browser.ts
Line: 129
Comment:
**Misplaced duplicate import**
This import statement lands in the middle of the file (after function bodies), and `SandboxConfig` is already imported at line 49. `SandboxBrowserConfig` is not used anywhere in this file. The import should be removed or consolidated at the top of the file with the other type imports.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(daemon): use KeepAlive SuccessfulExi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (image === DEFAULT_SANDBOX_IMAGE) { | ||
| throw new Error( | ||
| `Sandbox image not found: ${image}. Build it with scripts/sandbox-setup.sh before enabling Docker sandboxing. The default image includes python3 for sandbox write/edit helpers; OpenClaw will not substitute plain debian:bookworm-slim.`, | ||
| ); | ||
| const pullResult = await execDocker(["pull", "debian:bookworm-slim"], { allowFailure: true }); | ||
| if (pullResult.code !== 0) { | ||
| const stderr = pullResult.stderr.trim(); | ||
| if (isDockerDaemonUnavailable(stderr)) { | ||
| return; | ||
| } | ||
| throw new Error( | ||
| `Sandbox image not found: ${image}. Build it with scripts/sandbox-setup.sh before enabling Docker sandboxing. The default image includes python3 for sandbox write/edit helpers; OpenClaw will not substitute plain debian:bookworm-slim. Failed to pull fallback: ${stderr}`, | ||
| ); | ||
| } | ||
| await execDocker(["tag", "debian:bookworm-slim", DEFAULT_SANDBOX_IMAGE]); | ||
| return; |
There was a problem hiding this comment.
When Docker is running but the custom sandbox image is simply missing, this code silently pulls and tags debian:bookworm-slim as DEFAULT_SANDBOX_IMAGE. The original error message (still present in the failure branch on line 323) explicitly stated "OpenClaw will not substitute plain debian:bookworm-slim" — because the default image includes python3 and other sandbox helpers that bookworm-slim lacks. A user with Docker running who forgot to build the image will now silently get a degraded environment with no warning, and the error message they do see if the pull fails still says the substitution won't happen, which is now untrue.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/sandbox/docker.ts
Line: 315-327
Comment:
**Silent fallback contradicts original design intent**
When Docker is running but the custom sandbox image is simply missing, this code silently pulls and tags `debian:bookworm-slim` as `DEFAULT_SANDBOX_IMAGE`. The original error message (still present in the failure branch on line 323) explicitly stated "OpenClaw will not substitute plain debian:bookworm-slim" — because the default image includes python3 and other sandbox helpers that `bookworm-slim` lacks. A user with Docker running who forgot to build the image will now silently get a degraded environment with no warning, and the error message they do see if the pull fails still says the substitution won't happen, which is now untrue.
How can I resolve this? If you propose a fix, please make it concise.| const lower = stderr.toLowerCase(); | ||
| if (isDockerDaemonUnavailable(stderr)) { |
There was a problem hiding this comment.
lower
lower is assigned but never read — isDockerDaemonUnavailable is called with the original stderr string. Either pass lower to the function or remove the assignment.
| const lower = stderr.toLowerCase(); | |
| if (isDockerDaemonUnavailable(stderr)) { | |
| if (isDockerDaemonUnavailable(stderr)) { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/commands/doctor-sandbox.ts
Line: 89-90
Comment:
**Unused variable `lower`**
`lower` is assigned but never read — `isDockerDaemonUnavailable` is called with the original `stderr` string. Either pass `lower` to the function or remove the assignment.
```suggestion
if (isDockerDaemonUnavailable(stderr)) {
```
How can I resolve this? If you propose a fix, please make it concise.| }; | ||
| } | ||
|
|
||
| import type { SandboxBrowserConfig, SandboxConfig } from "./types.js"; |
There was a problem hiding this comment.
This import statement lands in the middle of the file (after function bodies), and SandboxConfig is already imported at line 49. SandboxBrowserConfig is not used anywhere in this file. The import should be removed or consolidated at the top of the file with the other type imports.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/sandbox/browser.ts
Line: 129
Comment:
**Misplaced duplicate import**
This import statement lands in the middle of the file (after function bodies), and `SandboxConfig` is already imported at line 49. `SandboxBrowserConfig` is not used anywhere in this file. The import should be removed or consolidated at the top of the file with the other type imports.
How can I resolve this? If you propose a fix, please make it concise.
|
Thanks for the context here. I swept through the related work, and this is now duplicate or superseded. Close as superseded: the focused launchd crash-loop work is now tracked by the newer maintainer-labeled PR, while this branch uses the wrong launchd failure policy and carries already-merged/regressive sandbox changes. Canonical path: Continue the focused launchd lifecycle fix in #81726 or a replacement that preserves clean-exit restart handoffs, and keep sandbox Docker behavior on the merged #73671 path. So I’m closing this here and keeping the remaining discussion on #81726 and #73671. Review detailsBest possible solution: Continue the focused launchd lifecycle fix in #81726 or a replacement that preserves clean-exit restart handoffs, and keep sandbox Docker behavior on the merged #73671 path. Do we have a high-confidence way to reproduce the issue? Yes, source-backed rather than live: current main emits boolean Is this the best way to solve the issue? No. This PR's Security review: Security review needs attention: The launchd change is not security-sensitive, but the sandbox diff introduces a concrete Docker image provenance regression.
What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 4e10969aded0. |
1 participant
Fixes #73673
Problem
The macOS LaunchAgent plist uses
KeepAlive=true, which restarts the gateway after ANY exit (including clean exits with code 0). This creates a crash-loop where:Solution
Change
KeepAlive=truetoKeepAlivewithSuccessfulExit=false:ThrottleInterval=1still applies to crash restartsChanges
src/daemon/launchd-plist.ts: Updated plist template to useKeepAlive<dict>SuccessfulExit<false/></dict>src/daemon/launchd.test.ts: Updated test to expect new KeepAlive formatsrc/daemon/service-audit.ts: Updated audit check to recognize both KeepAlive forms