fix(desktop): retire dead pooled remotes and log main-process faults - #72808
Closed
OutThisLife wants to merge 2 commits into
Closed
fix(desktop): retire dead pooled remotes and log main-process faults#72808OutThisLife wants to merge 2 commits into
OutThisLife wants to merge 2 commits into
Conversation
A pooled backend entry pointing at a remote host has no child process, so the 'exit' handler that clears a dead local backend never fires. The renderer's 60s keepalive touch also spares it from the idle reaper. Nothing was left to retire the descriptor, so once the host went away the pool kept serving it and every profile bound to that host stayed broken until restart. Pooled remote descriptors now share the primary's liveness policy: probed on the same revalidate tick, keyed per base URL, and dropped only after the same consecutive-failure limit, so the next ensureBackend() rebuilds. Co-authored-by: Rodrigo Fernandez <rod@nxtlevel.dev>
Electron pre-installs its own uncaughtException listener and only warns on unhandled rejections, so a main-process fault usually leaves the app running with the reason on stderr — which nothing captures when the app is launched from Finder or the Start menu. The fault never reaches desktop.log, so it is absent from `hermes debug share` and the user can only describe symptoms. Record both to desktop.log and flush synchronously, since a fault that does prove fatal leaves no chance for the batched async flush. Five loadURL calls were also unhandled, each able to leave a blank window with no explanation anywhere the user can send us; they now name the surface that failed. Co-authored-by: Rodrigo Fernandez <rod@nxtlevel.dev>
Contributor
૮ >ﻌ< ა ci reviewran on c34ff39 all good! |
19 tasks
Collaborator
Author
|
Folded into #72835 along with the rest of the cluster. |
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.
What does this PR do?
The last two pieces of #58108 that no port has picked up. #68250 took the liveness and auth-classification work, #72740 takes the global-remote pool routing, and #67578 takes the OAuth session aggregate. These two remain.
Pooled remote backends are never retired when their host dies. A pool entry backed by a remote host has
entry.process === null, so the'exit'handler that clears a dead local backend never fires, and the renderer's 60s keepalive touch keeps the idle reaper off it. Revalidation only ever probed the primary. Nothing was left to notice the host had gone, so the pool served a dead descriptor until the app restarted and every profile bound to that host stayed broken. Pooled remotes now share the primary's liveness policy — same probe on the same revalidate tick, keyed per base URL, dropped only after the same consecutive-failure limit, soensureBackend()rebuilds on the next call.A main-process fault leaves no trace we can read. Electron pre-installs its own
uncaughtExceptionlistener and only warns on unhandled rejections, so the app typically survives and the reason goes to stderr — discarded entirely when launched from Finder or the Start menu. It never reachesdesktop.log, so it is absent fromhermes debug shareand the user can only describe symptoms. Both are now recorded and flushed synchronously, since a fault that does prove fatal leaves no chance for the batched async flush. FiveloadURLcalls were unhandled too, each able to leave a blank window with nothing to explain it; they now name the surface that failed.The second half is diagnostics rather than a behavior fix, and it is here because it is what the current round of remote-desktop reports keeps costing us: users spend hours guessing because the bundle is silent about what actually broke.
Related Issue
Related: #58108. Completes the four defects listed in that PR's review — the other two land in #72740 and #67578.
Type of Change
Changes Made
apps/desktop/electron/remote-liveness.ts— addedrevalidatePooledRemoteBackends(), reusing the existingRemoteLivenessTrackerpolicy rather than introducing a second one.apps/desktop/electron/main.ts— recordentry.remoteBaseUrlwhen a pool entry resolves to a remote, and probe the pool on the samehermes:connection:revalidatetick as the primary.apps/desktop/electron/crash-forensics.ts(new) —describeCrashReason()andinstallCrashForensics(), dependency-injected so the handlers are tested for real rather than by reading source.apps/desktop/electron/main.ts— install the handlers, and route the five unhandledloadURLcalls through a labelled helper.How to Test
apps/desktop:npx vitest run --project electronapps/desktop:npm run typecheckdesktop.logand survives intohermes debug share.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests pass — N/A, Desktop-onlyDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AScreenshots / Logs
Local validation on macOS, branched from
c2e45b555:npx vitest run --project electron: 803 passed, 2 skippednpm run typecheck: passedOriginal diagnosis and the
main.cjsimplementation of both pieces are @rod-nxtlevel's in #58108; credited in the commit trailers.