From e37bc27ef2b4dd79bcb35863b673e2ac7831c740 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Sun, 11 Oct 2026 05:03:43 -0400 Subject: [PATCH 01/16] fix(errors): say a write's outcome is unknown when it got no verdict, not that it failed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A write answered 502/503/504, or not at all (a timeout, ERR_NETWORK), may already have been applied, but Studio showed it as an ordinary failure: the gateway's body or "Request failed with status code 504", and callers' own "Failed to restart …" / "Failed to import …" on top. Retrying by hand repeated the side effect. isUnansweredWrite (new) recognizes it from the request (isReadRequest) and the status, never the body. describeError checks it first and returns "Outcome unknown" copy, so the global mutation toast and every describeError caller get it. Callers that add their own failure toast to a mutation ask shouldToastOwnFailure first; rename and the chat tools, which have no mutation behind them, show the same copy; the cluster restart, rolling config and license loops count these apart from failures. secrets.ts no longer re-wraps such an error, so the predicate can still see it. package_component is classified as the read it is, and skips the gateway replay because each attempt rebuilds the archive. Closes #1890 Co-Authored-By: Claude Opus 5.5 --- DESIGN.md | 9 ++ src/features/auth/describeAuthFailure.ts | 12 +- .../clusters/components/ClusterCard.tsx | 25 ++-- .../Chat/tools/createApp/execute.ts | 5 +- .../Chat/tools/deleteTableRecords/execute.ts | 3 +- .../Chat/tools/describeToolError.ts | 6 + .../Chat/tools/dropComponentFile/execute.ts | 3 +- .../Chat/tools/insertTableRecords/execute.ts | 3 +- .../Chat/tools/restartHTTPService/execute.ts | 3 +- .../Chat/tools/setComponentFile/execute.ts | 3 +- .../Chat/tools/updateTableRecords/execute.ts | 3 +- .../useImportApplication.test.ts | 20 +++ .../NewApplication/useImportApplication.ts | 6 +- .../hooks/useRenameFiles.test.tsx | 63 ++++++++++ .../applications/hooks/useRenameFiles.tsx | 10 +- .../modals/RedeployApplicationModal.test.tsx | 45 +++++++ .../modals/RedeployApplicationModal.tsx | 13 +- .../applications/modals/RenameFileModal.tsx | 8 +- .../config/secrets/SecretGrantsEditor.tsx | 10 +- .../components/DatabaseTableView.tsx | 5 +- .../instance/secrets/SecretModals.tsx | 14 ++- .../modals/AddOrganizationRoleModal.test.tsx | 28 ++++- .../roles/modals/AddOrganizationRoleModal.tsx | 5 +- .../modals/EditOrganizationRoleModal.tsx | 7 ++ src/features/organization/settings/index.tsx | 7 +- .../users/modals/EditUserModal.tsx | 9 +- src/hooks/useApplyLicensesClick.test.tsx | 30 +++++ src/hooks/useApplyLicensesClick.tsx | 24 ++-- src/hooks/useRestartClusterClick.test.tsx | 30 ++++- src/hooks/useRestartClusterClick.tsx | 28 +++-- src/hooks/useRestartInstanceClick.test.tsx | 77 ++++++++++++ src/hooks/useRestartInstanceClick.ts | 19 +-- src/hooks/useRollingConfigUpdate.test.tsx | 26 +++- src/hooks/useRollingConfigUpdate.tsx | 28 +++-- .../instance/applications/packageComponent.ts | 3 +- .../api/instance/secrets/secrets.test.tsx | 55 +++++++- .../api/instance/secrets/secrets.ts | 8 +- src/integrations/api/isReadOperation.test.ts | 2 + src/integrations/api/isReadOperation.ts | 5 +- src/integrations/api/retryGatewayErrors.ts | 1 + .../api/unansweredWrite.integration.test.tsx | 118 ++++++++++++++++++ src/integrations/api/unansweredWrite.test.ts | 71 +++++++++++ src/integrations/api/unansweredWrite.ts | 46 +++++++ src/react-query/queryClient.test.ts | 62 ++++++++- src/react-query/queryClient.ts | 27 +++- 45 files changed, 888 insertions(+), 97 deletions(-) create mode 100644 src/features/instance/applications/components/Chat/tools/describeToolError.ts create mode 100644 src/features/instance/applications/hooks/useRenameFiles.test.tsx create mode 100644 src/hooks/useRestartInstanceClick.test.tsx create mode 100644 src/integrations/api/unansweredWrite.integration.test.tsx create mode 100644 src/integrations/api/unansweredWrite.test.ts create mode 100644 src/integrations/api/unansweredWrite.ts diff --git a/DESIGN.md b/DESIGN.md index 94546b92d..4d29df3bf 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -66,6 +66,15 @@ The wide hero artwork spans both columns beneath the form card in three independ Every `getInstanceClient` client replays a 502/503/504 up to three times (5s/10s/20s), but only for a request [`isReadRequest`](src/integrations/api/isReadOperation.ts) accepts: a GET, or a POST whose body's `operation` is a read by Harper's naming — the restart gate's rule, one predicate for both ([`retryGatewayErrors.ts`](src/integrations/api/retryGatewayErrors.ts)). A write is sent once and its first 5xx is final, because no gateway status proves the write was not applied: Harper itself answers 503 after handing an operation to a worker that exits or overruns. Do not add an exemption for a "usually safe" status or a write that looks idempotent; a caller whose write is safe to repeat retries it itself, where it can say why. +## A write with no verdict never reads as failed + +The same rule decides what a failure says. [`isUnansweredWrite`](src/integrations/api/unansweredWrite.ts) is a request `isReadRequest` rejects, answered 502/503/504 or not at all (`ERR_NETWORK`, a timeout). [`describeError`](src/react-query/queryClient.ts) checks it before reading any body and returns "Outcome unknown … check whether it took effect before you try again", so the global mutation toast and every `describeError` caller say so with no copy of their own. Gate on status, never body (a gateway's body describes the gateway), and keep it to display: rewriting `error.message` would defeat `shouldKeepEvent`, which recognises timeouts and `Network Error` by message. + +- **A caller that adds its own failure toast to a mutation** asks [`shouldToastOwnFailure`](src/react-query/queryClient.ts) first, so "Failed to …" never sits beside the global warning. A caller with no mutation behind it (rename, the chat tools) shows `describeError`'s text itself, and a loop that tallies one write per target (cluster restart, rolling config, licenses) counts these apart from failures. +- **Do not re-wrap an AxiosError into a plain `Error`** for a write: the predicate needs its `config`. `secrets.ts` passes an unanswered one through as is. +- **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`). + ## 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/auth/describeAuthFailure.ts b/src/features/auth/describeAuthFailure.ts index 3c285e2e5..fd957b168 100644 --- a/src/features/auth/describeAuthFailure.ts +++ b/src/features/auth/describeAuthFailure.ts @@ -1,3 +1,4 @@ +import { NO_RESPONSE_CODES } from '@/integrations/api/unansweredWrite'; import { describeError } from '@/react-query/queryClient'; import { AxiosError } from 'axios'; @@ -24,15 +25,6 @@ const TRANSIENT_STATUSES: ReadonlySet = new Set([503]); // safe once you know it did not). Repeating blindly turns a completed sign-up into a 409 with the // verification mail already sent (#1668). const OUTCOME_UNKNOWN_STATUSES: ReadonlySet = new Set([502, 504]); -// Axios reports a request that got no response as ERR_NETWORK — including a CORS rejection, and -// including a connection that dropped *after* the POST was applied — so it cannot be described as -// "we couldn't reach the server". It is the same unknown outcome. ECONNABORTED/ETIMEDOUT are the -// timeout pair; which one axios uses depends on `transitional.clarifyTimeoutError`. -const OUTCOME_UNKNOWN_CODES: ReadonlySet = new Set([ - AxiosError.ERR_NETWORK, - AxiosError.ETIMEDOUT, - AxiosError.ECONNABORTED, -]); /** * Our own copy for a failure the server's own words shouldn't answer, or `undefined` to defer. @@ -64,7 +56,7 @@ export function describeRetryableAuthFailure(rawErr: unknown, outcomeUnknownReco // ERR_CANCELED is a local abort that reached nothing, and a config fault (ERR_INVALID_URL, // ERR_BAD_OPTION) never left the browser — none of those is ours to describe. const code = axiosErr?.code; - return typeof code === 'string' && OUTCOME_UNKNOWN_CODES.has(code) ? outcomeUnknown : undefined; + return typeof code === 'string' && NO_RESPONSE_CODES.has(code) ? outcomeUnknown : undefined; } /** Display text for an auth-form rejection, preferring whatever the server said. */ diff --git a/src/features/clusters/components/ClusterCard.tsx b/src/features/clusters/components/ClusterCard.tsx index 9c10485be..596ecd766 100644 --- a/src/features/clusters/components/ClusterCard.tsx +++ b/src/features/clusters/components/ClusterCard.tsx @@ -28,6 +28,7 @@ import { excludeFalsy } from '@/lib/arrays/excludeFalsy'; import { LocalStorageKeys } from '@/lib/storage/localStorageKeys'; import { buildAbsoluteLinkToPage } from '@/lib/urls/buildAbsoluteLinkToPage'; import { getOperationsUrlForCluster } from '@/lib/urls/getOperationsUrlForCluster'; +import { shouldToastOwnFailure } from '@/react-query/queryClient'; import { useQueryClient } from '@tanstack/react-query'; import { Link, useRouter } from '@tanstack/react-router'; import { @@ -146,17 +147,19 @@ export function ClusterCard({ item: summary }: { item: ClusterListItem }) { }); setIsTerminateClusterModalOpen(false); }, - onError: () => { - toast.error('Error', { - description: isSelfManaged - ? `Failed to remove cluster: ${cluster.name}` - : `Failed to terminate cluster: ${cluster.name}.`, - duration: 5000, - action: { - label: 'Dismiss', - onClick: () => toast.dismiss(), - }, - }); + onError: (error) => { + if (shouldToastOwnFailure(error)) { + toast.error('Error', { + description: isSelfManaged + ? `Failed to remove cluster: ${cluster.name}` + : `Failed to terminate cluster: ${cluster.name}.`, + duration: 5000, + action: { + label: 'Dismiss', + onClick: () => toast.dismiss(), + }, + }); + } setIsTerminateClusterModalOpen(false); }, }); 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 1f6e53f5a..617b2fd0d 100644 --- a/src/features/instance/applications/components/Chat/tools/createApp/execute.ts +++ b/src/features/instance/applications/components/Chat/tools/createApp/execute.ts @@ -8,6 +8,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 { Output } from './output'; export async function execute( @@ -39,7 +40,7 @@ export async function execute( } catch (err) { return { success: false, - message: `Error: ${err}`, + message: `Error: ${describeToolError(err)}`, }; } @@ -60,7 +61,7 @@ export async function execute( } catch (err) { return { success: false, - message: `Error: ${err}`, + message: `Error: ${describeToolError(err)}`, }; } diff --git a/src/features/instance/applications/components/Chat/tools/deleteTableRecords/execute.ts b/src/features/instance/applications/components/Chat/tools/deleteTableRecords/execute.ts index cb41f2ff9..683337bb5 100644 --- a/src/features/instance/applications/components/Chat/tools/deleteTableRecords/execute.ts +++ b/src/features/instance/applications/components/Chat/tools/deleteTableRecords/execute.ts @@ -3,6 +3,7 @@ import { queryClient } from '@/react-query/queryClient'; import { inputSchema } from '@harperfast/agent-tools/tools/deleteTableRecords/inputSchema'; import { z } from 'zod'; import { ExecuteParams } from '../../types/executeParams'; +import { describeToolError } from '../describeToolError'; import { Output } from './output'; export async function execute( @@ -24,7 +25,7 @@ export async function execute( } catch (err) { return { success: false, - message: `Error: ${err}`, + 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 new file mode 100644 index 000000000..97f985231 --- /dev/null +++ b/src/features/instance/applications/components/Chat/tools/describeToolError.ts @@ -0,0 +1,6 @@ +import { isUnansweredWrite, UNANSWERED_WRITE_MESSAGE } from '@/integrations/api/unansweredWrite'; + +/** A failed tool call, as the agent reads it: told plainly when a retry could apply a write twice. */ +export function describeToolError(err: unknown): string { + return isUnansweredWrite(err) ? UNANSWERED_WRITE_MESSAGE : String(err); +} diff --git a/src/features/instance/applications/components/Chat/tools/dropComponentFile/execute.ts b/src/features/instance/applications/components/Chat/tools/dropComponentFile/execute.ts index fe9715563..0d022447e 100644 --- a/src/features/instance/applications/components/Chat/tools/dropComponentFile/execute.ts +++ b/src/features/instance/applications/components/Chat/tools/dropComponentFile/execute.ts @@ -2,6 +2,7 @@ import { dropComponent } from '@/integrations/api/instance/applications/dropComp import { inputSchema } from '@harperfast/agent-tools/tools/dropComponentFile/inputSchema'; import { z } from 'zod'; import { ExecuteParams } from '../../types/executeParams'; +import { describeToolError } from '../describeToolError'; import { Output } from './output'; export async function execute( @@ -19,7 +20,7 @@ export async function execute( } catch (err) { return { success: false, - message: `Error: ${err}`, + message: `Error: ${describeToolError(err)}`, }; } } diff --git a/src/features/instance/applications/components/Chat/tools/insertTableRecords/execute.ts b/src/features/instance/applications/components/Chat/tools/insertTableRecords/execute.ts index fd6af4be4..dabdcf37c 100644 --- a/src/features/instance/applications/components/Chat/tools/insertTableRecords/execute.ts +++ b/src/features/instance/applications/components/Chat/tools/insertTableRecords/execute.ts @@ -3,6 +3,7 @@ import { queryClient } from '@/react-query/queryClient'; import { inputSchema } from '@harperfast/agent-tools/tools/insertTableRecords/inputSchema'; import { z } from 'zod'; import { ExecuteParams } from '../../types/executeParams'; +import { describeToolError } from '../describeToolError'; import { Output } from './output'; export async function execute( @@ -24,7 +25,7 @@ export async function execute( } catch (err) { return { success: false, - message: `Error: ${err}`, + message: `Error: ${describeToolError(err)}`, }; } } 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 fb054f514..5e4525885 100644 --- a/src/features/instance/applications/components/Chat/tools/restartHTTPService/execute.ts +++ b/src/features/instance/applications/components/Chat/tools/restartHTTPService/execute.ts @@ -3,6 +3,7 @@ import { inputSchema } from '@harperfast/agent-tools/tools/restartHTTPService/in import { toast } from 'sonner'; import { z } from 'zod'; import { ExecuteParams } from '../../types/executeParams'; +import { describeToolError } from '../describeToolError'; import { Output } from './output'; export async function execute( @@ -22,7 +23,7 @@ export async function execute( } catch (err) { return { success: false, - message: `Error: ${err}`, + message: `Error: ${describeToolError(err)}`, }; } diff --git a/src/features/instance/applications/components/Chat/tools/setComponentFile/execute.ts b/src/features/instance/applications/components/Chat/tools/setComponentFile/execute.ts index 5b5b02af1..614ee120c 100644 --- a/src/features/instance/applications/components/Chat/tools/setComponentFile/execute.ts +++ b/src/features/instance/applications/components/Chat/tools/setComponentFile/execute.ts @@ -4,6 +4,7 @@ import { queryClient } from '@/react-query/queryClient'; import { inputSchema } from '@harperfast/agent-tools/tools/setComponentFile/inputSchema'; import { z } from 'zod'; import { ExecuteParams } from '../../types/executeParams'; +import { describeToolError } from '../describeToolError'; import { Output } from './output'; export async function execute( @@ -27,7 +28,7 @@ export async function execute( } catch (err) { return { success: false, - message: `Error: ${err}`, + message: `Error: ${describeToolError(err)}`, }; } } diff --git a/src/features/instance/applications/components/Chat/tools/updateTableRecords/execute.ts b/src/features/instance/applications/components/Chat/tools/updateTableRecords/execute.ts index 730c8fbda..fe8fe2302 100644 --- a/src/features/instance/applications/components/Chat/tools/updateTableRecords/execute.ts +++ b/src/features/instance/applications/components/Chat/tools/updateTableRecords/execute.ts @@ -3,6 +3,7 @@ import { queryClient } from '@/react-query/queryClient'; import { inputSchema } from '@harperfast/agent-tools/tools/updateTableRecords/inputSchema'; import { z } from 'zod'; import { ExecuteParams } from '../../types/executeParams'; +import { describeToolError } from '../describeToolError'; import { Output } from './output'; export async function execute( @@ -24,7 +25,7 @@ export async function execute( } catch (err) { return { success: false, - message: `Error: ${err}`, + message: `Error: ${describeToolError(err)}`, }; } } diff --git a/src/features/instance/applications/components/NewApplication/useImportApplication.test.ts b/src/features/instance/applications/components/NewApplication/useImportApplication.test.ts index e7d4e149c..521ec726e 100644 --- a/src/features/instance/applications/components/NewApplication/useImportApplication.test.ts +++ b/src/features/instance/applications/components/NewApplication/useImportApplication.test.ts @@ -52,6 +52,7 @@ vi.mock('@/integrations/api/instance/applications/deployComponentStream', () => import { deployComponentStream } from '@/integrations/api/instance/applications/deployComponentStream'; import { SSEInconclusiveError } from '@/integrations/api/sse/errors'; +import { UNANSWERED_WRITE_MESSAGE } from '@/integrations/api/unansweredWrite'; import { mutationErrorHandler } from '@/react-query/queryClient'; import { MutationCache, QueryClient, QueryClientProvider } from '@tanstack/react-query'; import { act, renderHook, waitFor } from '@testing-library/react'; @@ -260,6 +261,25 @@ describe('useImportApplication', () => { expect(mocks.post).toHaveBeenCalledWith('/', expect.objectContaining({ activate: false }), expect.anything()); }); + it.each([ + [false, "Couldn't confirm my-app was imported"], + [true, "Couldn't confirm my-app was staged"], + ])('never calls a deploy that got no verdict failed (stage: %s)', async (stageOnly, title) => { + mocks.canStage.enabled = stageOnly; + const config = { + headers: new AxiosHeaders(), + method: 'post', + data: JSON.stringify({ operation: 'deploy_component' }), + } as InternalAxiosRequestConfig; + mocks.post.mockRejectedValue(new AxiosError('timeout of 300000ms exceeded', AxiosError.ECONNABORTED, config)); + const { result } = renderImport(); + + act(() => result.current.importApplication({ contents: { ...contents, stageOnly }, project: 'my-app' })); + + await waitFor(() => expect(result.current.importFailure).toEqual({ title, message: UNANSWERED_WRITE_MESSAGE })); + expect(vi.mocked(toast.error)).not.toHaveBeenCalled(); + }); + describe('a rollout that settles after the form has gone', () => { function deployThenFollow(job: () => Promise) { mocks.post.mockImplementation((_url: string, body: { operation: string }) => diff --git a/src/features/instance/applications/components/NewApplication/useImportApplication.ts b/src/features/instance/applications/components/NewApplication/useImportApplication.ts index 83ab4cd48..a02e5a833 100644 --- a/src/features/instance/applications/components/NewApplication/useImportApplication.ts +++ b/src/features/instance/applications/components/NewApplication/useImportApplication.ts @@ -12,6 +12,7 @@ import { DeployComponentResult } from '@/integrations/api/instance/applications/ import { deployFailureDetails } from '@/integrations/api/instance/applications/deployOutcome'; import { DeployRolloutError } from '@/integrations/api/instance/applications/restartJob'; import { SSEInconclusiveError } from '@/integrations/api/sse/errors'; +import { isUnansweredWrite } from '@/integrations/api/unansweredWrite'; import { invalidateEntityQueries } from '@/react-query/invalidateEntityQueries'; import { describeError } from '@/react-query/queryClient'; import { useQueryClient } from '@tanstack/react-query'; @@ -149,7 +150,10 @@ export function useImportApplication( }); return; } - const title = `Failed to ${stage ? 'stage' : 'import'} ${project}`; + // No verdict: it may have gone through, so the title can't say it failed; the message says to check. + const title = isUnansweredWrite(error) + ? `Couldn't confirm ${project} was ${stage ? 'staged' : 'imported'}` + : `Failed to ${stage ? 'stage' : 'import'} ${project}`; const { message } = describeError(error); if (formEntity.current === entityId) { setImportFailure({ entityId, failure: { title, message, deploymentId } }); diff --git a/src/features/instance/applications/hooks/useRenameFiles.test.tsx b/src/features/instance/applications/hooks/useRenameFiles.test.tsx new file mode 100644 index 000000000..62565e7d1 --- /dev/null +++ b/src/features/instance/applications/hooks/useRenameFiles.test.tsx @@ -0,0 +1,63 @@ +/** @vitest-environment jsdom */ +import { dropComponent } from '@/integrations/api/instance/applications/dropComponent'; +import { OUTCOME_UNKNOWN_TITLE, UNANSWERED_WRITE_MESSAGE } from '@/integrations/api/unansweredWrite'; +import { renderHook } from '@testing-library/react'; +import { AxiosError, type InternalAxiosRequestConfig } from 'axios'; +import { toast } from 'sonner'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { useRenameFiles } from './useRenameFiles'; + +vi.mock('@/config/useInstanceClient', () => ({ + useInstanceClientIdParams: () => ({ entityId: 'ins-1', entityType: 'instance' }), +})); +vi.mock('@/features/instance/applications/hooks/useEditorView', () => ({ + useEditorView: () => ({ reloadRootEntries: vi.fn(), setFocusedItem: vi.fn(), setSelectedItems: vi.fn() }), +})); +vi.mock('@/integrations/api/instance/applications/getComponentFile', () => ({ + getComponentFile: vi.fn().mockResolvedValue({ message: 'Y29udGVudHM=' }), +})); +vi.mock('@/integrations/api/instance/applications/setComponentFile', () => ({ + setComponentFile: vi.fn().mockResolvedValue({}), +})); +vi.mock('@/integrations/api/instance/applications/dropComponent', () => ({ dropComponent: vi.fn() })); +vi.mock('sonner', () => ({ toast: { loading: vi.fn(() => 'rename-toast'), error: vi.fn(), success: vi.fn() } })); + +const dropSent = { + method: 'post', + data: JSON.stringify({ operation: 'drop_component' }), +} as InternalAxiosRequestConfig; + +function rename() { + const { result } = renderHook(() => useRenameFiles()); + return result.current([{ from: 'app/old.js', to: 'app/new.js' }]); +} + +describe('useRenameFiles', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('says a rename whose last write got no answer may have gone through', async () => { + vi.mocked(dropComponent).mockRejectedValue( + new AxiosError('timeout of 60000ms exceeded', AxiosError.ECONNABORTED, dropSent), + ); + + await expect(rename()).resolves.toBe(false); + + expect(toast.error).toHaveBeenCalledWith( + OUTCOME_UNKNOWN_TITLE, + expect.objectContaining({ id: 'rename-toast', description: UNANSWERED_WRITE_MESSAGE }), + ); + }); + + it('still says a refused rename failed', async () => { + vi.mocked(dropComponent).mockRejectedValue(new Error('Unable to locate project')); + + await expect(rename()).resolves.toBe(false); + + expect(toast.error).toHaveBeenCalledWith( + 'Rename Failed', + expect.objectContaining({ id: 'rename-toast', description: 'Unable to locate project' }), + ); + }); +}); diff --git a/src/features/instance/applications/hooks/useRenameFiles.tsx b/src/features/instance/applications/hooks/useRenameFiles.tsx index 67380b1a8..64a458428 100644 --- a/src/features/instance/applications/hooks/useRenameFiles.tsx +++ b/src/features/instance/applications/hooks/useRenameFiles.tsx @@ -4,7 +4,9 @@ import { useEditorView } from '@/features/instance/applications/hooks/useEditorV import { dropComponent } from '@/integrations/api/instance/applications/dropComponent'; import { getComponentFile } from '@/integrations/api/instance/applications/getComponentFile'; import { setComponentFile } from '@/integrations/api/instance/applications/setComponentFile'; +import { isUnansweredWrite } from '@/integrations/api/unansweredWrite'; import { pluralize } from '@/lib/pluralize'; +import { describeError } from '@/react-query/queryClient'; import { useCallback } from 'react'; import { ExternalToast, toast } from 'sonner'; @@ -98,9 +100,13 @@ export function useRenameFiles() { stepForward(); } } catch (error) { - toast.error('Rename Failed', { - id: toastConfig.id, + const { title, description } = isUnansweredWrite(error) ? describeError(error) : { + title: 'Rename Failed', description: error instanceof Error ? error.message : 'An unexpected error occurred while renaming.', + }; + toast.error(title, { + id: toastConfig.id, + description, duration: 10000, action: { label: 'OK', onClick: () => undefined }, }); diff --git a/src/features/instance/applications/modals/RedeployApplicationModal.test.tsx b/src/features/instance/applications/modals/RedeployApplicationModal.test.tsx index 4d93c6d35..abea8df3f 100644 --- a/src/features/instance/applications/modals/RedeployApplicationModal.test.tsx +++ b/src/features/instance/applications/modals/RedeployApplicationModal.test.tsx @@ -51,6 +51,7 @@ import { deployComponentStream, } from '@/integrations/api/instance/applications/deployComponentStream'; import { SSEOperationError } from '@/integrations/api/sse/errors'; +import { UNANSWERED_WRITE_MESSAGE } from '@/integrations/api/unansweredWrite'; import { setWatchedValue } from '@/lib/events/watcher'; import { mutationErrorHandler } from '@/react-query/queryClient'; import { MutationCache, QueryClient, QueryClientProvider } from '@tanstack/react-query'; @@ -190,6 +191,50 @@ describe('RedeployApplicationModal', () => { }); }); + describe('a redeploy that got no verdict', () => { + const unanswered = () => + new AxiosError( + 'timeout of 300000ms exceeded', + AxiosError.ECONNABORTED, + { + headers: new AxiosHeaders(), + method: 'post', + data: JSON.stringify({ operation: 'deploy_component' }), + } as InternalAxiosRequestConfig, + ); + + it("replaces a buffered redeploy's progress toast without calling it failed", async () => { + mocks.sse.enabled = false; + mocks.post.mockRejectedValue(unanswered()); + await renderOpenModal(); + + await submit(); + + await waitFor(() => expect(vi.mocked(toast.error)).toHaveBeenCalledOnce()); + expect(vi.mocked(toast.error)).toHaveBeenCalledWith("Couldn't confirm my-app was redeployed", { + id: 'loading-toast', + description: UNANSWERED_WRITE_MESSAGE, + }); + }); + + it('toasts a streamed one that lands after the modal was closed without calling it failed', async () => { + const pending = deferred(); + vi.mocked(deployComponentStream).mockReturnValue(pending.promise); + await renderOpenModal(); + await submit(); + + await act(async () => { + fireEvent.keyDown(document.activeElement ?? document.body, { key: 'Escape' }); + }); + await act(async () => pending.reject(unanswered())); + + await waitFor(() => expect(vi.mocked(toast.error)).toHaveBeenCalledOnce()); + expect(vi.mocked(toast.error)).toHaveBeenCalledWith("Couldn't confirm my-app was redeployed", { + description: UNANSWERED_WRITE_MESSAGE, + }); + }); + }); + describe('a rollout that settles after the modal was closed', () => { function deployThenFollow(job: () => Promise) { vi.mocked(deployComponentStream).mockResolvedValue({ message: 'ok', deployment_id: 'd1', restartJobId: 'job-1' }); diff --git a/src/features/instance/applications/modals/RedeployApplicationModal.tsx b/src/features/instance/applications/modals/RedeployApplicationModal.tsx index ae8cd5e6a..3c4ec50a0 100644 --- a/src/features/instance/applications/modals/RedeployApplicationModal.tsx +++ b/src/features/instance/applications/modals/RedeployApplicationModal.tsx @@ -24,6 +24,7 @@ import { DeployComponentResult } from '@/integrations/api/instance/applications/ import { deployFailureDetails } from '@/integrations/api/instance/applications/deployOutcome'; import { DeployRolloutError } from '@/integrations/api/instance/applications/restartJob'; import { SSEInconclusiveError } from '@/integrations/api/sse/errors'; +import { isUnansweredWrite } from '@/integrations/api/unansweredWrite'; import { attemptToRestoreFocus } from '@/lib/attemptToRestoreFocus'; import { setWatchedValue, useWatchedValue } from '@/lib/events/watcher'; import { describeError } from '@/react-query/queryClient'; @@ -146,7 +147,7 @@ export function RedeployApplicationModal() { } const title = rolloutFailed ? `${project} did not reach every node` - : `Failed to ${stage ? 'stage' : 'redeploy'} ${project}`; + : redeployFailureTitle(error, project, stage); toast.error(title, { description: message, action: recordAction(deployFailureDetails(error).deploymentId) }); return; } @@ -204,7 +205,7 @@ export function RedeployApplicationModal() { invalidateComponents(); return; } - toast.error(`Failed to ${stage ? 'stage' : 'redeploy'} ${project}`, { + toast.error(redeployFailureTitle(error, project, stage), { id: toastId, description: describeError(error).message, action, @@ -359,3 +360,11 @@ export function RedeployApplicationModal() { ); } + +/** Never "Failed" for a deploy that got no verdict: it may be live, and the message says to check first. */ +function redeployFailureTitle(error: unknown, project: string, stage: boolean): string { + if (isUnansweredWrite(error)) { + return `Couldn't confirm ${project} was ${stage ? 'staged' : 'redeployed'}`; + } + return `Failed to ${stage ? 'stage' : 'redeploy'} ${project}`; +} diff --git a/src/features/instance/applications/modals/RenameFileModal.tsx b/src/features/instance/applications/modals/RenameFileModal.tsx index 2170399df..f4b04ba9d 100644 --- a/src/features/instance/applications/modals/RenameFileModal.tsx +++ b/src/features/instance/applications/modals/RenameFileModal.tsx @@ -22,9 +22,11 @@ import { useEditorView } from '@/features/instance/applications/hooks/useEditorV import { useRenameFiles } from '@/features/instance/applications/hooks/useRenameFiles'; import { dropComponent } from '@/integrations/api/instance/applications/dropComponent'; import { setComponentFile } from '@/integrations/api/instance/applications/setComponentFile'; +import { isUnansweredWrite } from '@/integrations/api/unansweredWrite'; import { attemptToRestoreFocus } from '@/lib/attemptToRestoreFocus'; import { setWatchedValue, useWatchedValue } from '@/lib/events/watcher'; import { renameFileInPath } from '@/lib/string/paths/renameFileInPath'; +import { describeError } from '@/react-query/queryClient'; import { zodResolver } from '@hookform/resolvers/zod'; import { PencilIcon } from 'lucide-react'; import { useCallback, useEffect, useState } from 'react'; @@ -123,9 +125,11 @@ export function RenameFileModal() { await dropComponent({ ...instanceParams, project: openedEntry.project, file: relative(openedEntry.path) }); await reloadRootEntries(); } catch (error) { - toast.error('Rename Failed', { + const { title, description } = isUnansweredWrite(error) ? describeError(error) : { + title: 'Rename Failed', description: error instanceof Error ? error.message : 'Could not remove the original directory.', - }); + }; + toast.error(title, { description }); setIsPending(false); return; } diff --git a/src/features/instance/config/secrets/SecretGrantsEditor.tsx b/src/features/instance/config/secrets/SecretGrantsEditor.tsx index 9525adb40..fd233d066 100644 --- a/src/features/instance/config/secrets/SecretGrantsEditor.tsx +++ b/src/features/instance/config/secrets/SecretGrantsEditor.tsx @@ -9,7 +9,7 @@ import { Badge } from '@/components/ui/badge'; import { useInstanceClientIdParams } from '@/config/useInstanceClient'; import { ComponentGrantCombobox } from '@/features/instance/secrets/ComponentGrantCombobox'; import { useGrantSecret, useRevokeSecret } from '@/integrations/api/instance/secrets/secrets'; -import { describeError } from '@/react-query/queryClient'; +import { describeError, shouldToastOwnFailure } from '@/react-query/queryClient'; import { XIcon } from 'lucide-react'; import { useCallback, useState } from 'react'; import { toast } from 'sonner'; @@ -41,7 +41,9 @@ export function SecretGrantsEditor({ setGrants(response.grants); onChanged?.(); } catch (error) { - toast.error(describeError(error).message); + if (shouldToastOwnFailure(error)) { + toast.error(describeError(error).message); + } // Rethrow so the combobox keeps the typed text for a retry instead of clearing it. throw error; } @@ -53,7 +55,9 @@ export function SecretGrantsEditor({ setGrants(response.grants); onChanged?.(); } catch (error) { - toast.error(describeError(error).message); + if (shouldToastOwnFailure(error)) { + toast.error(describeError(error).message); + } } }, [revokeSecret, instanceParams, name, onChanged]); diff --git a/src/features/instance/databases/components/DatabaseTableView.tsx b/src/features/instance/databases/components/DatabaseTableView.tsx index 3548f9b3d..6d1444560 100644 --- a/src/features/instance/databases/components/DatabaseTableView.tsx +++ b/src/features/instance/databases/components/DatabaseTableView.tsx @@ -70,6 +70,7 @@ import { keyBy } from '@/lib/keyBy'; import { onClickStopPropagation } from '@/lib/onClickStopPropagation'; import { pluralize } from '@/lib/pluralize'; import { Row } from '@/lib/table'; +import { shouldToastOwnFailure } from '@/react-query/queryClient'; import { zodResolver } from '@hookform/resolvers/zod'; import { useQuery, useQueryClient } from '@tanstack/react-query'; import { Link, useParams, useSearch } from '@tanstack/react-router'; @@ -645,7 +646,9 @@ export function DatabaseTableView({ refreshTable(); }, onError: (error) => { - toast.error(error instanceof Error ? error.message : 'Failed to cleanup orphan blobs'); + if (shouldToastOwnFailure(error)) { + toast.error(error instanceof Error ? error.message : 'Failed to cleanup orphan blobs'); + } }, }); }, [cleanupOrphanBlobs, databaseName, instanceParams, refreshTable]); diff --git a/src/features/instance/secrets/SecretModals.tsx b/src/features/instance/secrets/SecretModals.tsx index 8e247ec16..807875ea7 100644 --- a/src/features/instance/secrets/SecretModals.tsx +++ b/src/features/instance/secrets/SecretModals.tsx @@ -25,7 +25,7 @@ import { revealFieldErrorOnEnter } from '@/components/ui/form/revealFieldErrorOn import { Input } from '@/components/ui/input'; import { Textarea } from '@/components/ui/textarea'; import { ENV_KEY_REGEX, ENV_VALUE_MASK } from '@/lib/env/envFile'; -import { describeError } from '@/react-query/queryClient'; +import { describeError, shouldToastOwnFailure } from '@/react-query/queryClient'; import { zodResolver } from '@hookform/resolvers/zod'; import { EyeIcon, Save, Trash2 } from 'lucide-react'; import { ReactNode, useCallback, useMemo, useState } from 'react'; @@ -118,7 +118,9 @@ export function AddSecretModal({ toast.success(`Secret "${formData.key}" saved.`); setIsModalOpen(false); } catch (error) { - toast.error(describeError(error).message); + if (shouldToastOwnFailure(error)) { + toast.error(describeError(error).message); + } } }, [onSubmit, form, setIsModalOpen, delivery, tier, grants, resetDelivery], @@ -282,7 +284,9 @@ export function EditSecretModal({ toast.success(`Secret "${name}" updated.`); closeModal(); } catch (error) { - toast.error(describeError(error).message); + if (shouldToastOwnFailure(error)) { + toast.error(describeError(error).message); + } } }, [onSave, name, closeModal, delivery, tier], @@ -301,7 +305,9 @@ export function EditSecretModal({ toast.success(`Secret "${name}" deleted.`); closeModal(); } catch (error) { - toast.error(describeError(error).message); + if (shouldToastOwnFailure(error)) { + toast.error(describeError(error).message); + } } finally { setIsDeleting(false); } diff --git a/src/features/organization/roles/modals/AddOrganizationRoleModal.test.tsx b/src/features/organization/roles/modals/AddOrganizationRoleModal.test.tsx index 1c652ac50..b57320cca 100644 --- a/src/features/organization/roles/modals/AddOrganizationRoleModal.test.tsx +++ b/src/features/organization/roles/modals/AddOrganizationRoleModal.test.tsx @@ -13,10 +13,13 @@ vi.mock('@tanstack/react-query', async (importOriginal) => ({ ...(await importOriginal()), useQueryClient: () => ({ invalidateQueries: vi.fn() }), })); +const { mutate } = vi.hoisted(() => ({ mutate: vi.fn() })); vi.mock('@/features/organization/mutations/addOrganizationRole', () => ({ - useAddOrganizationRole: () => ({ mutate: vi.fn(), isPending: false }), + useAddOrganizationRole: () => ({ mutate, isPending: false }), })); +import { AxiosError, type InternalAxiosRequestConfig } from 'axios'; +import { toast } from 'sonner'; import { AddOrganizationRoleModal } from './AddOrganizationRoleModal'; beforeAll(() => { @@ -76,4 +79,27 @@ describe('AddOrganizationRoleModal', () => { expect(await screen.findByText(message)).toBeTruthy(); expect((screen.getByRole('button', { name: 'Save Changes' }) as HTMLButtonElement).disabled).toBe(true); }); + + it.each([ + ['a refused one says it failed', new Error('Role already exists'), 1], + [ + 'one that got no answer leaves it to the global toast', + new AxiosError('Network Error', AxiosError.ERR_NETWORK, { + method: 'post', + data: '{"name":"readers"}', + } as InternalAxiosRequestConfig), + 0, + ], + ])('of the failed adds, %s', async (_, error, ownToasts) => { + vi.mocked(toast.error).mockClear(); + mutate.mockImplementation((_data, { onError }) => onError(error)); + render( {}} />); + await typeRoleName('readers'); + await act(async () => { + fireEvent.click(screen.getByRole('button', { name: 'Save Changes' })); + }); + + expect(mutate).toHaveBeenCalled(); + expect(toast.error).toHaveBeenCalledTimes(ownToasts); + }); }); diff --git a/src/features/organization/roles/modals/AddOrganizationRoleModal.tsx b/src/features/organization/roles/modals/AddOrganizationRoleModal.tsx index 458c8baec..cde3e0687 100644 --- a/src/features/organization/roles/modals/AddOrganizationRoleModal.tsx +++ b/src/features/organization/roles/modals/AddOrganizationRoleModal.tsx @@ -19,6 +19,7 @@ import { import { useMonacoTheme } from '@/hooks/useMonacoTheme'; import { Editor } from '@/lib/monaco/MonacoEditor'; import { safeParse } from '@/lib/string/safeParse'; +import { shouldToastOwnFailure } from '@/react-query/queryClient'; import { zodResolver } from '@hookform/resolvers/zod'; import { useQueryClient } from '@tanstack/react-query'; import { useParams } from '@tanstack/react-router'; @@ -97,7 +98,9 @@ export function AddOrganizationRoleModal({ form.reset(); }, onError: (error: Error) => { - toast.error(`Failed to add organization role: ${error.message}`); + if (shouldToastOwnFailure(error)) { + toast.error(`Failed to add organization role: ${error.message}`); + } }, }); } diff --git a/src/features/organization/roles/modals/EditOrganizationRoleModal.tsx b/src/features/organization/roles/modals/EditOrganizationRoleModal.tsx index 2b7fbcaef..bfc9b24c6 100644 --- a/src/features/organization/roles/modals/EditOrganizationRoleModal.tsx +++ b/src/features/organization/roles/modals/EditOrganizationRoleModal.tsx @@ -23,6 +23,7 @@ import { useOrganizationRolePermissions } from '@/hooks/usePermissions'; import { SchemaOrganizationRole } from '@/integrations/api/api.gen'; import { Editor } from '@/lib/monaco/MonacoEditor'; import { safeParse } from '@/lib/string/safeParse'; +import { shouldToastOwnFailure } from '@/react-query/queryClient'; import { zodResolver } from '@hookform/resolvers/zod'; import { useQueryClient, useSuspenseQuery } from '@tanstack/react-query'; import { Suspense, useCallback, useMemo, useState } from 'react'; @@ -107,6 +108,9 @@ function EditOrganizationRoleModalContent({ form.reset(); }, onError: (error: Error | unknown) => { + if (!shouldToastOwnFailure(error)) { + return; + } toast.error('Error', { description: `Failed to delete role: ${error instanceof Error ? error.message : String(error)}.`, action: { @@ -173,6 +177,9 @@ function EditOrganizationRoleModalContent({ form.reset(); }, onError: (error: Error | unknown) => { + if (!shouldToastOwnFailure(error)) { + return; + } toast.error('Error', { description: `Failed to update role: ${error instanceof Error ? error.message : String(error)}.`, action: { diff --git a/src/features/organization/settings/index.tsx b/src/features/organization/settings/index.tsx index 62b910dc1..3e06351ca 100644 --- a/src/features/organization/settings/index.tsx +++ b/src/features/organization/settings/index.tsx @@ -18,6 +18,7 @@ import { useOrganizationPermissions } from '@/hooks/usePermissions'; import { SchemaOrganization } from '@/integrations/api/api.gen'; import { OAuthConfig } from '@/integrations/api/api.patch'; import { getOAuthSignInUrl } from '@/lib/urls/getOAuthSignInUrl'; +import { shouldToastOwnFailure } from '@/react-query/queryClient'; import { zodResolver } from '@hookform/resolvers/zod'; import { useMutation, useQueryClient, useSuspenseQuery } from '@tanstack/react-query'; import { useParams } from '@tanstack/react-router'; @@ -80,7 +81,11 @@ export function OrgSettingsIndex() { const { mutate: patch, isPending } = useMutation({ mutationFn: (payload: Partial & { provider: string }) => patchOAuthConfig(organizationId, payload), onSuccess: () => void queryClient.invalidateQueries({ queryKey: [organizationId] }), - onError: () => toast.error('Failed to update OAuth settings.'), + onError: (error) => { + if (shouldToastOwnFailure(error)) { + toast.error('Failed to update OAuth settings.'); + } + }, }); // Add diff --git a/src/features/organization/users/modals/EditUserModal.tsx b/src/features/organization/users/modals/EditUserModal.tsx index 22e71c4ff..7ade43c5e 100644 --- a/src/features/organization/users/modals/EditUserModal.tsx +++ b/src/features/organization/users/modals/EditUserModal.tsx @@ -10,6 +10,7 @@ import { useCloudAuth } from '@/hooks/useAuth'; import { useOrganizationRolePermissions } from '@/hooks/usePermissions'; import { SchemaUser } from '@/integrations/api/api.gen'; import { keyBy } from '@/lib/keyBy'; +import { shouldToastOwnFailure } from '@/react-query/queryClient'; import { useSuspenseQuery } from '@tanstack/react-query'; import { useParams } from '@tanstack/react-router'; import { AxiosError } from 'axios'; @@ -59,16 +60,16 @@ export function EditUserModal({ const results = await Promise.allSettled( roles.map((role) => removeUserFromRole({ userId: data.id, roleId: role.id })), ); - const failed = results.some( - (result) => result.status === 'rejected' && (result.reason as AxiosError)?.response?.status !== 404, + const failures = results.flatMap((result) => + result.status === 'rejected' && (result.reason as AxiosError)?.response?.status !== 404 ? [result.reason] : [] ); - if (failed) { + if (failures.some(shouldToastOwnFailure)) { toast.error( isSelf ? 'Something went wrong leaving the organization. Please try again.' : `Couldn’t remove ${data.email} from the organization. Please try again.`, ); - } else { + } else if (failures.length === 0) { toast.success( isSelf ? 'You’ve left the organization.' diff --git a/src/hooks/useApplyLicensesClick.test.tsx b/src/hooks/useApplyLicensesClick.test.tsx index 87f4f5a62..7c1c401e2 100644 --- a/src/hooks/useApplyLicensesClick.test.tsx +++ b/src/hooks/useApplyLicensesClick.test.tsx @@ -2,8 +2,10 @@ * @vitest-environment jsdom */ import type { SchemaLicense } from '@/integrations/api/api.gen'; +import { OUTCOME_UNKNOWN_TITLE } from '@/integrations/api/unansweredWrite'; import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; import { act, renderHook } from '@testing-library/react'; +import { AxiosError, type InternalAxiosRequestConfig } from 'axios'; import { PropsWithChildren } from 'react'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; @@ -28,6 +30,7 @@ import { toast } from 'sonner'; import { useApplyLicensesClick } from './useApplyLicensesClick'; const LICENSES = [{ license: 'lic-1' }, { license: 'lic-2' }] as SchemaLicense[]; +const installSent = { method: 'post', data: '{"operation":"install_usage_license"}' } as InternalAxiosRequestConfig; function wrapper() { const queryClient = new QueryClient(); @@ -70,6 +73,33 @@ describe('useApplyLicensesClick', () => { expect(installUsageLicense).not.toHaveBeenCalled(); expect(toast.error).toHaveBeenCalledWith('Cancelled', expect.anything()); }); + + it('counts a license that got no answer apart from one that failed', async () => { + vi.mocked(installUsageLicense) + .mockResolvedValueOnce({} as never) + .mockRejectedValueOnce(new AxiosError('timeout of 60000ms exceeded', AxiosError.ECONNABORTED, installSent)); + + await applyLicenses(); + + const [title, options] = vi.mocked(toast.error).mock.calls.at(-1)!; + expect(title).toBe(OUTCOME_UNKNOWN_TITLE); + expect(options?.description).toBe( + "1 of 2 licenses confirmed applied. 1 request couldn't be confirmed, so it may have gone through. " + + 'Check before you try again, or it may run twice.', + ); + }); + + it('still says a refused license was not applied', async () => { + vi.mocked(installUsageLicense) + .mockResolvedValueOnce({} as never) + .mockRejectedValueOnce(new Error('License is expired')); + + await applyLicenses(); + + const [title, options] = vi.mocked(toast.error).mock.calls.at(-1)!; + expect(title).toBe('Error'); + expect(options?.description).toBe('Licenses not applied.\n1 of 2 licenses applied.'); + }); }); function cancel() { diff --git a/src/hooks/useApplyLicensesClick.tsx b/src/hooks/useApplyLicensesClick.tsx index 493838764..19c8ee0aa 100644 --- a/src/hooks/useApplyLicensesClick.tsx +++ b/src/hooks/useApplyLicensesClick.tsx @@ -3,6 +3,7 @@ import { useInstanceClient, useInstanceClientIdParams } from '@/config/useInstan import { SchemaLicense } from '@/integrations/api/api.gen'; import { installUsageLicense } from '@/integrations/api/instance/auth/installUsageLicense'; import { getInstanceUserInfo } from '@/integrations/api/instance/status/getInstanceUserInfo'; +import { describeUnansweredWrites, isUnansweredWrite, OUTCOME_UNKNOWN_TITLE } from '@/integrations/api/unansweredWrite'; import { excludeFalsy } from '@/lib/arrays/excludeFalsy'; import { sleep } from '@/lib/sleep'; import { useQueryClient } from '@tanstack/react-query'; @@ -50,6 +51,7 @@ export function useApplyLicensesClick({ licenses }: ApplyLicensesClickParams): A }); let licensesApplied = 0; + let licensesUnanswered = 0; for (let i = 0; i < licenses.length; i++) { const license = licenses[i]; @@ -83,10 +85,13 @@ export function useApplyLicensesClick({ licenses }: ApplyLicensesClickParams): A instanceClient, }); licensesApplied += 1; - } catch { + } catch (error) { + const unanswered = isUnansweredWrite(error); + licensesUnanswered += unanswered ? 1 : 0; if (i + 1 !== licenses.length) { // If it fails to applyLicenses, or wasn't available, warn for a bit then move on. - toast.loading(`Failed Applying License ${i + 1} of ${licenses.length}`, { + const verdict = unanswered ? 'Could Not Confirm' : 'Failed Applying'; + toast.loading(`${verdict} License ${i + 1} of ${licenses.length}`, { ...toastConfig, id: toastId, description: 'We will carry on momentarily.', @@ -127,13 +132,16 @@ export function useApplyLicensesClick({ licenses }: ApplyLicensesClickParams): A }, }); } else { - toast.error('Error', { + toast.error(licensesUnanswered ? OUTCOME_UNKNOWN_TITLE : 'Error', { id: toastId, - description: `${licenseWord} not applied.\n` - + ([ - licensesApplied > 0 && licenses.length !== licensesApplied - && `${licensesApplied} of ${licenses.length} ${licenseWord.toLowerCase()} applied.`, - ].filter(excludeFalsy).shift() || ''), + description: licensesUnanswered + ? `${licensesApplied} of ${licenses.length} ${licenseWord.toLowerCase()} confirmed applied. ` + + describeUnansweredWrites(licensesUnanswered) + : `${licenseWord} not applied.\n` + + ([ + licensesApplied > 0 && licenses.length !== licensesApplied + && `${licensesApplied} of ${licenses.length} ${licenseWord.toLowerCase()} applied.`, + ].filter(excludeFalsy).shift() || ''), duration: 10_000, action: { label: 'Dismiss', diff --git a/src/hooks/useRestartClusterClick.test.tsx b/src/hooks/useRestartClusterClick.test.tsx index 5d89b76a3..0d0000e7a 100644 --- a/src/hooks/useRestartClusterClick.test.tsx +++ b/src/hooks/useRestartClusterClick.test.tsx @@ -2,9 +2,10 @@ * @vitest-environment jsdom */ import type { Cluster, Instance } from '@/integrations/api/api.patch'; +import { describeUnansweredWrites, OUTCOME_UNKNOWN_TITLE } from '@/integrations/api/unansweredWrite'; import { getRestartState } from '@/lib/restart/restartTracker'; import { act, renderHook } from '@testing-library/react'; -import type { AxiosInstance } from 'axios'; +import { AxiosError, type AxiosInstance, type InternalAxiosRequestConfig } from 'axios'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; vi.mock('@tanstack/react-router', () => ({ @@ -124,6 +125,33 @@ describe('useRestartClusterClick', () => { expect(options?.description).not.toContain('No instances were in a "RUNNING" state'); }); + it('counts a member whose restart got no verdict apart from one that failed', async () => { + vi.mocked(getClusterInfo).mockResolvedValue( + { id: 'clu-1', instances: [instance('ins-a'), instance('ins-b')] } as Cluster, + ); + vi.mocked(restartInstance).mockImplementation(async ({ instanceClient }) => { + if (targetOf(instanceClient) === 'ins-b') { + throw new AxiosError('timeout of 60000ms exceeded', AxiosError.ECONNABORTED, { + method: 'post', + data: '{"operation":"restart"}', + } as InternalAxiosRequestConfig); + } + restarted.push(targetOf(instanceClient)); + return { message: 'ok' }; + }); + + await clickRestart(); + + expect(restarted).toEqual(['ins-a']); + expect(toast.loading).toHaveBeenCalledWith('Could Not Confirm Instance 1 of 2', expect.anything()); + const [title, options] = vi.mocked(toast.error).mock.calls.at(-1)!; + expect(title).toBe(OUTCOME_UNKNOWN_TITLE); + expect(options?.description).toBe( + 'Only 1 "RUNNING" instance was confirmed restarted of 2 instances. ' + + describeUnansweredWrites(1), + ); + }); + it('reports a cluster with no instances as a failure, not a success', async () => { vi.mocked(getClusterInfo).mockResolvedValue({ id: 'clu-1', instances: [] } as unknown as Cluster); diff --git a/src/hooks/useRestartClusterClick.tsx b/src/hooks/useRestartClusterClick.tsx index ca5eefc76..dc4b12471 100644 --- a/src/hooks/useRestartClusterClick.tsx +++ b/src/hooks/useRestartClusterClick.tsx @@ -2,6 +2,7 @@ import { ProgressBar } from '@/components/ProgressBar'; import { ClusterRestartTargets, getClusterRestartTargets } from '@/hooks/getClusterRestartTargets'; import { getInstanceUserInfo } from '@/integrations/api/instance/status/getInstanceUserInfo'; import { restartInstance } from '@/integrations/api/instance/status/restartInstance'; +import { describeUnansweredWrites, isUnansweredWrite, OUTCOME_UNKNOWN_TITLE } from '@/integrations/api/unansweredWrite'; import { excludeFalsy } from '@/lib/arrays/excludeFalsy'; import { pluralize } from '@/lib/pluralize'; import { markRestarting } from '@/lib/restart/restartTracker'; @@ -72,6 +73,7 @@ export function useRestartClusterClick( } const { allInstances, targets } = plan; let instancesRestarted = 0; + let instancesUnanswered = 0; // One instance at a time, so the cluster stays reachable through the others unless it has only // one. The TTLs only matter if this loop dies without reaching `finally`. @@ -116,10 +118,13 @@ export function useRestartClusterClick( instanceClient, }); instancesRestarted += 1; - } catch { + } catch (error) { + const unanswered = isUnansweredWrite(error); + instancesUnanswered += unanswered ? 1 : 0; if (i + 1 !== targets.length) { // If it fails to restart, or wasn't available, warn for a bit then move on. - toast.loading(`Failed Restarting Instance ${i + 1} of ${targets.length}`, { + 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.', @@ -164,15 +169,18 @@ export function useRestartClusterClick( '"RUNNING" instance was', '"RUNNING" instances were', ); - toast.error('Error', { + toast.error(instancesUnanswered ? OUTCOME_UNKNOWN_TITLE : 'Error', { id: toastId, - 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}.`, - allInstances.length !== instancesRestarted - && `Only ${someRunningInstancesWere} restarted of ${allTheInstances}.`, - ].filter(excludeFalsy).shift() || ''), + description: instancesUnanswered + ? `Only ${someRunningInstancesWere} confirmed restarted of ${allTheInstances}. ` + + describeUnansweredWrites(instancesUnanswered) + : `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}.`, + allInstances.length !== instancesRestarted + && `Only ${someRunningInstancesWere} restarted of ${allTheInstances}.`, + ].filter(excludeFalsy).shift() || ''), duration: 10_000, action: { label: 'Dismiss', diff --git a/src/hooks/useRestartInstanceClick.test.tsx b/src/hooks/useRestartInstanceClick.test.tsx new file mode 100644 index 000000000..9845146b5 --- /dev/null +++ b/src/hooks/useRestartInstanceClick.test.tsx @@ -0,0 +1,77 @@ +/** @vitest-environment jsdom */ +import { useRestartInstanceClick } from '@/hooks/useRestartInstanceClick'; +import { OUTCOME_UNKNOWN_TITLE } from '@/integrations/api/unansweredWrite'; +import { resetRestartTracker } from '@/lib/restart/restartTracker'; +import { mutationErrorHandler } from '@/react-query/queryClient'; +import { MutationCache, QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import { act, renderHook } from '@testing-library/react'; +import { AxiosError, type AxiosInstance, type InternalAxiosRequestConfig } from 'axios'; +import { PropsWithChildren } from 'react'; +import { toast } from 'sonner'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +vi.mock('@tanstack/react-router', () => ({ + useParams: () => ({ clusterId: 'clu-1', instanceId: 'ins-1' }), +})); + +vi.mock('sonner', () => ({ + toast: { loading: vi.fn(() => 'restart-toast'), error: vi.fn(), success: vi.fn(), dismiss: vi.fn() }, +})); + +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 render(instanceClient: AxiosInstance) { + const queryClient = new QueryClient({ mutationCache: new MutationCache({ onError: mutationErrorHandler }) }); + const wrapper = ({ children }: PropsWithChildren) => ( + {children} + ); + return renderHook(() => useRestartInstanceClick({ operation: 'restart', instanceClient }), { wrapper }).result; +} + +function errorTitles() { + return vi.mocked(toast.error).mock.calls.map(([title, options]) => [title, options?.description]); +} + +describe('useRestartInstanceClick', () => { + beforeEach(() => { + vi.clearAllMocks(); + resetRestartTracker(); + vi.spyOn(console, 'error').mockImplementation(() => undefined); + }); + afterEach(() => { + vi.restoreAllMocks(); + }); + + it('does not call a restart that got no answer a failure', async () => { + const result = render( + restartAnswering((config) => new AxiosError('timeout of 60000ms exceeded', AxiosError.ECONNABORTED, config)), + ); + + await act(() => result.current.onRestartClick()); + + expect(errorTitles()).toEqual([[OUTCOME_UNKNOWN_TITLE, expect.stringContaining("couldn't confirm whether")]]); + expect(toast.dismiss).toHaveBeenCalledWith('restart-toast'); + }); + + it('still says a refused restart failed', async () => { + const result = render( + restartAnswering((config) => + 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, + }) + ), + ); + + await act(() => result.current.onRestartClick()); + + expect(errorTitles()).toContainEqual(['Error', 'Failed to restart instance.']); + expect(errorTitles().map(([title]) => title)).not.toContain(OUTCOME_UNKNOWN_TITLE); + }); +}); diff --git a/src/hooks/useRestartInstanceClick.ts b/src/hooks/useRestartInstanceClick.ts index a8094d17c..6b50bd207 100644 --- a/src/hooks/useRestartInstanceClick.ts +++ b/src/hooks/useRestartInstanceClick.ts @@ -4,6 +4,7 @@ import { OverallAppSignIn } from '@/features/auth/store/authStore'; import { useRestartInstance } from '@/integrations/api/instance/status/restartInstance'; import { markRestarting } from '@/lib/restart/restartTracker'; import { invalidateEntityQueries } from '@/react-query/invalidateEntityQueries'; +import { shouldToastOwnFailure } from '@/react-query/queryClient'; import { useQueryClient } from '@tanstack/react-query'; import { useParams } from '@tanstack/react-router'; import { useCallback } from 'react'; @@ -69,15 +70,17 @@ export function useRestartInstanceClick({ }, }); onRestartedSuccessfully?.(); - } catch { + } catch (error) { toast.dismiss(toastId); - toast.error('Error', { - description: `Failed to restart ${targetNoun.toLowerCase()}.`, - action: { - label: 'Dismiss', - onClick: () => toast.dismiss(), - }, - }); + if (shouldToastOwnFailure(error)) { + toast.error('Error', { + description: `Failed to restart ${targetNoun.toLowerCase()}.`, + action: { + label: 'Dismiss', + onClick: () => toast.dismiss(), + }, + }); + } } finally { // Released on failure too: once the restart has failed, the errors that follow are real. releaseTarget?.(); diff --git a/src/hooks/useRollingConfigUpdate.test.tsx b/src/hooks/useRollingConfigUpdate.test.tsx index f2560055b..437b9d693 100644 --- a/src/hooks/useRollingConfigUpdate.test.tsx +++ b/src/hooks/useRollingConfigUpdate.test.tsx @@ -2,9 +2,10 @@ * @vitest-environment jsdom */ import type { Cluster, Instance } from '@/integrations/api/api.patch'; +import { describeUnansweredWrites, OUTCOME_UNKNOWN_TITLE } from '@/integrations/api/unansweredWrite'; import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; import { act, renderHook } from '@testing-library/react'; -import type { AxiosInstance } from 'axios'; +import { AxiosError, type AxiosInstance, type InternalAxiosRequestConfig } from 'axios'; import { PropsWithChildren } from 'react'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; @@ -445,6 +446,29 @@ describe('useRollingConfigUpdate outcome reporting', () => { expect(options?.description).toContain('Only 0 "RUNNING" instances were restarted of 2 instances.'); expect(options?.description).not.toContain('No instances were in a "RUNNING" state'); }); + + it('counts a member whose restart got no verdict apart from one that failed', async () => { + useCluster([instance('ins-a', '5.1.29'), instance('ins-b', '5.1.29')]); + vi.mocked(restartInstance).mockImplementation(async ({ instanceClient }) => { + if (targetOf(instanceClient) === 'ins-b') { + throw new AxiosError('timeout of 60000ms exceeded', AxiosError.ECONNABORTED, { + method: 'post', + data: '{"operation":"restart_service"}', + } as InternalAxiosRequestConfig); + } + return { message: 'ok' }; + }); + + await save(); + + expect(toast.loading).toHaveBeenCalledWith('Could Not Confirm Instance 1 of 2', expect.anything()); + const [title, options] = vi.mocked(toast.error).mock.calls.at(-1)!; + expect(title).toBe(OUTCOME_UNKNOWN_TITLE); + expect(options?.description).toBe( + 'Only 1 "RUNNING" instance was confirmed updated of 2 instances. ' + + describeUnansweredWrites(1), + ); + }); }); describe('useRollingConfigUpdate cancellation', () => { diff --git a/src/hooks/useRollingConfigUpdate.tsx b/src/hooks/useRollingConfigUpdate.tsx index 12e8fe22a..e74a23907 100644 --- a/src/hooks/useRollingConfigUpdate.tsx +++ b/src/hooks/useRollingConfigUpdate.tsx @@ -7,6 +7,7 @@ import { getInstanceUserInfo } from '@/integrations/api/instance/status/getInsta import { restartInstance } from '@/integrations/api/instance/status/restartInstance'; 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 { excludeFalsy } from '@/lib/arrays/excludeFalsy'; import { pluralize } from '@/lib/pluralize'; import { sleep } from '@/lib/sleep'; @@ -109,6 +110,7 @@ export function useRollingConfigUpdate( let fanOutAttempted = false; let acknowledged = new Set(); let instancesRestarted = 0; + let instancesUnanswered = 0; for (let i = 0; i < targets.length; i++) { const { instanceClient, replicationFqdn } = targets[i]; @@ -149,10 +151,13 @@ export function useRollingConfigUpdate( }); instancesRestarted += 1; restarted = true; - } catch { + } catch (error) { + const unanswered = isUnansweredWrite(error); + instancesUnanswered += unanswered ? 1 : 0; if (i + 1 !== targets.length) { // If it fails to restart, or wasn't available, warn for a bit then move on. - toast.loading(`Failed Restarting Instance ${i + 1} of ${targets.length}`, { + 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.', @@ -211,15 +216,18 @@ export function useRollingConfigUpdate( '"RUNNING" instance was', '"RUNNING" instances were', ); - toast.error('Error', { + toast.error(instancesUnanswered ? OUTCOME_UNKNOWN_TITLE : 'Error', { id: toastId, - 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() || '') + description: (instancesUnanswered + ? `Only ${someRunningInstancesWere} confirmed updated of ${allTheInstances}. ` + + describeUnansweredWrites(instancesUnanswered) + : `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 : ''), duration: 10_000, action: { diff --git a/src/integrations/api/instance/applications/packageComponent.ts b/src/integrations/api/instance/applications/packageComponent.ts index 4b1d2c421..f1b0928c1 100644 --- a/src/integrations/api/instance/applications/packageComponent.ts +++ b/src/integrations/api/instance/applications/packageComponent.ts @@ -28,7 +28,8 @@ async function packageComponent({ skip_node_modules: skipNodeModules, skip_symlinks: skipSymlinks, }, - { timeout: 300_000 }, + // A read, but every attempt builds the whole archive again. + { timeout: 300_000, skipGatewayRetry: true }, ); return data; } diff --git a/src/integrations/api/instance/secrets/secrets.test.tsx b/src/integrations/api/instance/secrets/secrets.test.tsx index cb5e310f4..7143746b1 100644 --- a/src/integrations/api/instance/secrets/secrets.test.tsx +++ b/src/integrations/api/instance/secrets/secrets.test.tsx @@ -2,9 +2,10 @@ * @vitest-environment jsdom */ import { EntityIds } from '@/features/auth/store/authStore'; +import { isUnansweredWrite } from '@/integrations/api/unansweredWrite'; import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; import { renderHook, waitFor } from '@testing-library/react'; -import type { AxiosInstance } from 'axios'; +import { AxiosError, type AxiosInstance, type InternalAxiosRequestConfig } from 'axios'; import { webcrypto } from 'node:crypto'; import { PropsWithChildren } from 'react'; import { afterEach, beforeAll, describe, expect, it, vi } from 'vitest'; @@ -278,3 +279,55 @@ describe.each( await expect(result.current.mutateAsync({ ...params, name: 'API_KEY', component: 'app' })).rejects.toBe(failure); }); }); + +describe('a secrets write that got no verdict', () => { + function unanswered(body: { operation: string }) { + const config = { method: 'post', data: JSON.stringify(body) } as InternalAxiosRequestConfig; + return new AxiosError('timeout of 60000ms exceeded', AxiosError.ECONNABORTED, config); + } + + it('rejects with the AxiosError itself, so its failure can say the outcome is unknown', async () => { + const { post, wrapper, params } = harness(); + const key = await generateKey(); + post.mockImplementation((_url: string, body: { operation: string }) => { + if (body.operation === 'get_secrets_public_key') { + return Promise.resolve({ data: { public_key: key.pem, fingerprint: key.fingerprint } }); + } + return Promise.reject(unanswered(body)); + }); + + const set = renderHook(() => useSetSecret(), { wrapper }).result; + const remove = renderHook(() => useDeleteSecret(), { wrapper }).result; + const grant = renderHook(() => useGrantSecret(), { wrapper }).result; + const failures = await Promise.all([ + set.current.mutateAsync({ ...params, name: 'API_KEY', value: 'x' }).catch((e: unknown) => e), + remove.current.mutateAsync({ ...params, name: 'API_KEY' }).catch((e: unknown) => e), + grant.current.mutateAsync({ ...params, name: 'API_KEY', component: 'app' }).catch((e: unknown) => e), + ]); + + for (const failure of failures) { + expect(isUnansweredWrite(failure)).toBe(true); + } + }); + + // Central manager's proxy answers a 504 with a JSON `error` after forwarding the operation, so the + // body has the shape a Harper refusal has; the status, not the body, decides. + it('keeps a gateway 504 that carries a JSON message as the AxiosError, not a SecretsOperationError', async () => { + const { post, wrapper, params } = harness(); + const config = { + method: 'post', + data: JSON.stringify({ operation: 'delete_secret' }), + } as InternalAxiosRequestConfig; + const gateway = new AxiosError('Request failed with status code 504', AxiosError.ERR_BAD_RESPONSE, config, null, { + status: 504, + statusText: 'Gateway Timeout', + headers: {}, + config, + data: { error: 'Proxied operation timed out after 30s on instance ins-1' }, + }); + post.mockRejectedValueOnce(gateway); + + const { result } = renderHook(() => useDeleteSecret(), { wrapper }); + await expect(result.current.mutateAsync({ ...params, name: 'API_KEY' })).rejects.toBe(gateway); + }); +}); diff --git a/src/integrations/api/instance/secrets/secrets.ts b/src/integrations/api/instance/secrets/secrets.ts index 5957c6f72..e4b71a30c 100644 --- a/src/integrations/api/instance/secrets/secrets.ts +++ b/src/integrations/api/instance/secrets/secrets.ts @@ -19,6 +19,7 @@ */ import { apiClient } from '@/config/apiClient'; import { InstanceClientIdConfig } from '@/config/instanceClientConfig'; +import { isUnansweredWrite } from '@/integrations/api/unansweredWrite'; import { encryptEnvelope } from '@/lib/crypto/envSecret'; import { QueryClient, queryOptions, useMutation, useQueryClient } from '@tanstack/react-query'; import type { AxiosInstance } from 'axios'; @@ -65,9 +66,14 @@ export class SecretsOperationError extends Error { /** * Operations API errors carry Harper's message in the response body; surface that, not the Axios * noise. A failure with no such message (the network, a bug) is not Harper's text, so it stays as - * thrown and Error Tracking keeps its type and message. + * thrown and Error Tracking keeps its type and message. A write that got no verdict is rethrown as + * is too, even when a gateway body carries a message: re-wrapped, nothing downstream could say its + * outcome is unknown. */ function operationError(error: unknown): unknown { + if (isUnansweredWrite(error)) { + return error; + } const body = (error as { response?: { data?: { error?: unknown; message?: unknown } } })?.response?.data; const message = typeof body?.error === 'string' ? body.error : body?.message; return typeof message === 'string' ? new SecretsOperationError(message) : error; diff --git a/src/integrations/api/isReadOperation.test.ts b/src/integrations/api/isReadOperation.test.ts index 79f87b361..908069bbd 100644 --- a/src/integrations/api/isReadOperation.test.ts +++ b/src/integrations/api/isReadOperation.test.ts @@ -14,6 +14,7 @@ describe('isReadOperation', () => { 'system_information', 'cluster_status', 'registration_info', + 'package_component', ])('reads %s as a read', (operation) => { expect(isReadOperation(operation)).toBe(true); }); @@ -32,6 +33,7 @@ describe('isReadOperation', () => { 'my_component_operation', 'user_info_reset', 'registration_information', + 'package_component_project', ])('treats %s as able to change something', (operation) => { expect(isReadOperation(operation)).toBe(false); }); diff --git a/src/integrations/api/isReadOperation.ts b/src/integrations/api/isReadOperation.ts index 274dda618..17f0de160 100644 --- a/src/integrations/api/isReadOperation.ts +++ b/src/integrations/api/isReadOperation.ts @@ -3,10 +3,11 @@ * while the entity restarts), the gateway retry replays only these, and the operations console asks * before sending anything else. Token minting is deliberately not one — held, it would stall a route * guard's Fabric Connect attempt behind a notice that cannot mount yet; refused, the guard falls - * through to sign-in at once. + * through to sign-in at once. `package_component` reads despite its name: it packs a project's files + * into an archive and changes nothing. */ const READ_OPERATION = - /^(?:get_|search_|describe_|list_|read_|user_info$|system_information$|cluster_status$|registration_info$)/; + /^(?:get_|search_|describe_|list_|read_|user_info$|system_information$|cluster_status$|registration_info$|package_component$)/; export function isReadOperation(operation: unknown): boolean { return typeof operation === 'string' && READ_OPERATION.test(operation); diff --git a/src/integrations/api/retryGatewayErrors.ts b/src/integrations/api/retryGatewayErrors.ts index 766c2838d..85d438e5c 100644 --- a/src/integrations/api/retryGatewayErrors.ts +++ b/src/integrations/api/retryGatewayErrors.ts @@ -7,6 +7,7 @@ declare module 'axios' { /** * Send at most once, even a read. The operations console sets it: its freeform body can name a * component's operation, and a `get_` name from a component is no proof that it only reads. + * `package_component` sets it too, a read that rebuilds the whole archive on every attempt. */ skipGatewayRetry?: boolean; } diff --git a/src/integrations/api/unansweredWrite.integration.test.tsx b/src/integrations/api/unansweredWrite.integration.test.tsx new file mode 100644 index 000000000..000ad0c35 --- /dev/null +++ b/src/integrations/api/unansweredWrite.integration.test.tsx @@ -0,0 +1,118 @@ +/** @vitest-environment jsdom */ +import { getInstanceClient } from '@/config/getInstanceClient'; +import { usePackageComponentMutation } from '@/integrations/api/instance/applications/packageComponent'; +import { OUTCOME_UNKNOWN_TITLE, UNANSWERED_WRITE_MESSAGE } from '@/integrations/api/unansweredWrite'; +import { describeError, mutationErrorHandler } from '@/react-query/queryClient'; +import { MutationCache, MutationObserver, QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import { renderHook } from '@testing-library/react'; +import { AxiosError, type InternalAxiosRequestConfig } from 'axios'; +import { PropsWithChildren } from 'react'; +import { toast } from 'sonner'; +import { afterEach, beforeEach, describe, expect, it, MockInstance, vi } from 'vitest'; + +vi.mock('@/lib/sleep', () => ({ + sleep: vi.fn().mockResolvedValue(true), +})); + +vi.mock('sonner', () => ({ + toast: { error: vi.fn(), dismiss: vi.fn() }, +})); + +const GATEWAY_BODY = '504 Gateway Time-out'; + +/** A real instance client whose transport fails every request the way `fail` says. */ +function instanceClient(fail: (config: InternalAxiosRequestConfig) => never) { + const client = getInstanceClient({ id: 'ins-123', operationsUrl: 'https://ins-123.example.com:9925/' }); + client.defaults.adapter = async (config: InternalAxiosRequestConfig) => fail(config); + return client; +} + +function answering(status: number, data: unknown = GATEWAY_BODY) { + return (config: InternalAxiosRequestConfig): never => { + 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 60000ms exceeded', AxiosError.ECONNABORTED, config); +} + +async function failureOf(promise: Promise): Promise { + return promise.then(() => expect.unreachable('the request should have failed'), (error: unknown) => error); +} + +describe('a write that got no verdict, through a real instance client', () => { + let consoleMock: MockInstance; + + beforeEach(() => { + vi.clearAllMocks(); + consoleMock = vi.spyOn(console, 'error').mockImplementation(() => undefined); + }); + + afterEach(() => { + consoleMock.mockRestore(); + }); + + it.each([ + ['a 504 on insert', answering(504), 'insert'], + ['a 502 on deploy_component', answering(502), 'deploy_component'], + ['a 503 on restart', answering(503, 'The worker thread executing this operation exited'), 'restart'], + ['a timeout on drop_table', timingOut, 'drop_table'], + ])('toasts %s as an unknown outcome, never the gateway body', async (_label, fail, operation) => { + const client = instanceClient(fail); + const mutationClient = new QueryClient({ mutationCache: new MutationCache({ onError: mutationErrorHandler }) }); + const observer = new MutationObserver(mutationClient, { + mutationFn: () => client.post('/', { operation }), + }); + + await expect(observer.mutate()).rejects.toBeInstanceOf(AxiosError); + + expect(toast.error).toHaveBeenCalledOnce(); + expect(toast.error).toHaveBeenCalledWith(OUTCOME_UNKNOWN_TITLE, { + id: undefined, + description: UNANSWERED_WRITE_MESSAGE, + action: { label: 'Dismiss', onClick: expect.any(Function) }, + }); + }); + + it('keeps the server’s words for a write it refused', async () => { + const client = instanceClient(answering(400, { error: 'Table "dog" already exists' })); + + const error = await failureOf(client.post('/', { operation: 'create_table', table: 'dog' })); + + expect(describeError(error).message).toBe('Table "dog" already exists'); + }); + + it('keeps a read’s own failure text once the gateway retry gives up on it', async () => { + const client = instanceClient(answering(504)); + + const error = await failureOf(client.post('/', { operation: 'describe_all' })); + + expect(describeError(error).message).toBe(GATEWAY_BODY); + }); + + it('sends a package download once, and never says it may have gone through', async () => { + const sent: string[] = []; + const client = instanceClient((config) => { + sent.push(String(config.data)); + return answering(504)(config); + }); + const queryClient = new QueryClient(); + const { result } = renderHook(() => usePackageComponentMutation(), { + wrapper: ({ children }: PropsWithChildren) => ( + {children} + ), + }); + + const error = await failureOf(result.current.mutateAsync({ project: 'app', instanceClient: client })); + + expect(sent).toHaveLength(1); + expect(describeError(error).message).toBe(GATEWAY_BODY); + }); +}); diff --git a/src/integrations/api/unansweredWrite.test.ts b/src/integrations/api/unansweredWrite.test.ts new file mode 100644 index 000000000..a26bafe0b --- /dev/null +++ b/src/integrations/api/unansweredWrite.test.ts @@ -0,0 +1,71 @@ +import { isUnansweredWrite } from '@/integrations/api/unansweredWrite'; +import { AxiosError, type InternalAxiosRequestConfig } from 'axios'; +import { describe, expect, it } from 'vitest'; + +function config(data: unknown, method = 'post'): InternalAxiosRequestConfig { + return { method, data } as InternalAxiosRequestConfig; +} + +function answered(status: number, requestConfig = config({ operation: 'insert' })) { + return new AxiosError( + `Request failed with status code ${status}`, + status >= 500 ? AxiosError.ERR_BAD_RESPONSE : AxiosError.ERR_BAD_REQUEST, + requestConfig, + undefined, + { status, statusText: '', data: 'Bad Gateway', headers: {}, config: requestConfig }, + ); +} + +function unanswered(code: string, requestConfig = config({ operation: 'insert' })) { + return new AxiosError('Network Error', code, requestConfig); +} + +describe('isUnansweredWrite', () => { + it.each([502, 503, 504])('counts a write answered %i', (status) => { + expect(isUnansweredWrite(answered(status))).toBe(true); + }); + + it.each([AxiosError.ERR_NETWORK, AxiosError.ETIMEDOUT, AxiosError.ECONNABORTED])( + 'counts a write that got no response (%s)', + (code) => { + expect(isUnansweredWrite(unanswered(code))).toBe(true); + }, + ); + + it('reads the operation from a body axios already serialized', () => { + expect(isUnansweredWrite(answered(504, config(JSON.stringify({ operation: 'deploy_component' }))))).toBe(true); + expect(isUnansweredWrite(answered(504, config(JSON.stringify({ operation: 'describe_all' }))))).toBe(false); + }); + + it('counts a central-manager write, whose body names no operation', () => { + expect(isUnansweredWrite(answered(504, config({ name: 'my-cluster' })))).toBe(true); + expect(isUnansweredWrite(unanswered(AxiosError.ERR_NETWORK, config(undefined, 'delete')))).toBe(true); + }); + + it.each([400, 401, 403, 404, 409, 422, 429, 500])('leaves a write answered %i to its own words', (status) => { + expect(isUnansweredWrite(answered(status))).toBe(false); + }); + + it.each([AxiosError.ERR_CANCELED, AxiosError.ERR_INVALID_URL, AxiosError.ERR_BAD_OPTION, 'ERR_TARGET_RESTARTING'])( + 'leaves a request that never left the browser (%s) alone', + (code) => { + expect(isUnansweredWrite(unanswered(code))).toBe(false); + }, + ); + + it('leaves reads alone, whatever happened to them', () => { + expect(isUnansweredWrite(answered(504, config({ operation: 'get_job' })))).toBe(false); + expect(isUnansweredWrite(answered(503, config(undefined, 'get')))).toBe(false); + expect(isUnansweredWrite(unanswered(AxiosError.ECONNABORTED, config({ operation: 'package_component' })))).toBe( + false, + ); + }); + + it('needs a real AxiosError with the request it describes', () => { + expect(isUnansweredWrite(new AxiosError('Network Error', AxiosError.ERR_NETWORK))).toBe(false); + expect(isUnansweredWrite({ isAxiosError: false, code: AxiosError.ERR_NETWORK, config: config({}) })).toBe(false); + expect(isUnansweredWrite(new Error('Request failed with status code 504'))).toBe(false); + expect(isUnansweredWrite(undefined)).toBe(false); + expect(isUnansweredWrite('504')).toBe(false); + }); +}); diff --git a/src/integrations/api/unansweredWrite.ts b/src/integrations/api/unansweredWrite.ts new file mode 100644 index 000000000..7ef3aa61b --- /dev/null +++ b/src/integrations/api/unansweredWrite.ts @@ -0,0 +1,46 @@ +import { isReadRequest } from '@/integrations/api/isReadOperation'; +import { pluralize } from '@/lib/pluralize'; +import { AxiosError, isAxiosError } from 'axios'; + +export const OUTCOME_UNKNOWN_TITLE = 'Outcome unknown'; + +// "Couldn't confirm", not "didn't answer": a 503 is an answer, just not a verdict. +export const UNANSWERED_WRITE_MESSAGE = "The server couldn't confirm whether that went through. " + + 'Check whether it took effect before you try again, or it may happen twice.'; + +/** For a flow that sends one write per target and reports a tally: the sentence for the ones with no verdict. */ +export function describeUnansweredWrites(count: number): string { + const they = count === 1 ? 'it' : 'they'; + return `${pluralize(count, 'request', 'requests')} couldn't be confirmed, so ${they} may have gone through. ` + + `Check before you try again, or ${they} may run twice.`; +} + +// Axios reports a request that got no response as ERR_NETWORK, a CORS rejection and a connection +// dropped after the request went out alike, so none of them says the server was never reached. +// ECONNABORTED/ETIMEDOUT are the timeout pair; which one axios uses depends on +// `transitional.clarifyTimeoutError`. +export const NO_RESPONSE_CODES: ReadonlySet = new Set([ + AxiosError.ERR_NETWORK, + AxiosError.ETIMEDOUT, + AxiosError.ECONNABORTED, +]); + +// A proxy answers these for an upstream that may already have the request (central manager's +// operations proxy sends 504 after forwarding it), and Harper answers 503 when the worker it handed an +// operation to exits or overruns. The same reasoning stops the gateway retry from replaying a write. +const NO_VERDICT_STATUSES: ReadonlySet = new Set([502, 503, 504]); + +/** + * A request that may have been applied although it failed: not a read, and answered by a 502/503/504 + * or not at all. Its failure must not read as a plain failure, since a retry can apply it twice. + */ +export function isUnansweredWrite(rawErr: unknown): rawErr is AxiosError { + if (!isAxiosError(rawErr) || !rawErr.config || isReadRequest(rawErr.config)) { + return false; + } + const status = rawErr.response?.status; + if (typeof status === 'number') { + return NO_VERDICT_STATUSES.has(status); + } + return typeof rawErr.code === 'string' && NO_RESPONSE_CODES.has(rawErr.code); +} diff --git a/src/react-query/queryClient.test.ts b/src/react-query/queryClient.test.ts index 26c9af0ce..daf570a03 100644 --- a/src/react-query/queryClient.test.ts +++ b/src/react-query/queryClient.test.ts @@ -1,9 +1,10 @@ +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'; -import { AxiosError } from 'axios'; +import { AxiosError, type InternalAxiosRequestConfig } from 'axios'; import { toast } from 'sonner'; import { afterEach, beforeEach, describe, expect, it, MockInstance, vi } from 'vitest'; -import { errorHandler, queryClient, queryErrorHandler } from './queryClient'; +import { describeError, errorHandler, queryClient, queryErrorHandler, shouldToastOwnFailure } from './queryClient'; import { claimForRoute } from './routeQueryClaims'; // Mock the toast module @@ -368,3 +369,60 @@ describe('queryErrorHandler', () => { expect(toast.error).toHaveBeenCalled(); }); }); + +describe('a write that got no verdict', () => { + const sent = (data: unknown, method = 'post') => ({ method, data }) as InternalAxiosRequestConfig; + const gatewayTimeout = (config: InternalAxiosRequestConfig) => + new AxiosError('Request failed with status code 504', AxiosError.ERR_BAD_RESPONSE, config, undefined, { + status: 504, + statusText: '', + data: { type: 'about:blank', code: 'GATEWAY_TIMEOUT', title: 'Proxied operation timed out after 30s' }, + headers: {}, + config, + }); + const timedOut = (config: InternalAxiosRequestConfig) => + new AxiosError('timeout of 60000ms exceeded', AxiosError.ECONNABORTED, config); + + beforeEach(() => { + vi.clearAllMocks(); + vi.spyOn(console, 'error').mockImplementation(() => undefined); + }); + afterEach(() => { + vi.restoreAllMocks(); + }); + + it('is described by its status, not by whatever the gateway sent', () => { + expect(describeError(gatewayTimeout(sent({ operation: 'insert' })))).toEqual({ + title: OUTCOME_UNKNOWN_TITLE, + description: UNANSWERED_WRITE_MESSAGE, + message: UNANSWERED_WRITE_MESSAGE, + }); + }); + + it('leaves a read with the same failure to the usual extraction', () => { + expect(describeError(gatewayTimeout(sent({ operation: 'get_job' })))).toEqual({ + title: 'GATEWAY_TIMEOUT', + description: 'Proxied operation timed out after 30s', + message: 'Proxied operation timed out after 30s', + }); + }); + + it('gets a toast of its own, never collapsed with another write or with read timeouts', () => { + errorHandler(timedOut(sent({ operation: 'delete', hash_values: [1] }))); + errorHandler(timedOut(sent({ operation: 'insert', records: [{ id: 2 }] }))); + errorHandler(timedOut(sent({ operation: 'describe_all' }))); + + const toasts = vi.mocked(toast.error).mock.calls.map(([title, options]) => [title, options?.id]); + expect(toasts).toEqual([ + [OUTCOME_UNKNOWN_TITLE, undefined], + [OUTCOME_UNKNOWN_TITLE, undefined], + ['Error', 'request-timeout'], + ]); + }); + + it('tells a mutation’s caller not to add a "failed" toast of its own', () => { + expect(shouldToastOwnFailure(timedOut(sent({ operation: 'restart' })))).toBe(false); + expect(shouldToastOwnFailure(timedOut(sent({ operation: 'get_status' })))).toBe(true); + expect(shouldToastOwnFailure(new Error('Secret "x" not found'))).toBe(true); + }); +}); diff --git a/src/react-query/queryClient.ts b/src/react-query/queryClient.ts index 581fa4a4e..c4e02ca87 100644 --- a/src/react-query/queryClient.ts +++ b/src/react-query/queryClient.ts @@ -1,3 +1,4 @@ +import { isUnansweredWrite, OUTCOME_UNKNOWN_TITLE, UNANSWERED_WRITE_MESSAGE } from '@/integrations/api/unansweredWrite'; import { errorText } from '@/lib/errorText'; import { isRestartNoise } from '@/lib/restart/restartGate'; import { onRestartSettled } from '@/lib/restart/restartTracker'; @@ -15,8 +16,14 @@ import { toast } from 'sonner'; * inputs. They differ only for a legacy `"Conflict: user already exists"` body, where the split * below moves the first clause into `title`: a single line built from `description` alone would * lose it. Everything comes from this one extractor so the inline text and the toast can't drift. + * + * 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. */ 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 }; + } let errorTitle = 'Error'; let errorMsg = 'We had some trouble!'; let splitTitleFromMsg = true; @@ -62,6 +69,15 @@ export function describeError(rawErr: unknown): { title: string; description: st return { title: errorTitle, description: errorMsg, message }; } +/** + * 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. + */ +export function shouldToastOwnFailure(rawErr: unknown): boolean { + return !isUnansweredWrite(rawErr); +} + export function errorHandler(rawErr: unknown) { console.error(rawErr); const { title: errorTitle, description: errorMsg } = describeError(rawErr); @@ -75,8 +91,17 @@ export function errorHandler(rawErr: unknown) { // signed out, #1546), and a permission failure repeated N times is still one // thing for the user to know. const isForbidden = axiosWrappedErr?.response?.status === 403; + // Never collapsed: each warns about a different write, and a read timing out alongside one must + // not replace it through `request-timeout`. + const collapseId = isUnansweredWrite(rawErr) + ? undefined + : isTimeout + ? 'request-timeout' + : isForbidden + ? 'request-forbidden' + : undefined; toast.error(errorTitle, { - id: isTimeout ? 'request-timeout' : isForbidden ? 'request-forbidden' : undefined, + id: collapseId, description: errorMsg, action: { label: 'Dismiss', From 59eceefd65c71c1f9fa8674e4100215c922ab5b4 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Sun, 11 Oct 2026 05:18:23 -0400 Subject: [PATCH 02/16] fix(domains): a domain validation with no verdict is not blamed on the TXT record Validating pending domains posts one request per domain and, on any failure, said "Validation failed! Please make sure the TXT record has been put in place." A validation central manager answered 502/503/504, or never answered, may have succeeded, so it is now counted apart: the toast says the outcome is unknown and the list is refetched to show what changed. Also from review: isUnansweredWrite checks the status before it parses a serialized body, so only an ambiguous failure pays for the parse; trims comments that only restated their identifiers; guards a test teardown. Co-Authored-By: Claude Opus 5.5 --- .../cluster/domains/Management.test.tsx | 49 ++++++++++++++++++- src/features/cluster/domains/Management.tsx | 21 ++++++-- .../Chat/tools/describeToolError.ts | 1 - .../api/unansweredWrite.integration.test.tsx | 3 +- src/integrations/api/unansweredWrite.ts | 13 +++-- 5 files changed, 73 insertions(+), 14 deletions(-) diff --git a/src/features/cluster/domains/Management.test.tsx b/src/features/cluster/domains/Management.test.tsx index b2e99421d..2e460097e 100644 --- a/src/features/cluster/domains/Management.test.tsx +++ b/src/features/cluster/domains/Management.test.tsx @@ -3,7 +3,7 @@ */ import { MutationCache, onlineManager, QueryCache, QueryClient, QueryClientProvider } from '@tanstack/react-query'; import { act, fireEvent, render, waitFor, within } from '@testing-library/react'; -import { AxiosError } from 'axios'; +import { AxiosError, type InternalAxiosRequestConfig } from 'axios'; import { PropsWithChildren, Suspense } from 'react'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; @@ -30,6 +30,7 @@ vi.mock('@/hooks/usePermissions', () => ({ const { instancePost } = vi.hoisted(() => ({ instancePost: vi.fn() })); vi.mock('@/config/getInstanceClient', () => ({ getInstanceClient: () => ({ post: instancePost }) })); +import { describeUnansweredWrites, OUTCOME_UNKNOWN_TITLE } from '@/integrations/api/unansweredWrite'; import { mutationErrorHandler, queryErrorHandler } from '@/react-query/queryClient'; import { toast } from 'sonner'; @@ -1121,3 +1122,49 @@ describe('DomainsManagement — the Remove Domain tooltip', () => { expect(pendingCell.querySelectorAll('[tabindex]')).toHaveLength(0); }); }); + +describe('validating pending domains', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('says a validation that got no answer may have gone through, and refreshes the list to show it', async () => { + get.mockImplementation((url: string) => + url.startsWith('/Domain/') + ? Promise.resolve({ + data: [ + { id: 'dom-1', domain: 'a.example.com', status: 'PENDING_VALIDATION' }, + { id: 'dom-2', domain: 'b.example.com', status: 'PENDING_VALIDATION' }, + ], + }) + : Promise.resolve({ data: { id: 'clu-1', organizationId: 'org-1', status: 'RUNNING', domainIds: [] } }) + ); + post.mockImplementation((url: string) => { + const config = { method: 'post', url } as InternalAxiosRequestConfig; + return Promise.reject( + url.includes('dom-1') + ? new AxiosError('timeout of 60000ms exceeded', AxiosError.ECONNABORTED, config) + : new AxiosError('Request failed with status code 400', AxiosError.ERR_BAD_REQUEST, config, undefined, { + status: 400, + statusText: '', + data: 'TXT record not found', + headers: {}, + config, + }), + ); + }); + const { container } = renderManagement(); + const validateButton = () => container.querySelector('button[accesskey="r"]'); + await waitFor(() => expect(validateButton()?.disabled).toBe(false)); + const fetchesBefore = domainListFetches(); + + fireEvent.click(validateButton()!); + + await waitFor(() => expect(toast.error).toHaveBeenCalled()); + expect(toast.error).toHaveBeenCalledWith(OUTCOME_UNKNOWN_TITLE, { + description: `${describeUnansweredWrites(1)} 1 other domain failed to validate.`, + id: 'validatingDomains', + }); + expect(domainListFetches()).toBeGreaterThan(fetchesBefore); + }); +}); diff --git a/src/features/cluster/domains/Management.tsx b/src/features/cluster/domains/Management.tsx index 0fc8204d6..d8a71da46 100644 --- a/src/features/cluster/domains/Management.tsx +++ b/src/features/cluster/domains/Management.tsx @@ -40,6 +40,7 @@ import { getOrganizationDomainsQueryOptions } from '@/features/organization/quer import { useOrganizationRolePermissions } from '@/hooks/usePermissions'; import { useRefreshClick } from '@/hooks/useRefreshClick'; import { SchemaOrganizationDomain } from '@/integrations/api/api.patch'; +import { describeUnansweredWrites, isUnansweredWrite, OUTCOME_UNKNOWN_TITLE } from '@/integrations/api/unansweredWrite'; import { pluralize } from '@/lib/pluralize'; import { describeError } from '@/react-query/queryClient'; import { zodResolver } from '@hookform/resolvers/zod'; @@ -235,6 +236,7 @@ export function DomainsManagement() { const id = 'validatingDomains'; let checked = 0; let failed = 0; + let unanswered = 0; for (const pendingDomain of pendingDomains) { try { toast.loading(message, { @@ -242,11 +244,24 @@ export function DomainsManagement() { id, }); await validateDomainInOrganization(pendingDomain.id); - } catch { - failed += 1; + } catch (error) { + if (isUnansweredWrite(error)) { + unanswered += 1; + } else { + failed += 1; + } } } - if (failed > 0) { + if (unanswered > 0) { + await refetch(); + toast.error(OUTCOME_UNKNOWN_TITLE, { + description: [ + describeUnansweredWrites(unanswered), + failed > 0 && `${pluralize(failed, 'other domain', 'other domains')} failed to validate.`, + ].filter(Boolean).join(' '), + id, + }); + } else if (failed > 0) { toast.error('Validation failed!', { description: `Please make sure the TXT record has been put in place. You may need to wait a bit for the DNS change to propagate.`, diff --git a/src/features/instance/applications/components/Chat/tools/describeToolError.ts b/src/features/instance/applications/components/Chat/tools/describeToolError.ts index 97f985231..fe954ef0a 100644 --- a/src/features/instance/applications/components/Chat/tools/describeToolError.ts +++ b/src/features/instance/applications/components/Chat/tools/describeToolError.ts @@ -1,6 +1,5 @@ import { isUnansweredWrite, UNANSWERED_WRITE_MESSAGE } from '@/integrations/api/unansweredWrite'; -/** A failed tool call, as the agent reads it: told plainly when a retry could apply a write twice. */ export function describeToolError(err: unknown): string { return isUnansweredWrite(err) ? UNANSWERED_WRITE_MESSAGE : String(err); } diff --git a/src/integrations/api/unansweredWrite.integration.test.tsx b/src/integrations/api/unansweredWrite.integration.test.tsx index 000ad0c35..27042f04e 100644 --- a/src/integrations/api/unansweredWrite.integration.test.tsx +++ b/src/integrations/api/unansweredWrite.integration.test.tsx @@ -20,7 +20,6 @@ vi.mock('sonner', () => ({ const GATEWAY_BODY = '504 Gateway Time-out'; -/** A real instance client whose transport fails every request the way `fail` says. */ function instanceClient(fail: (config: InternalAxiosRequestConfig) => never) { const client = getInstanceClient({ id: 'ins-123', operationsUrl: 'https://ins-123.example.com:9925/' }); client.defaults.adapter = async (config: InternalAxiosRequestConfig) => fail(config); @@ -56,7 +55,7 @@ describe('a write that got no verdict, through a real instance client', () => { }); afterEach(() => { - consoleMock.mockRestore(); + consoleMock?.mockRestore(); }); it.each([ diff --git a/src/integrations/api/unansweredWrite.ts b/src/integrations/api/unansweredWrite.ts index 7ef3aa61b..ff4fc48bf 100644 --- a/src/integrations/api/unansweredWrite.ts +++ b/src/integrations/api/unansweredWrite.ts @@ -4,11 +4,9 @@ import { AxiosError, isAxiosError } from 'axios'; export const OUTCOME_UNKNOWN_TITLE = 'Outcome unknown'; -// "Couldn't confirm", not "didn't answer": a 503 is an answer, just not a verdict. export const UNANSWERED_WRITE_MESSAGE = "The server couldn't confirm whether that went through. " + 'Check whether it took effect before you try again, or it may happen twice.'; -/** For a flow that sends one write per target and reports a tally: the sentence for the ones with no verdict. */ export function describeUnansweredWrites(count: number): string { const they = count === 1 ? 'it' : 'they'; return `${pluralize(count, 'request', 'requests')} couldn't be confirmed, so ${they} may have gone through. ` @@ -35,12 +33,13 @@ const NO_VERDICT_STATUSES: ReadonlySet = new Set([502, 503, 504]); * or not at all. Its failure must not read as a plain failure, since a retry can apply it twice. */ export function isUnansweredWrite(rawErr: unknown): rawErr is AxiosError { - if (!isAxiosError(rawErr) || !rawErr.config || isReadRequest(rawErr.config)) { + if (!isAxiosError(rawErr) || !rawErr.config) { return false; } const status = rawErr.response?.status; - if (typeof status === 'number') { - return NO_VERDICT_STATUSES.has(status); - } - return typeof rawErr.code === 'string' && NO_RESPONSE_CODES.has(rawErr.code); + const noVerdict = typeof status === 'number' + ? NO_VERDICT_STATUSES.has(status) + : typeof rawErr.code === 'string' && NO_RESPONSE_CODES.has(rawErr.code); + // Last: it may parse a serialized body, and only these failures need it. + return noVerdict && !isReadRequest(rawErr.config); } From b3dee652938ff258b9f177f841628d870aef4b1c Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Sun, 11 Oct 2026 23:30:53 -0400 Subject: [PATCH 03/16] fix(restart): keep a restart with no verdict marked until its target answers again A restart answered 502/503/504, or not at all, may have run: central manager's proxy answers 504 after forwarding it, and Harper can go down mid-reply. The Restart button still released its restart mark at once on that failure, so the tracker's refetches hit an instance going down and their connectivity errors toasted beside "Outcome unknown". The cluster loop also released the member and restarted the next one while it might still be down, which defeats the one-at-a-time restart. Both now hold the mark until waitUntilAnsweringAfterRestart sees the target again, as the operations console already does: the button in the background, after showing the warning; the loop before it moves on. From the adjudicated cross-model review. Co-Authored-By: Claude Opus 5.5 --- DESIGN.md | 1 + src/hooks/useRestartClusterClick.test.tsx | 79 +++++++++++++++++++++- src/hooks/useRestartClusterClick.tsx | 12 +++- src/hooks/useRestartInstanceClick.test.tsx | 33 ++++++++- src/hooks/useRestartInstanceClick.ts | 15 +++- 5 files changed, 132 insertions(+), 8 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index 4d29df3bf..1314a417d 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -72,6 +72,7 @@ The same rule decides what a failure says. [`isUnansweredWrite`](src/integration - **A caller that adds its own failure toast to a mutation** asks [`shouldToastOwnFailure`](src/react-query/queryClient.ts) first, so "Failed to …" never sits beside the global warning. A caller with no mutation behind it (rename, the chat tools) shows `describeError`'s text itself, and a loop that tallies one write per target (cluster restart, rolling config, licenses) counts these apart from failures. - **Do not re-wrap an AxiosError into a plain `Error`** for a write: the predicate needs its `config`. `secrets.ts` passes an unanswered one through as is. +- **A restart with no verdict stays marked until its target answers again**, as an accepted one does, because a gateway can lose the answer to a restart that ran. The Restart button holds the mark while it watches, and the cluster loop waits on that member before restarting the next. - **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`). diff --git a/src/hooks/useRestartClusterClick.test.tsx b/src/hooks/useRestartClusterClick.test.tsx index 0d0000e7a..7f076515c 100644 --- a/src/hooks/useRestartClusterClick.test.tsx +++ b/src/hooks/useRestartClusterClick.test.tsx @@ -3,7 +3,7 @@ */ import type { Cluster, Instance } from '@/integrations/api/api.patch'; import { describeUnansweredWrites, OUTCOME_UNKNOWN_TITLE } from '@/integrations/api/unansweredWrite'; -import { getRestartState } from '@/lib/restart/restartTracker'; +import { getRestartState, resetRestartTracker } from '@/lib/restart/restartTracker'; import { act, renderHook } from '@testing-library/react'; import { AxiosError, type AxiosInstance, type InternalAxiosRequestConfig } from 'axios'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; @@ -19,7 +19,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('@/lib/sleep', () => ({ sleep: vi.fn(() => Promise.resolve()) })); vi.mock('sonner', () => ({ toast: { loading: vi.fn(() => 'toast-1'), error: vi.fn(), success: vi.fn(), dismiss: vi.fn() }, @@ -28,7 +31,7 @@ vi.mock('sonner', () => ({ import { getInstanceClient } from '@/config/getInstanceClient'; import { getClusterInfo } from '@/features/cluster/queries/getClusterInfoQuery'; import { getInstanceUserInfo } from '@/integrations/api/instance/status/getInstanceUserInfo'; -import { restartInstance } from '@/integrations/api/instance/status/restartInstance'; +import { restartInstance, waitUntilAnsweringAfterRestart } from '@/integrations/api/instance/status/restartInstance'; import { toast } from 'sonner'; import { useRestartClusterClick } from './useRestartClusterClick'; @@ -59,7 +62,9 @@ async function clickRestart() { beforeEach(() => { restarted = []; + resetRestartTracker(); vi.mocked(getInstanceUserInfo).mockResolvedValue({} as never); + vi.mocked(waitUntilAnsweringAfterRestart).mockResolvedValue(undefined); vi.mocked(restartInstance).mockImplementation(async ({ instanceClient }) => { restarted.push(targetOf(instanceClient)); return { message: 'ok' }; @@ -197,3 +202,71 @@ function cancel() { const [, options] = vi.mocked(toast.loading).mock.calls.at(-1)!; (options as unknown as { action: { onClick: () => void } }).action.onClick(); } + +function answeredWith(status: number) { + const config = { method: 'post', data: { operation: 'restart' } } as never; + const code = status >= 500 ? AxiosError.ERR_BAD_RESPONSE : AxiosError.ERR_BAD_REQUEST; + return new AxiosError(`Request failed with status code ${status}`, code, config, undefined, { + status, + statusText: '', + data: status === 504 ? 'Proxied operation timed out after 30s on instance ins-b' : { error: 'Not allowed' }, + headers: {}, + config, + }); +} + +function restartsEndingIn(outcomes: Record) { + vi.mocked(restartInstance).mockImplementation(async ({ instanceClient }) => { + const target = targetOf(instanceClient); + restarted.push(target); + if (outcomes[target]) { + throw outcomes[target]; + } + return { message: 'Restarting Harper. This may take up to 60 seconds.' }; + }); +} + +describe('useRestartClusterClick, for a member whose restart got no answer', () => { + beforeEach(() => { + vi.mocked(getClusterInfo).mockResolvedValue( + { id: 'clu-1', instances: [instance('ins-a'), instance('ins-b'), instance('ins-c')] } as Cluster, + ); + }); + + 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()); + + let click: Promise | undefined; + act(() => { + click = result.current.onRestartClick() as unknown as Promise; + }); + await vi.waitFor(() => expect(waitUntilAnsweringAfterRestart).toHaveBeenCalledTimes(1)); + + expect(targetOf(vi.mocked(waitUntilAnsweringAfterRestart).mock.calls[0][0].instanceClient)).toBe('ins-b'); + 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(getRestartState('ins-b')).toBeUndefined(); + expect(vi.mocked(toast.error).mock.calls.at(-1)?.[0]).toBe(OUTCOME_UNKNOWN_TITLE); + }); + + it('does not wait on a member that refused the restart', async () => { + restartsEndingIn({ 'ins-b': answeredWith(403) }); + + await clickRestart(); + + expect(restarted).toEqual(['ins-c', 'ins-b', 'ins-a']); + expect(waitUntilAnsweringAfterRestart).not.toHaveBeenCalled(); + expect(vi.mocked(toast.error).mock.calls.at(-1)?.[0]).toBe('Error'); + }); +}); diff --git a/src/hooks/useRestartClusterClick.tsx b/src/hooks/useRestartClusterClick.tsx index dc4b12471..ddeeaf09d 100644 --- a/src/hooks/useRestartClusterClick.tsx +++ b/src/hooks/useRestartClusterClick.tsx @@ -1,7 +1,7 @@ import { ProgressBar } from '@/components/ProgressBar'; import { ClusterRestartTargets, getClusterRestartTargets } from '@/hooks/getClusterRestartTargets'; 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 { describeUnansweredWrites, isUnansweredWrite, OUTCOME_UNKNOWN_TITLE } from '@/integrations/api/unansweredWrite'; import { excludeFalsy } from '@/lib/arrays/excludeFalsy'; import { pluralize } from '@/lib/pluralize'; @@ -121,6 +121,16 @@ export function useRestartClusterClick( } catch (error) { const unanswered = isUnansweredWrite(error); instancesUnanswered += unanswered ? 1 : 0; + 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.', + }); + await waitUntilAnsweringAfterRestart({ instanceClient }).catch(() => undefined); + } 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'; diff --git a/src/hooks/useRestartInstanceClick.test.tsx b/src/hooks/useRestartInstanceClick.test.tsx index 9845146b5..a9a0db51f 100644 --- a/src/hooks/useRestartInstanceClick.test.tsx +++ b/src/hooks/useRestartInstanceClick.test.tsx @@ -1,7 +1,8 @@ /** @vitest-environment jsdom */ import { useRestartInstanceClick } from '@/hooks/useRestartInstanceClick'; +import { waitUntilAnsweringAfterRestart } from '@/integrations/api/instance/status/restartInstance'; import { OUTCOME_UNKNOWN_TITLE } from '@/integrations/api/unansweredWrite'; -import { resetRestartTracker } from '@/lib/restart/restartTracker'; +import { isRestarting, resetRestartTracker } from '@/lib/restart/restartTracker'; import { mutationErrorHandler } from '@/react-query/queryClient'; import { MutationCache, QueryClient, QueryClientProvider } from '@tanstack/react-query'; import { act, renderHook } from '@testing-library/react'; @@ -14,6 +15,11 @@ vi.mock('@tanstack/react-router', () => ({ useParams: () => ({ clusterId: 'clu-1', instanceId: 'ins-1' }), })); +vi.mock(import('@/integrations/api/instance/status/restartInstance'), async (importOriginal) => ({ + ...await importOriginal(), + waitUntilAnsweringAfterRestart: vi.fn(), +})); + vi.mock('sonner', () => ({ toast: { loading: vi.fn(() => 'restart-toast'), error: vi.fn(), success: vi.fn(), dismiss: vi.fn() }, })); @@ -39,6 +45,7 @@ describe('useRestartInstanceClick', () => { beforeEach(() => { vi.clearAllMocks(); resetRestartTracker(); + vi.mocked(waitUntilAnsweringAfterRestart).mockResolvedValue(undefined); vi.spyOn(console, 'error').mockImplementation(() => undefined); }); afterEach(() => { @@ -56,6 +63,28 @@ describe('useRestartInstanceClick', () => { expect(toast.dismiss).toHaveBeenCalledWith('restart-toast'); }); + it('keeps a restart that got no answer marked until its target answers again', async () => { + let answered = () => {}; + vi.mocked(waitUntilAnsweringAfterRestart).mockReturnValueOnce(new Promise((resolve) => (answered = resolve))); + const result = render( + restartAnswering((config) => + new AxiosError('Request failed with status code 504', AxiosError.ERR_BAD_RESPONSE, config, undefined, { + status: 504, + statusText: '', + data: 'Proxied operation timed out after 30s on instance ins-1', + headers: {}, + config, + }) + ), + ); + + await act(() => result.current.onRestartClick()); + + expect(isRestarting('ins-1')).toBe(true); + await act(async () => answered()); + expect(isRestarting('ins-1')).toBe(false); + }); + it('still says a refused restart failed', async () => { const result = render( restartAnswering((config) => @@ -73,5 +102,7 @@ describe('useRestartInstanceClick', () => { expect(errorTitles()).toContainEqual(['Error', 'Failed to restart instance.']); expect(errorTitles().map(([title]) => title)).not.toContain(OUTCOME_UNKNOWN_TITLE); + expect(isRestarting('ins-1')).toBe(false); + expect(waitUntilAnsweringAfterRestart).not.toHaveBeenCalled(); }); }); diff --git a/src/hooks/useRestartInstanceClick.ts b/src/hooks/useRestartInstanceClick.ts index 6b50bd207..1430005f2 100644 --- a/src/hooks/useRestartInstanceClick.ts +++ b/src/hooks/useRestartInstanceClick.ts @@ -1,7 +1,8 @@ import { isLocalStudio } from '@/config/constants'; import { InstanceClientConfig } from '@/config/instanceClientConfig'; import { OverallAppSignIn } from '@/features/auth/store/authStore'; -import { useRestartInstance } from '@/integrations/api/instance/status/restartInstance'; +import { 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'; import { shouldToastOwnFailure } from '@/react-query/queryClient'; @@ -48,6 +49,7 @@ export function useRestartInstanceClick({ // dropped if the component unmounts before the restart finishes (e.g. the user navigates to // another tab while the instance comes back up), which would orphan the loading toast forever // since sonner never auto-dismisses a loading toast. The awaited continuation runs regardless. + let holdUntilAnswering = false; try { await restartInstance({ operation, @@ -72,6 +74,9 @@ export function useRestartInstanceClick({ onRestartedSuccessfully?.(); } catch (error) { toast.dismiss(toastId); + // A gateway can lose the answer to a restart that ran and took the target down mid-reply, so the + // mark holds until the target answers again, as it would after an accepted restart. + holdUntilAnswering = isUnansweredWrite(error); if (shouldToastOwnFailure(error)) { toast.error('Error', { description: `Failed to restart ${targetNoun.toLowerCase()}.`, @@ -82,8 +87,12 @@ export function useRestartInstanceClick({ }); } } finally { - // Released on failure too: once the restart has failed, the errors that follow are real. - releaseTarget?.(); + 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. + releaseTarget?.(); + } } }, [ clusterId, From e4df6a3f55d8a6b7f22794047a69ab56d9bd10b3 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Sun, 11 Oct 2026 23:31:32 -0400 Subject: [PATCH 04/16] fix(errors): count refused licenses beside unanswered ones, and refresh after an unanswered deploy The license loop's "Outcome unknown" toast dropped the licenses that were refused outright, so a run with one of each left the refused one unaccounted for. It now adds "N other licenses failed to apply", as the domain validation toast does. A redeploy or import that got no answer is told to check whether it took effect, but nothing refreshed what the user would check. The redeploy modal now refetches the components list on that failure, live progress or not, and the import refetches the instance and reloads the file tree. Also trims the isUnansweredWrite JSDoc to the part the code doesn't say. From the adjudicated cross-model review. Co-Authored-By: Claude Opus 5.5 --- .../useImportApplication.test.ts | 12 ++++++++ .../NewApplication/useImportApplication.ts | 20 +++++++++++-- .../modals/RedeployApplicationModal.test.tsx | 29 ++++++++++++++++++- .../modals/RedeployApplicationModal.tsx | 9 ++++-- src/hooks/useApplyLicensesClick.test.tsx | 20 +++++++++++-- src/hooks/useApplyLicensesClick.tsx | 10 +++++-- src/integrations/api/unansweredWrite.ts | 5 +--- 7 files changed, 92 insertions(+), 13 deletions(-) diff --git a/src/features/instance/applications/components/NewApplication/useImportApplication.test.ts b/src/features/instance/applications/components/NewApplication/useImportApplication.test.ts index 521ec726e..9ce0e9a2f 100644 --- a/src/features/instance/applications/components/NewApplication/useImportApplication.test.ts +++ b/src/features/instance/applications/components/NewApplication/useImportApplication.test.ts @@ -278,6 +278,18 @@ describe('useImportApplication', () => { await waitFor(() => expect(result.current.importFailure).toEqual({ title, message: UNANSWERED_WRITE_MESSAGE })); expect(vi.mocked(toast.error)).not.toHaveBeenCalled(); + // It may have imported, so the tree the message says to check is reloaded. + expect(mocks.editorView.reloadRootEntries).toHaveBeenCalledOnce(); + }); + + it('reloads nothing after an import Harper refused', async () => { + mocks.post.mockRejectedValue(serverFailure({ error: 'Component my-app could not be loaded' })); + const { result } = renderImport(); + + act(() => result.current.importApplication({ contents, project: 'my-app' })); + + await waitFor(() => expect(result.current.importFailure).toBeDefined()); + expect(mocks.editorView.reloadRootEntries).not.toHaveBeenCalled(); }); describe('a rollout that settles after the form has gone', () => { diff --git a/src/features/instance/applications/components/NewApplication/useImportApplication.ts b/src/features/instance/applications/components/NewApplication/useImportApplication.ts index a02e5a833..9c6ca2d91 100644 --- a/src/features/instance/applications/components/NewApplication/useImportApplication.ts +++ b/src/features/instance/applications/components/NewApplication/useImportApplication.ts @@ -151,9 +151,15 @@ export function useImportApplication( return; } // No verdict: it may have gone through, so the title can't say it failed; the message says to check. - const title = isUnansweredWrite(error) + const unanswered = isUnansweredWrite(error); + const title = unanswered ? `Couldn't confirm ${project} was ${stage ? 'staged' : 'imported'}` : `Failed to ${stage ? 'stage' : 'import'} ${project}`; + if (unanswered) { + // It may have imported, so what the message says to check must be current. + void invalidateEntityQueries(queryClient, entityId); + void reloadRootEntries(); + } const { message } = describeError(error); if (formEntity.current === entityId) { setImportFailure({ entityId, failure: { title, message, deploymentId } }); @@ -178,7 +184,17 @@ export function useImportApplication( duration: 5_000, }); } - }, [mutateAsync, sseDeploy, canStage, instanceParams, runHealthCheck, onImported, recordAction]); + }, [ + mutateAsync, + sseDeploy, + canStage, + instanceParams, + runHealthCheck, + onImported, + recordAction, + queryClient, + reloadRootEntries, + ]); const importApplication = useCallback((args: ImportArgs) => void runImport(args), [runImport]); diff --git a/src/features/instance/applications/modals/RedeployApplicationModal.test.tsx b/src/features/instance/applications/modals/RedeployApplicationModal.test.tsx index abea8df3f..f071e1cf7 100644 --- a/src/features/instance/applications/modals/RedeployApplicationModal.test.tsx +++ b/src/features/instance/applications/modals/RedeployApplicationModal.test.tsx @@ -202,8 +202,13 @@ describe('RedeployApplicationModal', () => { data: JSON.stringify({ operation: 'deploy_component' }), } as InternalAxiosRequestConfig, ); + const COMPONENTS = { queryKey: ['ins-1', 'get_components'], refetchType: 'active' }; + let invalidate: ReturnType; + beforeEach(() => { + invalidate = vi.spyOn(QueryClient.prototype, 'invalidateQueries'); + }); - it("replaces a buffered redeploy's progress toast without calling it failed", async () => { + it("replaces a buffered redeploy's progress toast without calling it failed, and refreshes the list", async () => { mocks.sse.enabled = false; mocks.post.mockRejectedValue(unanswered()); await renderOpenModal(); @@ -215,6 +220,28 @@ describe('RedeployApplicationModal', () => { id: 'loading-toast', description: UNANSWERED_WRITE_MESSAGE, }); + expect(invalidate).toHaveBeenCalledWith(COMPONENTS); + }); + + it('says so in the open progress panel, and refreshes the list', async () => { + vi.mocked(deployComponentStream).mockRejectedValue(unanswered()); + await renderOpenModal(); + + await submit(); + + expect((await screen.findByRole('alert')).textContent).toContain(UNANSWERED_WRITE_MESSAGE); + expect(vi.mocked(toast.error)).not.toHaveBeenCalled(); + expect(invalidate).toHaveBeenCalledWith(COMPONENTS); + }); + + it('refreshes nothing after a redeploy Harper refused', async () => { + vi.mocked(deployComponentStream).mockRejectedValue(new SSEOperationError(FAILURE, { phase: 'load' })); + await renderOpenModal(); + + await submit(); + + expect((await screen.findByRole('alert')).textContent).toContain(FAILURE); + expect(invalidate).not.toHaveBeenCalledWith(COMPONENTS); }); it('toasts a streamed one that lands after the modal was closed without calling it failed', async () => { diff --git a/src/features/instance/applications/modals/RedeployApplicationModal.tsx b/src/features/instance/applications/modals/RedeployApplicationModal.tsx index 3c4ec50a0..71bd9bd17 100644 --- a/src/features/instance/applications/modals/RedeployApplicationModal.tsx +++ b/src/features/instance/applications/modals/RedeployApplicationModal.tsx @@ -135,8 +135,9 @@ export function RedeployApplicationModal() { return; } const rolloutFailed = error instanceof DeployRolloutError; - if (rolloutFailed) { - // Live on the node that received it. + // Live on the node that received it, or, with no verdict, it may have deployed: either way the + // list the message says to check must be current. + if (rolloutFailed || isUnansweredWrite(error)) { invalidateComponents(); } const { message } = describeError(error); @@ -205,6 +206,10 @@ export function RedeployApplicationModal() { invalidateComponents(); return; } + if (isUnansweredWrite(error)) { + // It may have deployed, so the list the message says to check must be current. + invalidateComponents(); + } toast.error(redeployFailureTitle(error, project, stage), { id: toastId, description: describeError(error).message, diff --git a/src/hooks/useApplyLicensesClick.test.tsx b/src/hooks/useApplyLicensesClick.test.tsx index 7c1c401e2..5dd6c7411 100644 --- a/src/hooks/useApplyLicensesClick.test.tsx +++ b/src/hooks/useApplyLicensesClick.test.tsx @@ -38,8 +38,8 @@ function wrapper() { ; } -async function applyLicenses() { - const { result } = renderHook(() => useApplyLicensesClick({ licenses: LICENSES }), { wrapper: wrapper() }); +async function applyLicenses(licenses = LICENSES) { + const { result } = renderHook(() => useApplyLicensesClick({ licenses }), { wrapper: wrapper() }); await act(async () => { await (result.current.onApplyLicensesClick() as unknown as Promise); }); @@ -89,6 +89,22 @@ describe('useApplyLicensesClick', () => { ); }); + it('still counts a refused license when another got no answer', async () => { + vi.mocked(installUsageLicense) + .mockResolvedValueOnce({} as never) + .mockRejectedValueOnce(new AxiosError('timeout of 60000ms exceeded', AxiosError.ECONNABORTED, installSent)) + .mockRejectedValueOnce(new Error('License is expired')); + + await applyLicenses([...LICENSES, { license: 'lic-3' }] as SchemaLicense[]); + + const [title, options] = vi.mocked(toast.error).mock.calls.at(-1)!; + expect(title).toBe(OUTCOME_UNKNOWN_TITLE); + expect(options?.description).toBe( + "1 of 3 licenses confirmed applied. 1 request couldn't be confirmed, so it may have gone through. " + + 'Check before you try again, or it may run twice. 1 other license failed to apply.', + ); + }); + it('still says a refused license was not applied', async () => { vi.mocked(installUsageLicense) .mockResolvedValueOnce({} as never) diff --git a/src/hooks/useApplyLicensesClick.tsx b/src/hooks/useApplyLicensesClick.tsx index 19c8ee0aa..8ea696df4 100644 --- a/src/hooks/useApplyLicensesClick.tsx +++ b/src/hooks/useApplyLicensesClick.tsx @@ -5,6 +5,7 @@ import { installUsageLicense } from '@/integrations/api/instance/auth/installUsa import { getInstanceUserInfo } from '@/integrations/api/instance/status/getInstanceUserInfo'; import { describeUnansweredWrites, isUnansweredWrite, OUTCOME_UNKNOWN_TITLE } from '@/integrations/api/unansweredWrite'; import { excludeFalsy } from '@/lib/arrays/excludeFalsy'; +import { pluralize } from '@/lib/pluralize'; import { sleep } from '@/lib/sleep'; import { useQueryClient } from '@tanstack/react-query'; import { useCallback, useState } from 'react'; @@ -52,6 +53,7 @@ export function useApplyLicensesClick({ licenses }: ApplyLicensesClickParams): A let licensesApplied = 0; let licensesUnanswered = 0; + let licensesFailed = 0; for (let i = 0; i < licenses.length; i++) { const license = licenses[i]; @@ -88,6 +90,7 @@ export function useApplyLicensesClick({ licenses }: ApplyLicensesClickParams): A } catch (error) { const unanswered = isUnansweredWrite(error); licensesUnanswered += unanswered ? 1 : 0; + licensesFailed += unanswered ? 0 : 1; if (i + 1 !== licenses.length) { // If it fails to applyLicenses, or wasn't available, warn for a bit then move on. const verdict = unanswered ? 'Could Not Confirm' : 'Failed Applying'; @@ -135,8 +138,11 @@ export function useApplyLicensesClick({ licenses }: ApplyLicensesClickParams): A toast.error(licensesUnanswered ? OUTCOME_UNKNOWN_TITLE : 'Error', { id: toastId, description: licensesUnanswered - ? `${licensesApplied} of ${licenses.length} ${licenseWord.toLowerCase()} confirmed applied. ` - + describeUnansweredWrites(licensesUnanswered) + ? [ + `${licensesApplied} of ${licenses.length} ${licenseWord.toLowerCase()} confirmed applied.`, + describeUnansweredWrites(licensesUnanswered), + licensesFailed > 0 && `${pluralize(licensesFailed, 'other license', 'other licenses')} failed to apply.`, + ].filter(excludeFalsy).join(' ') : `${licenseWord} not applied.\n` + ([ licensesApplied > 0 && licenses.length !== licensesApplied diff --git a/src/integrations/api/unansweredWrite.ts b/src/integrations/api/unansweredWrite.ts index ff4fc48bf..d117b5eae 100644 --- a/src/integrations/api/unansweredWrite.ts +++ b/src/integrations/api/unansweredWrite.ts @@ -28,10 +28,7 @@ export const NO_RESPONSE_CODES: ReadonlySet = new Set([ // operation to exits or overruns. The same reasoning stops the gateway retry from replaying a write. const NO_VERDICT_STATUSES: ReadonlySet = new Set([502, 503, 504]); -/** - * A request that may have been applied although it failed: not a read, and answered by a 502/503/504 - * or not at all. Its failure must not read as a plain failure, since a retry can apply it twice. - */ +/** Its failure must not read as a plain failure: it may have been applied, so a retry can apply it twice. */ export function isUnansweredWrite(rawErr: unknown): rawErr is AxiosError { if (!isAxiosError(rawErr) || !rawErr.config) { return false; From 2aa5d2610f6d2175cada992316b78fa882b236e2 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Sun, 11 Oct 2026 23:42:36 -0400 Subject: [PATCH 05/16] docs(design): say a no-verdict restart's mark also ends when the watch gives up The note read as though the mark held indefinitely and the cluster loop never moved past a member that never answers. Both release when waitUntilAnsweringAfterRestart gives up, as they do after an accepted restart. From the adjudicated cross-model review. 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 1314a417d..9901d2c32 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -72,7 +72,7 @@ The same rule decides what a failure says. [`isUnansweredWrite`](src/integration - **A caller that adds its own failure toast to a mutation** asks [`shouldToastOwnFailure`](src/react-query/queryClient.ts) first, so "Failed to …" never sits beside the global warning. A caller with no mutation behind it (rename, the chat tools) shows `describeError`'s text itself, and a loop that tallies one write per target (cluster restart, rolling config, licenses) counts these apart from failures. - **Do not re-wrap an AxiosError into a plain `Error`** for a write: the predicate needs its `config`. `secrets.ts` passes an unanswered one through as is. -- **A restart with no verdict stays marked until its target answers again**, as an accepted one does, because a gateway can lose the answer to a restart that ran. The Restart button holds the mark while it watches, and the cluster loop waits on that member before restarting the next. +- **A restart with no verdict stays marked until its target answers again or the watch gives up**, as an accepted one does, because a gateway can lose the answer to a restart that ran. The Restart button holds the mark while it watches, and the cluster loop waits on that member before restarting the next. - **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`). From 03165c9c1751cd06c20d24ef9b63418afe818ac1 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Mon, 12 Oct 2026 21:23:36 -0400 Subject: [PATCH 06/16] fix(deploy): never call a staged-release activation with no verdict failed The rebase onto stage brought in a write surface this PR's caller audit predates: activating a staged release (#1876's chain). Its mutation opts out of the global toast, like import and redeploy since #1871, so an activation that timed out or got a 504 showed "Failed to activate " with axios's own text and no warning that it may have gone through. deployFailureTitle (new, beside the deploy reporters) is the one place a deploy's failure title is decided: "Couldn't confirm was activated" for a write with no verdict, "Failed to activate " otherwise. The import form and the redeploy modal, which the rebase had given their own copies of that rule, now use it too. reportDeployFailure shows the unknown-outcome copy, and the activation's progress panel says so as well. Co-Authored-By: Claude Opus 5.5 --- DESIGN.md | 2 +- .../NewApplication/useImportApplication.ts | 14 +++++------ .../modals/RedeployApplicationModal.tsx | 20 +++++++-------- .../modals/reportDeployHealth.test.ts | 14 +++++++++++ .../applications/modals/reportDeployHealth.ts | 25 ++++++++++++++++--- .../components/ActivateStagedRelease.test.tsx | 14 ++++++++++- .../components/ActivateStagedRelease.tsx | 9 ++++++- 7 files changed, 74 insertions(+), 24 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index 9901d2c32..4ecf3a582 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -70,7 +70,7 @@ Every `getInstanceClient` client replays a 502/503/504 up to three times (5s/10s The same rule decides what a failure says. [`isUnansweredWrite`](src/integrations/api/unansweredWrite.ts) is a request `isReadRequest` rejects, answered 502/503/504 or not at all (`ERR_NETWORK`, a timeout). [`describeError`](src/react-query/queryClient.ts) checks it before reading any body and returns "Outcome unknown … check whether it took effect before you try again", so the global mutation toast and every `describeError` caller say so with no copy of their own. Gate on status, never body (a gateway's body describes the gateway), and keep it to display: rewriting `error.message` would defeat `shouldKeepEvent`, which recognises timeouts and `Network Error` by message. -- **A caller that adds its own failure toast to a mutation** asks [`shouldToastOwnFailure`](src/react-query/queryClient.ts) first, so "Failed to …" never sits beside the global warning. A caller with no mutation behind it (rename, the chat tools) shows `describeError`'s text itself, and a loop that tallies one write per target (cluster restart, rolling config, licenses) counts these apart from failures. +- **A caller that adds its own failure toast to a mutation** asks [`shouldToastOwnFailure`](src/react-query/queryClient.ts) first, so "Failed to …" never sits beside the global warning. A caller with no mutation behind it (rename, the chat tools), or one whose mutation opts out of the global toast (the deploy mutation behind import, redeploy and staged-release activation), shows `describeError`'s text itself, under a title from [`deployFailureTitle`](src/features/instance/applications/modals/reportDeployHealth.ts) for a deploy ("Couldn't confirm … was redeployed", never "Failed to …"), and a loop that tallies one write per target (cluster restart, rolling config, licenses) counts these apart from failures. - **Do not re-wrap an AxiosError into a plain `Error`** for a write: the predicate needs its `config`. `secrets.ts` passes an unanswered one through as is. - **A restart with no verdict stays marked until its target answers again or the watch gives up**, as an accepted one does, because a gateway can lose the answer to a restart that ran. The Restart button holds the mark while it watches, and the cluster loop waits on that member before restarting the next. - **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`. diff --git a/src/features/instance/applications/components/NewApplication/useImportApplication.ts b/src/features/instance/applications/components/NewApplication/useImportApplication.ts index 9c6ca2d91..876b7c191 100644 --- a/src/features/instance/applications/components/NewApplication/useImportApplication.ts +++ b/src/features/instance/applications/components/NewApplication/useImportApplication.ts @@ -6,7 +6,11 @@ import { useStagedDeploymentsAvailable, useSupportsDeploymentSSE, } from '@/features/instance/applications/hooks/useSupportsDeploymentSSE'; -import { reportDeployOutcome, reportStaged } from '@/features/instance/applications/modals/reportDeployHealth'; +import { + deployFailureTitle, + reportDeployOutcome, + reportStaged, +} from '@/features/instance/applications/modals/reportDeployHealth'; import { useDeployComponentMutation } from '@/integrations/api/instance/applications/deployComponent'; import { DeployComponentResult } from '@/integrations/api/instance/applications/deployComponentStream'; import { deployFailureDetails } from '@/integrations/api/instance/applications/deployOutcome'; @@ -150,12 +154,8 @@ export function useImportApplication( }); return; } - // No verdict: it may have gone through, so the title can't say it failed; the message says to check. - const unanswered = isUnansweredWrite(error); - const title = unanswered - ? `Couldn't confirm ${project} was ${stage ? 'staged' : 'imported'}` - : `Failed to ${stage ? 'stage' : 'import'} ${project}`; - if (unanswered) { + const title = deployFailureTitle(error, project, stage ? 'stage' : 'import'); + if (isUnansweredWrite(error)) { // It may have imported, so what the message says to check must be current. void invalidateEntityQueries(queryClient, entityId); void reloadRootEntries(); diff --git a/src/features/instance/applications/modals/RedeployApplicationModal.tsx b/src/features/instance/applications/modals/RedeployApplicationModal.tsx index 71bd9bd17..b499a59be 100644 --- a/src/features/instance/applications/modals/RedeployApplicationModal.tsx +++ b/src/features/instance/applications/modals/RedeployApplicationModal.tsx @@ -33,7 +33,13 @@ import { RefreshCwIcon } from 'lucide-react'; import { useCallback, useEffect, useRef, useState } from 'react'; import { useForm } from 'react-hook-form'; import { toast } from 'sonner'; -import { deployNotes, reportDeployOutcome, reportDeployResult, reportStaged } from './reportDeployHealth'; +import { + deployFailureTitle, + deployNotes, + reportDeployOutcome, + reportDeployResult, + reportStaged, +} from './reportDeployHealth'; export function RedeployApplicationModal() { const { value: isModalOpen, trigger } = useWatchedValue('ShowRedeployApplicationModal', false); @@ -148,7 +154,7 @@ export function RedeployApplicationModal() { } const title = rolloutFailed ? `${project} did not reach every node` - : redeployFailureTitle(error, project, stage); + : deployFailureTitle(error, project, stage ? 'stage' : 'redeploy'); toast.error(title, { description: message, action: recordAction(deployFailureDetails(error).deploymentId) }); return; } @@ -210,7 +216,7 @@ export function RedeployApplicationModal() { // It may have deployed, so the list the message says to check must be current. invalidateComponents(); } - toast.error(redeployFailureTitle(error, project, stage), { + toast.error(deployFailureTitle(error, project, stage ? 'stage' : 'redeploy'), { id: toastId, description: describeError(error).message, action, @@ -365,11 +371,3 @@ export function RedeployApplicationModal() { ); } - -/** Never "Failed" for a deploy that got no verdict: it may be live, and the message says to check first. */ -function redeployFailureTitle(error: unknown, project: string, stage: boolean): string { - if (isUnansweredWrite(error)) { - return `Couldn't confirm ${project} was ${stage ? 'staged' : 'redeployed'}`; - } - return `Failed to ${stage ? 'stage' : 'redeploy'} ${project}`; -} diff --git a/src/features/instance/applications/modals/reportDeployHealth.test.ts b/src/features/instance/applications/modals/reportDeployHealth.test.ts index 66e51cb99..11bfacdd8 100644 --- a/src/features/instance/applications/modals/reportDeployHealth.test.ts +++ b/src/features/instance/applications/modals/reportDeployHealth.test.ts @@ -5,6 +5,8 @@ vi.mock('sonner', () => ({ toast })); import { DeployComponentResult } from '@/integrations/api/instance/applications/deployComponentStream'; import { DeployRolloutError } from '@/integrations/api/instance/applications/restartJob'; +import { UNANSWERED_WRITE_MESSAGE } from '@/integrations/api/unansweredWrite'; +import { AxiosError, type InternalAxiosRequestConfig } from 'axios'; import { deployNotes, reportDeployFailure, @@ -120,6 +122,18 @@ describe('reportDeployFailure', () => { reportDeployFailure('web', new Error('404: no such deployment'), 'activate'); expect(toast.error).toHaveBeenCalledWith('Failed to activate web', { description: '404: no such deployment' }); }); + + it('never calls an activation that got no verdict failed', () => { + const sent = { method: 'post', data: '{"operation":"deploy_component"}' } as InternalAxiosRequestConfig; + reportDeployFailure( + 'web', + new AxiosError('timeout of 300000ms exceeded', AxiosError.ECONNABORTED, sent), + 'activate', + ); + expect(toast.error).toHaveBeenCalledWith("Couldn't confirm web was activated", { + description: UNANSWERED_WRITE_MESSAGE, + }); + }); }); describe('deployNotes', () => { diff --git a/src/features/instance/applications/modals/reportDeployHealth.ts b/src/features/instance/applications/modals/reportDeployHealth.ts index 55a5c90d0..2f74e3b56 100644 --- a/src/features/instance/applications/modals/reportDeployHealth.ts +++ b/src/features/instance/applications/modals/reportDeployHealth.ts @@ -5,6 +5,7 @@ import { import { ComponentHealthResult } from '@/features/instance/applications/hooks/useComponentHealthCheck'; import { DeployComponentResult } from '@/integrations/api/instance/applications/deployComponentStream'; import { DeployRolloutError } from '@/integrations/api/instance/applications/restartJob'; +import { isUnansweredWrite, UNANSWERED_WRITE_MESSAGE } from '@/integrations/api/unansweredWrite'; import { toast } from 'sonner'; interface DeployNotes { @@ -135,20 +136,38 @@ export function reportDeployResult( toast.success(success.title, { ...options, description: withNotes(success.description, notes) }); } +export type DeployVerb = 'import' | 'stage' | 'redeploy' | 'activate'; + +const DONE: Record = { + import: 'imported', + stage: 'staged', + redeploy: 'redeployed', + activate: 'activated', +}; + +/** Never "Failed" for a deploy that got no verdict: it may be live, and its message says to check first. */ +export function deployFailureTitle(error: unknown, project: string, verb: DeployVerb): string { + return isUnansweredWrite(error) ? `Couldn't confirm ${project} was ${DONE[verb]}` : `Failed to ${verb} ${project}`; +} + /** A deploy that failed: a rollout failure says the release is live where it was received, not that nothing landed. */ export function reportDeployFailure( project: string, error: unknown, - verb: string, + verb: DeployVerb, options: DeployToastOptions = {}, ): void { if (error instanceof DeployRolloutError) { toast.error(`${project} did not reach every node`, { ...options, description: error.message }); return; } - toast.error(`Failed to ${verb} ${project}`, { + toast.error(deployFailureTitle(error, project, verb), { ...options, - description: error instanceof Error ? error.message : undefined, + description: isUnansweredWrite(error) + ? UNANSWERED_WRITE_MESSAGE + : error instanceof Error + ? error.message + : undefined, }); } diff --git a/src/features/instance/config/deployments/components/ActivateStagedRelease.test.tsx b/src/features/instance/config/deployments/components/ActivateStagedRelease.test.tsx index 99576d143..40fc2a564 100644 --- a/src/features/instance/config/deployments/components/ActivateStagedRelease.test.tsx +++ b/src/features/instance/config/deployments/components/ActivateStagedRelease.test.tsx @@ -1,8 +1,10 @@ /** @vitest-environment jsdom */ import { Deployment } from '@/integrations/api/instance/deployments/types'; import { SSEInconclusiveError } from '@/integrations/api/sse/errors'; +import { UNANSWERED_WRITE_MESSAGE } from '@/integrations/api/unansweredWrite'; import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; import { act, cleanup, fireEvent, render, screen } from '@testing-library/react'; +import { AxiosError, type InternalAxiosRequestConfig } from 'axios'; import { afterEach, describe, expect, it, vi } from 'vitest'; const state = vi.hoisted(() => ({ @@ -27,7 +29,7 @@ vi.mock('@/features/instance/applications/hooks/useDeploymentRecordAction', () = useDeploymentRecordAction: () => () => undefined, })); vi.mock('@/features/instance/applications/components/DeployProgress/DeployProgress', () => ({ - DeployProgress: () =>
progress
, + DeployProgress: ({ state }: { state: { error?: string } }) =>
{state.error ?? 'progress'}
, })); vi.mock('@/features/instance/applications/modals/reportDeployHealth', () => ({ reportDeployOutcome: state.reportDeployOutcome, @@ -134,6 +136,16 @@ describe('ActivateStagedRelease', () => { consoleError.mockRestore(); }); + it('says in its progress panel that an activation with no verdict may have gone through', async () => { + const sent = { method: 'post', data: '{"operation":"deploy_component"}' } as InternalAxiosRequestConfig; + const unanswered = new AxiosError('timeout of 300000ms exceeded', AxiosError.ECONNABORTED, sent); + state.mutateAsync.mockRejectedValue(unanswered); + renderButton(); + confirm(); + expect(await screen.findByText(UNANSWERED_WRITE_MESSAGE)).toBeTruthy(); + expect(state.reportDeployFailure).toHaveBeenCalledWith('web', unanswered, 'activate', { action: undefined }); + }); + it('offers nothing where staging is not known to exist', () => { state.canStage = false; renderButton(); diff --git a/src/features/instance/config/deployments/components/ActivateStagedRelease.tsx b/src/features/instance/config/deployments/components/ActivateStagedRelease.tsx index d68b10344..b0c90c735 100644 --- a/src/features/instance/config/deployments/components/ActivateStagedRelease.tsx +++ b/src/features/instance/config/deployments/components/ActivateStagedRelease.tsx @@ -25,6 +25,7 @@ import { deployFailureDetails } from '@/integrations/api/instance/applications/d import { DeployRolloutError } from '@/integrations/api/instance/applications/restartJob'; import { Deployment } from '@/integrations/api/instance/deployments/types'; import { SSEInconclusiveError } from '@/integrations/api/sse/errors'; +import { isUnansweredWrite, UNANSWERED_WRITE_MESSAGE } from '@/integrations/api/unansweredWrite'; import { invalidateEntityQueries } from '@/react-query/invalidateEntityQueries'; import { useIsMutating, useQueryClient } from '@tanstack/react-query'; import { RocketIcon } from 'lucide-react'; @@ -84,7 +85,13 @@ export function ActivateStagedRelease({ deployment }: { deployment: Deployment } } stream.markSettled( 'error', - error instanceof DeployRolloutError ? undefined : error instanceof Error ? error.message : String(error), + error instanceof DeployRolloutError + ? undefined + : isUnansweredWrite(error) + ? UNANSWERED_WRITE_MESSAGE + : error instanceof Error + ? error.message + : String(error), { failure: error }, ); reportDeployFailure(project, error, 'activate', { From 7b6f7475ae8c2e201aa3aae2ac9b126e670c8b56 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Mon, 12 Oct 2026 21:40:14 -0400 Subject: [PATCH 07/16] refactor(deploy): keep DeployVerb private, and drop comments that repeat their neighbours Review round after the rebase (codex nit, kept by the Harper domain adjudicator): DeployVerb had no consumer outside reportDeployHealth.ts, since every caller passes a literal. The redeploy modal's second refresh comment repeated the first, and the skipGatewayRetry type doc listed package_component, whose own call site already says why it opts out. Co-Authored-By: Claude Opus 5.5 --- .../instance/applications/modals/RedeployApplicationModal.tsx | 1 - src/features/instance/applications/modals/reportDeployHealth.ts | 2 +- src/integrations/api/retryGatewayErrors.ts | 1 - 3 files changed, 1 insertion(+), 3 deletions(-) diff --git a/src/features/instance/applications/modals/RedeployApplicationModal.tsx b/src/features/instance/applications/modals/RedeployApplicationModal.tsx index b499a59be..c82b17dd6 100644 --- a/src/features/instance/applications/modals/RedeployApplicationModal.tsx +++ b/src/features/instance/applications/modals/RedeployApplicationModal.tsx @@ -213,7 +213,6 @@ export function RedeployApplicationModal() { return; } if (isUnansweredWrite(error)) { - // It may have deployed, so the list the message says to check must be current. invalidateComponents(); } toast.error(deployFailureTitle(error, project, stage ? 'stage' : 'redeploy'), { diff --git a/src/features/instance/applications/modals/reportDeployHealth.ts b/src/features/instance/applications/modals/reportDeployHealth.ts index 2f74e3b56..2c5de6f71 100644 --- a/src/features/instance/applications/modals/reportDeployHealth.ts +++ b/src/features/instance/applications/modals/reportDeployHealth.ts @@ -136,7 +136,7 @@ export function reportDeployResult( toast.success(success.title, { ...options, description: withNotes(success.description, notes) }); } -export type DeployVerb = 'import' | 'stage' | 'redeploy' | 'activate'; +type DeployVerb = 'import' | 'stage' | 'redeploy' | 'activate'; const DONE: Record = { import: 'imported', diff --git a/src/integrations/api/retryGatewayErrors.ts b/src/integrations/api/retryGatewayErrors.ts index 85d438e5c..766c2838d 100644 --- a/src/integrations/api/retryGatewayErrors.ts +++ b/src/integrations/api/retryGatewayErrors.ts @@ -7,7 +7,6 @@ declare module 'axios' { /** * Send at most once, even a read. The operations console sets it: its freeform body can name a * component's operation, and a `get_` name from a component is no proof that it only reads. - * `package_component` sets it too, a read that rebuilds the whole archive on every attempt. */ skipGatewayRetry?: boolean; } From 6138c3b52cd14bf061915d536c6ad1a5890eed26 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Sun, 11 Oct 2026 06:08:43 -0400 Subject: [PATCH 08/16] 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 09/16] 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 10/16] 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 11/16] 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 12/16] 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 13/16] 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 14/16] 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 15/16] 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 16/16] 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); }