Skip to content

fix(updater): send status only to a live main window (#154) - #175

Merged
nplez1 merged 5 commits into
nplez1/mainfrom
nplez1/issue-154
Oct 24, 2026
Merged

nplez1 merged 5 commits into
nplez1/mainfrom
nplez1/issue-154

Conversation

@nplez1

@nplez1 nplez1 commented Oct 24, 2026

Copy link
Copy Markdown
Owner

Fixes #154

Summary

The updater held the main window forever and sent every status through
mainWindowRef?.webContents.send(...) with no isDestroyed() guard. When the window was destroyed
before the next updater event — closed and reopened on macOS, or recreated after a renderer failure —
the send threw TypeError: Object has been destroyed, escaped as an unhandled rejection, and no
window ever learned the status. currentStatus had already been updated, so the replacement window
started with a status the renderer was never told about.

  • One binding owns the window. New UpdaterWindowBinding layer (bindMainWindow,
    releaseMainWindow, liveMainWindow, sendToMainWindow). Binding registers a closed listener
    that clears the reference — only when it still points at that window, so a replacement window's
    predecessor cannot unbind it. All 7 former mainWindowRef?.webContents.send(...) sites now go
    through sendToMainWindow, which skips a destroyed window or webContents and swallows a send
    that races window teardown. Keeping sendStatus running past the send also means the queued
    user-initiated check still launches — previously a throw skipped it.
  • A status that lands in the gap is not lost. Binding replays currentStatus once. The replay is
    skipped only for the untouched { state: 'idle' } default — the renderer's own initial store value
    — so a real recorded idle (a benign background-check failure, an abandoned local-build card) still
    reaches the replacement window. The renderer also pulls status on bootstrap, but that pull can
    resolve before the bind, so the replay is what makes the recovery order-independent.
  • Window-parented native UI follows the same rule. The local-build picker gets liveMainWindow(),
    so a dialog is never parented to a destroyed window.
  • A failed send is recorded (renderer_send_failed) instead of vanishing silently.

Verification

pnpm tc                                  # clean — 18,061 node / 3,231 cli / 34 config / 16,084 web files
pnpm test src/main/updater               # 34 files, 423 tests passed
pnpm run check:code-quality:changed      # passed, 0 new findings
oxfmt --check, oxlint (changed files)    # clean

The full pnpm test suite was not run: this machine's unit gate has the known runtime trap
called out in the task, and the shared orca-queue was held by other worktrees' pnpm tc/test
for the whole session (load average stayed ~1.5, so the focus was verified without it). The focused
suites above are the updater's own; no test outside src/main/updater* was touched.

New src/main/updater.window-lifecycle.test.ts (8 tests) covers a send after the bound window
closed, after only its webContents went away, and a send that itself throws; the replay of a status
that landed while no window was bound, including a recorded idle; the untouched default not being
replayed; a replaced window's closed event not unbinding its successor; and headless serve setting
up with no window. Five of them fail against the previous code. The 18 updater suites now build their
window from createUpdaterMainWindow, which throws like Electron does once destroyed and drives
close/destroyWindow/destroyWebContents separately — the fake cannot make a missing guard look
harmless. That fixture is also a real BrowserWindow, so 97 as never escapes went away.

Review

Reviewed by Opus 5.5, high (claude/opus, Claude Code CLI backend), read-only, over the full diff
against nplez1/main:

  • R1 — no BLOCKER, no SHOULD-FIX. 3 MINOR fixed in 016b979e83 / 5e1c8051fa: the idle check
    skipped real recorded idles (now an identity comparison against the initial status), a failed send
    was swallowed without a trace (now recorded), and the fake window never threw the way Electron does
    (it does now).
  • Delta re-check R1..R2 — all three resolved; 1 new MINOR fixed in 07d5bd7fc3: with the fixture
    throwing, "the send did not throw" was also satisfied by the catch it is backstopped by, so the two
    destroyed-window tests now also assert that no send failed at all.

Remaining NITs, left by choice: the new tests drive the synchronous menu-check path rather than
autoUpdater's async error event (every send goes through the one helper); the local-build dialog
guard has no test; updater:quitAndInstallAborted still goes to the bound window rather than the
invoking renderer (pre-existing).

The updater kept the main window forever and sent every status through
`mainWindowRef?.webContents.send(...)`. When the window was destroyed before
the next updater event — closed and reopened on macOS, or recreated after a
renderer failure — the send threw `TypeError: Object has been destroyed`,
escaped as an unhandled rejection, and the renderer never learned the status.

- Bind the window through `bindMainWindow`, which registers the `closed`
  listener that clears the reference (only when it still points at that
  window) and replays the current status once so a status that landed during
  the gap is not lost. The untouched `idle` default is not replayed: it is
  already the renderer's own initial status.
- Route every updater send — status, quit-and-install abort, nudge
  dismissal clearing — through `sendToMainWindow`, which skips a destroyed
  window or webContents and swallows a send that races window teardown. That
  also keeps `sendStatus` running past the send, so the queued user-initiated
  check still launches.
- Parent the local-build picker to the window only while it is alive.
The updater suites faked the main window as `{ webContents: { send } }`, which
cannot express the teardown the service now guards against, and every fake
needed the same `as never` escape hatch at each call site.

- `createUpdaterMainWindow` is a `BrowserWindow` the service can bind, guard
  and send through, with `close`, `destroyWindow` and `destroyWebContents`
  driving the two teardown paths separately. Being a `BrowserWindow`, it also
  removes 97 now-pointless casts from the suites. It lives beside the other
  updater test fixtures rather than inside `updater-test-harness.ts`, which is
  already at its size limit.
- New `updater.window-lifecycle.test.ts` covers a send after the bound window
  closed, after only its webContents went away, and a send that itself throws;
  the replay of a status that landed while no window was bound; the untouched
  default not being replayed; a replaced window's `closed` event not unbinding
  its successor; and headless serve setting up with no window.
The bind-time replay skipped `state === 'idle'`, which is not the same set as
the untouched default. A replacement window's renderer can pull `checking` or
`available` from `updater:getStatus` before it is bound, while `ready-to-show`
plus a `setImmediate` — or the 15 s fallback — delays the bind. If the check
then settles back to idle, the bind skipped the replay and that window kept a
spinner or a dead card for a status the user should have seen cleared.

Compare against the initial status object instead, so only the default the
renderer already starts from is skipped.

Also record a failed send. The catch exists for the teardown race, but it hid
every other send failure — a payload Electron cannot clone, for one — with no
trace and no guarantee that a later bind arrives.
The fake window's `send` succeeded even after `close()` or `destroyWebContents()`,
so the `not.toThrow()` assertions in the lifecycle suite could never fail and
the fixture never reproduced the reported crash. It now throws the same
`TypeError: Object has been destroyed` Electron raises, before the spy records
the call.

Adds the recorded-idle replay case the looser guard missed, and drops three
`oxlint-disable` directives whose casts the fixture replaced.
Now that the fake window throws once it is destroyed, "the send did not throw"
is also satisfied by the send-failure catch alone, so the two destroyed-window
tests no longer prove an untouched guard — only a backstopped send. Assert
that no send failed at all, and cover the new lifecycle log the catch writes.

The recorded-idle case also keeps its replacement setup on the current cadence,
so a background check cannot run between the bind and the assertion.
@nplez1
nplez1 merged commit 45b2213 into nplez1/main Oct 24, 2026
@nplez1
nplez1 deleted the nplez1/issue-154 branch October 24, 2026 03:35
nplez1 added a commit that referenced this pull request Oct 28, 2026
The release gate on the merge found 140 failures across 25 test files. Three
were merge damage in product code:

- `updater-prerelease-feed.ts`: upstream's new `fetchLatestStableReleaseTag`
  hardcoded `stablyai/orca` in both the `releases/latest` URL and the redirect
  regex, so a fork build's stable-fallback probe never matched the fork's own
  redirect and every background check died in preflight. Retargeted to
  `nplez1/orca` with the rest of the file.
- `use-checks-panel-ai-acknowledgement.tsx`: the resolution dropped the
  latest-consumer refresh ref both parents had, so a head-churn rerender bound
  the post-launch refresh to a stale `fetchComments` and the forced refresh was
  silently dropped.
- `ai-vault-session-refresh.ts`: upstream declares the throttle test hook in
  this module; the fork's throttle lives in its own file, so the name is
  re-exported from where upstream's suites import it.

The rest were expectations that predate the code they assert — the fork's own
PRs (#134 refresh-on-refusal, #169 reply-only comment ack, #170 gh exec
options, #198 keep-cached-check-details, #175 window binding) changed behaviour
with `pr.yml` disabled and no unit lane backstopping them, so `main` was red
before this merge. Each expectation now asserts the fork's contract; no test was
deleted, skipped, or widened. Four reference repairs follow modules the merge
moved (`filesystem-auth`, `filesystem-search-file-paths`), two fixtures gained
fields upstream added, and two suites needed the WSL gate mocked so they stop
spawning a real `wsl.exe` per case.

`ws-inbound-message-queue.test.ts` is a GC-timing flake, not a fix: it passes
18/18 in isolation and the gate's isolation re-run reproduces it as green.

Verified per file with `pnpm test <path>` — all 25 green — plus `pnpm tc:web`
clean and the identity sweep still 0 fixable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Updater sends status to a destroyed window — unhandled rejection and a lost status during window recreation

1 participant