Skip to content

fix(update): skip redundant GUI restart after npm self-update - #515

Merged
Wibias merged 4 commits into
lidge-jun:devfrom
Wibias:fix/gui-update-redundant-restart
Jul 26, 2026
Merged

fix(update): skip redundant GUI restart after npm self-update#515
Wibias merged 4 commits into
lidge-jun:devfrom
Wibias:fix/gui-update-redundant-restart

Conversation

@Wibias

@Wibias Wibias commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • After a GUI update with restart enabled, npm installs already run node ocx.mjs update, which stops the proxy and reinstalls/starts the managed service (or falls back to a direct start on the captured port, default 10100).
  • The worker then ran a second service install, which calls stopWindows() on that healthy listener and often fails elevation from the non-interactive update worker — leaving the proxy dead until a manual restart.
  • Fix: confirm the npm self-update restart first; only run the explicit restart path when that soft health check fails. Bun installs still always take the explicit restart path.

Test plan

  • bun test tests/update-job.test.ts
  • bun x tsc --noEmit
  • bun run privacy:scan
  • Windows: GUI update with restart while Task Scheduler service is installed — proxy returns on 10100 without a manual ocx start
  • Windows: GUI update when npm self-update cannot restart — worker still performs explicit restart and surfaces failure if health never returns

Summary by CodeRabbit

  • Bug Fixes

    • Improved GUI update restart handling to avoid redundant restarts after npm-based self-updates.
    • Added clearer failure handling when the proxy does not recover, including actionable restart guidance.
    • Ensured explicit restarts continue for update methods that require them.
  • Documentation

    • Documented npm GUI self-update behavior and the risks of performing an unnecessary second service installation.

npm ocx update already restarts the service; a second service install stopped the healthy proxy on 10100 and often failed elevation from the non-interactive worker.
@github-actions github-actions Bot added the bug Something isn't working label Jul 26, 2026
@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@Wibias, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 19 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 9fd189b7-e8f4-44cd-bb6d-f25874124a97

📥 Commits

Reviewing files that changed from the base of the PR and between 0be28c4 and 7fb8e2f.

📒 Files selected for processing (5)
  • docs/adr/0001-gui-update-worker.md
  • gui/src/client-resource.ts
  • gui/tests/debug-put-install-order.test.tsx
  • src/update/job.ts
  • tests/update-job.test.ts
📝 Walkthrough

Walkthrough

Changes

GUI update restart flow

Layer / File(s) Summary
Structured proxy restart health
src/update/job.ts
RestartIo accepts an injected restart function, health polling returns timeout or flapped outcomes, and confirmation maps those outcomes to job failures with restart hints (lines 309–314, 413–486).
Installer-aware restart completion
src/update/job.ts
finishGuiUpdateRestart skips redundant npm restarts when the proxy is healthy, performs explicit restarts otherwise, and replaces the worker’s direct restart sequence (lines 499–531, 651).
Restart behavior validation and documentation
tests/update-job.test.ts, docs/adr/0001-gui-update-worker.md
Tests cover npm healthy and unhealthy paths plus Bun explicit restarts; the ADR documents the npm sequencing and failure consequence (lines 8, 370–461; lines 30–36).

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant runGuiUpdateWorker
  participant finishGuiUpdateRestart
  participant proxy
  participant restartAfterUpdate
  runGuiUpdateWorker->>finishGuiUpdateRestart: complete GUI update restart
  finishGuiUpdateRestart->>proxy: probe identity-verified health
  alt npm proxy already healthy
    finishGuiUpdateRestart-->>runGuiUpdateWorker: skip redundant restart
  else proxy unhealthy or non-npm installer
    finishGuiUpdateRestart->>restartAfterUpdate: perform explicit restart
    finishGuiUpdateRestart->>proxy: confirm restarted health
    proxy-->>finishGuiUpdateRestart: stable health result
  end
Loading

Suggested reviewers: ingwannu, lidge-jun

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: avoiding redundant GUI restarts after npm self-update.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

coderabbitai[bot]

This comment was marked as resolved.

chatgpt-codex-connector[bot]

This comment was marked as resolved.

Wibias added 3 commits July 26, 2026 22:53
Address CodeRabbit/Codex review: only probe-first for service-managed npm
installs, require PID change or target version before skipping, and keep
UpdateJobState fixtures type-complete.
Windows CI failed when a prior Debug test left a warm module cache for
debug-settings:http://localhost, skipping the cold-start GET so pollCount
stayed 0. Clear stores around the test and stub window.event for React 19.
A no-op or failed explicit restart could leave the pre-update proxy
healthy and still report success. Re-check PID/version evidence after
npm restart and fail when the old process remains.
@Wibias
Wibias merged commit 0aa0c62 into lidge-jun:dev Jul 26, 2026
9 checks passed
@Wibias
Wibias deleted the fix/gui-update-redundant-restart branch July 27, 2026 03:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant