From 6138c3b52cd14bf061915d536c6ad1a5890eed26 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Sun, 11 Oct 2026 06:08:43 -0400 Subject: [PATCH 1/9] fix(restart): say a restart Harper accepted is not back yet, never that it failed restartInstance sent the restart and then watched for the target with user_info, and both steps rejected the same promise, so a target slow to answer again read as a failed restart everywhere: "Failed to restart instance.", "Only N of M instances restarted", and success: false to the chat agent. Each of those invites a second restart of a target that already went down once. Harper's answer to the restart request is its only verdict: `restart` answers before it tears down (bin/restart.ts schedules the teardown 50ms out), and `restart_service` once its job has started (executeJob). So a probe failure after a 2xx now rejects with RestartUnconfirmedError (cause: the last probe error), shown as "Not back yet ... check its status before you restart it again". - The watch keeps going past the usual recovery time (a second probe schedule, no second settle) and calls onSlowToReturn once in between, so the Restart button and both loops say "still waiting" instead of giving up. Every restart mark's TTL comes from the same constants, and the probes skip the gateway replay, so no mark lapses mid-watch. - The cluster restart and rolling config update stop at a member that never comes back instead of restarting the next one, which could leave no member serving, and report the members they left alone. - createApp keeps success: true (the app exists) and restartHTTPService returns copy without the "Error:" prefix telling the agent not to restart again. Both now replace their progress toast on any failure, which used to keep spinning. Closes #1895 Co-Authored-By: Claude Opus 5.5 --- DESIGN.md | 8 + .../Chat/tools/createApp/execute.ts | 15 +- .../Chat/tools/describeToolError.ts | 8 + .../Chat/tools/restartHTTPService/execute.ts | 13 +- .../Chat/tools/restartTools.test.ts | 101 ++++++++++ .../instance/operations/index.test.tsx | 1 + src/features/instance/operations/index.tsx | 8 +- src/hooks/useRestartClusterClick.test.tsx | 113 +++++++++++- src/hooks/useRestartClusterClick.tsx | 47 ++++- src/hooks/useRestartInstanceClick.test.tsx | 104 +++++++++++ src/hooks/useRestartInstanceClick.ts | 20 +- src/hooks/useRollingConfigUpdate.test.tsx | 102 ++++++++++ src/hooks/useRollingConfigUpdate.tsx | 35 +++- .../instance/status/restartInstance.test.ts | 174 ++++++++++++++++++ .../api/instance/status/restartInstance.ts | 59 ++++-- .../api/instance/status/restartUnconfirmed.ts | 33 ++++ src/react-query/queryClient.test.ts | 27 +++ src/react-query/queryClient.ts | 19 +- 18 files changed, 847 insertions(+), 40 deletions(-) create mode 100644 src/features/instance/applications/components/Chat/tools/restartTools.test.ts create mode 100644 src/integrations/api/instance/status/restartInstance.test.ts create mode 100644 src/integrations/api/instance/status/restartUnconfirmed.ts diff --git a/DESIGN.md b/DESIGN.md index 4ecf3a582..fd75a9a1b 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -76,6 +76,14 @@ The same rule decides what a failure says. [`isUnansweredWrite`](src/integration - **These toasts are never collapsed** onto a shared id: each warns about a different write, and a read timing out alongside one must not replace it through `request-timeout`. - **A read that is costly to repeat** stays a read here and opts out of the replay instead (`package_component`, `skipGatewayRetry`). +## A restart Harper accepted never reads as failed + +Harper's answer to the restart request is its only verdict on the restart: `restart` answers before it tears down (harper `bin/restart.ts` schedules the teardown 50ms out so the reply can go first) and `restart_service` once its job has started (`executeJob`). So [`restartInstance`](src/integrations/api/instance/status/restartInstance.ts) rejects with the request's own error only when that request failed; after a 2xx, a target that never answers the `user_info` watch rejects with [`RestartUnconfirmedError`](src/integrations/api/instance/status/restartUnconfirmed.ts), which reads "Not back yet", never "Failed", and is never a reason to send the restart again. + +- **The watch outlasts the usual recovery time** (one settle, then two probe schedules), and `onSlowToReturn` fires once between them so a caller can say it is still waiting. Every restart mark uses `RESTART_MARK_TTL_MS`, derived from the same constants, so no mark lapses while its watch still runs; the probes skip the gateway replay for the same reason. +- **A one-at-a-time loop stops at a member that is not back** (cluster restart, rolling config update): restarting the next one could leave no member serving. It reports the members it left alone. +- **The chat tools return `success: true`** for it: their action was carried out, and `false` invites the agent to repeat it. + ## 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/features/instance/applications/components/Chat/tools/createApp/execute.ts b/src/features/instance/applications/components/Chat/tools/createApp/execute.ts index 617b2fd0d..d8e21a9aa 100644 --- a/src/features/instance/applications/components/Chat/tools/createApp/execute.ts +++ b/src/features/instance/applications/components/Chat/tools/createApp/execute.ts @@ -1,6 +1,7 @@ import { templates } from '@/features/instance/applications/components/NewApplication/templates'; import { addComponent } from '@/integrations/api/instance/applications/addComponent'; import { restartInstance } from '@/integrations/api/instance/status/restartInstance'; +import { RestartUnconfirmedError } from '@/integrations/api/instance/status/restartUnconfirmed'; import { findBy } from '@/lib/arrays/findBy'; import { emitToListeners } from '@/lib/events/listener'; import { toKebabCase } from '@/lib/string/to-kebab-case'; @@ -8,7 +9,7 @@ import { inputSchema } from '@harperfast/agent-tools/tools/createApp/inputSchema import { toast } from 'sonner'; import { z } from 'zod'; import { ExecuteParams } from '../../types/executeParams'; -import { describeToolError } from '../describeToolError'; +import { describeToolError, toastToolFailure } from '../describeToolError'; import { Output } from './output'; export async function execute( @@ -38,6 +39,7 @@ export async function execute( template: template.npm || template.githubUrl, }); } catch (err) { + toastToolFailure(toastId, err); return { success: false, message: `Error: ${describeToolError(err)}`, @@ -59,6 +61,17 @@ export async function execute( replicated: instanceClientParams.entityType === 'cluster', }); } catch (err) { + toastToolFailure(toastId, err); + if (err instanceof RestartUnconfirmedError) { + // The app exists either way; `success: false` would invite creating it again. + return { + success: true, + message: `App "${name}" created. Harper accepted the restart that loads it, but the instance hasn't ` + + "answered since, so it may still be starting. Don't create the app or restart again until its " + + 'status shows it is back.', + webURL: baseURL, + }; + } return { success: false, message: `Error: ${describeToolError(err)}`, diff --git a/src/features/instance/applications/components/Chat/tools/describeToolError.ts b/src/features/instance/applications/components/Chat/tools/describeToolError.ts index fe954ef0a..b77fa85c2 100644 --- a/src/features/instance/applications/components/Chat/tools/describeToolError.ts +++ b/src/features/instance/applications/components/Chat/tools/describeToolError.ts @@ -1,5 +1,13 @@ import { isUnansweredWrite, UNANSWERED_WRITE_MESSAGE } from '@/integrations/api/unansweredWrite'; +import { describeError } from '@/react-query/queryClient'; +import { toast } from 'sonner'; export function describeToolError(err: unknown): string { return isUnansweredWrite(err) ? UNANSWERED_WRITE_MESSAGE : String(err); } + +/** Replace a tool's progress toast with what its failure means; a loading toast never dismisses itself. */ +export function toastToolFailure(toastId: string | number, err: unknown) { + const { title, description } = describeError(err); + toast.error(title, { id: toastId, description, duration: 10_000 }); +} diff --git a/src/features/instance/applications/components/Chat/tools/restartHTTPService/execute.ts b/src/features/instance/applications/components/Chat/tools/restartHTTPService/execute.ts index 5e4525885..2ad230ca4 100644 --- a/src/features/instance/applications/components/Chat/tools/restartHTTPService/execute.ts +++ b/src/features/instance/applications/components/Chat/tools/restartHTTPService/execute.ts @@ -1,9 +1,10 @@ import { restartInstance } from '@/integrations/api/instance/status/restartInstance'; +import { RestartUnconfirmedError } from '@/integrations/api/instance/status/restartUnconfirmed'; import { inputSchema } from '@harperfast/agent-tools/tools/restartHTTPService/inputSchema'; import { toast } from 'sonner'; import { z } from 'zod'; import { ExecuteParams } from '../../types/executeParams'; -import { describeToolError } from '../describeToolError'; +import { describeToolError, toastToolFailure } from '../describeToolError'; import { Output } from './output'; export async function execute( @@ -21,6 +22,16 @@ export async function execute( replicated: instanceClientParams.entityType === 'cluster', }); } catch (err) { + toastToolFailure(toastId, err); + if (err instanceof RestartUnconfirmedError) { + // `success` is whether the tool's own action was carried out, and Harper accepted the restart. + return { + success: true, + message: "Harper accepted the restart, but the HTTP service hasn't answered since, so it may still be " + + "starting. Don't restart it again until its status shows it is back, or it goes down a second time.", + webURL: baseURL, + }; + } return { success: false, message: `Error: ${describeToolError(err)}`, diff --git a/src/features/instance/applications/components/Chat/tools/restartTools.test.ts b/src/features/instance/applications/components/Chat/tools/restartTools.test.ts new file mode 100644 index 000000000..a3aae6441 --- /dev/null +++ b/src/features/instance/applications/components/Chat/tools/restartTools.test.ts @@ -0,0 +1,101 @@ +import { + RESTART_UNCONFIRMED_MESSAGE, + RESTART_UNCONFIRMED_TITLE, + RestartUnconfirmedError, +} from '@/integrations/api/instance/status/restartUnconfirmed'; +import { AxiosError } from 'axios'; +import { toast } from 'sonner'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import type { ExecuteParams } from '../types/executeParams'; +import { execute as createApp } from './createApp/execute'; +import { execute as restartHTTPService } from './restartHTTPService/execute'; + +vi.mock('@/integrations/api/instance/status/restartInstance', () => ({ restartInstance: vi.fn() })); +vi.mock('@/integrations/api/instance/applications/addComponent', () => ({ addComponent: vi.fn() })); +vi.mock('@/lib/events/listener', () => ({ emitToListeners: vi.fn() })); +vi.mock('sonner', () => ({ + toast: { loading: vi.fn(() => 'tool-toast'), error: vi.fn(), success: vi.fn() }, +})); + +import { addComponent } from '@/integrations/api/instance/applications/addComponent'; +import { restartInstance } from '@/integrations/api/instance/status/restartInstance'; + +function params(input: T): ExecuteParams { + return { + input, + instanceClientParams: { entityType: 'instance', entityId: 'ins-1' } as ExecuteParams['instanceClientParams'], + baseURL: 'https://ins-1.example.com', + params: {}, + }; +} + +const notBackYet = () => new RestartUnconfirmedError(new AxiosError('Network Error', AxiosError.ERR_NETWORK)); + +function refused() { + const config = { method: 'post', data: { operation: 'restart_service' } } as never; + return new AxiosError('Request failed with status code 403', AxiosError.ERR_BAD_REQUEST, config, undefined, { + status: 403, + statusText: '', + data: { error: 'This operation is restricted to super users' }, + headers: {}, + config, + }); +} + +beforeEach(() => { + vi.clearAllMocks(); + vi.mocked(addComponent).mockResolvedValue({} as never); +}); + +describe('restartHTTPService', () => { + it('tells the agent a restart Harper accepted is not back yet, and not to restart it again', async () => { + vi.mocked(restartInstance).mockRejectedValue(notBackYet()); + + const output = await restartHTTPService(params({})); + + expect(output.success).toBe(true); + expect(output.message).not.toMatch(/^Error/); + expect(output.message).toContain("Don't restart it again"); + expect(restartInstance).toHaveBeenCalledOnce(); + expect(toast.error).toHaveBeenCalledWith( + RESTART_UNCONFIRMED_TITLE, + expect.objectContaining({ + id: 'tool-toast', + description: RESTART_UNCONFIRMED_MESSAGE, + }), + ); + }); + + it('still reports a refused restart as an error, and clears its progress toast', async () => { + vi.mocked(restartInstance).mockRejectedValue(refused()); + + const output = await restartHTTPService(params({})); + + expect(output.success).toBe(false); + expect(output.message).toMatch(/^Error: /); + expect(toast.error).toHaveBeenCalledWith('Error', expect.objectContaining({ id: 'tool-toast' })); + }); +}); + +describe('createApp', () => { + it('keeps the fact that the app was created when its restart is not back yet', async () => { + vi.mocked(restartInstance).mockRejectedValue(notBackYet()); + + const output = await createApp(params({ name: 'My App', type: 'vanilla' as const })); + + expect(output.success).toBe(true); + expect(output.message).toMatch(/^App "my-app" created\. Harper accepted the restart/); + expect(output.message).toContain("Don't create the app or restart again"); + expect(addComponent).toHaveBeenCalledOnce(); + }); + + it('clears its progress toast when the app could not be created', async () => { + vi.mocked(addComponent).mockRejectedValue(new Error('Component my-app already exists')); + + const output = await createApp(params({ name: 'My App', type: 'vanilla' as const })); + + expect(output.success).toBe(false); + expect(restartInstance).not.toHaveBeenCalled(); + expect(toast.error).toHaveBeenCalledWith(expect.any(String), expect.objectContaining({ id: 'tool-toast' })); + }); +}); diff --git a/src/features/instance/operations/index.test.tsx b/src/features/instance/operations/index.test.tsx index 57345b4f1..bf279f143 100644 --- a/src/features/instance/operations/index.test.tsx +++ b/src/features/instance/operations/index.test.tsx @@ -42,6 +42,7 @@ vi.mock('@/lib/restart/restartTracker', async (importOriginal) => ({ markRestarting, })); vi.mock('@/integrations/api/instance/status/restartInstance', () => ({ + RESTART_MARK_TTL_MS: 600_000, waitUntilAnsweringAfterRestart: () => restartSettles.promise, })); vi.mock('@tanstack/react-router', () => ({ diff --git a/src/features/instance/operations/index.tsx b/src/features/instance/operations/index.tsx index 4adcede8a..98e638fcf 100644 --- a/src/features/instance/operations/index.tsx +++ b/src/features/instance/operations/index.tsx @@ -22,7 +22,10 @@ import { MAX_RESPONSE_BYTES, OperationResult, sendOperation } from '@/features/i import { useInstanceAuth } from '@/hooks/useAuth'; import { useMonacoTheme } from '@/hooks/useMonacoTheme'; import { useInstanceManagePermission } from '@/hooks/usePermissions'; -import { waitUntilAnsweringAfterRestart } from '@/integrations/api/instance/status/restartInstance'; +import { + RESTART_MARK_TTL_MS, + waitUntilAnsweringAfterRestart, +} from '@/integrations/api/instance/status/restartInstance'; import { humanFileSize } from '@/lib/humanFileSize'; import { Editor } from '@/lib/monaco/MonacoEditor'; import { WORKER_FREE_JSON_LANGUAGE_ID } from '@/lib/monaco/workerFreeJsonLanguage'; @@ -38,7 +41,6 @@ import { useEffect, useRef, useState } from 'react'; const OPERATIONS_API_DOCS = 'https://docs.harperdb.io/docs/developers/operations-api/'; const INITIAL_REQUEST = '{\n\t"operation": "describe_all"\n}\n'; const CONFIRM_PREVIEW_CHARS = 4_000; -const RESTART_TTL_MS = 5 * 60_000; const LOGOUT_REFUSAL = 'Sign out from Studio instead: a logout sent from here would end the session the rest of ' + 'Studio is still using, without Studio knowing.'; @@ -100,7 +102,7 @@ function OperationsConsole({ instanceClient, entityId, noun, username }: { // Tracked like the Restart button's, so the page's polls ride out the outage instead of failing // into the global error handlers. const releaseRestart = request.operation === 'restart' - ? markRestarting([entityId], { reach: noun === 'instance' ? 'down' : 'rolling', ttlMs: RESTART_TTL_MS }) + ? markRestarting([entityId], { reach: noun === 'instance' ? 'down' : 'rolling', ttlMs: RESTART_MARK_TTL_MS }) : undefined; try { const result = await sendOperation(instanceClient, request.text, { diff --git a/src/hooks/useRestartClusterClick.test.tsx b/src/hooks/useRestartClusterClick.test.tsx index 7f076515c..0dee9d79c 100644 --- a/src/hooks/useRestartClusterClick.test.tsx +++ b/src/hooks/useRestartClusterClick.test.tsx @@ -2,6 +2,7 @@ * @vitest-environment jsdom */ import type { Cluster, Instance } from '@/integrations/api/api.patch'; +import { RestartUnconfirmedError } from '@/integrations/api/instance/status/restartUnconfirmed'; import { describeUnansweredWrites, OUTCOME_UNKNOWN_TITLE } from '@/integrations/api/unansweredWrite'; import { getRestartState, resetRestartTracker } from '@/lib/restart/restartTracker'; import { act, renderHook } from '@testing-library/react'; @@ -20,6 +21,7 @@ vi.mock('@/features/auth/store/authStore', () => ({ 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', () => ({ + RESTART_MARK_TTL_MS: 600_000, restartInstance: vi.fn(), waitUntilAnsweringAfterRestart: vi.fn(), })); @@ -215,17 +217,42 @@ function answeredWith(status: number) { }); } -function restartsEndingIn(outcomes: Record) { - vi.mocked(restartInstance).mockImplementation(async ({ instanceClient }) => { +function refused() { + return answeredWith(403); +} + +function notBackYet() { + return new RestartUnconfirmedError(new AxiosError('timeout of 3000ms exceeded', AxiosError.ECONNABORTED)); +} + +/** + * Each member's restart outcome, keyed by instance id: an error it rejects with, or a promise it is slow + * to come back until. A member not listed restarts and comes back. `restarted` records the order. + */ +function restartsEndingIn(outcomes: Record>) { + vi.mocked(restartInstance).mockImplementation(async ({ instanceClient, onSlowToReturn }) => { const target = targetOf(instanceClient); restarted.push(target); - if (outcomes[target]) { - throw outcomes[target]; + const outcome = outcomes[target]; + if (outcome instanceof RestartUnconfirmedError) { + onSlowToReturn?.(); + } + if (outcome instanceof Error) { + throw outcome; + } + if (outcome) { + onSlowToReturn?.(); + await outcome; } return { message: 'Restarting Harper. This may take up to 60 seconds.' }; }); } +function finalError() { + const [title, options] = vi.mocked(toast.error).mock.calls.at(-1) ?? []; + return { title, description: (options as { description?: string } | undefined)?.description }; +} + describe('useRestartClusterClick, for a member whose restart got no answer', () => { beforeEach(() => { vi.mocked(getClusterInfo).mockResolvedValue( @@ -270,3 +297,81 @@ describe('useRestartClusterClick, for a member whose restart got no answer', () expect(vi.mocked(toast.error).mock.calls.at(-1)?.[0]).toBe('Error'); }); }); + +describe('useRestartClusterClick, for a member that accepted the restart but has not answered since', () => { + beforeEach(() => { + vi.mocked(getClusterInfo).mockResolvedValue( + { id: 'clu-1', instances: [instance('ins-a'), instance('ins-b'), instance('ins-c')] } as Cluster, + ); + }); + + it('stops there rather than restart another member, and never says the cluster failed', async () => { + // Last member first: ins-c restarts, ins-b accepts and never comes back, ins-a is left alone. + restartsEndingIn({ 'ins-b': notBackYet() }); + + await clickRestart(); + + expect(restarted).toEqual(['ins-c', 'ins-b']); + expect(toast.loading).toHaveBeenCalledWith( + 'Waiting On Instance 2 of 3', + expect.objectContaining({ description: "It accepted the restart but hasn't answered yet." }), + ); + expect(finalError()).toEqual({ + title: 'Not back yet', + description: 'Only 1 "RUNNING" instance was confirmed restarted of 3 instances. ' + + "1 instance accepted the restart but hasn't answered since, so it may still be starting. " + + '1 other instance was not restarted, to keep the cluster available. ' + + 'Check its status before you restart it again.', + }); + expect(toast.success).not.toHaveBeenCalled(); + expect(getRestartState('ins-b')).toBeUndefined(); + expect(getRestartState('clu-1')).toBeUndefined(); + }); + + it('names no members left alone when the last one is the one not back', async () => { + restartsEndingIn({ 'ins-a': notBackYet() }); + + await clickRestart(); + + expect(restarted).toEqual(['ins-c', 'ins-b', 'ins-a']); + expect(finalError().description).toBe( + 'Only 2 "RUNNING" instances were confirmed restarted of 3 instances. ' + + "1 instance accepted the restart but hasn't answered since, so it may still be starting. " + + 'Check its status before you restart it again.', + ); + }); + + it('goes on to the next member once a slow one answers, holding it down until then', async () => { + let answer: (() => void) | undefined; + restartsEndingIn({ 'ins-b': new Promise((resolve) => (answer = resolve)) }); + const { result } = renderHook(() => useRestartClusterClick()); + + let click: Promise | undefined; + act(() => { + click = result.current.onRestartClick() as unknown as Promise; + }); + await vi.waitFor(() => expect(restarted).toEqual(['ins-c', 'ins-b'])); + + expect(getRestartState('ins-b')?.reach).toBe('down'); + + await act(async () => { + answer!(); + await click; + }); + + expect(restarted).toEqual(['ins-c', 'ins-b', 'ins-a']); + expect(toast.success).toHaveBeenCalledWith('Success', expect.objectContaining({ id: 'toast-1' })); + expect(getRestartState('ins-b')).toBeUndefined(); + }); + + it('still carries on past a member that refused, and says it failed', async () => { + restartsEndingIn({ 'ins-b': refused() }); + + await clickRestart(); + + expect(restarted).toEqual(['ins-c', 'ins-b', 'ins-a']); + expect(toast.loading).toHaveBeenCalledWith('Failed Restarting Instance 2 of 3', expect.anything()); + expect(finalError().title).toBe('Error'); + expect(finalError().description).toContain('Only 2 "RUNNING" instances were restarted'); + }); +}); diff --git a/src/hooks/useRestartClusterClick.tsx b/src/hooks/useRestartClusterClick.tsx index ddeeaf09d..81b8c15be 100644 --- a/src/hooks/useRestartClusterClick.tsx +++ b/src/hooks/useRestartClusterClick.tsx @@ -1,7 +1,16 @@ import { ProgressBar } from '@/components/ProgressBar'; import { ClusterRestartTargets, getClusterRestartTargets } from '@/hooks/getClusterRestartTargets'; import { getInstanceUserInfo } from '@/integrations/api/instance/status/getInstanceUserInfo'; -import { restartInstance, waitUntilAnsweringAfterRestart } from '@/integrations/api/instance/status/restartInstance'; +import { + RESTART_MARK_TTL_MS, + restartInstance, + waitUntilAnsweringAfterRestart, +} from '@/integrations/api/instance/status/restartInstance'; +import { + describeUnconfirmedRestart, + RESTART_UNCONFIRMED_TITLE, + RestartUnconfirmedError, +} from '@/integrations/api/instance/status/restartUnconfirmed'; import { describeUnansweredWrites, isUnansweredWrite, OUTCOME_UNKNOWN_TITLE } from '@/integrations/api/unansweredWrite'; import { excludeFalsy } from '@/lib/arrays/excludeFalsy'; import { pluralize } from '@/lib/pluralize'; @@ -11,9 +20,6 @@ import { useParams } from '@tanstack/react-router'; import { useCallback, useState } from 'react'; import { toast } from 'sonner'; -/** Longest a single `restartInstance` can take: 10s settle + 12 probes of up to 3s + 15s apart. */ -const INSTANCE_RESTART_TTL_MS = 5 * 60_000; - interface RestartClusterClickParams { onRestartedSuccessfully?: () => void; } @@ -74,12 +80,14 @@ export function useRestartClusterClick( const { allInstances, targets } = plan; let instancesRestarted = 0; let instancesUnanswered = 0; + /** Members left alone after one accepted the restart and never came back; undefined until then. */ + let instancesUntouched: number | undefined; // 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, + ttlMs: (targets.length + 1) * RESTART_MARK_TTL_MS, }); try { @@ -109,16 +117,27 @@ export function useRestartClusterClick( } releaseInstance = markRestarting([instance.id], { reach: 'down', - ttlMs: INSTANCE_RESTART_TTL_MS, + ttlMs: RESTART_MARK_TTL_MS, }); // Then restart it. await restartInstance({ operation: 'restart', replicated: false, instanceClient, + onSlowToReturn: () => + toast.loading(`Waiting On Instance ${i + 1} of ${targets.length}`, { + ...toastConfig, + id: toastId, + description: "It accepted the restart but hasn't answered yet.", + }), }); instancesRestarted += 1; } catch (error) { + if (error instanceof RestartUnconfirmedError) { + // It may still be down: restarting the next one could leave no member serving. + instancesUntouched = targets.length - i - 1; + break; + } const unanswered = isUnansweredWrite(error); instancesUnanswered += unanswered ? 1 : 0; if (unanswered) { @@ -179,11 +198,19 @@ export function useRestartClusterClick( '"RUNNING" instance was', '"RUNNING" instances were', ); - toast.error(instancesUnanswered ? OUTCOME_UNKNOWN_TITLE : 'Error', { + const title = instancesUnanswered + ? OUTCOME_UNKNOWN_TITLE + : instancesUntouched === undefined + ? 'Error' + : RESTART_UNCONFIRMED_TITLE; + toast.error(title, { id: toastId, - description: instancesUnanswered - ? `Only ${someRunningInstancesWere} confirmed restarted of ${allTheInstances}. ` - + describeUnansweredWrites(instancesUnanswered) + description: instancesUnanswered || instancesUntouched !== undefined + ? [ + `Only ${someRunningInstancesWere} confirmed restarted of ${allTheInstances}.`, + instancesUnanswered && describeUnansweredWrites(instancesUnanswered), + instancesUntouched !== undefined && describeUnconfirmedRestart(instancesUntouched, 'restarted'), + ].filter(excludeFalsy).join(' ') : `Failed to fully restart cluster.\n` + ([ allInstances.length === 0 && 'No instances were found within the cluster to restart.', diff --git a/src/hooks/useRestartInstanceClick.test.tsx b/src/hooks/useRestartInstanceClick.test.tsx index a9a0db51f..8562d5c72 100644 --- a/src/hooks/useRestartInstanceClick.test.tsx +++ b/src/hooks/useRestartInstanceClick.test.tsx @@ -1,6 +1,10 @@ /** @vitest-environment jsdom */ import { useRestartInstanceClick } from '@/hooks/useRestartInstanceClick'; import { waitUntilAnsweringAfterRestart } from '@/integrations/api/instance/status/restartInstance'; +import { + RESTART_UNCONFIRMED_MESSAGE, + RESTART_UNCONFIRMED_TITLE, +} from '@/integrations/api/instance/status/restartUnconfirmed'; import { OUTCOME_UNKNOWN_TITLE } from '@/integrations/api/unansweredWrite'; import { isRestarting, resetRestartTracker } from '@/lib/restart/restartTracker'; import { mutationErrorHandler } from '@/react-query/queryClient'; @@ -24,11 +28,40 @@ vi.mock('sonner', () => ({ toast: { loading: vi.fn(() => 'restart-toast'), error: vi.fn(), success: vi.fn(), dismiss: vi.fn() }, })); +vi.mock('@/lib/sleep', () => ({ + sleep: vi.fn().mockResolvedValue(true), +})); + function restartAnswering(failure: (config: InternalAxiosRequestConfig) => AxiosError) { const post = vi.fn((_url: string, data: unknown) => Promise.reject(failure({ method: 'post', data } as never))); return { post } as unknown as AxiosInstance; } +function probeTimeout(config: InternalAxiosRequestConfig) { + return new AxiosError('timeout of 3000ms exceeded', AxiosError.ECONNABORTED, config); +} + +/** + * Harper accepts the restart (bin/restart.ts answers before it tears down), then each `user_info` + * probe gets what `userInfo` returns for its 1-based attempt number. + */ +function restartAccepted(userInfo: (attempt: number, config: InternalAxiosRequestConfig) => Promise) { + let attempts = 0; + const post = vi.fn((_url: string, data: { operation: string }) => { + if (data.operation === 'restart') { + return Promise.resolve({ data: { message: 'Restarting Harper. This may take up to 60 seconds.' } }); + } + attempts += 1; + return userInfo(attempts, { method: 'post', data } as InternalAxiosRequestConfig).then((answer) => ({ + data: answer, + })); + }); + const count = (operation: string) => post.mock.calls.filter(([, data]) => data.operation === operation).length; + return { post, restarts: () => count('restart'), probes: () => count('user_info') }; +} + +const neverAnswers = (_attempt: number, config: InternalAxiosRequestConfig) => Promise.reject(probeTimeout(config)); + function render(instanceClient: AxiosInstance) { const queryClient = new QueryClient({ mutationCache: new MutationCache({ onError: mutationErrorHandler }) }); const wrapper = ({ children }: PropsWithChildren) => ( @@ -105,4 +138,75 @@ describe('useRestartInstanceClick', () => { expect(isRestarting('ins-1')).toBe(false); expect(waitUntilAnsweringAfterRestart).not.toHaveBeenCalled(); }); + + describe('a restart Harper accepted', () => { + it('says it is not back yet, never that it failed, when the target does not answer again', async () => { + const client = restartAccepted(neverAnswers); + const result = render(client as unknown as AxiosInstance); + + await act(() => result.current.onRestartClick()); + + expect(errorTitles()).toEqual([[RESTART_UNCONFIRMED_TITLE, RESTART_UNCONFIRMED_MESSAGE]]); + expect(toast.success).not.toHaveBeenCalled(); + expect(client.restarts()).toBe(1); + expect(client.probes()).toBe(24); + // Held until the watch gave up, then released: the errors that follow are real. + expect(isRestarting('ins-1')).toBe(false); + }); + + it('keeps the target marked and the button pending while it is slow to come back', async () => { + let answer: (() => void) | undefined; + const client = restartAccepted((attempt, config) => + attempt <= 12 + ? neverAnswers(attempt, config) + : new Promise((resolve) => { + answer = () => resolve({ username: 'admin' }); + }) + ); + const result = render(client as unknown as AxiosInstance); + + let click: Promise | undefined; + act(() => { + click = result.current.onRestartClick() as unknown as Promise; + }); + await vi.waitFor(() => expect(answer).toBeDefined()); + + expect(isRestarting('ins-1')).toBe(true); + expect(result.current.isRestartPending).toBe(true); + expect(toast.loading).toHaveBeenLastCalledWith('Still restarting', { + id: 'restart-toast', + description: + "Harper accepted the restart, but the instance hasn't answered yet. Still waiting for it to come back.", + }); + expect(toast.error).not.toHaveBeenCalled(); + + await act(async () => { + answer!(); + await click; + }); + + expect(isRestarting('ins-1')).toBe(false); + expect(toast.error).not.toHaveBeenCalled(); + expect(toast.success).toHaveBeenCalledWith( + 'Success', + expect.objectContaining({ description: 'Instance restarted!' }), + ); + expect(client.restarts()).toBe(1); + }); + + it('says it restarted, with no slow-return notice, when the target answers right away', async () => { + const client = restartAccepted(() => Promise.resolve({ username: 'admin' })); + const result = render(client as unknown as AxiosInstance); + + await act(() => result.current.onRestartClick()); + + expect(toast.error).not.toHaveBeenCalled(); + expect(toast.loading).not.toHaveBeenCalledWith('Still restarting', expect.anything()); + expect(toast.success).toHaveBeenCalledWith( + 'Success', + expect.objectContaining({ description: 'Instance restarted!' }), + ); + expect(isRestarting('ins-1')).toBe(false); + }); + }); }); diff --git a/src/hooks/useRestartInstanceClick.ts b/src/hooks/useRestartInstanceClick.ts index 1430005f2..efd5970a3 100644 --- a/src/hooks/useRestartInstanceClick.ts +++ b/src/hooks/useRestartInstanceClick.ts @@ -1,7 +1,11 @@ import { isLocalStudio } from '@/config/constants'; import { InstanceClientConfig } from '@/config/instanceClientConfig'; import { OverallAppSignIn } from '@/features/auth/store/authStore'; -import { useRestartInstance, waitUntilAnsweringAfterRestart } from '@/integrations/api/instance/status/restartInstance'; +import { + RESTART_MARK_TTL_MS, + useRestartInstance, + waitUntilAnsweringAfterRestart, +} from '@/integrations/api/instance/status/restartInstance'; import { isUnansweredWrite } from '@/integrations/api/unansweredWrite'; import { markRestarting } from '@/lib/restart/restartTracker'; import { invalidateEntityQueries } from '@/react-query/invalidateEntityQueries'; @@ -43,7 +47,10 @@ export function useRestartInstanceClick({ // target restarts whichever member the load balancer picked, so the cluster stays reachable. const entityId = isLocalStudio ? OverallAppSignIn : instanceId ?? clusterId; const releaseTarget = operation === 'restart' && entityId - ? markRestarting([entityId], { reach: targetNoun === 'Instance' ? 'down' : 'rolling', ttlMs: 5 * 60_000 }) + ? markRestarting([entityId], { + reach: targetNoun === 'Instance' ? 'down' : 'rolling', + ttlMs: RESTART_MARK_TTL_MS, + }) : undefined; // Await the mutation directly rather than using the mutate() callbacks. Those callbacks are // dropped if the component unmounts before the restart finishes (e.g. the user navigates to @@ -55,6 +62,12 @@ export function useRestartInstanceClick({ operation, replicated: operation === 'restart_service' && targetNoun === 'Cluster', instanceClient, + onSlowToReturn: () => + toast.loading('Still restarting', { + id: toastId, + description: `Harper accepted the restart, but the ${targetNoun.toLowerCase()} hasn't answered yet. ` + + 'Still waiting for it to come back.', + }), }); // Releasing a tracked target refetches its queries (`onRestartSettled`); invalidating it here // too would cancel those refetches and resend them. @@ -90,7 +103,8 @@ export function useRestartInstanceClick({ if (releaseTarget && holdUntilAnswering) { void waitUntilAnsweringAfterRestart({ instanceClient }).then(releaseTarget, releaseTarget); } else { - // Released on failure too: once the restart has failed, the errors that follow are real. + // Released on failure too: once the restart has failed, or the watch for its target has given up, + // the errors that follow are real. releaseTarget?.(); } } diff --git a/src/hooks/useRollingConfigUpdate.test.tsx b/src/hooks/useRollingConfigUpdate.test.tsx index 437b9d693..808435948 100644 --- a/src/hooks/useRollingConfigUpdate.test.tsx +++ b/src/hooks/useRollingConfigUpdate.test.tsx @@ -2,6 +2,7 @@ * @vitest-environment jsdom */ import type { Cluster, Instance } from '@/integrations/api/api.patch'; +import { RestartUnconfirmedError } from '@/integrations/api/instance/status/restartUnconfirmed'; import { describeUnansweredWrites, OUTCOME_UNKNOWN_TITLE } from '@/integrations/api/unansweredWrite'; import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; import { act, renderHook } from '@testing-library/react'; @@ -509,3 +510,104 @@ function cancel() { const [, options] = vi.mocked(toast.loading).mock.calls.at(-1)!; (options as unknown as { action: { onClick: () => void } }).action.onClick(); } + +/** + * Restarts record `restart ` as usual; the targets listed accept the restart but are never + * confirmed back, after saying once that they are slow. + */ +function notBackAfterRestart(...targets: string[]) { + vi.mocked(restartInstance).mockImplementation(async ({ instanceClient, onSlowToReturn }) => { + events.push(`restart ${targetOf(instanceClient)}`); + if (targets.includes(targetOf(instanceClient))) { + onSlowToReturn?.(); + throw new RestartUnconfirmedError(new AxiosError('Network Error', AxiosError.ERR_NETWORK)); + } + return { message: 'Restarting http' }; + }); +} + +function finalError() { + const [title, options] = vi.mocked(toast.error).mock.calls.at(-1) ?? []; + return { title, description: (options as { description?: string } | undefined)?.description }; +} + +describe('useRollingConfigUpdate, for a member that accepted the restart but has not answered since', () => { + it('stops there rather than update another member, and never says the update failed', async () => { + useCluster([instance('ins-a', '5.1.29'), instance('ins-b', '5.1.29')]); + notBackAfterRestart('ins-b'); + + await save(); + + // Last member first: ins-b took the config and never came back, so ins-a was not touched. + expect(events).toEqual(['check ins-b', 'write ins-b', 'restart ins-b']); + expect(toast.loading).toHaveBeenCalledWith( + 'Waiting On Instance 1 of 2', + expect.objectContaining({ description: "It accepted the restart but hasn't answered yet." }), + ); + expect(finalError()).toEqual({ + title: 'Not back yet', + description: 'Only 0 "RUNNING" instances were confirmed updated of 2 instances. ' + + "1 instance accepted the restart but hasn't answered since, so it may still be starting. " + + '1 other instance was not updated, to keep the cluster available. ' + + 'Check its status before you restart it again.', + }); + expect(toast.success).not.toHaveBeenCalled(); + }); + + it('still carries on past a member that refused the update, and says it failed', async () => { + useCluster([instance('ins-a', '5.1.29'), instance('ins-b', '5.1.29')]); + vi.mocked(setConfiguration).mockImplementation(async ({ instanceClient }) => { + events.push(`write ${targetOf(instanceClient)}`); + if (targetOf(instanceClient) === 'ins-b') { + throw new Error('Invalid configuration'); + } + return { message: 'ok' }; + }); + + await save(); + + expect(events).toEqual(['check ins-b', 'write ins-b', 'check ins-a', 'write ins-a', 'restart ins-a']); + expect(finalError().title).toBe('Error'); + expect(finalError().description).toContain('Failed to fully update cluster.'); + }); + + it('never fans the configuration out from a canary that is not back', async () => { + useCluster([instance('ins-a', '5.2.0'), instance('ins-b', '5.2.0'), instance('ins-c', '5.2.0')]); + notBackAfterRestart('ins-c'); + + await save(); + + expect(events).toEqual(['check ins-c', 'write ins-c', 'restart ins-c']); + expect(finalError().title).toBe('Not back yet'); + expect(finalError().description).not.toContain('may already have the new configuration saved'); + }); + + it('stops at a later member that is not back, and says the members it left may have 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(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' }; + }); + notBackAfterRestart('ins-b'); + + await save(); + + expect(events).toEqual([ + 'check ins-c', + 'write ins-c', + 'restart ins-c', + 'replicate ins-c', + 'check ins-b', + 'restart ins-b', + ]); + expect(finalError()).toEqual({ + title: 'Not back yet', + description: 'Only 1 "RUNNING" instance was confirmed updated of 3 instances. ' + + "1 instance accepted the restart but hasn't answered since, so it may still be starting. " + + '1 other instance was not updated, to keep the cluster available. ' + + 'Check its status before you restart it again.\n' + + 'Instances that were not restarted may already have the new configuration saved, and will apply it ' + + 'the next time they restart.', + }); + }); +}); diff --git a/src/hooks/useRollingConfigUpdate.tsx b/src/hooks/useRollingConfigUpdate.tsx index e74a23907..2c4dcab74 100644 --- a/src/hooks/useRollingConfigUpdate.tsx +++ b/src/hooks/useRollingConfigUpdate.tsx @@ -5,6 +5,11 @@ 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'; +import { + describeUnconfirmedRestart, + RESTART_UNCONFIRMED_TITLE, + RestartUnconfirmedError, +} from '@/integrations/api/instance/status/restartUnconfirmed'; import { setConfiguration, shouldFanOutSetConfiguration } from '@/integrations/api/instance/status/setConfiguration'; import { acknowledgedReplicationNodes } from '@/integrations/api/replication'; import { describeUnansweredWrites, isUnansweredWrite, OUTCOME_UNKNOWN_TITLE } from '@/integrations/api/unansweredWrite'; @@ -111,6 +116,8 @@ export function useRollingConfigUpdate( let acknowledged = new Set(); let instancesRestarted = 0; let instancesUnanswered = 0; + /** Members left alone after one accepted the restart and never came back; undefined until then. */ + let instancesUntouched: number | undefined; for (let i = 0; i < targets.length; i++) { const { instanceClient, replicationFqdn } = targets[i]; @@ -148,10 +155,22 @@ export function useRollingConfigUpdate( operation: 'restart_service', replicated: false, instanceClient, + onSlowToReturn: () => + toast.loading(`Waiting On Instance ${i + 1} of ${targets.length}`, { + ...toastConfig, + id: toastId, + description: "It accepted the restart but hasn't answered yet.", + }), }); instancesRestarted += 1; restarted = true; } catch (error) { + if (error instanceof RestartUnconfirmedError) { + // It may still be down: updating the next one could leave no member serving. A canary that is + // not back never proved it can run the configuration, so it is not fanned out either. + instancesUntouched = targets.length - i - 1; + break; + } const unanswered = isUnansweredWrite(error); instancesUnanswered += unanswered ? 1 : 0; if (i + 1 !== targets.length) { @@ -216,11 +235,19 @@ export function useRollingConfigUpdate( '"RUNNING" instance was', '"RUNNING" instances were', ); - toast.error(instancesUnanswered ? OUTCOME_UNKNOWN_TITLE : 'Error', { + const title = instancesUnanswered + ? OUTCOME_UNKNOWN_TITLE + : instancesUntouched === undefined + ? 'Error' + : RESTART_UNCONFIRMED_TITLE; + toast.error(title, { id: toastId, - description: (instancesUnanswered - ? `Only ${someRunningInstancesWere} confirmed updated of ${allTheInstances}. ` - + describeUnansweredWrites(instancesUnanswered) + description: (instancesUnanswered || instancesUntouched !== undefined + ? [ + `Only ${someRunningInstancesWere} confirmed updated of ${allTheInstances}.`, + instancesUnanswered && describeUnansweredWrites(instancesUnanswered), + instancesUntouched !== undefined && describeUnconfirmedRestart(instancesUntouched, 'updated'), + ].filter(excludeFalsy).join(' ') : `Failed to fully update cluster.\n` + ([ allInstances.length === 0 && 'No instances were found within the cluster to restart.', diff --git a/src/integrations/api/instance/status/restartInstance.test.ts b/src/integrations/api/instance/status/restartInstance.test.ts new file mode 100644 index 000000000..994c9699d --- /dev/null +++ b/src/integrations/api/instance/status/restartInstance.test.ts @@ -0,0 +1,174 @@ +/** @vitest-environment jsdom */ +import { getInstanceClient } from '@/config/getInstanceClient'; +import { restartInstance } from '@/integrations/api/instance/status/restartInstance'; +import { RestartUnconfirmedError } from '@/integrations/api/instance/status/restartUnconfirmed'; +import { isUnansweredWrite } from '@/integrations/api/unansweredWrite'; +import { shouldKeepEvent } from '@/integrations/datadog/shouldKeepEvent'; +import { AxiosError, type AxiosResponse, type InternalAxiosRequestConfig } from 'axios'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +vi.mock('@/lib/sleep', () => ({ + sleep: vi.fn().mockResolvedValue(true), +})); + +type Answer = (config: InternalAxiosRequestConfig, attempt: number) => AxiosResponse | never; + +// Harper's own answer to an accepted `restart` (bin/restart.ts RESTART_RESPONSE, wrapped as { message }). +const ACCEPTED = { message: 'Restarting Harper. This may take up to 60 seconds.' }; + +function ok(data: unknown): Answer { + return (config) => ({ status: 200, statusText: 'OK', data, headers: {}, config }); +} + +function failing(status: number, data: unknown): Answer { + return (config) => { + throw new AxiosError( + `Request failed with status code ${status}`, + status >= 500 ? AxiosError.ERR_BAD_RESPONSE : AxiosError.ERR_BAD_REQUEST, + config, + undefined, + { status, statusText: '', data, headers: {}, config }, + ); + }; +} + +function timingOut(config: InternalAxiosRequestConfig): never { + throw new AxiosError(`timeout of ${config.timeout}ms exceeded`, AxiosError.ECONNABORTED, config); +} + +function answersFrom(attempt: number): Answer { + return (config, probe) => probe < attempt ? timingOut(config) : ok({ username: 'admin' })(config, probe); +} + +const sent: string[] = []; + +function instanceClient({ restart, userInfo }: { restart: Answer; userInfo: Answer }) { + let probes = 0; + const client = getInstanceClient({ id: 'ins-restart', operationsUrl: 'https://ins-restart.example.com:9925/' }); + client.defaults.adapter = async (config: InternalAxiosRequestConfig) => { + const { operation } = JSON.parse(String(config.data)) as { operation: string }; + sent.push(operation); + return operation === 'user_info' ? userInfo(config, ++probes) : restart(config, 0); + }; + return client; +} + +const probesSent = () => sent.filter((operation) => operation === 'user_info').length; + +async function failureOf(promise: Promise): Promise { + return promise.then(() => expect.unreachable('the restart should have been rejected'), (error: unknown) => error); +} + +describe('restartInstance', () => { + beforeEach(() => { + sent.length = 0; + }); + + it('resolves with Harper’s answer once the target answers again', async () => { + const client = instanceClient({ restart: ok(ACCEPTED), userInfo: ok({ username: 'admin' }) }); + + await expect(restartInstance({ operation: 'restart', replicated: false, instanceClient: client })) + .resolves.toEqual(ACCEPTED); + expect(sent).toEqual(['restart', 'user_info']); + }); + + it('keeps watching a target that is slow to come back, and says so once', async () => { + const client = instanceClient({ restart: ok(ACCEPTED), userInfo: answersFrom(20) }); + const onSlowToReturn = vi.fn(); + + await expect(restartInstance({ operation: 'restart', replicated: false, instanceClient: client, onSlowToReturn })) + .resolves.toEqual(ACCEPTED); + expect(probesSent()).toBe(20); + expect(onSlowToReturn).toHaveBeenCalledOnce(); + }); + + it('does not call a target slow that answers within the usual time', async () => { + const client = instanceClient({ restart: ok(ACCEPTED), userInfo: answersFrom(12) }); + const onSlowToReturn = vi.fn(); + + await restartInstance({ operation: 'restart', replicated: false, instanceClient: client, onSlowToReturn }); + + expect(onSlowToReturn).not.toHaveBeenCalled(); + }); + + it('says an accepted restart is unconfirmed, not failed, when the target never answers again', async () => { + const client = instanceClient({ restart: ok(ACCEPTED), userInfo: timingOut }); + + const error = await failureOf(restartInstance({ operation: 'restart', replicated: false, instanceClient: client })); + + expect(error).toBeInstanceOf(RestartUnconfirmedError); + expect((error as RestartUnconfirmedError).cause).toBeInstanceOf(AxiosError); + expect((error as RestartUnconfirmedError).cause).toMatchObject({ code: AxiosError.ECONNABORTED }); + // Sent once: Harper accepted it, so nothing here may send it again. + expect(sent.filter((operation) => operation === 'restart')).toHaveLength(1); + expect(probesSent()).toBe(24); + }); + + it('sends each probe once, even when a gateway answers it', async () => { + const client = instanceClient({ restart: ok(ACCEPTED), userInfo: failing(503, 'Service Unavailable') }); + + const error = await failureOf(restartInstance({ operation: 'restart', replicated: false, instanceClient: client })); + + expect(error).toBeInstanceOf(RestartUnconfirmedError); + expect(probesSent()).toBe(24); + }); + + it('treats restart_service the same way: its job started, so the restart was accepted', async () => { + const client = instanceClient({ + restart: ok({ message: 'Restarting http', job_id: 'job-1' }), + userInfo: timingOut, + }); + + const error = await failureOf( + restartInstance({ operation: 'restart_service', replicated: false, instanceClient: client }), + ); + + expect(error).toBeInstanceOf(RestartUnconfirmedError); + }); + + it.each([ + ['refused', failing(403, { error: 'This operation is restricted to super users' })], + ['answered by a gateway', failing(504, '504 Gateway Time-out')], + ['never answered', timingOut], + ])('rejects with the restart request’s own error when it was %s', async (_label, restart) => { + const client = instanceClient({ restart, userInfo: ok({ username: 'admin' }) }); + + const error = await failureOf(restartInstance({ operation: 'restart', replicated: false, instanceClient: client })); + + expect(error).toBeInstanceOf(AxiosError); + expect(error).not.toBeInstanceOf(RestartUnconfirmedError); + expect(sent).not.toContain('user_info'); + }); + + it('leaves a restart that got no verdict to the unanswered-write warning', async () => { + const client = instanceClient({ restart: failing(504, 'gateway'), userInfo: ok({}) }); + + const error = await failureOf(restartInstance({ operation: 'restart', replicated: false, instanceClient: client })); + + expect(isUnansweredWrite(error)).toBe(true); + }); +}); + +describe('RestartUnconfirmedError', () => { + function consoleEvent(error: Error) { + return { type: 'error', error: { message: error.message, type: error.name, source: 'console' } }; + } + + it('reaches RUM, or not, exactly as the probe failure it wraps would', () => { + const timeout = new AxiosError('timeout of 3000ms exceeded', AxiosError.ECONNABORTED); + const network = new AxiosError('Network Error', AxiosError.ERR_NETWORK); + + for (const probeFailure of [timeout, network]) { + expect(shouldKeepEvent(consoleEvent(new RestartUnconfirmedError(probeFailure)))) + .toBe(shouldKeepEvent(consoleEvent(probeFailure))); + } + expect(shouldKeepEvent(consoleEvent(new RestartUnconfirmedError(timeout)))).toBe(false); + }); + + it.each([null, undefined, 'socket hang up', { code: 'E_SOMETHING' }])('accepts any cause (%o)', (cause) => { + const error = new RestartUnconfirmedError(cause); + + expect(error.cause).toBe(cause); + expect(error.message).toMatch(/^The restart was accepted, but the target has not answered since: /); + }); +}); diff --git a/src/integrations/api/instance/status/restartInstance.ts b/src/integrations/api/instance/status/restartInstance.ts index 4bff908ef..3c8551d83 100644 --- a/src/integrations/api/instance/status/restartInstance.ts +++ b/src/integrations/api/instance/status/restartInstance.ts @@ -1,17 +1,43 @@ import { InstanceClientConfig } from '@/config/instanceClientConfig'; import { getInstanceUserInfo } from '@/integrations/api/instance/status/getInstanceUserInfo'; +import { RestartUnconfirmedError } from '@/integrations/api/instance/status/restartUnconfirmed'; import { ReplicatedResponse } from '@/integrations/api/replication'; import { axiosRetry } from '@/lib/axiosRetry'; import { sleep } from '@/lib/sleep'; import { useMutation } from '@tanstack/react-query'; +const SETTLE_MS = 10_000; +const PROBE_TIMEOUT_MS = 3_000; +const PROBE_INTERVAL_MS = 15_000; +const PROBES = 24; +/** After this many unanswered probes the target is slow to return, and still watched. */ +const SLOW_AFTER_PROBES = 12; +const RESTART_REQUEST_TIMEOUT_MS = 60_000; + +const LONGEST_RESTART_MS = RESTART_REQUEST_TIMEOUT_MS + SETTLE_MS + PROBES * PROBE_TIMEOUT_MS + + (PROBES - 1) * PROBE_INTERVAL_MS; + +/** + * For marking a restart's target: longer than the longest restart, so the mark lapses only if its + * owner never releases it, never while the watch below still expects the target back. + */ +export const RESTART_MARK_TTL_MS = LONGEST_RESTART_MS + 60_000; + interface RestartInstanceParams { operation: 'restart_service' | 'restart'; replicated: boolean; + /** Called once the target has stayed unanswered past the usual recovery time; the watch goes on. */ + onSlowToReturn?: () => void; } +/** + * Rejects with the restart request's own error when Harper refused it or never answered it, and with + * {@link RestartUnconfirmedError} when Harper accepted it but the target never answered again. A 2xx is + * the only verdict Harper gives: `restart` answers before it tears down, `restart_service` once its job + * has started. + */ export async function restartInstance( - { operation, replicated, instanceClient }: RestartInstanceParams & InstanceClientConfig, + { operation, replicated, instanceClient, onSlowToReturn }: RestartInstanceParams & InstanceClientConfig, ): Promise { // Both calls skip the restart gate: the caller may already have marked this instance as // restarting, and these are the requests driving and watching that restart. @@ -19,19 +45,30 @@ export async function restartInstance( operation, service: operation === 'restart_service' ? 'http' : undefined, replicated, - }, { skipRestartGate: true }); - await waitUntilAnsweringAfterRestart({ instanceClient }); + }, { skipRestartGate: true, timeout: RESTART_REQUEST_TIMEOUT_MS }); + try { + await waitUntilAnsweringAfterRestart({ instanceClient, onSlowToReturn }); + } catch (cause) { + throw new RestartUnconfirmedError(cause); + } return data; } -/** Resolves once a just-restarted target answers again; rejects if it never does within ~3 minutes. */ -export async function waitUntilAnsweringAfterRestart({ instanceClient }: InstanceClientConfig): Promise { - await sleep(10_000); - await axiosRetry( - () => getInstanceUserInfo({ instanceClient, timeout: 3_000, skipRestartGate: true }), - 12, - 15_000, - ); +/** Resolves once a just-restarted target answers again; rejects with the last probe's error if it never does. */ +export async function waitUntilAnsweringAfterRestart( + { instanceClient, onSlowToReturn }: InstanceClientConfig & Pick, +): Promise { + // Not replayed on a gateway error: these probes are the retry, and a replay would outlast the mark. + const probe = () => + getInstanceUserInfo({ instanceClient, timeout: PROBE_TIMEOUT_MS, skipRestartGate: true, skipGatewayRetry: true }); + await sleep(SETTLE_MS); + try { + await axiosRetry(probe, SLOW_AFTER_PROBES, PROBE_INTERVAL_MS); + } catch { + onSlowToReturn?.(); + await sleep(PROBE_INTERVAL_MS); + await axiosRetry(probe, PROBES - SLOW_AFTER_PROBES, PROBE_INTERVAL_MS); + } } export function useRestartInstance() { diff --git a/src/integrations/api/instance/status/restartUnconfirmed.ts b/src/integrations/api/instance/status/restartUnconfirmed.ts new file mode 100644 index 000000000..e22a672e8 --- /dev/null +++ b/src/integrations/api/instance/status/restartUnconfirmed.ts @@ -0,0 +1,33 @@ +import { errorText } from '@/lib/errorText'; +import { pluralize } from '@/lib/pluralize'; + +export const RESTART_UNCONFIRMED_TITLE = 'Not back yet'; + +export const RESTART_UNCONFIRMED_MESSAGE = "Harper accepted the restart, but it hasn't answered since. " + + 'It may still be starting, so check its status before you restart it again.'; + +/** + * Harper answered the restart request with success, so the restart ran, but the target never + * answered again while Studio watched for it. Not a failed restart: sending it again would take the + * target down a second time. + */ +export class RestartUnconfirmedError extends Error { + constructor(cause: unknown) { + // The probe's text stays in the message because Datadog's `shouldKeepEvent` filters on it. + super(`The restart was accepted, but the target has not answered since: ${errorText(cause) ?? 'no answer'}`, { + cause, + }); + this.name = 'RestartUnconfirmedError'; + } +} + +/** + * For a loop that restarts one member at a time and stopped at the member that has not come back: + * `untouched` members were left alone, since restarting another could take the cluster down. + */ +export function describeUnconfirmedRestart(untouched: number, verb: 'restarted' | 'updated'): string { + const leftAlone = pluralize(untouched, 'other instance was', 'other instances were'); + return "1 instance accepted the restart but hasn't answered since, so it may still be starting. " + + (untouched > 0 ? `${leftAlone} not ${verb}, to keep the cluster available. ` : '') + + 'Check its status before you restart it again.'; +} diff --git a/src/react-query/queryClient.test.ts b/src/react-query/queryClient.test.ts index daf570a03..20c1c791f 100644 --- a/src/react-query/queryClient.test.ts +++ b/src/react-query/queryClient.test.ts @@ -1,3 +1,8 @@ +import { + RESTART_UNCONFIRMED_MESSAGE, + RESTART_UNCONFIRMED_TITLE, + RestartUnconfirmedError, +} from '@/integrations/api/instance/status/restartUnconfirmed'; import { OUTCOME_UNKNOWN_TITLE, UNANSWERED_WRITE_MESSAGE } from '@/integrations/api/unansweredWrite'; import { markRestarting, resetRestartTracker } from '@/lib/restart/restartTracker'; import { MutationObserver, QueryObserver } from '@tanstack/react-query'; @@ -426,3 +431,25 @@ describe('a write that got no verdict', () => { expect(shouldToastOwnFailure(new Error('Secret "x" not found'))).toBe(true); }); }); + +describe('a restart Harper accepted whose target has not answered since', () => { + const unconfirmed = () => + new RestartUnconfirmedError( + new AxiosError('timeout of 3000ms exceeded', AxiosError.ECONNABORTED, { + method: 'post', + data: { operation: 'user_info' }, + } as InternalAxiosRequestConfig), + ); + + it('is described as not back yet, never by the probe’s own failure', () => { + expect(describeError(unconfirmed())).toEqual({ + title: RESTART_UNCONFIRMED_TITLE, + description: RESTART_UNCONFIRMED_MESSAGE, + message: RESTART_UNCONFIRMED_MESSAGE, + }); + }); + + it('tells a mutation’s caller not to add a "failed" toast of its own', () => { + expect(shouldToastOwnFailure(unconfirmed())).toBe(false); + }); +}); diff --git a/src/react-query/queryClient.ts b/src/react-query/queryClient.ts index c4e02ca87..e7a17b98e 100644 --- a/src/react-query/queryClient.ts +++ b/src/react-query/queryClient.ts @@ -1,3 +1,8 @@ +import { + RESTART_UNCONFIRMED_MESSAGE, + RESTART_UNCONFIRMED_TITLE, + RestartUnconfirmedError, +} from '@/integrations/api/instance/status/restartUnconfirmed'; import { isUnansweredWrite, OUTCOME_UNKNOWN_TITLE, UNANSWERED_WRITE_MESSAGE } from '@/integrations/api/unansweredWrite'; import { errorText } from '@/lib/errorText'; import { isRestartNoise } from '@/lib/restart/restartGate'; @@ -19,11 +24,19 @@ import { toast } from 'sonner'; * * A write that got no verdict ({@link isUnansweredWrite}) gets our own copy instead, chosen by status * and never by body: a gateway's body describes the gateway, and "failed" is exactly what it can't claim. + * So does a restart Harper accepted whose target has not answered since ({@link RestartUnconfirmedError}). */ export function describeError(rawErr: unknown): { title: string; description: string; message: string } { if (isUnansweredWrite(rawErr)) { return { title: OUTCOME_UNKNOWN_TITLE, description: UNANSWERED_WRITE_MESSAGE, message: UNANSWERED_WRITE_MESSAGE }; } + if (rawErr instanceof RestartUnconfirmedError) { + return { + title: RESTART_UNCONFIRMED_TITLE, + description: RESTART_UNCONFIRMED_MESSAGE, + message: RESTART_UNCONFIRMED_MESSAGE, + }; + } let errorTitle = 'Error'; let errorMsg = 'We had some trouble!'; let splitTitleFromMsg = true; @@ -71,11 +84,11 @@ export function describeError(rawErr: unknown): { title: string; description: st /** * For a mutation's caller that toasts its own failure on top of the global toast: whether it should. - * Not for an unanswered write, whose global toast already says the outcome is unknown — a second one - * saying "failed" would contradict it and invite the retry it warns against. + * Not for an unanswered write or an unconfirmed restart, whose global toast already says what is known — + * a second one saying "failed" would contradict it and invite the retry it warns against. */ export function shouldToastOwnFailure(rawErr: unknown): boolean { - return !isUnansweredWrite(rawErr); + return !isUnansweredWrite(rawErr) && !(rawErr instanceof RestartUnconfirmedError); } export function errorHandler(rawErr: unknown) { From 8195f74b438474cf8c29d1c9b47fc7b0f3c32d79 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Sun, 11 Oct 2026 06:19:57 -0400 Subject: [PATCH 2/9] fix(restart): let a member not back outrank Cancel, and name refused members too Review of the previous commit: - A Cancel clicked while the loop waited on a member that never came back replaced the "Not back yet, check before you restart it again" toast with "Cancelled", hiding the one warning that matters. The uncertain outcome now wins; Cancelled still shows when nothing is uncertain. - The uncertain-outcome toast counted the members that refused only implicitly. It now says "N instances failed to restart", from one helper both loops share (describeUncertainRollingOutcome). - onSlowToReturn is queued rather than called inline, so a callback that throws cannot cut the watch short or become its cause. Not adopted: counting a 401/403 probe as the target being back. Through central manager's proxy a 403 is CM refusing the caller before it contacts the instance (notAllowed() in HDBInstance.js), so it says nothing about the target; recorded in DESIGN.md. Co-Authored-By: Claude Opus 5.5 --- DESIGN.md | 3 +- src/hooks/useRestartClusterClick.test.tsx | 47 +++++++++++++++++++ src/hooks/useRestartClusterClick.tsx | 33 ++++++------- src/hooks/useRollingConfigUpdate.test.tsx | 46 ++++++++++++++++++ src/hooks/useRollingConfigUpdate.tsx | 33 ++++++------- .../instance/status/restartInstance.test.ts | 4 ++ .../api/instance/status/restartInstance.ts | 6 ++- .../api/instance/status/restartUnconfirmed.ts | 38 +++++++++++++-- 8 files changed, 168 insertions(+), 42 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index fd75a9a1b..6f7e80324 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -81,7 +81,8 @@ The same rule decides what a failure says. [`isUnansweredWrite`](src/integration Harper's answer to the restart request is its only verdict on the restart: `restart` answers before it tears down (harper `bin/restart.ts` schedules the teardown 50ms out so the reply can go first) and `restart_service` once its job has started (`executeJob`). So [`restartInstance`](src/integrations/api/instance/status/restartInstance.ts) rejects with the request's own error only when that request failed; after a 2xx, a target that never answers the `user_info` watch rejects with [`RestartUnconfirmedError`](src/integrations/api/instance/status/restartUnconfirmed.ts), which reads "Not back yet", never "Failed", and is never a reason to send the restart again. - **The watch outlasts the usual recovery time** (one settle, then two probe schedules), and `onSlowToReturn` fires once between them so a caller can say it is still waiting. Every restart mark uses `RESTART_MARK_TTL_MS`, derived from the same constants, so no mark lapses while its watch still runs; the probes skip the gateway replay for the same reason. -- **A one-at-a-time loop stops at a member that is not back** (cluster restart, rolling config update): restarting the next one could leave no member serving. It reports the members it left alone. +- **Any failed probe counts as no answer, a 401/403 included.** Through central manager's operations proxy a 403 is CM refusing the caller before it contacts the instance (`notAllowed()` in its `HDBInstance.js`), so it says nothing about the target; reading it as "back" would report a restart Studio never saw finish. +- **A one-at-a-time loop stops at a member that is not back** (cluster restart, rolling config update): restarting the next one could leave no member serving. It reports the members it left alone, and that report outranks Cancel, which would otherwise hide the warning not to restart again. - **The chat tools return `success: true`** for it: their action was carried out, and `false` invites the agent to repeat it. ## Cluster upgrade picker — which versions an existing cluster is offered diff --git a/src/hooks/useRestartClusterClick.test.tsx b/src/hooks/useRestartClusterClick.test.tsx index 0dee9d79c..1b57e08a8 100644 --- a/src/hooks/useRestartClusterClick.test.tsx +++ b/src/hooks/useRestartClusterClick.test.tsx @@ -374,4 +374,51 @@ describe('useRestartClusterClick, for a member that accepted the restart but has expect(finalError().title).toBe('Error'); expect(finalError().description).toContain('Only 2 "RUNNING" instances were restarted'); }); + + it('names a member that refused alongside the one not back', async () => { + restartsEndingIn({ 'ins-c': refused(), 'ins-b': notBackYet() }); + + await clickRestart(); + + expect(restarted).toEqual(['ins-c', 'ins-b']); + expect(finalError()).toEqual({ + title: 'Not back yet', + description: 'Only 0 "RUNNING" instances were confirmed restarted of 3 instances. ' + + '1 instance failed to restart. ' + + "1 instance accepted the restart but hasn't answered since, so it may still be starting. " + + '1 other instance was not restarted, to keep the cluster available. ' + + 'Check its status before you restart it again.', + }); + }); + + it('still says it is not back yet when Cancel is clicked while waiting on it', async () => { + vi.mocked(restartInstance).mockImplementation(async ({ instanceClient, onSlowToReturn }) => { + restarted.push(targetOf(instanceClient)); + if (targetOf(instanceClient) === 'ins-b') { + onSlowToReturn?.(); + cancel(); + throw notBackYet(); + } + return { message: 'Restarting Harper. This may take up to 60 seconds.' }; + }); + + await clickRestart(); + + expect(restarted).toEqual(['ins-c', 'ins-b']); + expect(finalError().title).toBe('Not back yet'); + expect(vi.mocked(toast.error).mock.calls.map(([title]) => title)).not.toContain('Cancelled'); + }); + + it('still says Cancelled when nothing was left uncertain', async () => { + vi.mocked(restartInstance).mockImplementation(async ({ instanceClient }) => { + restarted.push(targetOf(instanceClient)); + cancel(); + return { message: 'Restarting Harper. This may take up to 60 seconds.' }; + }); + + await clickRestart(); + + expect(restarted).toEqual(['ins-c']); + expect(finalError().title).toBe('Cancelled'); + }); }); diff --git a/src/hooks/useRestartClusterClick.tsx b/src/hooks/useRestartClusterClick.tsx index 81b8c15be..a0fe6bfcf 100644 --- a/src/hooks/useRestartClusterClick.tsx +++ b/src/hooks/useRestartClusterClick.tsx @@ -7,11 +7,10 @@ import { waitUntilAnsweringAfterRestart, } from '@/integrations/api/instance/status/restartInstance'; import { - describeUnconfirmedRestart, - RESTART_UNCONFIRMED_TITLE, + describeUncertainRollingOutcome, RestartUnconfirmedError, } from '@/integrations/api/instance/status/restartUnconfirmed'; -import { describeUnansweredWrites, isUnansweredWrite, OUTCOME_UNKNOWN_TITLE } from '@/integrations/api/unansweredWrite'; +import { isUnansweredWrite } from '@/integrations/api/unansweredWrite'; import { excludeFalsy } from '@/lib/arrays/excludeFalsy'; import { pluralize } from '@/lib/pluralize'; import { markRestarting } from '@/lib/restart/restartTracker'; @@ -80,7 +79,7 @@ export function useRestartClusterClick( const { allInstances, targets } = plan; let instancesRestarted = 0; let instancesUnanswered = 0; - /** Members left alone after one accepted the restart and never came back; undefined until then. */ + let instancesFailed = 0; let instancesUntouched: number | undefined; // One instance at a time, so the cluster stays reachable through the others unless it has only @@ -140,6 +139,7 @@ export function useRestartClusterClick( } const unanswered = isUnansweredWrite(error); instancesUnanswered += unanswered ? 1 : 0; + instancesFailed += unanswered ? 0 : 1; if (unanswered) { // A gateway can lose the answer to a restart that ran and took this member down, so it // stays marked, and the next one waits, until it answers again or the watch gives up. @@ -170,7 +170,14 @@ export function useRestartClusterClick( releaseCluster(); } - if (canceled) { + const uncertain = describeUncertainRollingOutcome({ + total: allInstances.length, + confirmed: instancesRestarted, + failed: instancesFailed, + unanswered: instancesUnanswered, + untouched: instancesUntouched, + }, 'restarted'); + if (canceled && !uncertain) { toast.error('Cancelled', { id: toastId, description: `The restart was partially cancelled.`, @@ -198,20 +205,10 @@ export function useRestartClusterClick( '"RUNNING" instance was', '"RUNNING" instances were', ); - const title = instancesUnanswered - ? OUTCOME_UNKNOWN_TITLE - : instancesUntouched === undefined - ? 'Error' - : RESTART_UNCONFIRMED_TITLE; - toast.error(title, { + toast.error(uncertain?.title ?? 'Error', { id: toastId, - description: instancesUnanswered || instancesUntouched !== undefined - ? [ - `Only ${someRunningInstancesWere} confirmed restarted of ${allTheInstances}.`, - instancesUnanswered && describeUnansweredWrites(instancesUnanswered), - instancesUntouched !== undefined && describeUnconfirmedRestart(instancesUntouched, 'restarted'), - ].filter(excludeFalsy).join(' ') - : `Failed to fully restart cluster.\n` + description: uncertain?.description + ?? `Failed to fully restart cluster.\n` + ([ allInstances.length === 0 && 'No instances were found within the cluster to restart.', targets.length === 0 && `No instances were in a "RUNNING" state of ${allTheInstances}.`, diff --git a/src/hooks/useRollingConfigUpdate.test.tsx b/src/hooks/useRollingConfigUpdate.test.tsx index 808435948..7963f0939 100644 --- a/src/hooks/useRollingConfigUpdate.test.tsx +++ b/src/hooks/useRollingConfigUpdate.test.tsx @@ -610,4 +610,50 @@ describe('useRollingConfigUpdate, for a member that accepted the restart but has + 'the next time they restart.', }); }); + + it('still says it is not back yet when Cancel is clicked while waiting on it', async () => { + useCluster([instance('ins-a', '5.1.29'), instance('ins-b', '5.1.29')]); + vi.mocked(restartInstance).mockImplementation(async ({ instanceClient, onSlowToReturn }) => { + events.push(`restart ${targetOf(instanceClient)}`); + onSlowToReturn?.(); + cancel(); + throw new RestartUnconfirmedError(new AxiosError('Network Error', AxiosError.ERR_NETWORK)); + }); + + await save(); + + expect(events).toEqual(['check ins-b', 'write ins-b', 'restart ins-b']); + expect(finalError().title).toBe('Not back yet'); + expect(vi.mocked(toast.error).mock.calls.map(([title]) => title)).not.toContain('Cancelled'); + }); + + it('keeps the pending-configuration note when Cancel lands while waiting on a member after the fan-out', async () => { + useCluster([instance('ins-a', '5.2.0'), 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' }; + }); + vi.mocked(restartInstance).mockImplementation(async ({ instanceClient, onSlowToReturn }) => { + events.push(`restart ${targetOf(instanceClient)}`); + if (targetOf(instanceClient) === 'ins-a') { + onSlowToReturn?.(); + cancel(); + throw new RestartUnconfirmedError(new AxiosError('Network Error', AxiosError.ERR_NETWORK)); + } + return { message: 'Restarting http' }; + }); + + await save(); + + expect(events).toEqual([ + 'check ins-b', + 'write ins-b', + 'restart ins-b', + 'replicate ins-b', + 'check ins-a', + 'restart ins-a', + ]); + expect(finalError().title).toBe('Not back yet'); + expect(finalError().description).toContain('may already have the new configuration saved'); + }); }); diff --git a/src/hooks/useRollingConfigUpdate.tsx b/src/hooks/useRollingConfigUpdate.tsx index 2c4dcab74..6c67804a0 100644 --- a/src/hooks/useRollingConfigUpdate.tsx +++ b/src/hooks/useRollingConfigUpdate.tsx @@ -6,13 +6,12 @@ import { Instance } from '@/integrations/api/api.patch'; import { getInstanceUserInfo } from '@/integrations/api/instance/status/getInstanceUserInfo'; import { restartInstance } from '@/integrations/api/instance/status/restartInstance'; import { - describeUnconfirmedRestart, - RESTART_UNCONFIRMED_TITLE, + describeUncertainRollingOutcome, RestartUnconfirmedError, } from '@/integrations/api/instance/status/restartUnconfirmed'; import { setConfiguration, shouldFanOutSetConfiguration } from '@/integrations/api/instance/status/setConfiguration'; import { acknowledgedReplicationNodes } from '@/integrations/api/replication'; -import { describeUnansweredWrites, isUnansweredWrite, OUTCOME_UNKNOWN_TITLE } from '@/integrations/api/unansweredWrite'; +import { isUnansweredWrite } from '@/integrations/api/unansweredWrite'; import { excludeFalsy } from '@/lib/arrays/excludeFalsy'; import { pluralize } from '@/lib/pluralize'; import { sleep } from '@/lib/sleep'; @@ -116,7 +115,7 @@ export function useRollingConfigUpdate( let acknowledged = new Set(); let instancesRestarted = 0; let instancesUnanswered = 0; - /** Members left alone after one accepted the restart and never came back; undefined until then. */ + let instancesFailed = 0; let instancesUntouched: number | undefined; for (let i = 0; i < targets.length; i++) { @@ -173,6 +172,7 @@ export function useRollingConfigUpdate( } const unanswered = isUnansweredWrite(error); instancesUnanswered += unanswered ? 1 : 0; + instancesFailed += unanswered ? 0 : 1; if (i + 1 !== targets.length) { // If it fails to restart, or wasn't available, warn for a bit then move on. const verdict = unanswered ? 'Could Not Confirm' : 'Failed Restarting'; @@ -207,7 +207,14 @@ export function useRollingConfigUpdate( } } - if (canceled) { + const uncertain = describeUncertainRollingOutcome({ + total: allInstances.length, + confirmed: instancesRestarted, + failed: instancesFailed, + unanswered: instancesUnanswered, + untouched: instancesUntouched, + }, 'updated'); + if (canceled && !uncertain) { toast.error('Cancelled', { id: toastId, description: `The config update was partially cancelled.${fanOutAttempted ? ` ${MAY_BE_PENDING}` : ''}`, @@ -235,20 +242,10 @@ export function useRollingConfigUpdate( '"RUNNING" instance was', '"RUNNING" instances were', ); - const title = instancesUnanswered - ? OUTCOME_UNKNOWN_TITLE - : instancesUntouched === undefined - ? 'Error' - : RESTART_UNCONFIRMED_TITLE; - toast.error(title, { + toast.error(uncertain?.title ?? 'Error', { id: toastId, - description: (instancesUnanswered || instancesUntouched !== undefined - ? [ - `Only ${someRunningInstancesWere} confirmed updated of ${allTheInstances}.`, - instancesUnanswered && describeUnansweredWrites(instancesUnanswered), - instancesUntouched !== undefined && describeUnconfirmedRestart(instancesUntouched, 'updated'), - ].filter(excludeFalsy).join(' ') - : `Failed to fully update cluster.\n` + description: (uncertain?.description + ?? `Failed to fully update cluster.\n` + ([ allInstances.length === 0 && 'No instances were found within the cluster to restart.', targets.length === 0 && `No instances were in a "RUNNING" state of ${allTheInstances}.`, diff --git a/src/integrations/api/instance/status/restartInstance.test.ts b/src/integrations/api/instance/status/restartInstance.test.ts index 994c9699d..c8a7614e1 100644 --- a/src/integrations/api/instance/status/restartInstance.test.ts +++ b/src/integrations/api/instance/status/restartInstance.test.ts @@ -4,6 +4,7 @@ import { restartInstance } from '@/integrations/api/instance/status/restartInsta import { RestartUnconfirmedError } from '@/integrations/api/instance/status/restartUnconfirmed'; import { isUnansweredWrite } from '@/integrations/api/unansweredWrite'; import { shouldKeepEvent } from '@/integrations/datadog/shouldKeepEvent'; +import { sleep } from '@/lib/sleep'; import { AxiosError, type AxiosResponse, type InternalAxiosRequestConfig } from 'axios'; import { beforeEach, describe, expect, it, vi } from 'vitest'; @@ -62,6 +63,7 @@ async function failureOf(promise: Promise): Promise { describe('restartInstance', () => { beforeEach(() => { sent.length = 0; + vi.mocked(sleep).mockClear(); }); it('resolves with Harper’s answer once the target answers again', async () => { @@ -102,6 +104,8 @@ describe('restartInstance', () => { // Sent once: Harper accepted it, so nothing here may send it again. expect(sent.filter((operation) => operation === 'restart')).toHaveLength(1); expect(probesSent()).toBe(24); + // One settle, then every probe 15s after the last: the schedule RESTART_MARK_TTL_MS is derived from. + expect(vi.mocked(sleep).mock.calls.map(([ms]) => ms)).toEqual([10_000, ...Array(23).fill(15_000)]); }); it('sends each probe once, even when a gateway answers it', async () => { diff --git a/src/integrations/api/instance/status/restartInstance.ts b/src/integrations/api/instance/status/restartInstance.ts index 3c8551d83..27654c8a4 100644 --- a/src/integrations/api/instance/status/restartInstance.ts +++ b/src/integrations/api/instance/status/restartInstance.ts @@ -10,7 +10,6 @@ const SETTLE_MS = 10_000; const PROBE_TIMEOUT_MS = 3_000; const PROBE_INTERVAL_MS = 15_000; const PROBES = 24; -/** After this many unanswered probes the target is slow to return, and still watched. */ const SLOW_AFTER_PROBES = 12; const RESTART_REQUEST_TIMEOUT_MS = 60_000; @@ -65,7 +64,10 @@ export async function waitUntilAnsweringAfterRestart( try { await axiosRetry(probe, SLOW_AFTER_PROBES, PROBE_INTERVAL_MS); } catch { - onSlowToReturn?.(); + // Queued, so a callback that throws cannot cut the watch short. + if (onSlowToReturn) { + queueMicrotask(onSlowToReturn); + } await sleep(PROBE_INTERVAL_MS); await axiosRetry(probe, PROBES - SLOW_AFTER_PROBES, PROBE_INTERVAL_MS); } diff --git a/src/integrations/api/instance/status/restartUnconfirmed.ts b/src/integrations/api/instance/status/restartUnconfirmed.ts index e22a672e8..f61850a7c 100644 --- a/src/integrations/api/instance/status/restartUnconfirmed.ts +++ b/src/integrations/api/instance/status/restartUnconfirmed.ts @@ -1,3 +1,5 @@ +import { describeUnansweredWrites, OUTCOME_UNKNOWN_TITLE } from '@/integrations/api/unansweredWrite'; +import { excludeFalsy } from '@/lib/arrays/excludeFalsy'; import { errorText } from '@/lib/errorText'; import { pluralize } from '@/lib/pluralize'; @@ -21,11 +23,41 @@ export class RestartUnconfirmedError extends Error { } } +export interface RollingOutcome { + total: number; + confirmed: number; + failed: number; + unanswered: number; + /** Set when a member never came back and the loop stopped there: the members after it, left alone. */ + untouched?: number; +} + /** - * For a loop that restarts one member at a time and stopped at the member that has not come back: - * `untouched` members were left alone, since restarting another could take the cluster down. + * The final toast of a loop that restarts one member at a time, when some member's outcome is not + * known; undefined when every outcome is. */ -export function describeUnconfirmedRestart(untouched: number, verb: 'restarted' | 'updated'): string { +export function describeUncertainRollingOutcome( + { total, confirmed, failed, unanswered, untouched }: RollingOutcome, + verb: 'restarted' | 'updated', +): { title: string; description: string } | undefined { + if (!unanswered && untouched === undefined) { + return undefined; + } + const confirmedOf = `${pluralize(confirmed, '"RUNNING" instance was', '"RUNNING" instances were')} confirmed ${verb} ` + + `of ${pluralize(total, 'instance', 'instances')}.`; + return { + title: unanswered ? OUTCOME_UNKNOWN_TITLE : RESTART_UNCONFIRMED_TITLE, + description: [ + `Only ${confirmedOf}`, + failed > 0 + && `${pluralize(failed, 'instance', 'instances')} failed to ${verb === 'restarted' ? 'restart' : 'update'}.`, + unanswered > 0 && describeUnansweredWrites(unanswered), + untouched !== undefined && describeNoReturn(untouched, verb), + ].filter(excludeFalsy).join(' '), + }; +} + +function describeNoReturn(untouched: number, verb: 'restarted' | 'updated'): string { const leftAlone = pluralize(untouched, 'other instance was', 'other instances were'); return "1 instance accepted the restart but hasn't answered since, so it may still be starting. " + (untouched > 0 ? `${leftAlone} not ${verb}, to keep the cluster available. ` : '') From 68e91023ce41d3d0967afab2b4e7a5e7081648a6 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Sun, 11 Oct 2026 06:31:24 -0400 Subject: [PATCH 3/9] fix(restart): say Studio couldn't confirm the target came back, not that it never answered Review round 2: - A probe can fail with an answer (a 401/403, a Harper 500), so "hasn't answered since" was not always true. Every surface now says Studio couldn't confirm the target came back, which is what a failed watch proves. The watch itself still counts only a successful user_info as back: through central manager's proxy a 403 is CM refusing the caller (notAllowed() in HDBInstance.js) before it contacts the instance. - A throwing onSlowToReturn is caught and logged instead of escaping as an uncaught microtask exception; the watch carries on either way. - RollingOutcome is no longer exported; it has no consumer outside its module. Not adopted: dismissing the button's loading toast for an unconfirmed restart. It already is: the catch dismisses it before deciding whether to add its own toast. A test now pins that. Co-Authored-By: Claude Opus 5.5 --- DESIGN.md | 2 +- .../Chat/tools/createApp/execute.ts | 6 +++--- .../Chat/tools/restartHTTPService/execute.ts | 5 +++-- src/hooks/useRestartClusterClick.test.tsx | 10 +++++----- src/hooks/useRestartClusterClick.tsx | 2 +- src/hooks/useRestartInstanceClick.test.tsx | 4 ++-- src/hooks/useRestartInstanceClick.ts | 4 ++-- src/hooks/useRollingConfigUpdate.test.tsx | 8 ++++---- src/hooks/useRollingConfigUpdate.tsx | 2 +- .../instance/status/restartInstance.test.ts | 20 ++++++++++++++++++- .../api/instance/status/restartInstance.ts | 15 +++++++++----- .../api/instance/status/restartUnconfirmed.ts | 14 ++++++------- src/react-query/queryClient.test.ts | 2 +- src/react-query/queryClient.ts | 2 +- 14 files changed, 60 insertions(+), 36 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index 6f7e80324..5792e0286 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -81,7 +81,7 @@ The same rule decides what a failure says. [`isUnansweredWrite`](src/integration Harper's answer to the restart request is its only verdict on the restart: `restart` answers before it tears down (harper `bin/restart.ts` schedules the teardown 50ms out so the reply can go first) and `restart_service` once its job has started (`executeJob`). So [`restartInstance`](src/integrations/api/instance/status/restartInstance.ts) rejects with the request's own error only when that request failed; after a 2xx, a target that never answers the `user_info` watch rejects with [`RestartUnconfirmedError`](src/integrations/api/instance/status/restartUnconfirmed.ts), which reads "Not back yet", never "Failed", and is never a reason to send the restart again. - **The watch outlasts the usual recovery time** (one settle, then two probe schedules), and `onSlowToReturn` fires once between them so a caller can say it is still waiting. Every restart mark uses `RESTART_MARK_TTL_MS`, derived from the same constants, so no mark lapses while its watch still runs; the probes skip the gateway replay for the same reason. -- **Any failed probe counts as no answer, a 401/403 included.** Through central manager's operations proxy a 403 is CM refusing the caller before it contacts the instance (`notAllowed()` in its `HDBInstance.js`), so it says nothing about the target; reading it as "back" would report a restart Studio never saw finish. +- **Only a successful probe counts as back, never a 401/403.** Through central manager's operations proxy a 403 is CM refusing the caller before it contacts the instance (`notAllowed()` in its `HDBInstance.js`), so it says nothing about the target; reading it as "back" would report a restart Studio never saw finish. That is also why the copy says Studio "couldn't confirm it came back", not that the target never answered. - **A one-at-a-time loop stops at a member that is not back** (cluster restart, rolling config update): restarting the next one could leave no member serving. It reports the members it left alone, and that report outranks Cancel, which would otherwise hide the warning not to restart again. - **The chat tools return `success: true`** for it: their action was carried out, and `false` invites the agent to repeat it. diff --git a/src/features/instance/applications/components/Chat/tools/createApp/execute.ts b/src/features/instance/applications/components/Chat/tools/createApp/execute.ts index d8e21a9aa..b8bdf84f0 100644 --- a/src/features/instance/applications/components/Chat/tools/createApp/execute.ts +++ b/src/features/instance/applications/components/Chat/tools/createApp/execute.ts @@ -66,9 +66,9 @@ export async function execute( // The app exists either way; `success: false` would invite creating it again. return { success: true, - message: `App "${name}" created. Harper accepted the restart that loads it, but the instance hasn't ` - + "answered since, so it may still be starting. Don't create the app or restart again until its " - + 'status shows it is back.', + message: `App "${name}" created. Harper accepted the restart that loads it, but Studio couldn't confirm ` + + "the instance came back, so it may still be starting. Don't create the app or restart again until " + + 'its status shows it is back.', webURL: baseURL, }; } diff --git a/src/features/instance/applications/components/Chat/tools/restartHTTPService/execute.ts b/src/features/instance/applications/components/Chat/tools/restartHTTPService/execute.ts index 2ad230ca4..d2e5abb91 100644 --- a/src/features/instance/applications/components/Chat/tools/restartHTTPService/execute.ts +++ b/src/features/instance/applications/components/Chat/tools/restartHTTPService/execute.ts @@ -27,8 +27,9 @@ export async function execute( // `success` is whether the tool's own action was carried out, and Harper accepted the restart. return { success: true, - message: "Harper accepted the restart, but the HTTP service hasn't answered since, so it may still be " - + "starting. Don't restart it again until its status shows it is back, or it goes down a second time.", + message: "Harper accepted the restart, but Studio couldn't confirm the HTTP service came back, so it may " + + "still be starting. Don't restart it again until its status shows it is back, or it goes down a " + + 'second time.', webURL: baseURL, }; } diff --git a/src/hooks/useRestartClusterClick.test.tsx b/src/hooks/useRestartClusterClick.test.tsx index 1b57e08a8..508452f44 100644 --- a/src/hooks/useRestartClusterClick.test.tsx +++ b/src/hooks/useRestartClusterClick.test.tsx @@ -298,7 +298,7 @@ describe('useRestartClusterClick, for a member whose restart got no answer', () }); }); -describe('useRestartClusterClick, for a member that accepted the restart but has not answered since', () => { +describe('useRestartClusterClick, for a member that accepted the restart but was never confirmed back', () => { beforeEach(() => { vi.mocked(getClusterInfo).mockResolvedValue( { id: 'clu-1', instances: [instance('ins-a'), instance('ins-b'), instance('ins-c')] } as Cluster, @@ -314,12 +314,12 @@ describe('useRestartClusterClick, for a member that accepted the restart but has expect(restarted).toEqual(['ins-c', 'ins-b']); expect(toast.loading).toHaveBeenCalledWith( 'Waiting On Instance 2 of 3', - expect.objectContaining({ description: "It accepted the restart but hasn't answered yet." }), + expect.objectContaining({ description: "It accepted the restart but isn't back yet." }), ); expect(finalError()).toEqual({ title: 'Not back yet', description: 'Only 1 "RUNNING" instance was confirmed restarted of 3 instances. ' - + "1 instance accepted the restart but hasn't answered since, so it may still be starting. " + + "1 instance accepted the restart but couldn't be confirmed back, so it may still be starting. " + '1 other instance was not restarted, to keep the cluster available. ' + 'Check its status before you restart it again.', }); @@ -336,7 +336,7 @@ describe('useRestartClusterClick, for a member that accepted the restart but has expect(restarted).toEqual(['ins-c', 'ins-b', 'ins-a']); expect(finalError().description).toBe( 'Only 2 "RUNNING" instances were confirmed restarted of 3 instances. ' - + "1 instance accepted the restart but hasn't answered since, so it may still be starting. " + + "1 instance accepted the restart but couldn't be confirmed back, so it may still be starting. " + 'Check its status before you restart it again.', ); }); @@ -385,7 +385,7 @@ describe('useRestartClusterClick, for a member that accepted the restart but has title: 'Not back yet', description: 'Only 0 "RUNNING" instances were confirmed restarted of 3 instances. ' + '1 instance failed to restart. ' - + "1 instance accepted the restart but hasn't answered since, so it may still be starting. " + + "1 instance accepted the restart but couldn't be confirmed back, so it may still be starting. " + '1 other instance was not restarted, to keep the cluster available. ' + 'Check its status before you restart it again.', }); diff --git a/src/hooks/useRestartClusterClick.tsx b/src/hooks/useRestartClusterClick.tsx index a0fe6bfcf..77c48d0e2 100644 --- a/src/hooks/useRestartClusterClick.tsx +++ b/src/hooks/useRestartClusterClick.tsx @@ -127,7 +127,7 @@ export function useRestartClusterClick( toast.loading(`Waiting On Instance ${i + 1} of ${targets.length}`, { ...toastConfig, id: toastId, - description: "It accepted the restart but hasn't answered yet.", + description: "It accepted the restart but isn't back yet.", }), }); instancesRestarted += 1; diff --git a/src/hooks/useRestartInstanceClick.test.tsx b/src/hooks/useRestartInstanceClick.test.tsx index 8562d5c72..049ac62b2 100644 --- a/src/hooks/useRestartInstanceClick.test.tsx +++ b/src/hooks/useRestartInstanceClick.test.tsx @@ -147,6 +147,7 @@ describe('useRestartInstanceClick', () => { await act(() => result.current.onRestartClick()); expect(errorTitles()).toEqual([[RESTART_UNCONFIRMED_TITLE, RESTART_UNCONFIRMED_MESSAGE]]); + expect(toast.dismiss).toHaveBeenCalledWith('restart-toast'); expect(toast.success).not.toHaveBeenCalled(); expect(client.restarts()).toBe(1); expect(client.probes()).toBe(24); @@ -175,8 +176,7 @@ describe('useRestartInstanceClick', () => { expect(result.current.isRestartPending).toBe(true); expect(toast.loading).toHaveBeenLastCalledWith('Still restarting', { id: 'restart-toast', - description: - "Harper accepted the restart, but the instance hasn't answered yet. Still waiting for it to come back.", + description: "Harper accepted the restart, but the instance isn't back yet. Still waiting for it.", }); expect(toast.error).not.toHaveBeenCalled(); diff --git a/src/hooks/useRestartInstanceClick.ts b/src/hooks/useRestartInstanceClick.ts index efd5970a3..578bb240a 100644 --- a/src/hooks/useRestartInstanceClick.ts +++ b/src/hooks/useRestartInstanceClick.ts @@ -65,8 +65,8 @@ export function useRestartInstanceClick({ onSlowToReturn: () => toast.loading('Still restarting', { id: toastId, - description: `Harper accepted the restart, but the ${targetNoun.toLowerCase()} hasn't answered yet. ` - + 'Still waiting for it to come back.', + description: `Harper accepted the restart, but the ${targetNoun.toLowerCase()} isn't back yet. ` + + 'Still waiting for it.', }), }); // Releasing a tracked target refetches its queries (`onRestartSettled`); invalidating it here diff --git a/src/hooks/useRollingConfigUpdate.test.tsx b/src/hooks/useRollingConfigUpdate.test.tsx index 7963f0939..534290aa4 100644 --- a/src/hooks/useRollingConfigUpdate.test.tsx +++ b/src/hooks/useRollingConfigUpdate.test.tsx @@ -531,7 +531,7 @@ function finalError() { return { title, description: (options as { description?: string } | undefined)?.description }; } -describe('useRollingConfigUpdate, for a member that accepted the restart but has not answered since', () => { +describe('useRollingConfigUpdate, for a member that accepted the restart but was never confirmed back', () => { it('stops there rather than update another member, and never says the update failed', async () => { useCluster([instance('ins-a', '5.1.29'), instance('ins-b', '5.1.29')]); notBackAfterRestart('ins-b'); @@ -542,12 +542,12 @@ describe('useRollingConfigUpdate, for a member that accepted the restart but has expect(events).toEqual(['check ins-b', 'write ins-b', 'restart ins-b']); expect(toast.loading).toHaveBeenCalledWith( 'Waiting On Instance 1 of 2', - expect.objectContaining({ description: "It accepted the restart but hasn't answered yet." }), + expect.objectContaining({ description: "It accepted the restart but isn't back yet." }), ); expect(finalError()).toEqual({ title: 'Not back yet', description: 'Only 0 "RUNNING" instances were confirmed updated of 2 instances. ' - + "1 instance accepted the restart but hasn't answered since, so it may still be starting. " + + "1 instance accepted the restart but couldn't be confirmed back, so it may still be starting. " + '1 other instance was not updated, to keep the cluster available. ' + 'Check its status before you restart it again.', }); @@ -603,7 +603,7 @@ describe('useRollingConfigUpdate, for a member that accepted the restart but has expect(finalError()).toEqual({ title: 'Not back yet', description: 'Only 1 "RUNNING" instance was confirmed updated of 3 instances. ' - + "1 instance accepted the restart but hasn't answered since, so it may still be starting. " + + "1 instance accepted the restart but couldn't be confirmed back, so it may still be starting. " + '1 other instance was not updated, to keep the cluster available. ' + 'Check its status before you restart it again.\n' + 'Instances that were not restarted may already have the new configuration saved, and will apply it ' diff --git a/src/hooks/useRollingConfigUpdate.tsx b/src/hooks/useRollingConfigUpdate.tsx index 6c67804a0..cfa5dc8c7 100644 --- a/src/hooks/useRollingConfigUpdate.tsx +++ b/src/hooks/useRollingConfigUpdate.tsx @@ -158,7 +158,7 @@ export function useRollingConfigUpdate( toast.loading(`Waiting On Instance ${i + 1} of ${targets.length}`, { ...toastConfig, id: toastId, - description: "It accepted the restart but hasn't answered yet.", + description: "It accepted the restart but isn't back yet.", }), }); instancesRestarted += 1; diff --git a/src/integrations/api/instance/status/restartInstance.test.ts b/src/integrations/api/instance/status/restartInstance.test.ts index c8a7614e1..a194f5c1a 100644 --- a/src/integrations/api/instance/status/restartInstance.test.ts +++ b/src/integrations/api/instance/status/restartInstance.test.ts @@ -84,6 +84,24 @@ describe('restartInstance', () => { expect(onSlowToReturn).toHaveBeenCalledOnce(); }); + it('keeps watching when the slow-return callback throws', async () => { + const client = instanceClient({ restart: ok(ACCEPTED), userInfo: answersFrom(20) }); + const broken = new Error('toast unavailable'); + const consoleError = vi.spyOn(console, 'error').mockImplementation(() => undefined); + + await expect(restartInstance({ + operation: 'restart', + replicated: false, + instanceClient: client, + onSlowToReturn: () => { + throw broken; + }, + })).resolves.toEqual(ACCEPTED); + expect(probesSent()).toBe(20); + expect(consoleError).toHaveBeenCalledWith(broken); + consoleError.mockRestore(); + }); + it('does not call a target slow that answers within the usual time', async () => { const client = instanceClient({ restart: ok(ACCEPTED), userInfo: answersFrom(12) }); const onSlowToReturn = vi.fn(); @@ -173,6 +191,6 @@ describe('RestartUnconfirmedError', () => { const error = new RestartUnconfirmedError(cause); expect(error.cause).toBe(cause); - expect(error.message).toMatch(/^The restart was accepted, but the target has not answered since: /); + expect(error.message).toMatch(/^The restart was accepted, but the target was not confirmed back: /); }); }); diff --git a/src/integrations/api/instance/status/restartInstance.ts b/src/integrations/api/instance/status/restartInstance.ts index 27654c8a4..675c0cba6 100644 --- a/src/integrations/api/instance/status/restartInstance.ts +++ b/src/integrations/api/instance/status/restartInstance.ts @@ -25,7 +25,7 @@ export const RESTART_MARK_TTL_MS = LONGEST_RESTART_MS + 60_000; interface RestartInstanceParams { operation: 'restart_service' | 'restart'; replicated: boolean; - /** Called once the target has stayed unanswered past the usual recovery time; the watch goes on. */ + /** Called once the target is still not back past the usual recovery time; the watch goes on. */ onSlowToReturn?: () => void; } @@ -53,7 +53,10 @@ export async function restartInstance( return data; } -/** Resolves once a just-restarted target answers again; rejects with the last probe's error if it never does. */ +/** + * Resolves once a just-restarted target answers `user_info`; rejects with the last probe's error if it never + * does. Only a success counts: through central manager's proxy even a 401/403 is CM's answer, not the target's. + */ export async function waitUntilAnsweringAfterRestart( { instanceClient, onSlowToReturn }: InstanceClientConfig & Pick, ): Promise { @@ -64,9 +67,11 @@ export async function waitUntilAnsweringAfterRestart( try { await axiosRetry(probe, SLOW_AFTER_PROBES, PROBE_INTERVAL_MS); } catch { - // Queued, so a callback that throws cannot cut the watch short. - if (onSlowToReturn) { - queueMicrotask(onSlowToReturn); + try { + onSlowToReturn?.(); + } catch (error) { + // A broken progress callback must not cut the watch short. + console.error(error); } await sleep(PROBE_INTERVAL_MS); await axiosRetry(probe, PROBES - SLOW_AFTER_PROBES, PROBE_INTERVAL_MS); diff --git a/src/integrations/api/instance/status/restartUnconfirmed.ts b/src/integrations/api/instance/status/restartUnconfirmed.ts index f61850a7c..39ea436c4 100644 --- a/src/integrations/api/instance/status/restartUnconfirmed.ts +++ b/src/integrations/api/instance/status/restartUnconfirmed.ts @@ -5,25 +5,25 @@ import { pluralize } from '@/lib/pluralize'; export const RESTART_UNCONFIRMED_TITLE = 'Not back yet'; -export const RESTART_UNCONFIRMED_MESSAGE = "Harper accepted the restart, but it hasn't answered since. " +export const RESTART_UNCONFIRMED_MESSAGE = "Harper accepted the restart, but Studio couldn't confirm it came back. " + 'It may still be starting, so check its status before you restart it again.'; /** - * Harper answered the restart request with success, so the restart ran, but the target never - * answered again while Studio watched for it. Not a failed restart: sending it again would take the - * target down a second time. + * Harper answered the restart request with success, so the restart ran, but Studio never saw the + * target answer again while it watched. Not a failed restart: sending it again would take the target + * down a second time. */ export class RestartUnconfirmedError extends Error { constructor(cause: unknown) { // The probe's text stays in the message because Datadog's `shouldKeepEvent` filters on it. - super(`The restart was accepted, but the target has not answered since: ${errorText(cause) ?? 'no answer'}`, { + super(`The restart was accepted, but the target was not confirmed back: ${errorText(cause) ?? 'no answer'}`, { cause, }); this.name = 'RestartUnconfirmedError'; } } -export interface RollingOutcome { +interface RollingOutcome { total: number; confirmed: number; failed: number; @@ -59,7 +59,7 @@ export function describeUncertainRollingOutcome( function describeNoReturn(untouched: number, verb: 'restarted' | 'updated'): string { const leftAlone = pluralize(untouched, 'other instance was', 'other instances were'); - return "1 instance accepted the restart but hasn't answered since, so it may still be starting. " + return "1 instance accepted the restart but couldn't be confirmed back, so it may still be starting. " + (untouched > 0 ? `${leftAlone} not ${verb}, to keep the cluster available. ` : '') + 'Check its status before you restart it again.'; } diff --git a/src/react-query/queryClient.test.ts b/src/react-query/queryClient.test.ts index 20c1c791f..63503afc5 100644 --- a/src/react-query/queryClient.test.ts +++ b/src/react-query/queryClient.test.ts @@ -432,7 +432,7 @@ describe('a write that got no verdict', () => { }); }); -describe('a restart Harper accepted whose target has not answered since', () => { +describe('a restart Harper accepted whose target was never confirmed back', () => { const unconfirmed = () => new RestartUnconfirmedError( new AxiosError('timeout of 3000ms exceeded', AxiosError.ECONNABORTED, { diff --git a/src/react-query/queryClient.ts b/src/react-query/queryClient.ts index e7a17b98e..8185d0157 100644 --- a/src/react-query/queryClient.ts +++ b/src/react-query/queryClient.ts @@ -24,7 +24,7 @@ import { toast } from 'sonner'; * * A write that got no verdict ({@link isUnansweredWrite}) gets our own copy instead, chosen by status * and never by body: a gateway's body describes the gateway, and "failed" is exactly what it can't claim. - * So does a restart Harper accepted whose target has not answered since ({@link RestartUnconfirmedError}). + * So does a restart Harper accepted whose target was never confirmed back ({@link RestartUnconfirmedError}). */ export function describeError(rawErr: unknown): { title: string; description: string; message: string } { if (isUnansweredWrite(rawErr)) { From 14a6ee86e66043ac9979c81dafcb81b829e098ed Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Sun, 11 Oct 2026 06:40:36 -0400 Subject: [PATCH 4/9] docs(restart): say the watch never saw the target back, not that it never answered Review round 3: DESIGN.md and restartInstance's doc comment still said the target "never answers", which contradicts the rule that only a successful user_info counts as back. Co-Authored-By: Claude Opus 5.5 --- DESIGN.md | 2 +- src/integrations/api/instance/status/restartInstance.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index 5792e0286..180dd1649 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -78,7 +78,7 @@ The same rule decides what a failure says. [`isUnansweredWrite`](src/integration ## A restart Harper accepted never reads as failed -Harper's answer to the restart request is its only verdict on the restart: `restart` answers before it tears down (harper `bin/restart.ts` schedules the teardown 50ms out so the reply can go first) and `restart_service` once its job has started (`executeJob`). So [`restartInstance`](src/integrations/api/instance/status/restartInstance.ts) rejects with the request's own error only when that request failed; after a 2xx, a target that never answers the `user_info` watch rejects with [`RestartUnconfirmedError`](src/integrations/api/instance/status/restartUnconfirmed.ts), which reads "Not back yet", never "Failed", and is never a reason to send the restart again. +Harper's answer to the restart request is its only verdict on the restart: `restart` answers before it tears down (harper `bin/restart.ts` schedules the teardown 50ms out so the reply can go first) and `restart_service` once its job has started (`executeJob`). So [`restartInstance`](src/integrations/api/instance/status/restartInstance.ts) rejects with the request's own error only when that request failed; after a 2xx, a target Studio never sees answer the `user_info` watch successfully rejects with [`RestartUnconfirmedError`](src/integrations/api/instance/status/restartUnconfirmed.ts), which reads "Not back yet", never "Failed", and is never a reason to send the restart again. - **The watch outlasts the usual recovery time** (one settle, then two probe schedules), and `onSlowToReturn` fires once between them so a caller can say it is still waiting. Every restart mark uses `RESTART_MARK_TTL_MS`, derived from the same constants, so no mark lapses while its watch still runs; the probes skip the gateway replay for the same reason. - **Only a successful probe counts as back, never a 401/403.** Through central manager's operations proxy a 403 is CM refusing the caller before it contacts the instance (`notAllowed()` in its `HDBInstance.js`), so it says nothing about the target; reading it as "back" would report a restart Studio never saw finish. That is also why the copy says Studio "couldn't confirm it came back", not that the target never answered. diff --git a/src/integrations/api/instance/status/restartInstance.ts b/src/integrations/api/instance/status/restartInstance.ts index 675c0cba6..b1e0ba917 100644 --- a/src/integrations/api/instance/status/restartInstance.ts +++ b/src/integrations/api/instance/status/restartInstance.ts @@ -31,7 +31,7 @@ interface RestartInstanceParams { /** * Rejects with the restart request's own error when Harper refused it or never answered it, and with - * {@link RestartUnconfirmedError} when Harper accepted it but the target never answered again. A 2xx is + * {@link RestartUnconfirmedError} when Harper accepted it but the watch never saw the target back. A 2xx is * the only verdict Harper gives: `restart` answers before it tears down, `restart_service` once its job * has started. */ From 5068ebe3e7310d57092dc5cd412f91c131815759 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Mon, 12 Oct 2026 20:41:35 -0400 Subject: [PATCH 5/9] test(restart): pin the restart's own timeout, every mark's TTL, and refused config-update members Mutation checks after the rebase onto the new restart loops found three guards no test held: the restart request's explicit 60s timeout (the client default is also 60s, so it only shows with that default cleared), RESTART_MARK_TTL_MS on the Restart button and both cluster-loop marks (restoring the old five minutes stayed green, since sleep is mocked), and the rolling config update's failed count. - restartInstance: the request's bound, every probe's timeout and every wait add up to less than RESTART_MARK_TTL_MS. - The Restart button and the cluster loop still hold their marks six minutes in, past the old TTL, while the watch runs. - A config update names a member that refused alongside the one not back. Co-Authored-By: Claude Opus 5.5 --- src/hooks/useRestartClusterClick.test.tsx | 5 ++++ src/hooks/useRestartInstanceClick.test.tsx | 4 ++++ src/hooks/useRollingConfigUpdate.test.tsx | 24 +++++++++++++++++++ .../instance/status/restartInstance.test.ts | 21 +++++++++++++++- 4 files changed, 53 insertions(+), 1 deletion(-) diff --git a/src/hooks/useRestartClusterClick.test.tsx b/src/hooks/useRestartClusterClick.test.tsx index 508452f44..2360159ea 100644 --- a/src/hooks/useRestartClusterClick.test.tsx +++ b/src/hooks/useRestartClusterClick.test.tsx @@ -353,6 +353,11 @@ describe('useRestartClusterClick, for a member that accepted the restart but was await vi.waitFor(() => expect(restarted).toEqual(['ins-c', 'ins-b'])); expect(getRestartState('ins-b')?.reach).toBe('down'); + // Still held past the old five-minute mark: each mark lasts as long as its member's watch can. + const clock = vi.spyOn(Date, 'now').mockReturnValue(Date.now() + 6 * 60_000); + expect(getRestartState('ins-b')?.reach).toBe('down'); + expect(getRestartState('clu-1')).toBeDefined(); + clock.mockRestore(); await act(async () => { answer!(); diff --git a/src/hooks/useRestartInstanceClick.test.tsx b/src/hooks/useRestartInstanceClick.test.tsx index 049ac62b2..529e8c159 100644 --- a/src/hooks/useRestartInstanceClick.test.tsx +++ b/src/hooks/useRestartInstanceClick.test.tsx @@ -173,6 +173,10 @@ describe('useRestartInstanceClick', () => { await vi.waitFor(() => expect(answer).toBeDefined()); expect(isRestarting('ins-1')).toBe(true); + // Still held past the old five-minute mark: the mark lasts as long as the watch can. + const clock = vi.spyOn(Date, 'now').mockReturnValue(Date.now() + 6 * 60_000); + expect(isRestarting('ins-1')).toBe(true); + clock.mockRestore(); expect(result.current.isRestartPending).toBe(true); expect(toast.loading).toHaveBeenLastCalledWith('Still restarting', { id: 'restart-toast', diff --git a/src/hooks/useRollingConfigUpdate.test.tsx b/src/hooks/useRollingConfigUpdate.test.tsx index 534290aa4..148bdde8b 100644 --- a/src/hooks/useRollingConfigUpdate.test.tsx +++ b/src/hooks/useRollingConfigUpdate.test.tsx @@ -571,6 +571,30 @@ describe('useRollingConfigUpdate, for a member that accepted the restart but was expect(finalError().description).toContain('Failed to fully update cluster.'); }); + it('names a member that refused the update alongside the one not back', async () => { + useCluster([instance('ins-a', '5.1.29'), instance('ins-b', '5.1.29'), instance('ins-c', '5.1.29')]); + vi.mocked(setConfiguration).mockImplementation(async ({ instanceClient }) => { + events.push(`write ${targetOf(instanceClient)}`); + if (targetOf(instanceClient) === 'ins-c') { + throw new Error('Invalid configuration'); + } + return { message: 'ok' }; + }); + notBackAfterRestart('ins-b'); + + await save(); + + expect(events).toEqual(['check ins-c', 'write ins-c', 'check ins-b', 'write ins-b', 'restart ins-b']); + expect(finalError()).toEqual({ + title: 'Not back yet', + description: 'Only 0 "RUNNING" instances were confirmed updated of 3 instances. ' + + '1 instance failed to update. ' + + "1 instance accepted the restart but couldn't be confirmed back, so it may still be starting. " + + '1 other instance was not updated, to keep the cluster available. ' + + 'Check its status before you restart it again.', + }); + }); + it('never fans the configuration out from a canary that is not back', async () => { useCluster([instance('ins-a', '5.2.0'), instance('ins-b', '5.2.0'), instance('ins-c', '5.2.0')]); notBackAfterRestart('ins-c'); diff --git a/src/integrations/api/instance/status/restartInstance.test.ts b/src/integrations/api/instance/status/restartInstance.test.ts index a194f5c1a..752c2d38e 100644 --- a/src/integrations/api/instance/status/restartInstance.test.ts +++ b/src/integrations/api/instance/status/restartInstance.test.ts @@ -1,6 +1,6 @@ /** @vitest-environment jsdom */ import { getInstanceClient } from '@/config/getInstanceClient'; -import { restartInstance } from '@/integrations/api/instance/status/restartInstance'; +import { RESTART_MARK_TTL_MS, restartInstance } from '@/integrations/api/instance/status/restartInstance'; import { RestartUnconfirmedError } from '@/integrations/api/instance/status/restartUnconfirmed'; import { isUnansweredWrite } from '@/integrations/api/unansweredWrite'; import { shouldKeepEvent } from '@/integrations/datadog/shouldKeepEvent'; @@ -126,6 +126,25 @@ describe('restartInstance', () => { expect(vi.mocked(sleep).mock.calls.map(([ms]) => ms)).toEqual([10_000, ...Array(23).fill(15_000)]); }); + it('fits its request, every probe and every wait inside RESTART_MARK_TTL_MS', async () => { + const client = instanceClient({ restart: ok(ACCEPTED), userInfo: timingOut }); + // Whatever the client's own default, the restart request carries its own bound. + client.defaults.timeout = 0; + const timeouts: number[] = []; + const adapter = client.defaults.adapter as (config: InternalAxiosRequestConfig) => Promise; + client.defaults.adapter = (config: InternalAxiosRequestConfig) => { + timeouts.push(config.timeout ?? Number.POSITIVE_INFINITY); + return adapter(config); + }; + + await failureOf(restartInstance({ operation: 'restart', replicated: false, instanceClient: client })); + + expect(timeouts[0]).toBe(60_000); + expect(timeouts).toHaveLength(25); + const waited = vi.mocked(sleep).mock.calls.reduce((total, [ms = 1000]) => total + ms, 0); + expect(timeouts.reduce((total, ms) => total + ms, 0) + waited).toBeLessThan(RESTART_MARK_TTL_MS); + }); + it('sends each probe once, even when a gateway answers it', async () => { const client = instanceClient({ restart: ok(ACCEPTED), userInfo: failing(503, 'Service Unavailable') }); From 05a989c4ec67bfcb170a3b086af302e7b46eabd5 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Mon, 12 Oct 2026 20:42:27 -0400 Subject: [PATCH 6/9] docs(restart): say how a stop at a member not back meets the 5.2+ config fan-out After the rebase onto #1861's rolling config update, the stop rule also decides the fan-out: a canary that is not back never proved it can run the new configuration, so it is never fanned out from, and a stop after the fan-out keeps the note that the members left alone may already have the configuration saved. Both are pinned in useRollingConfigUpdate.test. Co-Authored-By: Claude Opus 5.5 --- DESIGN.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/DESIGN.md b/DESIGN.md index 180dd1649..7c216cc0e 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -82,7 +82,7 @@ Harper's answer to the restart request is its only verdict on the restart: `rest - **The watch outlasts the usual recovery time** (one settle, then two probe schedules), and `onSlowToReturn` fires once between them so a caller can say it is still waiting. Every restart mark uses `RESTART_MARK_TTL_MS`, derived from the same constants, so no mark lapses while its watch still runs; the probes skip the gateway replay for the same reason. - **Only a successful probe counts as back, never a 401/403.** Through central manager's operations proxy a 403 is CM refusing the caller before it contacts the instance (`notAllowed()` in its `HDBInstance.js`), so it says nothing about the target; reading it as "back" would report a restart Studio never saw finish. That is also why the copy says Studio "couldn't confirm it came back", not that the target never answered. -- **A one-at-a-time loop stops at a member that is not back** (cluster restart, rolling config update): restarting the next one could leave no member serving. It reports the members it left alone, and that report outranks Cancel, which would otherwise hide the warning not to restart again. +- **A one-at-a-time loop stops at a member that is not back** (cluster restart, rolling config update): restarting the next one could leave no member serving. It reports the members it left alone, and that report outranks Cancel, which would otherwise hide the warning not to restart again. On a 5.2+ cluster the config update never fans out from a canary that is not back, since it never proved it can run the configuration; a stop after the fan-out keeps the note that the members it left may already have the new configuration saved. - **The chat tools return `success: true`** for it: their action was carried out, and `false` invites the agent to repeat it. ## Cluster upgrade picker — which versions an existing cluster is offered From 3633963f72450b1f75a94beb669ae36d61a0b27b Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Mon, 12 Oct 2026 21:20:35 -0400 Subject: [PATCH 7/9] fix(restart): stop at a no-verdict member that never came back, and say 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 --- DESIGN.md | 2 +- .../Chat/tools/describeToolError.ts | 2 +- src/hooks/useRestartClusterClick.test.tsx | 35 +++++++++++++++++-- src/hooks/useRestartClusterClick.tsx | 14 +++++++- src/hooks/useRestartInstanceClick.test.tsx | 2 -- src/hooks/useRollingConfigUpdate.test.tsx | 23 +++++++++++- src/hooks/useRollingConfigUpdate.tsx | 4 +++ .../instance/status/restartInstance.test.ts | 2 -- .../api/instance/status/restartInstance.ts | 4 +-- .../api/instance/status/restartUnconfirmed.ts | 19 +++++++--- 10 files changed, 90 insertions(+), 17 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index 7c216cc0e..06af92012 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -82,7 +82,7 @@ Harper's answer to the restart request is its only verdict on the restart: `rest - **The watch outlasts the usual recovery time** (one settle, then two probe schedules), and `onSlowToReturn` fires once between them so a caller can say it is still waiting. Every restart mark uses `RESTART_MARK_TTL_MS`, derived from the same constants, so no mark lapses while its watch still runs; the probes skip the gateway replay for the same reason. - **Only a successful probe counts as back, never a 401/403.** Through central manager's operations proxy a 403 is CM refusing the caller before it contacts the instance (`notAllowed()` in its `HDBInstance.js`), so it says nothing about the target; reading it as "back" would report a restart Studio never saw finish. That is also why the copy says Studio "couldn't confirm it came back", not that the target never answered. -- **A one-at-a-time loop stops at a member that is not back** (cluster restart, rolling config update): restarting the next one could leave no member serving. It reports the members it left alone, and that report outranks Cancel, which would otherwise hide the warning not to restart again. On a 5.2+ cluster the config update never fans out from a canary that is not back, since it never proved it can run the configuration; a stop after the fan-out keeps the note that the members it left may already have the new configuration saved. +- **A one-at-a-time loop stops at a member that is not back** (cluster restart, rolling config update): restarting the next one could leave no member serving. The cluster restart also stops at a member whose restart got no verdict once its follow-up watch gives up too: it may have restarted and still be down. Either report names the members it left alone, and it, like the no-verdict warning, outranks Cancel, which would otherwise hide the warning not to restart again; the report then says which members Cancel left. On a 5.2+ cluster the config update never fans out from a canary that is not back, since it never proved it can run the configuration; a stop after the fan-out keeps the note that the members it left may already have the new configuration saved. - **The chat tools return `success: true`** for it: their action was carried out, and `false` invites the agent to repeat it. ## Cluster upgrade picker — which versions an existing cluster is offered diff --git a/src/features/instance/applications/components/Chat/tools/describeToolError.ts b/src/features/instance/applications/components/Chat/tools/describeToolError.ts index b77fa85c2..31c027455 100644 --- a/src/features/instance/applications/components/Chat/tools/describeToolError.ts +++ b/src/features/instance/applications/components/Chat/tools/describeToolError.ts @@ -6,7 +6,7 @@ export function describeToolError(err: unknown): string { return isUnansweredWrite(err) ? UNANSWERED_WRITE_MESSAGE : String(err); } -/** Replace a tool's progress toast with what its failure means; a loading toast never dismisses itself. */ +/** A loading toast never dismisses itself, so a failed tool must replace its progress toast. */ export function toastToolFailure(toastId: string | number, err: unknown) { const { title, description } = describeError(err); toast.error(title, { id: toastId, description, duration: 10_000 }); diff --git a/src/hooks/useRestartClusterClick.test.tsx b/src/hooks/useRestartClusterClick.test.tsx index 2360159ea..86d4de1b4 100644 --- a/src/hooks/useRestartClusterClick.test.tsx +++ b/src/hooks/useRestartClusterClick.test.tsx @@ -296,6 +296,39 @@ describe('useRestartClusterClick, for a member whose restart got no answer', () expect(waitUntilAnsweringAfterRestart).not.toHaveBeenCalled(); expect(vi.mocked(toast.error).mock.calls.at(-1)?.[0]).toBe('Error'); }); + + it('stops there when Studio cannot confirm it came back either, rather than restart the next member', async () => { + vi.mocked(waitUntilAnsweringAfterRestart).mockRejectedValueOnce(new Error('timeout of 3000ms exceeded')); + restartsEndingIn({ 'ins-b': answeredWith(504) }); + + await clickRestart(); + + expect(restarted).toEqual(['ins-c', 'ins-b']); + expect(getRestartState('ins-b')).toBeUndefined(); + expect(finalError()).toEqual({ + title: OUTCOME_UNKNOWN_TITLE, + description: 'Only 1 "RUNNING" instance was confirmed restarted of 3 instances. ' + + `${describeUnansweredWrites(1)} ` + + "Studio couldn't confirm it came back either, so it may still be starting. " + + '1 other instance was not restarted, to keep the cluster available. ' + + 'Check its status before you restart it again.', + }); + }); + + it('names the members a Cancel left alone, beside the warning, when Cancel lands while waiting on it', async () => { + vi.mocked(waitUntilAnsweringAfterRestart).mockImplementationOnce(async () => cancel()); + restartsEndingIn({ 'ins-b': answeredWith(504) }); + + await clickRestart(); + + expect(restarted).toEqual(['ins-c', 'ins-b']); + expect(finalError()).toEqual({ + title: OUTCOME_UNKNOWN_TITLE, + description: 'Only 1 "RUNNING" instance was confirmed restarted of 3 instances. ' + + `${describeUnansweredWrites(1)} ` + + '1 other instance was not restarted: Cancel stopped it there.', + }); + }); }); describe('useRestartClusterClick, for a member that accepted the restart but was never confirmed back', () => { @@ -306,7 +339,6 @@ describe('useRestartClusterClick, for a member that accepted the restart but was }); it('stops there rather than restart another member, and never says the cluster failed', async () => { - // Last member first: ins-c restarts, ins-b accepts and never comes back, ins-a is left alone. restartsEndingIn({ 'ins-b': notBackYet() }); await clickRestart(); @@ -353,7 +385,6 @@ describe('useRestartClusterClick, for a member that accepted the restart but was await vi.waitFor(() => expect(restarted).toEqual(['ins-c', 'ins-b'])); expect(getRestartState('ins-b')?.reach).toBe('down'); - // Still held past the old five-minute mark: each mark lasts as long as its member's watch can. const clock = vi.spyOn(Date, 'now').mockReturnValue(Date.now() + 6 * 60_000); expect(getRestartState('ins-b')?.reach).toBe('down'); expect(getRestartState('clu-1')).toBeDefined(); diff --git a/src/hooks/useRestartClusterClick.tsx b/src/hooks/useRestartClusterClick.tsx index 77c48d0e2..adf39c0b7 100644 --- a/src/hooks/useRestartClusterClick.tsx +++ b/src/hooks/useRestartClusterClick.tsx @@ -81,6 +81,7 @@ export function useRestartClusterClick( let instancesUnanswered = 0; let instancesFailed = 0; let instancesUntouched: number | undefined; + let instancesStoppedOnNoVerdict = false; // 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`. @@ -148,7 +149,13 @@ export function useRestartClusterClick( id: toastId, description: 'Waiting for it to answer before carrying on.', }); - await waitUntilAnsweringAfterRestart({ instanceClient }).catch(() => undefined); + const back = await waitUntilAnsweringAfterRestart({ instanceClient }).then(() => true, () => false); + if (!back) { + // As down as a member that accepted the restart and is not back: stop here too. + instancesUntouched = targets.length - i - 1; + instancesStoppedOnNoVerdict = true; + break; + } } if (i + 1 !== targets.length) { // If it fails to restart, or wasn't available, warn for a bit then move on. @@ -176,6 +183,11 @@ export function useRestartClusterClick( failed: instancesFailed, unanswered: instancesUnanswered, untouched: instancesUntouched, + stoppedOnNoVerdict: instancesStoppedOnNoVerdict, + // Every member Cancel reached first lands in none of the counts. + canceled: canceled && instancesUntouched === undefined + ? targets.length - instancesRestarted - instancesFailed - instancesUnanswered + : 0, }, 'restarted'); if (canceled && !uncertain) { toast.error('Cancelled', { diff --git a/src/hooks/useRestartInstanceClick.test.tsx b/src/hooks/useRestartInstanceClick.test.tsx index 529e8c159..d9f067a8b 100644 --- a/src/hooks/useRestartInstanceClick.test.tsx +++ b/src/hooks/useRestartInstanceClick.test.tsx @@ -151,7 +151,6 @@ describe('useRestartInstanceClick', () => { expect(toast.success).not.toHaveBeenCalled(); expect(client.restarts()).toBe(1); expect(client.probes()).toBe(24); - // Held until the watch gave up, then released: the errors that follow are real. expect(isRestarting('ins-1')).toBe(false); }); @@ -173,7 +172,6 @@ describe('useRestartInstanceClick', () => { await vi.waitFor(() => expect(answer).toBeDefined()); expect(isRestarting('ins-1')).toBe(true); - // Still held past the old five-minute mark: the mark lasts as long as the watch can. const clock = vi.spyOn(Date, 'now').mockReturnValue(Date.now() + 6 * 60_000); expect(isRestarting('ins-1')).toBe(true); clock.mockRestore(); diff --git a/src/hooks/useRollingConfigUpdate.test.tsx b/src/hooks/useRollingConfigUpdate.test.tsx index 148bdde8b..44b2906b0 100644 --- a/src/hooks/useRollingConfigUpdate.test.tsx +++ b/src/hooks/useRollingConfigUpdate.test.tsx @@ -538,7 +538,6 @@ describe('useRollingConfigUpdate, for a member that accepted the restart but was await save(); - // Last member first: ins-b took the config and never came back, so ins-a was not touched. expect(events).toEqual(['check ins-b', 'write ins-b', 'restart ins-b']); expect(toast.loading).toHaveBeenCalledWith( 'Waiting On Instance 1 of 2', @@ -651,6 +650,28 @@ describe('useRollingConfigUpdate, for a member that accepted the restart but was expect(vi.mocked(toast.error).mock.calls.map(([title]) => title)).not.toContain('Cancelled'); }); + it('names the members a Cancel left alone, beside the warning, after a restart that got no verdict', 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)}`); + cancel(); + throw new AxiosError('timeout of 60000ms exceeded', AxiosError.ECONNABORTED, { + method: 'post', + data: '{"operation":"restart_service"}', + } as InternalAxiosRequestConfig); + }); + + await save(); + + expect(events).toEqual(['check ins-b', 'write ins-b', 'restart ins-b']); + expect(finalError()).toEqual({ + title: OUTCOME_UNKNOWN_TITLE, + description: 'Only 0 "RUNNING" instances were confirmed updated of 2 instances. ' + + `${describeUnansweredWrites(1)} ` + + '1 other instance was not updated: Cancel stopped it there.', + }); + }); + it('keeps the pending-configuration note when Cancel lands while waiting on a member after the fan-out', async () => { useCluster([instance('ins-a', '5.2.0'), instance('ins-b', '5.2.0')]); vi.mocked(setConfiguration).mockImplementation(async ({ instanceClient, ...changes }) => { diff --git a/src/hooks/useRollingConfigUpdate.tsx b/src/hooks/useRollingConfigUpdate.tsx index cfa5dc8c7..ad43eb557 100644 --- a/src/hooks/useRollingConfigUpdate.tsx +++ b/src/hooks/useRollingConfigUpdate.tsx @@ -213,6 +213,10 @@ export function useRollingConfigUpdate( failed: instancesFailed, unanswered: instancesUnanswered, untouched: instancesUntouched, + // Every member Cancel reached first lands in none of the counts. + canceled: canceled && instancesUntouched === undefined + ? targets.length - instancesRestarted - instancesFailed - instancesUnanswered + : 0, }, 'updated'); if (canceled && !uncertain) { toast.error('Cancelled', { diff --git a/src/integrations/api/instance/status/restartInstance.test.ts b/src/integrations/api/instance/status/restartInstance.test.ts index 752c2d38e..651182ca8 100644 --- a/src/integrations/api/instance/status/restartInstance.test.ts +++ b/src/integrations/api/instance/status/restartInstance.test.ts @@ -119,7 +119,6 @@ describe('restartInstance', () => { expect(error).toBeInstanceOf(RestartUnconfirmedError); expect((error as RestartUnconfirmedError).cause).toBeInstanceOf(AxiosError); expect((error as RestartUnconfirmedError).cause).toMatchObject({ code: AxiosError.ECONNABORTED }); - // Sent once: Harper accepted it, so nothing here may send it again. expect(sent.filter((operation) => operation === 'restart')).toHaveLength(1); expect(probesSent()).toBe(24); // One settle, then every probe 15s after the last: the schedule RESTART_MARK_TTL_MS is derived from. @@ -128,7 +127,6 @@ describe('restartInstance', () => { it('fits its request, every probe and every wait inside RESTART_MARK_TTL_MS', async () => { const client = instanceClient({ restart: ok(ACCEPTED), userInfo: timingOut }); - // Whatever the client's own default, the restart request carries its own bound. client.defaults.timeout = 0; const timeouts: number[] = []; const adapter = client.defaults.adapter as (config: InternalAxiosRequestConfig) => Promise; diff --git a/src/integrations/api/instance/status/restartInstance.ts b/src/integrations/api/instance/status/restartInstance.ts index b1e0ba917..6f46302f7 100644 --- a/src/integrations/api/instance/status/restartInstance.ts +++ b/src/integrations/api/instance/status/restartInstance.ts @@ -31,9 +31,7 @@ interface RestartInstanceParams { /** * Rejects with the restart request's own error when Harper refused it or never answered it, and with - * {@link RestartUnconfirmedError} when Harper accepted it but the watch never saw the target back. A 2xx is - * the only verdict Harper gives: `restart` answers before it tears down, `restart_service` once its job - * has started. + * {@link RestartUnconfirmedError} when Harper accepted it but the watch never saw the target back. */ export async function restartInstance( { operation, replicated, instanceClient, onSlowToReturn }: RestartInstanceParams & InstanceClientConfig, diff --git a/src/integrations/api/instance/status/restartUnconfirmed.ts b/src/integrations/api/instance/status/restartUnconfirmed.ts index 39ea436c4..87efe6ba3 100644 --- a/src/integrations/api/instance/status/restartUnconfirmed.ts +++ b/src/integrations/api/instance/status/restartUnconfirmed.ts @@ -30,6 +30,10 @@ interface RollingOutcome { unanswered: number; /** Set when a member never came back and the loop stopped there: the members after it, left alone. */ untouched?: number; + /** The member the loop stopped at got no verdict on its restart, rather than accepting it. */ + stoppedOnNoVerdict?: boolean; + /** Members a Cancel left alone. */ + canceled?: number; } /** @@ -37,7 +41,7 @@ interface RollingOutcome { * known; undefined when every outcome is. */ export function describeUncertainRollingOutcome( - { total, confirmed, failed, unanswered, untouched }: RollingOutcome, + { total, confirmed, failed, unanswered, untouched, stoppedOnNoVerdict, canceled = 0 }: RollingOutcome, verb: 'restarted' | 'updated', ): { title: string; description: string } | undefined { if (!unanswered && untouched === undefined) { @@ -52,14 +56,21 @@ export function describeUncertainRollingOutcome( failed > 0 && `${pluralize(failed, 'instance', 'instances')} failed to ${verb === 'restarted' ? 'restart' : 'update'}.`, unanswered > 0 && describeUnansweredWrites(unanswered), - untouched !== undefined && describeNoReturn(untouched, verb), + untouched !== undefined && describeNoReturn(untouched, verb, stoppedOnNoVerdict ? unanswered : 0), + canceled > 0 + && `${pluralize(canceled, 'other instance was', 'other instances were')} not ${verb}: Cancel stopped it there.`, ].filter(excludeFalsy).join(' '), }; } -function describeNoReturn(untouched: number, verb: 'restarted' | 'updated'): string { +/** `unanswered` is non-zero when the member it stopped at got no verdict, and counts those requests. */ +function describeNoReturn(untouched: number, verb: 'restarted' | 'updated', unanswered: number): string { const leftAlone = pluralize(untouched, 'other instance was', 'other instances were'); - return "1 instance accepted the restart but couldn't be confirmed back, so it may still be starting. " + const notBack = !unanswered + ? "1 instance accepted the restart but couldn't be confirmed back, so it may still be starting. " + : `Studio couldn't confirm ${unanswered === 1 ? 'it' : 'the last of them'} came back either, so it may still be ` + + 'starting. '; + return notBack + (untouched > 0 ? `${leftAlone} not ${verb}, to keep the cluster available. ` : '') + 'Check its status before you restart it again.'; } From b44c9cedf31bb2a277e524a3242fc0fddd1721a3 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Mon, 12 Oct 2026 21:58:48 -0400 Subject: [PATCH 8/9] fix(config): stop at a no-verdict restart that never came back, and honor Cancel mid-write Review round 2 after the port (codex major twice; the Harper domain adjudicator kept both as minor and said the base stack could take them): - An unanswered restart_service moved on to the next member after 3s. If the restart ran onto a configuration that broke the member, the next member got the same configuration. The config update now waits on that member, as the cluster restart does, and stops there if the watch gives up too. An unanswered set_configuration does not wait: nothing restarted that member. - A Cancel clicked while a member's set_configuration was in flight still restarted it. The loop now stops before that restart, and the Cancelled report says the configuration may be pending there, as after a fan-out. - After a Cancel, the "Waiting On Instance" toast that onSlowToReturn brings back offered a second Cancel, as if the first had not landed. Both loops now show it without the action and say no other instance will be restarted or updated. Also drops the test helper comments the round flagged as narration. Rejected on evidence: gemini's "axiosRetry resends the restart POST" (the POST is sent once; axiosRetry wraps only the probes, and #1891's replay never repeats a write), and "a Cancel aborts the watch into RestartUnconfirmedError" (Cancel only sets a flag; no signal reaches the watch). Co-Authored-By: Claude Opus 5.5 --- DESIGN.md | 2 +- src/hooks/useRestartClusterClick.test.tsx | 23 +++-- src/hooks/useRestartClusterClick.tsx | 7 +- src/hooks/useRollingConfigUpdate.test.tsx | 89 +++++++++++++++++-- src/hooks/useRollingConfigUpdate.tsx | 39 ++++++-- .../instance/status/restartInstance.test.ts | 1 - 6 files changed, 141 insertions(+), 20 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index 06af92012..e3e11c5a6 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -82,7 +82,7 @@ Harper's answer to the restart request is its only verdict on the restart: `rest - **The watch outlasts the usual recovery time** (one settle, then two probe schedules), and `onSlowToReturn` fires once between them so a caller can say it is still waiting. Every restart mark uses `RESTART_MARK_TTL_MS`, derived from the same constants, so no mark lapses while its watch still runs; the probes skip the gateway replay for the same reason. - **Only a successful probe counts as back, never a 401/403.** Through central manager's operations proxy a 403 is CM refusing the caller before it contacts the instance (`notAllowed()` in its `HDBInstance.js`), so it says nothing about the target; reading it as "back" would report a restart Studio never saw finish. That is also why the copy says Studio "couldn't confirm it came back", not that the target never answered. -- **A one-at-a-time loop stops at a member that is not back** (cluster restart, rolling config update): restarting the next one could leave no member serving. The cluster restart also stops at a member whose restart got no verdict once its follow-up watch gives up too: it may have restarted and still be down. Either report names the members it left alone, and it, like the no-verdict warning, outranks Cancel, which would otherwise hide the warning not to restart again; the report then says which members Cancel left. On a 5.2+ cluster the config update never fans out from a canary that is not back, since it never proved it can run the configuration; a stop after the fan-out keeps the note that the members it left may already have the new configuration saved. +- **A one-at-a-time loop stops at a member that is not back** (cluster restart, rolling config update): restarting the next one could leave no member serving. Both loops also wait on a member whose restart got no verdict, and stop if that follow-up watch gives up too: it may have restarted and still be down, or, in a config update, restarted onto a configuration that broke it. Either report names the members it left alone, and it, like the no-verdict warning, outranks Cancel, which would otherwise hide the warning not to restart again; the report then says which members Cancel left. A Cancel clicked during a member's configuration write stops before its restart, and the report says the configuration may be pending there. On a 5.2+ cluster the config update never fans out from a canary that is not back, since it never proved it can run the configuration; a stop after the fan-out keeps the note that the members it left may already have the new configuration saved. - **The chat tools return `success: true`** for it: their action was carried out, and `false` invites the agent to repeat it. ## Cluster upgrade picker — which versions an existing cluster is offered diff --git a/src/hooks/useRestartClusterClick.test.tsx b/src/hooks/useRestartClusterClick.test.tsx index 86d4de1b4..769807660 100644 --- a/src/hooks/useRestartClusterClick.test.tsx +++ b/src/hooks/useRestartClusterClick.test.tsx @@ -225,10 +225,6 @@ function notBackYet() { return new RestartUnconfirmedError(new AxiosError('timeout of 3000ms exceeded', AxiosError.ECONNABORTED)); } -/** - * Each member's restart outcome, keyed by instance id: an error it rejects with, or a promise it is slow - * to come back until. A member not listed restarts and comes back. `restarted` records the order. - */ function restartsEndingIn(outcomes: Record>) { vi.mocked(restartInstance).mockImplementation(async ({ instanceClient, onSlowToReturn }) => { const target = targetOf(instanceClient); @@ -263,7 +259,6 @@ describe('useRestartClusterClick, for a member whose restart got no answer', () it('holds it down, and waits for it to answer, before restarting the next member', async () => { let answer: (() => void) | undefined; vi.mocked(waitUntilAnsweringAfterRestart).mockReturnValueOnce(new Promise((resolve) => (answer = resolve))); - // Last member first: ins-c restarts, then ins-b's restart gets a 504. restartsEndingIn({ 'ins-b': answeredWith(504) }); const { result } = renderHook(() => useRestartClusterClick()); @@ -445,6 +440,24 @@ describe('useRestartClusterClick, for a member that accepted the restart but was expect(vi.mocked(toast.error).mock.calls.map(([title]) => title)).not.toContain('Cancelled'); }); + it('brings its waiting toast back without a second Cancel once Cancel was clicked', async () => { + vi.mocked(restartInstance).mockImplementation(async ({ instanceClient, onSlowToReturn }) => { + restarted.push(targetOf(instanceClient)); + if (targetOf(instanceClient) === 'ins-b') { + cancel(); + onSlowToReturn?.(); + } + return { message: 'Restarting Harper. This may take up to 60 seconds.' }; + }); + + await clickRestart(); + + const [, options] = vi.mocked(toast.loading).mock.calls.find(([title]) => title === 'Waiting On Instance 2 of 3')!; + expect(options).not.toHaveProperty('action'); + expect(options?.description).toContain('Cancelled: no other instance will be restarted.'); + expect(restarted).toEqual(['ins-c', 'ins-b']); + }); + it('still says Cancelled when nothing was left uncertain', async () => { vi.mocked(restartInstance).mockImplementation(async ({ instanceClient }) => { restarted.push(targetOf(instanceClient)); diff --git a/src/hooks/useRestartClusterClick.tsx b/src/hooks/useRestartClusterClick.tsx index adf39c0b7..c7141f0ac 100644 --- a/src/hooks/useRestartClusterClick.tsx +++ b/src/hooks/useRestartClusterClick.tsx @@ -126,9 +126,12 @@ export function useRestartClusterClick( instanceClient, onSlowToReturn: () => toast.loading(`Waiting On Instance ${i + 1} of ${targets.length}`, { - ...toastConfig, + // A Cancel already closed the toast and stopped the loop: no second Cancel to offer. + ...(canceled ? { duration: toastConfig.duration } : toastConfig), id: toastId, - description: "It accepted the restart but isn't back yet.", + description: canceled + ? "It accepted the restart but isn't back yet. Cancelled: no other instance will be restarted." + : "It accepted the restart but isn't back yet.", }), }); instancesRestarted += 1; diff --git a/src/hooks/useRollingConfigUpdate.test.tsx b/src/hooks/useRollingConfigUpdate.test.tsx index 44b2906b0..d4b20029c 100644 --- a/src/hooks/useRollingConfigUpdate.test.tsx +++ b/src/hooks/useRollingConfigUpdate.test.tsx @@ -34,7 +34,10 @@ vi.mock('@/features/auth/store/authStore', () => ({ })); 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/restartInstance', () => ({ + restartInstance: vi.fn(), + waitUntilAnsweringAfterRestart: vi.fn(), +})); vi.mock('@/integrations/api/instance/status/setConfiguration', async (importOriginal) => ({ ...await importOriginal(), setConfiguration: vi.fn(), @@ -46,7 +49,7 @@ vi.mock('sonner', () => ({ import { getClusterInfo } from '@/features/cluster/queries/getClusterInfoQuery'; import { getInstanceUserInfo } from '@/integrations/api/instance/status/getInstanceUserInfo'; -import { restartInstance } from '@/integrations/api/instance/status/restartInstance'; +import { restartInstance, waitUntilAnsweringAfterRestart } from '@/integrations/api/instance/status/restartInstance'; import { setConfiguration } from '@/integrations/api/instance/status/setConfiguration'; import { toast } from 'sonner'; import { useRollingConfigUpdate } from './useRollingConfigUpdate'; @@ -119,6 +122,7 @@ beforeEach(() => { events.push(`restart ${targetOf(instanceClient)}`); return { message: 'ok' }; }); + vi.mocked(waitUntilAnsweringAfterRestart).mockResolvedValue(undefined); }); afterEach(() => vi.clearAllMocks()); @@ -511,10 +515,6 @@ function cancel() { (options as unknown as { action: { onClick: () => void } }).action.onClick(); } -/** - * Restarts record `restart ` as usual; the targets listed accept the restart but are never - * confirmed back, after saying once that they are slow. - */ function notBackAfterRestart(...targets: string[]) { vi.mocked(restartInstance).mockImplementation(async ({ instanceClient, onSlowToReturn }) => { events.push(`restart ${targetOf(instanceClient)}`); @@ -672,6 +672,83 @@ describe('useRollingConfigUpdate, for a member that accepted the restart but was }); }); + it('stops at a member whose restart got no verdict when Studio cannot confirm it came back either', 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)}`); + throw new AxiosError('timeout of 60000ms exceeded', AxiosError.ECONNABORTED, { + method: 'post', + data: '{"operation":"restart_service"}', + } as InternalAxiosRequestConfig); + }); + vi.mocked(waitUntilAnsweringAfterRestart).mockRejectedValueOnce(new Error('timeout of 3000ms exceeded')); + + await save(); + + expect(events).toEqual(['check ins-b', 'write ins-b', 'restart ins-b']); + expect(finalError()).toEqual({ + title: OUTCOME_UNKNOWN_TITLE, + description: 'Only 0 "RUNNING" instances were confirmed updated of 2 instances. ' + + `${describeUnansweredWrites(1)} ` + + "Studio couldn't confirm it came back either, so it may still be starting. " + + '1 other instance was not updated, to keep the cluster available. ' + + 'Check its status before you restart it again.', + }); + }); + + it('does not wait on a member whose configuration write got no verdict, since nothing restarted it', async () => { + useCluster([instance('ins-a', '5.1.29'), instance('ins-b', '5.1.29')]); + vi.mocked(setConfiguration).mockImplementation(async ({ instanceClient }) => { + events.push(`write ${targetOf(instanceClient)}`); + if (targetOf(instanceClient) === 'ins-b') { + throw new AxiosError('timeout of 60000ms exceeded', AxiosError.ECONNABORTED, { + method: 'post', + data: '{"operation":"set_configuration"}', + } as InternalAxiosRequestConfig); + } + return { message: 'ok' }; + }); + + await save(); + + expect(waitUntilAnsweringAfterRestart).not.toHaveBeenCalled(); + expect(events).toEqual(['check ins-b', 'write ins-b', 'check ins-a', 'write ins-a', 'restart ins-a']); + }); + + it('honors a Cancel clicked during the configuration write before restarting, and says it may be pending', async () => { + useCluster([instance('ins-a', '5.1.29'), instance('ins-b', '5.1.29')]); + vi.mocked(setConfiguration).mockImplementation(async ({ instanceClient }) => { + events.push(`write ${targetOf(instanceClient)}`); + cancel(); + return { message: 'ok' }; + }); + + await save(); + + expect(events).toEqual(['check ins-b', 'write ins-b']); + expect(finalError()).toEqual({ + title: 'Cancelled', + description: 'The config update was partially cancelled. Instances that were not restarted may already ' + + 'have the new configuration saved, and will apply it the next time they restart.', + }); + }); + + it('brings its waiting toast back without a second Cancel once Cancel was clicked', async () => { + useCluster([instance('ins-a', '5.1.29'), instance('ins-b', '5.1.29')]); + vi.mocked(restartInstance).mockImplementation(async ({ instanceClient, onSlowToReturn }) => { + events.push(`restart ${targetOf(instanceClient)}`); + cancel(); + onSlowToReturn?.(); + return { message: 'Restarting http' }; + }); + + await save(); + + const [, options] = vi.mocked(toast.loading).mock.calls.find(([title]) => title === 'Waiting On Instance 1 of 2')!; + expect(options).not.toHaveProperty('action'); + expect(options?.description).toContain('Cancelled: no other instance will be updated.'); + }); + it('keeps the pending-configuration note when Cancel lands while waiting on a member after the fan-out', async () => { useCluster([instance('ins-a', '5.2.0'), instance('ins-b', '5.2.0')]); vi.mocked(setConfiguration).mockImplementation(async ({ instanceClient, ...changes }) => { diff --git a/src/hooks/useRollingConfigUpdate.tsx b/src/hooks/useRollingConfigUpdate.tsx index ad43eb557..55db4a8e7 100644 --- a/src/hooks/useRollingConfigUpdate.tsx +++ b/src/hooks/useRollingConfigUpdate.tsx @@ -4,7 +4,7 @@ import { useInstanceClientIdParams } from '@/config/useInstanceClient'; 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'; +import { restartInstance, waitUntilAnsweringAfterRestart } from '@/integrations/api/instance/status/restartInstance'; import { describeUncertainRollingOutcome, RestartUnconfirmedError, @@ -117,6 +117,9 @@ export function useRollingConfigUpdate( let instancesUnanswered = 0; let instancesFailed = 0; let instancesUntouched: number | undefined; + let instancesStoppedOnNoVerdict = false; + // Saved on a member Cancel then kept from restarting, so pending there as after a fan-out. + let writtenBeforeCancel = false; for (let i = 0; i < targets.length; i++) { const { instanceClient, replicationFqdn } = targets[i]; @@ -134,6 +137,7 @@ export function useRollingConfigUpdate( ), }); let restarted = false; + let restartSent = false; try { // Make sure the instance is responding. await getInstanceUserInfo({ @@ -148,17 +152,25 @@ export function useRollingConfigUpdate( ...params, instanceClient, }); + if (canceled) { + writtenBeforeCancel = true; + break; + } } // Then restart it. + restartSent = true; await restartInstance({ operation: 'restart_service', replicated: false, instanceClient, onSlowToReturn: () => toast.loading(`Waiting On Instance ${i + 1} of ${targets.length}`, { - ...toastConfig, + // A Cancel already closed the toast and stopped the loop: no second Cancel to offer. + ...(canceled ? { duration: toastConfig.duration } : toastConfig), id: toastId, - description: "It accepted the restart but isn't back yet.", + description: canceled + ? "It accepted the restart but isn't back yet. Cancelled: no other instance will be updated." + : "It accepted the restart but isn't back yet.", }), }); instancesRestarted += 1; @@ -173,6 +185,20 @@ export function useRollingConfigUpdate( const unanswered = isUnansweredWrite(error); instancesUnanswered += unanswered ? 1 : 0; instancesFailed += unanswered ? 0 : 1; + if (unanswered && restartSent) { + // The restart may have run onto a configuration that broke it: wait, and stop if it never answers. + toast.loading(`Could Not Confirm Instance ${i + 1} of ${targets.length}`, { + ...toastConfig, + id: toastId, + description: 'Waiting for it to answer before carrying on.', + }); + const back = await waitUntilAnsweringAfterRestart({ instanceClient }).then(() => true, () => false); + if (!back) { + instancesUntouched = targets.length - i - 1; + instancesStoppedOnNoVerdict = true; + break; + } + } if (i + 1 !== targets.length) { // If it fails to restart, or wasn't available, warn for a bit then move on. const verdict = unanswered ? 'Could Not Confirm' : 'Failed Restarting'; @@ -213,6 +239,7 @@ export function useRollingConfigUpdate( failed: instancesFailed, unanswered: instancesUnanswered, untouched: instancesUntouched, + stoppedOnNoVerdict: instancesStoppedOnNoVerdict, // Every member Cancel reached first lands in none of the counts. canceled: canceled && instancesUntouched === undefined ? targets.length - instancesRestarted - instancesFailed - instancesUnanswered @@ -221,7 +248,9 @@ export function useRollingConfigUpdate( if (canceled && !uncertain) { toast.error('Cancelled', { id: toastId, - description: `The config update was partially cancelled.${fanOutAttempted ? ` ${MAY_BE_PENDING}` : ''}`, + description: `The config update was partially cancelled.${ + fanOutAttempted || writtenBeforeCancel ? ` ${MAY_BE_PENDING}` : '' + }`, duration: 10_000, action: { label: 'Dismiss', @@ -256,7 +285,7 @@ export function useRollingConfigUpdate( allInstances.length !== instancesRestarted && `Only ${someRunningInstancesWere} restarted of ${allTheInstances}.`, ].filter(excludeFalsy).shift() || '')) - + (fanOutAttempted ? '\n' + MAY_BE_PENDING : ''), + + (fanOutAttempted || writtenBeforeCancel ? '\n' + MAY_BE_PENDING : ''), duration: 10_000, action: { label: 'Dismiss', diff --git a/src/integrations/api/instance/status/restartInstance.test.ts b/src/integrations/api/instance/status/restartInstance.test.ts index 651182ca8..6579de8d1 100644 --- a/src/integrations/api/instance/status/restartInstance.test.ts +++ b/src/integrations/api/instance/status/restartInstance.test.ts @@ -121,7 +121,6 @@ describe('restartInstance', () => { expect((error as RestartUnconfirmedError).cause).toMatchObject({ code: AxiosError.ECONNABORTED }); expect(sent.filter((operation) => operation === 'restart')).toHaveLength(1); expect(probesSent()).toBe(24); - // One settle, then every probe 15s after the last: the schedule RESTART_MARK_TTL_MS is derived from. expect(vi.mocked(sleep).mock.calls.map(([ms]) => ms)).toEqual([10_000, ...Array(23).fill(15_000)]); }); From 6cd6fbcb2d3db9d0163d2b0effe4e95deec0911d Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Mon, 12 Oct 2026 22:11:05 -0400 Subject: [PATCH 9/9] fix(restart): offer no second Cancel on any progress toast once Cancel was clicked Review round 3 (codex minor, kept by the Harper domain adjudicator): round 2 took the Cancel action off the slow-return toast, but the no-verdict follow-up toast round 2 added to the config update, the cluster loop's one, and both loops' "carry on momentarily" toasts still came back with a Cancel button after Cancel had closed the toast, and promised to carry on for a watch of up to about seven minutes. One helper per loop now builds every progress toast after the first: the usual Cancel action and copy, or, once Cancel was clicked, neither, and a note that no other instance will be restarted or updated. Also drops a comment that only restated its flag. Co-Authored-By: Claude Opus 5.5 --- src/hooks/useRestartClusterClick.test.tsx | 23 ++++++++++++++++++++++ src/hooks/useRestartClusterClick.tsx | 22 ++++++++++++--------- src/hooks/useRollingConfigUpdate.test.tsx | 24 +++++++++++++++++++++++ src/hooks/useRollingConfigUpdate.tsx | 23 ++++++++++++---------- 4 files changed, 73 insertions(+), 19 deletions(-) diff --git a/src/hooks/useRestartClusterClick.test.tsx b/src/hooks/useRestartClusterClick.test.tsx index 769807660..a4e90b93e 100644 --- a/src/hooks/useRestartClusterClick.test.tsx +++ b/src/hooks/useRestartClusterClick.test.tsx @@ -458,6 +458,29 @@ describe('useRestartClusterClick, for a member that accepted the restart but was expect(restarted).toEqual(['ins-c', 'ins-b']); }); + it('brings back none of its later toasts with a second Cancel once Cancel was clicked', async () => { + vi.mocked(restartInstance).mockImplementation(async ({ instanceClient }) => { + restarted.push(targetOf(instanceClient)); + if (targetOf(instanceClient) === 'ins-b') { + cancel(); + throw answeredWith(504); + } + return { message: 'Restarting Harper. This may take up to 60 seconds.' }; + }); + + await clickRestart(); + + const afterCancel = vi.mocked(toast.loading).mock.calls.filter(([title]) => + title === 'Could Not Confirm Instance 2 of 3' + ); + expect(afterCancel).toHaveLength(2); + for (const [, options] of afterCancel) { + expect(options).not.toHaveProperty('action'); + expect(options?.description).toContain('Cancelled: no other instance will be restarted.'); + } + expect(restarted).toEqual(['ins-c', 'ins-b']); + }); + it('still says Cancelled when nothing was left uncertain', async () => { vi.mocked(restartInstance).mockImplementation(async ({ instanceClient }) => { restarted.push(targetOf(instanceClient)); diff --git a/src/hooks/useRestartClusterClick.tsx b/src/hooks/useRestartClusterClick.tsx index c7141f0ac..4c7c7a070 100644 --- a/src/hooks/useRestartClusterClick.tsx +++ b/src/hooks/useRestartClusterClick.tsx @@ -50,6 +50,16 @@ export function useRestartClusterClick( }, }; + // Once Cancel is clicked its toast is gone, and a progress toast offering Cancel again would read as if + // it had not landed: say so instead. + const progress = (description: string, onceCanceled = description) => + canceled + ? { + duration: toastConfig.duration, + description: `${onceCanceled} Cancelled: no other instance will be restarted.`.trim(), + } + : { ...toastConfig, description }; + const toastId = toast.loading('Restarting', { ...toastConfig, description: ( @@ -126,12 +136,8 @@ export function useRestartClusterClick( instanceClient, onSlowToReturn: () => toast.loading(`Waiting On Instance ${i + 1} of ${targets.length}`, { - // A Cancel already closed the toast and stopped the loop: no second Cancel to offer. - ...(canceled ? { duration: toastConfig.duration } : toastConfig), id: toastId, - description: canceled - ? "It accepted the restart but isn't back yet. Cancelled: no other instance will be restarted." - : "It accepted the restart but isn't back yet.", + ...progress("It accepted the restart but isn't back yet."), }), }); instancesRestarted += 1; @@ -148,9 +154,8 @@ export function useRestartClusterClick( // A gateway can lose the answer to a restart that ran and took this member down, so it // stays marked, and the next one waits, until it answers again or the watch gives up. toast.loading(`Could Not Confirm Instance ${i + 1} of ${targets.length}`, { - ...toastConfig, id: toastId, - description: 'Waiting for it to answer before carrying on.', + ...progress('Waiting for it to answer before carrying on.', 'Waiting for it to answer.'), }); const back = await waitUntilAnsweringAfterRestart({ instanceClient }).then(() => true, () => false); if (!back) { @@ -164,9 +169,8 @@ export function useRestartClusterClick( // If it fails to restart, or wasn't available, warn for a bit then move on. const verdict = unanswered ? 'Could Not Confirm' : 'Failed Restarting'; toast.loading(`${verdict} Instance ${i + 1} of ${targets.length}`, { - ...toastConfig, id: toastId, - description: 'We will carry on momentarily.', + ...progress('We will carry on momentarily.', ''), }); await sleep(3000); } diff --git a/src/hooks/useRollingConfigUpdate.test.tsx b/src/hooks/useRollingConfigUpdate.test.tsx index d4b20029c..b45cd590a 100644 --- a/src/hooks/useRollingConfigUpdate.test.tsx +++ b/src/hooks/useRollingConfigUpdate.test.tsx @@ -749,6 +749,30 @@ describe('useRollingConfigUpdate, for a member that accepted the restart but was expect(options?.description).toContain('Cancelled: no other instance will be updated.'); }); + it('brings back none of its later toasts with a second Cancel once Cancel was clicked', 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)}`); + cancel(); + throw new AxiosError('timeout of 60000ms exceeded', AxiosError.ECONNABORTED, { + method: 'post', + data: '{"operation":"restart_service"}', + } as InternalAxiosRequestConfig); + }); + + await save(); + + const afterCancel = vi.mocked(toast.loading).mock.calls.filter(([title]) => + title === 'Could Not Confirm Instance 1 of 2' + ); + expect(afterCancel).toHaveLength(2); + for (const [, options] of afterCancel) { + expect(options).not.toHaveProperty('action'); + expect(options?.description).toContain('Cancelled: no other instance will be updated.'); + } + expect(events).toEqual(['check ins-b', 'write ins-b', 'restart ins-b']); + }); + it('keeps the pending-configuration note when Cancel lands while waiting on a member after the fan-out', async () => { useCluster([instance('ins-a', '5.2.0'), instance('ins-b', '5.2.0')]); vi.mocked(setConfiguration).mockImplementation(async ({ instanceClient, ...changes }) => { diff --git a/src/hooks/useRollingConfigUpdate.tsx b/src/hooks/useRollingConfigUpdate.tsx index 55db4a8e7..5379d8e49 100644 --- a/src/hooks/useRollingConfigUpdate.tsx +++ b/src/hooks/useRollingConfigUpdate.tsx @@ -84,6 +84,16 @@ export function useRollingConfigUpdate( }, }; + // Once Cancel is clicked its toast is gone, and a progress toast offering Cancel again would read as if + // it had not landed: say so instead. + const progress = (description: string, onceCanceled = description) => + canceled + ? { + duration: toastConfig.duration, + description: `${onceCanceled} Cancelled: no other instance will be updated.`.trim(), + } + : { ...toastConfig, description }; + const toastId = toast.loading('Restarting', { ...toastConfig, description: ( @@ -118,7 +128,6 @@ export function useRollingConfigUpdate( let instancesFailed = 0; let instancesUntouched: number | undefined; let instancesStoppedOnNoVerdict = false; - // Saved on a member Cancel then kept from restarting, so pending there as after a fan-out. let writtenBeforeCancel = false; for (let i = 0; i < targets.length; i++) { @@ -165,12 +174,8 @@ export function useRollingConfigUpdate( instanceClient, onSlowToReturn: () => toast.loading(`Waiting On Instance ${i + 1} of ${targets.length}`, { - // A Cancel already closed the toast and stopped the loop: no second Cancel to offer. - ...(canceled ? { duration: toastConfig.duration } : toastConfig), id: toastId, - description: canceled - ? "It accepted the restart but isn't back yet. Cancelled: no other instance will be updated." - : "It accepted the restart but isn't back yet.", + ...progress("It accepted the restart but isn't back yet."), }), }); instancesRestarted += 1; @@ -188,9 +193,8 @@ export function useRollingConfigUpdate( if (unanswered && restartSent) { // The restart may have run onto a configuration that broke it: wait, and stop if it never answers. toast.loading(`Could Not Confirm Instance ${i + 1} of ${targets.length}`, { - ...toastConfig, id: toastId, - description: 'Waiting for it to answer before carrying on.', + ...progress('Waiting for it to answer before carrying on.', 'Waiting for it to answer.'), }); const back = await waitUntilAnsweringAfterRestart({ instanceClient }).then(() => true, () => false); if (!back) { @@ -203,9 +207,8 @@ export function useRollingConfigUpdate( // If it fails to restart, or wasn't available, warn for a bit then move on. const verdict = unanswered ? 'Could Not Confirm' : 'Failed Restarting'; toast.loading(`${verdict} Instance ${i + 1} of ${targets.length}`, { - ...toastConfig, id: toastId, - description: 'We will carry on momentarily.', + ...progress('We will carry on momentarily.', ''), }); await sleep(3000); }