Repository navigation
fix(clusters): Restart Cluster reports a failed cluster load instead of staying disabled - #1867
Merged
dawsontoth merged 3 commits intoOct 12, 2026
Conversation
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a new helper function getClusterRestartTargets to centralize the logic of retrieving and preparing running instances for cluster restarts, which is then integrated into useRestartClusterClick and useRollingConfigUpdate to reduce duplication. It also adds cancellation checks after verifying instance responsiveness across several hooks. The review feedback suggests optimizing these cancellation checks by replacing continue with break inside the loops, as the canceled state is persistent and breaking early avoids unnecessary iterations.
This was referenced Oct 11, 2026
Draft
dawsontoth
added this pull request to stack #1908
October 11, 2026 21:27
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
…of staying disabled useRestartClusterClick awaited getClusterInfo, and built the per-instance clients, outside any try. When either failed, the click handler rejected unhandled: the Restart button stayed disabled until remount, the "Restarting" toast spun until its 60s duration ran out, and the rejection reached the console as an unhandled one. Mirror useRollingConfigUpdate's handling from #1861: load the restart targets inside a try, end the restart toast with "Could not load the cluster's instances, so nothing was restarted.", and clear the pending state in a finally. Both hooks built their targets the same way (fetch the cluster, check Fabric Connect, keep RUNNING instances, build a client each, reverse), so that now lives in one getClusterRestartTargets helper. Also stop reporting an empty cluster as "Cluster fully restarted!", the same zero-instance fix #1861 made to the config update. Closes #1853 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The restart, config-update and apply-licenses loops only looked at their Cancel flag at the top of each iteration. A Cancel clicked while an instance's availability check was in flight was therefore ignored for that instance: the restart, the config write, or the license install went out anyway. Recheck the flag once the check returns, before the first request that changes the instance. Also stop telling the user no instance was RUNNING when every RUNNING instance failed to restart. Both cluster hooks keyed that message on nothing having restarted; key it on there being no RUNNING target, so a cluster whose restarts all failed reads "Only 0 RUNNING instances were restarted of N". Found by the pre-push cross-model review of the previous commit; both predate it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s during the check Cancellation never unsets, so continuing only re-checked the flag on every remaining target. Refs #1853 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dawsontoth
marked this pull request as ready for review
October 12, 2026 21:19
dawsontoth
force-pushed
the
claude/1853-restart-cluster-load-failure
branch
from
October 12, 2026 21:19
1bbbc43 to
3afc3a9
Compare
dawsontoth
added a commit
that referenced
this pull request
Oct 13, 2026
…ay what Cancel left Review round 1 after the port (codex major, kept by the Harper domain adjudicator): the cluster loop waits on a member whose restart got no verdict, but when that watch also ran out it discarded the rejection and restarted the next member. On a two-member cluster, a 504 after the restart ran plus a member that stays down left no member serving, which is exactly what the stop rule exists to prevent. The loop now stops there too, and the report says Studio couldn't confirm that member came back either, instead of claiming it accepted the restart. The adjudicator's nit: a Cancel after a no-verdict member now yields the "Outcome unknown" report, which never said Cancel skipped the rest. Both loops now name the members Cancel left alone in that report. Also from the round: drop the test comments that only narrated their setup, and trim restartInstance's JSDoc to its contract (DESIGN.md carries the reasoning). Not taken: Cancel during set_configuration still restarts that member; that check is #1867's, unchanged here. The watch outliving the progress toasts' durations is not a problem: sonner never starts an auto-close timer for a loading toast (sonner 2.0.8, dist/index.mjs:644). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
⊙ Problem
Restart Cluster fetched the cluster and built a client for each instance before entering any
try. When that failed (a central manager 5xx or 403, a dropped connection, a malformed instance FQDN), the click handler rejected unhandled. The Restart button stayed disabled until the page remounted, the "Restarting" toast spun for its full 60s without an error, and the rejection reached the console as an unhandled one (#1853). #1861 fixed the same failure in the config editor's save.💡 Solution
getClusterRestartTargetshelper (src/hooks/getClusterRestartTargets.ts) fetches the cluster, checks Fabric Connect, keeps the RUNNING instances, builds one client each, and reverses the order. Restart Cluster and the config update used to build this list separately; both now call the helper. InuseRollingConfigUpdate.tsx,planRollingConfigUpdatekeeps only the replication FQDN it adds on top.useRestartClusterClick.tsx, the load runs inside atry. If it fails, the restart toast ends with "Could not load the cluster's instances, so nothing was restarted." The pending state is cleared in afinallythat covers every exit. This mirrors feat(config): fan cluster config saves out with replicated set_configuration on Harper 5.2+ #1861.Also fixed (found by the pre-push review; both bugs are older than this branch):
useApplyLicensesClick.tsx).⚖️ Alternatives
try/finally. That gives a smaller diff with less re-indentation, but it is easier to miss an exit path, and it would not match feat(config): fan cluster config saves out with replicated set_configuration on Harper 5.2+ #1861. The links here use?w=1, which hides the indentation noise.✅ Verification
useRestartClusterClick.test.tsxfail with the reported bug:onRestartClickrejects withError: 503(the cluster fetch) orTypeError: Invalid URL(client construction). The four review-fix tests also fail before their fix: Cancel during the check for restart, config update (useRollingConfigUpdate.test.tsx) and licenses (useApplyLicensesClick.test.tsx, a new harness), plus the all-failed message for restart and config update. All of them pass with the fix.getClusterRestartTargets.test.ts) pin the RUNNING filter, the reverse order, the Fabric Connect flag, a missing instance list, and that a failed fetch rejects before any client is built.finallythat clears the pending state.tsc -b,oxlintanddprint checkall exited 0.renderHook, with the real restart tracker, and assert that the cluster is never marked as restarting when the load fails. I did not verify in a browser.useRollingConfigUpdatealready has the empty-cluster success guard from feat(config): fan cluster config saves out with replicated set_configuration on Harper 5.2+ #1861. The Harper adjudicator could not run because its Claude OAuth session had expired, and the Cursor leg could not run because the 1Password SSH agent refused to sign.breakinstead ofcontinue, because the flag never resets and later iterations only re-checked it. Thefinallythat releases each instance's restart marker still runs, and all of the cancel-during-check tests above pass. The branch also merges feat(config): fan cluster config saves out with replicated set_configuration on Harper 5.2+ #1861's matching change to its own loop-top guard (3581be6). These are refactors with unchanged behavior, so no further review round ran.Closes #1853
🤖 Generated with Claude Code
🤖 Generated by Claude Code (Anthropic Claude); posted via @dawsontoth.
Related PRs: #1861 overlaps (this PR's base; it reuses #1861's cluster-load catch and replaces its target planning with the shared helper), #1899 overlaps (reworks the same restart and config-update loops in
useRestartClusterClick.tsxanduseRollingConfigUpdate.tsx)Complexity: medium
Review-Coverage: authored=claude; ran=codex,gemini; blocked=cursor-composer(no-receipt),domain(auth); declined=cursor-grok,cursor-kimi,cursor-muse; rounds=2; full=1 @ 1bbbc43
Review-Attention: read ~6m (raised: degraded review) @ 1bbbc43