perf(ipc): cap the flush handshake at one ipcMain listener and free window-rpc on teardown - #1270
Merged
Merged
Conversation
|
React Doctor found no new issues. 🎉 Reviewed by React Doctor for commit |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1098. Part of epic #987 (memory/CPU audit 2026-08).
Prior work (#1021 / PR #1150)
#1021 already shipped:
flushWindowremoves its listener on the timeout path and scopes each reply with a request id and anevent.sendercheck. This PR does not re-fix any of that — it only adds what #1098 asks for on top: the listener count, and the window-rpc cleanup path.What was wrong
1.
apps/desktop/src/main/index.ts:1919-1964— oneipcMainlistener per window on a shared channel.app:flush-doneis a single process-wide channel, but everyflushWindow()call registered its own listener on it:flushAllWindows()runsflushWindowfor every open window concurrently, so an 11-window quit put 11 listeners onapp:flush-doneand Node emittedMaxListenersExceededWarningonipcMain. The request-id scoping from #1021 made those N listeners redundant: the id is what separates the replies, not the listener identity.2.
apps/desktop/src/main/lib/window-rpc.ts:38-56— the per-call listener only ever came off on reply or timeout.mainToRendererInvokeregisters a listener onmain:invoke:response:<uuid>and drops any reply whose sender is not the target window. When the target window goes away mid-call (closed, or reloaded out from under the request) no reply is ever coming, so the listener — and the caller's promise — sat onipcMainfor the full 2 s timeout. The handler also re-readwin.webContentsat reply time, which throws on a destroyedBrowserWindow.What changed
pendingFlushesmap keyed by request id plus a single sharedhandleFlushDonelistener. It attaches when the first flush starts and detaches when the last one settles (reply or timeout), so listener count onapp:flush-doneis 0 or 1 regardless of window count. The sender + request-id checks from [HIGH][shutdown] flushWindow leaks an ipcMain listener on timeout and lets any window satisfy any flush #1021 are preserved verbatim, just moved into the shared handler.win.webContentsonce astarget(no more re-read that can throw), and settlenullontarget.once('destroyed')so the listener is released at teardown instead of at timeout.cleanup()removes both listeners.Deliberately not done: the issue's item (1) says to "settle-or-cleanup on mismatch". Settling on a foreign sender's reply would hand any renderer a way to null out another window's in-flight RPC, so a mismatched reply is still dropped. The reachable cause of the lingering listener is the target going away, and that is what now triggers cleanup.
How it was proven
Test files:
apps/desktop/src/main/index.phase2.test.ts,apps/desktop/src/main/lib/window-rpc.test.ts.New/changed coverage:
keeps one flush-done listener when more than ten windows flush at once— 13 windows, asserts exactly 1 registration onapp:flush-done, 13 distinct request ids still sent, and the listener detached once all settle.waits for every window to answer its own flush before shutdown continues— assertion changed fromtoHaveLength(browserWindows.length)totoHaveLength(1).drops the per-call listener as soon as the target window is destroyed— asserts the response listener is gone and no timer is left pending.Before/after with the two source files reverted to
origin/main(test files kept):Verification
pnpm --filter @memry/desktop typecheck:node— clean.vitest run --project main src/main/lib src/main/index.phase2.test.ts src/main/index.phase1.test.ts src/main/index.phase3.test.ts src/main/agent/mcp/tools/__tests__—Test Files 30 passed (30) | Tests 334 passed (334).pnpm exec eslint <changed files>— 0 errors (2 "file ignored" warnings for the two test files, pre-existing config behaviour).git diff --check— clean.pnpm docs:impact --base origin/main --strict—covered.pnpm docs:build—build complete in 4.54s.Backward compatibility
Main-process-internal only: the
app:request-flush/app:flush-donewire shape and themain:invokepayload are unchanged, so no preload, renderer, contract, schema, or settings change is involved.