Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 56 additions & 4 deletions apps/desktop/src/main/lib/host-service-coordinator.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
10 changes: 4 additions & 6 deletions apps/desktop/src/main/lib/host-service-coordinator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -520,6 +515,9 @@ export class HostServiceCoordinator extends EventEmitter {
const childEnv = await getProcessEnvWithShellPath({
...(process.env as Record<string, string>),
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(),
Expand Down
14 changes: 6 additions & 8 deletions apps/desktop/src/main/lib/host-service-manifest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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 = "";
}
Expand Down
4 changes: 2 additions & 2 deletions packages/host-service/src/daemon/DaemonSupervisor.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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", () => {
Expand Down
6 changes: 3 additions & 3 deletions packages/host-service/src/daemon/DaemonSupervisor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
}

/**
Expand Down Expand Up @@ -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"]
Expand Down
2 changes: 1 addition & 1 deletion packages/host-service/src/serve.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ async function main(): Promise<void> {
// 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) => {
Expand Down
5 changes: 2 additions & 3 deletions packages/host-service/src/trpc/router/host/host.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Loading