diff --git a/DESIGN.md b/DESIGN.md index 4ecf3a582..e3e11c5a6 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -76,6 +76,15 @@ 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 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. +- **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 [`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..b8bdf84f0 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 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, + }; + } 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..31c027455 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); } + +/** 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/features/instance/applications/components/Chat/tools/restartHTTPService/execute.ts b/src/features/instance/applications/components/Chat/tools/restartHTTPService/execute.ts index 5e4525885..d2e5abb91 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,17 @@ 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 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, + }; + } 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..a4e90b93e 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,38 @@ 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)); +} + +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( @@ -236,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()); @@ -269,4 +291,206 @@ 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', () => { + 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 () => { + 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 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 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.', + }); + 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 couldn't be confirmed back, 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'); + 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!(); + 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'); + }); + + 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 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.', + }); + }); + + 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('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('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)); + 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 ddeeaf09d..4c7c7a070 100644 --- a/src/hooks/useRestartClusterClick.tsx +++ b/src/hooks/useRestartClusterClick.tsx @@ -1,8 +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 { describeUnansweredWrites, isUnansweredWrite, OUTCOME_UNKNOWN_TITLE } from '@/integrations/api/unansweredWrite'; +import { + RESTART_MARK_TTL_MS, + restartInstance, + waitUntilAnsweringAfterRestart, +} from '@/integrations/api/instance/status/restartInstance'; +import { + describeUncertainRollingOutcome, + RestartUnconfirmedError, +} from '@/integrations/api/instance/status/restartUnconfirmed'; +import { isUnansweredWrite } from '@/integrations/api/unansweredWrite'; import { excludeFalsy } from '@/lib/arrays/excludeFalsy'; import { pluralize } from '@/lib/pluralize'; import { markRestarting } from '@/lib/restart/restartTracker'; @@ -11,9 +19,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; } @@ -45,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: ( @@ -74,12 +89,15 @@ export function useRestartClusterClick( const { allInstances, targets } = plan; let instancesRestarted = 0; 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`. 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,35 +127,50 @@ 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}`, { + id: toastId, + ...progress("It accepted the restart but isn't back 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; + 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. 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.'), }); - 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. 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); } @@ -151,7 +184,19 @@ export function useRestartClusterClick( releaseCluster(); } - if (canceled) { + const uncertain = describeUncertainRollingOutcome({ + total: allInstances.length, + confirmed: instancesRestarted, + 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', { id: toastId, description: `The restart was partially cancelled.`, @@ -179,12 +224,10 @@ export function useRestartClusterClick( '"RUNNING" instance was', '"RUNNING" instances were', ); - toast.error(instancesUnanswered ? OUTCOME_UNKNOWN_TITLE : 'Error', { + toast.error(uncertain?.title ?? 'Error', { id: toastId, - description: instancesUnanswered - ? `Only ${someRunningInstancesWere} confirmed restarted of ${allTheInstances}. ` - + describeUnansweredWrites(instancesUnanswered) - : `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/useRestartInstanceClick.test.tsx b/src/hooks/useRestartInstanceClick.test.tsx index a9a0db51f..d9f067a8b 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,77 @@ 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.dismiss).toHaveBeenCalledWith('restart-toast'); + expect(toast.success).not.toHaveBeenCalled(); + expect(client.restarts()).toBe(1); + expect(client.probes()).toBe(24); + 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); + 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', + description: "Harper accepted the restart, but the instance isn't back yet. Still waiting for it.", + }); + 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..578bb240a 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()} isn't back yet. ` + + 'Still waiting for it.', + }), }); // 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..b45cd590a 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'; @@ -33,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(), @@ -45,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'; @@ -118,6 +122,7 @@ beforeEach(() => { events.push(`restart ${targetOf(instanceClient)}`); return { message: 'ok' }; }); + vi.mocked(waitUntilAnsweringAfterRestart).mockResolvedValue(undefined); }); afterEach(() => vi.clearAllMocks()); @@ -509,3 +514,292 @@ function cancel() { const [, options] = vi.mocked(toast.loading).mock.calls.at(-1)!; (options as unknown as { action: { onClick: () => void } }).action.onClick(); } + +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 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'); + + await save(); + + 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 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 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.', + }); + 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('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'); + + 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 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 ' + + '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('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('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('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 }) => { + 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 e74a23907..5379d8e49 100644 --- a/src/hooks/useRollingConfigUpdate.tsx +++ b/src/hooks/useRollingConfigUpdate.tsx @@ -4,10 +4,14 @@ 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, +} 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'; @@ -80,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: ( @@ -111,6 +125,10 @@ export function useRollingConfigUpdate( let acknowledged = new Set(); let instancesRestarted = 0; let instancesUnanswered = 0; + let instancesFailed = 0; + let instancesUntouched: number | undefined; + let instancesStoppedOnNoVerdict = false; + let writtenBeforeCancel = false; for (let i = 0; i < targets.length; i++) { const { instanceClient, replicationFqdn } = targets[i]; @@ -128,6 +146,7 @@ export function useRollingConfigUpdate( ), }); let restarted = false; + let restartSent = false; try { // Make sure the instance is responding. await getInstanceUserInfo({ @@ -142,25 +161,54 @@ 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}`, { + id: toastId, + ...progress("It accepted the restart but isn't back 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; + 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}`, { + id: toastId, + ...progress('Waiting for it to answer before carrying on.', 'Waiting for it to answer.'), + }); + 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'; 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); } @@ -188,10 +236,24 @@ export function useRollingConfigUpdate( } } - if (canceled) { + const uncertain = describeUncertainRollingOutcome({ + total: allInstances.length, + confirmed: instancesRestarted, + 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, + }, 'updated'); + 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', @@ -216,19 +278,17 @@ export function useRollingConfigUpdate( '"RUNNING" instance was', '"RUNNING" instances were', ); - toast.error(instancesUnanswered ? OUTCOME_UNKNOWN_TITLE : 'Error', { + toast.error(uncertain?.title ?? 'Error', { id: toastId, - description: (instancesUnanswered - ? `Only ${someRunningInstancesWere} confirmed updated of ${allTheInstances}. ` - + describeUnansweredWrites(instancesUnanswered) - : `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}.`, 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 new file mode 100644 index 000000000..6579de8d1 --- /dev/null +++ b/src/integrations/api/instance/status/restartInstance.test.ts @@ -0,0 +1,212 @@ +/** @vitest-environment jsdom */ +import { getInstanceClient } from '@/config/getInstanceClient'; +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'; +import { sleep } from '@/lib/sleep'; +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; + vi.mocked(sleep).mockClear(); + }); + + 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('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(); + + 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 }); + expect(sent.filter((operation) => operation === 'restart')).toHaveLength(1); + expect(probesSent()).toBe(24); + 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 }); + 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') }); + + 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 was not confirmed back: /); + }); +}); diff --git a/src/integrations/api/instance/status/restartInstance.ts b/src/integrations/api/instance/status/restartInstance.ts index 4bff908ef..6f46302f7 100644 --- a/src/integrations/api/instance/status/restartInstance.ts +++ b/src/integrations/api/instance/status/restartInstance.ts @@ -1,17 +1,40 @@ 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; +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 is still not back 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 watch never saw the target back. + */ 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 +42,38 @@ 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 `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 { + // 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 { + 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); + } } 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..87efe6ba3 --- /dev/null +++ b/src/integrations/api/instance/status/restartUnconfirmed.ts @@ -0,0 +1,76 @@ +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'; + +export const RESTART_UNCONFIRMED_TITLE = 'Not back yet'; + +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 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 was not confirmed back: ${errorText(cause) ?? 'no answer'}`, { + cause, + }); + this.name = 'RestartUnconfirmedError'; + } +} + +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; + /** 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; +} + +/** + * 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 describeUncertainRollingOutcome( + { total, confirmed, failed, unanswered, untouched, stoppedOnNoVerdict, canceled = 0 }: 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, stoppedOnNoVerdict ? unanswered : 0), + canceled > 0 + && `${pluralize(canceled, 'other instance was', 'other instances were')} not ${verb}: Cancel stopped it there.`, + ].filter(excludeFalsy).join(' '), + }; +} + +/** `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'); + 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.'; +} diff --git a/src/react-query/queryClient.test.ts b/src/react-query/queryClient.test.ts index daf570a03..63503afc5 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 was never confirmed back', () => { + 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..8185d0157 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 was never confirmed back ({@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) {