refactor(desktop): host-service detach — rotation, perms, windowsHide, dev pipes#3616
Conversation
The host-service child was spawned without `detached: true`, so it
shared the Electron main's process group. When Squirrel relaunched
the app on auto-update, the old app's process group got killed —
taking the host-service and all its PTYs with it. The whole
manifest-adoption design in HOST_SERVICE_LIFECYCLE.md assumed the
child survives, but the spawn options contradicted that.
Mirror the v1 terminal-host daemon pattern (client.ts:1160-1221):
`detached: true` in prod, stdio pointed at a per-org rotating log
file at `~/.superset/host/{orgId}/host.log` (piped stdio would EPIPE
once the parent exits). Dev mode keeps pipes for live console logs
since enableDevReload restarts instances on bundle rebuild anyway.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors HostServiceCoordinator to use shared utilities for port selection and health polling, changes spawn behavior based on Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
participant Coordinator
end
participant FS as "Filesystem"
participant Child as "Host Service (child)"
participant Health as "Child HTTP /health"
Coordinator->>FS: openRotatingLogFd(path, MAX_HOST_LOG_BYTES)
FS-->>Coordinator: fd or -1
Coordinator->>Coordinator: findFreePort()
Coordinator-->>Child: spawn(port, detached?, stdio->pipes|fd, windowsHide)
alt non-packaged (dev)
Child->>Coordinator: stdout/stderr chunks
Coordinator-->>Coordinator: forward to Electron console
else packaged
Child->>FS: write to log fd (rotating)
end
Coordinator->>Health: pollHealthCheck(endpoint, secret, HEALTH_POLL_TIMEOUT_MS)
loop until success or timeout
Health-->>Coordinator: HTTP OK / not-OK
end
Coordinator->>Child: on timeout -> log failure using HEALTH_POLL_TIMEOUT_MS
Coordinator->>FS: best-effort close(fd) when applicable
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
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 |
Greptile SummaryThis PR fixes a critical bug where the v2 host-service child process was killed during Electron auto-updates because it shared the parent app's process group. The fix spawns the host-service with Key changes:
Confidence Score: 4/5Safe to merge — the fix is correct and well-reasoned; two non-blocking P2 suggestions remain. Core logic is sound: detached spawn + real fds + unref() is the standard pattern for daemon-style child processes. The try/finally for fd cleanup is correct. Dev mode is unaffected. Only minor improvements remain around windowsHide for Windows UX and log rotation preserving history. No files require special attention; both P2s are in host-service-coordinator.ts.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/main/lib/host-service-coordinator.ts | Adds detached: true in prod so the host-service survives auto-update relaunch; introduces per-org rotating log file for prod stdio; keeps piped stdio in dev. Two minor P2s: missing windowsHide: true and truncation-only log rotation. |
Sequence Diagram
sequenceDiagram
participant OldApp as Electron Main (old)
participant HSC as HostServiceCoordinator
participant HS as host-service (detached)
participant Updater as Squirrel Updater
participant NewApp as Electron Main (new)
participant HSC2 as HostServiceCoordinator (new)
OldApp->>HSC: spawn(orgId, config)
HSC->>HS: "childProcess.spawn(detached=true, stdio to host.log)"
Note over HS: "setsid() - new process group leader"
HSC->>HSC: "close parent fd copy after spawn"
HSC->>HS: child.unref()
Updater->>OldApp: quitAndInstall()
Note over OldApp: "Old process group killed"
Note over HS: "Survives in own session - PTYs intact"
NewApp->>HSC2: discoverAll()
HSC2->>HSC2: listManifests()
HSC2->>HS: "tryAdopt - health check"
HS-->>HSC2: 200 OK
HSC2->>HSC2: "instances.set with adopted pid and port"
Note over HSC2: "Adopted pid=X logged"
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/lib/host-service-coordinator.ts
Line: 454-458
Comment:
**Missing `windowsHide` for detached Windows processes**
On Windows, `detached: true` can cause a new console window to flash briefly (or remain visible) when the subprocess is created, since Electron's main process is a GUI process but the spawned Node process is a console subsystem process. Adding `windowsHide: true` suppresses this. Without it, users on Windows may see a transient CMD/terminal window on app launch or after auto-update.
```suggestion
child = childProcess.spawn(process.execPath, [this.scriptPath], {
detached: !isDev,
stdio,
env: childEnv,
windowsHide: true,
});
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/desktop/src/main/lib/host-service-coordinator.ts
Line: 65-83
Comment:
**Truncation-based rotation loses all previous logs**
The current rotation strategy overwrites the file with an empty string when it exceeds `MAX_HOST_LOG_BYTES`, meaning the entire log history (up to 5 MB) is discarded at once. A simple improvement is to rename the existing file to `host.log.1` before truncating, giving users a one-rotation window for debugging:
```typescript
if (size > maxBytes) {
try {
fs.renameSync(logPath, logPath + ".1");
} catch {
// Fall back to truncating in-place
fs.writeFileSync(logPath, "", { mode: 0o600 });
}
}
```
This is non-blocking to merge, but worth considering since post-crash diagnosis often relies on the rotated file.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(desktop): keep v2 host-service alive..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| child = childProcess.spawn(process.execPath, [this.scriptPath], { | ||
| detached: !isDev, | ||
| stdio, | ||
| env: childEnv, | ||
| }); |
There was a problem hiding this comment.
windowsHide for detached Windows processes
On Windows, detached: true can cause a new console window to flash briefly (or remain visible) when the subprocess is created, since Electron's main process is a GUI process but the spawned Node process is a console subsystem process. Adding windowsHide: true suppresses this. Without it, users on Windows may see a transient CMD/terminal window on app launch or after auto-update.
| child = childProcess.spawn(process.execPath, [this.scriptPath], { | |
| detached: !isDev, | |
| stdio, | |
| env: childEnv, | |
| }); | |
| child = childProcess.spawn(process.execPath, [this.scriptPath], { | |
| detached: !isDev, | |
| stdio, | |
| env: childEnv, | |
| windowsHide: true, | |
| }); |
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: 454-458
Comment:
**Missing `windowsHide` for detached Windows processes**
On Windows, `detached: true` can cause a new console window to flash briefly (or remain visible) when the subprocess is created, since Electron's main process is a GUI process but the spawned Node process is a console subsystem process. Adding `windowsHide: true` suppresses this. Without it, users on Windows may see a transient CMD/terminal window on app launch or after auto-update.
```suggestion
child = childProcess.spawn(process.execPath, [this.scriptPath], {
detached: !isDev,
stdio,
env: childEnv,
windowsHide: true,
});
```
How can I resolve this? If you propose a fix, please make it concise.| function openRotatingLogFd(logPath: string, maxBytes: number): number { | ||
| try { | ||
| fs.mkdirSync(path.dirname(logPath), { recursive: true, mode: 0o700 }); | ||
| if (fs.existsSync(logPath)) { | ||
| try { | ||
| const { size } = fs.statSync(logPath); | ||
| if (size > maxBytes) { | ||
| fs.writeFileSync(logPath, "", { mode: 0o600 }); | ||
| } | ||
| } catch { | ||
| // Best-effort rotate | ||
| } | ||
| } | ||
| return fs.openSync(logPath, "a", 0o600); | ||
| } catch (error) { | ||
| console.warn(`[host-service] Failed to open log file ${logPath}: ${error}`); | ||
| return -1; | ||
| } | ||
| } |
There was a problem hiding this comment.
The current rotation strategy overwrites the file with an empty string when it exceeds MAX_HOST_LOG_BYTES, meaning the entire log history (up to 5 MB) is discarded at once. A simple improvement is to rename the existing file to host.log.1 before truncating, giving users a one-rotation window for debugging:
if (size > maxBytes) {
try {
fs.renameSync(logPath, logPath + ".1");
} catch {
// Fall back to truncating in-place
fs.writeFileSync(logPath, "", { mode: 0o600 });
}
}This is non-blocking to merge, but worth considering since post-crash diagnosis often relies on the rotated file.
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: 65-83
Comment:
**Truncation-based rotation loses all previous logs**
The current rotation strategy overwrites the file with an empty string when it exceeds `MAX_HOST_LOG_BYTES`, meaning the entire log history (up to 5 MB) is discarded at once. A simple improvement is to rename the existing file to `host.log.1` before truncating, giving users a one-rotation window for debugging:
```typescript
if (size > maxBytes) {
try {
fs.renameSync(logPath, logPath + ".1");
} catch {
// Fall back to truncating in-place
fs.writeFileSync(logPath, "", { mode: 0o600 });
}
}
```
This is non-blocking to merge, but worth considering since post-crash diagnosis often relies on the rotated file.
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 the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/lib/host-service-coordinator.ts`:
- Around line 65-78: openRotatingLogFd currently only sets 0o600 when creating a
new file, leaving existing host.log with its old mode; to normalize permissions
like the daemon log pattern, after opening the file in openRotatingLogFd call
fs.chmodSync(logPath, 0o600) (wrapped in a try/catch to preserve best-effort
behavior) so both newly created and pre-existing log files end up with 0o600
before returning the fd; reference function name openRotatingLogFd and the
logPath variable when making this change.
- Around line 431-456: The code uses process.env.NODE_ENV (isDev) to decide
whether to detach the child process; change this to use Electron's packaging
flag instead: import Electron's app and replace the isDev check with a check on
app.isPackaged (or an inverted name like isPackaged) so the detach/stdio logic
is driven by app.isPackaged rather than NODE_ENV; update references around the
stdio/logFd/detached logic in host-service-coordinator.ts (the block that
computes isDev, logFd, stdio and spawns the child via childProcess.spawn) so
packaged apps detach regardless of NODE_ENV.
🪄 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: 6e1acd88-543c-4921-8ba2-a5ed33f50722
📒 Files selected for processing (1)
apps/desktop/src/main/lib/host-service-coordinator.ts
- chmodSync log file after open: openSync's mode arg only applies on create, so a pre-existing file rotated out-of-band keeps old perms. - windowsHide: true on the spawn so the detached Node child doesn't flash a CMD window on Windows.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/desktop/src/main/lib/host-service-coordinator.ts (1)
439-464:⚠️ Potential issue | 🟠 MajorGate detach on
app.isPackaged, notNODE_ENV.Per the earlier review on this hunk: the whole point of this PR is that the host-service must detach in the packaged/updateable app. Tying that to
process.env.NODE_ENVmeans a packaged build launched withNODE_ENV=development(e.g., by a wrapper, CI smoke, or a user who exported it in their shell) silently falls back to piped stdio + non-detached, which reintroduces the exact auto-updater kill chain this PR fixes.appis already imported (line 9, used at 548).🐛 Proposed fix
- const childEnv = await this.buildEnv(organizationId, port, secret, config); - const isDev = process.env.NODE_ENV === "development"; + const childEnv = await this.buildEnv(organizationId, port, secret, config); + const isDev = !app.isPackaged;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/host-service-coordinator.ts` around lines 439 - 464, Replace the current NODE_ENV-based gating with the Electron packaging flag so detaching and log/stdio choices use app.isPackaged: change the isDev check (currently "process.env.NODE_ENV === 'development'") to use app.isPackaged (or invert as needed), then use that value when computing logFd, stdio, and the detached option passed to childProcess.spawn (referencing symbols isDev/logFd/stdio/childProcess.spawn/scriptPath/buildEnv). Ensure the code detaches when app.isPackaged is true (and keeps pipes when not packaged); app is already imported near the top, so use that import.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/desktop/src/main/lib/host-service-coordinator.ts`:
- Around line 439-464: Replace the current NODE_ENV-based gating with the
Electron packaging flag so detaching and log/stdio choices use app.isPackaged:
change the isDev check (currently "process.env.NODE_ENV === 'development'") to
use app.isPackaged (or invert as needed), then use that value when computing
logFd, stdio, and the detached option passed to childProcess.spawn (referencing
symbols isDev/logFd/stdio/childProcess.spawn/scriptPath/buildEnv). Ensure the
code detaches when app.isPackaged is true (and keeps pipes when not packaged);
app is already imported near the top, so use that import.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8d0cf3d3-5cbb-4913-ad3f-18fe4d8aa47a
📒 Files selected for processing (1)
apps/desktop/src/main/lib/host-service-coordinator.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
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:83">
P2: Do not silently swallow `chmodSync` failures here; log a warning so permission-hardening failures are observable.
(Based on your team's feedback about avoiding silent catch blocks that hide failures.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/desktop/src/main/lib/host-service-coordinator.ts (1)
368-397:⚠️ Potential issue | 🟠 MajorDrive the detach decision off
app.isPackaged, notNODE_ENV.The whole point of this PR is for the packaged/auto-updating build to keep the host-service alive across relaunch.
process.env.NODE_ENVis ambient and can be"development"in a packaged app (inherited shell env, debug launches, etc.) — in that caseisDevistrue, stdio stays piped,detachedisfalse, and the child dies with the old process group onquitAndInstall, defeating manifest re-adoption. Electron'sapp.isPackagedis the authoritative signal for "running from an installed bundle."🐛 Proposed fix
- const childEnv = await this.buildEnv(organizationId, port, secret, config); - const isDev = process.env.NODE_ENV === "development"; + const childEnv = await this.buildEnv(organizationId, port, secret, config); + const isDev = !app.isPackaged;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/host-service-coordinator.ts` around lines 368 - 397, The detach/stdio decision currently uses process.env.NODE_ENV (isDev) which is unreliable; change it to use Electron's app.isPackaged so the child is detached only for packaged apps. Replace the isDev definition with a boolean based on app.isPackaged (e.g. const isPackaged = app.isPackaged) and flip the logic where you decide logFd, stdio and the detached option in childProcess.spawn (use detached: isPackaged and non-piped stdio when packaged, keep pipes when not packaged). Ensure the module imports or accesses Electron's app and update references to isDev accordingly (look for isDev, logFd, stdio, and the childProcess.spawn call).
🧹 Nitpick comments (2)
apps/desktop/src/main/lib/host-service-utils.ts (2)
60-82: Minor:clearTimeoutskipped whenfetchrejects.In the catch branch the 2s abort timer is never cleared, so each failed attempt keeps a pending timer alive until it fires (and aborts an already-settled controller). Harmless, but in a tight 200ms poll loop against a not-yet-listening endpoint (ECONNREFUSED resolves fast), you'll briefly stack ~10 timers. Move
clearTimeoutinto afinallyso it runs on both success and failure paths.♻️ Suggested tweak
- try { - const controller = new AbortController(); - const timeout = setTimeout(() => controller.abort(), 2_000); - const res = await fetch(`${endpoint}/trpc/health.check`, { - signal: controller.signal, - headers: { Authorization: `Bearer ${secret}` }, - }); - clearTimeout(timeout); - if (res.ok) return true; - } catch { - // Not ready yet - } + const controller = new AbortController(); + const timeout = setTimeout(() => controller.abort(), 2_000); + try { + const res = await fetch(`${endpoint}/trpc/health.check`, { + signal: controller.signal, + headers: { Authorization: `Bearer ${secret}` }, + }); + if (res.ok) return true; + } catch { + // Not ready yet + } finally { + clearTimeout(timeout); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/host-service-utils.ts` around lines 60 - 82, The pollHealthCheck function creates a 2s abort timer for each fetch but only calls clearTimeout(timeout) on the success path, leaving timers alive when fetch throws; fix by moving clearTimeout(timeout) into a finally block that runs after the try/catch so the timeout is always cleared (refer to pollHealthCheck, the AbortController instance named controller, the timeout variable, and the fetch call to /trpc/health.check) ensuring clearTimeout(timeout) executes regardless of success or failure.
19-28: Rotation drops all prior log history.When
host.logexceedsmaxBytesyou truncate in place, losing every line written before the rotation — exactly the history you'd want when debugging a host-service that just auto-updated. Consider a simple one-file rollover (host.log→host.log.1) before reopening. Keeping one previous file preserves the pre-rotation tail without unbounded growth.♻️ Suggested rollover
- if (fs.existsSync(logPath)) { - try { - const { size } = fs.statSync(logPath); - if (size > maxBytes) { - fs.writeFileSync(logPath, "", { mode: 0o600 }); - } - } catch { - // Best-effort rotate - } - } + if (fs.existsSync(logPath)) { + try { + const { size } = fs.statSync(logPath); + if (size > maxBytes) { + const rotated = `${logPath}.1`; + try { + fs.rmSync(rotated, { force: true }); + } catch { + // Best-effort + } + fs.renameSync(logPath, rotated); + } + } catch { + // Best-effort rotate + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/host-service-utils.ts` around lines 19 - 28, Currently the code truncates logPath in place which drops all prior history; instead implement a single-file rollover: if fs.existsSync(logPath) and size > maxBytes, attempt to rename logPath to `${logPath}.1` (use fs.renameSync) — if `${logPath}.1` already exists, unlink it first (fs.unlinkSync) or replace it safely inside a try/catch — then create a new empty file at logPath with the same permissions (fs.writeFileSync(logPath, "", { mode: 0o600 })) so the logger can continue writing; fall back to the existing truncation behavior on any rename/unlink error to preserve the current best-effort behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/desktop/src/main/lib/host-service-coordinator.ts`:
- Around line 368-397: The detach/stdio decision currently uses
process.env.NODE_ENV (isDev) which is unreliable; change it to use Electron's
app.isPackaged so the child is detached only for packaged apps. Replace the
isDev definition with a boolean based on app.isPackaged (e.g. const isPackaged =
app.isPackaged) and flip the logic where you decide logFd, stdio and the
detached option in childProcess.spawn (use detached: isPackaged and non-piped
stdio when packaged, keep pipes when not packaged). Ensure the module imports or
accesses Electron's app and update references to isDev accordingly (look for
isDev, logFd, stdio, and the childProcess.spawn call).
---
Nitpick comments:
In `@apps/desktop/src/main/lib/host-service-utils.ts`:
- Around line 60-82: The pollHealthCheck function creates a 2s abort timer for
each fetch but only calls clearTimeout(timeout) on the success path, leaving
timers alive when fetch throws; fix by moving clearTimeout(timeout) into a
finally block that runs after the try/catch so the timeout is always cleared
(refer to pollHealthCheck, the AbortController instance named controller, the
timeout variable, and the fetch call to /trpc/health.check) ensuring
clearTimeout(timeout) executes regardless of success or failure.
- Around line 19-28: Currently the code truncates logPath in place which drops
all prior history; instead implement a single-file rollover: if
fs.existsSync(logPath) and size > maxBytes, attempt to rename logPath to
`${logPath}.1` (use fs.renameSync) — if `${logPath}.1` already exists, unlink it
first (fs.unlinkSync) or replace it safely inside a try/catch — then create a
new empty file at logPath with the same permissions (fs.writeFileSync(logPath,
"", { mode: 0o600 })) so the logger can continue writing; fall back to the
existing truncation behavior on any rename/unlink error to preserve the current
best-effort behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7c8da1fa-a77a-487f-a462-76d844dedf4d
📒 Files selected for processing (2)
apps/desktop/src/main/lib/host-service-coordinator.tsapps/desktop/src/main/lib/host-service-utils.ts
Reconcile with #3620 which landed a simpler version of the same fix on main. Keep our implementation (dev/prod split, log rotation, chmod for pre-existing files, windowsHide, utils extraction) and pick up the `host-service/index.ts` change: drop `removeManifest` from the child's SIGTERM handler — manifest lifecycle belongs to the coordinator. Align log filename to `host-service.log` to match what landed on main.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/desktop/src/main/lib/host-service-coordinator.ts (1)
369-393:⚠️ Potential issue | 🟠 MajorDrive the detach decision from
app.isPackaged.Line 369 uses ambient
NODE_ENVto decide Line 392’sdetachedvalue. A packaged app launched withNODE_ENV=developmentwould keep piped stdio and skip detaching, so the host-service would not survive the updater relaunch.🐛 Proposed fix
- const isDev = process.env.NODE_ENV === "development"; + const isDev = !app.isPackaged;#!/bin/bash # Verify the packaged-app detach decision is not tied to ambient NODE_ENV. rg -n -C4 'const isDev = process\.env\.NODE_ENV|const isDev = !app\.isPackaged|detached: !isDev' apps/desktop/src/main/lib/host-service-coordinator.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/host-service-coordinator.ts` around lines 369 - 393, Change the dev/packaged detection so detach is driven by Electron's app.isPackaged: set isDev to process.env.NODE_ENV === "development" && !app.isPackaged (so a packaged app with NODE_ENV=development is not treated as dev), and use app.isPackaged (or !app.isPackaged as appropriate) for the spawn option instead of !isDev—i.e., replace detached: !isDev with detached: app.isPackaged and keep stdio logic using the revised isDev; update references to isDev and detached where they appear (the const isDev declaration and the childProcess.spawn options).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/lib/host-service-coordinator.ts`:
- Around line 377-382: The current rotation call with openRotatingLogFd(...)
enforces MAX_HOST_LOG_BYTES only in the parent before spawning, but the detached
child inherits the fd and can grow the file unbounded; move log-rotation
responsibility into the spawned host service process so the process that owns
the FD enforces rotation for its lifetime. Modify the coordinator to pass a
filepath (path.join(manifestDir(organizationId), "host-service.log")) and
MAX_HOST_LOG_BYTES to the host-service entrypoint instead of pre-opening the FD
with openRotatingLogFd, update the host-service startup (the child process code
that runs as the service) to call openRotatingLogFd or equivalent rotation logic
on startup and keep the FD open and managed there, and remove/replace the
parent-side use of the returned logFd to avoid orphaned-unrotated files. Ensure
the unique symbols to change are openRotatingLogFd, MAX_HOST_LOG_BYTES,
manifestDir(organizationId), and the host-service process start routine so
rotation runs inside the spawned process.
---
Duplicate comments:
In `@apps/desktop/src/main/lib/host-service-coordinator.ts`:
- Around line 369-393: Change the dev/packaged detection so detach is driven by
Electron's app.isPackaged: set isDev to process.env.NODE_ENV === "development"
&& !app.isPackaged (so a packaged app with NODE_ENV=development is not treated
as dev), and use app.isPackaged (or !app.isPackaged as appropriate) for the
spawn option instead of !isDev—i.e., replace detached: !isDev with detached:
app.isPackaged and keep stdio logic using the revised isDev; update references
to isDev and detached where they appear (the const isDev declaration and the
childProcess.spawn options).
🪄 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: 703a37b7-ee5a-4add-b8e0-774175963294
📒 Files selected for processing (2)
apps/desktop/src/main/lib/host-service-coordinator.tsapps/desktop/src/main/lib/host-service-utils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/main/lib/host-service-utils.ts
| const logFd = isDev | ||
| ? -1 | ||
| : openRotatingLogFd( | ||
| path.join(manifestDir(organizationId), "host-service.log"), | ||
| MAX_HOST_LOG_BYTES, | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the log filename and whether rotation is only applied before spawn.
sed -n '16,42p' apps/desktop/src/main/lib/host-service-utils.ts
sed -n '377,405p' apps/desktop/src/main/lib/host-service-coordinator.ts
rg -n '"host\.log"|"host-service\.log"|MAX_HOST_LOG_BYTES|openRotatingLogFd' apps/desktop/src/main/lib packages/host-service --type tsRepository: superset-sh/superset
Length of output: 2257
🏁 Script executed:
#!/bin/bash
# Search for test files and specifications related to host-service log filename
fd -e test.ts -e spec.ts -e test.tsx | xargs rg -l "host.*log|host-service.*log" 2>/dev/null | head -20Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Search for any references to the expected log filename in tests, fixtures, or docs
rg -n 'host\.log|"host-service\.log"' --type ts --type tsx apps/desktop packages/host-service -B 2 -A 2Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Check if there's a test file for host-service-coordinator
find . -name '*host-service-coordinator*test*' -o -name '*host-service-coordinator*spec*' | head -10Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Search for any references to either filename (broader search, all file types)
rg -i 'host-service\.log|host\.log' apps/desktop packages/host-serviceRepository: superset-sh/superset
Length of output: 296
🏁 Script executed:
#!/bin/bash
# Check for any PR, issue, or documentation comments about the log filename requirement
rg -n 'host.*service.*log|rotating.*log|MAX_HOST_LOG_BYTES' apps/desktop/src/main/lib/host-service-coordinator.ts -B 5 -A 5Repository: superset-sh/superset
Length of output: 836
🏁 Script executed:
#!/bin/bash
# Look for any test or spec files with different naming patterns
find . -type f \( -name '*.test.ts' -o -name '*.spec.ts' -o -name '*.test.tsx' -o -name '*.spec.tsx' \) | xargs grep -l "host.*log" 2>/dev/nullRepository: superset-sh/superset
Length of output: 48
Address log rotation enforcement after child inherits the file descriptor.
openRotatingLogFd() only enforces MAX_HOST_LOG_BYTES before spawn; after the detached child inherits the fd, a surviving/adopted service can grow the log past 5MB indefinitely. Consider moving rotation into the host-service process that owns the fd for its lifetime.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/lib/host-service-coordinator.ts` around lines 377 -
382, The current rotation call with openRotatingLogFd(...) enforces
MAX_HOST_LOG_BYTES only in the parent before spawning, but the detached child
inherits the fd and can grow the file unbounded; move log-rotation
responsibility into the spawned host service process so the process that owns
the FD enforces rotation for its lifetime. Modify the coordinator to pass a
filepath (path.join(manifestDir(organizationId), "host-service.log")) and
MAX_HOST_LOG_BYTES to the host-service entrypoint instead of pre-opening the FD
with openRotatingLogFd, update the host-service startup (the child process code
that runs as the service) to call openRotatingLogFd or equivalent rotation logic
on startup and keep the FD open and managed there, and remove/replace the
parent-side use of the returned logFd to avoid orphaned-unrotated files. Ensure
the unique symbols to change are openRotatingLogFd, MAX_HOST_LOG_BYTES,
manifestDir(organizationId), and the host-service process start routine so
rotation runs inside the spawned process.
- Gate detach on app.isPackaged instead of NODE_ENV. NODE_ENV is ambient and could silently flip detach off in a packaged app (e.g. shell env), reintroducing the Squirrel kill-chain this PR exists to fix. - Log a warning on chmodSync failure so permission issues are observable instead of being swallowed silently. - Move clearTimeout into finally in pollHealthCheck so the 2s abort timer is also cleared when fetch rejects, not just on success.
Kitenite
changed the title
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/desktop/src/main/lib/host-service-coordinator.ts (1)
382-386:⚠️ Potential issue | 🟠 MajorKeep the log contract with the long-lived service.
openRotatingLogFd()enforces size only before spawn; after the detached child inherits the fd and survives app updates, the file can grow pastMAX_HOST_LOG_BYTESuntil the service is restarted. Also, this writeshost-service.log, while the PR objective/test plan calls outhost.log.Consider moving rotation/reopen ownership into the host-service process and aligning the filename with the documented path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/host-service-coordinator.ts` around lines 382 - 386, The coordinator currently opens a rotating fd via openRotatingLogFd (using manifestDir(organizationId) and MAX_HOST_LOG_BYTES) and hands it to the detached host service, which prevents ongoing rotation/reopen after the child outlives the parent and also uses host-service.log instead of host.log; instead, stop opening/rotating the file in host-service-coordinator.ts and change the coordinator to pass a simple path/filename (use host.log) or an instruction flag to the host service, then move rotation and reopen logic into the host-service process so the long-lived service owns enforcement of MAX_HOST_LOG_BYTES, log rotation and filename (refer to openRotatingLogFd, manifestDir, MAX_HOST_LOG_BYTES, and the detached host service spawn logic).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/desktop/src/main/lib/host-service-coordinator.ts`:
- Around line 382-386: The coordinator currently opens a rotating fd via
openRotatingLogFd (using manifestDir(organizationId) and MAX_HOST_LOG_BYTES) and
hands it to the detached host service, which prevents ongoing rotation/reopen
after the child outlives the parent and also uses host-service.log instead of
host.log; instead, stop opening/rotating the file in host-service-coordinator.ts
and change the coordinator to pass a simple path/filename (use host.log) or an
instruction flag to the host service, then move rotation and reopen logic into
the host-service process so the long-lived service owns enforcement of
MAX_HOST_LOG_BYTES, log rotation and filename (refer to openRotatingLogFd,
manifestDir, MAX_HOST_LOG_BYTES, and the detached host service spawn logic).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3d261b1f-a6c5-4028-bd31-d745d0ee29ba
📒 Files selected for processing (2)
apps/desktop/src/main/lib/host-service-coordinator.tsapps/desktop/src/main/lib/host-service-utils.ts
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/main/lib/host-service-utils.ts
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
1 participant
Summary
Builds on #3620, which landed the core fix (spawn host-service with
detached: true+ file-backed stdio so it survives Squirrel's process-group SIGTERM on auto-update). This PR adds hardening and cleanup on top:app.isPackaged, notNODE_ENV. Packaged builds detach and log to file; unpackaged (dev) keeps piped stdio so logs stream to the Electron console. NODE_ENV is ambient and could silently flip detach off in a packaged app —app.isPackagedis the authoritative signal.~/.superset/host/{orgId}/host-service.logis truncated once it exceeds 5MB (mirrorsMAX_DAEMON_LOG_BYTESinterminal-host/client.ts). Without rotation, a long-lived detached host-service's log grows unbounded.chmodSync.openSync's mode arg only applies on create, so a pre-existing file rotated out-of-band keeps old perms. Warn on failure.windowsHide: trueon the spawn so the detached Node child doesn't flash a CMD window on Windows.bun devandenableDevReloadstay ergonomic.openRotatingLogFd,findFreePort,pollHealthCheck) tohost-service-utils.tsto keep the coordinator focused on lifecycle.clearTimeoutnow runs infinallyinpollHealthCheckso the 2s abort timer is cleared on fetch failure too.Test plan
discoverAll(); PTYs stay alive.~/.superset/host/{orgId}/host-service.logis created at 0o600; verify truncation past 5MB.NODE_ENV=developmentexported in shell: still detaches and writes to log file (provesapp.isPackagedgate).bun dev): host-service logs still stream to Electron console;enableDevReloadrestart on source edit still works.Summary by CodeRabbit
New Features
Bug Fixes