fix(desktop): drop Command Code login attempts nobody will collect by Totoro-qaq · Pull Request #5421 · apache/maka

Totoro-qaq · 2026-09-17T02:41:29Z

Summary

A Command Code browser login start puts its attempt into the controller's #attempts map before binding. The port_unavailable, bind-time superseded and browser_unavailable failures return no id and only call #finish(), which never removes the entry; only complete() deleted. Every failed start therefore stayed in the map for the life of the process. The renderer's stale-start path, which cancel()s an ok start instead of completing it, left an entry behind as well.

The three failure paths and cancel() now finish the attempt and remove its entry through a small #abandon() helper. A complete() that is already waiting holds its attempt and still reads the result.
dispose() clears the map.
Starts are owned by the WebContents that invoked them, following bindCopyOwner. When that document goes away, its attempts are abandoned and their ports released. That happens on:
- render-process-gone;
- destroyed;
- a new main-frame document commit (did-frame-navigate, for example a reload);
- a reload that commits an error page (did-fail-provisional-load).
A navigation that is only started, is blocked, or stays within the same document keeps the attempt.
attemptCount() exposes the map size so tests can assert the leak invariant, following BrowserViewManager.liveCount().

Behavior changes:

complete(id) after cancel(id) now answers superseded (unknown id) instead of cancelled. The renderer sends complete() in the same tick as a successful start, before any cancel(), so no current caller sees it.
A login in progress is cancelled when its renderer crashes, is destroyed, or reloads (for example from the error boundary). A complete() already waiting settles as cancelled.

Not changed: within one document, an attempt replaced by a newer start stays in the map until it is collected, the document goes away, or the controller is disposed, because a waiting complete() may still need it.

Fixes #5404

Verification

Failed starts and cancel: leak assertions were added to the existing tests, plus two new tests: a cancel-only stale start, and dispose with an uncollected superseded attempt. All failed before the fix.
Renderer ownership: production-shaped IPC tests drive the real controller. The event sequences come from a real Electron 43.4.1 probe. The tests cover:
- crash, destroy, reload and error-page reload while start() opens the browser (the attempt and its port are gone, and a start from the new document holds one attempt);
- a start still binding;
- a waiting complete();
- another renderer's attempt;
- same-document, subframe, subframe error-page and blocked navigations;
- listener counts staying bounded across reloads.
Mutation checks: every guard was mutated (the abandon paths, the dispose clear, the owner filter and forget, the main-frame and error-page filters, listener removal), and each mutant fails a test.
Command Code suites: commandcode-browser-login 23/23, commandcode-browser-login-flow 9/9, commandcode-login-ipc-main 20/20.
@maka/desktop test:dist: 2,701 passed, 0 failed, and 8 cancelled, all in the unrelated mcp-oauth-controller deadline suite.
Other checks: desktop typecheck, biome lint, git diff --check and the Windows test inventory pass. biome format does not cover apps/desktop.

AI use

No generative tool made a substantive contribution
Generative tooling made a substantive contribution

Tool(s) and scope: Claude Code helped investigate, implement and test this change. The commits carry a Generated-by: Claude Code trailer.

Checklist

Tests cover the change and fail without it
Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

Yes — described under Summary above
No

Generated-by: Claude Code

A Command Code browser login start put its attempt into the controller's map before binding, but the port_unavailable, superseded and browser_unavailable failures returned no id and only finished the attempt, so every failed start stayed in the map for the life of the process. The renderer's stale-start path, which cancels an ok start instead of completing it, left one behind as well. Remove the entry when a start fails or an attempt is cancelled, and clear the map on dispose. A complete() that is already waiting holds its attempt and still reads the result. Generated-by: Claude Code

hqhq1025

Reviewed the current head. The new abandon path correctly removes failed starts and explicit cancellations, but one renderer-ownership leak remains.

Local verification passed: clean Node 24.18.1 install, build:test, full workspace typecheck, the three Command Code suites (35/35), Desktop test:dist (2572/2572), lint, format check, ASF headers, changed-file Biome, and git diff --check. Hosted test and label are green, and the head is directly based on current main.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

hqhq1025 · 2026-09-17T03:34:56Z

    if (this.#disposed) return;
    this.#disposed = true;
    this.#finish(this.#current, { ok: false, reason: 'cancelled' });
+    // Every other attempt settled when it was retired, and a waiting


[P3] Release successful starts when their renderer owner disappears

Clearing the map only during whole-app disposal still leaves an attempt orphaned across the app’s supported renderer crash/reload path. registerCommandCodeLoginIpc() ignores event.sender, while this controller is process-scoped and survives reloadMainRenderer(). If the renderer exits while start() is awaiting shell.openExternal(), the handler can later return an attemptId to a destroyed frame; no caller can issue complete() or cancel(), and the timeout uses #finish() without deleting the entry. A production-shaped IPC probe that emitted render-process-gone during that await retained 1 entry after timeout; starting again from the recovered renderer raised the count to 2. Please bind attempts to the invoking WebContents and abandon them on render-process-gone/destroyed (as other long-lived Desktop IPC owners do), while preserving an already-running complete().

Thanks, confirmed. A renderer that crashed or was destroyed while start() awaited openExternal left its attempt behind, and a start from the recovered renderer added a second one.

Fixed in 1e64e41, following bindCopyOwner: the IPC layer binds each start to web-contents:<id>, and on the first of render-process-gone or destroyed calls the controller's new abandonOwner(), which releases that renderer's attempts and ports through #abandon. A start still binding gives up its port without opening a browser, a complete() already waiting settles as cancelled as it does after cancel(), and another renderer's attempt is left alone.

New tests drive the real controller through the IPC handlers for both events: after the renderer goes, the attempt and its port are gone, and a new start from the same WebContents holds one attempt. They also cover a start that is still binding and a waiting complete(), and they failed before the fix.

One case this does not cover, same as the existing owners: a plain document reload (for example the error boundary's Reload button) emits neither event, so an attempt started in that narrow window keeps its port until the login window times out and its settled entry until dispose.

Claude Code-assisted.

… goes away The login controller outlives every renderer, and the login IPC ignored the sender. A renderer that crashed or was destroyed while start() was opening the browser never sent the complete() or cancel() that releases its attempt, so the entry stayed in the map, and a start from the recovered renderer added another. Bind each start to the invoking WebContents, as the session copy owners do, and abandon that owner's attempts on render-process-gone or destroyed. A complete() already waiting settles as cancelled, as it does after cancel(). Generated-by: Claude Code

hqhq1025

Reviewed the current head. The renderer-owner cleanup now covers renderer crashes and destroyed WebContents, and the original failed-start/cancel leak remains fixed. One non-blocking document-reload edge remains below.

Local verification passed under Node 24.18.1: clean install, build:test, full workspace typecheck, the three Command Code suites (42/42), Desktop test:dist (2584/2584), lint, format check, ASF headers, changed-file Biome, and git diff --check. Hosted test is green, and the head is directly based on current main.

I did not connect a real Command Code Studio account or run a packaged macOS/Windows build.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

hqhq1025 · 2026-09-17T04:56:25Z

+        deps.controller.abandonOwner(ownerId);
+      };
+      event.sender.once('render-process-gone', abandon);
+      event.sender.once('destroyed', abandon);


[P3] Also retire an owner when its renderer document reloads

These two events cover crashes and WebContents destruction, but not a normal main-frame reload. The production error boundary calls window.location.reload(), which keeps the same WebContents alive. In a real Electron probe, reloading that BrowserWindow while start() was waiting in openExternal() emitted neither render-process-gone nor destroyed; the reply returned to the discarded document, attemptCount() remained 1, and a start from the new document raised it to 2. Because the owner ID and observedOwners entry are both keyed only by WebContents ID, the replacement document is not a new owner either. Please cover main-frame document replacement (or explicitly cancel before reload) and add a regression for this path.

Thanks, confirmed with a real Electron 43.4.1 probe: location.reload() and webContents.reload() keep the WebContents and fire neither lifecycle event, so the old document's attempt stayed and a start from the new document added another.

Fixed in 57408be. Besides render-process-gone and destroyed, bindOwner now retires the owner when a new main-frame document commits. It listens to did-frame-navigate with isMainFrame, as subscribeMainFrameCommitted in main-window.ts does.

I used the commit rather than did-start-navigation: in the probe that event also fired for navigations will-navigate blocks and for 204 responses, where the document survives.

A reload that commits an error page reports did-fail-provisional-load instead, so that main-frame event retires the owner too. In the probe it fired only when an error page replaced the document. It did not fire for blocked, stopped, 204 or download navigations.

Same-document routes and subframe navigations keep the attempt.

The tests replay the observed Electron 43 event sequences:

a reload while start() opens the browser: the attempt and its port are gone, and a start from the new document holds one attempt

an error-page reload

same-document, subframe, subframe error-page and blocked navigations

a waiting complete()

listener counts across repeated reloads

Claude Code-assisted.

… reloads A reload keeps the WebContents, so neither render-process-gone nor destroyed fires. A login the old document started stayed in the map with its port, and a start from the new document added another. Also retire the owner when a new main-frame document commits (did-frame-navigate) or a reload commits an error page (did-fail-provisional-load). A navigation that is only started, blocked, or kept within the same document leaves the attempt alone. Generated-by: Claude Code

hqhq1025

Reviewed exact head 57408beaf84a4209edf4e73f974e5352b6d97a25. I found no P0-P3 issues.

The latest increment closes the remaining document-reload lifecycle gap. commandcode-login-ipc-main.ts now retires the WebContents owner when a new main-frame document commits or when a failed main-frame load commits an error page, while leaving same-document, subframe, and blocked navigations alone. The existing controller cleanup still releases the loopback listener, timer, and attempt entry for renderer crashes, destroyed WebContents, failed starts, cancellations, concurrent losers, and application disposal.

I independently exercised Electron 43.4.1 with a real hidden BrowserWindow and the production IPC/controller path. On this head, a successful reload reduced attemptCount() to 0, the replacement document reused the same loopback port, and a missing-file error page again reduced the count to 0. Running the same probe against the prior head left counts of 1 after reload and 2 after the replacement start/error page.

Local verification under Node 24.18.1 passed: clean install, build:test, full workspace typecheck, 43 focused Command Code tests, Desktop test:dist (2585/2585), lint, format check, ASF headers, changed-file Biome, and git diff --check. Hosted test is green, and the head is directly based on current main with a clean merge tree.

I did not connect a real Command Code Studio account or run packaged macOS/Windows builds.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

Astro-Han

Thanks — the leak fix reads correctly and I confirmed the invariants locally. Comment only, nothing blocking.

What I verified

Built the head (57408be) in a scratch checkout and ran the three Command Code suites: commandcode-browser-login 23/23, commandcode-login-ipc-main 20/20, commandcode-browser-login-flow 9/9 (52/52). No flakes across two runs.
biome lint is clean on all four changed files (the formatter is excluded for apps/desktop, per biome.jsonc).
Mutation-checked three of the new guards: reverting cancel() to #finish and dropping the dispose() clear each fail a test (commandcode-browser-login.test.ts:324, :446); inverting the isMainFrame check in onErrorPageCommitted fails 4 tests. The assertions are load-bearing, not vacuous.
Walked every #attempts insertion/deletion pair. #reserve() is the only writer; the readers are now #abandon (the three failed-start paths at commandcode-browser-login.ts:184,191,209, cancel() at :233, abandonOwner() at :243), complete(), and the dispose() clear at :255. Nothing is left holding an entry it will never look up again, and the documented "not changed" case (a superseded loser held for a complete() that may still be waiting) is still bounded — the flow never issues two concurrent starts, and the loser's complete() is always in flight.
The complete()-after-cancel() → superseded change is safe: commandcode-browser-login-flow.ts only calls cancel() on the stale-start path (started.ok branch) and from cancel(), both of which bump the generation and drop the late result, and #attemptId is cleared so dispose() cannot double-cancel. No other caller does complete-then-cancel, and the mirrored reason unions in ports.ts:88 and bridge-contract.d.ts:423 carry no per-reason docs that went stale.
Nothing else constructs CommandCodeLoginIpcDeps besides runtime-host-boot.ts:1949 and the test harness, so widening the Pick<> breaks no other mock. registerCommandCodeLoginIpc/dispose() are called once each (runtime-host-boot.ts:1949, :2189), so the module-level observedOwners set really is app-scoped, as the comment claims.

One thing I could not confirm (non-blocking)

onErrorPageCommitted (commandcode-login-ipc-main.ts:88-95) keys only on isMainFrame and ignores errorCode. A main-frame did-fail-provisional-load with net::ERR_ABORTED (-3) therefore abandons the login exactly like an error page — I probed this against the real compiled controller + IPC path: attempt dropped, loopback port released, owner detached, pending complete() settles as cancelled.

Electron/Chromium also reports -3 for main-frame loads that are redirected, window.stop()-ed, turned into a download, or cancelled by a will-navigate preventDefault() — i.e. cases where the document is not replaced. The comment at commandcode-login-ipc-main.ts:86-87 ("a load that is stopped, blocked, or turned into a download keeps the document") and the navigate() helper's comment at commandcode-login-ipc-main.test.ts:60-64 assume those emit nothing after did-start-navigation. Your Electron 43.4.1 probe may well have confirmed that; if it did not, a login in flight would be cancelled by any such navigation. Cheap way to pin it either way: keep only non-aborted codes (errorCode !== -3) and add a stopped/blocked case to navigate() asserting attemptCount() stays 1.

Nits

bindOwner() runs before the controller accepts the start (commandcode-login-ipc-main.ts:108), so a start refused because the controller is disposed still records the owner and attaches four listeners to the WebContents. Harmless while the app is quitting, but ordering it after the start() result would keep the two in step.
The two suites coordinate loopback ranges by comment only — commandcode-browser-login.test.ts:49 (46_959, 10 attempts → through 46_968) and commandcode-login-ipc-main.test.ts:244 (46_979). Raising maxPortAttempts past 20 in the first file would silently collide with the second under node --test's parallel files. One exported constant for the test range would make that impossible.
attemptCount() matches the BrowserViewManager.liveCount() convention nicely, so no objection to it being production API for a test invariant.

… load The error-page handler abandoned the owner for any main-frame did-fail-provisional-load. Electron documents that event for a load window.stop() cancels, which Chromium fails with net::ERR_ABORTED and which commits no error page, so ignore that code. Also share the loopback port ranges of the two Command Code test suites. Generated-by: Claude Code

Totoro-qaq · 2026-09-17T11:47:48Z

Thanks. Changes in d80bffe:

errorCode: the error-page handler now abandons only on isMainFrame && errorCode !== NET_ERR_ABORTED (-3).
- On the open question: a fresh Electron 43.4.1 probe saw no did-fail-provisional-load for window.stop(), webContents.stop(), a will-navigate or will-redirect preventDefault, a download or a 204. It fired only for committed error pages (-6, -20, -102).
- The guard is still worth having, because Electron documents the event for a load window.stop() cancels.
- Tests add a cancelled navigation, which fails without the guard, and a -102 refused-error-page departure, so an allowlist or range filter can't regress the dev-server-down reload.
Port ranges: a shared test-only commandcode-login-test-ports.ts now defines both suites' ranges, and the IPC range starts where the controller range ends.
bindOwner ordering: kept.
- The listeners have to be attached while start() is still awaiting the bind and openExternal. Binding after start() settles fails five wired tests: the departures while opening the browser, and a renderer gone while binding.
- Unbinding after a refused start isn't safe either, since a superseded result can leave a newer live attempt from the same owner.
- A start refused after dispose only happens during quit, and those listeners come off on destroyed.

github-actions Bot added the effort/S Under 100 readable lines label Sep 17, 2026

Totoro-qaq force-pushed the fix/commandcode-login-attempt-leak branch from 16d2cec to 1e65ddf Compare September 17, 2026 02:46

Totoro-qaq marked this pull request as ready for review September 17, 2026 02:48

hqhq1025 reviewed Sep 17, 2026

View reviewed changes

github-actions Bot added effort/M Under 500 readable lines and removed effort/S Under 100 readable lines labels Sep 17, 2026

hqhq1025 reviewed Sep 17, 2026

View reviewed changes

Astro-Han reviewed Sep 17, 2026

View reviewed changes

Navigation Menu

fix(desktop): drop Command Code login attempts nobody will collect - #5421

fix(desktop): drop Command Code login attempts nobody will collect#5421
Totoro-qaq wants to merge 4 commits into
apache:mainfrom
Totoro-qaq:fix/commandcode-login-attempt-leak

Totoro-qaq commented Sep 17, 2026 •

edited

Loading

Uh oh!

hqhq1025 left a comment

Uh oh!

hqhq1025 Sep 17, 2026

Uh oh!

Totoro-qaq Sep 17, 2026

Uh oh!

hqhq1025 left a comment

Uh oh!

hqhq1025 Sep 17, 2026

Uh oh!

Totoro-qaq Sep 17, 2026

Uh oh!

hqhq1025 left a comment

Uh oh!

Astro-Han left a comment

Uh oh!

Totoro-qaq commented Sep 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

Totoro-qaq commented Sep 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Verification

AI use

Checklist

Uh oh!

hqhq1025 left a comment

Choose a reason for hiding this comment

Uh oh!

hqhq1025 Sep 17, 2026

Choose a reason for hiding this comment

Uh oh!

Totoro-qaq Sep 17, 2026

Choose a reason for hiding this comment

Uh oh!

hqhq1025 left a comment

Choose a reason for hiding this comment

Uh oh!

hqhq1025 Sep 17, 2026

Choose a reason for hiding this comment

Uh oh!

Totoro-qaq Sep 17, 2026

Choose a reason for hiding this comment

Uh oh!

hqhq1025 left a comment

Choose a reason for hiding this comment

Uh oh!

Astro-Han left a comment

Choose a reason for hiding this comment

What I verified

One thing I could not confirm (non-blocking)

Nits

Uh oh!

Totoro-qaq commented Sep 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Totoro-qaq commented Sep 17, 2026 •

edited

Loading