diff --git a/apps/desktop/src/main/index.phase2.test.ts b/apps/desktop/src/main/index.phase2.test.ts index 50da0c0fe..a0ef84583 100644 --- a/apps/desktop/src/main/index.phase2.test.ts +++ b/apps/desktop/src/main/index.phase2.test.ts @@ -1744,7 +1744,9 @@ describe('main index phase2 exports', () => { )?.[1] as (event: { preventDefault: () => void }) => void beforeQuitHandler({ preventDefault: vi.fn() }) - expect(getFlushDoneListeners()).toHaveLength(browserWindows.length) + // One shared listener covers every in-flight flush; the request id is what + // separates them. + expect(getFlushDoneListeners()).toHaveLength(1) completeFlush(browserWindows[0]) await flushUntil(() => closeVaultMock.mock.calls.length > 0, 40) @@ -1755,6 +1757,40 @@ describe('main index phase2 exports', () => { expect(closeVaultMock).toHaveBeenCalled() }) + it('keeps one flush-done listener when more than ten windows flush at once', async () => { + // `app:flush-done` lives on the shared ipcMain, and Node warns + // (MaxListenersExceededWarning) past ten listeners on one channel. A + // listener per window tripped that on any quit with enough windows open. + vi.useFakeTimers() + whenReadyMock.mockResolvedValue(undefined) + + await importMainModule() + await flushReadyWork() + const { ipcMain } = await import('electron') + + for (let i = 0; i < 12; i++) createBrowserWindowMock() + expect(browserWindows.length).toBeGreaterThan(10) + + const beforeQuitHandler = appOnMock.mock.calls.find( + ([event]) => event === 'before-quit' + )?.[1] as (event: { preventDefault: () => void }) => void + beforeQuitHandler({ preventDefault: vi.fn() }) + + const flushListeners = getFlushDoneListeners() + expect(flushListeners).toHaveLength(1) + // Every window still got its own request, so no flush is silently skipped. + const requestIds = browserWindows.map((window) => getFlushRequestId(window)) + expect(requestIds.every((requestId) => typeof requestId === 'string')).toBe(true) + expect(new Set(requestIds).size).toBe(browserWindows.length) + + for (const window of browserWindows) completeFlush(window) + await flushUntil(() => closeVaultMock.mock.calls.length > 0) + + expect(closeVaultMock).toHaveBeenCalled() + // Nothing is pending any more, so the shared listener is detached too. + expect(ipcMain.removeListener).toHaveBeenCalledWith('app:flush-done', flushListeners[0]) + }) + it('ignores a flush-done from another window even when it carries the pending request id', async () => { // Defence in depth on top of the request id: no renderer may answer on // another window's behalf, because that window's saves are still in flight. diff --git a/apps/desktop/src/main/index.ts b/apps/desktop/src/main/index.ts index 65f6ab1e4..e8549b478 100644 --- a/apps/desktop/src/main/index.ts +++ b/apps/desktop/src/main/index.ts @@ -1916,6 +1916,52 @@ function registerQuickCaptureTestHooks(): void { // Track if shutdown is already in progress to prevent duplicate handling let isShuttingDown = false +const FLUSH_DONE_CHANNEL = 'app:flush-done' + +interface PendingFlush { + windowId: number + webContents: Electron.WebContents + settle: () => void +} + +// One entry per in-flight flush, keyed by request id. `app:flush-done` used to +// get one ipcMain listener per window, so a multi-window quit registered N +// listeners on the same channel and tripped Node's MaxListenersExceededWarning +// past 10 windows. The request id already scopes each reply, so a single shared +// listener plus this map does the same job with a constant listener count. +const pendingFlushes = new Map() +let flushDoneListenerAttached = false + +// A reply only counts when it comes from the window we asked AND answers the +// request we are waiting on. Without both checks the first window to reply +// resolved all of them, and a late reply to an earlier request satisfied the +// next one — either way a renderer gets torn down with unsaved edits pending. +const handleFlushDone = (event: IpcMainEvent, doneRequestId?: string): void => { + if (typeof doneRequestId !== 'string') return + const pending = pendingFlushes.get(doneRequestId) + if (!pending) return + if (event.sender !== pending.webContents) return + shutdownLog.info('flushWindow: flush-done received from window', pending.windowId) + pending.settle() +} + +function addPendingFlush(requestId: string, pending: PendingFlush): void { + pendingFlushes.set(requestId, pending) + if (flushDoneListenerAttached) return + ipcMain.on(FLUSH_DONE_CHANNEL, handleFlushDone) + flushDoneListenerAttached = true +} + +function removePendingFlush(requestId: string): void { + pendingFlushes.delete(requestId) + if (pendingFlushes.size > 0 || !flushDoneListenerAttached) return + // Detach once nothing is waiting. The timeout path used to skip cleanup: a + // renderer that never answers left a listener on the shared ipcMain forever, + // and window-close runs this on every close. + ipcMain.removeListener(FLUSH_DONE_CHANNEL, handleFlushDone) + flushDoneListenerAttached = false +} + function flushWindow(win: BrowserWindow, timeoutMs = 2000): Promise { return new Promise((resolve) => { if (win.isDestroyed() || !win.webContents) { @@ -1926,7 +1972,6 @@ function flushWindow(win: BrowserWindow, timeoutMs = 2000): Promise { shutdownLog.info('flushWindow: requesting flush from window', win.id) - const channel = 'app:flush-done' const requestId = randomUUID() let settled = false @@ -1934,31 +1979,16 @@ function flushWindow(win: BrowserWindow, timeoutMs = 2000): Promise { if (settled) return settled = true clearTimeout(timer) - // The timeout path used to skip this: a renderer that never answers left a - // listener on the shared ipcMain forever, and window-close runs this on - // every close. - ipcMain.removeListener(channel, handler) + removePendingFlush(requestId) resolve() } - // `app:flush-done` is one shared channel for every in-flight flush, so a - // reply only counts when it comes from this window AND answers this request. - // Without both checks the first window to reply resolved all of them, and a - // late reply to an earlier request satisfied the next one — either way a - // renderer gets torn down with unsaved edits still pending. - const handler = (event: IpcMainEvent, doneRequestId?: string): void => { - if (event.sender !== win.webContents) return - if (doneRequestId !== requestId) return - shutdownLog.info('flushWindow: flush-done received from window', win.id) - settle() - } - const timer = setTimeout(() => { shutdownLog.warn('flushWindow: timeout for window', win.id) settle() }, timeoutMs) - ipcMain.on(channel, handler) + addPendingFlush(requestId, { windowId: win.id, webContents: win.webContents, settle }) win.webContents.send('app:request-flush', requestId) }) } diff --git a/apps/desktop/src/main/lib/window-rpc.test.ts b/apps/desktop/src/main/lib/window-rpc.test.ts index 5d7e8a4ac..c7ade42cd 100644 --- a/apps/desktop/src/main/lib/window-rpc.test.ts +++ b/apps/desktop/src/main/lib/window-rpc.test.ts @@ -23,6 +23,27 @@ vi.mock('electron', () => ({ import { ipcMain } from 'electron' import { mainToRendererInvoke } from './window-rpc' +// Minimal stand-in for the target `webContents`: `send` plus the EventEmitter +// surface the RPC uses to notice the window going away mid-call. +function createWebContentsMock() { + const destroyedListeners = new Set<() => void>() + return { + send: vi.fn(), + once: vi.fn((event: string, listener: () => void) => { + if (event === 'destroyed') destroyedListeners.add(listener) + }), + removeListener: vi.fn((event: string, listener: () => void) => { + if (event === 'destroyed') destroyedListeners.delete(listener) + }), + emitDestroyed: () => { + for (const listener of [...destroyedListeners]) { + destroyedListeners.delete(listener) + listener() + } + } + } +} + describe('mainToRendererInvoke', () => { beforeEach(() => { vi.useFakeTimers() @@ -37,7 +58,7 @@ describe('mainToRendererInvoke', () => { }) it('sends a main invoke request and resolves with the matching renderer response', async () => { - const webContents = { send: vi.fn() } + const webContents = createWebContentsMock() const win = { isDestroyed: () => false, webContents @@ -61,7 +82,7 @@ describe('mainToRendererInvoke', () => { }) it('ignores responses from other renderers and resolves null on timeout', async () => { - const webContents = { send: vi.fn() } + const webContents = createWebContentsMock() const win = { isDestroyed: () => false, webContents @@ -76,4 +97,32 @@ describe('mainToRendererInvoke', () => { await expect(promise).resolves.toBeNull() }) + + it('drops the per-call listener as soon as the target window is destroyed', async () => { + // A window that goes away mid-call never answers. Waiting out the full + // timeout parks a listener on the shared ipcMain (and the caller) for + // nothing, so teardown has to release it immediately. + const webContents = createWebContentsMock() + const win = { + isDestroyed: () => false, + webContents + } as unknown as Electron.BrowserWindow + + const promise = mainToRendererInvoke(win, 'agent_mcp:get_current_note', undefined, { + timeoutMs: 2_000 + }) + + expect(hoisted.listeners.has('main:invoke:response:request-1')).toBe(true) + + webContents.emitDestroyed() + + await expect(promise).resolves.toBeNull() + expect(hoisted.listeners.has('main:invoke:response:request-1')).toBe(false) + expect(ipcMain.removeListener).toHaveBeenCalledWith( + 'main:invoke:response:request-1', + expect.any(Function) + ) + // Timers are still frozen: the cleanup happened on teardown, not on timeout. + expect(vi.getTimerCount()).toBe(0) + }) }) diff --git a/apps/desktop/src/main/lib/window-rpc.ts b/apps/desktop/src/main/lib/window-rpc.ts index 3facbe241..22b08205e 100644 --- a/apps/desktop/src/main/lib/window-rpc.ts +++ b/apps/desktop/src/main/lib/window-rpc.ts @@ -25,8 +25,11 @@ export async function mainToRendererInvoke( options: MainToRendererInvokeOptions = {} ): Promise { if (win.isDestroyed()) return null - if (typeof win.webContents.isDestroyed === 'function' && win.webContents.isDestroyed()) - return null + // Resolve the target once: reading `win.webContents` again from inside the + // response handler throws if the window was destroyed while the call was in + // flight, and the captured reference keeps the sender check stable. + const target = win.webContents + if (typeof target.isDestroyed === 'function' && target.isDestroyed()) return null const requestId = randomUUID() const responseChannel = getResponseChannel(requestId) @@ -38,6 +41,7 @@ export async function mainToRendererInvoke( const cleanup = (): void => { clearTimeout(timeout) ipcMain.removeListener(responseChannel, handleResponse) + target.removeListener('destroyed', handleTargetDestroyed) } const settle = (result: T | null): void => { @@ -47,17 +51,26 @@ export async function mainToRendererInvoke( resolve(result) } + // A reply on this per-request channel only counts when it comes from the + // window we asked. A foreign sender is dropped rather than settled — letting + // it settle would hand any renderer a way to null out another window's call. const handleResponse = (event: IpcMainEvent, result: T | null): void => { - if (event.sender !== win.webContents) return + if (event.sender !== target) return settle(result) } + // The window went away before answering (closed, or reloaded out from under + // the request). No reply is ever coming, so drop the per-call listener now + // instead of parking it on the shared ipcMain until the timeout fires. + const handleTargetDestroyed = (): void => settle(null) + ipcMain.on(responseChannel, handleResponse) + target.once('destroyed', handleTargetDestroyed) const timeout = setTimeout(() => settle(null), timeoutMs) try { const message: MainInvokePayload = { requestId, channel, payload } - win.webContents.send(MAIN_INVOKE_CHANNEL, message) + target.send(MAIN_INVOKE_CHANNEL, message) } catch { settle(null) } diff --git a/apps/docs/src/architecture/ipc.md b/apps/docs/src/architecture/ipc.md index 80e139f8f..6b8731466 100644 --- a/apps/docs/src/architecture/ipc.md +++ b/apps/docs/src/architecture/ipc.md @@ -122,3 +122,11 @@ The redundant work is therefore removed at the value, not at the sender. Subscri This matters most on the sync path: `sync/item-handlers/settings-handler.ts` re-broadcasts the whole merged `general` / `editor` / `inbox` group on every applied settings item, not only the fields that differ, so without the identity check each apply re-rendered every settings consumer in every window. React cannot use its eager-state shortcut on the first dispatch after a real state change, so that one still renders the hook's own component once before bailing out; every subsequent no-op echo costs zero renders. Write the merge as `setSettings((prev) => mergeSettingsPatch(prev, value))`, never `setSettings((prev) => ({ ...prev, ...value }))` — the spread mints a new object every time and defeats the bail-out. The comparison is shallow, matching the merge it guards; groups whose values are nested objects (keyboard bindings arrive as fresh references over IPC) simply never hit the bail-out, which is correct rather than a missed update. + +## Main → Renderer Request/Response + +Two main-process handshakes need an _answer_ from one specific window rather than a fan-out: `mainToRendererInvoke` in `src/main/lib/window-rpc.ts` (Vault MCP tools asking a window for the current note, a desktop API call, a canvas write) and the shutdown flush in `src/main/index.ts`, which asks each window to persist pending edits before the app quits. Both reply over `ipcMain`, which is process-wide, so both follow the same two rules. + +**Scope every reply to its request.** `mainToRendererInvoke` gives each call its own response channel (`main:invoke:response:`); the flush handshake shares one `app:flush-done` channel and carries the request id in the payload. Both then also compare `event.sender` against the window that was asked. Without the sender check a renderer could answer on another window's behalf — for the flush that means a window gets torn down with unsaved edits still in flight. A reply from an unexpected sender is dropped, never treated as a settled answer. + +**Never leave a listener behind, and never scale listener count with window count.** Node warns (`MaxListenersExceededWarning`) past ten listeners on a single channel, so the flush keeps one shared `app:flush-done` listener plus a map of pending request ids: it attaches when the first flush starts and detaches when the last one settles, whether that was a reply or the 2 s timeout. A per-call listener is fine on a per-request channel, but it has to be released on every exit path — `mainToRendererInvoke` drops its listener on reply, on timeout, and as soon as the target `webContents` is destroyed, so a window closed mid-call resolves `null` right away instead of parking a listener for the rest of the timeout.