diff --git a/.ai/contexts/viewer-panel.md b/.ai/contexts/viewer-panel.md index 3d7d826de..1d455ab32 100644 --- a/.ai/contexts/viewer-panel.md +++ b/.ai/contexts/viewer-panel.md @@ -165,7 +165,7 @@ Each of these reads and clears `pendingTouchedOpen` before `destroyCurrentTab`, Nothing ends the window with unsaved file-panel edits without the user saying so. The scope is the Touched editor in the slot (a tab with an `absolutePath`) and every `touchedStashes` entry, in every session of `filePanelState`; the Memory and Work Files panels, MCP diff tabs and Changes buffers are not covered. - **Main** (`unsaved-guard.js`, one guard, `attach(win)` per window, `beforeQuit(event, win)` for the app). `main.js`'s `before-quit` handler calls `beforeQuit` first: it prevents the quit, asks the renderer, and calls `app.quit()` again on a yes, so the cleanup that follows (PTYs killed, MCP servers, watchers) runs only once the quit is confirmed and a Cancel leaves the app intact. This covers every `app.quit()` caller (☰ Quit, the last window closing). `updater-install` asks first (`confirmQuit`), because electron-updater's `quitAndInstall()` starts the installer before it calls `app.quit()`: a Cancel must leave the installer unstarted, and a yes pre-approves the quit that follows. A Windows `session-end` approves the quit, so logoff and shutdown never wait on the dialog; `query-session-end` approves it for 60 s (`LOGOFF_CANCEL_MS`), so a logoff that is cancelled does not skip the question on every later quit. A window close, a quit and an install share one question while it is open; one that joins a `'reload'` question sends `unsaved-check-reason` (`id`, `'quit'`), and the renderer acts on the reason it holds when the user answers. The window's own `close` event is held the same way for a plain window close. The question is `unsaved-check` (`id`, `'quit'` or `'reload'`); the renderer's `unsaved-check-result` (`id`, `proceed`) approves it. A `will-prevent-unload` (the renderer's `beforeunload` veto, which a reload hits) asks with `'reload'`; on a yes the next unload is allowed once (`allowNextUnload`, answered with `preventDefault()`, which in Electron means "unload anyway") and `webContents.reload()` is called. Every close asks the renderer, even with nothing dirty; a main-side dirty flag would save that round trip and is not built. -- **Bounded.** The renderer acknowledges (`unsaved-check-ack`) as soon as it receives the check, before any dialog. The 2.5 s bound (`DEFAULT_TIMEOUT_MS`) covers only send to ack: no ack answers yes, so a hung renderer never keeps the app from quitting. After the ack the guard waits for the answer without a limit, so a slow user or a slow save loses nothing, and answers yes only if the renderer process is gone (`render-process-gone`, `destroyed`) or a send fails. A late answer is ignored. +- **Bounded.** The renderer acknowledges (`unsaved-check-ack`) as soon as it receives the check, before any dialog. The 2.5 s bound (`DEFAULT_TIMEOUT_MS`) covers only send to ack: no ack answers yes, so a hung renderer never keeps the app from quitting. The window is then closed the usual way (`win.close()`, or `app.quit()` for a quit or an update install), so a page that is only slow (still starting, or busy) closes as before, without the question, once it runs its `beforeunload`. A hung page never runs it, and that close would never finish: the guard destroys such a window when Electron reports it `unresponsive`, or when it is closed again, the guard's own `close()` or `app.quit()` aside. Electron reports a page `unresponsive` about 5 s after a close it has not completed, and cannot tell a hung page from a busy one: a page still busy about 5 s after that close, about 7.5 s after the click counting the 2.5 s bound, is destroyed, and its pending work is lost (the debounced working-set save among it). Only a page that recovers before then closes normally; a stall of 8–10 s that starts before the click (#526) does not. `e2e/hung-window.spec.js` measures it: the app exits about 7.5 s after the close it schedules. A page reported unresponsive earlier, and neither `responsive` nor acknowledging a check since, is destroyed when that close reaches it: right after `win.close()` for a window close, and for a quit or an update install only once the quit closes the window (its `close` event), so a quit still held by another `before-quit` handler (the ActivityWatch flush) is not cut short. A check the page acknowledged never destroys the window, however long the user takes. After the ack the guard waits for the answer without a limit, so a slow user or a slow save loses nothing, and answers yes only if the renderer process is gone (`render-process-gone`, `destroyed`) or a send fails. A late answer is ignored. - **Renderer** (`file-panel.js`). `askAboutUnsavedEdits()` lists the dirty tabs (`collectUnsavedFileTabs`) in the `#unsaved-edits-dialog` dialog, built on the add-project dialog's classes (already in the frameless no-drag list). Save writes every dirty entry: the editor in the slot through `handleChangesSave`, a stashed one through `saveFileForPanel(filePath, content, savedContent, {git, version})`, removed from its session once written. A save that fails (including a disk that moved) keeps the dialog open with the reason, and nothing is answered. Discard answers yes. Cancel and Escape answer no. A second request while the dialog is open gets the same dialog's answer. - **`beforeunload`.** The renderer vetoes an unload while an entry is dirty, unless `unloadApproved`, set for 10 s after the user answered yes. That is what stops a reload from the keyboard or devtools from slipping past, and what keeps the veto from asking a second time after an approved close. - **Closing a session** does not drop its file panel: `destroySession` leaves `filePanelState` alone, so the tab and the stashes, dirty or not, are still there when the session is opened again, and the quit check still sees them. Deleting a session leaves its state in memory too, so its edits are asked about at quit rather than lost. diff --git a/CHANGELOG.md b/CHANGELOG.md index 54f2a3db8..6b7be204a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ What changes for you in each release of Switchboard. How to write an entry: [doc - Switching to a hidden terminal tab no longer disposes and recreates its graphics context every time, which blocked the window for several seconds. The three most recently shown terminals keep theirs, and a context that has to be recreated is made once the tab is shown. (#526) - In a sandboxed session, a repository or a `.claude` folder below an Additional Directory that is a symbolic link now has its Git config, hooks and Claude settings read-only, like one below a plain directory. Before, they were writable from inside the sandbox. Such a directory holding a `.claude` that is itself a symbolic link, or a repository Git cannot read, now refuses to start the session (status 125), as a plain directory already did, instead of starting it unprotected. (#538) - A double click on a session in the sidebar opens it once, instead of sometimes starting two terminals that resume the same conversation. (#539) +- Closing the window or quitting no longer leaves Switchboard running when its window has stopped responding: the window closes as before, and once the system reports it not responding, about 5 seconds after the close, or once you close it again, it is closed anyway. A window still busy at that point is closed too, and its last unsaved changes to the list of open sessions are lost; one that recovers sooner closes normally. (#540) ## v0.0.92 — 2026-10-10 diff --git a/docs/e2e.md b/docs/e2e.md index 063a6a7f5..c874a0327 100644 --- a/docs/e2e.md +++ b/docs/e2e.md @@ -49,6 +49,7 @@ here. | The shell opened with no tab fills the panel and is not a sidebar row | `panel.spec.js` | the `.shell-only` layout, and `buildProjectsFromCache` skipping the panel shell | | Changes on a project with no git work tree says so, with no git output | `panel.spec.js` | the not-a-repository note instead of git's raw output | | Refresh beside Stop redraws a plain terminal and restores its fitted size | `terminal-refresh.spec.js` | visible header geometry, one refresh IPC, actual PTY size events, and final fitted size | +| Closing a window whose page hangs still ends the app | `hung-window.spec.js` | the 2.5 s bound on an unacknowledged check, the window destroyed once Electron reports the page unresponsive, and exit code 0 | The issue's fifth journey expected the Changes control to disappear. #310 made it unconditional, so the journey checks what the panel says instead (see @@ -94,7 +95,9 @@ the OS temporary directory locally, outside the app directory. `taskkill /T /F` on that pid, which kills the process and its descendants. Then it deletes the temporary `HOME`. If the app exits within the 10 s, no - kill is sent. + kill is sent. A journey may end the app itself (`hung-window.spec.js` does); + the fixture then skips the trace, the screenshot and the close, and only + deletes the temporary `HOME`. ## Proving a journey can fail diff --git a/e2e/fixtures.js b/e2e/fixtures.js index 941450285..616c99589 100644 --- a/e2e/fixtures.js +++ b/e2e/fixtures.js @@ -102,9 +102,8 @@ function killTree(proc) { } } -async function closeApp(app) { - const proc = app.process(); - await bounded(app.close(), CLOSE_TIMEOUT_MS); +async function closeApp(app, proc) { + if (!hasExited(proc)) await bounded(Promise.resolve().then(() => app.close()), CLOSE_TIMEOUT_MS); if (await waitForExit(proc, CLOSE_TIMEOUT_MS)) return; killTree(proc); await waitForExit(proc, CLOSE_TIMEOUT_MS); @@ -126,6 +125,7 @@ const test = base.extend({ launch: async ({ env }, use, testInfo) => { let app = null; + let proc = null; let page = null; const launch = async () => { app = await electron.launch({ @@ -134,6 +134,7 @@ const test = base.extend({ cwd: APP_DIR, env: { ...env, ELECTRON_IS_DEV: '0' }, }); + proc = app.process(); await app.context().tracing.start({ screenshots: true, snapshots: true }); page = await app.firstWindow(); await page.locator('.project-new-btn').first().waitFor(); @@ -145,10 +146,12 @@ const test = base.extend({ if (app) { const failed = testInfo.status !== testInfo.expectedStatus; try { - if (failed && page) await bounded(page.screenshot({ path: testInfo.outputPath('failure.png'), timeout: CLOSE_TIMEOUT_MS }), CLOSE_TIMEOUT_MS); - await bounded(app.context().tracing.stop(failed ? { path: testInfo.outputPath('trace.zip') } : undefined), CLOSE_TIMEOUT_MS * 3); + if (!hasExited(proc)) { + if (failed && page) await bounded(page.screenshot({ path: testInfo.outputPath('failure.png'), timeout: CLOSE_TIMEOUT_MS }), CLOSE_TIMEOUT_MS); + await bounded(app.context().tracing.stop(failed ? { path: testInfo.outputPath('trace.zip') } : undefined), CLOSE_TIMEOUT_MS * 3); + } } finally { - await closeApp(app); + await closeApp(app, proc); } } } diff --git a/e2e/hung-window.spec.js b/e2e/hung-window.spec.js new file mode 100644 index 000000000..66925ec03 --- /dev/null +++ b/e2e/hung-window.spec.js @@ -0,0 +1,17 @@ +'use strict'; + +const { test, expect, makeRepo } = require('./fixtures'); + +const EXIT_TIMEOUT_MS = 60_000; + +test('closing a window whose page hangs still ends the app', async ({ home, env, launch }) => { + test.setTimeout(150_000); + makeRepo(home, env, { 'readme.txt': 'fixture\n' }); + const { app, page } = await launch(); + const proc = app.process(); + await app.evaluate(({ BrowserWindow }) => { setTimeout(() => BrowserWindow.getAllWindows()[0].close(), 2000); }); + await page.evaluate(() => { setTimeout(() => { for (;;) Date.now(); }, 500); }); + await expect.poll(() => proc.exitCode !== null || proc.signalCode !== null, { timeout: EXIT_TIMEOUT_MS }).toBe(true); + expect(proc.signalCode).toBe(null); + expect(proc.exitCode).toBe(0); +}); diff --git a/test/unsaved-guard.test.js b/test/unsaved-guard.test.js index c94c84610..7668c91a3 100644 --- a/test/unsaved-guard.test.js +++ b/test/unsaved-guard.test.js @@ -9,7 +9,7 @@ const { EventEmitter } = require('node:events'); const { createUnsavedGuard, LOGOFF_CANCEL_MS } = require('../unsaved-guard'); -function setup({ timeoutMs = 1000, quit } = {}) { +function setup({ timeoutMs = 1000, quit, closeFinishes = false } = {}) { const ipcMain = new EventEmitter(); const timers = []; const setTimeoutFn = (fn, ms) => { const t = { fn, ms, cleared: false }; timers.push(t); return t; }; @@ -24,7 +24,15 @@ function setup({ timeoutMs = 1000, quit } = {}) { reloads: 0, }); const win = new EventEmitter(); - Object.assign(win, { webContents: wc, isDestroyed: () => false, close: () => { win.closes += 1; }, closes: 0 }); + Object.assign(win, { + webContents: wc, + isDestroyed: () => win.destroys > 0 || win.closed, + close: () => { win.closes += 1; if (closeFinishes) win.closed = true; }, + destroy: () => { win.destroys += 1; }, + closes: 0, + destroys: 0, + closed: false, + }); const guard = createUnsavedGuard({ ipcMain, timeoutMs, setTimeoutFn, clearTimeoutFn, quit }); guard.attach(win); @@ -299,3 +307,133 @@ test('a logoff that never ends the session stops approving quits after a while; assert.ok(s.timers.filter((x) => x.ms === LOGOFF_CANCEL_MS).every((x) => x.cleared)); assert.equal(s.guard.beforeQuit(s.closeEvent(), s.win), false); }); + +async function closeUnanswered(t) { + t.win.emit('close', t.closeEvent()); + t.timers.filter((x) => x.ms === 2500 && !x.cleared).pop().fn(); + await tick(); +} + +test('a page that never acknowledges is closed the usual way, and destroyed once reported unresponsive', async () => { + const t = setup({ timeoutMs: 2500 }); + await closeUnanswered(t); + assert.equal(t.win.closes, 1, 'a page that is only slow still closes the usual way'); + assert.equal(t.win.destroys, 0); + assert.equal(t.timers.filter((x) => !x.cleared && x.ms !== 2500).length, 0, 'no fixed delay destroys a busy page'); + t.win.emit('unresponsive'); + assert.equal(t.win.destroys, 1, 'a hung page never runs its beforeunload, so its close would never finish'); +}); + +test('a busy page that answers again is not destroyed', async () => { + const t = setup({ timeoutMs: 2500, closeFinishes: true }); + await closeUnanswered(t); + t.win.emit('responsive'); + t.win.emit('unresponsive'); + assert.equal(t.win.destroys, 0, 'the close finished, nothing left to force'); +}); + +test('a page already reported unresponsive is destroyed when its unanswered close does not finish', async () => { + const t = setup({ timeoutMs: 2500 }); + t.win.emit('unresponsive'); + assert.equal(t.win.destroys, 0, 'unresponsive alone never destroys'); + await closeUnanswered(t); + assert.equal(t.win.destroys, 1); + + const recovered = setup({ timeoutMs: 2500 }); + recovered.win.emit('unresponsive'); + recovered.win.emit('responsive'); + await closeUnanswered(recovered); + assert.equal(recovered.win.destroys, 0, 'a page that answered again is given its close'); +}); + +test('closing again a window whose unanswered close did not finish destroys it', async () => { + const t = setup({ timeoutMs: 2500 }); + await closeUnanswered(t); + const ours = t.closeEvent(); + t.win.emit('close', ours); + assert.equal(t.win.destroys, 0, 'the close the guard itself started is not a second one'); + const again = t.closeEvent(); + t.win.emit('close', again); + assert.equal(t.win.destroys, 1); +}); + +test('a quit the page never acknowledges goes ahead, and the window is destroyed once reported unresponsive', async () => { + let quits = 0; + const t = setup({ timeoutMs: 2500, quit: () => { quits += 1; } }); + assert.equal(t.guard.beforeQuit(t.closeEvent(), t.win), true); + t.timers.find((x) => x.ms === 2500).fn(); + await tick(); + assert.equal(quits, 1); + assert.equal(t.win.destroys, 0); + t.win.emit('close', t.closeEvent()); + assert.equal(t.win.destroys, 0, 'app.quit() closes the window the usual way'); + t.win.emit('unresponsive'); + assert.equal(t.win.destroys, 1, 'a hung page holds that close'); +}); + +test('an answered check never destroys the window, even while the user takes time to answer', async () => { + const t = setup({ timeoutMs: 2500 }); + t.win.emit('close', t.closeEvent()); + t.ack(t.sent[0].args[0]); + t.win.emit('unresponsive'); + t.answer(t.sent[0].args[0], true); + await tick(); + t.win.emit('unresponsive'); + t.win.emit('close', t.closeEvent()); + t.win.emit('close', t.closeEvent()); + assert.equal(t.win.closes, 1); + assert.equal(t.win.destroys, 0); +}); + +test('a page that acknowledges a check is no longer counted unresponsive', async () => { + const t = setup({ timeoutMs: 2500 }); + t.win.emit('unresponsive'); + t.win.emit('close', t.closeEvent()); + t.ack(t.sent[0].args[0]); + t.answer(t.sent[0].args[0], false); + await tick(); + await closeUnanswered(t); + assert.equal(t.win.closes, 1); + assert.equal(t.win.destroys, 0, 'a busy page is given its close'); +}); + +test('an updater install the page never acknowledges is approved, and the window destroyed once reported unresponsive', async () => { + const t = setup({ timeoutMs: 2500 }); + const confirmed = t.guard.confirmQuit(t.win); + t.timers.find((x) => x.ms === 2500).fn(); + assert.equal(await confirmed, true); + assert.equal(t.win.destroys, 0, 'quitAndInstall still closes the window the usual way'); + t.win.emit('unresponsive'); + assert.equal(t.win.destroys, 0, 'not before the quit reaches the window'); + t.win.emit('close', t.closeEvent()); + assert.equal(t.win.destroys, 1); + assert.equal(t.guard.beforeQuit(t.closeEvent(), t.win), false, 'the quit that follows is approved'); +}); + +test('a quit held by another before-quit handler does not destroy the window before it closes it', async () => { + let quits = 0; + const t = setup({ timeoutMs: 2500, quit: () => { quits += 1; } }); + t.win.emit('unresponsive'); + assert.equal(t.guard.beforeQuit(t.closeEvent(), t.win), true); + t.timers.find((x) => x.ms === 2500).fn(); + await tick(); + assert.equal(quits, 1); + assert.equal(t.win.destroys, 0, 'the quit may still be held, by the ActivityWatch flush'); + t.win.emit('unresponsive'); + assert.equal(t.win.destroys, 0); + const e = t.closeEvent(); + t.win.emit('close', e); + assert.equal(e.prevented, true); + assert.equal(t.win.destroys, 1, 'the quit closing a window already reported unresponsive destroys it'); + + const recovered = setup({ timeoutMs: 2500, quit: () => {} }); + recovered.win.emit('unresponsive'); + recovered.guard.beforeQuit(recovered.closeEvent(), recovered.win); + recovered.timers.find((x) => x.ms === 2500).fn(); + await tick(); + recovered.win.emit('responsive'); + recovered.win.emit('close', recovered.closeEvent()); + assert.equal(recovered.win.destroys, 0, 'a page that answers again is given its close'); + recovered.win.emit('unresponsive'); + assert.equal(recovered.win.destroys, 1); +}); diff --git a/unsaved-guard.js b/unsaved-guard.js index 46a5638ed..3dbbc846d 100644 --- a/unsaved-guard.js +++ b/unsaved-guard.js @@ -14,12 +14,15 @@ function createUnsavedGuard({ ipcMain, timeoutMs = DEFAULT_TIMEOUT_MS, logoffCan let quitAsking = false; let inflight = null; const approvedToQuit = () => quitApproved || logoffApproved; + const windows = new WeakMap(); ipcMain.on('unsaved-check-ack', (_event, id) => { const entry = pending.get(id); if (!entry || entry.acked) return; entry.acked = true; clearTimeoutFn(entry.timer); + const state = windows.get(entry.win); + if (state) state.unresponsive = false; }); ipcMain.on('unsaved-check-result', (_event, id, proceed) => { @@ -28,7 +31,7 @@ function createUnsavedGuard({ ipcMain, timeoutMs = DEFAULT_TIMEOUT_MS, logoffCan entry.finish(proceed === true); }); - function ask(win, reason) { + function askFull(win, reason) { if (inflight) { if (inflight.reason === 'reload') { inflight.reason = reason; @@ -37,13 +40,13 @@ function createUnsavedGuard({ ipcMain, timeoutMs = DEFAULT_TIMEOUT_MS, logoffCan return inflight.asked; } const wc = win.webContents; - if (win.isDestroyed() || !wc || wc.isDestroyed() || wc.isCrashed()) return Promise.resolve(true); + if (win.isDestroyed() || !wc || wc.isDestroyed() || wc.isCrashed()) return Promise.resolve({ proceed: true, unanswered: false }); let settled = false; const id = nextId++; const asked = new Promise((resolve) => { - const entry = { acked: false, timer: null, finish: null }; + const entry = { win, acked: false, timer: null, finish: null }; const onGone = () => entry.finish(true); - entry.finish = (proceed) => { + entry.finish = (proceed, unanswered = false) => { if (!pending.has(id)) return; pending.delete(id); clearTimeoutFn(entry.timer); @@ -51,9 +54,9 @@ function createUnsavedGuard({ ipcMain, timeoutMs = DEFAULT_TIMEOUT_MS, logoffCan wc.removeListener('destroyed', onGone); settled = true; inflight = null; - resolve(proceed); + resolve({ proceed, unanswered }); }; - entry.timer = setTimeoutFn(() => entry.finish(true), timeoutMs); + entry.timer = setTimeoutFn(() => entry.finish(true, true), timeoutMs); pending.set(id, entry); wc.on('render-process-gone', onGone); wc.on('destroyed', onGone); @@ -67,15 +70,39 @@ function createUnsavedGuard({ ipcMain, timeoutMs = DEFAULT_TIMEOUT_MS, logoffCan return asked; } + function ask(win, reason) { + return askFull(win, reason).then(({ proceed }) => proceed); + } + + function force(win) { + if (!win.isDestroyed()) win.destroy(); + } + + function markUnanswered(win) { + const state = windows.get(win); + if (state) state.unanswered = true; + } + + function markQuitUnanswered(win) { + const state = windows.get(win); + if (state) state.quitUnanswered = true; + } + + function forceIfUnresponsive(win) { + const state = windows.get(win); + if (state && state.unresponsive) force(win); + } + function beforeQuit(event, win) { if (approvedToQuit() || !win || win.isDestroyed()) return false; event.preventDefault(); if (quitAsking) return true; quitAsking = true; - ask(win, 'quit').then((proceed) => { + askFull(win, 'quit').then(({ proceed, unanswered }) => { quitAsking = false; if (!proceed) return; quitApproved = true; + if (unanswered) markQuitUnanswered(win); quit(); }); return true; @@ -86,6 +113,8 @@ function createUnsavedGuard({ ipcMain, timeoutMs = DEFAULT_TIMEOUT_MS, logoffCan let closing = false; let reloading = false; let allowNextUnload = false; + const state = { unanswered: false, quitUnanswered: false, unresponsive: false, closesSeen: 0 }; + windows.set(win, state); const endSession = (final) => { logoffApproved = true; @@ -100,17 +129,39 @@ function createUnsavedGuard({ ipcMain, timeoutMs = DEFAULT_TIMEOUT_MS, logoffCan }; win.on('query-session-end', () => endSession(false)); win.on('session-end', () => endSession(true)); + win.on('unresponsive', () => { + state.unresponsive = true; + if (state.unanswered) force(win); + }); + win.on('responsive', () => { state.unresponsive = false; }); win.on('close', (event) => { - if (approved || approvedToQuit()) return; + if (approved || approvedToQuit()) { + if (state.quitUnanswered && !state.unanswered) { + state.unanswered = true; + if (state.unresponsive) { + event.preventDefault(); + force(win); + return; + } + } + if (state.unanswered && ++state.closesSeen > 1) { + event.preventDefault(); + force(win); + } + return; + } event.preventDefault(); if (closing) return; closing = true; - ask(win, 'quit').then((proceed) => { + askFull(win, 'quit').then(({ proceed, unanswered }) => { closing = false; if (!proceed) return; approved = true; - if (!win.isDestroyed()) win.close(); + if (win.isDestroyed()) return; + if (unanswered) markUnanswered(win); + win.close(); + if (unanswered) forceIfUnresponsive(win); }); }); @@ -132,9 +183,11 @@ function createUnsavedGuard({ ipcMain, timeoutMs = DEFAULT_TIMEOUT_MS, logoffCan } function confirmQuit(win) { - return ask(win, 'quit').then((proceed) => { - if (proceed) quitApproved = true; - return proceed; + return askFull(win, 'quit').then(({ proceed, unanswered }) => { + if (!proceed) return false; + quitApproved = true; + if (unanswered) markQuitUnanswered(win); + return true; }); }