Skip to content

fix(desktop): auto-recover unresponsive host-service with a health watchdog #5711

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.

Sign up for GitHub

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

Closed
sasha1107 wants to merge 1 commit into from
Closed

fix(desktop): auto-recover unresponsive host-service with a health watchdog #5711

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 7 additions & 5 deletions apps/desktop/src/main/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,12 +441,14 @@ if (!gotTheLock) {
console.error("[main] Failed to install bundled CLI shim:", error);
}

const hostServiceConfigProvider = async () => {
const { token } = await loadToken();
if (!token) return null;

@greptile-apps greptile-apps Bot Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Cleanup function from enableHealthWatchdog is discarded

enableHealthWatchdog returns a teardown callback that clears the setInterval timer and the watchdogFailures map. Discarding it means there is no programmatic way to stop the watchdog once started — the only protection against the timer outliving its intended scope is timer.unref(). This is technically safe for the current Electron singleton (the process exits on quit), but it departs from the pattern established by enableDevReload, and would cause an orphaned, uncancellable timer if the coordinator is ever reset or recreated in tests or future code paths.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: apps/desktop/src/main/index.ts
Line: 422

Comment:
**Cleanup function from `enableHealthWatchdog` is discarded**

`enableHealthWatchdog` returns a teardown callback that clears the `setInterval` timer and the `watchdogFailures` map. Discarding it means there is no programmatic way to stop the watchdog once started — the only protection against the timer outliving its intended scope is `timer.unref()`. This is technically safe for the current Electron singleton (the process exits on quit), but it departs from the pattern established by `enableDevReload`, and would cause an orphaned, uncancellable timer if the coordinator is ever reset or recreated in tests or future code paths.

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@sasha1107 sasha1107 Jul 16, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving as-is intentionally: this mirrors the existing enableDevReload call site, which also discards its disposer. The coordinator is a process-lifetime singleton and the timer is unref()d, so there's no scope it can outlive today. Happy to wire both disposers into a teardown if a maintainer prefers.

return { authToken: token, cloudApiUrl: mainEnv.NEXT_PUBLIC_API_URL };
};
getHostServiceCoordinator().enableHealthWatchdog(hostServiceConfigProvider);
if (IS_DEV) {
getHostServiceCoordinator().enableDevReload(async () => {
const { token } = await loadToken();
if (!token) return null;
return { authToken: token, cloudApiUrl: mainEnv.NEXT_PUBLIC_API_URL };
});
getHostServiceCoordinator().enableDevReload(hostServiceConfigProvider);
}

await makeAppSetup(() => MainWindow());
Expand Down
342 changes: 341 additions & 1 deletion apps/desktop/src/main/lib/host-service-coordinator.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,8 @@ mock.module("./local-db", () => ({
},
}));

const { HostServiceCoordinator } = await import("./host-service-coordinator");
const { HostServiceCoordinator, HEALTH_WATCHDOG_FAILURE_THRESHOLD } =
await import("./host-service-coordinator");

const baseManifest = (pid: number, endpoint = "http://127.0.0.1:55555") => ({
pid,
Expand Down Expand Up @@ -370,6 +371,345 @@ describe("HostServiceCoordinator single-flight / adoption", () => {
});
});

