Skip to content

fix(desktop): attempt renderer reload on killed reason, not just crashed/oom - #43716

Closed
liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:fix/desktop-renderer-killed-reload
Closed

fix(desktop): attempt renderer reload on killed reason, not just crashed/oom#43716
liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:fix/desktop-renderer-killed-reload

Conversation

@liuhao1024

Copy link
Copy Markdown
Contributor

What does this PR do?

Adds 'killed' to the render-process-gone handler's reload condition so the renderer attempts recovery when externally terminated (e.g. SIGTERM), not just on crashed or oom. This prevents the boot loop reported in #43640 where the renderer is killed but never reloaded while the backend keeps restarting.

Related Issue

Fixes #43640

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • apps/desktop/electron/main.cjs: Added 'killed' to the render-process-gone reason check. The existing crash-loop guard (RENDERER_RELOAD_MAX = 3 within RENDERER_RELOAD_WINDOW_MS = 60s) already prevents infinite reload attempts if the kill is persistent.

How to Test

  1. Launch the Desktop app normally — verify renderer loads without issues
  2. Send SIGTERM to the renderer's PID (or simulate a killed renderer) — the app should attempt a reload instead of staying on a blank screen
  3. Verify the crash-loop guard still works: if the renderer keeps getting killed (>3 times within 60s), the app should suppress further reload attempts and log the suppression

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Code Intelligence

  • Analyzed: apps/desktop/electron/main.cjs render-process-gone handler (lines 4963–4995)
  • Blast radius: LOW — single conditional branch in Electron main process, guarded by existing crash-loop limiter
  • Related patterns: RENDERER_RELOAD_MAX, RENDERER_RELOAD_WINDOW_MS constants at line 564–565

…hed/oom

The render-process-gone handler only reloaded the renderer when the
reason was 'crashed' or 'oom'. When the renderer is killed externally
(reason='killed', exitCode=15/SIGTERM), the handler logged but never
attempted a reload, leaving the window blank while the backend
continued restarting — producing the boot loop reported in NousResearch#43640.

Add 'killed' to the reload condition. The existing crash-loop guard
(RENDERER_RELOAD_MAX=3 within 60s) prevents infinite reload attempts
if the kill is persistent.

Fixes NousResearch#43640
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused recovery hardening. The premise remains valid on current main: apps/desktop/electron/main.ts:7312 reloads only crashed and oom renderers, so killed still bypasses the existing bounded recovery path.

Problems

  • The PR modifies apps/desktop/electron/main.cjs, but that file was renamed to apps/desktop/electron/main.ts by 39d09453f95e8aefc0c97e5d9b30ff341cae9ed8. Desktop now bundles electron/main.ts (apps/desktop/package.json:9,22), so this diff cannot affect the current product as written.
  • No test currently exercises the reload-reason decision; a source/test search finds render-process-gone and RENDERER_RELOAD_* only in apps/desktop/electron/main.ts.

Suggested changes

  • Port the killed condition to apps/desktop/electron/main.ts:7312 and retain the existing limiter.
  • Add focused coverage for killed using the same bounded-reload contract as crashed and oom.

The #43640 reporter later identified dashboard-auth 401 as that incident's root cause, while also confirming this remains useful general bounded-recovery hardening. This is an automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Closing as superseded by design — PR #83567 (salvaged from #81533) just landed per-window renderer-lifecycle handling, and it deliberately keeps killed as a log-only reason rather than a reload trigger.

The reasoning: killed is what Chromium reports for expected teardown (user closes the window, app quits, OS reclaims the process). Blindly reloading on it would loop windows back to life after the user closed them. The recoverable set stays crashed/oom, bounded by a shared 3-reloads/60s crash-loop budget.

The diagnostic gap that motivated this PR is covered too: every window now logs [renderer:<kind>] render-process-gone reason=killed exitCode=…, annotated with (expected teardown) when the window was already destroyed — so a suspicious killed on a live window is visible in desktop.log and can be investigated with evidence rather than guessed at.

Thanks for the contribution, and sorry it sat so long — if you're seeing live windows getting killed without user action (e.g. by an OOM killer that bypasses Chromium's oom reason), that log line is now the breadcrumb to file with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform 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.

Desktop App: Renderer crashes with exitCode=15 causing boot loop

3 participants