From c3d86bd062a483b9e90aeb4198a8619b72b9a984 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Sun, 11 Oct 2026 02:10:34 -0400 Subject: [PATCH 1/5] feat(config): fan cluster config saves out with replicated set_configuration on Harper 5.2+ Saving the config editor on a cluster wrote set_configuration to every RUNNING instance from the browser, one at a time, before restarting it, and never wrote to members it did not restart. Harper 5.2.0 added `replicated: true` to set_configuration (harper#1556), which applies the write on every replication peer in one call. It does not apply the config: each node still needs its restart, so the one-at-a-time roll stays. On a cluster with more than one live member, all reporting 5.2.0 or later, the first instance is still written and restarted on its own. Once it is back up on the new config, the write fans out from it with `replicated: true` -- also when it was the last instance to restart, so STOPPED or failed members still get it. A later instance skips its own write only when the response acknowledged that exact node: a success entry carrying the peer's `message` string and a `node` equal to the instance's replicationFqdn (the name Fabric gives each Harper node). Pre-5.2, mixed-version and unknown-version clusters, per-instance views, a rejected fan-out, and any unacknowledged target keep the per-instance write. After a fan-out, the Cancel and partial-failure toasts say instances that were not restarted may already have the new config saved. Also: a failure loading the cluster's instances left Save disabled and the loading toast spinning, with an unhandled rejection; it now reports that nothing changed and clears the pending state. A cluster with no instances reported Success; it now reports that none were found. Closes #1339 Co-Authored-By: Claude Opus 5.5 --- DESIGN.md | 8 + src/hooks/useRollingConfigUpdate.test.tsx | 457 ++++++++++++++++++ src/hooks/useRollingConfigUpdate.tsx | 278 +++++++---- .../instance/status/setConfiguration.test.ts | 59 +++ .../api/instance/status/setConfiguration.ts | 24 + src/integrations/api/replication.test.ts | 48 +- src/integrations/api/replication.ts | 24 +- 7 files changed, 794 insertions(+), 104 deletions(-) create mode 100644 src/hooks/useRollingConfigUpdate.test.tsx create mode 100644 src/integrations/api/instance/status/setConfiguration.test.ts diff --git a/DESIGN.md b/DESIGN.md index b20775d52..e7e13ac77 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -44,6 +44,14 @@ The wide hero artwork spans both columns beneath the form card in three independ - **Only query errors are muted, and only connectivity-class ones.** Mutations still toast; a 403 or a Harper 500 during a restart still toasts. - **Detection is per tab.** A page learns of a container op started elsewhere only from a cluster-record fetch, and the in-cluster studio pages don't poll the cluster, so they don't notice one started in another tab. +## Cluster config updates — when an instance may skip its own `set_configuration` + +[`useRollingConfigUpdate`](src/hooks/useRollingConfigUpdate.tsx) saves the config editor's change by restarting a cluster's RUNNING instances one at a time, and every instance it restarts must already have the new config on disk: `set_configuration` only writes the file, and nothing applies it until a restart. + +- **Pre-5.2, mixed-version, or unknown-version clusters write each instance before restarting it.** `replicated: true` on `set_configuration` exists from Harper 5.2.0 ([`shouldFanOutSetConfiguration`](src/integrations/api/instance/status/setConfiguration.ts)); 5.0 and 5.1 apply the change locally and report no peers. +- **On 5.2+ the first instance is still written and restarted alone.** Only once it is back up on the new config is the write fanned out with `replicated: true` from that instance — even when it was the last instance to restart, because the fan-out also reaches live members Studio does not restart (not RUNNING, or failed earlier), which the per-instance path leaves on the old config. Fanning out first would leave an unproven config pending on every node, which a Cancel could no longer contain. +- **A later instance skips its write only when the fan-out acknowledged that exact node** — its CM `replicationFqdn` (lower-cased) appears among the successful `replicated[]` entries. Fabric names each Harper node after that FQDN (host-manager sets `NODE_HOSTNAME` and `REPLICATION_HOSTNAME` to it). A count of acknowledgements is not proof: the origin's replication peers are not CM's restart targets, so a stale peer can stand in for a missing one. A target with no `replicationFqdn`, a rejected fan-out (the instance client rejects any response with a failed peer), or a response with no usable `replicated[]` all keep the per-instance write. + ## Cluster upgrade picker — which versions an existing cluster is offered [`buildUpgradeVersionOptions`](src/features/clusters/upsert/lib/buildUpgradeVersionOptions.ts) is the one place that decides, and its test file is the spec. Two rules, and they differ on purpose: diff --git a/src/hooks/useRollingConfigUpdate.test.tsx b/src/hooks/useRollingConfigUpdate.test.tsx new file mode 100644 index 000000000..1a70ba91b --- /dev/null +++ b/src/hooks/useRollingConfigUpdate.test.tsx @@ -0,0 +1,457 @@ +/** + * @vitest-environment jsdom + */ +import type { Cluster, Instance } from '@/integrations/api/api.patch'; +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import { act, renderHook } from '@testing-library/react'; +import type { AxiosInstance } from 'axios'; +import { PropsWithChildren } from 'react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +interface FakeClient { + target: string; +} + +const { operationsParams } = vi.hoisted(() => ({ + operationsParams: { + current: { + entityType: 'cluster' as 'cluster' | 'instance', + entityId: 'clu-1', + instanceClient: { target: 'cluster-load-balancer' } as unknown, + }, + }, +})); +vi.mock('@/config/useInstanceClient', () => ({ + useInstanceClientIdParams: () => operationsParams.current, +})); +vi.mock('@/config/getInstanceClient', () => ({ + getInstanceClient: ({ id }: { id: string }) => ({ target: id }), +})); +vi.mock('@/features/auth/store/authStore', () => ({ + authStore: { checkForFabricConnect: () => false }, +})); +vi.mock('@/features/cluster/queries/getClusterInfoQuery', () => ({ getClusterInfo: vi.fn() })); +vi.mock('@/integrations/api/instance/status/getInstanceUserInfo', () => ({ getInstanceUserInfo: vi.fn() })); +vi.mock('@/integrations/api/instance/status/restartInstance', () => ({ restartInstance: vi.fn() })); +vi.mock('@/integrations/api/instance/status/setConfiguration', async (importOriginal) => ({ + ...await importOriginal(), + setConfiguration: vi.fn(), +})); +vi.mock('@/lib/sleep', () => ({ sleep: vi.fn(() => Promise.resolve()) })); +vi.mock('sonner', () => ({ + toast: { loading: vi.fn(() => 'toast-1'), error: vi.fn(), success: vi.fn(), dismiss: vi.fn() }, +})); + +import { getClusterInfo } from '@/features/cluster/queries/getClusterInfoQuery'; +import { getInstanceUserInfo } from '@/integrations/api/instance/status/getInstanceUserInfo'; +import { restartInstance } from '@/integrations/api/instance/status/restartInstance'; +import { setConfiguration } from '@/integrations/api/instance/status/setConfiguration'; +import { toast } from 'sonner'; +import { useRollingConfigUpdate } from './useRollingConfigUpdate'; + +const CHANGES = { logging_level: 'debug' }; + +function instance(id: string, version: string | undefined, overrides: Partial = {}): Instance { + return { + id, + version, + status: 'RUNNING', + instanceFqdn: `${id}.example.com`, + replicationFqdn: `${id}-repl.example.com`, + operationsApiPort: 9925, + operationsApiSecure: true, + ...overrides, + } as Instance; +} + +function useCluster(instances: Instance[]) { + vi.mocked(getClusterInfo).mockResolvedValue({ id: 'clu-1', instances } as Cluster); +} + +let events: string[] = []; + +function targetOf(client: AxiosInstance): string { + return (client as unknown as FakeClient).target; +} + +const CONFIGURED = 'Configuration successfully set. You must restart Harper for new config settings to take effect.'; + +/** The shape harper-pro's `replicateOperation` returns: each peer's own answer with its node name attached. */ +function acknowledging(...nodes: string[]) { + return { + message: CONFIGURED, + replicated: nodes.map((node, requestId) => ({ message: CONFIGURED, requestId, node })), + }; +} + +function wrapper() { + const queryClient = new QueryClient(); + return ({ children }: PropsWithChildren) => {children} + ; +} + +async function save() { + const { result } = renderHook(() => useRollingConfigUpdate(), { wrapper: wrapper() }); + await act(async () => { + await (result.current.onConfigUpdate(CHANGES) as unknown as Promise); + }); + return result; +} + +beforeEach(() => { + events = []; + operationsParams.current = { + entityType: 'cluster', + entityId: 'clu-1', + instanceClient: { target: 'cluster-load-balancer' }, + }; + vi.mocked(getInstanceUserInfo).mockImplementation(async ({ instanceClient }) => { + events.push(`check ${targetOf(instanceClient)}`); + return {} as never; + }); + vi.mocked(setConfiguration).mockImplementation(async ({ instanceClient, ...changes }) => { + events.push(`${changes.replicated ? 'replicate' : 'write'} ${targetOf(instanceClient)}`); + return { message: 'ok' }; + }); + vi.mocked(restartInstance).mockImplementation(async ({ instanceClient }) => { + events.push(`restart ${targetOf(instanceClient)}`); + return { message: 'ok' }; + }); +}); + +afterEach(() => vi.clearAllMocks()); + +describe('useRollingConfigUpdate on a cluster that replicates set_configuration (5.2+)', () => { + it('writes and restarts one instance, fans the write out from it, then only restarts the acknowledged rest', async () => { + useCluster([instance('ins-a', '5.2.13'), instance('ins-b', '5.3.0'), instance('ins-c', '5.2.0')]); + vi.mocked(setConfiguration).mockImplementation(async ({ instanceClient, ...changes }) => { + events.push(`${changes.replicated ? 'replicate' : 'write'} ${targetOf(instanceClient)}`); + // Node names are compared case-insensitively. + return changes.replicated ? acknowledging('INS-B-REPL.example.com', 'ins-a-repl.example.com') : { message: 'ok' }; + }); + + await save(); + + expect(events).toEqual([ + 'check ins-c', + 'write ins-c', + 'restart ins-c', + 'replicate ins-c', + 'check ins-b', + 'restart ins-b', + 'check ins-a', + 'restart ins-a', + ]); + expect(setConfiguration).toHaveBeenCalledWith(expect.objectContaining({ ...CHANGES, replicated: true })); + expect(toast.success).toHaveBeenCalled(); + }); + + it('never starts the next restart before the previous one has finished', async () => { + useCluster([instance('ins-a', '5.2.0'), instance('ins-b', '5.2.0'), instance('ins-c', '5.2.0')]); + vi.mocked(setConfiguration).mockImplementation(async ({ instanceClient, ...changes }) => { + events.push(`${changes.replicated ? 'replicate' : 'write'} ${targetOf(instanceClient)}`); + return changes.replicated ? acknowledging('ins-a-repl.example.com', 'ins-b-repl.example.com') : { message: 'ok' }; + }); + let inFlight = 0; + let maxInFlight = 0; + const pending: Array<() => void> = []; + vi.mocked(restartInstance).mockImplementation(({ instanceClient }) => { + events.push(`restart ${targetOf(instanceClient)}`); + inFlight += 1; + maxInFlight = Math.max(maxInFlight, inFlight); + return new Promise(resolve => + pending.push(() => { + inFlight -= 1; + resolve({ message: 'ok' }); + }) + ); + }); + const { result } = renderHook(() => useRollingConfigUpdate(), { wrapper: wrapper() }); + + let done: Promise | undefined; + act(() => { + done = result.current.onConfigUpdate(CHANGES) as unknown as Promise; + }); + for (const expected of ['ins-c', 'ins-b', 'ins-a']) { + await vi.waitFor(() => expect(pending).toHaveLength(1)); + expect(events.at(-1)).toBe(`restart ${expected}`); + await act(async () => pending.shift()!()); + } + await act(async () => done); + + expect(maxInFlight).toBe(1); + expect(toast.success).toHaveBeenCalled(); + }); + + it('keeps the local write for a target replication did not acknowledge, whatever the count', async () => { + useCluster([instance('ins-a', '5.2.0'), instance('ins-b', '5.2.0'), instance('ins-c', '5.2.0')]); + vi.mocked(setConfiguration).mockImplementation(async ({ instanceClient, ...changes }) => { + events.push(`${changes.replicated ? 'replicate' : 'write'} ${targetOf(instanceClient)}`); + // Two acknowledgements for two remaining targets — but one is a peer Studio is not restarting. + return changes.replicated ? acknowledging('ins-b-repl.example.com', 'stale-peer.example.com') : { message: 'ok' }; + }); + + await save(); + + expect(events).toEqual([ + 'check ins-c', + 'write ins-c', + 'restart ins-c', + 'replicate ins-c', + 'check ins-b', + 'restart ins-b', + 'check ins-a', + 'write ins-a', + 'restart ins-a', + ]); + }); + + it('keeps the local write for a target whose replication name is unknown', async () => { + useCluster([ + instance('ins-a', '5.2.0', { replicationFqdn: undefined }), + instance('ins-b', '5.2.0'), + ]); + vi.mocked(setConfiguration).mockImplementation(async ({ instanceClient, ...changes }) => { + events.push(`${changes.replicated ? 'replicate' : 'write'} ${targetOf(instanceClient)}`); + return changes.replicated ? acknowledging('ins-a-repl.example.com') : { message: 'ok' }; + }); + + await save(); + + expect(events).toEqual([ + 'check ins-b', + 'write ins-b', + 'restart ins-b', + 'replicate ins-b', + 'check ins-a', + 'write ins-a', + 'restart ins-a', + ]); + }); + + it('falls back to writing every remaining instance when the fan-out is rejected, still counting the canary', async () => { + useCluster([instance('ins-a', '5.2.0'), instance('ins-b', '5.2.0'), instance('ins-c', '5.2.0')]); + vi.mocked(setConfiguration).mockImplementation(async ({ instanceClient, ...changes }) => { + events.push(`${changes.replicated ? 'replicate' : 'write'} ${targetOf(instanceClient)}`); + if (changes.replicated) { + // What `rejectReplicationFailures` turns a partial fan-out into. + throw 'The operation partially succeeded, but 1 node failed:\nins-a-repl.example.com: timeout'; + } + return { message: 'ok' }; + }); + + await save(); + + expect(events).toEqual([ + 'check ins-c', + 'write ins-c', + 'restart ins-c', + 'replicate ins-c', + 'check ins-b', + 'write ins-b', + 'restart ins-b', + 'check ins-a', + 'write ins-a', + 'restart ins-a', + ]); + expect(toast.success).toHaveBeenCalled(); + expect(toast.error).not.toHaveBeenCalled(); + expect(toast.loading).not.toHaveBeenCalledWith(expect.stringContaining('Failed'), expect.anything()); + }); + + it('falls back to writing every remaining instance when the response names no peers', async () => { + useCluster([instance('ins-a', '5.2.0'), instance('ins-b', '5.2.0')]); + + await save(); + + expect(events).toEqual([ + 'check ins-b', + 'write ins-b', + 'restart ins-b', + 'replicate ins-b', + 'check ins-a', + 'write ins-a', + 'restart ins-a', + ]); + }); + + it('waits to fan out until an instance has actually come back on the new configuration', async () => { + useCluster([instance('ins-a', '5.2.0'), instance('ins-b', '5.2.0'), instance('ins-c', '5.2.0')]); + vi.mocked(restartInstance).mockImplementation(async ({ instanceClient }) => { + events.push(`restart ${targetOf(instanceClient)}`); + if (targetOf(instanceClient) === 'ins-c') { + throw new Error('did not come back'); + } + return { message: 'ok' }; + }); + vi.mocked(setConfiguration).mockImplementation(async ({ instanceClient, ...changes }) => { + events.push(`${changes.replicated ? 'replicate' : 'write'} ${targetOf(instanceClient)}`); + return changes.replicated ? acknowledging('ins-a-repl.example.com', 'ins-c-repl.example.com') : { message: 'ok' }; + }); + + await save(); + + expect(events).toEqual([ + 'check ins-c', + 'write ins-c', + 'restart ins-c', + 'check ins-b', + 'write ins-b', + 'restart ins-b', + 'replicate ins-b', + 'check ins-a', + 'restart ins-a', + ]); + expect(toast.error).toHaveBeenCalledWith( + 'Error', + expect.objectContaining({ description: expect.stringContaining('may already have the new configuration saved') }), + ); + }); + + it('fans out after the last restart too, so live members Studio does not restart still get the write', async () => { + useCluster([instance('ins-a', '5.2.0'), instance('ins-b', '5.2.0', { status: 'STOPPED' })]); + + await save(); + + expect(events).toEqual(['check ins-a', 'write ins-a', 'restart ins-a', 'replicate ins-a']); + expect(toast.error).toHaveBeenCalledWith( + 'Error', + expect.objectContaining({ description: expect.stringContaining('may already have the new configuration saved') }), + ); + }); + + it('does not fan out on a cluster with no other live member', async () => { + useCluster([instance('ins-a', '5.2.0'), instance('ins-b', '5.2.0', { status: 'TERMINATED' })]); + + await save(); + + expect(events).toEqual(['check ins-a', 'write ins-a', 'restart ins-a']); + }); + + it('says configuration may be pending when cancelled after a fan-out', async () => { + useCluster([instance('ins-a', '5.2.0'), instance('ins-b', '5.2.0'), instance('ins-c', '5.2.0')]); + vi.mocked(setConfiguration).mockImplementation(async ({ instanceClient, ...changes }) => { + events.push(`${changes.replicated ? 'replicate' : 'write'} ${targetOf(instanceClient)}`); + if (changes.replicated) { + cancel(); + return acknowledging('ins-a-repl.example.com', 'ins-b-repl.example.com'); + } + return { message: 'ok' }; + }); + + await save(); + + expect(events).toEqual(['check ins-c', 'write ins-c', 'restart ins-c', 'replicate ins-c']); + expect(toast.error).toHaveBeenCalledWith( + 'Cancelled', + expect.objectContaining({ description: expect.stringContaining('may already have the new configuration saved') }), + ); + }); + + it('does not fan out once cancelled, and keeps the plain cancellation copy', async () => { + useCluster([instance('ins-a', '5.2.0'), instance('ins-b', '5.2.0')]); + vi.mocked(restartInstance).mockImplementation(async ({ instanceClient }) => { + events.push(`restart ${targetOf(instanceClient)}`); + cancel(); + return { message: 'ok' }; + }); + + await save(); + + expect(events).toEqual(['check ins-b', 'write ins-b', 'restart ins-b']); + expect(toast.error).toHaveBeenCalledWith( + 'Cancelled', + expect.objectContaining({ description: 'The config update was partially cancelled.' }), + ); + }); +}); + +describe('useRollingConfigUpdate on clusters that must not replicate', () => { + it.each([ + ['a mixed-version cluster', ['5.2.13', '5.1.20', '5.2.13']], + ['an instance with no reported version', ['5.2.13', undefined, '5.2.13']], + ['a 5.2.0 prerelease', ['5.2.0-beta.1', '5.2.0', '5.2.0']], + ['a pre-5.2 cluster', ['5.1.29', '5.1.29', '5.1.29']], + ])('writes every instance locally on %s', async (_label, versions) => { + useCluster(versions.map((version, index) => instance(`ins-${index}`, version))); + + await save(); + + expect(vi.mocked(setConfiguration).mock.calls.some(([body]) => 'replicated' in body)).toBe(false); + expect(events).toEqual([ + 'check ins-2', + 'write ins-2', + 'restart ins-2', + 'check ins-1', + 'write ins-1', + 'restart ins-1', + 'check ins-0', + 'write ins-0', + 'restart ins-0', + ]); + }); + + it('writes the single instance a per-instance view targets', async () => { + operationsParams.current = { + entityType: 'instance', + entityId: 'ins-a', + instanceClient: { target: 'ins-a-direct' }, + }; + + await save(); + + expect(getClusterInfo).not.toHaveBeenCalled(); + expect(events).toEqual(['check ins-a-direct', 'write ins-a-direct', 'restart ins-a-direct']); + }); +}); + +describe('useRollingConfigUpdate outcome reporting', () => { + it('reports a cluster with no instances as a failure, not a success', async () => { + useCluster([]); + + await save(); + + expect(toast.success).not.toHaveBeenCalled(); + expect(toast.error).toHaveBeenCalledWith( + 'Error', + expect.objectContaining({ description: expect.stringContaining('No instances were found within the cluster') }), + ); + }); + + it('does not mention pending configuration when nothing was fanned out', async () => { + useCluster([instance('ins-a', '5.1.29'), instance('ins-b', '5.1.29')]); + vi.mocked(restartInstance).mockImplementation(async ({ instanceClient }) => { + events.push(`restart ${targetOf(instanceClient)}`); + if (targetOf(instanceClient) === 'ins-b') { + throw new Error('did not come back'); + } + return { message: 'ok' }; + }); + + await save(); + + const [, options] = vi.mocked(toast.error).mock.calls.at(-1)!; + expect(options?.description).toContain('Only 1 "RUNNING" instance was restarted of 2 instances.'); + expect(options?.description).not.toContain('may already have the new configuration saved'); + }); +}); + +describe('useRollingConfigUpdate when the cluster cannot be loaded', () => { + it('reports it, changes nothing, and stops pending', async () => { + vi.mocked(getClusterInfo).mockRejectedValue(new Error('503')); + + const result = await save(); + + expect(events).toEqual([]); + expect(result.current.isPending).toBe(false); + expect(toast.error).toHaveBeenCalledWith( + 'Error', + expect.objectContaining({ description: expect.stringContaining('no configuration was changed') }), + ); + }); +}); + +function cancel() { + const [, options] = vi.mocked(toast.loading).mock.calls.at(-1)!; + (options as unknown as { action: { onClick: () => void } }).action.onClick(); +} diff --git a/src/hooks/useRollingConfigUpdate.tsx b/src/hooks/useRollingConfigUpdate.tsx index 3d1ccee48..129b6490e 100644 --- a/src/hooks/useRollingConfigUpdate.tsx +++ b/src/hooks/useRollingConfigUpdate.tsx @@ -1,17 +1,20 @@ import { ProgressBar } from '@/components/ProgressBar'; import { getInstanceClient } from '@/config/getInstanceClient'; +import { InstanceClientIdConfig, InstanceTypeConfig } from '@/config/instanceClientConfig'; import { useInstanceClientIdParams } from '@/config/useInstanceClient'; import { authStore } from '@/features/auth/store/authStore'; import { getClusterInfo } from '@/features/cluster/queries/getClusterInfoQuery'; import { Instance } from '@/integrations/api/api.patch'; import { getInstanceUserInfo } from '@/integrations/api/instance/status/getInstanceUserInfo'; import { restartInstance } from '@/integrations/api/instance/status/restartInstance'; -import { setConfiguration } from '@/integrations/api/instance/status/setConfiguration'; +import { setConfiguration, shouldFanOutSetConfiguration } from '@/integrations/api/instance/status/setConfiguration'; +import { acknowledgedReplicationNodes } from '@/integrations/api/replication'; import { excludeFalsy } from '@/lib/arrays/excludeFalsy'; import { pluralize } from '@/lib/pluralize'; import { sleep } from '@/lib/sleep'; import { getOperationsUrlForInstance } from '@/lib/urls/getOperationsUrlForInstance'; import { useQueryClient } from '@tanstack/react-query'; +import type { AxiosInstance } from 'axios'; import { useCallback, useState } from 'react'; import { toast } from 'sonner'; @@ -24,6 +27,51 @@ interface RollingConfigUpdateResponse { isPending: boolean; } +const MAY_BE_PENDING = + 'Instances that were not restarted may already have the new configuration saved, and will apply it the next time they restart.'; + +interface RestartTarget { + instanceClient: AxiosInstance; + replicationFqdn?: string; +} + +interface RollingConfigUpdatePlan { + allInstances: Instance[]; + targets: RestartTarget[]; + fanOutAfterCanary: boolean; +} + +async function planRollingConfigUpdate( + operationsParams: InstanceClientIdConfig & InstanceTypeConfig, +): Promise { + if (operationsParams.entityType !== 'cluster') { + return { + allInstances: [{ id: operationsParams.entityId, status: 'RUNNING' } as Instance], + targets: [{ instanceClient: operationsParams.instanceClient }], + fanOutAfterCanary: false, + }; + } + const cluster = await getClusterInfo(operationsParams.entityId); + const allInstances = cluster?.instances || []; + const clusterUsesFabricConnect = !!cluster && authStore.checkForFabricConnect(cluster.id); + const targets = allInstances + .filter(instance => instance.status === 'RUNNING') + .map(instance => ({ + instanceClient: getInstanceClient({ + id: instance.id, + forceFabricConnect: clusterUsesFabricConnect, + operationsUrl: getOperationsUrlForInstance(instance), + }), + replicationFqdn: instance.replicationFqdn, + })) + .reverse(); + return { + allInstances, + targets, + fanOutAfterCanary: shouldFanOutSetConfiguration(allInstances), + }; +} + export function useRollingConfigUpdate( { onRestartedSuccessfully }: RollingConfigUpdateParams = {}, ): RollingConfigUpdateResponse { @@ -54,119 +102,145 @@ export function useRollingConfigUpdate( ), }); - const cluster = operationsParams.entityType === 'cluster' - ? await getClusterInfo(operationsParams.entityId) - : null; - const allInstances = operationsParams.entityType === 'cluster' - ? cluster?.instances || [] - : [{ id: operationsParams.entityId, status: 'RUNNING' } as Instance]; - const clusterUsesFabricConnect = !!cluster && authStore.checkForFabricConnect(cluster.id); - const instanceClients = operationsParams.entityType === 'cluster' - ? allInstances - .filter(instance => instance.status === 'RUNNING') - .map(instance => - getInstanceClient({ - id: instance.id, - forceFabricConnect: clusterUsesFabricConnect, - operationsUrl: getOperationsUrlForInstance(instance), - }) - ) - .reverse() - : [operationsParams.instanceClient]; - let instancesRestarted = 0; + try { + let plan: RollingConfigUpdatePlan; + try { + plan = await planRollingConfigUpdate(operationsParams); + } catch { + toast.error('Error', { + id: toastId, + description: "Could not load the cluster's instances, so no configuration was changed.", + duration: 10_000, + action: { + label: 'Dismiss', + onClick: () => toast.dismiss(), + }, + }); + return; + } + const { allInstances, targets, fanOutAfterCanary } = plan; + let fanOutAttempted = false; + let acknowledged = new Set(); + let instancesRestarted = 0; - if (instanceClients.length) { - for (let i = 0; i < instanceClients.length; i++) { - const instanceClient = instanceClients[i]; - if (!canceled) { - toast.loading(`Updating Instance ${i + 1} of ${instanceClients.length}`, { - ...toastConfig, - id: toastId, - description: ( - - ), + for (let i = 0; i < targets.length; i++) { + const { instanceClient, replicationFqdn } = targets[i]; + if (canceled) { + continue; + } + toast.loading(`Updating Instance ${i + 1} of ${targets.length}`, { + ...toastConfig, + id: toastId, + description: ( + + ), + }); + let restarted = false; + try { + // Make sure the instance is responding. + await getInstanceUserInfo({ + instanceClient, }); - try { - // Make sure the instance is responding. - await getInstanceUserInfo({ - instanceClient, - }); + if (!replicationFqdn || !acknowledged.has(replicationFqdn.toLowerCase())) { await setConfiguration({ ...params, instanceClient, }); - // Then restart it. - await restartInstance({ - operation: 'restart_service', - replicated: false, - instanceClient, + } + // Then restart it. + await restartInstance({ + operation: 'restart_service', + replicated: false, + instanceClient, + }); + instancesRestarted += 1; + restarted = true; + } catch { + if (i + 1 !== targets.length) { + // If it fails to restart, or wasn't available, warn for a bit then move on. + toast.loading(`Failed Restarting Instance ${i + 1} of ${targets.length}`, { + ...toastConfig, + id: toastId, + description: 'We will carry on momentarily.', }); - instancesRestarted += 1; - } catch { - if (i + 1 !== instanceClients.length) { - // If it fails to restart, or wasn't available, warn for a bit then move on. - toast.loading(`Failed Restarting Instance ${i + 1} of ${instanceClients.length}`, { - ...toastConfig, - id: toastId, - description: 'We will carry on momentarily.', - }); - await sleep(3000); - } + await sleep(3000); } } - } - } - setIsPending(false); - void queryClient.invalidateQueries({ queryKey: ['get_configuration', operationsParams.entityId] }); + // The configuration spreads only once an instance has come back up running it, and a + // later target skips its own write only if replication acknowledged that exact node. + if (restarted && fanOutAfterCanary && !fanOutAttempted && !canceled) { + fanOutAttempted = true; + toast.loading('Saving configuration to the cluster', { + ...toastConfig, + id: toastId, + description: ( + + ), + }); + acknowledged = await setConfiguration({ + ...params, + replicated: true, + instanceClient, + }).then(acknowledgedReplicationNodes, () => new Set()); + } + } - if (canceled) { - toast.error('Cancelled', { - id: toastId, - description: `The config update was partially cancelled.`, - duration: 10_000, - action: { - label: 'Dismiss', - onClick: () => toast.dismiss(), - }, - }); - } else if (allInstances.length === instancesRestarted) { - onRestartedSuccessfully?.(); - toast.success('Success', { - id: toastId, - description: `Your configuration has been updated successfully!`, - duration: 10_000, - action: { - label: 'Dismiss', - onClick: () => toast.dismiss(), - }, - }); - } else { - const allTheInstances = pluralize(allInstances.length, 'instance', 'instances'); - const someRunningInstancesWere = pluralize( - instancesRestarted, - '"RUNNING" instance was', - '"RUNNING" instances were', - ); - toast.error('Error', { - id: toastId, - description: `Failed to fully update cluster.\n` - + ([ - allInstances.length === 0 && 'No instances were found within the cluster to restart.', - instancesRestarted === 0 && `No instances were in a "RUNNING" state of ${allTheInstances}.`, - allInstances.length !== instancesRestarted - && `Only ${someRunningInstancesWere} restarted of ${allTheInstances}.`, - ].filter(excludeFalsy).shift() || ''), - duration: 10_000, - action: { - label: 'Dismiss', - onClick: () => toast.dismiss(), - }, - }); + if (canceled) { + toast.error('Cancelled', { + id: toastId, + description: `The config update was partially cancelled.${fanOutAttempted ? ` ${MAY_BE_PENDING}` : ''}`, + duration: 10_000, + action: { + label: 'Dismiss', + onClick: () => toast.dismiss(), + }, + }); + } else if (allInstances.length > 0 && allInstances.length === instancesRestarted) { + onRestartedSuccessfully?.(); + toast.success('Success', { + id: toastId, + description: `Your configuration has been updated successfully!`, + duration: 10_000, + action: { + label: 'Dismiss', + onClick: () => toast.dismiss(), + }, + }); + } else { + const allTheInstances = pluralize(allInstances.length, 'instance', 'instances'); + const someRunningInstancesWere = pluralize( + instancesRestarted, + '"RUNNING" instance was', + '"RUNNING" instances were', + ); + toast.error('Error', { + id: toastId, + description: `Failed to fully update cluster.\n` + + ([ + allInstances.length === 0 && 'No instances were found within the cluster to restart.', + instancesRestarted === 0 && `No instances were in a "RUNNING" state of ${allTheInstances}.`, + allInstances.length !== instancesRestarted + && `Only ${someRunningInstancesWere} restarted of ${allTheInstances}.`, + ].filter(excludeFalsy).shift() || '') + + (fanOutAttempted ? '\n' + MAY_BE_PENDING : ''), + duration: 10_000, + action: { + label: 'Dismiss', + onClick: () => toast.dismiss(), + }, + }); + } + } finally { + setIsPending(false); + void queryClient.invalidateQueries({ queryKey: ['get_configuration', operationsParams.entityId] }); } }, [operationsParams, onRestartedSuccessfully, queryClient]); diff --git a/src/integrations/api/instance/status/setConfiguration.test.ts b/src/integrations/api/instance/status/setConfiguration.test.ts new file mode 100644 index 000000000..7c7c13dd9 --- /dev/null +++ b/src/integrations/api/instance/status/setConfiguration.test.ts @@ -0,0 +1,59 @@ +import { describe, expect, it } from 'vitest'; +import { shouldFanOutSetConfiguration } from './setConfiguration'; + +describe('shouldFanOutSetConfiguration', () => { + it('is true when every live instance runs 5.2.0 or later', () => { + expect(shouldFanOutSetConfiguration([ + { status: 'RUNNING', version: '5.2.0' }, + { status: 'RUNNING', version: '5.3.1' }, + { status: 'STOPPED', version: '5.2.13' }, + ])).toBe(true); + }); + + it('includes exactly 5.2.0', () => { + expect(shouldFanOutSetConfiguration([ + { status: 'RUNNING', version: '5.2.0' }, + { status: 'RUNNING', version: '5.2.0' }, + ])).toBe(true); + }); + + it('is false when any live instance is older', () => { + expect(shouldFanOutSetConfiguration([ + { status: 'RUNNING', version: '5.2.0' }, + { status: 'RUNNING', version: '5.1.29' }, + ])).toBe(false); + }); + + it('is false for a 5.2.0 prerelease', () => { + expect(shouldFanOutSetConfiguration([ + { status: 'RUNNING', version: '5.2.0' }, + { status: 'RUNNING', version: '5.2.0-beta.4' }, + ])).toBe(false); + }); + + it('is false when any live instance reports no version', () => { + expect(shouldFanOutSetConfiguration([ + { status: 'RUNNING', version: '5.2.0' }, + { status: 'RUNNING', version: undefined }, + ])).toBe(false); + }); + + it('ignores instances that are leaving the cluster', () => { + expect(shouldFanOutSetConfiguration([ + { status: 'RUNNING', version: '5.2.0' }, + { status: 'STOPPED', version: '5.2.0' }, + { status: 'TERMINATING', version: '5.1.29' }, + { status: 'TERMINATED', version: undefined }, + { status: 'REMOVED', version: '4.7.36' }, + ])).toBe(true); + }); + + it('is false with nothing else live to reach', () => { + expect(shouldFanOutSetConfiguration([])).toBe(false); + expect(shouldFanOutSetConfiguration([{ status: 'RUNNING', version: '5.2.0' }])).toBe(false); + expect(shouldFanOutSetConfiguration([ + { status: 'RUNNING', version: '5.2.0' }, + { status: 'TERMINATED', version: '5.2.0' }, + ])).toBe(false); + }); +}); diff --git a/src/integrations/api/instance/status/setConfiguration.ts b/src/integrations/api/instance/status/setConfiguration.ts index 17f4b9530..bb8b55062 100644 --- a/src/integrations/api/instance/status/setConfiguration.ts +++ b/src/integrations/api/instance/status/setConfiguration.ts @@ -1,5 +1,29 @@ +import { deletedClusterStatuses } from '@/config/clusterStatuses'; import { InstanceClientConfig } from '@/config/instanceClientConfig'; +import { Instance } from '@/integrations/api/api.patch'; import { ReplicatedResponse } from '@/integrations/api/replication'; +import { wasAReleasedBeforeB } from '@/lib/string/wasAReleasedBeforeB'; + +/** + * The first Harper release whose `set_configuration` takes `replicated: true` and fans the write out + * to every replication peer (HarperFast/harper#1556). 5.0 and 5.1 apply the change locally and report + * no peers. Prereleases of 5.2.0 read as unsupported. + */ +const REPLICATED_SET_CONFIGURATION_MIN_VERSION = '5.2.0'; + +/** + * Whether a cluster-wide config save should fan its write out: some other live member exists to + * reach — including ones Studio will not restart — and every live member runs a Harper that can. + */ +export function shouldFanOutSetConfiguration( + instances: ReadonlyArray>, +): boolean { + const live = instances.filter(instance => !(instance.status && deletedClusterStatuses.includes(instance.status))); + return live.length > 1 + && live.every(instance => + !!instance.version && wasAReleasedBeforeB(REPLICATED_SET_CONFIGURATION_MIN_VERSION, instance.version) + ); +} interface SetConfigurationParams extends InstanceClientConfig { [key: string]: unknown; diff --git a/src/integrations/api/replication.test.ts b/src/integrations/api/replication.test.ts index b87ff81d1..4cc4f7bb6 100644 --- a/src/integrations/api/replication.test.ts +++ b/src/integrations/api/replication.test.ts @@ -1,4 +1,5 @@ import { + acknowledgedReplicationNodes, isReplicatedResponseFailure, isReplicatedResponseSuccess, rejectReplicationFailures, @@ -127,7 +128,52 @@ describe('rejectReplicationFailures', () => { }); }); -function success(node: string, requestId = 1, message = true): ReplicatedResponseSuccess { +describe('acknowledgedReplicationNodes', () => { + it('names every peer that applied the operation, lower-cased', () => { + expect( + acknowledgedReplicationNodes({ message: 'ok', replicated: [answered('Node-A.example.com'), answered('node-b')] }), + ) + .toEqual(new Set(['node-a.example.com', 'node-b'])); + }); + + it('leaves out failed peers', () => { + expect( + acknowledgedReplicationNodes({ message: 'ok', replicated: [answered('node-a'), failure('node-b', 'timeout')] }), + ) + .toEqual(new Set(['node-a'])); + }); + + it.each([ + ['no replicated array', { message: 'ok' }], + ['a non-array replicated', { message: 'ok', replicated: 'yes' }], + ['a bare string', 'Configuration successfully set.'], + ['undefined', undefined], + ['null', null], + [ + 'entries with no usable node name', + { message: 'ok', replicated: [null, 7, {}, { node: '', message: 'ok' }, { node: 42, message: 'ok' }] }, + ], + ['entries with no answer from the peer', { + message: 'ok', + replicated: [{ node: 'node-a' }, { node: 'node-b', requestId: 1 }], + }], + ['entries whose answer is not a message', { + message: 'ok', + replicated: [{ node: 'a', message: {} }, { node: 'b', message: 0 }, { node: 'c', message: true }, { + node: 'd', + message: '', + }], + }], + ])('reads %s as reaching no one', (_label, response) => { + expect(acknowledgedReplicationNodes(response)).toEqual(new Set()); + }); +}); + +function answered(node: string): ReplicatedResponseSuccess { + return success(node, 1, 'Configuration successfully set.'); +} + +function success(node: string, requestId = 1, message: boolean | string = true): ReplicatedResponseSuccess { return { node, requestId, message }; } diff --git a/src/integrations/api/replication.ts b/src/integrations/api/replication.ts index a37348309..bf56c7cfe 100644 --- a/src/integrations/api/replication.ts +++ b/src/integrations/api/replication.ts @@ -12,7 +12,7 @@ interface ReplicatedResponseNarrowed extends ReplicatedResponse { export interface ReplicatedResponseSuccess { node: string; - message: boolean; + message: boolean | string; requestId: number; } @@ -38,6 +38,28 @@ export function isReplicatedResponseFailure( return (message as ReplicatedResponseFailure).status === 'failed'; } +/** + * The lower-cased names of the peers a replicated operation reports as applied: entries carrying the + * peer's own answer (a `message` string) and the `node` Harper attached to it. Anything else is left + * out, so an unrecognized response reads as reaching no one. + */ +export function acknowledgedReplicationNodes(response: unknown): Set { + const acknowledged = new Set(); + const replicated: unknown = (response as ReplicatedResponse | null | undefined)?.replicated; + if (!Array.isArray(replicated)) { + return acknowledged; + } + for (const entry of replicated) { + if ( + entry && typeof entry === 'object' && isReplicatedResponseSuccess(entry) + && typeof entry.message === 'string' && entry.message && typeof entry.node === 'string' && entry.node + ) { + acknowledged.add(entry.node.toLowerCase()); + } + } + return acknowledged; +} + export function rejectReplicationFailures(response: Pick, 'data'>) { if (replicationFailed(response.data)) { const successes = response.data.replicated.filter(isReplicatedResponseSuccess); From 411e2929a695f186c07fe6c05e1c308743d90084 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Mon, 12 Oct 2026 12:06:08 -0400 Subject: [PATCH 2/5] refactor(config): stop the rolling update loop on cancel instead of skipping each target Cancellation never unsets, so every remaining iteration only re-checked the flag. Refs #1339 Co-Authored-By: Claude Opus 5.5 --- src/hooks/useRollingConfigUpdate.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/hooks/useRollingConfigUpdate.tsx b/src/hooks/useRollingConfigUpdate.tsx index 129b6490e..b3423d961 100644 --- a/src/hooks/useRollingConfigUpdate.tsx +++ b/src/hooks/useRollingConfigUpdate.tsx @@ -126,7 +126,7 @@ export function useRollingConfigUpdate( for (let i = 0; i < targets.length; i++) { const { instanceClient, replicationFqdn } = targets[i]; if (canceled) { - continue; + break; } toast.loading(`Updating Instance ${i + 1} of ${targets.length}`, { ...toastConfig, From 69ca8f960ec633708e5dfa1a47c4be13ca31b904 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Sun, 11 Oct 2026 02:20:34 -0400 Subject: [PATCH 3/5] fix(clusters): Restart Cluster reports a failed cluster load instead 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 --- src/hooks/getClusterRestartTargets.test.ts | 70 +++++++ src/hooks/getClusterRestartTargets.ts | 35 ++++ src/hooks/useRestartClusterClick.test.tsx | 138 +++++++++++++ src/hooks/useRestartClusterClick.tsx | 230 +++++++++++---------- src/hooks/useRollingConfigUpdate.tsx | 25 +-- 5 files changed, 366 insertions(+), 132 deletions(-) create mode 100644 src/hooks/getClusterRestartTargets.test.ts create mode 100644 src/hooks/getClusterRestartTargets.ts create mode 100644 src/hooks/useRestartClusterClick.test.tsx diff --git a/src/hooks/getClusterRestartTargets.test.ts b/src/hooks/getClusterRestartTargets.test.ts new file mode 100644 index 000000000..1e380def9 --- /dev/null +++ b/src/hooks/getClusterRestartTargets.test.ts @@ -0,0 +1,70 @@ +import type { Cluster, Instance } from '@/integrations/api/api.patch'; +import { afterEach, describe, expect, it, vi } from 'vitest'; + +const { fabricConnect } = vi.hoisted(() => ({ fabricConnect: { current: false } })); +vi.mock('@/config/getInstanceClient', () => ({ + getInstanceClient: vi.fn((config: unknown) => ({ config })), +})); +vi.mock('@/features/auth/store/authStore', () => ({ + authStore: { checkForFabricConnect: vi.fn(() => fabricConnect.current) }, +})); +vi.mock('@/features/cluster/queries/getClusterInfoQuery', () => ({ getClusterInfo: vi.fn() })); + +import { getInstanceClient } from '@/config/getInstanceClient'; +import { authStore } from '@/features/auth/store/authStore'; +import { getClusterInfo } from '@/features/cluster/queries/getClusterInfoQuery'; +import { getClusterRestartTargets } from './getClusterRestartTargets'; + +function instance(id: string, status: Instance['status'] = 'RUNNING'): Instance { + return { + id, + status, + instanceFqdn: `${id}.example.com`, + operationsApiPort: 9925, + operationsApiSecure: true, + } as Instance; +} + +afterEach(() => { + fabricConnect.current = false; + vi.clearAllMocks(); +}); + +describe('getClusterRestartTargets', () => { + it('builds a client for each RUNNING instance, last instance first, and keeps every instance', async () => { + const instances = [instance('ins-a'), instance('ins-b', 'STOPPED'), instance('ins-c')]; + vi.mocked(getClusterInfo).mockResolvedValue({ id: 'clu-1', instances } as Cluster); + + const { allInstances, targets } = await getClusterRestartTargets('clu-1'); + + expect(getClusterInfo).toHaveBeenCalledWith('clu-1'); + expect(allInstances).toBe(instances); + expect(targets.map(({ instance }) => instance.id)).toEqual(['ins-c', 'ins-a']); + expect(targets[0].instanceClient).toEqual({ + config: { id: 'ins-c', forceFabricConnect: false, operationsUrl: 'https://ins-c.example.com:9925/' }, + }); + }); + + it("forwards the cluster's Fabric Connect mode to every client", async () => { + fabricConnect.current = true; + vi.mocked(getClusterInfo).mockResolvedValue({ id: 'clu-1', instances: [instance('ins-a')] } as Cluster); + + await getClusterRestartTargets('clu-1'); + + expect(authStore.checkForFabricConnect).toHaveBeenCalledWith('clu-1'); + expect(getInstanceClient).toHaveBeenCalledWith(expect.objectContaining({ forceFabricConnect: true })); + }); + + it('treats a cluster without an instance list as empty', async () => { + vi.mocked(getClusterInfo).mockResolvedValue({ id: 'clu-1' } as Cluster); + + expect(await getClusterRestartTargets('clu-1')).toEqual({ allInstances: [], targets: [] }); + }); + + it('rejects when the cluster cannot be fetched, so callers can report it', async () => { + vi.mocked(getClusterInfo).mockRejectedValue(new Error('503')); + + await expect(getClusterRestartTargets('clu-1')).rejects.toThrow('503'); + expect(getInstanceClient).not.toHaveBeenCalled(); + }); +}); diff --git a/src/hooks/getClusterRestartTargets.ts b/src/hooks/getClusterRestartTargets.ts new file mode 100644 index 000000000..f5f430941 --- /dev/null +++ b/src/hooks/getClusterRestartTargets.ts @@ -0,0 +1,35 @@ +import { getInstanceClient } from '@/config/getInstanceClient'; +import { authStore } from '@/features/auth/store/authStore'; +import { getClusterInfo } from '@/features/cluster/queries/getClusterInfoQuery'; +import { Instance } from '@/integrations/api/api.patch'; +import { getOperationsUrlForInstance } from '@/lib/urls/getOperationsUrlForInstance'; +import type { AxiosInstance } from 'axios'; + +export interface ClusterRestartTarget { + instance: Instance; + instanceClient: AxiosInstance; +} + +export interface ClusterRestartTargets { + allInstances: Instance[]; + /** A client for each RUNNING instance, in the order a rolling restart visits them. */ + targets: ClusterRestartTarget[]; +} + +export async function getClusterRestartTargets(clusterId: string): Promise { + const cluster = await getClusterInfo(clusterId); + const allInstances = cluster?.instances ?? []; + const clusterUsesFabricConnect = !!cluster && authStore.checkForFabricConnect(cluster.id); + const targets = allInstances + .filter(instance => instance.status === 'RUNNING') + .map(instance => ({ + instance, + instanceClient: getInstanceClient({ + id: instance.id, + forceFabricConnect: clusterUsesFabricConnect, + operationsUrl: getOperationsUrlForInstance(instance), + }), + })) + .reverse(); + return { allInstances, targets }; +} diff --git a/src/hooks/useRestartClusterClick.test.tsx b/src/hooks/useRestartClusterClick.test.tsx new file mode 100644 index 000000000..edffce2a0 --- /dev/null +++ b/src/hooks/useRestartClusterClick.test.tsx @@ -0,0 +1,138 @@ +/** + * @vitest-environment jsdom + */ +import type { Cluster, Instance } from '@/integrations/api/api.patch'; +import { getRestartState } from '@/lib/restart/restartTracker'; +import { act, renderHook } from '@testing-library/react'; +import type { AxiosInstance } from 'axios'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +vi.mock('@tanstack/react-router', () => ({ + useParams: () => ({ clusterId: 'clu-1' }), +})); +vi.mock('@/config/getInstanceClient', () => ({ + getInstanceClient: vi.fn(({ id }: { id: string }) => ({ target: id })), +})); +vi.mock('@/features/auth/store/authStore', () => ({ + authStore: { checkForFabricConnect: () => false }, +})); +vi.mock('@/features/cluster/queries/getClusterInfoQuery', () => ({ getClusterInfo: vi.fn() })); +vi.mock('@/integrations/api/instance/status/getInstanceUserInfo', () => ({ getInstanceUserInfo: vi.fn() })); +vi.mock('@/integrations/api/instance/status/restartInstance', () => ({ restartInstance: vi.fn() })); +vi.mock('@/lib/sleep', () => ({ sleep: vi.fn(() => Promise.resolve()) })); +vi.mock('sonner', () => ({ + toast: { loading: vi.fn(() => 'toast-1'), error: vi.fn(), success: vi.fn(), dismiss: vi.fn() }, +})); + +import { getInstanceClient } from '@/config/getInstanceClient'; +import { getClusterInfo } from '@/features/cluster/queries/getClusterInfoQuery'; +import { getInstanceUserInfo } from '@/integrations/api/instance/status/getInstanceUserInfo'; +import { restartInstance } from '@/integrations/api/instance/status/restartInstance'; +import { toast } from 'sonner'; +import { useRestartClusterClick } from './useRestartClusterClick'; + +function instance(id: string, overrides: Partial = {}): Instance { + return { + id, + status: 'RUNNING', + instanceFqdn: `${id}.example.com`, + operationsApiPort: 9925, + operationsApiSecure: true, + ...overrides, + } as Instance; +} + +function targetOf(client: AxiosInstance): string { + return (client as unknown as { target: string }).target; +} + +let restarted: string[] = []; + +async function clickRestart() { + const { result } = renderHook(() => useRestartClusterClick()); + await act(async () => { + await (result.current.onRestartClick() as unknown as Promise); + }); + return result; +} + +beforeEach(() => { + restarted = []; + vi.mocked(getInstanceUserInfo).mockResolvedValue({} as never); + vi.mocked(restartInstance).mockImplementation(async ({ instanceClient }) => { + restarted.push(targetOf(instanceClient)); + return { message: 'ok' }; + }); +}); + +afterEach(() => vi.clearAllMocks()); + +describe('useRestartClusterClick', () => { + it('restarts only the RUNNING instances, last first, and reports the one it could not restart', async () => { + vi.mocked(getClusterInfo).mockResolvedValue( + { + id: 'clu-1', + instances: [instance('ins-a'), instance('ins-b', { status: 'STOPPED' }), instance('ins-c')], + } as Cluster, + ); + + const result = await clickRestart(); + + expect(restarted).toEqual(['ins-c', 'ins-a']); + expect(result.current.isRestartPending).toBe(false); + expect(toast.error).toHaveBeenCalledWith( + 'Error', + expect.objectContaining({ description: expect.stringContaining('Only 2 "RUNNING" instances were restarted') }), + ); + }); + + it('reports success once every instance restarted', async () => { + vi.mocked(getClusterInfo).mockResolvedValue({ id: 'clu-1', instances: [instance('ins-a')] } as Cluster); + + await clickRestart(); + + expect(restarted).toEqual(['ins-a']); + expect(toast.success).toHaveBeenCalledWith('Success', expect.objectContaining({ id: 'toast-1' })); + expect(getRestartState('clu-1')).toBeUndefined(); + }); + + it('reports a cluster with no instances as a failure, not a success', async () => { + vi.mocked(getClusterInfo).mockResolvedValue({ id: 'clu-1', instances: [] } as unknown as Cluster); + + await clickRestart(); + + expect(toast.success).not.toHaveBeenCalled(); + expect(toast.error).toHaveBeenCalledWith( + 'Error', + expect.objectContaining({ description: expect.stringContaining('No instances were found within the cluster') }), + ); + }); + + describe('when the cluster cannot be loaded', () => { + it.each([ + ['the cluster fetch rejects', () => vi.mocked(getClusterInfo).mockRejectedValue(new Error('503'))], + ['an instance client cannot be built', () => { + vi.mocked(getClusterInfo).mockResolvedValue({ id: 'clu-1', instances: [instance('ins-a')] } as Cluster); + vi.mocked(getInstanceClient).mockImplementationOnce(() => { + throw new TypeError('Invalid URL'); + }); + }], + ])('when %s, it reports it on the restart toast, restarts nothing, and stops pending', async (_label, arrange) => { + arrange(); + + const result = await clickRestart(); + + expect(restartInstance).not.toHaveBeenCalled(); + expect(result.current.isRestartPending).toBe(false); + expect(getRestartState('clu-1')).toBeUndefined(); + expect(toast.success).not.toHaveBeenCalled(); + expect(toast.error).toHaveBeenCalledWith( + 'Error', + expect.objectContaining({ + id: 'toast-1', + description: "Could not load the cluster's instances, so nothing was restarted.", + }), + ); + }); + }); +}); diff --git a/src/hooks/useRestartClusterClick.tsx b/src/hooks/useRestartClusterClick.tsx index 923374f84..8311e9d12 100644 --- a/src/hooks/useRestartClusterClick.tsx +++ b/src/hooks/useRestartClusterClick.tsx @@ -1,14 +1,11 @@ import { ProgressBar } from '@/components/ProgressBar'; -import { getInstanceClient } from '@/config/getInstanceClient'; -import { authStore } from '@/features/auth/store/authStore'; -import { getClusterInfo } from '@/features/cluster/queries/getClusterInfoQuery'; +import { ClusterRestartTargets, getClusterRestartTargets } from '@/hooks/getClusterRestartTargets'; import { getInstanceUserInfo } from '@/integrations/api/instance/status/getInstanceUserInfo'; import { restartInstance } from '@/integrations/api/instance/status/restartInstance'; import { excludeFalsy } from '@/lib/arrays/excludeFalsy'; import { pluralize } from '@/lib/pluralize'; import { markRestarting } from '@/lib/restart/restartTracker'; import { sleep } from '@/lib/sleep'; -import { getOperationsUrlForInstance } from '@/lib/urls/getOperationsUrlForInstance'; import { useParams } from '@tanstack/react-router'; import { useCallback, useState } from 'react'; import { toast } from 'sonner'; @@ -57,124 +54,131 @@ export function useRestartClusterClick( ), }); - const cluster = await getClusterInfo(clusterId); - const clusterUsesFabricConnect = authStore.checkForFabricConnect(cluster.id); - const allInstances = cluster?.instances ?? []; - const runningInstances = allInstances.filter(instance => instance.status === 'RUNNING').reverse(); - const instanceClients = runningInstances.map(instance => - getInstanceClient({ - id: instance.id, - forceFabricConnect: clusterUsesFabricConnect, - operationsUrl: getOperationsUrlForInstance(instance), - }) - ); - let instancesRestarted = 0; + try { + let plan: ClusterRestartTargets; + try { + plan = await getClusterRestartTargets(clusterId); + } catch { + toast.error('Error', { + id: toastId, + description: "Could not load the cluster's instances, so nothing was restarted.", + duration: 10_000, + action: { + label: 'Dismiss', + onClick: () => toast.dismiss(), + }, + }); + return; + } + const { allInstances, targets } = plan; + let instancesRestarted = 0; - // One instance at a time, so the cluster stays reachable through the others unless it has only - // one. The TTLs only matter if this loop dies without reaching `finally`. - const releaseCluster = markRestarting([clusterId], { - reach: runningInstances.length === 1 ? 'down' : 'rolling', - ttlMs: (runningInstances.length + 1) * INSTANCE_RESTART_TTL_MS, - }); + // One instance at a time, so the cluster stays reachable through the others unless it has only + // one. The TTLs only matter if this loop dies without reaching `finally`. + const releaseCluster = markRestarting([clusterId], { + reach: targets.length === 1 ? 'down' : 'rolling', + ttlMs: (targets.length + 1) * INSTANCE_RESTART_TTL_MS, + }); - try { - for (let i = 0; i < instanceClients.length; i++) { - const instanceClient = instanceClients[i]; - if (!canceled) { - toast.loading(`Restarting Instance ${i + 1} of ${instanceClients.length}`, { - ...toastConfig, - id: toastId, - description: ( - - ), - }); - let releaseInstance: (() => void) | undefined; - try { - // Make sure the instance is responding. Ungated, so an instance central manager already - // reports as restarting fails here instead of stalling the loop. - await getInstanceUserInfo({ - instanceClient, - skipRestartGate: true, + try { + for (let i = 0; i < targets.length; i++) { + const { instance, instanceClient } = targets[i]; + if (!canceled) { + toast.loading(`Restarting Instance ${i + 1} of ${targets.length}`, { + ...toastConfig, + id: toastId, + description: ( + + ), }); - releaseInstance = markRestarting([runningInstances[i].id], { - reach: 'down', - ttlMs: INSTANCE_RESTART_TTL_MS, - }); - // Then restart it. - await restartInstance({ - operation: 'restart', - replicated: false, - instanceClient, - }); - instancesRestarted += 1; - } catch { - if (i + 1 !== instanceClients.length) { - // If it fails to restart, or wasn't available, warn for a bit then move on. - toast.loading(`Failed Restarting Instance ${i + 1} of ${instanceClients.length}`, { - ...toastConfig, - id: toastId, - description: 'We will carry on momentarily.', + let releaseInstance: (() => void) | undefined; + try { + // Make sure the instance is responding. Ungated, so an instance central manager already + // reports as restarting fails here instead of stalling the loop. + await getInstanceUserInfo({ + instanceClient, + skipRestartGate: true, + }); + releaseInstance = markRestarting([instance.id], { + reach: 'down', + ttlMs: INSTANCE_RESTART_TTL_MS, }); - await sleep(3000); + // Then restart it. + await restartInstance({ + operation: 'restart', + replicated: false, + instanceClient, + }); + instancesRestarted += 1; + } catch { + if (i + 1 !== targets.length) { + // If it fails to restart, or wasn't available, warn for a bit then move on. + toast.loading(`Failed Restarting Instance ${i + 1} of ${targets.length}`, { + ...toastConfig, + id: toastId, + description: 'We will carry on momentarily.', + }); + await sleep(3000); + } + } finally { + releaseInstance?.(); } - } finally { - releaseInstance?.(); } } + } finally { + // Releasing refetches the cluster's queries (`onRestartSettled`). + releaseCluster(); } - } finally { - // Releasing refetches the cluster's queries (`onRestartSettled`). - releaseCluster(); - } - - setIsRestartPending(false); - if (canceled) { - toast.error('Cancelled', { - id: toastId, - description: `The restart was partially cancelled.`, - duration: 10_000, - action: { - label: 'Dismiss', - onClick: () => toast.dismiss(), - }, - }); - } else if (allInstances.length === instancesRestarted) { - onRestartedSuccessfully?.(); - toast.success('Success', { - id: toastId, - description: `Cluster fully restarted!`, - duration: 10_000, - action: { - label: 'Dismiss', - onClick: () => toast.dismiss(), - }, - }); - } else { - const allTheInstances = pluralize(allInstances.length, 'instance', 'instances'); - const someRunningInstancesWere = pluralize( - instancesRestarted, - '"RUNNING" instance was', - '"RUNNING" instances were', - ); - toast.error('Error', { - id: toastId, - description: `Failed to fully restart cluster.\n` - + ([ - allInstances.length === 0 && 'No instances were found within the cluster to restart.', - instancesRestarted === 0 && `No instances were in a "RUNNING" state of ${allTheInstances}.`, - allInstances.length !== instancesRestarted - && `Only ${someRunningInstancesWere} restarted of ${allTheInstances}.`, - ].filter(excludeFalsy).shift() || ''), - duration: 10_000, - action: { - label: 'Dismiss', - onClick: () => toast.dismiss(), - }, - }); + if (canceled) { + toast.error('Cancelled', { + id: toastId, + description: `The restart was partially cancelled.`, + duration: 10_000, + action: { + label: 'Dismiss', + onClick: () => toast.dismiss(), + }, + }); + } else if (allInstances.length > 0 && allInstances.length === instancesRestarted) { + onRestartedSuccessfully?.(); + toast.success('Success', { + id: toastId, + description: `Cluster fully restarted!`, + duration: 10_000, + action: { + label: 'Dismiss', + onClick: () => toast.dismiss(), + }, + }); + } else { + const allTheInstances = pluralize(allInstances.length, 'instance', 'instances'); + const someRunningInstancesWere = pluralize( + instancesRestarted, + '"RUNNING" instance was', + '"RUNNING" instances were', + ); + toast.error('Error', { + id: toastId, + description: `Failed to fully restart cluster.\n` + + ([ + allInstances.length === 0 && 'No instances were found within the cluster to restart.', + instancesRestarted === 0 && `No instances were in a "RUNNING" state of ${allTheInstances}.`, + allInstances.length !== instancesRestarted + && `Only ${someRunningInstancesWere} restarted of ${allTheInstances}.`, + ].filter(excludeFalsy).shift() || ''), + duration: 10_000, + action: { + label: 'Dismiss', + onClick: () => toast.dismiss(), + }, + }); + } + } finally { + setIsRestartPending(false); } }, [clusterId, onRestartedSuccessfully]); diff --git a/src/hooks/useRollingConfigUpdate.tsx b/src/hooks/useRollingConfigUpdate.tsx index b3423d961..19b34090b 100644 --- a/src/hooks/useRollingConfigUpdate.tsx +++ b/src/hooks/useRollingConfigUpdate.tsx @@ -1,9 +1,7 @@ import { ProgressBar } from '@/components/ProgressBar'; -import { getInstanceClient } from '@/config/getInstanceClient'; import { InstanceClientIdConfig, InstanceTypeConfig } from '@/config/instanceClientConfig'; import { useInstanceClientIdParams } from '@/config/useInstanceClient'; -import { authStore } from '@/features/auth/store/authStore'; -import { getClusterInfo } from '@/features/cluster/queries/getClusterInfoQuery'; +import { getClusterRestartTargets } from '@/hooks/getClusterRestartTargets'; import { Instance } from '@/integrations/api/api.patch'; import { getInstanceUserInfo } from '@/integrations/api/instance/status/getInstanceUserInfo'; import { restartInstance } from '@/integrations/api/instance/status/restartInstance'; @@ -12,7 +10,6 @@ import { acknowledgedReplicationNodes } from '@/integrations/api/replication'; import { excludeFalsy } from '@/lib/arrays/excludeFalsy'; import { pluralize } from '@/lib/pluralize'; import { sleep } from '@/lib/sleep'; -import { getOperationsUrlForInstance } from '@/lib/urls/getOperationsUrlForInstance'; import { useQueryClient } from '@tanstack/react-query'; import type { AxiosInstance } from 'axios'; import { useCallback, useState } from 'react'; @@ -51,23 +48,13 @@ async function planRollingConfigUpdate( fanOutAfterCanary: false, }; } - const cluster = await getClusterInfo(operationsParams.entityId); - const allInstances = cluster?.instances || []; - const clusterUsesFabricConnect = !!cluster && authStore.checkForFabricConnect(cluster.id); - const targets = allInstances - .filter(instance => instance.status === 'RUNNING') - .map(instance => ({ - instanceClient: getInstanceClient({ - id: instance.id, - forceFabricConnect: clusterUsesFabricConnect, - operationsUrl: getOperationsUrlForInstance(instance), - }), - replicationFqdn: instance.replicationFqdn, - })) - .reverse(); + const { allInstances, targets } = await getClusterRestartTargets(operationsParams.entityId); return { allInstances, - targets, + targets: targets.map(({ instance, instanceClient }) => ({ + instanceClient, + replicationFqdn: instance.replicationFqdn, + })), fanOutAfterCanary: shouldFanOutSetConfiguration(allInstances), }; } From b9dfc8be7eafa0180182e0aa61357deabf540a0e Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Sun, 11 Oct 2026 02:29:06 -0400 Subject: [PATCH 4/5] fix(clusters): honor Cancel clicked while an instance is being checked 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 --- src/hooks/useApplyLicensesClick.test.tsx | 78 +++++++++++++++++++++++ src/hooks/useApplyLicensesClick.tsx | 3 + src/hooks/useRestartClusterClick.test.tsx | 33 ++++++++++ src/hooks/useRestartClusterClick.tsx | 5 +- src/hooks/useRollingConfigUpdate.test.tsx | 30 +++++++++ src/hooks/useRollingConfigUpdate.tsx | 5 +- 6 files changed, 152 insertions(+), 2 deletions(-) create mode 100644 src/hooks/useApplyLicensesClick.test.tsx diff --git a/src/hooks/useApplyLicensesClick.test.tsx b/src/hooks/useApplyLicensesClick.test.tsx new file mode 100644 index 000000000..87f4f5a62 --- /dev/null +++ b/src/hooks/useApplyLicensesClick.test.tsx @@ -0,0 +1,78 @@ +/** + * @vitest-environment jsdom + */ +import type { SchemaLicense } from '@/integrations/api/api.gen'; +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import { act, renderHook } from '@testing-library/react'; +import { PropsWithChildren } from 'react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +vi.mock('@/config/useInstanceClient', () => { + const instanceClient = { target: 'ins-a' }; + const params = { entityType: 'instance', entityId: 'ins-a', instanceClient }; + return { + useInstanceClient: () => instanceClient, + useInstanceClientIdParams: () => params, + }; +}); +vi.mock('@/integrations/api/instance/status/getInstanceUserInfo', () => ({ getInstanceUserInfo: vi.fn() })); +vi.mock('@/integrations/api/instance/auth/installUsageLicense', () => ({ installUsageLicense: vi.fn() })); +vi.mock('@/lib/sleep', () => ({ sleep: vi.fn(() => Promise.resolve()) })); +vi.mock('sonner', () => ({ + toast: { loading: vi.fn(() => 'toast-1'), error: vi.fn(), success: vi.fn(), dismiss: vi.fn() }, +})); + +import { installUsageLicense } from '@/integrations/api/instance/auth/installUsageLicense'; +import { getInstanceUserInfo } from '@/integrations/api/instance/status/getInstanceUserInfo'; +import { toast } from 'sonner'; +import { useApplyLicensesClick } from './useApplyLicensesClick'; + +const LICENSES = [{ license: 'lic-1' }, { license: 'lic-2' }] as SchemaLicense[]; + +function wrapper() { + const queryClient = new QueryClient(); + return ({ children }: PropsWithChildren) => {children} + ; +} + +async function applyLicenses() { + const { result } = renderHook(() => useApplyLicensesClick({ licenses: LICENSES }), { wrapper: wrapper() }); + await act(async () => { + await (result.current.onApplyLicensesClick() as unknown as Promise); + }); + return result; +} + +beforeEach(() => { + vi.mocked(getInstanceUserInfo).mockResolvedValue({} as never); + vi.mocked(installUsageLicense).mockResolvedValue({ message: 'ok' } as never); +}); + +afterEach(() => vi.clearAllMocks()); + +describe('useApplyLicensesClick', () => { + it('installs every license and reports success', async () => { + const result = await applyLicenses(); + + expect(vi.mocked(installUsageLicense).mock.calls.map(([body]) => body.license)).toEqual(['lic-1', 'lic-2']); + expect(result.current.isApplyLicensesPending).toBe(false); + expect(toast.success).toHaveBeenCalled(); + }); + + it('does not install a license when Cancel is clicked while the instance is being checked', async () => { + vi.mocked(getInstanceUserInfo).mockImplementation(async () => { + cancel(); + return {} as never; + }); + + await applyLicenses(); + + expect(installUsageLicense).not.toHaveBeenCalled(); + expect(toast.error).toHaveBeenCalledWith('Cancelled', expect.anything()); + }); +}); + +function cancel() { + const [, options] = vi.mocked(toast.loading).mock.calls.at(-1)!; + (options as unknown as { action: { onClick: () => void } }).action.onClick(); +} diff --git a/src/hooks/useApplyLicensesClick.tsx b/src/hooks/useApplyLicensesClick.tsx index a6303e539..90750d256 100644 --- a/src/hooks/useApplyLicensesClick.tsx +++ b/src/hooks/useApplyLicensesClick.tsx @@ -74,6 +74,9 @@ export function useApplyLicensesClick({ licenses }: ApplyLicensesClickParams): A await getInstanceUserInfo({ instanceClient, }); + if (canceled) { + continue; + } // Then install the usage license to it. await installUsageLicense({ license: license.license, diff --git a/src/hooks/useRestartClusterClick.test.tsx b/src/hooks/useRestartClusterClick.test.tsx index edffce2a0..5d89b76a3 100644 --- a/src/hooks/useRestartClusterClick.test.tsx +++ b/src/hooks/useRestartClusterClick.test.tsx @@ -96,6 +96,34 @@ describe('useRestartClusterClick', () => { expect(getRestartState('clu-1')).toBeUndefined(); }); + it('does not restart an instance when Cancel is clicked while it is being checked', async () => { + vi.mocked(getClusterInfo).mockResolvedValue( + { id: 'clu-1', instances: [instance('ins-a'), instance('ins-b')] } as Cluster, + ); + vi.mocked(getInstanceUserInfo).mockImplementation(async () => { + cancel(); + return {} as never; + }); + + await clickRestart(); + + expect(restartInstance).not.toHaveBeenCalled(); + expect(toast.error).toHaveBeenCalledWith('Cancelled', expect.anything()); + }); + + it('says the RUNNING instances failed to restart, not that none were RUNNING', async () => { + vi.mocked(getClusterInfo).mockResolvedValue( + { id: 'clu-1', instances: [instance('ins-a'), instance('ins-b')] } as Cluster, + ); + vi.mocked(restartInstance).mockRejectedValue(new Error('did not come back')); + + await clickRestart(); + + const [, options] = vi.mocked(toast.error).mock.calls.at(-1)!; + expect(options?.description).toContain('Only 0 "RUNNING" instances were restarted of 2 instances.'); + expect(options?.description).not.toContain('No instances were in a "RUNNING" state'); + }); + it('reports a cluster with no instances as a failure, not a success', async () => { vi.mocked(getClusterInfo).mockResolvedValue({ id: 'clu-1', instances: [] } as unknown as Cluster); @@ -136,3 +164,8 @@ describe('useRestartClusterClick', () => { }); }); }); + +function cancel() { + const [, options] = vi.mocked(toast.loading).mock.calls.at(-1)!; + (options as unknown as { action: { onClick: () => void } }).action.onClick(); +} diff --git a/src/hooks/useRestartClusterClick.tsx b/src/hooks/useRestartClusterClick.tsx index 8311e9d12..e028d1af8 100644 --- a/src/hooks/useRestartClusterClick.tsx +++ b/src/hooks/useRestartClusterClick.tsx @@ -102,6 +102,9 @@ export function useRestartClusterClick( instanceClient, skipRestartGate: true, }); + if (canceled) { + continue; + } releaseInstance = markRestarting([instance.id], { reach: 'down', ttlMs: INSTANCE_RESTART_TTL_MS, @@ -166,7 +169,7 @@ export function useRestartClusterClick( description: `Failed to fully restart cluster.\n` + ([ allInstances.length === 0 && 'No instances were found within the cluster to restart.', - instancesRestarted === 0 && `No instances were in a "RUNNING" state of ${allTheInstances}.`, + targets.length === 0 && `No instances were in a "RUNNING" state of ${allTheInstances}.`, allInstances.length !== instancesRestarted && `Only ${someRunningInstancesWere} restarted of ${allTheInstances}.`, ].filter(excludeFalsy).shift() || ''), diff --git a/src/hooks/useRollingConfigUpdate.test.tsx b/src/hooks/useRollingConfigUpdate.test.tsx index 1a70ba91b..f2560055b 100644 --- a/src/hooks/useRollingConfigUpdate.test.tsx +++ b/src/hooks/useRollingConfigUpdate.test.tsx @@ -434,6 +434,36 @@ describe('useRollingConfigUpdate outcome reporting', () => { expect(options?.description).toContain('Only 1 "RUNNING" instance was restarted of 2 instances.'); expect(options?.description).not.toContain('may already have the new configuration saved'); }); + + it('says the RUNNING instances failed to restart, not that none were RUNNING', async () => { + useCluster([instance('ins-a', '5.1.29'), instance('ins-b', '5.1.29')]); + vi.mocked(restartInstance).mockRejectedValue(new Error('did not come back')); + + await save(); + + const [, options] = vi.mocked(toast.error).mock.calls.at(-1)!; + expect(options?.description).toContain('Only 0 "RUNNING" instances were restarted of 2 instances.'); + expect(options?.description).not.toContain('No instances were in a "RUNNING" state'); + }); +}); + +describe('useRollingConfigUpdate cancellation', () => { + it('does not write or restart an instance when Cancel is clicked while it is being checked', async () => { + useCluster([instance('ins-a', '5.1.29'), instance('ins-b', '5.1.29')]); + vi.mocked(getInstanceUserInfo).mockImplementation(async ({ instanceClient }) => { + events.push(`check ${targetOf(instanceClient)}`); + cancel(); + return {} as never; + }); + + await save(); + + expect(events).toEqual(['check ins-b']); + expect(toast.error).toHaveBeenCalledWith( + 'Cancelled', + expect.objectContaining({ description: 'The config update was partially cancelled.' }), + ); + }); }); describe('useRollingConfigUpdate when the cluster cannot be loaded', () => { diff --git a/src/hooks/useRollingConfigUpdate.tsx b/src/hooks/useRollingConfigUpdate.tsx index 19b34090b..8acab2351 100644 --- a/src/hooks/useRollingConfigUpdate.tsx +++ b/src/hooks/useRollingConfigUpdate.tsx @@ -131,6 +131,9 @@ export function useRollingConfigUpdate( await getInstanceUserInfo({ instanceClient, }); + if (canceled) { + continue; + } if (!replicationFqdn || !acknowledged.has(replicationFqdn.toLowerCase())) { await setConfiguration({ @@ -213,7 +216,7 @@ export function useRollingConfigUpdate( description: `Failed to fully update cluster.\n` + ([ allInstances.length === 0 && 'No instances were found within the cluster to restart.', - instancesRestarted === 0 && `No instances were in a "RUNNING" state of ${allTheInstances}.`, + targets.length === 0 && `No instances were in a "RUNNING" state of ${allTheInstances}.`, allInstances.length !== instancesRestarted && `Only ${someRunningInstancesWere} restarted of ${allTheInstances}.`, ].filter(excludeFalsy).shift() || '') From 3afc3a91b8132c6d9982e4bfb8b4b2b31a6f0e8c Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Mon, 12 Oct 2026 12:08:39 -0400 Subject: [PATCH 5/5] refactor(restart): end the restart and license loops when Cancel lands 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 --- src/hooks/useApplyLicensesClick.tsx | 2 +- src/hooks/useRestartClusterClick.tsx | 2 +- src/hooks/useRollingConfigUpdate.tsx | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/hooks/useApplyLicensesClick.tsx b/src/hooks/useApplyLicensesClick.tsx index 90750d256..493838764 100644 --- a/src/hooks/useApplyLicensesClick.tsx +++ b/src/hooks/useApplyLicensesClick.tsx @@ -75,7 +75,7 @@ export function useApplyLicensesClick({ licenses }: ApplyLicensesClickParams): A instanceClient, }); if (canceled) { - continue; + break; } // Then install the usage license to it. await installUsageLicense({ diff --git a/src/hooks/useRestartClusterClick.tsx b/src/hooks/useRestartClusterClick.tsx index e028d1af8..ca5eefc76 100644 --- a/src/hooks/useRestartClusterClick.tsx +++ b/src/hooks/useRestartClusterClick.tsx @@ -103,7 +103,7 @@ export function useRestartClusterClick( skipRestartGate: true, }); if (canceled) { - continue; + break; } releaseInstance = markRestarting([instance.id], { reach: 'down', diff --git a/src/hooks/useRollingConfigUpdate.tsx b/src/hooks/useRollingConfigUpdate.tsx index 8acab2351..12e8fe22a 100644 --- a/src/hooks/useRollingConfigUpdate.tsx +++ b/src/hooks/useRollingConfigUpdate.tsx @@ -132,7 +132,7 @@ export function useRollingConfigUpdate( instanceClient, }); if (canceled) { - continue; + break; } if (!replicationFqdn || !acknowledged.has(replicationFqdn.toLowerCase())) {