From f129e65575042ac048699dd0e5d6b839badd3fcb Mon Sep 17 00:00:00 2001 From: JonathanLab Date: Tue, 27 Jan 2026 14:52:08 +0100 Subject: [PATCH] fix: properly shutdown all services using preDestroy and unbindAll --- apps/twig/src/main/services/agent/service.ts | 3 +- .../services/app-lifecycle/service.test.ts | 39 +++++++------------ .../main/services/app-lifecycle/service.ts | 22 ++--------- .../src/main/services/connectivity/service.ts | 10 ++++- .../src/main/services/file-watcher/service.ts | 11 +++++- apps/twig/src/main/services/focus/service.ts | 3 +- .../src/main/services/focus/sync-service.ts | 3 +- apps/twig/src/main/services/shell/service.ts | 3 +- .../twig/src/main/services/updates/service.ts | 17 +++++++- 9 files changed, 61 insertions(+), 50 deletions(-) diff --git a/apps/twig/src/main/services/agent/service.ts b/apps/twig/src/main/services/agent/service.ts index d296f9a17..1126919ea 100644 --- a/apps/twig/src/main/services/agent/service.ts +++ b/apps/twig/src/main/services/agent/service.ts @@ -12,7 +12,7 @@ import { } from "@agentclientprotocol/sdk"; import { Agent, getLlmGatewayUrl, type OnLogCallback } from "@posthog/agent"; import { app } from "electron"; -import { injectable } from "inversify"; +import { injectable, preDestroy } from "inversify"; import type { AcpMessage } from "../../../shared/types/session-events.js"; import { logger } from "../../lib/logger.js"; import { TypedEventEmitter } from "../../lib/typed-event-emitter.js"; @@ -748,6 +748,7 @@ For git operations while detached: return `Your worktree is back on branch \`${context.branchName}\`. Normal git commands work again.`; } + @preDestroy() async cleanupAll(): Promise { log.info("Cleaning up all agent sessions", { sessionCount: this.sessions.size, diff --git a/apps/twig/src/main/services/app-lifecycle/service.test.ts b/apps/twig/src/main/services/app-lifecycle/service.test.ts index 744b4d254..45fd0dac9 100644 --- a/apps/twig/src/main/services/app-lifecycle/service.test.ts +++ b/apps/twig/src/main/services/app-lifecycle/service.test.ts @@ -1,13 +1,13 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; import { AppLifecycleService } from "./service.js"; -const { mockApp, mockAgentService, mockTrackAppEvent, mockShutdownPostHog } = +const { mockApp, mockContainer, mockTrackAppEvent, mockShutdownPostHog } = vi.hoisted(() => ({ mockApp: { exit: vi.fn(), }, - mockAgentService: { - cleanupAll: vi.fn(() => Promise.resolve()), + mockContainer: { + unbindAll: vi.fn(() => Promise.resolve()), }, mockTrackAppEvent: vi.fn(), mockShutdownPostHog: vi.fn(() => Promise.resolve()), @@ -33,10 +33,8 @@ vi.mock("../posthog-analytics.js", () => ({ shutdownPostHog: mockShutdownPostHog, })); -vi.mock("../../di/tokens.js", () => ({ - MAIN_TOKENS: { - AgentService: Symbol.for("AgentService"), - }, +vi.mock("../../di/container.js", () => ({ + container: mockContainer, })); vi.mock("../../../types/analytics.js", () => ({ @@ -50,11 +48,7 @@ describe("AppLifecycleService", () => { beforeEach(() => { vi.clearAllMocks(); - service = new AppLifecycleService(); - ( - service as unknown as { agentService: typeof mockAgentService } - ).agentService = mockAgentService; }); describe("isQuittingForUpdate", () => { @@ -69,9 +63,9 @@ describe("AppLifecycleService", () => { }); describe("shutdown", () => { - it("cleans up agents", async () => { + it("unbinds all container services", async () => { await service.shutdown(); - expect(mockAgentService.cleanupAll).toHaveBeenCalled(); + expect(mockContainer.unbindAll).toHaveBeenCalled(); }); it("tracks app quit event", async () => { @@ -87,8 +81,8 @@ describe("AppLifecycleService", () => { it("calls cleanup steps in order", async () => { const callOrder: string[] = []; - mockAgentService.cleanupAll.mockImplementation(async () => { - callOrder.push("cleanupAll"); + mockContainer.unbindAll.mockImplementation(async () => { + callOrder.push("unbindAll"); }); mockTrackAppEvent.mockImplementation(() => { callOrder.push("trackAppEvent"); @@ -100,16 +94,14 @@ describe("AppLifecycleService", () => { await service.shutdown(); expect(callOrder).toEqual([ - "cleanupAll", + "unbindAll", "trackAppEvent", "shutdownPostHog", ]); }); - it("continues shutdown if agent cleanup fails", async () => { - mockAgentService.cleanupAll.mockRejectedValue( - new Error("cleanup failed"), - ); + it("continues shutdown if container unbind fails", async () => { + mockContainer.unbindAll.mockRejectedValue(new Error("unbind failed")); await service.shutdown(); @@ -120,7 +112,6 @@ describe("AppLifecycleService", () => { it("continues shutdown if PostHog shutdown fails", async () => { mockShutdownPostHog.mockRejectedValue(new Error("posthog failed")); - // Should not throw await expect(service.shutdown()).resolves.toBeUndefined(); }); }); @@ -129,8 +120,8 @@ describe("AppLifecycleService", () => { it("calls shutdown before exit", async () => { const callOrder: string[] = []; - mockAgentService.cleanupAll.mockImplementation(async () => { - callOrder.push("cleanupAll"); + mockContainer.unbindAll.mockImplementation(async () => { + callOrder.push("unbindAll"); }); mockApp.exit.mockImplementation(() => { callOrder.push("exit"); @@ -138,7 +129,7 @@ describe("AppLifecycleService", () => { await service.shutdownAndExit(); - expect(callOrder[0]).toBe("cleanupAll"); + expect(callOrder[0]).toBe("unbindAll"); expect(callOrder[callOrder.length - 1]).toBe("exit"); }); diff --git a/apps/twig/src/main/services/app-lifecycle/service.ts b/apps/twig/src/main/services/app-lifecycle/service.ts index ca04e29ba..8426a405f 100644 --- a/apps/twig/src/main/services/app-lifecycle/service.ts +++ b/apps/twig/src/main/services/app-lifecycle/service.ts @@ -1,22 +1,14 @@ import { app } from "electron"; -import { inject, injectable } from "inversify"; +import { injectable } from "inversify"; import { ANALYTICS_EVENTS } from "../../../types/analytics.js"; -import { MAIN_TOKENS } from "../../di/tokens.js"; +import { container } from "../../di/container.js"; import { logger } from "../../lib/logger.js"; -import type { AgentService } from "../agent/service.js"; import { shutdownPostHog, trackAppEvent } from "../posthog-analytics.js"; -import type { ShellService } from "../shell/service.js"; const log = logger.scope("app-lifecycle"); @injectable() export class AppLifecycleService { - @inject(MAIN_TOKENS.AgentService) - private agentService!: AgentService; - - @inject(MAIN_TOKENS.ShellService) - private shellService!: ShellService; - private _isQuittingForUpdate = false; get isQuittingForUpdate(): boolean { @@ -31,15 +23,9 @@ export class AppLifecycleService { log.info("Performing graceful shutdown..."); try { - this.shellService.destroyAll(); - } catch (error) { - log.error("Error cleaning up ShellService during shutdown", error); - } - - try { - await this.agentService.cleanupAll(); + await container.unbindAll(); } catch (error) { - log.error("Error cleaning up agents during shutdown", error); + log.error("Error during container unbind", error); } trackAppEvent(ANALYTICS_EVENTS.APP_QUIT); diff --git a/apps/twig/src/main/services/connectivity/service.ts b/apps/twig/src/main/services/connectivity/service.ts index aea447053..2d0f03956 100644 --- a/apps/twig/src/main/services/connectivity/service.ts +++ b/apps/twig/src/main/services/connectivity/service.ts @@ -1,5 +1,5 @@ import { net } from "electron"; -import { injectable, postConstruct } from "inversify"; +import { injectable, postConstruct, preDestroy } from "inversify"; import { getBackoffDelay } from "../../../shared/utils/backoff.js"; import { logger } from "../../lib/logger.js"; import { TypedEventEmitter } from "../../lib/typed-event-emitter.js"; @@ -102,4 +102,12 @@ export class ConnectivityService extends TypedEventEmitter { this.schedulePoll(); }, interval); } + + @preDestroy() + stopPolling(): void { + if (this.pollTimeoutId) { + clearTimeout(this.pollTimeoutId); + this.pollTimeoutId = null; + } + } } diff --git a/apps/twig/src/main/services/file-watcher/service.ts b/apps/twig/src/main/services/file-watcher/service.ts index 25c7ee4c9..8779f3bae 100644 --- a/apps/twig/src/main/services/file-watcher/service.ts +++ b/apps/twig/src/main/services/file-watcher/service.ts @@ -3,7 +3,7 @@ import fs from "node:fs/promises"; import path from "node:path"; import * as watcher from "@parcel/watcher"; import { app } from "electron"; -import { injectable } from "inversify"; +import { injectable, preDestroy } from "inversify"; import { logger } from "../../lib/logger.js"; import { TypedEventEmitter } from "../../lib/typed-event-emitter.js"; import { @@ -89,6 +89,15 @@ export class FileWatcherService extends TypedEventEmitter { this.watchers.delete(repoPath); } + @preDestroy() + async shutdown(): Promise { + log.info("Shutting down file watcher service", { + watcherCount: this.watchers.size, + }); + const repoPaths = Array.from(this.watchers.keys()); + await Promise.all(repoPaths.map((repoPath) => this.stopWatching(repoPath))); + } + private get snapshotsDir(): string { return path.join(app.getPath("userData"), "snapshots"); } diff --git a/apps/twig/src/main/services/focus/service.ts b/apps/twig/src/main/services/focus/service.ts index 6f74a6321..7bd5597a0 100644 --- a/apps/twig/src/main/services/focus/service.ts +++ b/apps/twig/src/main/services/focus/service.ts @@ -3,7 +3,7 @@ import * as fs from "node:fs/promises"; import * as path from "node:path"; import { promisify } from "node:util"; import * as watcher from "@parcel/watcher"; -import { injectable } from "inversify"; +import { injectable, preDestroy } from "inversify"; import { logger } from "../../lib/logger"; import { TypedEventEmitter } from "../../lib/typed-event-emitter"; import { type FocusSession, focusStore } from "../../utils/store.js"; @@ -108,6 +108,7 @@ export class FocusService extends TypedEventEmitter { }); } + @preDestroy() async stopWatchingMainRepo(): Promise { if (this.mainRepoWatcher) { await this.mainRepoWatcher.unsubscribe(); diff --git a/apps/twig/src/main/services/focus/sync-service.ts b/apps/twig/src/main/services/focus/sync-service.ts index 9c72be770..46077f75c 100644 --- a/apps/twig/src/main/services/focus/sync-service.ts +++ b/apps/twig/src/main/services/focus/sync-service.ts @@ -2,7 +2,7 @@ import fs from "node:fs/promises"; import path from "node:path"; import * as watcher from "@parcel/watcher"; import ignore, { type Ignore } from "ignore"; -import { injectable } from "inversify"; +import { injectable, preDestroy } from "inversify"; import { logger } from "../../lib/logger.js"; import { git, withGitLock } from "./service.js"; @@ -149,6 +149,7 @@ export class FocusSyncService { } } + @preDestroy() async stopSync(): Promise { if (this.pending.timer) { clearTimeout(this.pending.timer); diff --git a/apps/twig/src/main/services/shell/service.ts b/apps/twig/src/main/services/shell/service.ts index 13ba2e8f7..5d566f090 100644 --- a/apps/twig/src/main/services/shell/service.ts +++ b/apps/twig/src/main/services/shell/service.ts @@ -2,7 +2,7 @@ import { exec, execSync } from "node:child_process"; import { existsSync } from "node:fs"; import { homedir, platform } from "node:os"; import path from "node:path"; -import { injectable } from "inversify"; +import { injectable, preDestroy } from "inversify"; import * as pty from "node-pty"; import { logger } from "../../lib/logger.js"; import { TypedEventEmitter } from "../../lib/typed-event-emitter.js"; @@ -215,6 +215,7 @@ export class ShellService extends TypedEventEmitter { * Destroy all active shell sessions. * Used during application shutdown to ensure all child processes are cleaned up. */ + @preDestroy() destroyAll(): void { log.info(`Destroying all shell sessions (${this.sessions.size} active)`); for (const sessionId of this.sessions.keys()) { diff --git a/apps/twig/src/main/services/updates/service.ts b/apps/twig/src/main/services/updates/service.ts index 0acdf8edc..6fd1139ed 100644 --- a/apps/twig/src/main/services/updates/service.ts +++ b/apps/twig/src/main/services/updates/service.ts @@ -1,5 +1,5 @@ import { app, autoUpdater } from "electron"; -import { inject, injectable, postConstruct } from "inversify"; +import { inject, injectable, postConstruct, preDestroy } from "inversify"; import { MAIN_TOKENS } from "../../di/tokens.js"; import { logger } from "../../lib/logger.js"; import { TypedEventEmitter } from "../../lib/typed-event-emitter.js"; @@ -30,6 +30,7 @@ export class UpdatesService extends TypedEventEmitter { private pendingNotification = false; private checkingForUpdates = false; private checkTimeoutId: ReturnType | null = null; + private checkIntervalId: ReturnType | null = null; private downloadedVersion: string | null = null; private initialized = false; @@ -155,7 +156,10 @@ export class UpdatesService extends TypedEventEmitter { this.performCheck(); // Set up periodic checks - setInterval(() => this.performCheck(), UpdatesService.CHECK_INTERVAL_MS); + this.checkIntervalId = setInterval( + () => this.performCheck(), + UpdatesService.CHECK_INTERVAL_MS, + ); } private handleError(error: Error): void { @@ -265,4 +269,13 @@ export class UpdatesService extends TypedEventEmitter { this.checkTimeoutId = null; } } + + @preDestroy() + shutdown(): void { + this.clearCheckTimeout(); + if (this.checkIntervalId) { + clearInterval(this.checkIntervalId); + this.checkIntervalId = null; + } + } }