fix(desktop): attach to a daemon that owns the data dir instead of wedging#1086
Conversation
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
executor-marketing | b9a96c3 | Commit Preview URL Branch Preview URL |
Jun 28 2026, 09:43 AM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
executor-cloud | b9a96c3 | Jun 28 2026, 09:44 AM |
Cloudflare previewTorn down — the PR is closed. |
Greptile SummaryThis PR fixes the "scope-ownership wedge" where the desktop app would sit stuck on the crash screen if a CLI-launched
Confidence Score: 4/5Mostly safe to merge; the core attach-on-conflict recovery logic is correct and well-tested, but a narrow gap in the boot catch path leaves an adopted daemon unwatched in one specific failure sequence. The boot() catch block executes a second startWithCurrentSettings() at line ~996 (after a createWindow failure triggers a data reset). If that call adopts a CLI daemon, createWindow succeeds at line ~1007 but armSupervisedMonitor() is never called. The adopted daemon has no child process for onUnexpectedSidecarExit to observe, so if it later dies the app goes silent with no crash screen or recovery attempt. This path requires an unusual double failure (createWindow fails AND the reset retry ends up adopting a daemon), but the gap was flagged as a visible review comment in the previous round and remains unaddressed. apps/desktop/src/main/index.ts — specifically the post-reset createWindow retry block (~line 1007) in the boot() catch handler. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[startWithCurrentSettings] --> B[startSidecar]
B -->|success| C[return SidecarConnection]
B -->|SidecarPortInUseError| D[showPortInUseDialog\nreturn null]
B -->|other error| E{isScopeOwnershipConflict?}
E -->|no| F[lastSidecarStartError = error\nreturn null]
E -->|yes| G[waitForSupervisedAttach 10s]
G -->|adopted| H[log info\nreturn adopted connection]
G -->|null timeout| F
H --> I{caller}
I -->|boot primary path\ncreateWindow succeeds| J{connection.supervisedDaemon?}
J -->|yes| K[armSupervisedMonitor ✓]
J -->|no| L[child-exit path via\nonUnexpectedSidecarExit]
I -->|restartSidecarAndReload| M{next.supervisedDaemon?}
M -->|yes| N[armSupervisedMonitor ✓]
M -->|no| O[stopSupervisedMonitor ✓]
I -->|boot post-reset retry\ncreateWindow line 1007| P[createWindow\nNO armSupervisedMonitor ⚠️]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[startWithCurrentSettings] --> B[startSidecar]
B -->|success| C[return SidecarConnection]
B -->|SidecarPortInUseError| D[showPortInUseDialog\nreturn null]
B -->|other error| E{isScopeOwnershipConflict?}
E -->|no| F[lastSidecarStartError = error\nreturn null]
E -->|yes| G[waitForSupervisedAttach 10s]
G -->|adopted| H[log info\nreturn adopted connection]
G -->|null timeout| F
H --> I{caller}
I -->|boot primary path\ncreateWindow succeeds| J{connection.supervisedDaemon?}
J -->|yes| K[armSupervisedMonitor ✓]
J -->|no| L[child-exit path via\nonUnexpectedSidecarExit]
I -->|restartSidecarAndReload| M{next.supervisedDaemon?}
M -->|yes| N[armSupervisedMonitor ✓]
M -->|no| O[stopSupervisedMonitor ✓]
I -->|boot post-reset retry\ncreateWindow line 1007| P[createWindow\nNO armSupervisedMonitor ⚠️]
Reviews (2): Last reviewed commit: "fix(desktop): attach to a daemon that ow..." | Re-trigger Greptile |
@executor-js/cli
@executor-js/config
@executor-js/execution
@executor-js/sdk
@executor-js/codemode-core
@executor-js/runtime-quickjs
@executor-js/plugin-file-secrets
@executor-js/plugin-graphql
@executor-js/plugin-keychain
@executor-js/plugin-mcp
@executor-js/plugin-onepassword
@executor-js/plugin-openapi
executor
commit: |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // The data dir is already owned by another local daemon. If it's a healthy | ||
| // cli-daemon, adopt it instead of failing — the same handoff boot() does up | ||
| // front, applied here so the fallback and restart paths recover from a | ||
| // mid-session daemon takeover. Poll briefly to ride out an owner that's | ||
| // alive but still finishing its own startup (the health-probe vs. | ||
| // scope-lock race that left the app wedged on the crash screen). | ||
| if (isScopeOwnershipConflict(error)) { | ||
| const adopted = await waitForSupervisedAttach(10_000); | ||
| if (adopted) { | ||
| log.info( | ||
| "Adopted the local daemon that owns the data dir instead of spawning a second one", | ||
| ); | ||
| return adopted; | ||
| } | ||
| } |
There was a problem hiding this comment.
AGENTS.md states: "Never use em-dashes (the — character) anywhere: prose, docs, code comments, commit messages, or PRs." There are three new em-dashes in the comments added by this PR: "adopt it instead of failing — the same handoff" (around line 524), "can't see it die — watch it" (around line 576), and "supervised monitor — it has no child process" (around line 953). All three should be replaced with a comma, colon, or a separate sentence.
Context Used: AGENTS.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
RhysSullivan
force-pushed
the
fix/desktop-daemon-attach-spawn-wedge
branch
2 times, most recently
from
ff22cca to
4739a9b
Compare
…dging
The desktop app and a standalone `executor daemon run` both default to owning
~/.executor, and only one process can own it. boot() attaches to an existing
daemon before spawning, but the restart and boot-fallback paths only spawned:
when a foreign cli-daemon already owned the scope, the spawn died on the scope
lock ("already running ... owns the current data directory") and was treated as
fatal, wedging the app on the sidecar crash screen until the daemon was stopped
by hand.
startWithCurrentSettings now adopts a healthy cli-daemon that already owns the
data dir instead of failing, polling briefly to ride out an owner that is alive
but still finishing startup (the health-probe vs scope-lock race). Adopted
daemons are watched by the supervised monitor, since they have no child process
for onUnexpectedSidecarExit to observe.
Also adds EXECUTOR_TEST_SKIP_BACKGROUND_SERVICE so the packaged app can boot
headlessly, a deterministic dev e2e regression test, and a headless Linux+Xvfb
harness that reproduces the wedge against the real packaged bundle.
RhysSullivan
force-pushed
the
fix/desktop-daemon-attach-spawn-wedge
branch
from
4739a9b to
b9a96c3
Compare
1 participant
Problem
The desktop app and a standalone
executor daemon runboth default to owning~/.executor, and only one process can own it at a time.boot()attaches to an already-running daemon before spawning, so cold starts are fine. But the two recovery paths,restartSidecarAndReload(after a sidecar exit) and the boot fallback, only ever spawned. When a foreign cli-daemon already owned the scope, the spawned server died on the scope lock:That failure was treated as fatal, so the window sat on the sidecar crash screen and "Restart server" could not recover until the foreign daemon was stopped by hand.
Fix
startWithCurrentSettingsnow adopts a healthy cli-daemon that already owns the data dir instead of failing, polling briefly to ride out an owner that is alive but still finishing startup (the health-probe vs scope-lock race). This is the single chokepoint for both the boot-fallback and restart paths, and mirrors whatreplaceSupervisedDaemonForDesktopalready does at boot.Adopted daemons are now watched by the supervised monitor, since an adopted daemon has no child process for
onUnexpectedSidecarExitto observe.Reproductions
e2e/desktop/sidecar-attach-conflict.test.ts(dev project, deterministic): a cli-daemon owns the temp HOME's.executor; the app must come up by attaching instead of dying on the scope lock.e2e/desktop-packaged/linux-vm/: a headless Linux + Xvfb harness that drives the real packaged bundle over CDP and replays the incident (spawn own sidecar, kill it, a cli-daemon takes over, click Restart). Verified failing before the fix (the app stays wedged on the crash screen) and passing after (the app recovers by attaching, no second server spawned).Adds
EXECUTOR_TEST_SKIP_BACKGROUND_SERVICE(mirrorsEXECUTOR_TEST_AUTO_CONFIRM_RESET) so the packaged app can boot headlessly.Follow-up (not in this PR)
notes/desktop-daemon-ownership-wedge.mddocuments a separate, macOS-specific defect seen in the same logs: the launchd supervised-service install fails (launchctl bootstrap exit 5,kickstart exit 113), so the app can never promote a daemon to a supervised service and stays in the fragile attach/spawn mode where this wedge was reachable. Not reproducible under Linux + Xvfb; worth its own investigation.Stack