Skip to content

fix(shutdown): scope the window flush handshake per window and request - #1150

Merged
h4yfans merged 1 commit into
mainfrom
shutdown-flush-window-scoping
Aug 11, 2026
Merged

fix(shutdown): scope the window flush handshake per window and request#1150
h4yfans merged 1 commit into
mainfrom
shutdown-flush-window-scoping

Conversation

@h4yfans

@h4yfans h4yfans commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Closes #1021

Problem

flushWindow() in apps/desktop/src/main/index.ts is the last line of defence against losing unsaved edits at window close and at quit. Two defects on that path:

  1. Listener leak. The 2 s timeout branch called resolve() without ipcMain.removeListener. A busy or hung renderer that never answered left its listener on the shared ipcMain permanently. ipcMain.on('window-close') calls flushWindow on every window close, so the listener count grew for the life of the process.
  2. Any window could satisfy any flush. app:flush-done had no request id and no event.sender check. flushAllWindows() registers one listener per window on that same channel, so the first window to reply resolved all of them — shutdown then continued while slower renderers still had saves in flight. A late reply to an earlier request (close flush timed out, renderer answered afterwards) could likewise satisfy the next request for the same window.

Root cause

One shared, unscoped IPC channel used for N concurrent request/response pairs, plus a cleanup path that only ran on the success branch. apps/desktop/src/main/lib/window-rpc.ts already solves exactly this with a per-request id and a sender check; flushWindow predated it and never adopted the pattern.

Fix

  • app:request-flush now carries a randomUUID() request id; the renderer echoes it back on app:flush-done (preload/api/core.ts, renderer/src/hooks/use-flush-on-quit.ts).
  • The main-side handler accepts a reply only when event.sender === win.webContents and the echoed id matches this request.
  • Reply and timeout both go through a single idempotent settle() that always clearTimeouts and ipcMain.removeListeners.

Nothing was dropped or shortened. The failure mode is unchanged and still conservative: an unrecognised or absent reply simply falls through to the existing 2 s timeout, which resolves and lets shutdown continue exactly as it does today. No new way to lose data was introduced; two existing ways were removed.

Test evidence

New tests in apps/desktop/src/main/index.phase2.test.ts (four), written before the fix.

RED, on unmodified origin/main code:

 FAIL  src/main/index.phase2.test.ts > removes the flush-done listener when a window flush times out
 FAIL  src/main/index.phase2.test.ts > waits for every window to answer its own flush before shutdown continues
 FAIL  src/main/index.phase2.test.ts > ignores a stale flush-done from an earlier request on the same window
 Tests  3 failed | 53 passed (56)

GREEN, after the fix:

 Tests  57 passed (57)

Full main-process suite:

 Test Files  475 passed | 1 skipped (476)
      Tests  5420 passed | 1 expected fail | 4 skipped (5425)

Mutation verification

Each fix line reverted individually; every mutant died.

Mutant Test that caught it Result
timeout branch settle() -> resolve() (drops the listener cleanup) removes the flush-done listener when a window flush times out Tests 1 failed | 56 passed (57)
drop if (event.sender !== win.webContents) return ignores a flush-done from another window even when it carries the pending request id Tests 1 failed | 56 passed (57)
drop if (doneRequestId !== requestId) return ignores a stale flush-done from an earlier request on the same window Tests 1 failed | 56 passed (57)

Note on the second row: on the first pass the sender mutant survived, because the multi-window test was already killed by the request-id check alone (a randomUUID no other window can produce). Rather than accept a near-equivalent mutant, a fourth test was added that pins the sender check on its own terms — a window replying with another window's live request id must be ignored. That test kills it. The sender check is defence in depth on top of the id; both are kept because both are cheap and the issue asked for both.

Verification

pnpm typecheck        -> 16 successful, 16 total
pnpm lint             -> 14 problems (0 errors, 14 warnings); zero in the touched files, all pre-existing
pnpm ipc:generate && pnpm ipc:check
                      -> Generated RPC bindings are up to date / IPC invoke map is up to date
                         (no regenerated output; the flush channels are hand-wired, not in the generated map)
pnpm --filter @memry/desktop test:main                        -> 5420 passed
pnpm exec vitest run --project renderer .../missing-hooks.test.tsx -> PASS (13) FAIL (0)
pnpm exec vitest run --project preload  .../preload-api.test.ts    -> PASS (8)  FAIL (0)
git diff --check      -> clean

Risk and backward compatibility

Low. No database, sync-protocol, vault-format or settings change — nothing persisted or sent over the wire is touched, so nothing here can be read by, or has to tolerate, another app version. app:request-flush / app:flush-done are purely intra-process channels between the main process and its own renderer; main, preload and renderer ship in the same asar, so there is no version skew across the new argument. The main-side handler still tolerates a missing request id (it just falls through to the pre-existing 2 s timeout), so a stale renderer surviving a hot reload degrades to today's behavior rather than hanging.

The user-visible effect is only that a multi-window quit now waits for each window's own flush instead of the fastest one — strictly more saved work, at the cost of up to the same 2 s per-window bound that already existed.

No cache or latch was added.

Scope

flushWindow/flushAllWindows only. broadcastToAllWindows and the window fan-out helpers touched by PRs #1119 and #937 are deliberately untouched; this change sits beside them in index.ts but shares no code with them, so it should rebase cleanly either way.

Docs gate: pnpm docs:impact --strict reports missing-docs for apps/desktop/src/main/index.ts, preload/api/core.ts, preload/index.d.ts and use-flush-on-quit.ts. Pushed with MEMRY_DOCS_IMPACT_SKIP=1: this is an internal IPC-handshake correctness fix on the shutdown path with no user-facing surface, setting, or workflow change — there is no page under apps/docs/src/** that describes this behavior, and adding one would document an implementation detail.

`flushWindow` shared one `app:flush-done` channel across every in-flight
flush with no sender and no request id, and its 2s timeout branch resolved
without removing the listener.

- Leak: a renderer that never answers left its listener on the shared
  `ipcMain` permanently. `window-close` runs this on every close, so the
  listener count grew for the life of the process.
- Correctness: `flushAllWindows()` registers one listener per window on the
  same channel, so the first window to reply resolved all of them and quit
  proceeded while slower renderers still had unsaved edits pending. A late
  reply to an earlier request could likewise satisfy the next request for the
  same window.

`app:request-flush` now carries a `randomUUID` request id that the renderer
echoes back on `app:flush-done`; the main-side handler accepts a reply only
when it comes from that window's `webContents` and matches that request id,
and both the reply and the timeout path go through one `settle()` that always
removes the listener.

Closes #1021
@github-actions github-actions Bot added bug Something isn't working test labels Aug 7, 2026
@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown

React Doctor found no new issues. 🎉

Reviewed by React Doctor for commit 1389daa.

@codecov

codecov Bot commented Aug 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[HIGH][shutdown] flushWindow leaks an ipcMain listener on timeout and lets any window satisfy any flush

1 participant