(window): close a window whose page stopped answering

[abate]

Split out of #478, as asked in its review: the bug fix on its own, with the regression tests pushed first so CI shows them failing.

Base. Rebased on main 8b12045, which carries #479; the guard keeps #479's logoff handling (LOGOFF_CANCEL_MS, exit-flush) and its reason upgrade.

The bug. A window close, a quit or an update install asks the page about unsaved edits (#373). When the page never acknowledges within 2.5 s, main approves and calls win.close() (or app.quit(), which closes the window). A hung page never runs its beforeunload, so that close never finishes: the window stays and the app keeps running, SIGTERM included.

The fix. After an unanswered check approves the close, the quit or the install, the window is still closed the usual way. The guard destroys it only:

• when Electron reports the page unresponsive (or had reported it and nothing since showed it responsive: no responsive event, no acknowledged check), or
• when the window is closed again, the guard's own close() or app.quit() aside.

For a quit or an install the force waits for the quit to close the window (its close event), so a quit still held by another before-quit handler (the ActivityWatch flush) is not cut short.

What it costs a busy page. Electron reports a page unresponsive about 5 s after a close it has not completed, and cannot tell a busy page from a hung one. So a page still busy about 5 s after the close — about 7.5 s after the click, counting the 2.5 s bound — is destroyed, and its debounced working-set save is lost. Only a page that recovers before then closes normally; an 8–10 s stall (#526) clicked early is destroyed. A check the page acknowledged never destroys the window.

Commits.

1. The tests alone, failing on main: four unit tests in test/unsaved-guard.test.js, and the journey e2e/hung-window.spec.js (hangs the page with a busy loop, closes the window from main, expects exit code 0). The e2e fixture now lets a journey end the app itself.
2. The fix, its changelog entry, and .ai/contexts/viewer-panel.md ("Bounded").
3. Docs: the timing the journey measures.
4. Tests alone: a quit held by another before-quit handler does not destroy the window first; the updater-install test now expects the force at the window's close.
5. The fix for 4, and the busy-page consequence in the changelog and the context.

CI. Red on the tests alone: run 38069565122 on the first commit (the four unit tests on all four test jobs, the journey timing out), and run 38140257552 on 9536fdc (exactly the two tests of commit 4 on all four test jobs). Green: run 38070113232 on the first fix; the head's run is linked in the latest comment.

Unit tests plus one journey: the journey covers the real Electron close; the timing cases (a page reported unresponsive before a quit, a quit held by ActivityWatch) need a controlled event order, which only the unit tests give.

🤖 Generated with Claude Code

[abate]

CI evidence:

• f1e68a9, the tests alone: run 38039912120 fails on every test job, with exactly the three new tests failing (# fail 3): a close and a quit the page never acknowledges, and the close that finishes normally (its grace timer does not exist on main). Lint, changelog and e2e are green.
• 1fb50e9, the fix: run 38040440728 is green on every job.

Mutations, each run locally on the fix and each turning a test red: no bound after an unanswered quit; no bound after an unanswered close; destroying without checking the window is still there; the unanswered flag dropped from the 2.5 s timer.

[devsuitup]

Review at 1fb50e9 — request changes (1 major, 3 minor).

Verified

• Red-first holds: on the tests-only commit f1e68a9 (run 38039912120) all four test jobs fail with exactly the three new tests (assert.ok(grace)); every check is green on 1fb50e9.
• Locally (Node 22): head 19/19; the head's test file against main's unsaved-guard.js: 16 pass, the same 3 fail.
• A check the page acknowledged never arms the destroy timer, so an unsaved-edits confirm the user is answering stays open. A crashed page or a failed send never arms it either.
• After destroy(), closed still kills PTYs, watchers and bg agents (main.js:429-445); bounds are saved on the preceding win.close().
• Changelog under Unreleased, no code comments added.

Major — a busy page is destroyed, not only a hung one (unsaved-guard.js:65-69, :106)
The bound is a fixed delay: any page whose check goes unanswered and that is still blocked 3 s after win.close() is destroyed, about 5.5 s after the click. Renderer stalls of 8–10 s happen today on tab reveal (#526), and on Windows/Linux the system-drawn close button stays clickable during them. Destroy drops the page's queued work: the 500 ms debounced persistWorkingSet (public/app.js:213-219), a save in flight, and once #479 lands its exit flush. No extra unsaved edits are lost (main already overrides the late veto). The changelog, the commit body and .ai/contexts/viewer-panel.md:168 describe it as "a page that stopped answering", which a busy page is not. Either base it on Electron's unresponsive event (and cancel on responsive), or keep the delay and say plainly that a page still blocked 3 s after the close is destroyed, with the working-set consequence.

Minor

1. On quit the 3 s grace starts before quit() (unsaved-guard.js:80); the second before-quit (ActivityWatch flush, main.js:3509-3515) can hold the quit up to 1.5 s, so the window gets ~1.5 s. Start the timer from the window's close.
2. Conflicts with #479 (unsaved-guard.js ask/inflight and exports, its test file, CHANGELOG, viewer-panel.md). Combined, a page busy >5.5 s never answers, so #479's flush never runs. Please say which lands first; the second needs a fresh review.
3. No journey covers the real path (Electron close() never finishing on a hung page, then closed → window-all-closed → will-quit and exit). A Playwright journey looks feasible (hang with page.evaluate(() => setTimeout(() => { for(;;){} }, 0)), close from main, expect exit in ~10 s), not tried; otherwise state why it cannot run.

Nit: the boundClose timer handle is never kept or cleared (harmless thanks to the isDestroyed() guard).

[abate]

Addressed at fba2056 (branch redone from main: tests first, then the fix).

Major (busy page destroyed). The fixed 3 s delay is gone. After an unanswered check approves a close, a quit or an install, the window is closed the usual way, and the guard destroys it only:

• when Electron reports it unresponsive, or had reported it and nothing since showed it responsive (responsive, or the page acknowledging a check);
• when it is closed again, the guard's own close() and app.quit() aside.

A page that recovers first closes normally. A page still blocked when Electron reports it is destroyed, and the context says so, along with the working-set consequence. The journey measures that point: the app exits about 6 s after the close, 2.5 s of which are the guard's bound. An acknowledged check never destroys the window.

Minor 1 (timer before quit()). No timer is left. The destroy keys on events of the window itself, so the second before-quit (ActivityWatch flush) no longer eats into a grace period.

Minor 2 (merge order with #479). #479 first; I will rebase this on it. Since a page that answers is never destroyed, #479's flush runs whenever the page can run it.

Minor 3 (journey). e2e/hung-window.spec.js hangs the page with a busy loop, closes the window from main, and expects exit with exitCode === 0 and no signal. The fixture now lets a journey end the app itself (moved here from #478).

• Red on main: run 38069565122 on 46c342e. The poll timed out, and the four new unit tests failed on all four test jobs.
• Green: run 38070113232 on c0131fb, where the journey passed in 10.4 s.
• Two earlier red attempts failed for the wrong reason. Playwright's evaluate cannot return from a page that hangs, so the close is now scheduled in main before the page hangs.

Nit. No boundClose timer remains.

Also added: an update install the page never acknowledges is marked the same way (confirmQuit). The quitAndInstall() close would otherwise hang too.

[devsuitup]

Re-review at fba2056 — request changes (1 major on the stated behaviour, 3 minor). The design is much better: no fixed delay destroys the window any more.

Previous point	Status
Major: a busy page is destroyed after a fixed 3 s	partly closed — destroy only on unresponsive (unsaved-guard.js:107-111) or a second close (:114-118); responsive (:111) or an ack (:21) clears it. Electron's own timer still applies (below)
Grace overlapping the ActivityWatch flush	closed — Electron closes the window only at the final unprevented before-quit (main.js:3508-3515); one edge left (minor 2)
Conflicts with #479	order declared (#479 first, then rebase); conflicts remain against b41237a (unsaved-guard.js 4 hunks, its test file, CHANGELOG, viewer-panel.md) — the rebased head will need a fresh look
No e2e of the hung path	closed — e2e/hung-window.spec.js hangs the page, closes from main, asserts exit code 0; red at 46c342e (60 s timeout), green at c0131fb (10.4 s, run 38070113232)
boundClose timer	gone

Red-first in CI: run 38069565122 on 46c342e fails all four test jobs on exactly the new tests (assertions). Two tests that came with c0131fb ("an updater install the page never acknowledges…", "a page that acknowledges a check is no longer counted unresponsive") were never shown red.

Major — the PR says a #526 stall survives; the code says otherwise past ~7.5 s
Electron emits unresponsive ~5 s after a win.close() the page has not completed (OnCloseButtonClicked → ScheduleUnresponsiveEvent(5000); from Electron's source, not measured here), and it cannot tell a busy loop from a stall. The journey timing agrees: 10.4 s ≈ launch + 2 s + 2.5 s bound + ~5 s. So a page still busy ~7.5 s after the click is destroyed and its debounced persistWorkingSet lost — an 8–10 s stall clicked early is. viewer-panel.md:168 says so, but the PR body ("a page busy for 8–10 s (#526) that recovers is not destroyed"), the c0131fb body ("no fixed delay destroys it") and the changelog ("busy for a moment") say the opposite. Please state the ~5 s-after-close / ~7.5 s-after-click consequence in all four.

Minor

1. The doc's "exits about 6 s after the close" does not match the journey (~7.4 s); plausible, CI logs only per-test totals.
2. unsaved-guard.js:89-91 (plausible): when the page was already flagged unresponsive before a quit, forceIfUnresponsive runs right after quit(); if ActivityWatch prevented that quit, the window is destroyed during the 1.5 s flush and window-all-closed → app.quit() runs cleanup with the flush in flight. No test.
3. The updater-install regression test was never shown red (came with the fix).

Checked: no listener leak per close (attach registers unresponsive/responsive once; askFull removes its listeners in finish). Locally (Node 22) 23/23; the head's tests against main's guard fail 5. Not run: Electron, the e2e locally, any measurement of unresponsive timing on Windows.

[devsuitup]

Re-review at 8aa15c9: approve.

Previous findings, all closed:

Finding	How it is closed
#526 stall claim	The commit and PR bodies, the CHANGELOG and viewer-panel.md "Bounded" now state the real bound: about 5 s after the close (≈7.5 s after the click) a busy page is destroyed, its working-set save is lost, and an 8–10 s stall is not spared
"about 6 s"	Now 7.5 s, consistent with the head's e2e (hung-window 10.7 s)
Destroyed during the ActivityWatch flush	A quit or install now only marks quitUnanswered (unsaved-guard.js:86-89,105,189). The force is armed at the window's own close (:140-147). Covered at test/unsaved-guard.test.js:413
Updater-install test never shown red	Done: run 38140257552 on 9536fdc failed exactly the two new tests (real assertions) on all four test jobs

The head is green (runs 38140787466, 38140787515). It merges cleanly with #479's reviewed head b41237a; only viewer-panel.md and the CHANGELOG need trivial resolution, and unsaved-guard tests pass 28/28 merged.

Not blocking:

1. Plausible, from Electron's source, not measured: on the quit path the guard calls preventDefault on the quit's own close and then destroy(). A cancelled close resets Electron's quitting state (WindowCloseCancelled). window-all-closed then calls app.quit() again, so before-quit cleanup (main.js:3512-3544: MCP shutdown, indexer dispose, killPty) runs twice. The quit still completes. You could skip preventDefault on that path and force on setImmediate, or prove the cleanup runs once with a journey.
2. The quit/install path is unit-tested only. A journey that hangs the page, calls app.quit() from main and expects exit 0 needs no controlled event order.
3. Nit: "about 5 seconds after the close" reads as after the click.
4. Nit: the PR body's mention of a run linked in the latest comment points to fba2056. Keep the "Generated with …" line out of the squash message.

[abate]

Addressed at 8aa15c9, rebased on main 8b12045 (which carries #479). The #479 conflicts are resolved keeping main's behaviour: LOGOFF_CANCEL_MS and the exit-flush on query-session-end/session-end, the 'reload' → quit/close reason upgrade (unsaved-check-reason), and approvedToQuit() wherever the guard checked quitApproved. The rebased tests-only commit 43529c4 still fails 4 tests against main's guard (replayed locally).

Major (the stated behaviour). The consequence is now stated the same way in the PR body, the fix commit's message (e8fe936), the changelog and viewer-panel.md ("Bounded"): Electron reports a page unresponsive about 5 s after a close it has not completed and cannot tell busy from hung, so a page still busy then — about 7.5 s after the click, counting the 2.5 s bound — is destroyed and its debounced working-set save is lost. Only a page that recovers sooner closes normally. The "8–10 s stall survives" sentence is gone.

Minor 1 (timing figure). "About 6 s" is replaced by "about 7.5 s after the close it schedules", matching the journey.

Minor 2 (destroyed during the ActivityWatch flush). A quit or install the page never acknowledged no longer forces right after quit(). It marks the window (quitUnanswered), and the window's own close event — which comes only once the quit really proceeds to close windows — turns it into unanswered and destroys a page already reported unresponsive. An unresponsive report while the quit is still held does nothing.

• Tests first: 9536fdc, run 38140257552 red on exactly a quit held by another before-quit handler does not destroy the window before it closes it and the updater-install test (now expecting the force at the window's close), on all four test jobs.
• Fix: 8aa15c9, run 38140787466 green on every job, e2e included.
• Mutations: marking unanswered at once in beforeQuit or confirmQuit, dropping the force on the quit's close, dropping the promotion, or dropping the ack clearing unresponsive each fail a test.

Minor 3 (tests never shown red).

• an updater install the page never acknowledges…: replayed against main's guard it fails (main never destroys), and it was red in CI this round (run 38140257552).
• a page that acknowledges a check is no longer counted unresponsive passes on main by construction (main never destroys). Shown red by mutation instead: removing state.unresponsive = false from the ack handler fails it.

Locally (Node 22): full suite fails only the sandboxed-session baseline (13); lint 0 errors.