Repository navigation
Conversation
…iter) A host-service left running by a previous process (Electron hard-crash before before-quit, a CLI-spawned one with no watchdog, or a wedged child from a failed spawn) keeps a connection to the org's host.db. The coordinator only tracks instances it spawned this process, so on the next launch it spawns a competitor on a fresh port — two writers on one host.db, the contention behind the migration lock-up. Reap based on the manifest (the only cross-process record) before spawning: confirm identity first (health.check with the manifest secret, or a best-effort POSIX argv match for a wedged child) so PID reuse can't cause friendly-fire — an unrecognized live pid is left alone and the stale manifest dropped — then SIGTERM, await exit, escalate to SIGKILL. - host-service-reap.ts: dependency-injected reap logic (electron-free, unit-testable). - coordinator: wire real deps + identity probe, call at the top of spawn(). - host-service-utils: add healthCheckOnce (single-shot identity probe; pollHealthCheck now reuses it). - host-service-reap.test.ts: 6 cases incl. PID-reuse-left-alone and SIGTERM->SIGKILL escalation.
📝 WalkthroughWalkthroughThe PR introduces a proactive host-service process reaping system to clean up stale processes before spawning a new one per organization. It extracts and reuses health-check logic, defines a structured reap module with signal-escalation and grace-period handling, and integrates reaping into the coordinator's spawn flow with dual identity-confirmation strategies (health endpoint + POSIX fallback). ChangesHost-service reaping feature
Sequence DiagramsequenceDiagram
participant Coordinator as HostServiceCoordinator
participant Reaper as reapPreviousHostService
participant Manifest as HostServiceManifest (disk)
participant HealthAPI as Health Endpoint
participant Process as OS Process (PID)
Coordinator->>Reaper: spawn() → reapPreviousHostService(orgId)
Reaper->>Manifest: readManifest(orgId)
alt No manifest
Reaper-->>Coordinator: ReapResult{reason: 'no-manifest'}
else Own live instance
Reaper-->>Coordinator: ReapResult{reason: 'own-live-instance'}
else Dead PID
Reaper->>Manifest: removeManifest(orgId)
Reaper-->>Coordinator: ReapResult{reason: 'dead-pid-cleared'}
else Unconfirmed live PID
Reaper->>HealthAPI: healthCheckOnce(endpoint, secret)
HealthAPI-->>Reaper: false/error
Reaper->>Manifest: removeManifest(orgId)
Reaper-->>Coordinator: ReapResult{reason: 'unidentified-left-alone'}
else Confirmed PID needs reap
Reaper->>HealthAPI: healthCheckOnce(endpoint, secret)
HealthAPI-->>Reaper: true
Reaper->>Process: kill(pid, SIGTERM)
Reaper->>Process: waitForPidExit(pid, REAP_SIGTERM_GRACE_MS)
alt Process exited
Reaper->>Manifest: removeManifest(orgId)
Reaper-->>Coordinator: ReapResult{reason: 'terminated-sigterm'}
else Process still alive
Reaper->>Process: kill(pid, SIGKILL)
Reaper->>Process: waitForPidExit(pid, REAP_SIGKILL_GRACE_MS)
Reaper->>Manifest: removeManifest(orgId)
Reaper-->>Coordinator: ReapResult{reason: 'terminated-sigkill'}
end
end
Coordinator->>Coordinator: select/bind ports
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Ready to review this PR? Stage has broken it down into 3 individual chapters for you:
Chapters generated by Stage for commit 19143e5 on May 29, 2026 6:23pm UTC. |
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
Greptile SummaryThis PR fixes the "two writers on the same
Confidence Score: 4/5The reap logic is structurally sound and PID-reuse-safe; two non-blocking issues exist that do not affect production correctness. The reap decision tree covers all important cases correctly, identity verification via the manifest secret is a strong guard against friendly-fire, and the SIGTERM to SIGKILL escalation is well-tested. The synchronous execFileSync is a fallback path that only activates when an orphan health endpoint is unresponsive and ps completes in milliseconds in practice. The SIGKILL-escalation test functionally verifies the right behavior but spends ~4 real seconds doing so due to the un-injected grace-period constant. The execFileSync call in processLooksLikeHostService and the SIGKILL test timing in host-service-reap.test.ts are worth revisiting.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/main/lib/host-service-reap.ts | New reap module with dependency-injected logic; kill flow (SIGTERM → wait → SIGKILL → wait) is correct and PID-reuse-safe, but grace-period timeouts are hard-coded constants not injected through ReapDeps. |
| apps/desktop/src/main/lib/host-service-coordinator.ts | Wires reap logic into spawn(); confirmOurHostService uses healthCheckOnce then falls back to processLooksLikeHostService, which calls execFileSync synchronously and can block the Electron main process event loop for up to 1,500 ms. |
| apps/desktop/src/main/lib/host-service-reap.test.ts | Six test cases cover all branches; sleep is mocked as a no-op but waitForPidExit's deadline uses real Date.now(), so the SIGKILL-escalation test spin-waits for the full REAP_SIGTERM_GRACE_MS (4 s) of real wall-clock time. |
| apps/desktop/src/main/lib/host-service-utils.ts | Correctly extracts healthCheckOnce from pollHealthCheck; no logic change to the existing poll loop, clean refactor. |
Sequence Diagram
sequenceDiagram
participant Caller
participant Coordinator as HostServiceCoordinator
participant Reap as reapPreviousHostService
participant Manifest as host-service-manifest
participant ChildProc as orphan process
Caller->>Coordinator: start(orgId, config)
Coordinator->>Coordinator: spawn(orgId, config)
Coordinator->>Reap: reapPreviousHostService(orgId, deps)
Reap->>Manifest: readManifest(orgId)
alt no manifest
Manifest-->>Reap: null
Reap-->>Coordinator: reason: no-manifest
else pid is own live instance
Reap-->>Coordinator: reason: own-live-instance
else pid is dead
Reap->>Manifest: removeManifest(orgId)
Reap-->>Coordinator: reason: dead-pid-cleared
else pid is alive
Reap->>Coordinator: confirmOurHostService(manifest)
Coordinator->>ChildProc: healthCheckOnce(endpoint, authToken)
alt health check passes
ChildProc-->>Coordinator: 2xx OK
Coordinator-->>Reap: true
else health check fails
Coordinator->>Coordinator: processLooksLikeHostService [ps, POSIX]
Coordinator-->>Reap: true / false
end
alt not our host-service
Reap->>Manifest: removeManifest(orgId)
Reap-->>Coordinator: reason: unidentified-left-alone
else confirmed ours
Reap->>ChildProc: SIGTERM
Reap->>Reap: waitForPidExit(4000ms)
alt exits on SIGTERM
Reap->>Manifest: removeManifest(orgId)
Reap-->>Coordinator: reason: terminated-sigterm
else survives SIGTERM
Reap->>ChildProc: SIGKILL
Reap->>Reap: waitForPidExit(2000ms)
Reap->>Manifest: removeManifest(orgId)
Reap-->>Coordinator: reason: terminated-sigkill
end
end
end
Coordinator->>Coordinator: findFreePort() then spawn new child
Coordinator-->>Caller: Connection
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
apps/desktop/src/main/lib/host-service-reap.test.ts:60-103
**SIGKILL-escalation test takes ~4 s of real wall-clock time**
`sleep` is mocked as a no-op, but `waitForPidExit` uses `Date.now()` for its deadline — it doesn't rely on `sleep` to advance time. In the SIGKILL test `isProcessAlive` returns `true` on every iteration until SIGKILL fires, so the loop busy-spins against real wall time for the full `REAP_SIGTERM_GRACE_MS = 4_000 ms` before the deadline expires. The "No real waiting in tests" comment in `makeDeps` is therefore inaccurate for this case.
The clean fix is to add injectable timeout overrides to `ReapDeps` (e.g. `termGraceMs` and `killGraceMs`) and default them to the exported constants, then pass `0` in the two escalation tests. Alternatively, make `waitForPidExit` itself injectable in `ReapDeps`.
### Issue 2 of 2
apps/desktop/src/main/lib/host-service-coordinator.ts:587-599
**Synchronous `execFileSync` may block the Electron main-process event loop for up to 1,500 ms**
`processLooksLikeHostService` is reached only after `healthCheckOnce` already timed out (1,500 ms). The subsequent `execFileSync("ps", …, { timeout: 1_500 })` is fully synchronous and holds the Node.js event loop for up to another 1,500 ms. In practice `ps` finishes in single-digit milliseconds, but in a pathological environment (e.g. high load, container with limited CPU shares) this can make the Electron UI transiently unresponsive.
Replacing `execFileSync` with promisified `execFile` and `await`-ing the result inside the already-`async` `confirmOurHostService` caller would eliminate the synchronous block without changing observable behavior.
Reviews (1): Last reviewed commit: "fix(host-service): reap orphaned host-se..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| killProcess, | ||
| }); | ||
| const result = await reapPreviousHostService(ORG, deps); | ||
| expect(result.reason).toBe("unidentified-left-alone"); | ||
| expect(killProcess).not.toHaveBeenCalled(); | ||
| expect(deps.removeManifest).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| test("alive + ours + exits on SIGTERM → SIGTERM only, no SIGKILL", async () => { | ||
| const signals: NodeJS.Signals[] = []; | ||
| // Alive until SIGTERM is observed, then gone. | ||
| let sigtermed = false; | ||
| const deps = makeDeps({ | ||
| killProcess: mock((_pid: number, sig: NodeJS.Signals) => { | ||
| signals.push(sig); | ||
| if (sig === "SIGTERM") sigtermed = true; | ||
| }), | ||
| isProcessAlive: mock(() => !sigtermed), | ||
| }); | ||
| const result = await reapPreviousHostService(ORG, deps); | ||
| expect(result.reaped).toBe(true); | ||
| expect(result.reason).toBe("terminated-sigterm"); | ||
| expect(signals).toEqual(["SIGTERM"]); | ||
| expect(deps.removeManifest).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| test("alive + ours + ignores SIGTERM → escalates to SIGKILL", async () => { | ||
| const signals: NodeJS.Signals[] = []; | ||
| let sigkilled = false; | ||
| const deps = makeDeps({ | ||
| killProcess: mock((_pid: number, sig: NodeJS.Signals) => { | ||
| signals.push(sig); | ||
| if (sig === "SIGKILL") sigkilled = true; | ||
| }), | ||
| // Survives SIGTERM (always alive until SIGKILL lands). | ||
| isProcessAlive: mock(() => !sigkilled), | ||
| }); | ||
| const result = await reapPreviousHostService(ORG, deps); | ||
| expect(result.reaped).toBe(true); | ||
| expect(result.reason).toBe("terminated-sigkill"); | ||
| expect(signals).toEqual(["SIGTERM", "SIGKILL"]); | ||
| expect(deps.removeManifest).toHaveBeenCalledTimes(1); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
sleep is mocked as a no-op, but waitForPidExit uses Date.now() for its deadline — it doesn't rely on sleep to advance time. In the SIGKILL test isProcessAlive returns true on every iteration until SIGKILL fires, so the loop busy-spins against real wall time for the full REAP_SIGTERM_GRACE_MS = 4_000 ms before the deadline expires. The "No real waiting in tests" comment in makeDeps is therefore inaccurate for this case.
The clean fix is to add injectable timeout overrides to ReapDeps (e.g. termGraceMs and killGraceMs) and default them to the exported constants, then pass 0 in the two escalation tests. Alternatively, make waitForPidExit itself injectable in ReapDeps.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/lib/host-service-reap.test.ts
Line: 60-103
Comment:
**SIGKILL-escalation test takes ~4 s of real wall-clock time**
`sleep` is mocked as a no-op, but `waitForPidExit` uses `Date.now()` for its deadline — it doesn't rely on `sleep` to advance time. In the SIGKILL test `isProcessAlive` returns `true` on every iteration until SIGKILL fires, so the loop busy-spins against real wall time for the full `REAP_SIGTERM_GRACE_MS = 4_000 ms` before the deadline expires. The "No real waiting in tests" comment in `makeDeps` is therefore inaccurate for this case.
The clean fix is to add injectable timeout overrides to `ReapDeps` (e.g. `termGraceMs` and `killGraceMs`) and default them to the exported constants, then pass `0` in the two escalation tests. Alternatively, make `waitForPidExit` itself injectable in `ReapDeps`.
How can I resolve this? If you propose a fix, please make it concise.| function processLooksLikeHostService(pid: number, scriptPath: string): boolean { | ||
| if (process.platform === "win32") return false; | ||
| try { | ||
| const command = childProcess | ||
| .execFileSync("ps", ["-p", String(pid), "-o", "command="], { | ||
| encoding: "utf8", | ||
| timeout: 1_500, | ||
| }) | ||
| .trim(); | ||
| return command.includes(path.basename(scriptPath)); | ||
| } catch { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
execFileSync may block the Electron main-process event loop for up to 1,500 ms
processLooksLikeHostService is reached only after healthCheckOnce already timed out (1,500 ms). The subsequent execFileSync("ps", …, { timeout: 1_500 }) is fully synchronous and holds the Node.js event loop for up to another 1,500 ms. In practice ps finishes in single-digit milliseconds, but in a pathological environment (e.g. high load, container with limited CPU shares) this can make the Electron UI transiently unresponsive.
Replacing execFileSync with promisified execFile and await-ing the result inside the already-async confirmOurHostService caller would eliminate the synchronous block without changing observable behavior.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/lib/host-service-coordinator.ts
Line: 587-599
Comment:
**Synchronous `execFileSync` may block the Electron main-process event loop for up to 1,500 ms**
`processLooksLikeHostService` is reached only after `healthCheckOnce` already timed out (1,500 ms). The subsequent `execFileSync("ps", …, { timeout: 1_500 })` is fully synchronous and holds the Node.js event loop for up to another 1,500 ms. In practice `ps` finishes in single-digit milliseconds, but in a pathological environment (e.g. high load, container with limited CPU shares) this can make the Electron UI transiently unresponsive.
Replacing `execFileSync` with promisified `execFile` and `await`-ing the result inside the already-`async` `confirmOurHostService` caller would eliminate the synchronous block without changing observable behavior.
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/main/lib/host-service-coordinator.ts`:
- Around line 590-596: Replace the raw substring check with a token-aware match:
call childProcess.execFileSync(...) to read the process command/args for pid (as
currently done), then test whether any argv token's basename equals
path.basename(scriptPath) (or whether any token ends with path.sep +
path.basename(scriptPath)) instead of using command.includes(...). In other
words, split/parse the command into arguments (or use a regex that matches a
path tail or token boundary, e.g. (?:^|[\/\s])<basename>(?:$|\s)) and only
return true when a full token/path tail matches scriptPath's basename to avoid
matching substrings like "not-host-service.js". Use the existing symbols
childProcess.execFileSync, pid, scriptPath, and path.basename to locate and
implement the change.
In `@apps/desktop/src/main/lib/host-service-reap.ts`:
- Around line 130-139: The code removes the manifest and returns reaped: true
even if waitForPidExit(pid, REAP_SIGKILL_GRACE_MS, deps.isProcessAlive, sleep)
returns false; change the logic in the function containing that call so you only
call deps.removeManifest(organizationId) and return { reaped: true, ... } when
waitForPidExit returns true, and if it returns false keep the manifest and
return a failure result (e.g. { reaped: false, reason: "failed-to-terminate",
pid }) so HostServiceCoordinator.spawn() can bail out on "failed-to-terminate";
locate and update the block around waitForPidExit, deps.removeManifest, and the
returned object to implement this conditional behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2e98fd39-c0fa-427a-8611-de2839e94601
📒 Files selected for processing (4)
apps/desktop/src/main/lib/host-service-coordinator.tsapps/desktop/src/main/lib/host-service-reap.test.tsapps/desktop/src/main/lib/host-service-reap.tsapps/desktop/src/main/lib/host-service-utils.ts
| const command = childProcess | ||
| .execFileSync("ps", ["-p", String(pid), "-o", "command="], { | ||
| encoding: "utf8", | ||
| timeout: 1_500, | ||
| }) | ||
| .trim(); | ||
| return command.includes(path.basename(scriptPath)); |
There was a problem hiding this comment.
Make the argv fallback token-aware instead of using includes().
A recycled PID whose command line contains host-service.js as a substring — for example not-host-service.js or some unrelated arg value — will be treated as ours and killed. That breaks the PID-reuse safety guarantee this fallback is supposed to preserve. Match a basename token / path tail, not a raw substring.
Possible tightening
const command = childProcess
.execFileSync("ps", ["-p", String(pid), "-o", "command="], {
encoding: "utf8",
timeout: 1_500,
})
.trim();
- return command.includes(path.basename(scriptPath));
+ const scriptName = path
+ .basename(scriptPath)
+ .replace(/[.*+?^${}()|[\]\\]/g, "\\$&");
+ return new RegExp(`(?:^|[\\s/])${scriptName}(?:$|\\s)`).test(command);
} catch {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/main/lib/host-service-coordinator.ts` around lines 590 -
596, Replace the raw substring check with a token-aware match: call
childProcess.execFileSync(...) to read the process command/args for pid (as
currently done), then test whether any argv token's basename equals
path.basename(scriptPath) (or whether any token ends with path.sep +
path.basename(scriptPath)) instead of using command.includes(...). In other
words, split/parse the command into arguments (or use a regex that matches a
path tail or token boundary, e.g. (?:^|[\/\s])<basename>(?:$|\s)) and only
return true when a full token/path tail matches scriptPath's basename to avoid
matching substrings like "not-host-service.js". Use the existing symbols
childProcess.execFileSync, pid, scriptPath, and path.basename to locate and
implement the change.
| await waitForPidExit( | ||
| pid, | ||
| REAP_SIGKILL_GRACE_MS, | ||
| deps.isProcessAlive, | ||
| sleep, | ||
| ); | ||
| } | ||
|
|
||
| deps.removeManifest(organizationId); | ||
| return { reaped: true, reason, pid }; |
There was a problem hiding this comment.
Don't report success until the old PID is actually gone.
If waitForPidExit() is still false after the SIGKILL grace period, this code still removes the manifest and returns reaped: true. That lets spawn() continue while the previous writer may still hold host.db, which reintroduces the lockup this PR is meant to prevent. Keep the manifest and surface a failure result when the PID survives SIGKILL.
Suggested direction
export type ReapReason =
| "no-manifest"
| "own-live-instance"
| "dead-pid-cleared"
| "unidentified-left-alone"
| "terminated-sigterm"
- | "terminated-sigkill";
+ | "terminated-sigkill"
+ | "failed-to-terminate";
@@
- await waitForPidExit(
+ const exitedAfterSigkill = await waitForPidExit(
pid,
REAP_SIGKILL_GRACE_MS,
deps.isProcessAlive,
sleep,
);
+ if (!exitedAfterSigkill) {
+ deps.log?.warn(
+ `[host-service:${organizationId}] pid=${pid} survived SIGKILL; refusing to spawn a second writer`,
+ );
+ return { reaped: false, reason: "failed-to-terminate", pid };
+ }
}
deps.removeManifest(organizationId);
return { reaped: true, reason, pid };HostServiceCoordinator.spawn() should then bail out on "failed-to-terminate" instead of proceeding.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/main/lib/host-service-reap.ts` around lines 130 - 139, The
code removes the manifest and returns reaped: true even if waitForPidExit(pid,
REAP_SIGKILL_GRACE_MS, deps.isProcessAlive, sleep) returns false; change the
logic in the function containing that call so you only call
deps.removeManifest(organizationId) and return { reaped: true, ... } when
waitForPidExit returns true, and if it returns false keep the manifest and
return a failure result (e.g. { reaped: false, reason: "failed-to-terminate",
pid }) so HostServiceCoordinator.spawn() can bail out on "failed-to-terminate";
locate and update the block around waitForPidExit, deps.removeManifest, and the
returned object to implement this conditional behavior.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/main/lib/host-service-coordinator.ts">
<violation number="1" location="apps/desktop/src/main/lib/host-service-coordinator.ts:591">
P2: `execFileSync` blocks the Electron main-process event loop for up to 1.5 s. Since the caller (`confirmOurHostService`) is already `async`, replace with the promisified `execFile` + `await` to avoid UI jank under pathological load.</violation>
<violation number="2" location="apps/desktop/src/main/lib/host-service-coordinator.ts:596">
P1: Process identity check too loose. Basename substring can match unrelated commands, then reap can kill wrong PID. Use full script path (or exact argv token) for confirmation.</violation>
</file>
<file name="apps/desktop/src/main/lib/host-service-reap.ts">
<violation number="1" location="apps/desktop/src/main/lib/host-service-reap.ts:110">
P1: SIGTERM catch marks reap as success for every error. Non-race kill failures can leave the old process alive and still drop the manifest. Check liveness in catch, only treat real exit as success, otherwise rethrow.</violation>
<violation number="2" location="apps/desktop/src/main/lib/host-service-reap.ts:130">
P0: SIGKILL path ignores exit result. Code always clears manifest and returns reaped=true even if pid still alive. Require confirmed exit before success path.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Coordinator as HostServiceCoordinator
participant Reap as reapPreviousHostService
participant Manifest as Manifest Store (Disk)
participant OldHS as Previous HostService
participant NewHS as New HostService
participant Health as Health Check Client
Note over Coordinator,Health: SPAWN FLOW WITH REAP
Coordinator->>Coordinator: spawn(organizationId)
Coordinator->>Reap: reapPreviousHostService(orgId, deps)
Reap->>Manifest: readManifest(orgId)
Manifest-->>Reap: HostServiceManifest {pid, endpoint, authToken}
alt no manifest
Reap-->>Coordinator: {reason: "no-manifest", reaped: false}
else manifest exists
Reap->>Coordinator: isOwnLivePid(pid)
alt PID tracked and live in this process
Coordinator-->>Reap: true
Reap-->>Coordinator: {reason: "own-live-instance", reaped: false}
else PID not ours
Coordinator-->>Reap: false
Reap->>Reap: isProcessAlive(pid)
alt PID dead
Reap->>Manifest: removeManifest(orgId)
Reap-->>Coordinator: {reason: "dead-pid-cleared", reaped: false}
else PID alive
Reap->>Reap: confirmOurHostService(manifest)
Reap->>Health: healthCheckOnce(endpoint, authToken)
Health-->>Reap: true if endpoint answers with secret
alt unrecognized (PID reuse)
Reap->>Reap: processLooksLikeHostService(pid, scriptPath)
Note over Reap: POSIX fallback via ps command
alt recognized
Reap->>OldHS: killProcess(pid, SIGTERM)
Reap->>Reap: waitForPidExit(pid, 4s)
alt exited
Manifest-->>Reap: removeManifest(orgId)
Reap-->>Coordinator: {reason: "terminated-sigterm", reaped: true}
else still alive
Reap->>OldHS: killProcess(pid, SIGKILL)
Reap->>Reap: waitForPidExit(pid, 2s)
Reap->>Manifest: removeManifest(orgId)
Reap-->>Coordinator: {reason: "terminated-sigkill", reaped: true}
end
else not recognized
Reap->>Manifest: removeManifest(orgId)
Reap-->>Coordinator: {reason: "unidentified-left-alone", reaped: false}
end
end
end
end
end
Coordinator->>Coordinator: [after reap complete] findFreePort()
Coordinator->>NewHS: spawn child process
NewHS->>Health: start health.check endpoint
Coordinator->>Health: pollHealthCheck(endpoint, secret)
Health-->>Coordinator: ready
Coordinator->>Coordinator: register instance in instances map
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| } catch { | ||
| // Raced to exit during the grace window. | ||
| } | ||
| await waitForPidExit( |
There was a problem hiding this comment.
P0: SIGKILL path ignores exit result. Code always clears manifest and returns reaped=true even if pid still alive. Require confirmed exit before success path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/main/lib/host-service-reap.ts, line 130:
<comment>SIGKILL path ignores exit result. Code always clears manifest and returns reaped=true even if pid still alive. Require confirmed exit before success path.</comment>
<file context>
@@ -0,0 +1,140 @@
+ } catch {
+ // Raced to exit during the grace window.
+ }
+ await waitForPidExit(
+ pid,
+ REAP_SIGKILL_GRACE_MS,
</file context>
| timeout: 1_500, | ||
| }) | ||
| .trim(); | ||
| return command.includes(path.basename(scriptPath)); |
There was a problem hiding this comment.
P1: Process identity check too loose. Basename substring can match unrelated commands, then reap can kill wrong PID. Use full script path (or exact argv token) for confirmation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/main/lib/host-service-coordinator.ts, line 596:
<comment>Process identity check too loose. Basename substring can match unrelated commands, then reap can kill wrong PID. Use full script path (or exact argv token) for confirmation.</comment>
<file context>
@@ -533,6 +578,27 @@ function pipeWithPrefix(
+ timeout: 1_500,
+ })
+ .trim();
+ return command.includes(path.basename(scriptPath));
+ } catch {
+ return false;
</file context>
| let reason: ReapReason = "terminated-sigterm"; | ||
| try { | ||
| deps.killProcess(pid, "SIGTERM"); | ||
| } catch { |
There was a problem hiding this comment.
P1: SIGTERM catch marks reap as success for every error. Non-race kill failures can leave the old process alive and still drop the manifest. Check liveness in catch, only treat real exit as success, otherwise rethrow.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/main/lib/host-service-reap.ts, line 110:
<comment>SIGTERM catch marks reap as success for every error. Non-race kill failures can leave the old process alive and still drop the manifest. Check liveness in catch, only treat real exit as success, otherwise rethrow.</comment>
<file context>
@@ -0,0 +1,140 @@
+ let reason: ReapReason = "terminated-sigterm";
+ try {
+ deps.killProcess(pid, "SIGTERM");
+ } catch {
+ // Exited between the liveness check and the signal — nothing to do.
+ deps.removeManifest(organizationId);
</file context>
| if (process.platform === "win32") return false; | ||
| try { | ||
| const command = childProcess | ||
| .execFileSync("ps", ["-p", String(pid), "-o", "command="], { |
There was a problem hiding this comment.
P2: execFileSync blocks the Electron main-process event loop for up to 1.5 s. Since the caller (confirmOurHostService) is already async, replace with the promisified execFile + await to avoid UI jank under pathological load.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/main/lib/host-service-coordinator.ts, line 591:
<comment>`execFileSync` blocks the Electron main-process event loop for up to 1.5 s. Since the caller (`confirmOurHostService`) is already `async`, replace with the promisified `execFile` + `await` to avoid UI jank under pathological load.</comment>
<file context>
@@ -533,6 +578,27 @@ function pipeWithPrefix(
+ if (process.platform === "win32") return false;
+ try {
+ const command = childProcess
+ .execFileSync("ps", ["-p", String(pid), "-o", "command="], {
+ encoding: "utf8",
+ timeout: 1_500,
</file context>
Kitenite
left a comment
Kitenite
left a comment
There was a problem hiding this comment.
Automated review (high effort) of the reap / single-writer change. Solid and well-factored — the dependency injection makes the core logic genuinely unit-testable. One finding worth addressing before merge (the ps-fallback friendly-fire); the rest are minor.
Non-blocking notes:
- The
isOwnLivePidguard (coordinator ~357, reap.ts) is effectively unreachable on thespawn()path:this.instances.setruns after reap, so the org has no tracked instance at reap time. Harmless and fine as defense-in-depth, but currently dead. pollHealthCheck's refactor preserves the prior per-attempt-2s / overall-deadline behavior exactly — no regression there.
| // Fallback for a wedged child whose endpoint no longer responds: match | ||
| // the pid's command line to our host-service script. POSIX only — on | ||
| // win32 we fall back to health-only and leave unrecognized pids alone. | ||
| return processLooksLikeHostService(manifest.pid, this.scriptPath); |
There was a problem hiding this comment.
The health probe (line 373) is precise — it uses the manifest secret, so it confirms this specific host-service. This ps fallback is not: it matches any process whose command line contains host-service.js, and the org is passed via env (ORGANIZATION_ID, ~line 500), not argv — so ps -o command can't tell orgs apart.
Friendly-fire scenario: org A's manifest pid is dead; the OS reuses that pid for org B's host-service; the health probe to A's stale endpoint fails → this fallback matches host-service.js → we SIGTERM/SIGKILL org B's healthy host-service. That undercuts the PID-reuse safety the health probe is supposed to give.
Options: drop the argv fallback and rely on the health probe alone (a wedged-ours child that never answers health is rare, and reset() still covers it), or make the fallback org-specific (hard, since org isn't in argv). Minor, same line: execFileSync is synchronous, so this blocks the Electron main event loop for up to the 1.5s ps timeout whenever the health probe fails.
| } catch { | ||
| // Exited between the liveness check and the signal — nothing to do. | ||
| deps.removeManifest(organizationId); | ||
| return { reaped: true, reason: "terminated-sigterm", pid }; |
There was a problem hiding this comment.
Returning reaped: true / "terminated-sigterm" here is slightly misleading: killProcess threw, which (per the comment above) means the process had already exited on its own — we didn't terminate it. Harmless since the manifest is removed either way, but a distinct reason like "already-exited" would keep the result/telemetry honest.
| expect(deps.removeManifest).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| test("alive + ours + ignores SIGTERM → escalates to SIGKILL", async () => { |
There was a problem hiding this comment.
Two branches stay uncovered: (a) the SIGTERM-throws path in reap.ts (killProcess is never mocked to throw) — a case asserting it still returns reaped:true + removes the manifest would pin it; and (b) the real confirmOurHostService identity logic is injected as a fake, so the ps/health fallback (and the friendly-fire risk flagged in the coordinator) has no test at all. Worth a focused test of the real identity probe.
The fail-closed behavior was the swallowing try/catch all along. Delete it instead of rewriting it: migrate() now throws naturally to serve.ts main().catch -> exit(1). Drops the re-throw wrapper and sqlite.close() (the OS reclaims the fd on exit). Keep the one genuine addition, busy_timeout = 8000, and document that it applies for the connection lifetime. Export MIGRATION_BUSY_TIMEOUT_MS and import it in the contention test, deleting the hand-mirrored copy so the budget can't drift. Plan reframed removal-first: drops the redundant retry loop (busy_timeout already waits) and notes the structural fix (#4997) lets busy_timeout and the widened health window be removed later.
Problem
The coordinator only tracks host-services it spawned in this Electron process (its in-memory
instancesmap). After a restart that map is empty, so a host-service left alive by a previous process is invisible — andspawn()never consults the manifest before starting a new one. It just binds a competitor on a fresh port, giving two processes connected to the same orghost.db. That concurrent-writer contention is the root trigger behind the migration lock-up (see #4996, which hardens the migration side).How an orphan arises:
before-quitran (hard crash / SIGKILL / OOM) — child never SIGTERMed, parent watchdog hadn't fired yet;HOST_PARENT_PID, so no watchdog at all;(
reset()already reaps via the manifest, but it's only reachable manually via tRPC — never on the normal start path.)Fix
Reap based on the manifest (the only cross-process record) before spawning, in
spawn():health.checkwith the manifest secret, or (best-effort, POSIX) its command line matches ourhost-service.js. A live-but-unrecognized pid is left alone and the stale manifest dropped, so a recycled pid can't cause friendly-fire.Why safe to kill: host-service is stateless w.r.t. terminals — PTYs live in the detached pty-daemon, which a fresh host-service re-adopts. Reaping an orphan doesn't touch user sessions.
Structure
host-service-reap.ts— dependency-injected reap logic (electron-free, so it unit-tests underbun testwithout spawning real processes).host-service-coordinator.ts— wires real deps + the health/argv identity probe; calls reap at the top ofspawn().host-service-utils.ts— addshealthCheckOnce(single-shot identity probe;pollHealthChecknow reuses it).host-service-reap.test.ts— 6 cases: no-manifest, own-live-instance, dead-pid-cleared, unidentified-left-alone (PID reuse), SIGTERM-exit, SIGTERM→SIGKILL escalation.Desktop typecheck + lint clean; reap tests 6/6.
Relationship to #4996
Independent and complementary. #4996 makes the DB migration survive contention (busy_timeout + fail-closed); this PR removes the source of the contention. Either can merge first.
Summary by cubic
Prevents two host-service processes from writing to the same org DB by reaping any orphaned host-service before spawn. Uses manifest-driven identity checks to safely terminate previous instances and ensure a single writer, resolving migration contention.
spawn()using the manifest.health.checkwith the manifest secret, with a POSIX argv fallback; unrecognized live PIDs are left alone and the stale manifest is removed.SIGTERMwith grace, thenSIGKILLif needed; clears the manifest after successful reaping or when the PID is dead.healthCheckOnceand updatedpollHealthCheckto reuse it.host-service-reap.tswith unit tests covering no-manifest, own-live, dead-pid, PID-reuse, and SIGTERM→SIGKILL paths.Written for commit 19143e5. Summary will update on new commits.
Summary by CodeRabbit
Improvements
Tests