Skip to content

fix(desktop): attach renderer-lifecycle diagnostics to all BrowserWindow instances (#81290) - #81533

Closed
Enough1122 wants to merge 4 commits into
NousResearch:mainfrom
Enough1122:fix/81290-secondary-window-lifecycle
Closed

fix(desktop): attach renderer-lifecycle diagnostics to all BrowserWindow instances (#81290)#81533
Enough1122 wants to merge 4 commits into
NousResearch:mainfrom
Enough1122:fix/81290-secondary-window-lifecycle

Conversation

@Enough1122

Copy link
Copy Markdown
Contributor

Summary

Secondary session windows and full-instance windows on Hermes Desktop previously attached no renderer-lifecycle diagnostics (render-process-gone, unresponsive, oom, console errors). A peer renderer crash therefore produced a permanently black window with no log evidence and no recovery, exactly as described in #81290 by @akivavh (three reproductions, sanitized live captures, all render-process-gone/unresponsive/did-fail-load lookups returning empty for the affected HWND).

Root cause

The main window had a hand-rolled crash handler attached directly. spawnSecondaryWindow() and createInstanceWindow() only attached navigation / shortcut / zoom / context-menu wiring; nothing listened for renderer death. The diagnostic gap and the recovery gap were the same gap.

Fix

Promote the primary-window crash handling into a shared, Electron-free, dependency-injectable helper electron/window-renderer-lifecycle.ts and install it on every BrowserWindow we create:

Window kind Behavior
main / secondary / instance Full recovery under a shared rolling crash-loop budget (3 reloads / 60s). Identical semantics to the previous primary-only behavior, including the #38216 Windows sandbox-relaunch callback, which is wired through callbacks.onCrashLoopSuppressed and only fires for kind: 'main'.
overlay / quick / wake Log-only. Auxiliary windows must never resurrect themselves.

Logs carry the window kind so black-window reports become attributable, e.g.:

[renderer:secondary] render-process-gone reason=crashed exitCode=3

This is the "黑窗有据可查" property requested in the issue.

Why this is the whole-bug-class fix

Sibling call paths (spawnSecondaryWindow, createInstanceWindow, createOverlayWindow, createQuickWindow, createWakeIndicatorWindowController) all route through the new helper, so any future window added to main.ts only needs the helper call — it inherits the same diagnostics and budget.

Verification

Check Result
pnpm -C apps/desktop test (electron project) — new + wake-indicator suites 21/21 pass
tsc -p tsconfig.electron.json --noEmit 0 error
eslint apps/desktop/electron/ 0 error / 0 warning
Existing electron test surface (full project) Same 17 pre-existing failures as a clean HEAD stash — environmental, not introduced (verified by stash)
Independent code-reviewer agent review APPROVE, 0 critical / 0 high. 3 LOW findings fixed in-tree. Behavior equivalence verified line-by-line against the original primary-window path (rolling-guard semantics, #38216 guard inversion, console dual-shape parsing)

Out of scope (deliberate)

  • An e2e Playwright regression that crashes a secondary renderer in a real Electron environment. The unit suite covers the helper exhaustively (event dispatch, budget rollover, kind-specific behavior, cleanup, console-error capture), but a true e2e requires running the full desktop app and is parked behind a separate issue if reviewers want it.

Related

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/desktop Electron desktop app (apps/desktop/*) platform/windows Native Windows-specific behavior or breakage sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows labels Aug 8, 2026
@Enough1122
Enough1122 force-pushed the fix/81290-secondary-window-lifecycle branch from 6d466c6 to c213f99 Compare August 8, 2026 06:30
@Enough1122

Copy link
Copy Markdown
Contributor Author

PR cleanup: removed developer-process docs (INVESTIGATION.md / PLAN.md / STATUS.md) and the accidental .omc/ .gitignore entry that were committed with the fix; normalized the new TS files to LF line endings. The change is now scoped to the desktop renderer-lifecycle fix only.

@spfcraze

spfcraze commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

This was generated by AI during triage.

Summary:
The description says the helper installs on every BrowserWindow, but the OAuth login and portal login windows in main.ts are not wired, so a crashed sign-in renderer leaves no lifecycle entry in desktop.log.

Problems:

  • apps/desktop/electron/main.ts creates a BrowserWindow in both openOauthLoginWindow (main.ts:6089) and openPortalLoginWindow (main.ts:6591); neither calls installWindowRendererLifecycle.
  • The description's "install it on every BrowserWindow we create" is broader than the diff's coverage: its table enumerates the six wired kinds — main/secondary/instance (full recovery, 3-reloads/60s) and overlay/quick/wake (log-only) — the login windows are in neither.

Solution:
As a wording that matches the diff: the helper installs on the main, secondary, instance, overlay, quick, and wake windows. If the login windows are meant to be included, wiring them log-only would make the description accurate.


Checked against c213f99 — the tip of fix/81290-secondary-window-lifecycle when this was written — and 70c6cf8, main at the same moment.

@Enough1122
Enough1122 force-pushed the fix/81290-secondary-window-lifecycle branch 2 times, most recently from ab9a785 to 518cab6 Compare August 9, 2026 04:16
Enough1122 and others added 3 commits August 10, 2026 15:17
…al login windows (NousResearch#81290 follow-up)

@spfcraze's triage review noted the PR description claimed "every
BrowserWindow" but the OAuth and portal sign-in windows were not wired:
a crashed sign-in renderer leaves the window's promise path never
settling, with no trace in desktop.log.

Wire both with the same log-only lifecycle diagnostics as the overlay
and quick windows — `kind: 'oauth'` and `kind: 'portal'` respectively.
Neither window gets crash-reload treatment (a sign-in window that
reloads itself mid-auth would be surprising); the lifecycle helper's
log-only callback is the exact contract needed here.

window-renderer-lifecycle.test.ts: 17/17 pass.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@Enough1122
Enough1122 force-pushed the fix/81290-secondary-window-lifecycle branch from 518cab6 to 0709827 Compare August 10, 2026 07:36
…re window-reveal (NousResearch#81290)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
teknium1 added a commit that referenced this pull request Aug 11, 2026
Reconcile the salvaged #81533 lifecycle helper with the renderer-log
console pipeline that landed in #83535 (the two PRs raced):

- window-renderer-lifecycle.ts no longer handles console-message —
  renderer-log.ts is the single owner (per-window labels, boundary
  reports). One owner means no double-logged errors on windows wearing
  both, and OAuth/portal windows (lifecycle-wired for process events)
  cannot spill third-party page console output into desktop.log.
- wake indicator window gets attachRendererConsoleCapture, keeping the
  console coverage it previously got from the helper.
- HUD window (added after the PR branched) gets log-only lifecycle
  coverage — it was the one renderer window the PR couldn't have known
  about.
- Tests updated: lifecycle helper asserts it attaches NO console-message
  listener; parser tests live in renderer-log.test.ts.
@teknium1

Copy link
Copy Markdown
Contributor

Merged via PR #83567 — thank you for the excellent work here! All four of your commits were cherry-picked onto current main with your authorship preserved in git log (your branch had fallen ~114 commits behind).

One reconciliation was needed on top: PR #83535 landed after your branch and gave console-message capture a dedicated owner (renderer-log.ts, with per-window labels and error-boundary persistence), so the lifecycle helper's console handling was removed to keep a single owner — this also prevents OAuth/portal pages from spilling third-party console output into desktop.log. Your process-event coverage (render-process-gone / unresponsive / did-fail-load), the shared crash-loop budget, and the full test suite are intact, and the HUD window (added after you branched) now gets the same coverage.

The Electron-free dependency-injected design made this salvage painless — nice architecture.

@teknium1 teknium1 closed this Aug 11, 2026
@Enough1122
Enough1122 deleted the fix/81290-secondary-window-lifecycle branch August 11, 2026 02:40
blut-agent pushed a commit to blut-agent/hermes-agent-fork that referenced this pull request Aug 11, 2026
Reconcile the salvaged NousResearch#81533 lifecycle helper with the renderer-log
console pipeline that landed in NousResearch#83535 (the two PRs raced):

- window-renderer-lifecycle.ts no longer handles console-message —
  renderer-log.ts is the single owner (per-window labels, boundary
  reports). One owner means no double-logged errors on windows wearing
  both, and OAuth/portal windows (lifecycle-wired for process events)
  cannot spill third-party page console output into desktop.log.
- wake indicator window gets attachRendererConsoleCapture, keeping the
  console coverage it previously got from the helper.
- HUD window (added after the PR branched) gets log-only lifecycle
  coverage — it was the one renderer window the PR couldn't have known
  about.
- Tests updated: lifecycle helper asserts it attaches NO console-message
  listener; parser tests live in renderer-log.test.ts.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/desktop Electron desktop app (apps/desktop/*) P2 Medium — degraded but workaround exists platform/windows Native Windows-specific behavior or breakage sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Secondary Desktop window stays black with no lifecycle diagnostics or recovery

4 participants