-
Notifications
You must be signed in to change notification settings - Fork 962
refactor(desktop): host-service detach — rotation, perms, windowsHide, dev pipes #3616
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
f7c0d56
0979a05
fc39b98
f79f57d
3bd9c1b
affabbc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,7 +2,6 @@ import * as childProcess from "node:child_process"; | |||||||||||||||||||||||
| import { randomBytes } from "node:crypto"; | ||||||||||||||||||||||||
| import { EventEmitter } from "node:events"; | ||||||||||||||||||||||||
| import * as fs from "node:fs"; | ||||||||||||||||||||||||
| import { createServer } from "node:net"; | ||||||||||||||||||||||||
| import path from "node:path"; | ||||||||||||||||||||||||
| import { settings } from "@superset/local-db"; | ||||||||||||||||||||||||
| import { getDeviceName, getHashedDeviceId } from "@superset/shared/device-info"; | ||||||||||||||||||||||||
|
|
@@ -19,6 +18,13 @@ import { | |||||||||||||||||||||||
| readManifest, | ||||||||||||||||||||||||
| removeManifest, | ||||||||||||||||||||||||
| } from "./host-service-manifest"; | ||||||||||||||||||||||||
| import { | ||||||||||||||||||||||||
| findFreePort, | ||||||||||||||||||||||||
| HEALTH_POLL_TIMEOUT_MS, | ||||||||||||||||||||||||
| MAX_HOST_LOG_BYTES, | ||||||||||||||||||||||||
| openRotatingLogFd, | ||||||||||||||||||||||||
| pollHealthCheck, | ||||||||||||||||||||||||
| } from "./host-service-utils"; | ||||||||||||||||||||||||
| import { localDb } from "./local-db"; | ||||||||||||||||||||||||
| import { HOOK_PROTOCOL_VERSION } from "./terminal/env"; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
|
|
@@ -51,62 +57,8 @@ interface HostServiceProcess { | |||||||||||||||||||||||
| status: HostServiceStatus; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| const HEALTH_POLL_INTERVAL = 200; | ||||||||||||||||||||||||
| const HEALTH_POLL_TIMEOUT = 10_000; | ||||||||||||||||||||||||
| const ADOPTED_LIVENESS_INTERVAL = 5_000; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| function openLogFile(organizationId: string): number { | ||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||
| const dir = manifestDir(organizationId); | ||||||||||||||||||||||||
| if (!fs.existsSync(dir)) { | ||||||||||||||||||||||||
| fs.mkdirSync(dir, { recursive: true, mode: 0o700 }); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| return fs.openSync(path.join(dir, "host-service.log"), "a", 0o600); | ||||||||||||||||||||||||
| } catch { | ||||||||||||||||||||||||
| return -1; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| async function findFreePort(): Promise<number> { | ||||||||||||||||||||||||
| return new Promise((resolve, reject) => { | ||||||||||||||||||||||||
| const server = createServer(); | ||||||||||||||||||||||||
| server.listen(0, "127.0.0.1", () => { | ||||||||||||||||||||||||
| const addr = server.address(); | ||||||||||||||||||||||||
| if (addr && typeof addr === "object") { | ||||||||||||||||||||||||
| const { port } = addr; | ||||||||||||||||||||||||
| server.close(() => resolve(port)); | ||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||
| server.close(() => reject(new Error("Could not get port"))); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||
| server.on("error", reject); | ||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| async function pollHealthCheck( | ||||||||||||||||||||||||
| endpoint: string, | ||||||||||||||||||||||||
| secret: string, | ||||||||||||||||||||||||
| timeoutMs = HEALTH_POLL_TIMEOUT, | ||||||||||||||||||||||||
| ): Promise<boolean> { | ||||||||||||||||||||||||
| const deadline = Date.now() + timeoutMs; | ||||||||||||||||||||||||
| while (Date.now() < deadline) { | ||||||||||||||||||||||||
| 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 | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| await new Promise((r) => setTimeout(r, HEALTH_POLL_INTERVAL)); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| export class HostServiceCoordinator extends EventEmitter { | ||||||||||||||||||||||||
| private instances = new Map<string, HostServiceProcess>(); | ||||||||||||||||||||||||
| private pendingStarts = new Map<string, Promise<Connection>>(); | ||||||||||||||||||||||||
|
|
@@ -413,24 +365,48 @@ export class HostServiceCoordinator extends EventEmitter { | |||||||||||||||||||||||
| this.instances.set(organizationId, instance); | ||||||||||||||||||||||||
| this.emitStatus(organizationId, "starting", null); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| const env = await this.buildEnv(organizationId, port, secret, config); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Detached + file-backed stdio so the child outlives the parent's process | ||||||||||||||||||||||||
| // group (Squirrel.Mac SIGTERMs it during updates) and doesn't depend on | ||||||||||||||||||||||||
| // parent-held stdout/stderr pipes. | ||||||||||||||||||||||||
| const logFd = openLogFile(organizationId); | ||||||||||||||||||||||||
| let child: childProcess.ChildProcess; | ||||||||||||||||||||||||
| const childEnv = await this.buildEnv(organizationId, port, secret, config); | ||||||||||||||||||||||||
| // Gate on app.isPackaged — the authoritative "running from an installed | ||||||||||||||||||||||||
| // bundle" signal. NODE_ENV is ambient (shell, wrappers, debug launches) | ||||||||||||||||||||||||
| // and could silently flip detach off in a packaged app, which would | ||||||||||||||||||||||||
| // re-introduce the exact Squirrel kill-chain this file exists to fix. | ||||||||||||||||||||||||
| const isPackaged = app.isPackaged; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // In packaged builds, detach so the child survives app relaunch: | ||||||||||||||||||||||||
| // auto-updater's quitAndInstall would otherwise take the host-service | ||||||||||||||||||||||||
| // (and its PTYs) down with the old app's process group. Stdio must | ||||||||||||||||||||||||
| // point at real fds — piped stdio would EPIPE once the parent exits. | ||||||||||||||||||||||||
| // Unpackaged (dev) keeps pipes so logs flow to the Electron console; | ||||||||||||||||||||||||
| // enableDevReload restarts instances on rebuild, so survival isn't | ||||||||||||||||||||||||
| // needed. | ||||||||||||||||||||||||
| const logFd = isPackaged | ||||||||||||||||||||||||
| ? openRotatingLogFd( | ||||||||||||||||||||||||
| path.join(manifestDir(organizationId), "host-service.log"), | ||||||||||||||||||||||||
| MAX_HOST_LOG_BYTES, | ||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||
| : -1; | ||||||||||||||||||||||||
| const stdio: childProcess.StdioOptions = !isPackaged | ||||||||||||||||||||||||
| ? ["ignore", "pipe", "pipe"] | ||||||||||||||||||||||||
| : logFd >= 0 | ||||||||||||||||||||||||
| ? ["ignore", logFd, logFd] | ||||||||||||||||||||||||
| : ["ignore", "ignore", "ignore"]; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| let child: ReturnType<typeof childProcess.spawn>; | ||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||
| child = childProcess.spawn(process.execPath, [this.scriptPath], { | ||||||||||||||||||||||||
| detached: true, | ||||||||||||||||||||||||
| stdio: logFd >= 0 ? ["ignore", logFd, logFd] : "ignore", | ||||||||||||||||||||||||
| env, | ||||||||||||||||||||||||
| detached: isPackaged, | ||||||||||||||||||||||||
| stdio, | ||||||||||||||||||||||||
| env: childEnv, | ||||||||||||||||||||||||
| // Avoid a flashing CMD window on Windows for the detached child. | ||||||||||||||||||||||||
| windowsHide: true, | ||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||
|
Comment on lines
+396
to
+402
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
On Windows,
Suggested change
Prompt To Fix With AIThis 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. |
||||||||||||||||||||||||
| } finally { | ||||||||||||||||||||||||
| if (logFd >= 0) { | ||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||
| fs.closeSync(logFd); | ||||||||||||||||||||||||
| } catch {} | ||||||||||||||||||||||||
| } catch { | ||||||||||||||||||||||||
| // Best-effort — child has its own dup of the fd. | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
|
|
@@ -442,6 +418,18 @@ export class HostServiceCoordinator extends EventEmitter { | |||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| instance.pid = childPid; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| if (!isPackaged) { | ||||||||||||||||||||||||
| child.stdout?.on("data", (data: Buffer) => { | ||||||||||||||||||||||||
| console.log( | ||||||||||||||||||||||||
| `[host-service:${organizationId}] ${data.toString().trim()}`, | ||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||
| child.stderr?.on("data", (data: Buffer) => { | ||||||||||||||||||||||||
| console.error( | ||||||||||||||||||||||||
| `[host-service:${organizationId}] ${data.toString().trim()}`, | ||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| child.on("exit", (code) => { | ||||||||||||||||||||||||
| console.log(`[host-service:${organizationId}] exited with code ${code}`); | ||||||||||||||||||||||||
| const current = this.instances.get(organizationId); | ||||||||||||||||||||||||
|
|
@@ -460,7 +448,7 @@ export class HostServiceCoordinator extends EventEmitter { | |||||||||||||||||||||||
| child.kill("SIGTERM"); | ||||||||||||||||||||||||
| this.instances.delete(organizationId); | ||||||||||||||||||||||||
| throw new Error( | ||||||||||||||||||||||||
| `Host service failed to start within ${HEALTH_POLL_TIMEOUT}ms`, | ||||||||||||||||||||||||
| `Host service failed to start within ${HEALTH_POLL_TIMEOUT_MS}ms`, | ||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| import * as fs from "node:fs"; | ||
| import { createServer } from "node:net"; | ||
| import path from "node:path"; | ||
|
|
||
| /** Rotate per-org host-service.log once it exceeds this size. */ | ||
| export const MAX_HOST_LOG_BYTES = 5 * 1024 * 1024; | ||
|
|
||
| export const HEALTH_POLL_TIMEOUT_MS = 10_000; | ||
|
|
||
| const HEALTH_POLL_INTERVAL_MS = 200; | ||
|
|
||
| /** | ||
| * Open an append-mode log fd, truncating first if it exceeds maxBytes. | ||
| * Returns -1 on failure so callers can fall back to ignoring child stdio. | ||
| */ | ||
| export 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 | ||
| } | ||
| } | ||
| const fd = fs.openSync(logPath, "a", 0o600); | ||
| // openSync's mode arg only applies on create — normalize an existing | ||
| // file's perms in case it was rotated out-of-band with laxer bits. | ||
| try { | ||
| fs.chmodSync(logPath, 0o600); | ||
| } catch (error) { | ||
| console.warn( | ||
| `[host-service] Failed to chmod log file ${logPath}: ${error}`, | ||
| ); | ||
| } | ||
| return fd; | ||
| } catch (error) { | ||
| console.warn(`[host-service] Failed to open log file ${logPath}: ${error}`); | ||
| return -1; | ||
| } | ||
| } | ||
|
|
||
| export async function findFreePort(): Promise<number> { | ||
| return new Promise((resolve, reject) => { | ||
| const server = createServer(); | ||
| server.listen(0, "127.0.0.1", () => { | ||
| const addr = server.address(); | ||
| if (addr && typeof addr === "object") { | ||
| const { port } = addr; | ||
| server.close(() => resolve(port)); | ||
| } else { | ||
| server.close(() => reject(new Error("Could not get port"))); | ||
| } | ||
| }); | ||
| server.on("error", reject); | ||
| }); | ||
| } | ||
|
|
||
| export async function pollHealthCheck( | ||
| endpoint: string, | ||
| secret: string, | ||
| timeoutMs = HEALTH_POLL_TIMEOUT_MS, | ||
| ): Promise<boolean> { | ||
| const deadline = Date.now() + timeoutMs; | ||
| while (Date.now() < deadline) { | ||
| 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); | ||
| } | ||
| await new Promise((r) => setTimeout(r, HEALTH_POLL_INTERVAL_MS)); | ||
| } | ||
| return false; | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.