-
Notifications
You must be signed in to change notification settings - Fork 990
refactor(desktop): couple host-service to Electron, drop adoption #4739
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
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 | ||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -23,7 +23,55 @@ import { loadToken } from "lib/trpc/routers/auth/utils/auth-functions"; | |||||||||||||||||||||||||||||||||||||||||
| import { writeManifest } from "main/lib/host-service-manifest"; | ||||||||||||||||||||||||||||||||||||||||||
| import { env } from "./env"; | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| const SHUTDOWN_GRACE_MS = 3_000; | ||||||||||||||||||||||||||||||||||||||||||
| const WATCHDOG_INTERVAL_MS = 2_000; | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| type Server = ReturnType<typeof serve>; | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| async function main(): Promise<void> { | ||||||||||||||||||||||||||||||||||||||||||
| // Install the parent watchdog before any awaits so a crash during | ||||||||||||||||||||||||||||||||||||||||||
| // startup can still reap this child. `serverRef` is filled in once | ||||||||||||||||||||||||||||||||||||||||||
| // serve() returns; shutdown handles both pre- and post-bind states. | ||||||||||||||||||||||||||||||||||||||||||
| const serverRef: { current: Server | null } = { current: null }; | ||||||||||||||||||||||||||||||||||||||||||
| let shuttingDown = false; | ||||||||||||||||||||||||||||||||||||||||||
| const shutdown = (reason: string) => { | ||||||||||||||||||||||||||||||||||||||||||
| if (shuttingDown) return; | ||||||||||||||||||||||||||||||||||||||||||
| shuttingDown = true; | ||||||||||||||||||||||||||||||||||||||||||
| console.log(`[host-service] shutdown (${reason}), draining connections`); | ||||||||||||||||||||||||||||||||||||||||||
| const server = serverRef.current; | ||||||||||||||||||||||||||||||||||||||||||
| if (!server) { | ||||||||||||||||||||||||||||||||||||||||||
| process.exit(0); | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
| server.close(); | ||||||||||||||||||||||||||||||||||||||||||
| // SSE/WS streams (chat, watchers) ignore server.close() — give in-flight | ||||||||||||||||||||||||||||||||||||||||||
| // HTTP a brief window, then forcibly tear sockets down. | ||||||||||||||||||||||||||||||||||||||||||
| const forceExit = setTimeout(() => { | ||||||||||||||||||||||||||||||||||||||||||
| const httpServer = server as unknown as { | ||||||||||||||||||||||||||||||||||||||||||
| closeAllConnections?: () => void; | ||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||
| httpServer.closeAllConnections?.(); | ||||||||||||||||||||||||||||||||||||||||||
| process.exit(0); | ||||||||||||||||||||||||||||||||||||||||||
| }, SHUTDOWN_GRACE_MS); | ||||||||||||||||||||||||||||||||||||||||||
| forceExit.unref(); | ||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| process.on("SIGTERM", () => shutdown("SIGTERM")); | ||||||||||||||||||||||||||||||||||||||||||
| process.on("SIGINT", () => shutdown("SIGINT")); | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| // Self-exit if our Electron parent dies without sending SIGTERM | ||||||||||||||||||||||||||||||||||||||||||
| // (orphan reparenting to init/launchd). CLI-spawned host-services | ||||||||||||||||||||||||||||||||||||||||||
| // don't set HOST_PARENT_PID and skip this. | ||||||||||||||||||||||||||||||||||||||||||
| const parentPid = Number(process.env.HOST_PARENT_PID); | ||||||||||||||||||||||||||||||||||||||||||
| if (Number.isInteger(parentPid) && parentPid > 1) { | ||||||||||||||||||||||||||||||||||||||||||
| const interval = setInterval(() => { | ||||||||||||||||||||||||||||||||||||||||||
| if (!isParentAlive(parentPid)) { | ||||||||||||||||||||||||||||||||||||||||||
| clearInterval(interval); | ||||||||||||||||||||||||||||||||||||||||||
| shutdown("parent-exit"); | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
| }, WATCHDOG_INTERVAL_MS); | ||||||||||||||||||||||||||||||||||||||||||
| interval.unref(); | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| const terminalBaseEnv = await resolveTerminalBaseEnv(); | ||||||||||||||||||||||||||||||||||||||||||
| initTerminalBaseEnv(terminalBaseEnv); | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -75,7 +123,6 @@ async function main(): Promise<void> { | |||||||||||||||||||||||||||||||||||||||||
| authToken: env.HOST_SERVICE_SECRET, | ||||||||||||||||||||||||||||||||||||||||||
| startedAt, | ||||||||||||||||||||||||||||||||||||||||||
| organizationId: env.ORGANIZATION_ID, | ||||||||||||||||||||||||||||||||||||||||||
| spawnedByAppVersion: env.SUPERSET_APP_VERSION, | ||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||
| } catch (error) { | ||||||||||||||||||||||||||||||||||||||||||
| console.error("[host-service] Failed to write manifest:", error); | ||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -94,16 +141,17 @@ async function main(): Promise<void> { | |||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||
| serverRef.current = server; | ||||||||||||||||||||||||||||||||||||||||||
| injectWebSocket(server); | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| // Manifest lifecycle belongs to the coordinator, not the child. | ||||||||||||||||||||||||||||||||||||||||||
| const shutdown = () => { | ||||||||||||||||||||||||||||||||||||||||||
| server.close(); | ||||||||||||||||||||||||||||||||||||||||||
| process.exit(0); | ||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| process.on("SIGTERM", shutdown); | ||||||||||||||||||||||||||||||||||||||||||
| process.on("SIGINT", shutdown); | ||||||||||||||||||||||||||||||||||||||||||
| function isParentAlive(parentPid: number): boolean { | ||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||
| process.kill(parentPid, 0); | ||||||||||||||||||||||||||||||||||||||||||
| return process.ppid === parentPid; | ||||||||||||||||||||||||||||||||||||||||||
| } catch { | ||||||||||||||||||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+148
to
155
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.
Suggested change
Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/desktop/src/main/host-service/index.ts
Line: 139-146
Comment:
The `catch` block swallows all errors from `process.kill(parentPid, 0)`, including `EPERM`. On POSIX, `EPERM` means the target process **exists** but the caller lacks permission to signal it (e.g. different uid after a privilege drop). Returning `false` in that case would cause the watchdog to call `shutdown("parent-exit")` even though Electron is still alive, killing the host-service spuriously. `ESRCH` (process not found) is the only error that genuinely means "dead parent".
```suggestion
function isParentAlive(parentPid: number): boolean {
try {
process.kill(parentPid, 0);
return process.ppid === parentPid;
} catch (err) {
// EPERM means the process exists but we can't signal it — still alive.
if ((err as NodeJS.ErrnoException).code === "EPERM") {
return process.ppid === parentPid;
}
return false;
}
}
```
How can I resolve this? If you propose a fix, please make it concise. |
||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| void main().catch((error) => { | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specify a language for the fenced diagram block.
Line 7 opens a fenced block without a language, which triggers markdownlint MD040.
Suggested fix
📝 Committable suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents