You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@maka.apache.org by "Totoro-qaq (via GitHub)" <gi...@apache.org> on 2026/09/17 02:41:32 UTC
[PR] fix(desktop): drop Command Code login attempts nobody will collect [maka]
Totoro-qaq opened a new pull request, #5421:
URL: https://github.com/apache/maka/pull/5421
## 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.
- `attemptCount()` exposes the map size so tests can assert the leak invariant, following `BrowserViewManager.liveCount()`.
One visible difference: `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.
Not changed: an attempt replaced by a newer start stays in the map until it is collected or the controller is disposed, because a waiting `complete()` may still need it.
Fixes #5404
## Verification
- Added leak assertions to the existing `port_unavailable`, `browser_unavailable` and simultaneous-start tests, plus two new tests: a cancel-only stale start, and dispose with an uncollected superseded attempt. All five failed before the fix.
- Six mutants each make a test fail: turning each `#abandon()` call back into `#finish()`, dropping the delete in `#abandon()`, and dropping `dispose()`'s clear.
- `commandcode-browser-login`, `commandcode-browser-login-flow` and `commandcode-login-ipc-main` tests: 35/35.
- `@maka/desktop` test:dist: 2684 passed, 0 failed. 8 were cancelled, all in the unrelated `mcp-oauth-controller` deadline suite.
- Desktop typecheck, biome lint, `git diff --check` and the Windows test inventory all pass. biome format does not cover `apps/desktop`.
## AI use
- [ ] No generative tool made a substantive contribution
- [x] Generative tooling made a substantive contribution
Tool(s) and scope: Claude Code helped investigate, implement and test this change.
## Checklist
- [x] Tests cover the change and fail without it
- [x] Lint, format, typecheck and the affected suites pass locally
Does this PR entail a change in behavior?
- [x] Yes — described under Summary above
- [ ] No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): drop Command Code login attempts nobody will collect [maka]
Posted by "Totoro-qaq (via GitHub)" <gi...@apache.org>.
Totoro-qaq commented on PR #5421:
URL: https://github.com/apache/maka/pull/5421#issuecomment-5713819289
Thanks. Changes in d80bffe0:
- **`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`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): drop Command Code login attempts nobody will collect [maka]
Posted by "hqhq1025 (via GitHub)" <gi...@apache.org>.
hqhq1025 commented on code in PR #5421:
URL: https://github.com/apache/maka/pull/5421#discussion_r4032837960
##########
apps/desktop/src/main/commandcode-browser-login.ts:
##########
@@ -227,13 +227,21 @@ export class CommandCodeBrowserLoginController {
cancel(attemptId?: string): void {
const attempt = attemptId === undefined ? this.#current : this.#attempts.get(attemptId);
if (attempt === undefined) return;
- this.#finish(attempt, { ok: false, reason: 'cancelled' });
+ this.#abandon(attempt, { ok: false, reason: 'cancelled' });
}
dispose(): void {
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
Review Comment:
[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()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): drop Command Code login attempts nobody will collect [maka]
Posted by "Totoro-qaq (via GitHub)" <gi...@apache.org>.
Totoro-qaq closed pull request #5421: fix(desktop): drop Command Code login attempts nobody will collect
URL: https://github.com/apache/maka/pull/5421
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): drop Command Code login attempts nobody will collect [maka]
Posted by "hqhq1025 (via GitHub)" <gi...@apache.org>.
hqhq1025 commented on code in PR #5421:
URL: https://github.com/apache/maka/pull/5421#discussion_r4033250855
##########
apps/desktop/src/main/commandcode-login-ipc-main.ts:
##########
@@ -39,17 +39,42 @@ export const COMMANDCODE_LOGIN_IPC_CHANNELS = {
*/
export interface CommandCodeLoginIpcDeps {
readonly ipcMain: Pick<IpcMain, 'handle'>;
- readonly controller: Pick<CommandCodeBrowserLoginController, 'start' | 'complete' | 'cancel'>;
+ readonly controller: Pick<
+ CommandCodeBrowserLoginController,
+ 'start' | 'complete' | 'cancel' | 'abandonOwner'
+ >;
}
const MAX_BASE_URL_CHARS = 2_048;
const MAX_ATTEMPT_ID_CHARS = 128;
export function registerCommandCodeLoginIpc(deps: CommandCodeLoginIpcDeps): void {
+ // The controller outlives every renderer, and crash recovery reloads the
+ // same WebContents without destroying it. A renderer that goes away mid-login
+ // never sends the complete() or cancel() that would release its attempt.
+ const observedOwners = new Set<string>();
+ const bindOwner = (event: IpcMainInvokeEvent): string => {
+ const ownerId = `web-contents:${event.sender.id}`;
+ if (!observedOwners.has(ownerId)) {
+ observedOwners.add(ownerId);
+ // Whichever event comes first detaches both, so this runs once per
+ // observation; a recovered renderer's next start observes it afresh.
+ const abandon = () => {
+ observedOwners.delete(ownerId);
+ event.sender.removeListener('render-process-gone', abandon);
+ event.sender.removeListener('destroyed', abandon);
+ deps.controller.abandonOwner(ownerId);
+ };
+ event.sender.once('render-process-gone', abandon);
+ event.sender.once('destroyed', abandon);
Review Comment:
[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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): drop Command Code login attempts nobody will collect [maka]
Posted by "Totoro-qaq (via GitHub)" <gi...@apache.org>.
Totoro-qaq commented on PR #5421:
URL: https://github.com/apache/maka/pull/5421#issuecomment-5794925730
Superseded by #5545, which removed the Command Code GO provider together with `commandcode-browser-login` and its IPC, so there is nothing left for this change to fix. Closing. Thanks to hqhq1025 and Astro-Han for the reviews.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): drop Command Code login attempts nobody will collect [maka]
Posted by "Totoro-qaq (via GitHub)" <gi...@apache.org>.
Totoro-qaq commented on code in PR #5421:
URL: https://github.com/apache/maka/pull/5421#discussion_r4033114009
##########
apps/desktop/src/main/commandcode-browser-login.ts:
##########
@@ -227,13 +227,21 @@ export class CommandCodeBrowserLoginController {
cancel(attemptId?: string): void {
const attempt = attemptId === undefined ? this.#current : this.#attempts.get(attemptId);
if (attempt === undefined) return;
- this.#finish(attempt, { ok: false, reason: 'cancelled' });
+ this.#abandon(attempt, { ok: false, reason: 'cancelled' });
}
dispose(): void {
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
Review Comment:
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._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): drop Command Code login attempts nobody will collect [maka]
Posted by "Totoro-qaq (via GitHub)" <gi...@apache.org>.
Totoro-qaq commented on code in PR #5421:
URL: https://github.com/apache/maka/pull/5421#discussion_r4033633522
##########
apps/desktop/src/main/commandcode-login-ipc-main.ts:
##########
@@ -39,17 +39,42 @@ export const COMMANDCODE_LOGIN_IPC_CHANNELS = {
*/
export interface CommandCodeLoginIpcDeps {
readonly ipcMain: Pick<IpcMain, 'handle'>;
- readonly controller: Pick<CommandCodeBrowserLoginController, 'start' | 'complete' | 'cancel'>;
+ readonly controller: Pick<
+ CommandCodeBrowserLoginController,
+ 'start' | 'complete' | 'cancel' | 'abandonOwner'
+ >;
}
const MAX_BASE_URL_CHARS = 2_048;
const MAX_ATTEMPT_ID_CHARS = 128;
export function registerCommandCodeLoginIpc(deps: CommandCodeLoginIpcDeps): void {
+ // The controller outlives every renderer, and crash recovery reloads the
+ // same WebContents without destroying it. A renderer that goes away mid-login
+ // never sends the complete() or cancel() that would release its attempt.
+ const observedOwners = new Set<string>();
+ const bindOwner = (event: IpcMainInvokeEvent): string => {
+ const ownerId = `web-contents:${event.sender.id}`;
+ if (!observedOwners.has(ownerId)) {
+ observedOwners.add(ownerId);
+ // Whichever event comes first detaches both, so this runs once per
+ // observation; a recovered renderer's next start observes it afresh.
+ const abandon = () => {
+ observedOwners.delete(ownerId);
+ event.sender.removeListener('render-process-gone', abandon);
+ event.sender.removeListener('destroyed', abandon);
+ deps.controller.abandonOwner(ownerId);
+ };
+ event.sender.once('render-process-gone', abandon);
+ event.sender.once('destroyed', abandon);
Review Comment:
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 57408bea. 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._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org