Skip to content

fix(update): tolerate late proxy readiness - #737

Merged
lidge-jun merged 1 commit into
lidge-jun:devfrom
luvs01:fix/720-update-readiness-boundary
Jul 30, 2026
Merged

fix(update): tolerate late proxy readiness#737
lidge-jun merged 1 commit into
lidge-jun:devfrom
luvs01:fix/720-update-readiness-boundary

Conversation

@luvs01

@luvs01 luvs01 commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Summary

  • extend the replacement-proxy arrival budget from 15 seconds to 30 seconds while preserving the existing 15-second stability window
  • guarantee one identity-aware health probe at the arrival deadline so a proxy that becomes ready on the boundary is not misclassified as timed out
  • add deterministic regressions for readiness at 30 seconds and for the reported post-15-second npm service replacement, including the no-redundant-restart evidence path

This stays intentionally narrower than #533/#557: it does not change process recovery, port reclamation, or restart ownership, and both current PR heads still retain the 15-second arrival loop.

Fixes #720

Verification

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed. (No documentation change is needed for this bounded readiness correction.)
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Summary by CodeRabbit

  • Bug Fixes

    • Improved restart health checks to recognize services that become healthy right at the timeout deadline.
    • Extended the restart health-check window from 15 to 30 seconds.
    • Adjusted restart confirmation timing to avoid unnecessary restarts when a replacement service becomes healthy shortly after the previous cutoff.
  • Tests

    • Added coverage for deadline health checks and successful replacement-service confirmation.

@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The restart health timeout increases to 30 seconds, polling now performs a deadline-accurate final probe, and GUI update tests cover both deadline-boundary recovery and delayed replacement proxy acceptance without redundant restarts.

Changes

Restart health handling

Layer / File(s) Summary
Deadline-aware restart polling
src/update/job.ts
The restart health timeout changes from 15 to 30 seconds; polling dynamically bounds sleep intervals and performs a final probe at the deadline. The decision-log comment describes the 30-second arrival plus 15-second stability window.
Delayed replacement proxy coverage
tests/update-job.test.ts
Tests verify the final deadline probe and confirm that a replacement proxy becoming healthy after the previous cutoff succeeds without invoking another restart, while expected log entries are recorded.

Estimated code review effort: 2 (Simple) | ~10 minutes

Sequence Diagram(s)

sequenceDiagram
  participant awaitRestartedProxyHealthy
  participant probeProxy
  participant sleepMs
  awaitRestartedProxyHealthy->>probeProxy: Probe proxy health
  probeProxy-->>awaitRestartedProxyHealthy: Return health status
  awaitRestartedProxyHealthy->>sleepMs: Sleep for bounded remaining interval
  sleepMs-->>awaitRestartedProxyHealthy: Reach deadline
  awaitRestartedProxyHealthy->>probeProxy: Run final deadline probe
Loading

Possibly related PRs

  • lidge-jun/opencodex#515: Updates the related GUI restart and health-probing path in src/update/job.ts with corresponding test coverage.

Suggested labels: bug

Suggested reviewers: wibias, lidge-jun, ingwannu

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The code change matches #720 by extending the readiness window, adding a final probe, and covering the false-failure regression.
Out of Scope Changes check ✅ Passed The diff stays focused on update-job readiness logic and targeted tests, with no obvious unrelated changes.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: the update job now tolerates proxy readiness that arrives late.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@github-actions github-actions Bot added the bug Something isn't working label Jul 30, 2026
@luvs01
luvs01 marked this pull request as ready for review July 30, 2026 06:50
@lidge-jun
lidge-jun merged commit 62e9376 into lidge-jun:dev Jul 30, 2026
9 checks passed
@lidge-jun

Copy link
Copy Markdown
Owner

Merged into dev as 62e937614 (head 11069f055).

Verified locally on the merged tree:

  • The loop change is the substantive part, not the 15s→30s constant. awaitRestartedProxyHealthy now always makes one final identity-aware probe at the deadline instead of exiting the while (now() < startDeadline) test first, and the sleep is clamped to the remaining time so it cannot overshoot past the boundary. A replacement that becomes healthy on the last tick is no longer misreported as a timeout.
  • Falsification: 34 pass / 0 fail on the merged tree. Reverting only src/update/job.ts to the pre-merge cf30878fc gives 32 pass / 2 fail; restoring it returns 34 pass / 0 fail. The 15.25s arrival case in tests/update-job.test.ts is doing real work.
  • Full suite on the merged tree: 6080 pass / 0 fail, 6081 tests across 438 files.

The honest limit, which your own doc comment already records: a replacement arriving later than 30s is still reported as a restart failure. This narrows the false-positive window rather than removing it, and the worst case now takes 30s arrival plus a 15s stability window before success is declared. That tradeoff is the right one for an update path where a wrong "restart failed" costs the user more than a slower confirmation.

Closing as merged. Issue #720 is closed with the same evidence.

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.

2 participants