diff --git a/.github/workflows/claude-code-review.yml b/.github/workflows/claude-code-review.yml index 51e0d9de..36cab808 100644 --- a/.github/workflows/claude-code-review.yml +++ b/.github/workflows/claude-code-review.yml @@ -1,10 +1,6 @@ name: Claude Code Review on: - pull_request: - types: [opened] - pull_request_target: - types: [opened] workflow_dispatch: # Cancel in-progress runs for the same PR to prevent duplicate reviews diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 6727f9de..ac330330 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -63,6 +63,10 @@ jobs: if: steps.check_release.outputs.skip != 'true' run: pnpm -r prebuild + - name: Lint + if: steps.check_release.outputs.skip != 'true' + run: pnpm -C apps/desktop lint + - name: Typecheck if: steps.check_release.outputs.skip != 'true' run: pnpm -C apps/desktop typecheck diff --git a/apps/desktop/package.json b/apps/desktop/package.json index 1871789c..f45df792 100644 --- a/apps/desktop/package.json +++ b/apps/desktop/package.json @@ -1,6 +1,6 @@ { "name": "desktop", - "version": "0.9.0", + "version": "0.9.1", "description": "ClosedLoop Desktop", "author": "ClosedLoop AI ", "private": true, diff --git a/apps/desktop/src/main/app.ts b/apps/desktop/src/main/app.ts index 775441ff..352a63a6 100644 --- a/apps/desktop/src/main/app.ts +++ b/apps/desktop/src/main/app.ts @@ -58,6 +58,8 @@ import { import { LocalSessionStore } from "./local-session-store.js"; import { enrichJobSnapshot } from "../server/operations/symphony-job-snapshot.js"; import { GatewayRecoveryManager } from "./gateway-recovery.js"; +import { runShutdownSequence } from "./shutdown.js"; +import type { ShutdownResult } from "./shutdown.js"; import pkg from "electron-updater"; const { autoUpdater } = pkg; import { BUILD_COMMIT_HASH } from "../shared/build-info.js"; @@ -360,21 +362,26 @@ export class DesktopApplication { this.desktopWindow.show(); } - async shutdown(): Promise { + async shutdown(): Promise { if (this.shuttingDown) { - return; + return "clean"; } this.shuttingDown = true; - if (this.updateCheckTimer) { - clearInterval(this.updateCheckTimer); - this.updateCheckTimer = null; - } - this.cloudSocket.stop(); - this.commandExecutor.dispose(); - await this.server.stop(); - this.desktopWindow.dispose(); - this.tray.dispose(); + return runShutdownSequence({ + updateCheckTimer: this.updateCheckTimer, + clearUpdateCheckTimer: () => { + if (this.updateCheckTimer) { + clearInterval(this.updateCheckTimer); + this.updateCheckTimer = null; + } + }, + cloudSocket: this.cloudSocket, + commandExecutor: this.commandExecutor, + server: this.server, + desktopWindow: this.desktopWindow, + tray: this.tray, + }); } private async probeGatewayAlive(): Promise { diff --git a/apps/desktop/src/main/index.ts b/apps/desktop/src/main/index.ts index 61844211..5f8b6bd6 100644 --- a/apps/desktop/src/main/index.ts +++ b/apps/desktop/src/main/index.ts @@ -18,10 +18,21 @@ app.on("activate", () => { desktopApplication.showWindow(); }); -app.on("before-quit", () => { - void desktopApplication.shutdown().catch((error) => { - const message = error instanceof Error ? error.message : "unknown shutdown error"; - console.error(`desktop shutdown failed: ${message}`); +let quitPromise: Promise | null = null; + +app.on("before-quit", (event) => { + // Prevent Electron from proceeding until async shutdown completes. + event.preventDefault(); + + // If shutdown is already in progress (e.g. window-all-closed fired app.quit() + // on non-macOS after DesktopWindow.dispose() closed the last window), do nothing. + // The first invocation's .then() continuation will call app.exit() exactly once. + if (quitPromise) { + return; + } + + quitPromise = desktopApplication.shutdown().then((result) => { + app.exit(result === "clean" ? 0 : 1); }); }); diff --git a/apps/desktop/src/main/shutdown.ts b/apps/desktop/src/main/shutdown.ts new file mode 100644 index 00000000..d089be8e --- /dev/null +++ b/apps/desktop/src/main/shutdown.ts @@ -0,0 +1,46 @@ +export interface ShutdownDeps { + updateCheckTimer: NodeJS.Timeout | null; + clearUpdateCheckTimer: () => void; + cloudSocket: { stop: () => void }; + commandExecutor: { dispose: () => void }; + server: { stop: () => Promise }; + desktopWindow: { dispose: () => void }; + tray: { dispose: () => void }; +} + +export type ShutdownResult = "clean" | "timed_out" | "failed"; + +export async function runShutdownSequence( + deps: ShutdownDeps, + options?: { timeoutMs?: number; setTimeoutFn?: typeof setTimeout } +): Promise { + const timeoutMs = options?.timeoutMs ?? 5000; + const setTimeoutFn = options?.setTimeoutFn ?? setTimeout; + + let timer: ReturnType | null = null; + + const cleanup = async (): Promise<"clean"> => { + deps.clearUpdateCheckTimer(); + deps.cloudSocket.stop(); + deps.commandExecutor.dispose(); + await deps.server.stop(); + deps.desktopWindow.dispose(); + deps.tray.dispose(); + return "clean"; + }; + + const timeout = new Promise<"timed_out">((resolve) => { + timer = setTimeoutFn(() => resolve("timed_out"), timeoutMs); + }); + + try { + const result = await Promise.race([cleanup(), timeout]); + return result; + } catch { + return "failed"; + } finally { + if (timer != null) { + clearTimeout(timer); + } + } +} diff --git a/apps/desktop/src/server/operations/symphony-loop.ts b/apps/desktop/src/server/operations/symphony-loop.ts index 88a7ae51..34e3954c 100644 --- a/apps/desktop/src/server/operations/symphony-loop.ts +++ b/apps/desktop/src/server/operations/symphony-loop.ts @@ -311,7 +311,7 @@ function buildClaudePipeline( `tee -a ${shellEscape(jsonlFile)}`, `python3 ${shellEscape(formatter)}`, ].join(" | "); - return { cmd: "bash", args: ["-c", pipeline] }; + return { cmd: "bash", args: ["-c", `${pipeline}; exit \${PIPESTATUS[0]}`] }; } // No formatter — wrap in bash pipeline so grep|tee still writes claude-output.jsonl @@ -320,7 +320,7 @@ function buildClaudePipeline( "grep --line-buffered '^{'", `tee -a ${shellEscape(jsonlFile)}`, ].join(" | "); - return { cmd: "bash", args: ["-c", pipeline] }; + return { cmd: "bash", args: ["-c", `${pipeline}; exit \${PIPESTATUS[0]}`] }; } /** Find the local repo path for a given fullName (e.g. "org/repo"). */ diff --git a/apps/desktop/test/shutdown.test.ts b/apps/desktop/test/shutdown.test.ts new file mode 100644 index 00000000..036982b4 --- /dev/null +++ b/apps/desktop/test/shutdown.test.ts @@ -0,0 +1,138 @@ +import assert from "node:assert/strict"; +import { describe, test } from "node:test"; +import { + runShutdownSequence, + type ShutdownDeps, +} from "../src/main/shutdown.js"; + +/** Build stub deps that record call order. */ +function makeStubDeps(overrides?: Partial) { + const calls: string[] = []; + const deps: ShutdownDeps = { + updateCheckTimer: null, + clearUpdateCheckTimer: () => { + calls.push("clearUpdateCheckTimer"); + }, + cloudSocket: { + stop: () => { + calls.push("cloudSocket.stop"); + }, + }, + commandExecutor: { + dispose: () => { + calls.push("commandExecutor.dispose"); + }, + }, + server: { + stop: async () => { + calls.push("server.stop"); + }, + }, + desktopWindow: { + dispose: () => { + calls.push("desktopWindow.dispose"); + }, + }, + tray: { + dispose: () => { + calls.push("tray.dispose"); + }, + }, + ...overrides, + }; + return { deps, calls }; +} + +describe("runShutdownSequence", () => { + test("clean path: all deps succeed, cleanup steps called in order", async () => { + const { deps, calls } = makeStubDeps(); + + const result = await runShutdownSequence(deps); + + assert.equal(result, "clean"); + assert.deepEqual(calls, [ + "clearUpdateCheckTimer", + "cloudSocket.stop", + "commandExecutor.dispose", + "server.stop", + "desktopWindow.dispose", + "tray.dispose", + ]); + }); + + test("timeout path: result is 'timed_out' when server.stop never resolves", async () => { + const { deps } = makeStubDeps({ + server: { + stop: () => new Promise(() => {}), // never resolves + }, + }); + + // Stub setTimeoutFn that fires the callback immediately + const stubSetTimeout = ((cb: () => void) => { + cb(); + return 999 as unknown as ReturnType; + }) as unknown as typeof setTimeout; + + const result = await runShutdownSequence(deps, { + setTimeoutFn: stubSetTimeout, + }); + + assert.equal(result, "timed_out"); + }); + + test("failed path: server.stop rejects with an error", async () => { + const { deps } = makeStubDeps({ + server: { + stop: () => Promise.reject(new Error("stop failed")), + }, + }); + + // Use a setTimeoutFn that never fires so timeout doesn't win + const neverTimeout = (() => + 42 as unknown as ReturnType) as unknown as typeof setTimeout; + + const result = await runShutdownSequence(deps, { + setTimeoutFn: neverTimeout, + }); + + assert.equal(result, "failed"); + }); + + test("timer is cleared after cleanup resolves (no leaked handles)", async () => { + const { deps } = makeStubDeps(); + + let capturedTimerId: ReturnType | null = null; + let clearTimeoutCalledWith: unknown = null; + + // Monkey-patch clearTimeout to observe the call + const origClearTimeout = globalThis.clearTimeout; + globalThis.clearTimeout = ((id: unknown) => { + clearTimeoutCalledWith = id; + origClearTimeout(id as ReturnType); + }) as typeof clearTimeout; + + try { + // Use a real-ish setTimeoutFn that returns a recognizable timer id + const stubSetTimeout = ((_cb: () => void, _ms: number) => { + const id = origClearTimeout.bind( + null + ) as unknown as ReturnType; + capturedTimerId = 12345 as unknown as ReturnType; + return capturedTimerId; + }) as unknown as typeof setTimeout; + + const result = await runShutdownSequence(deps, { + setTimeoutFn: stubSetTimeout, + }); + + assert.equal(result, "clean"); + assert.equal( + clearTimeoutCalledWith, + capturedTimerId, + "clearTimeout should be called with the timer id returned by setTimeoutFn" + ); + } finally { + globalThis.clearTimeout = origClearTimeout; + } + }); +});