diff --git a/DESIGN.md b/DESIGN.md index 5c4709e59..e3e11c5a6 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -33,7 +33,7 @@ The `/apis` route is a **custom, in-house API explorer** ([`src/features/instanc [`src/features/instance/operations/`](src/features/instance/operations/) sends one freeform operations-API body through the entity's own `getInstanceClient` client (direct, basic, or proxied — whatever every other page uses). The rules, each pinned by a test there: - **The typed text is the request body.** It is parsed only to validate it and read `operation`; re-serializing the parse would round integers past 2^53 (a primary key) before Harper saw them. Axios sends a string body that parses as JSON as-is, only trimmed. -- **At most one send.** It sets `skipGatewayRetry`, because the shared client otherwise replays a 502/503/504 up to three times and a gateway can answer that after Harper applied the operation. Only a request Studio refused before sending counts as unsent. Anything that reached Harper marks data stale, an error answer included, because an arbitrary operation can change something before it fails; and a request abandoned mid-flight (the page unmounted) is reported as possibly applied. +- **At most one send.** It sets `skipGatewayRetry`, because the shared client replays a 502/503/504 for any body whose `operation` reads by name, and a component's `get_…` operation is no proof that it only reads. Only a request Studio refused before sending counts as unsent. Anything that reached Harper marks data stale, an error answer included, because an arbitrary operation can change something before it fails; and a request abandoned mid-flight (the page unmounted) is reported as possibly applied. - **Only Harper's own reads skip the confirmation.** [`isBuiltInRead`](src/features/instance/operations/operationKinds.ts) is the restart gate's naming rule ([`isReadOperation`](src/integrations/api/isReadOperation.ts)) _and_ membership in the role editor's catalog of Harper's operations, because a component can register a mutating `get_…`. Everything else — unknown names, every `sql` — confirms, showing the exact body. It guards against a mistyped paste; Harper's `verifyPerms` is what authorizes each operation. - **Operations Studio has to know about are not sent blind.** A `restart` is tracked with `markRestarting` exactly like the Restart button's (released at once on a 4xx; a 5xx can be a gateway losing the answer to a restart that ran), so the page's reads are held (an instance) or their connectivity errors muted (a cluster, which restarts one member) until `waitUntilAnsweringAfterRestart` sees the target again. `logout` is refused: it would end the session the rest of Studio is still using. Any other operation with such a side effect needs the same treatment rather than a raw send. - **Nothing leaves the page.** Failures render inline and never pass through `console.error` or the global React Query handlers (RUM reports the former, and bodies carry passwords and secrets). The editor buffer is not persisted, and the console remounts per entity so a confirmation written for one instance can never be sent to another. After a possible write, entity queries are marked stale with `refetchType: 'none'`, never refetched into whatever the operation just did. The download is abandoned once progress passes 5 MB (XHR reports progress about every 50 ms, so this bounds, not caps, what arrives), and display truncates like the API explorer. @@ -62,6 +62,29 @@ The wide hero artwork spans both columns beneath the form card in three independ - **On 5.2+ the first instance is still written and restarted alone.** Only once it is back up on the new config is the write fanned out with `replicated: true` from that instance — even when it was the last instance to restart, because the fan-out also reaches live members Studio does not restart (not RUNNING, or failed earlier), which the per-instance path leaves on the old config. Fanning out first would leave an unproven config pending on every node, which a Cancel could no longer contain. - **A later instance skips its write only when the fan-out acknowledged that exact node** — its CM `replicationFqdn` (lower-cased) appears among the successful `replicated[]` entries. Fabric names each Harper node after that FQDN (host-manager sets `NODE_HOSTNAME` and `REPLICATION_HOSTNAME` to it). A count of acknowledgements is not proof: the origin's replication peers are not CM's restart targets, so a stale peer can stand in for a missing one. A target with no `replicationFqdn`, a rejected fan-out (the instance client rejects any response with a failed peer), or a response with no usable `replicated[]` all keep the per-instance write. +## Instance clients replay only reads + +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), 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`. +- **A read that is costly to repeat** stays a read here and opts out of the replay instead (`package_component`, `skipGatewayRetry`). + +## A restart Harper accepted never reads as failed + +Harper's answer to the restart request is its only verdict on the restart: `restart` answers before it tears down (harper `bin/restart.ts` schedules the teardown 50ms out so the reply can go first) and `restart_service` once its job has started (`executeJob`). So [`restartInstance`](src/integrations/api/instance/status/restartInstance.ts) rejects with the request's own error only when that request failed; after a 2xx, a target Studio never sees answer the `user_info` watch successfully rejects with [`RestartUnconfirmedError`](src/integrations/api/instance/status/restartUnconfirmed.ts), which reads "Not back yet", never "Failed", and is never a reason to send the restart again. + +- **The watch outlasts the usual recovery time** (one settle, then two probe schedules), and `onSlowToReturn` fires once between them so a caller can say it is still waiting. Every restart mark uses `RESTART_MARK_TTL_MS`, derived from the same constants, so no mark lapses while its watch still runs; the probes skip the gateway replay for the same reason. +- **Only a successful probe counts as back, never a 401/403.** Through central manager's operations proxy a 403 is CM refusing the caller before it contacts the instance (`notAllowed()` in its `HDBInstance.js`), so it says nothing about the target; reading it as "back" would report a restart Studio never saw finish. That is also why the copy says Studio "couldn't confirm it came back", not that the target never answered. +- **A one-at-a-time loop stops at a member that is not back** (cluster restart, rolling config update): restarting the next one could leave no member serving. Both loops also wait on a member whose restart got no verdict, and stop if that follow-up watch gives up too: it may have restarted and still be down, or, in a config update, restarted onto a configuration that broke it. Either report names the members it left alone, and it, like the no-verdict warning, outranks Cancel, which would otherwise hide the warning not to restart again; the report then says which members Cancel left. A Cancel clicked during a member's configuration write stops before its restart, and the report says the configuration may be pending there. On a 5.2+ cluster the config update never fans out from a canary that is not back, since it never proved it can run the configuration; a stop after the fan-out keeps the note that the members it left may already have the new configuration saved. +- **The chat tools return `success: true`** for it: their action was carried out, and `false` invites the agent to repeat it. + ## Cluster upgrade picker — which versions an existing cluster is offered [`buildUpgradeVersionOptions`](src/features/clusters/upsert/lib/buildUpgradeVersionOptions.ts) is the one place that decides, and its test file is the spec. Two rules, and they differ on purpose: diff --git a/src/features/auth/describeAuthFailure.ts b/src/features/auth/describeAuthFailure.ts index c866db7ba..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'; @@ -15,24 +16,15 @@ export const OUTCOME_UNKNOWN_MESSAGE = "We didn't get an answer from the server, so we can't tell whether that went through."; // 503 alone: the server is declining requests, so it very likely never processed this one and a -// plain retry is honest. (`curryRetryGatewayErrors` also classes 502/504 as retryable, but it is -// installed on instance clients only, never on `apiClient` — nothing retries these calls -// automatically, and the retry this copy invites is the user's own.) +// plain retry is honest. (`curryRetryGatewayErrors` replays 502/503/504, but only for reads and +// only on instance clients, never on `apiClient` — nothing retries these calls automatically, and +// the retry this copy invites is the user's own.) const TRANSIENT_STATUSES: ReadonlySet = new Set([503]); // Every form using this submits a non-idempotent POST, and each of these means the request had // already been handed upstream, so the write may have applied (RFC 9110 §9.2.2 — a retry is only // 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/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/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..b8bdf84f0 100644 --- a/src/features/instance/applications/components/Chat/tools/createApp/execute.ts +++ b/src/features/instance/applications/components/Chat/tools/createApp/execute.ts @@ -1,6 +1,7 @@ import { templates } from '@/features/instance/applications/components/NewApplication/templates'; import { addComponent } from '@/integrations/api/instance/applications/addComponent'; import { restartInstance } from '@/integrations/api/instance/status/restartInstance'; +import { RestartUnconfirmedError } from '@/integrations/api/instance/status/restartUnconfirmed'; import { findBy } from '@/lib/arrays/findBy'; import { emitToListeners } from '@/lib/events/listener'; import { toKebabCase } from '@/lib/string/to-kebab-case'; @@ -8,6 +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, toastToolFailure } from '../describeToolError'; import { Output } from './output'; export async function execute( @@ -37,9 +39,10 @@ export async function execute( template: template.npm || template.githubUrl, }); } catch (err) { + toastToolFailure(toastId, err); return { success: false, - message: `Error: ${err}`, + message: `Error: ${describeToolError(err)}`, }; } @@ -58,9 +61,20 @@ export async function execute( replicated: instanceClientParams.entityType === 'cluster', }); } catch (err) { + toastToolFailure(toastId, err); + if (err instanceof RestartUnconfirmedError) { + // The app exists either way; `success: false` would invite creating it again. + return { + success: true, + message: `App "${name}" created. Harper accepted the restart that loads it, but Studio couldn't confirm ` + + "the instance came back, so it may still be starting. Don't create the app or restart again until " + + 'its status shows it is back.', + webURL: baseURL, + }; + } return { success: false, - message: `Error: ${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..31c027455 --- /dev/null +++ b/src/features/instance/applications/components/Chat/tools/describeToolError.ts @@ -0,0 +1,13 @@ +import { isUnansweredWrite, UNANSWERED_WRITE_MESSAGE } from '@/integrations/api/unansweredWrite'; +import { describeError } from '@/react-query/queryClient'; +import { toast } from 'sonner'; + +export function describeToolError(err: unknown): string { + return isUnansweredWrite(err) ? UNANSWERED_WRITE_MESSAGE : String(err); +} + +/** A loading toast never dismisses itself, so a failed tool must replace its progress toast. */ +export function toastToolFailure(toastId: string | number, err: unknown) { + const { title, description } = describeError(err); + toast.error(title, { id: toastId, description, duration: 10_000 }); +} diff --git a/src/features/instance/applications/components/Chat/tools/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..d2e5abb91 100644 --- a/src/features/instance/applications/components/Chat/tools/restartHTTPService/execute.ts +++ b/src/features/instance/applications/components/Chat/tools/restartHTTPService/execute.ts @@ -1,8 +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, toastToolFailure } from '../describeToolError'; import { Output } from './output'; export async function execute( @@ -20,9 +22,20 @@ export async function execute( replicated: instanceClientParams.entityType === 'cluster', }); } catch (err) { + toastToolFailure(toastId, err); + if (err instanceof RestartUnconfirmedError) { + // `success` is whether the tool's own action was carried out, and Harper accepted the restart. + return { + success: true, + message: "Harper accepted the restart, but Studio couldn't confirm the HTTP service came back, so it may " + + "still be starting. Don't restart it again until its status shows it is back, or it goes down a " + + 'second time.', + webURL: baseURL, + }; + } return { success: false, - message: `Error: ${err}`, + 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/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..9ce0e9a2f 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,37 @@ 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(); + // 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', () => { 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..876b7c191 100644 --- a/src/features/instance/applications/components/NewApplication/useImportApplication.ts +++ b/src/features/instance/applications/components/NewApplication/useImportApplication.ts @@ -6,12 +6,17 @@ 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'; 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 +154,12 @@ export function useImportApplication( }); return; } - const title = `Failed to ${stage ? 'stage' : 'import'} ${project}`; + 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(); + } const { message } = describeError(error); if (formEntity.current === entityId) { setImportFailure({ entityId, failure: { title, message, deploymentId } }); @@ -174,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/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..f071e1cf7 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,77 @@ 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, + ); + 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, and refreshes the list", 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, + }); + 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 () => { + 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..c82b17dd6 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'; @@ -32,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); @@ -134,8 +141,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); @@ -146,7 +154,7 @@ export function RedeployApplicationModal() { } const title = rolloutFailed ? `${project} did not reach every node` - : `Failed to ${stage ? 'stage' : 'redeploy'} ${project}`; + : deployFailureTitle(error, project, stage ? 'stage' : 'redeploy'); toast.error(title, { description: message, action: recordAction(deployFailureDetails(error).deploymentId) }); return; } @@ -204,7 +212,10 @@ export function RedeployApplicationModal() { invalidateComponents(); return; } - toast.error(`Failed to ${stage ? 'stage' : 'redeploy'} ${project}`, { + if (isUnansweredWrite(error)) { + invalidateComponents(); + } + toast.error(deployFailureTitle(error, project, stage ? 'stage' : 'redeploy'), { id: toastId, description: describeError(error).message, action, 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/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..2c5de6f71 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) }); } +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', { 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/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/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..5dd6c7411 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(); @@ -35,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); }); @@ -70,6 +73,49 @@ 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 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) + .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..8ea696df4 100644 --- a/src/hooks/useApplyLicensesClick.tsx +++ b/src/hooks/useApplyLicensesClick.tsx @@ -3,7 +3,9 @@ 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 { pluralize } from '@/lib/pluralize'; import { sleep } from '@/lib/sleep'; import { useQueryClient } from '@tanstack/react-query'; import { useCallback, useState } from 'react'; @@ -50,6 +52,8 @@ 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]; @@ -83,10 +87,14 @@ export function useApplyLicensesClick({ licenses }: ApplyLicensesClickParams): A instanceClient, }); licensesApplied += 1; - } catch { + } 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. - 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 +135,19 @@ 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), + licensesFailed > 0 && `${pluralize(licensesFailed, 'other license', 'other licenses')} failed to apply.`, + ].filter(excludeFalsy).join(' ') + : `${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..a4e90b93e 100644 --- a/src/hooks/useRestartClusterClick.test.tsx +++ b/src/hooks/useRestartClusterClick.test.tsx @@ -2,9 +2,11 @@ * @vitest-environment jsdom */ import type { Cluster, Instance } from '@/integrations/api/api.patch'; -import { getRestartState } from '@/lib/restart/restartTracker'; +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'; -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', () => ({ @@ -18,7 +20,11 @@ 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', () => ({ + RESTART_MARK_TTL_MS: 600_000, + 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() }, @@ -27,7 +33,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'; @@ -58,7 +64,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' }; @@ -124,6 +132,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); @@ -169,3 +204,293 @@ 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 refused() { + return answeredWith(403); +} + +function notBackYet() { + return new RestartUnconfirmedError(new AxiosError('timeout of 3000ms exceeded', AxiosError.ECONNABORTED)); +} + +function restartsEndingIn(outcomes: Record>) { + vi.mocked(restartInstance).mockImplementation(async ({ instanceClient, onSlowToReturn }) => { + const target = targetOf(instanceClient); + restarted.push(target); + 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( + { 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))); + 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'); + }); + + it('stops there when Studio cannot confirm it came back either, rather than restart the next member', async () => { + vi.mocked(waitUntilAnsweringAfterRestart).mockRejectedValueOnce(new Error('timeout of 3000ms exceeded')); + restartsEndingIn({ 'ins-b': answeredWith(504) }); + + await clickRestart(); + + expect(restarted).toEqual(['ins-c', 'ins-b']); + expect(getRestartState('ins-b')).toBeUndefined(); + expect(finalError()).toEqual({ + title: OUTCOME_UNKNOWN_TITLE, + description: 'Only 1 "RUNNING" instance was confirmed restarted of 3 instances. ' + + `${describeUnansweredWrites(1)} ` + + "Studio couldn't confirm it came back either, so it may still be starting. " + + '1 other instance was not restarted, to keep the cluster available. ' + + 'Check its status before you restart it again.', + }); + }); + + it('names the members a Cancel left alone, beside the warning, when Cancel lands while waiting on it', async () => { + vi.mocked(waitUntilAnsweringAfterRestart).mockImplementationOnce(async () => cancel()); + restartsEndingIn({ 'ins-b': answeredWith(504) }); + + await clickRestart(); + + expect(restarted).toEqual(['ins-c', 'ins-b']); + expect(finalError()).toEqual({ + title: OUTCOME_UNKNOWN_TITLE, + description: 'Only 1 "RUNNING" instance was confirmed restarted of 3 instances. ' + + `${describeUnansweredWrites(1)} ` + + '1 other instance was not restarted: Cancel stopped it there.', + }); + }); +}); + +describe('useRestartClusterClick, for a member that accepted the restart but was never confirmed back', () => { + beforeEach(() => { + vi.mocked(getClusterInfo).mockResolvedValue( + { id: 'clu-1', instances: [instance('ins-a'), instance('ins-b'), instance('ins-c')] } as Cluster, + ); + }); + + it('stops there rather than restart another member, and never says the cluster failed', async () => { + restartsEndingIn({ 'ins-b': notBackYet() }); + + await clickRestart(); + + expect(restarted).toEqual(['ins-c', 'ins-b']); + expect(toast.loading).toHaveBeenCalledWith( + 'Waiting On Instance 2 of 3', + expect.objectContaining({ description: "It accepted the restart but isn't back yet." }), + ); + expect(finalError()).toEqual({ + title: 'Not back yet', + description: 'Only 1 "RUNNING" instance was confirmed restarted of 3 instances. ' + + "1 instance accepted the restart but couldn't be confirmed back, so it may still be starting. " + + '1 other instance was not restarted, to keep the cluster available. ' + + 'Check its status before you restart it again.', + }); + expect(toast.success).not.toHaveBeenCalled(); + expect(getRestartState('ins-b')).toBeUndefined(); + expect(getRestartState('clu-1')).toBeUndefined(); + }); + + it('names no members left alone when the last one is the one not back', async () => { + restartsEndingIn({ 'ins-a': notBackYet() }); + + await clickRestart(); + + expect(restarted).toEqual(['ins-c', 'ins-b', 'ins-a']); + expect(finalError().description).toBe( + 'Only 2 "RUNNING" instances were confirmed restarted of 3 instances. ' + + "1 instance accepted the restart but couldn't be confirmed back, so it may still be starting. " + + 'Check its status before you restart it again.', + ); + }); + + it('goes on to the next member once a slow one answers, holding it down until then', async () => { + let answer: (() => void) | undefined; + restartsEndingIn({ 'ins-b': new Promise((resolve) => (answer = resolve)) }); + const { result } = renderHook(() => useRestartClusterClick()); + + let click: Promise | undefined; + act(() => { + click = result.current.onRestartClick() as unknown as Promise; + }); + await vi.waitFor(() => expect(restarted).toEqual(['ins-c', 'ins-b'])); + + expect(getRestartState('ins-b')?.reach).toBe('down'); + const clock = vi.spyOn(Date, 'now').mockReturnValue(Date.now() + 6 * 60_000); + expect(getRestartState('ins-b')?.reach).toBe('down'); + expect(getRestartState('clu-1')).toBeDefined(); + clock.mockRestore(); + + await act(async () => { + answer!(); + await click; + }); + + expect(restarted).toEqual(['ins-c', 'ins-b', 'ins-a']); + expect(toast.success).toHaveBeenCalledWith('Success', expect.objectContaining({ id: 'toast-1' })); + expect(getRestartState('ins-b')).toBeUndefined(); + }); + + it('still carries on past a member that refused, and says it failed', async () => { + restartsEndingIn({ 'ins-b': refused() }); + + await clickRestart(); + + expect(restarted).toEqual(['ins-c', 'ins-b', 'ins-a']); + expect(toast.loading).toHaveBeenCalledWith('Failed Restarting Instance 2 of 3', expect.anything()); + expect(finalError().title).toBe('Error'); + expect(finalError().description).toContain('Only 2 "RUNNING" instances were restarted'); + }); + + it('names a member that refused alongside the one not back', async () => { + restartsEndingIn({ 'ins-c': refused(), 'ins-b': notBackYet() }); + + await clickRestart(); + + expect(restarted).toEqual(['ins-c', 'ins-b']); + expect(finalError()).toEqual({ + title: 'Not back yet', + description: 'Only 0 "RUNNING" instances were confirmed restarted of 3 instances. ' + + '1 instance failed to restart. ' + + "1 instance accepted the restart but couldn't be confirmed back, so it may still be starting. " + + '1 other instance was not restarted, to keep the cluster available. ' + + 'Check its status before you restart it again.', + }); + }); + + it('still says it is not back yet when Cancel is clicked while waiting on it', async () => { + vi.mocked(restartInstance).mockImplementation(async ({ instanceClient, onSlowToReturn }) => { + restarted.push(targetOf(instanceClient)); + if (targetOf(instanceClient) === 'ins-b') { + onSlowToReturn?.(); + cancel(); + throw notBackYet(); + } + return { message: 'Restarting Harper. This may take up to 60 seconds.' }; + }); + + await clickRestart(); + + expect(restarted).toEqual(['ins-c', 'ins-b']); + expect(finalError().title).toBe('Not back yet'); + expect(vi.mocked(toast.error).mock.calls.map(([title]) => title)).not.toContain('Cancelled'); + }); + + it('brings its waiting toast back without a second Cancel once Cancel was clicked', async () => { + vi.mocked(restartInstance).mockImplementation(async ({ instanceClient, onSlowToReturn }) => { + restarted.push(targetOf(instanceClient)); + if (targetOf(instanceClient) === 'ins-b') { + cancel(); + onSlowToReturn?.(); + } + return { message: 'Restarting Harper. This may take up to 60 seconds.' }; + }); + + await clickRestart(); + + const [, options] = vi.mocked(toast.loading).mock.calls.find(([title]) => title === 'Waiting On Instance 2 of 3')!; + expect(options).not.toHaveProperty('action'); + expect(options?.description).toContain('Cancelled: no other instance will be restarted.'); + expect(restarted).toEqual(['ins-c', 'ins-b']); + }); + + it('brings back none of its later toasts with a second Cancel once Cancel was clicked', async () => { + vi.mocked(restartInstance).mockImplementation(async ({ instanceClient }) => { + restarted.push(targetOf(instanceClient)); + if (targetOf(instanceClient) === 'ins-b') { + cancel(); + throw answeredWith(504); + } + return { message: 'Restarting Harper. This may take up to 60 seconds.' }; + }); + + await clickRestart(); + + const afterCancel = vi.mocked(toast.loading).mock.calls.filter(([title]) => + title === 'Could Not Confirm Instance 2 of 3' + ); + expect(afterCancel).toHaveLength(2); + for (const [, options] of afterCancel) { + expect(options).not.toHaveProperty('action'); + expect(options?.description).toContain('Cancelled: no other instance will be restarted.'); + } + expect(restarted).toEqual(['ins-c', 'ins-b']); + }); + + it('still says Cancelled when nothing was left uncertain', async () => { + vi.mocked(restartInstance).mockImplementation(async ({ instanceClient }) => { + restarted.push(targetOf(instanceClient)); + cancel(); + return { message: 'Restarting Harper. This may take up to 60 seconds.' }; + }); + + await clickRestart(); + + expect(restarted).toEqual(['ins-c']); + expect(finalError().title).toBe('Cancelled'); + }); +}); diff --git a/src/hooks/useRestartClusterClick.tsx b/src/hooks/useRestartClusterClick.tsx index ca5eefc76..4c7c7a070 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 } from '@/integrations/api/instance/status/restartInstance'; +import { + RESTART_MARK_TTL_MS, + restartInstance, + waitUntilAnsweringAfterRestart, +} from '@/integrations/api/instance/status/restartInstance'; +import { + describeUncertainRollingOutcome, + RestartUnconfirmedError, +} from '@/integrations/api/instance/status/restartUnconfirmed'; +import { isUnansweredWrite } from '@/integrations/api/unansweredWrite'; import { excludeFalsy } from '@/lib/arrays/excludeFalsy'; import { pluralize } from '@/lib/pluralize'; import { markRestarting } from '@/lib/restart/restartTracker'; @@ -10,9 +19,6 @@ import { useParams } from '@tanstack/react-router'; import { useCallback, useState } from 'react'; import { toast } from 'sonner'; -/** Longest a single `restartInstance` can take: 10s settle + 12 probes of up to 3s + 15s apart. */ -const INSTANCE_RESTART_TTL_MS = 5 * 60_000; - interface RestartClusterClickParams { onRestartedSuccessfully?: () => void; } @@ -44,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: ( @@ -72,12 +88,16 @@ export function useRestartClusterClick( } const { allInstances, targets } = plan; let instancesRestarted = 0; + let instancesUnanswered = 0; + let instancesFailed = 0; + let instancesUntouched: number | undefined; + let instancesStoppedOnNoVerdict = false; // One instance at a time, so the cluster stays reachable through the others unless it has only // one. The TTLs only matter if this loop dies without reaching `finally`. const releaseCluster = markRestarting([clusterId], { reach: targets.length === 1 ? 'down' : 'rolling', - ttlMs: (targets.length + 1) * INSTANCE_RESTART_TTL_MS, + ttlMs: (targets.length + 1) * RESTART_MARK_TTL_MS, }); try { @@ -107,22 +127,50 @@ export function useRestartClusterClick( } releaseInstance = markRestarting([instance.id], { reach: 'down', - ttlMs: INSTANCE_RESTART_TTL_MS, + ttlMs: RESTART_MARK_TTL_MS, }); // Then restart it. await restartInstance({ operation: 'restart', replicated: false, instanceClient, + onSlowToReturn: () => + toast.loading(`Waiting On Instance ${i + 1} of ${targets.length}`, { + id: toastId, + ...progress("It accepted the restart but isn't back yet."), + }), }); instancesRestarted += 1; - } catch { + } catch (error) { + if (error instanceof RestartUnconfirmedError) { + // It may still be down: restarting the next one could leave no member serving. + instancesUntouched = targets.length - i - 1; + break; + } + const unanswered = isUnansweredWrite(error); + instancesUnanswered += unanswered ? 1 : 0; + instancesFailed += unanswered ? 0 : 1; + if (unanswered) { + // A gateway can lose the answer to a restart that ran and took this member down, so it + // stays marked, and the next one waits, until it answers again or the watch gives up. + toast.loading(`Could Not Confirm Instance ${i + 1} of ${targets.length}`, { + id: toastId, + ...progress('Waiting for it to answer before carrying on.', 'Waiting for it to answer.'), + }); + const back = await waitUntilAnsweringAfterRestart({ instanceClient }).then(() => true, () => false); + if (!back) { + // 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. - toast.loading(`Failed Restarting Instance ${i + 1} of ${targets.length}`, { - ...toastConfig, + const verdict = unanswered ? 'Could Not Confirm' : 'Failed Restarting'; + toast.loading(`${verdict} Instance ${i + 1} of ${targets.length}`, { id: toastId, - description: 'We will carry on momentarily.', + ...progress('We will carry on momentarily.', ''), }); await sleep(3000); } @@ -136,7 +184,19 @@ export function useRestartClusterClick( releaseCluster(); } - if (canceled) { + const uncertain = describeUncertainRollingOutcome({ + total: allInstances.length, + confirmed: instancesRestarted, + failed: instancesFailed, + unanswered: instancesUnanswered, + untouched: instancesUntouched, + stoppedOnNoVerdict: instancesStoppedOnNoVerdict, + // Every member Cancel reached first lands in none of the counts. + canceled: canceled && instancesUntouched === undefined + ? targets.length - instancesRestarted - instancesFailed - instancesUnanswered + : 0, + }, 'restarted'); + if (canceled && !uncertain) { toast.error('Cancelled', { id: toastId, description: `The restart was partially cancelled.`, @@ -164,15 +224,16 @@ export function useRestartClusterClick( '"RUNNING" instance was', '"RUNNING" instances were', ); - toast.error('Error', { + toast.error(uncertain?.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: 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}.`, + 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..d9f067a8b --- /dev/null +++ b/src/hooks/useRestartInstanceClick.test.tsx @@ -0,0 +1,214 @@ +/** @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'; +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(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() }, +})); + +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) => ( + {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.mocked(waitUntilAnsweringAfterRestart).mockResolvedValue(undefined); + 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('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) => + 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); + expect(isRestarting('ins-1')).toBe(false); + expect(waitUntilAnsweringAfterRestart).not.toHaveBeenCalled(); + }); + + describe('a restart Harper accepted', () => { + it('says it is not back yet, never that it failed, when the target does not answer again', async () => { + const client = restartAccepted(neverAnswers); + const result = render(client as unknown as AxiosInstance); + + await act(() => result.current.onRestartClick()); + + expect(errorTitles()).toEqual([[RESTART_UNCONFIRMED_TITLE, RESTART_UNCONFIRMED_MESSAGE]]); + expect(toast.dismiss).toHaveBeenCalledWith('restart-toast'); + expect(toast.success).not.toHaveBeenCalled(); + expect(client.restarts()).toBe(1); + expect(client.probes()).toBe(24); + expect(isRestarting('ins-1')).toBe(false); + }); + + it('keeps the target marked and the button pending while it is slow to come back', async () => { + let answer: (() => void) | undefined; + const client = restartAccepted((attempt, config) => + attempt <= 12 + ? neverAnswers(attempt, config) + : new Promise((resolve) => { + answer = () => resolve({ username: 'admin' }); + }) + ); + const result = render(client as unknown as AxiosInstance); + + let click: Promise | undefined; + act(() => { + click = result.current.onRestartClick() as unknown as Promise; + }); + await vi.waitFor(() => expect(answer).toBeDefined()); + + expect(isRestarting('ins-1')).toBe(true); + const clock = vi.spyOn(Date, 'now').mockReturnValue(Date.now() + 6 * 60_000); + expect(isRestarting('ins-1')).toBe(true); + clock.mockRestore(); + expect(result.current.isRestartPending).toBe(true); + expect(toast.loading).toHaveBeenLastCalledWith('Still restarting', { + id: 'restart-toast', + description: "Harper accepted the restart, but the instance isn't back yet. Still waiting for it.", + }); + expect(toast.error).not.toHaveBeenCalled(); + + await act(async () => { + answer!(); + await click; + }); + + expect(isRestarting('ins-1')).toBe(false); + expect(toast.error).not.toHaveBeenCalled(); + expect(toast.success).toHaveBeenCalledWith( + 'Success', + expect.objectContaining({ description: 'Instance restarted!' }), + ); + expect(client.restarts()).toBe(1); + }); + + it('says it restarted, with no slow-return notice, when the target answers right away', async () => { + const client = restartAccepted(() => Promise.resolve({ username: 'admin' })); + const result = render(client as unknown as AxiosInstance); + + await act(() => result.current.onRestartClick()); + + expect(toast.error).not.toHaveBeenCalled(); + expect(toast.loading).not.toHaveBeenCalledWith('Still restarting', expect.anything()); + expect(toast.success).toHaveBeenCalledWith( + 'Success', + expect.objectContaining({ description: 'Instance restarted!' }), + ); + expect(isRestarting('ins-1')).toBe(false); + }); + }); +}); diff --git a/src/hooks/useRestartInstanceClick.ts b/src/hooks/useRestartInstanceClick.ts index a8094d17c..578bb240a 100644 --- a/src/hooks/useRestartInstanceClick.ts +++ b/src/hooks/useRestartInstanceClick.ts @@ -1,9 +1,15 @@ 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 { + 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'; +import { shouldToastOwnFailure } from '@/react-query/queryClient'; import { useQueryClient } from '@tanstack/react-query'; import { useParams } from '@tanstack/react-router'; import { useCallback } from 'react'; @@ -41,17 +47,27 @@ 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 // 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, replicated: operation === 'restart_service' && targetNoun === 'Cluster', instanceClient, + onSlowToReturn: () => + toast.loading('Still restarting', { + id: toastId, + description: `Harper accepted the restart, but the ${targetNoun.toLowerCase()} isn't back yet. ` + + 'Still waiting for it.', + }), }); // Releasing a tracked target refetches its queries (`onRestartSettled`); invalidating it here // too would cancel those refetches and resend them. @@ -69,18 +85,28 @@ 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(), - }, - }); + // 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()}.`, + action: { + label: 'Dismiss', + onClick: () => toast.dismiss(), + }, + }); + } } 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, or the watch for its target has given up, + // the errors that follow are real. + releaseTarget?.(); + } } }, [ clusterId, diff --git a/src/hooks/useRollingConfigUpdate.test.tsx b/src/hooks/useRollingConfigUpdate.test.tsx index f2560055b..b45cd590a 100644 --- a/src/hooks/useRollingConfigUpdate.test.tsx +++ b/src/hooks/useRollingConfigUpdate.test.tsx @@ -2,9 +2,11 @@ * @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'; -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'; @@ -32,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(), @@ -44,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'; @@ -117,6 +122,7 @@ beforeEach(() => { events.push(`restart ${targetOf(instanceClient)}`); return { message: 'ok' }; }); + vi.mocked(waitUntilAnsweringAfterRestart).mockResolvedValue(undefined); }); afterEach(() => vi.clearAllMocks()); @@ -445,6 +451,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', () => { @@ -485,3 +514,292 @@ function cancel() { const [, options] = vi.mocked(toast.loading).mock.calls.at(-1)!; (options as unknown as { action: { onClick: () => void } }).action.onClick(); } + +function notBackAfterRestart(...targets: string[]) { + vi.mocked(restartInstance).mockImplementation(async ({ instanceClient, onSlowToReturn }) => { + events.push(`restart ${targetOf(instanceClient)}`); + if (targets.includes(targetOf(instanceClient))) { + onSlowToReturn?.(); + throw new RestartUnconfirmedError(new AxiosError('Network Error', AxiosError.ERR_NETWORK)); + } + return { message: 'Restarting http' }; + }); +} + +function finalError() { + const [title, options] = vi.mocked(toast.error).mock.calls.at(-1) ?? []; + return { title, description: (options as { description?: string } | undefined)?.description }; +} + +describe('useRollingConfigUpdate, for a member that accepted the restart but was never confirmed back', () => { + it('stops there rather than update another member, and never says the update failed', async () => { + useCluster([instance('ins-a', '5.1.29'), instance('ins-b', '5.1.29')]); + notBackAfterRestart('ins-b'); + + await save(); + + expect(events).toEqual(['check ins-b', 'write ins-b', 'restart ins-b']); + expect(toast.loading).toHaveBeenCalledWith( + 'Waiting On Instance 1 of 2', + expect.objectContaining({ description: "It accepted the restart but isn't back yet." }), + ); + expect(finalError()).toEqual({ + title: 'Not back yet', + description: 'Only 0 "RUNNING" instances were confirmed updated of 2 instances. ' + + "1 instance accepted the restart but couldn't be confirmed back, so it may still be starting. " + + '1 other instance was not updated, to keep the cluster available. ' + + 'Check its status before you restart it again.', + }); + expect(toast.success).not.toHaveBeenCalled(); + }); + + it('still carries on past a member that refused the update, and says it failed', async () => { + useCluster([instance('ins-a', '5.1.29'), instance('ins-b', '5.1.29')]); + vi.mocked(setConfiguration).mockImplementation(async ({ instanceClient }) => { + events.push(`write ${targetOf(instanceClient)}`); + if (targetOf(instanceClient) === 'ins-b') { + throw new Error('Invalid configuration'); + } + return { message: 'ok' }; + }); + + await save(); + + expect(events).toEqual(['check ins-b', 'write ins-b', 'check ins-a', 'write ins-a', 'restart ins-a']); + expect(finalError().title).toBe('Error'); + expect(finalError().description).toContain('Failed to fully update cluster.'); + }); + + it('names a member that refused the update alongside the one not back', async () => { + useCluster([instance('ins-a', '5.1.29'), instance('ins-b', '5.1.29'), instance('ins-c', '5.1.29')]); + vi.mocked(setConfiguration).mockImplementation(async ({ instanceClient }) => { + events.push(`write ${targetOf(instanceClient)}`); + if (targetOf(instanceClient) === 'ins-c') { + throw new Error('Invalid configuration'); + } + return { message: 'ok' }; + }); + notBackAfterRestart('ins-b'); + + await save(); + + expect(events).toEqual(['check ins-c', 'write ins-c', 'check ins-b', 'write ins-b', 'restart ins-b']); + expect(finalError()).toEqual({ + title: 'Not back yet', + description: 'Only 0 "RUNNING" instances were confirmed updated of 3 instances. ' + + '1 instance failed to update. ' + + "1 instance accepted the restart but couldn't be confirmed back, so it may still be starting. " + + '1 other instance was not updated, to keep the cluster available. ' + + 'Check its status before you restart it again.', + }); + }); + + it('never fans the configuration out from a canary that is not back', async () => { + useCluster([instance('ins-a', '5.2.0'), instance('ins-b', '5.2.0'), instance('ins-c', '5.2.0')]); + notBackAfterRestart('ins-c'); + + await save(); + + expect(events).toEqual(['check ins-c', 'write ins-c', 'restart ins-c']); + expect(finalError().title).toBe('Not back yet'); + expect(finalError().description).not.toContain('may already have the new configuration saved'); + }); + + it('stops at a later member that is not back, and says the members it left may have the new configuration', async () => { + useCluster([instance('ins-a', '5.2.0'), instance('ins-b', '5.2.0'), instance('ins-c', '5.2.0')]); + vi.mocked(setConfiguration).mockImplementation(async ({ instanceClient, ...changes }) => { + events.push(`${changes.replicated ? 'replicate' : 'write'} ${targetOf(instanceClient)}`); + return changes.replicated ? acknowledging('ins-a-repl.example.com', 'ins-b-repl.example.com') : { message: 'ok' }; + }); + notBackAfterRestart('ins-b'); + + await save(); + + expect(events).toEqual([ + 'check ins-c', + 'write ins-c', + 'restart ins-c', + 'replicate ins-c', + 'check ins-b', + 'restart ins-b', + ]); + expect(finalError()).toEqual({ + title: 'Not back yet', + description: 'Only 1 "RUNNING" instance was confirmed updated of 3 instances. ' + + "1 instance accepted the restart but couldn't be confirmed back, so it may still be starting. " + + '1 other instance was not updated, to keep the cluster available. ' + + 'Check its status before you restart it again.\n' + + 'Instances that were not restarted may already have the new configuration saved, and will apply it ' + + 'the next time they restart.', + }); + }); + + it('still says it is not back yet when Cancel is clicked while waiting on it', async () => { + useCluster([instance('ins-a', '5.1.29'), instance('ins-b', '5.1.29')]); + vi.mocked(restartInstance).mockImplementation(async ({ instanceClient, onSlowToReturn }) => { + events.push(`restart ${targetOf(instanceClient)}`); + onSlowToReturn?.(); + cancel(); + throw new RestartUnconfirmedError(new AxiosError('Network Error', AxiosError.ERR_NETWORK)); + }); + + await save(); + + expect(events).toEqual(['check ins-b', 'write ins-b', 'restart ins-b']); + expect(finalError().title).toBe('Not back yet'); + expect(vi.mocked(toast.error).mock.calls.map(([title]) => title)).not.toContain('Cancelled'); + }); + + it('names the members a Cancel left alone, beside the warning, after a restart that got no verdict', async () => { + useCluster([instance('ins-a', '5.1.29'), instance('ins-b', '5.1.29')]); + vi.mocked(restartInstance).mockImplementation(async ({ instanceClient }) => { + events.push(`restart ${targetOf(instanceClient)}`); + cancel(); + throw new AxiosError('timeout of 60000ms exceeded', AxiosError.ECONNABORTED, { + method: 'post', + data: '{"operation":"restart_service"}', + } as InternalAxiosRequestConfig); + }); + + await save(); + + expect(events).toEqual(['check ins-b', 'write ins-b', 'restart ins-b']); + expect(finalError()).toEqual({ + title: OUTCOME_UNKNOWN_TITLE, + description: 'Only 0 "RUNNING" instances were confirmed updated of 2 instances. ' + + `${describeUnansweredWrites(1)} ` + + '1 other instance was not updated: Cancel stopped it there.', + }); + }); + + it('stops at a member whose restart got no verdict when Studio cannot confirm it came back either', async () => { + useCluster([instance('ins-a', '5.1.29'), instance('ins-b', '5.1.29')]); + vi.mocked(restartInstance).mockImplementation(async ({ instanceClient }) => { + events.push(`restart ${targetOf(instanceClient)}`); + throw new AxiosError('timeout of 60000ms exceeded', AxiosError.ECONNABORTED, { + method: 'post', + data: '{"operation":"restart_service"}', + } as InternalAxiosRequestConfig); + }); + vi.mocked(waitUntilAnsweringAfterRestart).mockRejectedValueOnce(new Error('timeout of 3000ms exceeded')); + + await save(); + + expect(events).toEqual(['check ins-b', 'write ins-b', 'restart ins-b']); + expect(finalError()).toEqual({ + title: OUTCOME_UNKNOWN_TITLE, + description: 'Only 0 "RUNNING" instances were confirmed updated of 2 instances. ' + + `${describeUnansweredWrites(1)} ` + + "Studio couldn't confirm it came back either, so it may still be starting. " + + '1 other instance was not updated, to keep the cluster available. ' + + 'Check its status before you restart it again.', + }); + }); + + it('does not wait on a member whose configuration write got no verdict, since nothing restarted it', async () => { + useCluster([instance('ins-a', '5.1.29'), instance('ins-b', '5.1.29')]); + vi.mocked(setConfiguration).mockImplementation(async ({ instanceClient }) => { + events.push(`write ${targetOf(instanceClient)}`); + if (targetOf(instanceClient) === 'ins-b') { + throw new AxiosError('timeout of 60000ms exceeded', AxiosError.ECONNABORTED, { + method: 'post', + data: '{"operation":"set_configuration"}', + } as InternalAxiosRequestConfig); + } + return { message: 'ok' }; + }); + + await save(); + + expect(waitUntilAnsweringAfterRestart).not.toHaveBeenCalled(); + expect(events).toEqual(['check ins-b', 'write ins-b', 'check ins-a', 'write ins-a', 'restart ins-a']); + }); + + it('honors a Cancel clicked during the configuration write before restarting, and says it may be pending', async () => { + useCluster([instance('ins-a', '5.1.29'), instance('ins-b', '5.1.29')]); + vi.mocked(setConfiguration).mockImplementation(async ({ instanceClient }) => { + events.push(`write ${targetOf(instanceClient)}`); + cancel(); + return { message: 'ok' }; + }); + + await save(); + + expect(events).toEqual(['check ins-b', 'write ins-b']); + expect(finalError()).toEqual({ + title: 'Cancelled', + description: 'The config update was partially cancelled. Instances that were not restarted may already ' + + 'have the new configuration saved, and will apply it the next time they restart.', + }); + }); + + it('brings its waiting toast back without a second Cancel once Cancel was clicked', async () => { + useCluster([instance('ins-a', '5.1.29'), instance('ins-b', '5.1.29')]); + vi.mocked(restartInstance).mockImplementation(async ({ instanceClient, onSlowToReturn }) => { + events.push(`restart ${targetOf(instanceClient)}`); + cancel(); + onSlowToReturn?.(); + return { message: 'Restarting http' }; + }); + + await save(); + + const [, options] = vi.mocked(toast.loading).mock.calls.find(([title]) => title === 'Waiting On Instance 1 of 2')!; + expect(options).not.toHaveProperty('action'); + expect(options?.description).toContain('Cancelled: no other instance will be updated.'); + }); + + it('brings back none of its later toasts with a second Cancel once Cancel was clicked', async () => { + useCluster([instance('ins-a', '5.1.29'), instance('ins-b', '5.1.29')]); + vi.mocked(restartInstance).mockImplementation(async ({ instanceClient }) => { + events.push(`restart ${targetOf(instanceClient)}`); + cancel(); + throw new AxiosError('timeout of 60000ms exceeded', AxiosError.ECONNABORTED, { + method: 'post', + data: '{"operation":"restart_service"}', + } as InternalAxiosRequestConfig); + }); + + await save(); + + const afterCancel = vi.mocked(toast.loading).mock.calls.filter(([title]) => + title === 'Could Not Confirm Instance 1 of 2' + ); + expect(afterCancel).toHaveLength(2); + for (const [, options] of afterCancel) { + expect(options).not.toHaveProperty('action'); + expect(options?.description).toContain('Cancelled: no other instance will be updated.'); + } + expect(events).toEqual(['check ins-b', 'write ins-b', 'restart ins-b']); + }); + + it('keeps the pending-configuration note when Cancel lands while waiting on a member after the fan-out', async () => { + useCluster([instance('ins-a', '5.2.0'), instance('ins-b', '5.2.0')]); + vi.mocked(setConfiguration).mockImplementation(async ({ instanceClient, ...changes }) => { + events.push(`${changes.replicated ? 'replicate' : 'write'} ${targetOf(instanceClient)}`); + return changes.replicated ? acknowledging('ins-a-repl.example.com') : { message: 'ok' }; + }); + vi.mocked(restartInstance).mockImplementation(async ({ instanceClient, onSlowToReturn }) => { + events.push(`restart ${targetOf(instanceClient)}`); + if (targetOf(instanceClient) === 'ins-a') { + onSlowToReturn?.(); + cancel(); + throw new RestartUnconfirmedError(new AxiosError('Network Error', AxiosError.ERR_NETWORK)); + } + return { message: 'Restarting http' }; + }); + + await save(); + + expect(events).toEqual([ + 'check ins-b', + 'write ins-b', + 'restart ins-b', + 'replicate ins-b', + 'check ins-a', + 'restart ins-a', + ]); + expect(finalError().title).toBe('Not back yet'); + expect(finalError().description).toContain('may already have the new configuration saved'); + }); +}); diff --git a/src/hooks/useRollingConfigUpdate.tsx b/src/hooks/useRollingConfigUpdate.tsx index 12e8fe22a..5379d8e49 100644 --- a/src/hooks/useRollingConfigUpdate.tsx +++ b/src/hooks/useRollingConfigUpdate.tsx @@ -4,9 +4,14 @@ import { useInstanceClientIdParams } from '@/config/useInstanceClient'; import { getClusterRestartTargets } from '@/hooks/getClusterRestartTargets'; import { Instance } from '@/integrations/api/api.patch'; import { getInstanceUserInfo } from '@/integrations/api/instance/status/getInstanceUserInfo'; -import { restartInstance } from '@/integrations/api/instance/status/restartInstance'; +import { restartInstance, waitUntilAnsweringAfterRestart } from '@/integrations/api/instance/status/restartInstance'; +import { + describeUncertainRollingOutcome, + RestartUnconfirmedError, +} from '@/integrations/api/instance/status/restartUnconfirmed'; import { setConfiguration, shouldFanOutSetConfiguration } from '@/integrations/api/instance/status/setConfiguration'; import { acknowledgedReplicationNodes } from '@/integrations/api/replication'; +import { isUnansweredWrite } from '@/integrations/api/unansweredWrite'; import { excludeFalsy } from '@/lib/arrays/excludeFalsy'; import { pluralize } from '@/lib/pluralize'; import { sleep } from '@/lib/sleep'; @@ -79,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: ( @@ -109,6 +124,11 @@ export function useRollingConfigUpdate( let fanOutAttempted = false; let acknowledged = new Set(); let instancesRestarted = 0; + let instancesUnanswered = 0; + let instancesFailed = 0; + let instancesUntouched: number | undefined; + let instancesStoppedOnNoVerdict = false; + let writtenBeforeCancel = false; for (let i = 0; i < targets.length; i++) { const { instanceClient, replicationFqdn } = targets[i]; @@ -126,6 +146,7 @@ export function useRollingConfigUpdate( ), }); let restarted = false; + let restartSent = false; try { // Make sure the instance is responding. await getInstanceUserInfo({ @@ -140,22 +161,54 @@ export function useRollingConfigUpdate( ...params, instanceClient, }); + if (canceled) { + writtenBeforeCancel = true; + break; + } } // Then restart it. + restartSent = true; await restartInstance({ operation: 'restart_service', replicated: false, instanceClient, + onSlowToReturn: () => + toast.loading(`Waiting On Instance ${i + 1} of ${targets.length}`, { + id: toastId, + ...progress("It accepted the restart but isn't back yet."), + }), }); instancesRestarted += 1; restarted = true; - } catch { + } catch (error) { + if (error instanceof RestartUnconfirmedError) { + // It may still be down: updating the next one could leave no member serving. A canary that is + // not back never proved it can run the configuration, so it is not fanned out either. + instancesUntouched = targets.length - i - 1; + break; + } + const unanswered = isUnansweredWrite(error); + instancesUnanswered += unanswered ? 1 : 0; + instancesFailed += unanswered ? 0 : 1; + if (unanswered && restartSent) { + // The restart may have run onto a configuration that broke it: wait, and stop if it never answers. + toast.loading(`Could Not Confirm Instance ${i + 1} of ${targets.length}`, { + id: toastId, + ...progress('Waiting for it to answer before carrying on.', 'Waiting for it to answer.'), + }); + const back = await waitUntilAnsweringAfterRestart({ instanceClient }).then(() => true, () => false); + if (!back) { + instancesUntouched = targets.length - i - 1; + instancesStoppedOnNoVerdict = true; + break; + } + } if (i + 1 !== targets.length) { // If it fails to restart, or wasn't available, warn for a bit then move on. - toast.loading(`Failed Restarting Instance ${i + 1} of ${targets.length}`, { - ...toastConfig, + const verdict = unanswered ? 'Could Not Confirm' : 'Failed Restarting'; + toast.loading(`${verdict} Instance ${i + 1} of ${targets.length}`, { id: toastId, - description: 'We will carry on momentarily.', + ...progress('We will carry on momentarily.', ''), }); await sleep(3000); } @@ -183,10 +236,24 @@ export function useRollingConfigUpdate( } } - if (canceled) { + const uncertain = describeUncertainRollingOutcome({ + total: allInstances.length, + confirmed: instancesRestarted, + failed: instancesFailed, + unanswered: instancesUnanswered, + untouched: instancesUntouched, + stoppedOnNoVerdict: instancesStoppedOnNoVerdict, + // Every member Cancel reached first lands in none of the counts. + canceled: canceled && instancesUntouched === undefined + ? targets.length - instancesRestarted - instancesFailed - instancesUnanswered + : 0, + }, 'updated'); + if (canceled && !uncertain) { toast.error('Cancelled', { id: toastId, - description: `The config update was partially cancelled.${fanOutAttempted ? ` ${MAY_BE_PENDING}` : ''}`, + description: `The config update was partially cancelled.${ + fanOutAttempted || writtenBeforeCancel ? ` ${MAY_BE_PENDING}` : '' + }`, duration: 10_000, action: { label: 'Dismiss', @@ -211,16 +278,17 @@ export function useRollingConfigUpdate( '"RUNNING" instance was', '"RUNNING" instances were', ); - toast.error('Error', { + toast.error(uncertain?.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() || '') - + (fanOutAttempted ? '\n' + MAY_BE_PENDING : ''), + description: (uncertain?.description + ?? `Failed to fully update cluster.\n` + + ([ + allInstances.length === 0 && 'No instances were found within the cluster to restart.', + targets.length === 0 && `No instances were in a "RUNNING" state of ${allTheInstances}.`, + allInstances.length !== instancesRestarted + && `Only ${someRunningInstancesWere} restarted of ${allTheInstances}.`, + ].filter(excludeFalsy).shift() || '')) + + (fanOutAttempted || writtenBeforeCancel ? '\n' + MAY_BE_PENDING : ''), duration: 10_000, action: { label: 'Dismiss', 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/instance/status/restartInstance.test.ts b/src/integrations/api/instance/status/restartInstance.test.ts new file mode 100644 index 000000000..6579de8d1 --- /dev/null +++ b/src/integrations/api/instance/status/restartInstance.test.ts @@ -0,0 +1,212 @@ +/** @vitest-environment jsdom */ +import { getInstanceClient } from '@/config/getInstanceClient'; +import { RESTART_MARK_TTL_MS, restartInstance } from '@/integrations/api/instance/status/restartInstance'; +import { RestartUnconfirmedError } from '@/integrations/api/instance/status/restartUnconfirmed'; +import { isUnansweredWrite } from '@/integrations/api/unansweredWrite'; +import { shouldKeepEvent } from '@/integrations/datadog/shouldKeepEvent'; +import { sleep } from '@/lib/sleep'; +import { AxiosError, type AxiosResponse, type InternalAxiosRequestConfig } from 'axios'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +vi.mock('@/lib/sleep', () => ({ + sleep: vi.fn().mockResolvedValue(true), +})); + +type Answer = (config: InternalAxiosRequestConfig, attempt: number) => AxiosResponse | never; + +// Harper's own answer to an accepted `restart` (bin/restart.ts RESTART_RESPONSE, wrapped as { message }). +const ACCEPTED = { message: 'Restarting Harper. This may take up to 60 seconds.' }; + +function ok(data: unknown): Answer { + return (config) => ({ status: 200, statusText: 'OK', data, headers: {}, config }); +} + +function failing(status: number, data: unknown): Answer { + return (config) => { + throw new AxiosError( + `Request failed with status code ${status}`, + status >= 500 ? AxiosError.ERR_BAD_RESPONSE : AxiosError.ERR_BAD_REQUEST, + config, + undefined, + { status, statusText: '', data, headers: {}, config }, + ); + }; +} + +function timingOut(config: InternalAxiosRequestConfig): never { + throw new AxiosError(`timeout of ${config.timeout}ms exceeded`, AxiosError.ECONNABORTED, config); +} + +function answersFrom(attempt: number): Answer { + return (config, probe) => probe < attempt ? timingOut(config) : ok({ username: 'admin' })(config, probe); +} + +const sent: string[] = []; + +function instanceClient({ restart, userInfo }: { restart: Answer; userInfo: Answer }) { + let probes = 0; + const client = getInstanceClient({ id: 'ins-restart', operationsUrl: 'https://ins-restart.example.com:9925/' }); + client.defaults.adapter = async (config: InternalAxiosRequestConfig) => { + const { operation } = JSON.parse(String(config.data)) as { operation: string }; + sent.push(operation); + return operation === 'user_info' ? userInfo(config, ++probes) : restart(config, 0); + }; + return client; +} + +const probesSent = () => sent.filter((operation) => operation === 'user_info').length; + +async function failureOf(promise: Promise): Promise { + return promise.then(() => expect.unreachable('the restart should have been rejected'), (error: unknown) => error); +} + +describe('restartInstance', () => { + beforeEach(() => { + sent.length = 0; + vi.mocked(sleep).mockClear(); + }); + + it('resolves with Harper’s answer once the target answers again', async () => { + const client = instanceClient({ restart: ok(ACCEPTED), userInfo: ok({ username: 'admin' }) }); + + await expect(restartInstance({ operation: 'restart', replicated: false, instanceClient: client })) + .resolves.toEqual(ACCEPTED); + expect(sent).toEqual(['restart', 'user_info']); + }); + + it('keeps watching a target that is slow to come back, and says so once', async () => { + const client = instanceClient({ restart: ok(ACCEPTED), userInfo: answersFrom(20) }); + const onSlowToReturn = vi.fn(); + + await expect(restartInstance({ operation: 'restart', replicated: false, instanceClient: client, onSlowToReturn })) + .resolves.toEqual(ACCEPTED); + expect(probesSent()).toBe(20); + expect(onSlowToReturn).toHaveBeenCalledOnce(); + }); + + it('keeps watching when the slow-return callback throws', async () => { + const client = instanceClient({ restart: ok(ACCEPTED), userInfo: answersFrom(20) }); + const broken = new Error('toast unavailable'); + const consoleError = vi.spyOn(console, 'error').mockImplementation(() => undefined); + + await expect(restartInstance({ + operation: 'restart', + replicated: false, + instanceClient: client, + onSlowToReturn: () => { + throw broken; + }, + })).resolves.toEqual(ACCEPTED); + expect(probesSent()).toBe(20); + expect(consoleError).toHaveBeenCalledWith(broken); + consoleError.mockRestore(); + }); + + it('does not call a target slow that answers within the usual time', async () => { + const client = instanceClient({ restart: ok(ACCEPTED), userInfo: answersFrom(12) }); + const onSlowToReturn = vi.fn(); + + await restartInstance({ operation: 'restart', replicated: false, instanceClient: client, onSlowToReturn }); + + expect(onSlowToReturn).not.toHaveBeenCalled(); + }); + + it('says an accepted restart is unconfirmed, not failed, when the target never answers again', async () => { + const client = instanceClient({ restart: ok(ACCEPTED), userInfo: timingOut }); + + const error = await failureOf(restartInstance({ operation: 'restart', replicated: false, instanceClient: client })); + + expect(error).toBeInstanceOf(RestartUnconfirmedError); + expect((error as RestartUnconfirmedError).cause).toBeInstanceOf(AxiosError); + expect((error as RestartUnconfirmedError).cause).toMatchObject({ code: AxiosError.ECONNABORTED }); + expect(sent.filter((operation) => operation === 'restart')).toHaveLength(1); + expect(probesSent()).toBe(24); + expect(vi.mocked(sleep).mock.calls.map(([ms]) => ms)).toEqual([10_000, ...Array(23).fill(15_000)]); + }); + + it('fits its request, every probe and every wait inside RESTART_MARK_TTL_MS', async () => { + const client = instanceClient({ restart: ok(ACCEPTED), userInfo: timingOut }); + client.defaults.timeout = 0; + const timeouts: number[] = []; + const adapter = client.defaults.adapter as (config: InternalAxiosRequestConfig) => Promise; + client.defaults.adapter = (config: InternalAxiosRequestConfig) => { + timeouts.push(config.timeout ?? Number.POSITIVE_INFINITY); + return adapter(config); + }; + + await failureOf(restartInstance({ operation: 'restart', replicated: false, instanceClient: client })); + + expect(timeouts[0]).toBe(60_000); + expect(timeouts).toHaveLength(25); + const waited = vi.mocked(sleep).mock.calls.reduce((total, [ms = 1000]) => total + ms, 0); + expect(timeouts.reduce((total, ms) => total + ms, 0) + waited).toBeLessThan(RESTART_MARK_TTL_MS); + }); + + it('sends each probe once, even when a gateway answers it', async () => { + const client = instanceClient({ restart: ok(ACCEPTED), userInfo: failing(503, 'Service Unavailable') }); + + const error = await failureOf(restartInstance({ operation: 'restart', replicated: false, instanceClient: client })); + + expect(error).toBeInstanceOf(RestartUnconfirmedError); + expect(probesSent()).toBe(24); + }); + + it('treats restart_service the same way: its job started, so the restart was accepted', async () => { + const client = instanceClient({ + restart: ok({ message: 'Restarting http', job_id: 'job-1' }), + userInfo: timingOut, + }); + + const error = await failureOf( + restartInstance({ operation: 'restart_service', replicated: false, instanceClient: client }), + ); + + expect(error).toBeInstanceOf(RestartUnconfirmedError); + }); + + it.each([ + ['refused', failing(403, { error: 'This operation is restricted to super users' })], + ['answered by a gateway', failing(504, '504 Gateway Time-out')], + ['never answered', timingOut], + ])('rejects with the restart request’s own error when it was %s', async (_label, restart) => { + const client = instanceClient({ restart, userInfo: ok({ username: 'admin' }) }); + + const error = await failureOf(restartInstance({ operation: 'restart', replicated: false, instanceClient: client })); + + expect(error).toBeInstanceOf(AxiosError); + expect(error).not.toBeInstanceOf(RestartUnconfirmedError); + expect(sent).not.toContain('user_info'); + }); + + it('leaves a restart that got no verdict to the unanswered-write warning', async () => { + const client = instanceClient({ restart: failing(504, 'gateway'), userInfo: ok({}) }); + + const error = await failureOf(restartInstance({ operation: 'restart', replicated: false, instanceClient: client })); + + expect(isUnansweredWrite(error)).toBe(true); + }); +}); + +describe('RestartUnconfirmedError', () => { + function consoleEvent(error: Error) { + return { type: 'error', error: { message: error.message, type: error.name, source: 'console' } }; + } + + it('reaches RUM, or not, exactly as the probe failure it wraps would', () => { + const timeout = new AxiosError('timeout of 3000ms exceeded', AxiosError.ECONNABORTED); + const network = new AxiosError('Network Error', AxiosError.ERR_NETWORK); + + for (const probeFailure of [timeout, network]) { + expect(shouldKeepEvent(consoleEvent(new RestartUnconfirmedError(probeFailure)))) + .toBe(shouldKeepEvent(consoleEvent(probeFailure))); + } + expect(shouldKeepEvent(consoleEvent(new RestartUnconfirmedError(timeout)))).toBe(false); + }); + + it.each([null, undefined, 'socket hang up', { code: 'E_SOMETHING' }])('accepts any cause (%o)', (cause) => { + const error = new RestartUnconfirmedError(cause); + + expect(error.cause).toBe(cause); + expect(error.message).toMatch(/^The restart was accepted, but the target was not confirmed back: /); + }); +}); diff --git a/src/integrations/api/instance/status/restartInstance.ts b/src/integrations/api/instance/status/restartInstance.ts index 4bff908ef..6f46302f7 100644 --- a/src/integrations/api/instance/status/restartInstance.ts +++ b/src/integrations/api/instance/status/restartInstance.ts @@ -1,17 +1,40 @@ import { InstanceClientConfig } from '@/config/instanceClientConfig'; import { getInstanceUserInfo } from '@/integrations/api/instance/status/getInstanceUserInfo'; +import { RestartUnconfirmedError } from '@/integrations/api/instance/status/restartUnconfirmed'; import { ReplicatedResponse } from '@/integrations/api/replication'; import { axiosRetry } from '@/lib/axiosRetry'; import { sleep } from '@/lib/sleep'; import { useMutation } from '@tanstack/react-query'; +const SETTLE_MS = 10_000; +const PROBE_TIMEOUT_MS = 3_000; +const PROBE_INTERVAL_MS = 15_000; +const PROBES = 24; +const SLOW_AFTER_PROBES = 12; +const RESTART_REQUEST_TIMEOUT_MS = 60_000; + +const LONGEST_RESTART_MS = RESTART_REQUEST_TIMEOUT_MS + SETTLE_MS + PROBES * PROBE_TIMEOUT_MS + + (PROBES - 1) * PROBE_INTERVAL_MS; + +/** + * For marking a restart's target: longer than the longest restart, so the mark lapses only if its + * owner never releases it, never while the watch below still expects the target back. + */ +export const RESTART_MARK_TTL_MS = LONGEST_RESTART_MS + 60_000; + interface RestartInstanceParams { operation: 'restart_service' | 'restart'; replicated: boolean; + /** Called once the target is still not back past the usual recovery time; the watch goes on. */ + onSlowToReturn?: () => void; } +/** + * Rejects with the restart request's own error when Harper refused it or never answered it, and with + * {@link RestartUnconfirmedError} when Harper accepted it but the watch never saw the target back. + */ export async function restartInstance( - { operation, replicated, instanceClient }: RestartInstanceParams & InstanceClientConfig, + { operation, replicated, instanceClient, onSlowToReturn }: RestartInstanceParams & InstanceClientConfig, ): Promise { // Both calls skip the restart gate: the caller may already have marked this instance as // restarting, and these are the requests driving and watching that restart. @@ -19,19 +42,38 @@ export async function restartInstance( operation, service: operation === 'restart_service' ? 'http' : undefined, replicated, - }, { skipRestartGate: true }); - await waitUntilAnsweringAfterRestart({ instanceClient }); + }, { skipRestartGate: true, timeout: RESTART_REQUEST_TIMEOUT_MS }); + try { + await waitUntilAnsweringAfterRestart({ instanceClient, onSlowToReturn }); + } catch (cause) { + throw new RestartUnconfirmedError(cause); + } return data; } -/** Resolves once a just-restarted target answers again; rejects if it never does within ~3 minutes. */ -export async function waitUntilAnsweringAfterRestart({ instanceClient }: InstanceClientConfig): Promise { - await sleep(10_000); - await axiosRetry( - () => getInstanceUserInfo({ instanceClient, timeout: 3_000, skipRestartGate: true }), - 12, - 15_000, - ); +/** + * Resolves once a just-restarted target answers `user_info`; rejects with the last probe's error if it never + * does. Only a success counts: through central manager's proxy even a 401/403 is CM's answer, not the target's. + */ +export async function waitUntilAnsweringAfterRestart( + { instanceClient, onSlowToReturn }: InstanceClientConfig & Pick, +): Promise { + // Not replayed on a gateway error: these probes are the retry, and a replay would outlast the mark. + const probe = () => + getInstanceUserInfo({ instanceClient, timeout: PROBE_TIMEOUT_MS, skipRestartGate: true, skipGatewayRetry: true }); + await sleep(SETTLE_MS); + try { + await axiosRetry(probe, SLOW_AFTER_PROBES, PROBE_INTERVAL_MS); + } catch { + try { + onSlowToReturn?.(); + } catch (error) { + // A broken progress callback must not cut the watch short. + console.error(error); + } + await sleep(PROBE_INTERVAL_MS); + await axiosRetry(probe, PROBES - SLOW_AFTER_PROBES, PROBE_INTERVAL_MS); + } } export function useRestartInstance() { diff --git a/src/integrations/api/instance/status/restartUnconfirmed.ts b/src/integrations/api/instance/status/restartUnconfirmed.ts new file mode 100644 index 000000000..87efe6ba3 --- /dev/null +++ b/src/integrations/api/instance/status/restartUnconfirmed.ts @@ -0,0 +1,76 @@ +import { describeUnansweredWrites, OUTCOME_UNKNOWN_TITLE } from '@/integrations/api/unansweredWrite'; +import { excludeFalsy } from '@/lib/arrays/excludeFalsy'; +import { errorText } from '@/lib/errorText'; +import { pluralize } from '@/lib/pluralize'; + +export const RESTART_UNCONFIRMED_TITLE = 'Not back yet'; + +export const RESTART_UNCONFIRMED_MESSAGE = "Harper accepted the restart, but Studio couldn't confirm it came back. " + + 'It may still be starting, so check its status before you restart it again.'; + +/** + * Harper answered the restart request with success, so the restart ran, but Studio never saw the + * target answer again while it watched. Not a failed restart: sending it again would take the target + * down a second time. + */ +export class RestartUnconfirmedError extends Error { + constructor(cause: unknown) { + // The probe's text stays in the message because Datadog's `shouldKeepEvent` filters on it. + super(`The restart was accepted, but the target was not confirmed back: ${errorText(cause) ?? 'no answer'}`, { + cause, + }); + this.name = 'RestartUnconfirmedError'; + } +} + +interface RollingOutcome { + total: number; + confirmed: number; + failed: number; + unanswered: number; + /** Set when a member never came back and the loop stopped there: the members after it, left alone. */ + untouched?: number; + /** The member the loop stopped at got no verdict on its restart, rather than accepting it. */ + stoppedOnNoVerdict?: boolean; + /** Members a Cancel left alone. */ + canceled?: number; +} + +/** + * The final toast of a loop that restarts one member at a time, when some member's outcome is not + * known; undefined when every outcome is. + */ +export function describeUncertainRollingOutcome( + { total, confirmed, failed, unanswered, untouched, stoppedOnNoVerdict, canceled = 0 }: RollingOutcome, + verb: 'restarted' | 'updated', +): { title: string; description: string } | undefined { + if (!unanswered && untouched === undefined) { + return undefined; + } + const confirmedOf = `${pluralize(confirmed, '"RUNNING" instance was', '"RUNNING" instances were')} confirmed ${verb} ` + + `of ${pluralize(total, 'instance', 'instances')}.`; + return { + title: unanswered ? OUTCOME_UNKNOWN_TITLE : RESTART_UNCONFIRMED_TITLE, + description: [ + `Only ${confirmedOf}`, + failed > 0 + && `${pluralize(failed, 'instance', 'instances')} failed to ${verb === 'restarted' ? 'restart' : 'update'}.`, + unanswered > 0 && describeUnansweredWrites(unanswered), + untouched !== undefined && describeNoReturn(untouched, verb, stoppedOnNoVerdict ? unanswered : 0), + canceled > 0 + && `${pluralize(canceled, 'other instance was', 'other instances were')} not ${verb}: Cancel stopped it there.`, + ].filter(excludeFalsy).join(' '), + }; +} + +/** `unanswered` is non-zero when the member it stopped at got no verdict, and counts those requests. */ +function describeNoReturn(untouched: number, verb: 'restarted' | 'updated', unanswered: number): string { + const leftAlone = pluralize(untouched, 'other instance was', 'other instances were'); + const notBack = !unanswered + ? "1 instance accepted the restart but couldn't be confirmed back, so it may still be starting. " + : `Studio couldn't confirm ${unanswered === 1 ? 'it' : 'the last of them'} came back either, so it may still be ` + + 'starting. '; + return notBack + + (untouched > 0 ? `${leftAlone} not ${verb}, to keep the cluster available. ` : '') + + 'Check its status before you restart it again.'; +} diff --git a/src/integrations/api/isReadOperation.test.ts b/src/integrations/api/isReadOperation.test.ts index 9ce03821d..908069bbd 100644 --- a/src/integrations/api/isReadOperation.test.ts +++ b/src/integrations/api/isReadOperation.test.ts @@ -1,4 +1,4 @@ -import { isReadOperation } from '@/integrations/api/isReadOperation'; +import { isReadOperation, isReadRequest } from '@/integrations/api/isReadOperation'; import { describe, expect, it } from 'vitest'; describe('isReadOperation', () => { @@ -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); }); @@ -43,3 +45,32 @@ describe('isReadOperation', () => { }, ); }); + +describe('isReadRequest', () => { + it.each([ + ['a GET', { method: 'get' }], + ['an upper-case GET', { method: 'GET' }], + ['a read body', { method: 'post', data: { operation: 'describe_all' } }], + ['a read body under an upper-case POST', { method: 'POST', data: { operation: 'describe_all' } }], + ['a read body axios already serialized', { method: 'post', data: JSON.stringify({ operation: 'get_job' }) }], + ])('reads %s as a read', (_, config) => { + expect(isReadRequest(config)).toBe(true); + }); + + it.each([ + ['a write body', { method: 'post', data: { operation: 'insert' } }], + ['a serialized write body', { method: 'post', data: JSON.stringify({ operation: 'drop_table' }) }], + ['a body that is not JSON', { method: 'post', data: 'describe_all' }], + ['a JSON null body', { method: 'post', data: 'null' }], + ['no body', { method: 'post' }], + ['a DELETE', { method: 'delete' }], + ['a DELETE with a read body', { method: 'delete', data: { operation: 'describe_all' } }], + ['a PUT with a serialized read body', { method: 'put', data: JSON.stringify({ operation: 'get_job' }) }], + ['a PATCH with a read body', { method: 'patch', data: { operation: 'search_by_value' } }], + ['a read body with no method', { data: { operation: 'describe_all' } }], + ['a method that is not a string', { method: 42 }], + ['a read body under a method that is not a string', { method: 42, data: { operation: 'describe_all' } }], + ])('treats %s as able to change something', (_, config) => { + expect(isReadRequest(config)).toBe(false); + }); +}); diff --git a/src/integrations/api/isReadOperation.ts b/src/integrations/api/isReadOperation.ts index 84ff5c56b..17f0de160 100644 --- a/src/integrations/api/isReadOperation.ts +++ b/src/integrations/api/isReadOperation.ts @@ -1,12 +1,37 @@ /** * Reads, by Harper's operation naming: the restart gate holds only these (anything else is refused - * while the entity restarts), 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. + * 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. `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); } + +function operationOf(data: unknown): unknown { + if (typeof data !== 'string') { + return (data as { operation?: unknown } | undefined)?.operation; + } + // By the time a request has been sent, axios has serialized `config.data` in place; the operations + // console also sends raw text. + try { + return (JSON.parse(data) as { operation?: unknown } | null)?.operation; + } catch { + return undefined; + } +} + +/** + * Whether a request is safe to send late (the restart gate) or twice (the gateway retry); anything + * unrecognized counts as a write. Harper and central manager's proxy take an operation only from a + * POST body, so on any other method an `operation` field is just data. + */ +export function isReadRequest({ method, data }: { method?: unknown; data?: unknown }): boolean { + const verb = typeof method === 'string' ? method.toLowerCase() : undefined; + return verb === 'get' || (verb === 'post' && isReadOperation(operationOf(data))); +} diff --git a/src/integrations/api/retryGatewayErrors.integration.test.ts b/src/integrations/api/retryGatewayErrors.integration.test.ts new file mode 100644 index 000000000..c7c14b9eb --- /dev/null +++ b/src/integrations/api/retryGatewayErrors.integration.test.ts @@ -0,0 +1,101 @@ +/** @vitest-environment jsdom */ +import { getInstanceClient } from '@/config/getInstanceClient'; +import * as sleepModule from '@/lib/sleep'; +import { AxiosError, type InternalAxiosRequestConfig } from 'axios'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +vi.mock('@/lib/sleep', () => ({ + sleep: vi.fn().mockResolvedValue(true), +})); + +const OPERATIONS_URL = 'https://ins-123.example.com:9925/'; + +/** + * A target behind a gateway that answers `statuses` in turn, then 200. Axios applies + * `validateStatus` inside its own adapters, so this one rejects a non-2xx itself. + */ +function gateway(...statuses: number[]) { + const sent: Array<{ method?: string; body: unknown }> = []; + const client = getInstanceClient({ id: 'ins-123', operationsUrl: OPERATIONS_URL }); + client.defaults.adapter = async (config: InternalAxiosRequestConfig) => { + sent.push({ method: config.method, body: typeof config.data === 'string' ? JSON.parse(config.data) : config.data }); + const status = statuses.shift() ?? 200; + const response = { status, statusText: '', data: { status }, headers: {}, config }; + if (status >= 400) { + throw new AxiosError( + `Request failed with status code ${status}`, + AxiosError.ERR_BAD_RESPONSE, + config, + undefined, + response, + ); + } + return response; + }; + return { client, sent }; +} + +function statusOf(error: unknown): number | undefined { + return (error as AxiosError).response?.status; +} + +describe('the gateway retry on a real instance client', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it.each([502, 503, 504])('sends a write once and surfaces its %i', async (status) => { + const { client, sent } = gateway(status, status, status, status); + + const error = await client.post('/', { operation: 'insert', records: [{ name: 'a' }] }).catch((e: unknown) => e); + + expect(statusOf(error)).toBe(status); + expect(sent).toEqual([{ method: 'post', body: { operation: 'insert', records: [{ name: 'a' }] } }]); + expect(sleepModule.sleep).not.toHaveBeenCalled(); + }); + + it.each(['drop_table', 'deploy_component', 'set_configuration', 'restart', 'create_authentication_tokens'])( + 'never replays %s', + async (operation) => { + const { client, sent } = gateway(504); + + await expect(client.post('/', { operation })).rejects.toBeInstanceOf(AxiosError); + expect(sent).toHaveLength(1); + }, + ); + + it.each([502, 503, 504])('replays a read three times on a %i, 5s/10s/20s apart, as before', async (status) => { + const { client, sent } = gateway(status, status, status, status); + + const error = await client.post('/', { operation: 'describe_all' }).catch((e: unknown) => e); + + expect(statusOf(error)).toBe(status); + expect(sent).toHaveLength(4); + expect(sent.every(({ body }) => (body as { operation: string }).operation === 'describe_all')).toBe(true); + expect(vi.mocked(sleepModule.sleep).mock.calls).toEqual([[5_000], [10_000], [20_000]]); + }); + + it('returns the answer to a read that recovers on its replay', async () => { + const { client, sent } = gateway(503); + + const { data } = await client.post('/', { operation: 'get_job', id: 'job-1' }); + + expect(data).toEqual({ status: 200 }); + expect(sent).toHaveLength(2); + }); + + it('replays a GET', async () => { + const { client, sent } = gateway(502, 502); + + await expect(client.get('/health')).resolves.toMatchObject({ status: 200 }); + expect(sent).toHaveLength(3); + }); + + it('sends a read that opted out once', async () => { + const { client, sent } = gateway(503); + + await expect(client.post('/', { operation: 'describe_all' }, { skipGatewayRetry: true })).rejects + .toBeInstanceOf(AxiosError); + expect(sent).toHaveLength(1); + }); +}); diff --git a/src/integrations/api/retryGatewayErrors.test.ts b/src/integrations/api/retryGatewayErrors.test.ts index 3d1a8c827..f2f7b8fc5 100644 --- a/src/integrations/api/retryGatewayErrors.test.ts +++ b/src/integrations/api/retryGatewayErrors.test.ts @@ -7,6 +7,9 @@ vi.mock('@/lib/sleep', () => ({ sleep: vi.fn().mockResolvedValue(true), })); +/** A read as the interceptor sees it: axios has serialized `config.data` by the time it fails. */ +const READ = { method: 'post', data: JSON.stringify({ operation: 'describe_all' }) }; + describe('curryRetryGatewayErrors', () => { let client: Pick; @@ -32,9 +35,9 @@ describe('curryRetryGatewayErrors', () => { expect(client.request).not.toHaveBeenCalled(); }); - it('rejects and does not retry for non-gateway status codes', async () => { + it('rejects and does not retry a read for non-gateway status codes', async () => { const handler = curryRetryGatewayErrors(client); - const config: AxiosRequestConfig & { __retryCount?: number } = {}; + const config: AxiosRequestConfig & { __retryCount?: number } = { ...READ }; const err = { response: { status: 500 }, config }; await expect(handler(err)).rejects.toBe(err); @@ -44,9 +47,9 @@ describe('curryRetryGatewayErrors', () => { expect(config.__retryCount).toBeUndefined(); }); - it('retries on 502 and increments __retryCount with initial delay on first retry', async () => { + it('retries a read on 502 and increments __retryCount with initial delay on first retry', async () => { const handler = curryRetryGatewayErrors(client); - const config: AxiosRequestConfig & { __retryCount?: number } = {}; + const config: AxiosRequestConfig & { __retryCount?: number } = { ...READ }; const err = { response: { status: 502 }, config }; const result = await handler(err); @@ -59,9 +62,9 @@ describe('curryRetryGatewayErrors', () => { expect(result).toEqual({ ok: true }); }); - it('retries on 503 with exponential backoff: more backoff on second retry', async () => { + it('retries a read on 503 with exponential backoff: more backoff on second retry', async () => { const handler = curryRetryGatewayErrors(client); - const config: AxiosRequestConfig & { __retryCount?: number } = { __retryCount: 1 }; + const config: AxiosRequestConfig & { __retryCount?: number } = { ...READ, __retryCount: 1 }; const err = { response: { status: 503 }, config }; await handler(err); @@ -72,9 +75,9 @@ describe('curryRetryGatewayErrors', () => { expect(client.request).toHaveBeenCalledTimes(1); }); - it('retries on 504 with the largest backoff on third retry', async () => { + it('retries a read on 504 with the largest backoff on third retry', async () => { const handler = curryRetryGatewayErrors(client); - const config: AxiosRequestConfig & { __retryCount?: number } = { __retryCount: 2 }; + const config: AxiosRequestConfig & { __retryCount?: number } = { ...READ, __retryCount: 2 }; const err = { response: { status: 504 }, config }; await handler(err); @@ -85,9 +88,37 @@ describe('curryRetryGatewayErrors', () => { expect(client.request).toHaveBeenCalledTimes(1); }); - it.each([502, 503, 504])('never replays a %i for a request that opted out', async (status) => { + it.each([ + ['an object body', { operation: 'insert', records: [] }], + ['a serialized body', JSON.stringify({ operation: 'drop_table', table: 'dog' })], + ['token minting', JSON.stringify({ operation: 'create_authentication_tokens' })], + ['a body that is not JSON', 'operation=describe_all'], + ['no body', undefined], + ])('never replays a write on any gateway status: %s', async (_, data) => { + const handler = curryRetryGatewayErrors(client); + for (const status of [502, 503, 504]) { + const config: AxiosRequestConfig & { __retryCount?: number } = { method: 'post', data }; + const err = { response: { status }, config }; + + await expect(handler(err)).rejects.toBe(err); + expect(config.__retryCount).toBeUndefined(); + } + expect(sleepModule.sleep).not.toHaveBeenCalled(); + expect(client.request).not.toHaveBeenCalled(); + }); + + it('replays a GET, which carries no operation', async () => { + const handler = curryRetryGatewayErrors(client); + const config: AxiosRequestConfig & { __retryCount?: number } = { method: 'get' }; + + await handler({ response: { status: 503 }, config }); + + expect(client.request).toHaveBeenCalledWith(config); + }); + + it.each([502, 503, 504])('never replays a %i for a read that opted out', async (status) => { const handler = curryRetryGatewayErrors(client); - const config: AxiosRequestConfig & { __retryCount?: number } = { skipGatewayRetry: true }; + const config: AxiosRequestConfig & { __retryCount?: number } = { ...READ, skipGatewayRetry: true }; const err = { response: { status }, config }; await expect(handler(err)).rejects.toBe(err); @@ -98,7 +129,7 @@ describe('curryRetryGatewayErrors', () => { it('stops retrying when max retries reached (>= 3)', async () => { const handler = curryRetryGatewayErrors(client); - const config: AxiosRequestConfig & { __retryCount?: number } = { __retryCount: 3 }; + const config: AxiosRequestConfig & { __retryCount?: number } = { ...READ, __retryCount: 3 }; const err = { response: { status: 502 }, config }; await expect(handler(err)).rejects.toBe(err); diff --git a/src/integrations/api/retryGatewayErrors.ts b/src/integrations/api/retryGatewayErrors.ts index 306c0e8d0..766c2838d 100644 --- a/src/integrations/api/retryGatewayErrors.ts +++ b/src/integrations/api/retryGatewayErrors.ts @@ -1,16 +1,23 @@ +import { isReadRequest } from '@/integrations/api/isReadOperation'; import { sleep } from '@/lib/sleep'; import { AxiosInstance } from 'axios'; declare module 'axios' { interface AxiosRequestConfig { /** - * Send at most once. A 502/504 can arrive after Harper has applied the operation, so a request - * whose idempotence is unknown (the operations console's freeform body) must not be replayed. + * 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. */ skipGatewayRetry?: boolean; } } +/** + * Replays a 502/503/504 up to three times, for reads only. None of the three means the request went + * unapplied — a proxy answers for an upstream that may already have it, and Harper itself answers 503 + * when the worker running an operation exits or overruns — so a write is sent once and its first + * answer is final. + */ export function curryRetryGatewayErrors(client: Pick) { // eslint-disable-next-line @typescript-eslint/no-explicit-any return async (error: any) => { @@ -20,7 +27,7 @@ export function curryRetryGatewayErrors(client: Pick) return Promise.reject(error); } - const shouldRetry = status === 502 || status === 503 || status === 504; + const shouldRetry = (status === 502 || status === 503 || status === 504) && isReadRequest(config); if (!shouldRetry) { return Promise.reject(error); } diff --git a/src/integrations/api/sse/resolveInstanceConnection.ts b/src/integrations/api/sse/resolveInstanceConnection.ts index 44bce0e9b..6ccadbd76 100644 --- a/src/integrations/api/sse/resolveInstanceConnection.ts +++ b/src/integrations/api/sse/resolveInstanceConnection.ts @@ -32,8 +32,9 @@ export interface ResolvedInstanceConnection { * connection/auth resolution in `src/config/getInstanceClient.ts` one-way: `getInstanceClient` * is deliberately left untouched so its battle-tested axios path — including the * `rejectReplicationFailures` / gateway-retry interceptors that this fetch path intentionally - * does NOT carry — keeps working for every non-streaming caller. Callers recover gateway/ - * cold-start resilience by falling back to the axios path when the stream can't be established. + * does NOT carry — keeps working for every non-streaming caller. Callers fall back to the axios + * path when the stream can't be established; a read regains its gateway retry there, a deploy does + * not, because that retry replays reads only. * * Fabric Connect now resolves to a direct Bearer (operation token) connection when it can * (see authStore.establishFabricConnectAuth); we prefer that so SSE streams straight from diff --git a/src/integrations/api/unansweredWrite.integration.test.tsx b/src/integrations/api/unansweredWrite.integration.test.tsx new file mode 100644 index 000000000..27042f04e --- /dev/null +++ b/src/integrations/api/unansweredWrite.integration.test.tsx @@ -0,0 +1,117 @@ +/** @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'; + +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..d117b5eae --- /dev/null +++ b/src/integrations/api/unansweredWrite.ts @@ -0,0 +1,42 @@ +import { isReadRequest } from '@/integrations/api/isReadOperation'; +import { pluralize } from '@/lib/pluralize'; +import { AxiosError, isAxiosError } from 'axios'; + +export const OUTCOME_UNKNOWN_TITLE = 'Outcome unknown'; + +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.'; + +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]); + +/** 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; + } + const status = rawErr.response?.status; + 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); +} diff --git a/src/lib/restart/restartGate.ts b/src/lib/restart/restartGate.ts index 81a7fbcb6..b9a2537cb 100644 --- a/src/lib/restart/restartGate.ts +++ b/src/lib/restart/restartGate.ts @@ -1,4 +1,4 @@ -import { isReadOperation } from '@/integrations/api/isReadOperation'; +import { isReadRequest } from '@/integrations/api/isReadOperation'; import { getRestartState, holdsRequests, isRestarting, waitUntilReachable } from '@/lib/restart/restartTracker'; import { AxiosError, type AxiosInstance, CanceledError, type InternalAxiosRequestConfig } from 'axios'; @@ -15,23 +15,6 @@ declare module 'axios' { export const RESTARTING_ERROR_CODE = 'ERR_TARGET_RESTARTING'; -function operationOf(data: unknown): unknown { - if (typeof data !== 'string') { - return (data as { operation?: unknown } | undefined)?.operation; - } - // A gateway retry re-sends the config axios already serialized; the operations console sends raw text. - try { - return (JSON.parse(data) as { operation?: unknown } | null)?.operation; - } catch { - return undefined; - } -} - -function isSafeToDelay(config: InternalAxiosRequestConfig): boolean { - if (config.method?.toLowerCase() === 'get') { return true; } - return isReadOperation(operationOf(config.data)); -} - /** * While the tracker says this kind of client cannot reach `entityId`, hold its reads and send them * once it can — a held read is a query still fetching, where a failed one is an error state, a retry @@ -55,7 +38,7 @@ export function installRestartGate( ) { client.interceptors.request.use( (config: InternalAxiosRequestConfig) => { - if (!isSafeToDelay(config)) { + if (!isReadRequest(config)) { const target = entityId.startsWith('clu-') ? 'This cluster' : 'This instance'; throw new AxiosError( `${target} is restarting. Try again once it's back online.`, diff --git a/src/react-query/queryClient.test.ts b/src/react-query/queryClient.test.ts index 26c9af0ce..63503afc5 100644 --- a/src/react-query/queryClient.test.ts +++ b/src/react-query/queryClient.test.ts @@ -1,9 +1,15 @@ +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'; -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 +374,82 @@ 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); + }); +}); + +describe('a restart Harper accepted whose target was never confirmed back', () => { + const unconfirmed = () => + new RestartUnconfirmedError( + new AxiosError('timeout of 3000ms exceeded', AxiosError.ECONNABORTED, { + method: 'post', + data: { operation: 'user_info' }, + } as InternalAxiosRequestConfig), + ); + + it('is described as not back yet, never by the probe’s own failure', () => { + expect(describeError(unconfirmed())).toEqual({ + title: RESTART_UNCONFIRMED_TITLE, + description: RESTART_UNCONFIRMED_MESSAGE, + message: RESTART_UNCONFIRMED_MESSAGE, + }); + }); + + it('tells a mutation’s caller not to add a "failed" toast of its own', () => { + expect(shouldToastOwnFailure(unconfirmed())).toBe(false); + }); +}); diff --git a/src/react-query/queryClient.ts b/src/react-query/queryClient.ts index 581fa4a4e..8185d0157 100644 --- a/src/react-query/queryClient.ts +++ b/src/react-query/queryClient.ts @@ -1,3 +1,9 @@ +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'; import { onRestartSettled } from '@/lib/restart/restartTracker'; @@ -15,8 +21,22 @@ 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. + * So does a restart Harper accepted whose target was never confirmed back ({@link RestartUnconfirmedError}). */ export function describeError(rawErr: unknown): { title: string; description: string; message: string } { + if (isUnansweredWrite(rawErr)) { + return { title: OUTCOME_UNKNOWN_TITLE, description: UNANSWERED_WRITE_MESSAGE, message: UNANSWERED_WRITE_MESSAGE }; + } + if (rawErr instanceof RestartUnconfirmedError) { + return { + title: RESTART_UNCONFIRMED_TITLE, + description: RESTART_UNCONFIRMED_MESSAGE, + message: RESTART_UNCONFIRMED_MESSAGE, + }; + } let errorTitle = 'Error'; let errorMsg = 'We had some trouble!'; let splitTitleFromMsg = true; @@ -62,6 +82,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 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) && !(rawErr instanceof RestartUnconfirmedError); +} + export function errorHandler(rawErr: unknown) { console.error(rawErr); const { title: errorTitle, description: errorMsg } = describeError(rawErr); @@ -75,8 +104,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',