diff --git a/apps/desktop/src/main/lib/host-service-coordinator.test.ts b/apps/desktop/src/main/lib/host-service-coordinator.test.ts index 63ae1f79e93..d3feabd0ee4 100644 --- a/apps/desktop/src/main/lib/host-service-coordinator.test.ts +++ b/apps/desktop/src/main/lib/host-service-coordinator.test.ts @@ -188,17 +188,69 @@ describe("HostServiceCoordinator.tryAdopt — adoption health check", () => { expect(conn.port).toBe(60000); }); - test("kills with SIGTERM (existing behavior) on app-version mismatch, before health check", async () => { + test("adopts a healthy service when only the app-version changed", async () => { manifestStore.current = { ...baseManifest(5555), spawnedByAppVersion: "0.9.0", }; + pollHealthCheckMock.mockImplementationOnce(() => Promise.resolve(true)); const conn = await coordinator.start("org-1", spawnConfig); - // App-version gate runs before the new health check. - expect(pollHealthCheckMock).not.toHaveBeenCalled(); - expect(killedPids).toContainEqual({ pid: 5555, signal: "SIGTERM" }); + expect(pollHealthCheckMock).toHaveBeenCalledTimes(1); + expect(killedPids).toHaveLength(0); + expect(removeManifestMock).not.toHaveBeenCalled(); + expect(spawnMock).not.toHaveBeenCalled(); + expect(conn.port).toBe(55555); + expect(conn.secret).toBe("manifest-secret"); + expect(coordinator.getProcessStatus("org-1")).toBe("running"); + }); + + test("kills an unhealthy app-version mismatch with SIGKILL after health check", async () => { + manifestStore.current = { + ...baseManifest(5556), + spawnedByAppVersion: "0.9.0", + }; + pollHealthCheckMock.mockImplementationOnce(() => Promise.resolve(false)); + + const conn = await coordinator.start("org-1", spawnConfig); + + expect(pollHealthCheckMock).toHaveBeenCalledTimes(1); + expect(killedPids).toContainEqual({ pid: 5556, signal: "SIGKILL" }); + expect(removeManifestMock).toHaveBeenCalledTimes(1); + expect(spawnMock).toHaveBeenCalledTimes(1); + expect(conn.port).toBe(60000); + }); + + test("adopts a healthy pre-upgrade manifest with no recorded app version", async () => { + manifestStore.current = { + ...baseManifest(5557), + spawnedByAppVersion: "", + }; + pollHealthCheckMock.mockImplementationOnce(() => Promise.resolve(true)); + + const conn = await coordinator.start("org-1", spawnConfig); + + expect(pollHealthCheckMock).toHaveBeenCalledTimes(1); + expect(killedPids).toHaveLength(0); + expect(removeManifestMock).not.toHaveBeenCalled(); + expect(spawnMock).not.toHaveBeenCalled(); + expect(conn.port).toBe(55555); + expect(conn.secret).toBe("manifest-secret"); + expect(coordinator.getProcessStatus("org-1")).toBe("running"); + }); + + test("kills an unhealthy pre-upgrade manifest with SIGKILL after health check", async () => { + manifestStore.current = { + ...baseManifest(5558), + spawnedByAppVersion: "", + }; + pollHealthCheckMock.mockImplementationOnce(() => Promise.resolve(false)); + + const conn = await coordinator.start("org-1", spawnConfig); + + expect(pollHealthCheckMock).toHaveBeenCalledTimes(1); + expect(killedPids).toContainEqual({ pid: 5558, signal: "SIGKILL" }); expect(removeManifestMock).toHaveBeenCalledTimes(1); expect(spawnMock).toHaveBeenCalledTimes(1); expect(conn.port).toBe(60000); diff --git a/apps/desktop/src/main/lib/host-service-coordinator.ts b/apps/desktop/src/main/lib/host-service-coordinator.ts index f39a9926f4f..854426db5de 100644 --- a/apps/desktop/src/main/lib/host-service-coordinator.ts +++ b/apps/desktop/src/main/lib/host-service-coordinator.ts @@ -333,13 +333,8 @@ export class HostServiceCoordinator extends EventEmitter { ? `spawned by app ${manifest.spawnedByAppVersion} != current ${currentAppVersion}` : "no recorded app version (pre-upgrade manifest)"; log.info( - `[host-service:${organizationId}] Adopted service ${reason}, killing`, + `[host-service:${organizationId}] Adopted service ${reason}, checking health before reuse`, ); - try { - process.kill(manifest.pid, "SIGTERM"); - } catch {} - removeManifest(organizationId); - return null; } // A live pid is not the same as a serving host-service — the process can @@ -520,6 +515,9 @@ export class HostServiceCoordinator extends EventEmitter { const childEnv = await getProcessEnvWithShellPath({ ...(process.env as Record), ELECTRON_RUN_AS_NODE: "1", + NODE_ENV: app.isPackaged + ? "production" + : (process.env.NODE_ENV ?? "development"), ORGANIZATION_ID: organizationId, HOST_CLIENT_ID: getHostId(), HOST_NAME: getHostName(), diff --git a/apps/desktop/src/main/lib/host-service-manifest.ts b/apps/desktop/src/main/lib/host-service-manifest.ts index 8e1d61901fc..c94e2e9693d 100644 --- a/apps/desktop/src/main/lib/host-service-manifest.ts +++ b/apps/desktop/src/main/lib/host-service-manifest.ts @@ -16,12 +16,10 @@ export interface HostServiceManifest { startedAt: number; organizationId: string; /** - * Desktop app version that spawned this host-service. Compared against - * the current `app.getVersion()` on adoption — any mismatch triggers a - * kill + respawn so every Electron auto-update lands on a freshly - * spawned host-service, even when the host-service version pin alone - * would have allowed adoption (e.g. host-service code changed but its - * `package.json#version` was not bumped). + * Desktop app version that spawned this host-service. This is diagnostic + * adoption context; a version mismatch alone must not kill a healthy + * service because canary app timestamps can change without requiring a + * host-service restart. */ spawnedByAppVersion: string; } @@ -71,8 +69,8 @@ export function readManifest( // `spawnedByAppVersion` is required going forward, but pre-existing // manifests on upgraded users won't have it. Coerce to empty string so - // `tryAdopt` still finds the old PID, then trip the app-version pin - // (current version !== "") so the stale daemon gets killed and respawned. + // `tryAdopt` can log the provenance gap and still health-check the + // existing service before deciding whether to reuse it. if (typeof data.spawnedByAppVersion !== "string") { data.spawnedByAppVersion = ""; } diff --git a/packages/host-service/src/daemon/DaemonSupervisor.test.ts b/packages/host-service/src/daemon/DaemonSupervisor.test.ts index 869cd6bca1a..625755c62ba 100644 --- a/packages/host-service/src/daemon/DaemonSupervisor.test.ts +++ b/packages/host-service/src/daemon/DaemonSupervisor.test.ts @@ -199,9 +199,9 @@ describe("shouldKillStaleDaemonForDev", () => { expect(shouldKillStaleDaemonForDev({ NODE_ENV: "production" })).toBe(false); }); - test("kills stale daemons in dev by default", () => { + test("kills stale daemons only in explicit dev mode", () => { expect(shouldKillStaleDaemonForDev({ NODE_ENV: "development" })).toBe(true); - expect(shouldKillStaleDaemonForDev({})).toBe(true); + expect(shouldKillStaleDaemonForDev({})).toBe(false); }); test("allows production-like adoption in dev for handoff testing", () => { diff --git a/packages/host-service/src/daemon/DaemonSupervisor.ts b/packages/host-service/src/daemon/DaemonSupervisor.ts index 2813de5d495..f4c425bb470 100644 --- a/packages/host-service/src/daemon/DaemonSupervisor.ts +++ b/packages/host-service/src/daemon/DaemonSupervisor.ts @@ -73,8 +73,8 @@ const ADOPT_IN_DEV_ENV = "SUPERSET_PTY_DAEMON_ADOPT_IN_DEV"; export function shouldKillStaleDaemonForDev( env: NodeJS.ProcessEnv = process.env, ): boolean { - if (env.NODE_ENV === "production") return false; - return env[ADOPT_IN_DEV_ENV] !== "1"; + if (env[ADOPT_IN_DEV_ENV] === "1") return false; + return env.NODE_ENV === "development"; } /** @@ -907,7 +907,7 @@ export class DaemonSupervisor { // flow up to the developer's `bun dev` terminal. Production: // hard-back stdio with the rotating log file so the detached // daemon survives host-service teardown without losing logs. - const isDev = process.env.NODE_ENV !== "production"; + const isDev = process.env.NODE_ENV === "development"; const logFd = isDev ? -1 : openRotatingLogFd(logPath, MAX_DAEMON_LOG_BYTES); const stdio: childProcess.StdioOptions = isDev ? ["ignore", "pipe", "pipe"] diff --git a/packages/host-service/src/serve.ts b/packages/host-service/src/serve.ts index bdee830a243..760139a447d 100644 --- a/packages/host-service/src/serve.ts +++ b/packages/host-service/src/serve.ts @@ -52,7 +52,7 @@ async function main(): Promise { // iteration on daemon code resets cleanly. Production keeps the // daemon detached so PTYs survive host-service restarts. // Per the migration plan's D5 decision. - const isDev = process.env.NODE_ENV !== "production"; + const isDev = process.env.NODE_ENV === "development"; if (isDev) { let shuttingDown = false; const devShutdown = async (signal: NodeJS.Signals) => { diff --git a/packages/host-service/src/trpc/router/host/host.ts b/packages/host-service/src/trpc/router/host/host.ts index a73ad330854..1c909aec4d1 100644 --- a/packages/host-service/src/trpc/router/host/host.ts +++ b/packages/host-service/src/trpc/router/host/host.ts @@ -7,9 +7,8 @@ import { TRPCError } from "@trpc/server"; import type { ApiClient } from "../../../types"; import { protectedProcedure, router } from "../../index"; -// Auto-derived from this package's package.json so a host-service version -// bump automatically flows through to `host.info` and the desktop's -// strict-equality adoption check (see host-service-coordinator.tryAdopt). +// Auto-derived from this package's package.json so callers can report exactly +// which bundled host-service build is currently serving requests. const HOST_SERVICE_VERSION: string = hostServicePackageJson.version; const ORGANIZATION_CACHE_TTL_MS = 60 * 60 * 1000;