describe("HostServiceCoordinator health watchdog", () => {
let coordinator: InstanceType<typeof HostServiceCoordinator>;
let spawnMock: ReturnType<typeof mock>;

type WatchdogStatus = "starting" | "running" | "stopped";

interface WatchdogInternals {
instances: Map<
string,
{
pid: number;
port: number;
secret: string;
status: WatchdogStatus;
owned: boolean;
}
>;
watchdogFailures: Map<string, { pid: number; count: number }>;
runWatchdogTick(
configProvider: () => Promise<{
authToken: string;
cloudApiUrl: string;
} | null>,
): Promise<void>;
}

const configProvider = async () => spawnConfig;
const nullConfigProvider = async () => null;

function internals(): WatchdogInternals {
return coordinator as unknown as WatchdogInternals;
}

function seedInstance(
organizationId: string,
overrides: Partial<{
pid: number;
port: number;
secret: string;
status: WatchdogStatus;
owned: boolean;
}> = {},
) {
internals().instances.set(organizationId, {
pid: 1234,
port: 48123,
secret: "secret",
status: "running",
// The watchdog only supervises children this instance owns; adopted
// entries are another instance's responsibility.
owned: true,
...overrides,
});
}

async function runTicks(
count: number,
provider: () => Promise<{
authToken: string;
cloudApiUrl: string;
} | null> = configProvider,
) {
for (let i = 0; i < count; i++) {
await internals().runWatchdogTick(provider);
}
}

beforeEach(() => {
resetMocks();
testManifestRoot = fs.mkdtempSync(path.join(os.tmpdir(), "hsc-test-"));
pollHealthCheckMock.mockImplementation(() => Promise.resolve(true));

coordinator = new HostServiceCoordinator();
spawnMock = mock(async () => ({
port: 60000,
secret: "fresh-secret",
machineId: "host-1",
}));
(coordinator as unknown as { spawn: typeof spawnMock }).spawn = spawnMock;
});

afterEach(() => {
coordinator.stopAll();
pollHealthCheckMock.mockImplementation(() => Promise.resolve(true));
if (testManifestRoot) {
fs.rmSync(testManifestRoot, { recursive: true, force: true });
testManifestRoot = "";
}
});

test("healthy instance clears the failure counter and is never restarted", async () => {
seedInstance("org-1");
internals().watchdogFailures.set("org-1", { pid: 1234, count: 2 });

await runTicks(1);

expect(internals().watchdogFailures.has("org-1")).toBe(false);
expect(spawnMock).not.toHaveBeenCalled();
expect(killedPids).toHaveLength(0);
});

test("does not restart before the failure threshold", async () => {
seedInstance("org-1");
pollHealthCheckMock.mockImplementation(() => Promise.resolve(false));

await runTicks(HEALTH_WATCHDOG_FAILURE_THRESHOLD - 1);

expect(internals().watchdogFailures.get("org-1")).toEqual({
pid: 1234,
count: HEALTH_WATCHDOG_FAILURE_THRESHOLD - 1,
});
expect(spawnMock).not.toHaveBeenCalled();
expect(killedPids).toHaveLength(0);
});

test("force-restarts after consecutive failures reach the threshold", async () => {
seedInstance("org-1", { pid: 4321 });
pollHealthCheckMock.mockImplementation(() => Promise.resolve(false));

await runTicks(HEALTH_WATCHDOG_FAILURE_THRESHOLD);

expect(spawnMock).toHaveBeenCalledTimes(1);
// stop() SIGTERMs the tracked pid, then the watchdog escalates to
// SIGKILL because the wedged process is still alive.
expect(killedPids).toContainEqual({ pid: 4321, signal: "SIGTERM" });
expect(killedPids).toContainEqual({ pid: 4321, signal: "SIGKILL" });
expect(internals().watchdogFailures.has("org-1")).toBe(false);
});

test("stays armed when no spawn config is available and recovers on the next tick", async () => {
seedInstance("org-1", { pid: 4321 });
pollHealthCheckMock.mockImplementation(() => Promise.resolve(false));

await runTicks(HEALTH_WATCHDOG_FAILURE_THRESHOLD, nullConfigProvider);

expect(spawnMock).not.toHaveBeenCalled();
expect(killedPids).toHaveLength(0);
// The counter survives the missing config, so recovery does not have
// to wait out another full threshold once a config is available.
expect(internals().watchdogFailures.get("org-1")?.count).toBe(
HEALTH_WATCHDOG_FAILURE_THRESHOLD,
);

await runTicks(1);

expect(spawnMock).toHaveBeenCalledTimes(1);
expect(killedPids).toContainEqual({ pid: 4321, signal: "SIGKILL" });
expect(internals().watchdogFailures.has("org-1")).toBe(false);
});

test("skips the restart when the child is replaced while fetching config", async () => {
seedInstance("org-1", { pid: 1111 });
pollHealthCheckMock.mockImplementation(() => Promise.resolve(false));
const replacingProvider = async () => {
// Simulate a manual restart landing while the config was fetched.
seedInstance("org-1", { pid: 2222 });
return spawnConfig;
};

await runTicks(HEALTH_WATCHDOG_FAILURE_THRESHOLD, replacingProvider);

expect(spawnMock).not.toHaveBeenCalled();
expect(killedPids).toHaveLength(0);
expect(internals().watchdogFailures.has("org-1")).toBe(false);
});

test("ignores instances that are not running", async () => {
seedInstance("org-1", { status: "starting" });

await runTicks(1);

expect(pollHealthCheckMock).not.toHaveBeenCalled();
expect(spawnMock).not.toHaveBeenCalled();
});

test("ignores adopted instances it does not own", async () => {
// An adopted entry points at another live app instance's child. Even if
// it stops responding, force-restarting it would SIGKILL a process this
// instance doesn't control — the owner runs its own watchdog.
seedInstance("org-1", { pid: 4321, owned: false });
pollHealthCheckMock.mockImplementation(() => Promise.resolve(false));

await runTicks(HEALTH_WATCHDOG_FAILURE_THRESHOLD + 1);

expect(pollHealthCheckMock).not.toHaveBeenCalled();
expect(killedPids).toHaveLength(0);
expect(spawnMock).not.toHaveBeenCalled();
expect(internals().watchdogFailures.has("org-1")).toBe(false);
});

test("a probe failure does not count against a replaced child", async () => {
seedInstance("org-1", { pid: 1111 });
pollHealthCheckMock.mockImplementation(() => {
// Simulate a restart landing while the probe was in flight.
seedInstance("org-1", { pid: 2222 });
return Promise.resolve(false);
});

await runTicks(1);

expect(internals().watchdogFailures.has("org-1")).toBe(false);
expect(spawnMock).not.toHaveBeenCalled();
});

test("failure counter rebinds when a new child replaces the old one between ticks", async () => {
seedInstance("org-1", { pid: 1111 });
pollHealthCheckMock.mockImplementation(() => Promise.resolve(false));

await runTicks(HEALTH_WATCHDOG_FAILURE_THRESHOLD - 1);
expect(internals().watchdogFailures.get("org-1")?.count).toBe(
HEALTH_WATCHDOG_FAILURE_THRESHOLD - 1,
);

// A new child spawned between ticks must not inherit its
// predecessor's failures and get restarted on its first bad probe.
seedInstance("org-1", { pid: 2222 });
await runTicks(1);

expect(internals().watchdogFailures.get("org-1")).toEqual({
pid: 2222,
count: 1,
});
expect(spawnMock).not.toHaveBeenCalled();
expect(killedPids).toHaveLength(0);
});

test("escalates the wedged pid to SIGKILL before the replacement spawns", async () => {
seedInstance("org-1", { pid: 4321 });
pollHealthCheckMock.mockImplementation(() => Promise.resolve(false));
// If the SIGKILL only landed after reset() returned, a slow spawn
// would leave a window where the OS recycles the pid and the kill
// hits an innocent process.
let sigkilledBeforeSpawn = false;
spawnMock.mockImplementation(async () => {
sigkilledBeforeSpawn = killedPids.some(
(k) => k.pid === 4321 && k.signal === "SIGKILL",
);
return { port: 60000, secret: "fresh-secret", machineId: "host-1" };
});

await runTicks(HEALTH_WATCHDOG_FAILURE_THRESHOLD);

expect(spawnMock).toHaveBeenCalledTimes(1);
expect(sigkilledBeforeSpawn).toBe(true);
});

test("a failed recovery is retried until a spawn sticks", async () => {
seedInstance("org-1", { pid: 4321 });
pollHealthCheckMock.mockImplementation(() => Promise.resolve(false));
spawnMock.mockImplementationOnce(async () => {
throw new Error("spawn blew up");
});

await runTicks(HEALTH_WATCHDOG_FAILURE_THRESHOLD);

// The failed reset() left no instance behind; the retry pass respawns
// the org instead of forgetting it.
expect(spawnMock).toHaveBeenCalledTimes(2);
expect(internals().watchdogFailures.has("org-1")).toBe(false);
});

test("stays armed while respawns keep failing", async () => {
seedInstance("org-1", { pid: 4321 });
pollHealthCheckMock.mockImplementation(() => Promise.resolve(false));
spawnMock.mockImplementation(async () => {
throw new Error("spawn keeps failing");
});

await runTicks(HEALTH_WATCHDOG_FAILURE_THRESHOLD + 1);

expect(internals().watchdogFailures.get("org-1")?.count).toBe(
HEALTH_WATCHDOG_FAILURE_THRESHOLD,
);
expect(spawnMock.mock.calls.length).toBeGreaterThanOrEqual(3);
});

test("stop() disarms the watchdog for that org", () => {
internals().watchdogFailures.set("org-1", {
pid: 1234,
count: HEALTH_WATCHDOG_FAILURE_THRESHOLD,
});

coordinator.stop("org-1");

expect(internals().watchdogFailures.has("org-1")).toBe(false);
});

test("retry pass aborts when the org is stopped while fetching config", async () => {
internals().watchdogFailures.set("org-1", {
pid: 4321,
count: HEALTH_WATCHDOG_FAILURE_THRESHOLD,
});
const stoppingProvider = async () => {
// A deliberate shutdown lands while the config is being fetched.
coordinator.stop("org-1");
return spawnConfig;
};

await runTicks(1, stoppingProvider);

expect(spawnMock).not.toHaveBeenCalled();
expect(internals().watchdogFailures.has("org-1")).toBe(false);
});

test("a rejecting config lookup fails the tick quietly", async () => {
seedInstance("org-1", { pid: 4321 });
pollHealthCheckMock.mockImplementation(() => Promise.resolve(false));
const rejectingProvider = async (): Promise<{
authToken: string;
cloudApiUrl: string;
} | null> => {
throw new Error("token lookup failed");
};

// The tick runs detached from a timer — it must swallow the rejection
// instead of surfacing an unhandled rejection.
await runTicks(HEALTH_WATCHDOG_FAILURE_THRESHOLD, rejectingProvider);

expect(spawnMock).not.toHaveBeenCalled();
});

test("a spawn failure leaves no ghost 'starting' instance behind", async () => {
// Use the real spawn() with a failing buildEnv — pre-fix, the instance
// stayed tracked as "starting" forever, permanently blocking both
// startWithPreferredPorts status readers and the watchdog retry pass.
delete (coordinator as unknown as { spawn?: unknown }).spawn;
(coordinator as unknown as { buildEnv: () => Promise<never> }).buildEnv =
async () => {
throw new Error("env resolution failed");
};

await expect(coordinator.start("org-1", spawnConfig)).rejects.toThrow(
"env resolution failed",
);

expect(internals().instances.has("org-1")).toBe(false);
});
});

afterAll(() => {
mock.restore();
});
Loading