From ce4ad1530f269ce5e603525267c27a9e4fba909b Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Sat, 10 Oct 2026 22:55:20 -0400 Subject: [PATCH 01/21] fix(clusters): don't poll get_status on an instance older than Harper 4.6 get_status and set_status first shipped in Harper 4.6; an older instance answers the operation with a 400. The instance page's nav bar already floored its get_status poll at 4.6.0, but the cluster instances list (one 10s poll per row) and the row menu sent it to every instance regardless of version. Both now read the instance's Harper version from the cluster record through one shared supportsGetStatus check, and a pre-4.6 row shows a muted dot explaining that availability needs Harper 4.6 or later instead of a spinner. Refs #1569 Co-Authored-By: Claude Opus 5.5 --- .../cluster/InstanceActionsMenu.test.tsx | 37 ++++++++++++++++--- .../cluster/InstanceStatusCell.test.tsx | 35 +++++++++++++++--- src/features/cluster/InstanceStatusCell.tsx | 20 +++++++--- src/features/cluster/useInstanceMenuItems.tsx | 11 +++++- src/features/instance/InstanceNavBar.tsx | 4 +- .../api/instance/status/getStatus.ts | 9 +++++ 6 files changed, 95 insertions(+), 21 deletions(-) diff --git a/src/features/cluster/InstanceActionsMenu.test.tsx b/src/features/cluster/InstanceActionsMenu.test.tsx index 7aee1f7ae..45824b131 100644 --- a/src/features/cluster/InstanceActionsMenu.test.tsx +++ b/src/features/cluster/InstanceActionsMenu.test.tsx @@ -7,7 +7,7 @@ import { afterEach, beforeAll, describe, expect, it, vi } from 'vitest'; import { InstanceActionsMenu } from './InstanceActionsMenu'; const ORG = 'org-a'; -const state = vi.hoisted(() => ({ user: null as unknown, run: vi.fn() })); +const state = vi.hoisted(() => ({ user: null as unknown, run: vi.fn(), statusEnabled: [] as (boolean | undefined)[] })); vi.mock('@tanstack/react-router', () => ({ useParams: () => ({ organizationId: 'org-a', clusterId: 'clu-b' }), Link: ({ children, to }: { children: ReactNode; to: string }) => {children}, @@ -25,8 +25,12 @@ vi.mock('@/config/useInstanceClient', () => ({ useInstanceClient: () => ({}), useInstanceClientIdParams: () => ({}), })); -vi.mock('@/integrations/api/instance/status/getStatus', () => ({ - getStatusQueryOptions: () => ({ queryKey: ['status'], queryFn: () => null, enabled: false }), +vi.mock('@/integrations/api/instance/status/getStatus', async importOriginal => ({ + ...await importOriginal(), + getStatusQueryOptions: (_params: unknown, enabled?: boolean) => { + state.statusEnabled.push(enabled); + return { queryKey: ['status'], queryFn: () => null, enabled: false }; + }, getSystemStatusById: () => undefined, })); vi.mock('@/integrations/api/instance/status/setStatus', () => ({ useSetStatus: () => ({ mutate: vi.fn() }) })); @@ -46,9 +50,16 @@ beforeAll(() => { afterEach(() => { cleanup(); state.run.mockClear(); + state.statusEnabled = []; }); -const instance = { id: 'ins-c', name: 'primary', status: 'RUNNING', instanceFqdn: 'ins-c.example.com' } as Instance; +const instance = { + id: 'ins-c', + name: 'primary', + status: 'RUNNING', + instanceFqdn: 'ins-c.example.com', + version: '4.7.3', +} as Instance; const member = (role: string) => ({ fabricRole: 'least_privileged', @@ -65,10 +76,10 @@ const member = (role: string) => }, }) as unknown as User; -function openMenu() { +function openMenu(target: Instance = instance) { render( - + , ); fireEvent.pointerDown(screen.getByRole('button', { name: 'Instance options' }), { button: 0, ctrlKey: false }); @@ -94,3 +105,17 @@ describe('InstanceActionsMenu container ops', () => { expect(state.run).toHaveBeenCalledWith('stop'); }); }); + +describe('InstanceActionsMenu availability status', () => { + it('asks for it while the menu is open', () => { + state.user = member('admin'); + openMenu(); + expect(state.statusEnabled.at(-1)).toBe(true); + }); + + it('never asks a Harper older than 4.6, which has no get_status', () => { + state.user = member('admin'); + openMenu({ ...instance, version: '4.5.12' }); + expect(state.statusEnabled.at(-1)).toBe(false); + }); +}); diff --git a/src/features/cluster/InstanceStatusCell.test.tsx b/src/features/cluster/InstanceStatusCell.test.tsx index ff484e8d1..6e637b83c 100644 --- a/src/features/cluster/InstanceStatusCell.test.tsx +++ b/src/features/cluster/InstanceStatusCell.test.tsx @@ -9,7 +9,8 @@ const state = vi.hoisted(() => ({ enabled: [] as (boolean | undefined)[] })); vi.mock('@/hooks/usePermissions', () => ({ useOrganizationClusterInstancePermissions: () => ({ update: true }) })); vi.mock('@/config/useInstanceClient', () => ({ useInstanceClientIdParams: () => ({}) })); vi.mock('@/integrations/api/instance/status/setStatus', () => ({ useSetStatus: () => ({ mutate: vi.fn() }) })); -vi.mock('@/integrations/api/instance/status/getStatus', () => ({ +vi.mock('@/integrations/api/instance/status/getStatus', async importOriginal => ({ + ...await importOriginal(), getStatusQueryOptions: (_params: unknown, enabled?: boolean) => { state.enabled.push(enabled); return { queryKey: ['status'], queryFn: () => null, enabled: false }; @@ -26,17 +27,20 @@ afterEach(() => { vi.useRealTimers(); }); -async function renderCell(status: string) { +async function renderCell(status: string, version: string | undefined = '4.7.3') { render( - + , ); await act(() => vi.advanceTimersByTimeAsync(1_000)); } -async function pollEnabledFor(status: string) { - await renderCell(status); +async function pollEnabledFor(status: string, version?: string) { + await renderCell(status, version); return state.enabled.at(-1); } @@ -57,4 +61,25 @@ describe('InstanceStatusCell get_status poll', () => { expect(screen.getByLabelText('Status unavailable')).toBeTruthy(); expect(screen.queryByRole('button')).toBeNull(); }); + + it.each(['4.6.0', '4.7.3', '5.0.2'])('polls a Harper %s instance', async version => { + expect(await pollEnabledFor('RUNNING', version)).toBe(true); + }); + + it.each(['4.4.24', '4.5.12'])( + 'does not poll a Harper %s instance, which rejects get_status with a 400 on every tick', + async version => { + expect(await pollEnabledFor('RUNNING', version)).toBe(false); + }, + ); + + it('does not poll an instance whose Harper version central manager has not reported', async () => { + expect(await pollEnabledFor('RUNNING', '')).toBe(false); + }); + + it('says why a pre-4.6 instance has no availability, instead of spinning or offering rotation', async () => { + await renderCell('RUNNING', '4.5.12'); + expect(screen.getByLabelText('Status needs Harper 4.6 or later')).toBeTruthy(); + expect(screen.queryByRole('button')).toBeNull(); + }); }); diff --git a/src/features/cluster/InstanceStatusCell.tsx b/src/features/cluster/InstanceStatusCell.tsx index 109aa7b36..1156203e2 100644 --- a/src/features/cluster/InstanceStatusCell.tsx +++ b/src/features/cluster/InstanceStatusCell.tsx @@ -5,7 +5,11 @@ import { isOperationsProxyRefused, isStoppedOrTransitioning } from '@/components import { useInstanceClientIdParams } from '@/config/useInstanceClient'; import { useOrganizationClusterInstancePermissions } from '@/hooks/usePermissions'; import { Instance } from '@/integrations/api/api.patch'; -import { getStatusQueryOptions, getSystemStatusById } from '@/integrations/api/instance/status/getStatus'; +import { + getStatusQueryOptions, + getSystemStatusById, + supportsGetStatus, +} from '@/integrations/api/instance/status/getStatus'; import { useSetStatus } from '@/integrations/api/instance/status/setStatus'; import { getOperationsUrlForInstance } from '@/lib/urls/getOperationsUrlForInstance'; import { useQuery } from '@tanstack/react-query'; @@ -33,7 +37,8 @@ export function InstanceStatusCell( }, [index]); const notProxied = isOperationsProxyRefused(instance.status); - const statusPollEnabled = ready && canManage && !notProxied; + const statusSupported = supportsGetStatus(instance.version); + const statusPollEnabled = ready && canManage && !notProxied && statusSupported; const { data: statusResponse, isLoading, isFetching } = useQuery( getStatusQueryOptions(instanceParams, statusPollEnabled), ); @@ -46,10 +51,13 @@ export function InstanceStatusCell( return null; } - // Availability and rotation are N/A while central manager refuses the instance's operations. Show - // a muted dot rather than a stale green "Available" (cached from before) or an endless spinner. - if (notProxied) { - const label = isStoppedOrTransitioning(instance.status) ? 'Not running' : 'Status unavailable'; + // Availability and rotation are N/A while central manager refuses the instance's operations, or on + // a Harper too old to have them. Show a muted dot rather than a stale green "Available" (cached + // from before) or an endless spinner. + if (notProxied || !statusSupported) { + const label = notProxied + ? isStoppedOrTransitioning(instance.status) ? 'Not running' : 'Status unavailable' + : 'Status needs Harper 4.6 or later'; return (
diff --git a/src/features/cluster/useInstanceMenuItems.tsx b/src/features/cluster/useInstanceMenuItems.tsx index 18c51c4e8..2c1161c39 100644 --- a/src/features/cluster/useInstanceMenuItems.tsx +++ b/src/features/cluster/useInstanceMenuItems.tsx @@ -11,7 +11,11 @@ import { useCopyToClipboard } from '@/hooks/useCopyToClipboard'; import { useInstanceContainerOps } from '@/hooks/useInstanceContainerOps'; import { useContainerOpsPermission, useOrganizationClusterInstancePermissions } from '@/hooks/usePermissions'; import { Instance } from '@/integrations/api/api.patch'; -import { getStatusQueryOptions, getSystemStatusById } from '@/integrations/api/instance/status/getStatus'; +import { + getStatusQueryOptions, + getSystemStatusById, + supportsGetStatus, +} from '@/integrations/api/instance/status/getStatus'; import { useSetStatus } from '@/integrations/api/instance/status/setStatus'; import { excludeFalsy } from '@/lib/arrays/excludeFalsy'; import { buildAbsoluteLinkToPage } from '@/lib/urls/buildAbsoluteLinkToPage'; @@ -60,7 +64,10 @@ export function useInstanceMenuItems( const statusParams = useInstanceClientIdParams({ operationsUrl, instanceId: instance.id, forceFabricConnect: true }); const { data: statusResponse } = useQuery( - getStatusQueryOptions(statusParams, enabled && canManage && !isOperationsProxyRefused(instance.status)), + getStatusQueryOptions( + statusParams, + enabled && canManage && !isOperationsProxyRefused(instance.status) && supportsGetStatus(instance.version), + ), ); const systemStatus = getSystemStatusById(statusResponse, 'availability') || 'Unknown'; const isAvailable = systemStatus === 'Available'; diff --git a/src/features/instance/InstanceNavBar.tsx b/src/features/instance/InstanceNavBar.tsx index da9d2bead..2bf185988 100644 --- a/src/features/instance/InstanceNavBar.tsx +++ b/src/features/instance/InstanceNavBar.tsx @@ -12,7 +12,7 @@ import { Tooltip, TooltipContent, TooltipTrigger } from '@/components/ui/tooltip import { useInstanceClientIdParams } from '@/config/useInstanceClient'; import { useInstanceManagePermission } from '@/hooks/usePermissions'; import { RegistrationInfoResponse } from '@/integrations/api/instance/status/getRegistrationInfo'; -import { getStatusQueryOptions } from '@/integrations/api/instance/status/getStatus'; +import { getStatusQueryOptions, supportsGetStatus } from '@/integrations/api/instance/status/getStatus'; import { excludeFalsy } from '@/lib/arrays/excludeFalsy'; import { wasAReleasedBeforeB } from '@/lib/string/wasAReleasedBeforeB'; import { buildAbsoluteLinkToPage } from '@/lib/urls/buildAbsoluteLinkToPage'; @@ -37,7 +37,7 @@ export function InstanceNavBar() { const params = useParams({ strict: false }); const { version }: RegistrationInfoResponse = useLoaderData({ strict: false }); - const statusAvailable = wasAReleasedBeforeB('4.6.0', version) && canManage; + const statusAvailable = supportsGetStatus(version) && canManage; const apisAvailable = wasAReleasedBeforeB('4.7.0-beta.7', version); const instanceParams = useInstanceClientIdParams(); diff --git a/src/integrations/api/instance/status/getStatus.ts b/src/integrations/api/instance/status/getStatus.ts index 5ea5bda6c..a6ab53b48 100644 --- a/src/integrations/api/instance/status/getStatus.ts +++ b/src/integrations/api/instance/status/getStatus.ts @@ -1,4 +1,5 @@ import { InstanceClientIdConfig } from '@/config/instanceClientConfig'; +import { wasAReleasedBeforeB } from '@/lib/string/wasAReleasedBeforeB'; import { pollUnlessForbidden, retryUnlessRejected } from '@/react-query/pollUnlessForbidden'; import { queryOptions } from '@tanstack/react-query'; @@ -38,6 +39,14 @@ interface StatusResponse { [key: string]: unknown; } +/** `get_status` / `set_status` first shipped in Harper 4.6; an older instance rejects the + * operation with a 400 on every poll. */ +const GET_STATUS_MIN_VERSION = '4.6.0'; + +export function supportsGetStatus(harperVersion: string | undefined): boolean { + return !!harperVersion && wasAReleasedBeforeB(GET_STATUS_MIN_VERSION, harperVersion); +} + export function getStatusQueryOptions({ entityId, instanceClient }: InstanceClientIdConfig, enabled?: boolean) { return queryOptions({ queryKey: ['get_status', entityId] as const, From 61d770d5a42d58dd2bbdc8c10e0fc5f1663cb286 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Sat, 10 Oct 2026 23:09:32 -0400 Subject: [PATCH 02/21] fix(clusters): never offer rotation on a pre-4.6 instance, nor blame an unreported version Review follow-up: the row menu still derived its rotation actions from any cached availability, so it could offer set_status to an instance too old to have it; it now requires supportsGetStatus too. A row whose Harper version central manager has not reported says "Status unavailable" rather than claiming it needs an upgrade. Refs #1569 Co-Authored-By: Claude Opus 5.5 --- .../cluster/InstanceActionsMenu.test.tsx | 24 +++++++++++++++++-- .../cluster/InstanceStatusCell.test.tsx | 1 + src/features/cluster/InstanceStatusCell.tsx | 9 ++++--- src/features/cluster/useInstanceMenuItems.tsx | 5 ++-- 4 files changed, 32 insertions(+), 7 deletions(-) diff --git a/src/features/cluster/InstanceActionsMenu.test.tsx b/src/features/cluster/InstanceActionsMenu.test.tsx index 45824b131..1314e6310 100644 --- a/src/features/cluster/InstanceActionsMenu.test.tsx +++ b/src/features/cluster/InstanceActionsMenu.test.tsx @@ -7,7 +7,12 @@ import { afterEach, beforeAll, describe, expect, it, vi } from 'vitest'; import { InstanceActionsMenu } from './InstanceActionsMenu'; const ORG = 'org-a'; -const state = vi.hoisted(() => ({ user: null as unknown, run: vi.fn(), statusEnabled: [] as (boolean | undefined)[] })); +const state = vi.hoisted(() => ({ + user: null as unknown, + run: vi.fn(), + statusEnabled: [] as (boolean | undefined)[], + availability: undefined as string | undefined, +})); vi.mock('@tanstack/react-router', () => ({ useParams: () => ({ organizationId: 'org-a', clusterId: 'clu-b' }), Link: ({ children, to }: { children: ReactNode; to: string }) => {children}, @@ -31,7 +36,7 @@ vi.mock('@/integrations/api/instance/status/getStatus', async importOriginal => state.statusEnabled.push(enabled); return { queryKey: ['status'], queryFn: () => null, enabled: false }; }, - getSystemStatusById: () => undefined, + getSystemStatusById: () => state.availability, })); vi.mock('@/integrations/api/instance/status/setStatus', () => ({ useSetStatus: () => ({ mutate: vi.fn() }) })); vi.mock('@/hooks/useInstanceContainerOps', () => ({ useInstanceContainerOps: () => ({ run: state.run }) })); @@ -51,6 +56,7 @@ afterEach(() => { cleanup(); state.run.mockClear(); state.statusEnabled = []; + state.availability = undefined; }); const instance = { @@ -118,4 +124,18 @@ describe('InstanceActionsMenu availability status', () => { openMenu({ ...instance, version: '4.5.12' }); expect(state.statusEnabled.at(-1)).toBe(false); }); + + it('offers rotation from a known availability', () => { + state.user = member('admin'); + state.availability = 'Available'; + openMenu(); + expect(screen.getByRole('menuitem', { name: 'Bring out of rotation' })).toBeTruthy(); + }); + + it('never offers rotation on a Harper older than 4.6, even from a cached availability', () => { + state.user = member('admin'); + state.availability = 'Available'; + openMenu({ ...instance, version: '4.5.12' }); + expect(screen.queryByRole('menuitem', { name: 'Bring out of rotation' })).toBeNull(); + }); }); diff --git a/src/features/cluster/InstanceStatusCell.test.tsx b/src/features/cluster/InstanceStatusCell.test.tsx index 6e637b83c..a2a2a9e5d 100644 --- a/src/features/cluster/InstanceStatusCell.test.tsx +++ b/src/features/cluster/InstanceStatusCell.test.tsx @@ -75,6 +75,7 @@ describe('InstanceStatusCell get_status poll', () => { it('does not poll an instance whose Harper version central manager has not reported', async () => { expect(await pollEnabledFor('RUNNING', '')).toBe(false); + expect(screen.getByLabelText('Status unavailable')).toBeTruthy(); }); it('says why a pre-4.6 instance has no availability, instead of spinning or offering rotation', async () => { diff --git a/src/features/cluster/InstanceStatusCell.tsx b/src/features/cluster/InstanceStatusCell.tsx index 1156203e2..305391dd5 100644 --- a/src/features/cluster/InstanceStatusCell.tsx +++ b/src/features/cluster/InstanceStatusCell.tsx @@ -55,9 +55,12 @@ export function InstanceStatusCell( // a Harper too old to have them. Show a muted dot rather than a stale green "Available" (cached // from before) or an endless spinner. if (notProxied || !statusSupported) { - const label = notProxied - ? isStoppedOrTransitioning(instance.status) ? 'Not running' : 'Status unavailable' - : 'Status needs Harper 4.6 or later'; + let label = 'Status unavailable'; + if (notProxied && isStoppedOrTransitioning(instance.status)) { + label = 'Not running'; + } else if (!notProxied && instance.version) { + label = 'Status needs Harper 4.6 or later'; + } return (
diff --git a/src/features/cluster/useInstanceMenuItems.tsx b/src/features/cluster/useInstanceMenuItems.tsx index 2c1161c39..294fdfe5e 100644 --- a/src/features/cluster/useInstanceMenuItems.tsx +++ b/src/features/cluster/useInstanceMenuItems.tsx @@ -63,10 +63,11 @@ export function useInstanceMenuItems( const isFabricConnect = authStore.checkForFabricConnect(instance.id); const statusParams = useInstanceClientIdParams({ operationsUrl, instanceId: instance.id, forceFabricConnect: true }); + const statusSupported = supportsGetStatus(instance.version); const { data: statusResponse } = useQuery( getStatusQueryOptions( statusParams, - enabled && canManage && !isOperationsProxyRefused(instance.status) && supportsGetStatus(instance.version), + enabled && canManage && !isOperationsProxyRefused(instance.status) && statusSupported, ), ); const systemStatus = getSystemStatusById(statusResponse, 'availability') || 'Unknown'; @@ -94,7 +95,7 @@ export function useInstanceMenuItems( const isDirectlyLoggedIn = !!instanceUser && !isFabricConnect; const hasAuth = isReady; const hasCopy = !!fqdn; - const hasRotation = canManage && isReady && (isAvailable || isUnavailable); + const hasRotation = canManage && isReady && statusSupported && (isAvailable || isUnavailable); // Container lifecycle ops (stop/start/restart) — distinct from the proxied Harper "restart". // Hidden mid-transition (the instances poll reveals the resting state and the actions reappear). From 71d5aa00f6d9dff471050c3d3ee9ae1d38bbedf2 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Sat, 10 Oct 2026 23:25:15 -0400 Subject: [PATCH 03/21] fix(clusters): only a version that parses as pre-4.6 is told it needs Harper 4.6 Review follow-up: an unrecognised version string still got the upgrade label. predatesGetStatus is true only for a version that parses and is older than the floor; anything else reads "Status unavailable". Refs #1569 Co-Authored-By: Claude Opus 5.5 --- .../cluster/InstanceStatusCell.test.tsx | 11 ++++++---- src/features/cluster/InstanceStatusCell.tsx | 3 ++- .../api/instance/status/getStatus.test.ts | 21 ++++++++++++++++++- .../api/instance/status/getStatus.ts | 6 ++++++ 4 files changed, 35 insertions(+), 6 deletions(-) diff --git a/src/features/cluster/InstanceStatusCell.test.tsx b/src/features/cluster/InstanceStatusCell.test.tsx index a2a2a9e5d..feeb95b90 100644 --- a/src/features/cluster/InstanceStatusCell.test.tsx +++ b/src/features/cluster/InstanceStatusCell.test.tsx @@ -73,10 +73,13 @@ describe('InstanceStatusCell get_status poll', () => { }, ); - it('does not poll an instance whose Harper version central manager has not reported', async () => { - expect(await pollEnabledFor('RUNNING', '')).toBe(false); - expect(screen.getByLabelText('Status unavailable')).toBeTruthy(); - }); + it.each(['', 'custom-build'])( + 'does not poll an instance whose Harper version is %j, nor blame the version', + async version => { + expect(await pollEnabledFor('RUNNING', version)).toBe(false); + expect(screen.getByLabelText('Status unavailable')).toBeTruthy(); + }, + ); it('says why a pre-4.6 instance has no availability, instead of spinning or offering rotation', async () => { await renderCell('RUNNING', '4.5.12'); diff --git a/src/features/cluster/InstanceStatusCell.tsx b/src/features/cluster/InstanceStatusCell.tsx index 305391dd5..678896d2a 100644 --- a/src/features/cluster/InstanceStatusCell.tsx +++ b/src/features/cluster/InstanceStatusCell.tsx @@ -8,6 +8,7 @@ import { Instance } from '@/integrations/api/api.patch'; import { getStatusQueryOptions, getSystemStatusById, + predatesGetStatus, supportsGetStatus, } from '@/integrations/api/instance/status/getStatus'; import { useSetStatus } from '@/integrations/api/instance/status/setStatus'; @@ -58,7 +59,7 @@ export function InstanceStatusCell( let label = 'Status unavailable'; if (notProxied && isStoppedOrTransitioning(instance.status)) { label = 'Not running'; - } else if (!notProxied && instance.version) { + } else if (!notProxied && predatesGetStatus(instance.version)) { label = 'Status needs Harper 4.6 or later'; } return ( diff --git a/src/integrations/api/instance/status/getStatus.test.ts b/src/integrations/api/instance/status/getStatus.test.ts index 9ddc6a2b6..112b96173 100644 --- a/src/integrations/api/instance/status/getStatus.test.ts +++ b/src/integrations/api/instance/status/getStatus.test.ts @@ -1,7 +1,7 @@ import type { AxiosInstance } from 'axios'; import { AxiosError } from 'axios'; import { describe, expect, it } from 'vitest'; -import { getStatusQueryOptions, getSystemStatusById } from './getStatus'; +import { getStatusQueryOptions, getSystemStatusById, predatesGetStatus, supportsGetStatus } from './getStatus'; /** The subset of `Query` the `refetchInterval` callback reads. */ type ErrorStateQuery = { state: { error: unknown } }; @@ -100,3 +100,22 @@ describe('getSystemStatusById', () => { expect(getSystemStatusById(mockStatusResponse, 'availability')).toBeUndefined(); }); }); + +describe('get_status version floor', () => { + it('supports Harper 4.6.0 and later, and nothing unknown', () => { + for (const version of ['4.6.0', '4.7.3', '5.0.2']) { + expect(supportsGetStatus(version)).toBe(true); + } + for (const version of ['4.5.12', '', undefined, 'custom-build']) { + expect(supportsGetStatus(version)).toBe(false); + } + }); + + it('says a version predates get_status only when it parses and is older', () => { + expect(predatesGetStatus('4.5.12')).toBe(true); + expect(predatesGetStatus('4.6.0-beta.3')).toBe(true); + for (const version of ['4.6.0', '5.0.2', '', undefined, 'custom-build']) { + expect(predatesGetStatus(version)).toBe(false); + } + }); +}); diff --git a/src/integrations/api/instance/status/getStatus.ts b/src/integrations/api/instance/status/getStatus.ts index a6ab53b48..d95f3245e 100644 --- a/src/integrations/api/instance/status/getStatus.ts +++ b/src/integrations/api/instance/status/getStatus.ts @@ -47,6 +47,12 @@ export function supportsGetStatus(harperVersion: string | undefined): boolean { return !!harperVersion && wasAReleasedBeforeB(GET_STATUS_MIN_VERSION, harperVersion); } +/** Unlike `!supportsGetStatus`, false for a version that is missing or doesn't parse. */ +export function predatesGetStatus(harperVersion: string | undefined): boolean { + return !!harperVersion && !supportsGetStatus(harperVersion) + && wasAReleasedBeforeB(harperVersion, GET_STATUS_MIN_VERSION); +} + export function getStatusQueryOptions({ entityId, instanceClient }: InstanceClientIdConfig, enabled?: boolean) { return queryOptions({ queryKey: ['get_status', entityId] as const, From 9005e40f234a8618da0cc500eb3262e805a26c8e Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Sat, 10 Oct 2026 23:03:18 -0400 Subject: [PATCH 04/21] fix(clusters): only instances on screen poll their status on the instances list Each row of a cluster's instances list polled get_status every 10s for as long as the tab was visible, so the list's cost grew with the cluster rather than with what anyone could see (one session sent 5,271 operation requests in a day). A row now polls only while it is within 200px of the viewport, through a small useIsOnScreen hook over IntersectionObserver: a visible row keeps its 10s cadence, a row scrolled out of view stops, and a row scrolling back in fetches at once if its status is stale. A row that has never loaded shows the spinner rather than an unknown dot. Browsers without IntersectionObserver poll every row as before. Refs #1603 Co-Authored-By: Claude Opus 5.5 --- .../InstanceStatusCell.polling.test.tsx | 92 +++++++++++++++++++ .../cluster/InstanceStatusCell.test.tsx | 30 ++++++ src/features/cluster/InstanceStatusCell.tsx | 11 ++- src/hooks/useIsOnScreen.test.ts | 50 ++++++++++ src/hooks/useIsOnScreen.ts | 28 ++++++ src/test/fakeIntersectionObserver.ts | 48 ++++++++++ 6 files changed, 256 insertions(+), 3 deletions(-) create mode 100644 src/features/cluster/InstanceStatusCell.polling.test.tsx create mode 100644 src/hooks/useIsOnScreen.test.ts create mode 100644 src/hooks/useIsOnScreen.ts create mode 100644 src/test/fakeIntersectionObserver.ts diff --git a/src/features/cluster/InstanceStatusCell.polling.test.tsx b/src/features/cluster/InstanceStatusCell.polling.test.tsx new file mode 100644 index 000000000..6542d7b71 --- /dev/null +++ b/src/features/cluster/InstanceStatusCell.polling.test.tsx @@ -0,0 +1,92 @@ +/** @vitest-environment jsdom */ +import type { Instance } from '@/integrations/api/api.patch'; +import { FakeIntersectionObserver } from '@/test/fakeIntersectionObserver'; +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import { act, cleanup, render } from '@testing-library/react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { InstanceStatusCell } from './InstanceStatusCell'; + +/** Real `getStatusQueryOptions` and a real QueryClient: only the transport is faked, so these + * count the requests the instances list actually sends. */ +const state = vi.hoisted(() => ({ requests: [] as string[] })); +vi.mock('@/hooks/usePermissions', () => ({ useOrganizationClusterInstancePermissions: () => ({ update: true }) })); +vi.mock('@/config/useInstanceClient', () => ({ + useInstanceClientIdParams: ({ instanceId }: { instanceId: string }) => ({ + entityId: instanceId, + instanceClient: { + post: async () => { + state.requests.push(instanceId); + return { data: { systemStatus: [{ id: 'availability', status: 'Available' }], componentStatus: [] } }; + }, + }, + }), +})); +vi.mock('@/integrations/api/instance/status/setStatus', () => ({ useSetStatus: () => ({ mutate: vi.fn() }) })); + +const ids = ['ins-0', 'ins-1', 'ins-2']; + +beforeEach(() => { + vi.useFakeTimers(); + vi.stubGlobal('IntersectionObserver', FakeIntersectionObserver); + state.requests = []; +}); +afterEach(() => { + cleanup(); + vi.useRealTimers(); + vi.unstubAllGlobals(); + FakeIntersectionObserver.reset(); +}); + +function renderList() { + const queryClient = new QueryClient(); + render( + + {ids.map((id, index) => ( +
+ +
+ ))} + , + ); + return queryClient; +} + +function rowIs(id: string) { + return (target: Element) => target.closest('[data-row]')?.getAttribute('data-row') === id; +} + +function requestsFor(id: string) { + return state.requests.filter(r => r === id).length; +} + +describe('instances list get_status polling', () => { + it('polls only the rows on screen', async () => { + const queryClient = renderList(); + // Past the per-row stagger; React flushes the `ready` update only when an act() ends. + await act(() => vi.advanceTimersByTimeAsync(2_000)); + act(() => FakeIntersectionObserver.setIntersecting(true, rowIs('ins-0'))); + await act(() => vi.advanceTimersByTimeAsync(60_000)); + + expect(requestsFor('ins-0')).toBe(7); + expect(requestsFor('ins-1')).toBe(0); + expect(requestsFor('ins-2')).toBe(0); + queryClient.clear(); + }); + + it('fetches a row as soon as it scrolls into view, then keeps its 10s cadence', async () => { + const queryClient = renderList(); + await act(() => vi.advanceTimersByTimeAsync(30_000)); + expect(state.requests).toHaveLength(0); + + act(() => FakeIntersectionObserver.setIntersecting(true, rowIs('ins-1'))); + await act(() => vi.advanceTimersByTimeAsync(0)); + expect(requestsFor('ins-1')).toBe(1); + + await act(() => vi.advanceTimersByTimeAsync(30_000)); + expect(requestsFor('ins-1')).toBe(4); + queryClient.clear(); + }); +}); diff --git a/src/features/cluster/InstanceStatusCell.test.tsx b/src/features/cluster/InstanceStatusCell.test.tsx index feeb95b90..b46dec442 100644 --- a/src/features/cluster/InstanceStatusCell.test.tsx +++ b/src/features/cluster/InstanceStatusCell.test.tsx @@ -1,5 +1,6 @@ /** @vitest-environment jsdom */ import type { Instance } from '@/integrations/api/api.patch'; +import { FakeIntersectionObserver } from '@/test/fakeIntersectionObserver'; import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; import { act, cleanup, render, screen } from '@testing-library/react'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; @@ -25,6 +26,8 @@ beforeEach(() => { afterEach(() => { cleanup(); vi.useRealTimers(); + vi.unstubAllGlobals(); + FakeIntersectionObserver.reset(); }); async function renderCell(status: string, version: string | undefined = '4.7.3') { @@ -87,3 +90,30 @@ describe('InstanceStatusCell get_status poll', () => { expect(screen.queryByRole('button')).toBeNull(); }); }); + +describe('InstanceStatusCell off-screen gating', () => { + beforeEach(() => { + vi.stubGlobal('IntersectionObserver', FakeIntersectionObserver); + }); + + it('does not poll a row until it scrolls into view', async () => { + await renderCell('RUNNING'); + expect(state.enabled.at(-1)).toBe(false); + + act(() => FakeIntersectionObserver.setIntersecting(true)); + expect(state.enabled.at(-1)).toBe(true); + }); + + it('stops polling a row once it scrolls out of view', async () => { + await renderCell('RUNNING'); + act(() => FakeIntersectionObserver.setIntersecting(true)); + act(() => FakeIntersectionObserver.setIntersecting(false)); + expect(state.enabled.at(-1)).toBe(false); + }); + + it('shows a spinner rather than an unknown status for a row that has not loaded yet', async () => { + await renderCell('RUNNING'); + expect(screen.queryByText('Offline')).toBeNull(); + expect(document.querySelector('.animate-spin')).toBeTruthy(); + }); +}); diff --git a/src/features/cluster/InstanceStatusCell.tsx b/src/features/cluster/InstanceStatusCell.tsx index 678896d2a..01b7b743e 100644 --- a/src/features/cluster/InstanceStatusCell.tsx +++ b/src/features/cluster/InstanceStatusCell.tsx @@ -3,6 +3,7 @@ import { Button } from '@/components/ui/button'; import { Tooltip, TooltipContent, TooltipTrigger } from '@/components/ui/tooltip'; import { isOperationsProxyRefused, isStoppedOrTransitioning } from '@/components/ui/utils/badgeStatus'; import { useInstanceClientIdParams } from '@/config/useInstanceClient'; +import { useIsOnScreen } from '@/hooks/useIsOnScreen'; import { useOrganizationClusterInstancePermissions } from '@/hooks/usePermissions'; import { Instance } from '@/integrations/api/api.patch'; import { @@ -30,6 +31,10 @@ export function InstanceStatusCell( const { mutate: setStatus, isPending: isSettingStatus } = useSetStatus(); const [ready, setReady] = useState(false); + const [cellElement, setCellElement] = useState(null); + // A row scrolled out of view stops polling, so a large cluster costs what is on screen rather than + // one request per instance every 10s. The margin starts a row's fetch just before it scrolls in. + const onScreen = useIsOnScreen(cellElement, '200px'); // Spread the status requests out. useEffect(() => { @@ -39,7 +44,7 @@ export function InstanceStatusCell( const notProxied = isOperationsProxyRefused(instance.status); const statusSupported = supportsGetStatus(instance.version); - const statusPollEnabled = ready && canManage && !notProxied && statusSupported; + const statusPollEnabled = ready && onScreen && canManage && !notProxied && statusSupported; const { data: statusResponse, isLoading, isFetching } = useQuery( getStatusQueryOptions(instanceParams, statusPollEnabled), ); @@ -75,11 +80,11 @@ export function InstanceStatusCell( } return ( -
+
- {isLoading || !ready || (isFetching && !statusResponse) + {isLoading || !ready || ((isFetching || !onScreen) && !statusResponse) ? : ( { + cleanup(); + vi.unstubAllGlobals(); + FakeIntersectionObserver.reset(); +}); + +describe('useIsOnScreen', () => { + it('is always true where the browser has no IntersectionObserver', () => { + expect(typeof IntersectionObserver).toBe('undefined'); + const { result } = renderHook(() => useIsOnScreen(document.createElement('div'))); + expect(result.current).toBe(true); + }); + + it('is false until the observer reports the element, then follows it in and out of view', () => { + vi.stubGlobal('IntersectionObserver', FakeIntersectionObserver); + const { result } = renderHook(() => useIsOnScreen(document.createElement('div'))); + expect(result.current).toBe(false); + + act(() => FakeIntersectionObserver.setIntersecting(true)); + expect(result.current).toBe(true); + + act(() => FakeIntersectionObserver.setIntersecting(false)); + expect(result.current).toBe(false); + }); + + it('observes with the given root margin and stops observing on unmount', () => { + vi.stubGlobal('IntersectionObserver', FakeIntersectionObserver); + const element = document.createElement('div'); + const { unmount } = renderHook(() => useIsOnScreen(element, '200px')); + const [observer] = FakeIntersectionObserver.instances; + expect(observer.options?.rootMargin).toBe('200px'); + expect([...observer.targets]).toEqual([element]); + + unmount(); + expect(observer.targets.size).toBe(0); + }); + + it('waits for an element before observing anything', () => { + vi.stubGlobal('IntersectionObserver', FakeIntersectionObserver); + const { result } = renderHook(() => useIsOnScreen(null)); + expect(result.current).toBe(false); + expect(FakeIntersectionObserver.instances).toHaveLength(0); + }); +}); diff --git a/src/hooks/useIsOnScreen.ts b/src/hooks/useIsOnScreen.ts new file mode 100644 index 000000000..edf5248ff --- /dev/null +++ b/src/hooks/useIsOnScreen.ts @@ -0,0 +1,28 @@ +import { useEffect, useState } from 'react'; + +/** + * Whether `element` is within `rootMargin` of the viewport. Always true where the browser has no + * `IntersectionObserver`, so work gated on it degrades to always-on rather than never running. + */ +export function useIsOnScreen(element: Element | null, rootMargin = '0px'): boolean { + const [onScreen, setOnScreen] = useState(() => typeof IntersectionObserver === 'undefined'); + + useEffect(() => { + if (!element || typeof IntersectionObserver === 'undefined') { + return; + } + const observer = new IntersectionObserver( + (entries) => { + const entry = entries.at(-1); + if (entry) { + setOnScreen(entry.isIntersecting); + } + }, + { rootMargin }, + ); + observer.observe(element); + return () => observer.disconnect(); + }, [element, rootMargin]); + + return onScreen; +} diff --git a/src/test/fakeIntersectionObserver.ts b/src/test/fakeIntersectionObserver.ts new file mode 100644 index 000000000..448fbbb4a --- /dev/null +++ b/src/test/fakeIntersectionObserver.ts @@ -0,0 +1,48 @@ +/** A controllable stand-in for `IntersectionObserver`, which jsdom does not implement. Install it + * with `vi.stubGlobal('IntersectionObserver', FakeIntersectionObserver)`, then drive visibility + * with `setIntersecting`. */ +export class FakeIntersectionObserver { + static instances: FakeIntersectionObserver[] = []; + + readonly callback: IntersectionObserverCallback; + readonly options: IntersectionObserverInit | undefined; + readonly targets = new Set(); + + constructor(callback: IntersectionObserverCallback, options?: IntersectionObserverInit) { + this.callback = callback; + this.options = options; + FakeIntersectionObserver.instances.push(this); + } + + observe(target: Element) { + this.targets.add(target); + } + + unobserve(target: Element) { + this.targets.delete(target); + } + + disconnect() { + this.targets.clear(); + } + + takeRecords(): IntersectionObserverEntry[] { + return []; + } + + /** Report observed targets as (not) intersecting — all of them, or those `which` accepts. */ + static setIntersecting(isIntersecting: boolean, which: (target: Element) => boolean = () => true) { + for (const observer of FakeIntersectionObserver.instances) { + const targets = [...observer.targets].filter(which); + if (targets.length === 0) { + continue; + } + const entries = targets.map(target => ({ isIntersecting, target }) as IntersectionObserverEntry); + observer.callback(entries, observer as unknown as IntersectionObserver); + } + } + + static reset() { + FakeIntersectionObserver.instances = []; + } +} From 6fef406806394f45c5bf314cb7667922066710ba Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Sat, 10 Oct 2026 23:32:38 -0400 Subject: [PATCH 05/21] fix(clusters): a row's visibility belongs to the element that was observed Review follow-up: useIsOnScreen kept the last reported visibility when its element was removed or replaced, so a row that went RESTARTING while on screen, was scrolled away, and came back RUNNING would fetch once before the observer reported its new element off-screen. The state now records which element it describes and reads false for any other. Refs #1603 Co-Authored-By: Claude Opus 5.5 --- src/hooks/useIsOnScreen.test.ts | 18 +++++++++++++++++- src/hooks/useIsOnScreen.ts | 12 ++++++++---- src/test/fakeIntersectionObserver.ts | 1 - 3 files changed, 25 insertions(+), 6 deletions(-) diff --git a/src/hooks/useIsOnScreen.test.ts b/src/hooks/useIsOnScreen.test.ts index 92cbe9cad..8f0dd29ab 100644 --- a/src/hooks/useIsOnScreen.test.ts +++ b/src/hooks/useIsOnScreen.test.ts @@ -19,7 +19,8 @@ describe('useIsOnScreen', () => { it('is false until the observer reports the element, then follows it in and out of view', () => { vi.stubGlobal('IntersectionObserver', FakeIntersectionObserver); - const { result } = renderHook(() => useIsOnScreen(document.createElement('div'))); + const element = document.createElement('div'); + const { result } = renderHook(() => useIsOnScreen(element)); expect(result.current).toBe(false); act(() => FakeIntersectionObserver.setIntersecting(true)); @@ -41,6 +42,21 @@ describe('useIsOnScreen', () => { expect(observer.targets.size).toBe(0); }); + it("does not carry a removed element's visibility over to its replacement", () => { + vi.stubGlobal('IntersectionObserver', FakeIntersectionObserver); + const { result, rerender } = renderHook(({ element }) => useIsOnScreen(element), { + initialProps: { element: document.createElement('div') as Element | null }, + }); + act(() => FakeIntersectionObserver.setIntersecting(true)); + expect(result.current).toBe(true); + + rerender({ element: null }); + expect(result.current).toBe(false); + + rerender({ element: document.createElement('div') }); + expect(result.current).toBe(false); + }); + it('waits for an element before observing anything', () => { vi.stubGlobal('IntersectionObserver', FakeIntersectionObserver); const { result } = renderHook(() => useIsOnScreen(null)); diff --git a/src/hooks/useIsOnScreen.ts b/src/hooks/useIsOnScreen.ts index edf5248ff..c0f0f7e6a 100644 --- a/src/hooks/useIsOnScreen.ts +++ b/src/hooks/useIsOnScreen.ts @@ -3,9 +3,10 @@ import { useEffect, useState } from 'react'; /** * Whether `element` is within `rootMargin` of the viewport. Always true where the browser has no * `IntersectionObserver`, so work gated on it degrades to always-on rather than never running. + * False for an element the observer has not reported on yet, including a replacement element. */ export function useIsOnScreen(element: Element | null, rootMargin = '0px'): boolean { - const [onScreen, setOnScreen] = useState(() => typeof IntersectionObserver === 'undefined'); + const [observed, setObserved] = useState<{ element: Element; onScreen: boolean } | null>(null); useEffect(() => { if (!element || typeof IntersectionObserver === 'undefined') { @@ -13,9 +14,9 @@ export function useIsOnScreen(element: Element | null, rootMargin = '0px'): bool } const observer = new IntersectionObserver( (entries) => { - const entry = entries.at(-1); + const entry = entries[entries.length - 1]; if (entry) { - setOnScreen(entry.isIntersecting); + setObserved({ element, onScreen: entry.isIntersecting }); } }, { rootMargin }, @@ -24,5 +25,8 @@ export function useIsOnScreen(element: Element | null, rootMargin = '0px'): bool return () => observer.disconnect(); }, [element, rootMargin]); - return onScreen; + if (typeof IntersectionObserver === 'undefined') { + return true; + } + return observed !== null && observed.element === element && observed.onScreen; } diff --git a/src/test/fakeIntersectionObserver.ts b/src/test/fakeIntersectionObserver.ts index 448fbbb4a..94af592d4 100644 --- a/src/test/fakeIntersectionObserver.ts +++ b/src/test/fakeIntersectionObserver.ts @@ -30,7 +30,6 @@ export class FakeIntersectionObserver { return []; } - /** Report observed targets as (not) intersecting — all of them, or those `which` accepts. */ static setIntersecting(isIntersecting: boolean, which: (target: Element) => boolean = () => true) { for (const observer of FakeIntersectionObserver.instances) { const targets = [...observer.targets].filter(which); From 3a341c3def445196d337ebbea7c851bb34de2ac2 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Mon, 12 Oct 2026 11:03:19 -0400 Subject: [PATCH 06/21] fix(hooks): let useIsOnScreen release an element once it is gone The hook kept the last observed element in state after the element went away, so an instances-list cell that switched to its muted branch held its detached status subtree for as long as the row stayed mounted. Reset the stale report during render, the documented pattern for adjusting state to a changed input; the return value is unchanged. Co-Authored-By: Claude Opus 5.5 --- src/hooks/useIsOnScreen.test.ts | 32 ++++++++++++++++++++++++++++++++ src/hooks/useIsOnScreen.ts | 4 ++++ 2 files changed, 36 insertions(+) diff --git a/src/hooks/useIsOnScreen.test.ts b/src/hooks/useIsOnScreen.test.ts index 8f0dd29ab..606c210e0 100644 --- a/src/hooks/useIsOnScreen.test.ts +++ b/src/hooks/useIsOnScreen.test.ts @@ -1,9 +1,19 @@ /** @vitest-environment jsdom */ import { FakeIntersectionObserver } from '@/test/fakeIntersectionObserver'; import { act, cleanup, renderHook } from '@testing-library/react'; +import { setFlagsFromString } from 'node:v8'; +import { runInNewContext } from 'node:vm'; import { afterEach, describe, expect, it, vi } from 'vitest'; import { useIsOnScreen } from './useIsOnScreen'; +/** Hands the hook an element that no test variable holds, returning whether it has been collected. */ +function showFreshElement(rerender: (props: { element: Element | null }) => void) { + const element = document.createElement('div'); + const reference = new WeakRef(element); + rerender({ element }); + return () => reference.deref() === undefined; +} + afterEach(() => { cleanup(); vi.unstubAllGlobals(); @@ -57,6 +67,28 @@ describe('useIsOnScreen', () => { expect(result.current).toBe(false); }); + it('lets go of an element once it is gone, so the detached node can be collected', async () => { + vi.stubGlobal('IntersectionObserver', FakeIntersectionObserver); + setFlagsFromString('--expose-gc'); + const collectGarbage = runInNewContext('gc') as () => void; + // Testing Library holds on to the initial props, so the element has to arrive by rerender. + const { rerender } = renderHook(({ element }) => useIsOnScreen(element), { + initialProps: { element: null } as { element: Element | null }, + }); + const collected = showFreshElement(rerender); + act(() => FakeIntersectionObserver.setIntersecting(true)); + + rerender({ element: null }); + // React keeps the previous render's props on the alternate fiber until the next render. + rerender({ element: null }); + // The disconnected fake still holds its callback, which closes over the element. + FakeIntersectionObserver.reset(); + await new Promise(resolve => setTimeout(resolve, 0)); + collectGarbage(); + + expect(collected()).toBe(true); + }); + it('waits for an element before observing anything', () => { vi.stubGlobal('IntersectionObserver', FakeIntersectionObserver); const { result } = renderHook(() => useIsOnScreen(null)); diff --git a/src/hooks/useIsOnScreen.ts b/src/hooks/useIsOnScreen.ts index c0f0f7e6a..cb725902f 100644 --- a/src/hooks/useIsOnScreen.ts +++ b/src/hooks/useIsOnScreen.ts @@ -7,6 +7,10 @@ import { useEffect, useState } from 'react'; */ export function useIsOnScreen(element: Element | null, rootMargin = '0px'): boolean { const [observed, setObserved] = useState<{ element: Element; onScreen: boolean } | null>(null); + // Drop a report about an element that is gone, so a detached element is not kept alive in state. + if (observed !== null && observed.element !== element) { + setObserved(null); + } useEffect(() => { if (!element || typeof IntersectionObserver === 'undefined') { From fd2e004ed15f434d31165fcdb2981be2424e6a04 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Mon, 12 Oct 2026 11:03:30 -0400 Subject: [PATCH 07/21] fix(clusters): give the instances list's status spinner screen-reader text The spinner icon is aria-hidden, so a row whose status had not loaded read as an empty cell. Off-screen rows now stay on the spinner until they scroll into view, which made that state common. Co-Authored-By: Claude Opus 5.5 --- src/features/cluster/InstanceStatusCell.test.tsx | 4 +++- src/features/cluster/InstanceStatusCell.tsx | 7 ++++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/features/cluster/InstanceStatusCell.test.tsx b/src/features/cluster/InstanceStatusCell.test.tsx index b46dec442..c0d972461 100644 --- a/src/features/cluster/InstanceStatusCell.test.tsx +++ b/src/features/cluster/InstanceStatusCell.test.tsx @@ -114,6 +114,8 @@ describe('InstanceStatusCell off-screen gating', () => { it('shows a spinner rather than an unknown status for a row that has not loaded yet', async () => { await renderCell('RUNNING'); expect(screen.queryByText('Offline')).toBeNull(); - expect(document.querySelector('.animate-spin')).toBeTruthy(); + // The icon itself is hidden from screen readers, so the text is all they get. + expect(document.querySelector('.animate-spin')?.getAttribute('aria-hidden')).toBe('true'); + expect(screen.getByText('Loading status')).toBeTruthy(); }); }); diff --git a/src/features/cluster/InstanceStatusCell.tsx b/src/features/cluster/InstanceStatusCell.tsx index 01b7b743e..415d2ec25 100644 --- a/src/features/cluster/InstanceStatusCell.tsx +++ b/src/features/cluster/InstanceStatusCell.tsx @@ -85,7 +85,12 @@ export function InstanceStatusCell(
{isLoading || !ready || ((isFetching || !onScreen) && !statusResponse) - ? + ? ( + <> + + Loading status + + ) : ( Date: Mon, 12 Oct 2026 11:12:24 -0400 Subject: [PATCH 08/21] test(clusters): drop a comment the aria-hidden assertion now states Co-Authored-By: Claude Opus 5.5 --- src/features/cluster/InstanceStatusCell.test.tsx | 1 - src/hooks/useIsOnScreen.test.ts | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/features/cluster/InstanceStatusCell.test.tsx b/src/features/cluster/InstanceStatusCell.test.tsx index c0d972461..e5ab738d0 100644 --- a/src/features/cluster/InstanceStatusCell.test.tsx +++ b/src/features/cluster/InstanceStatusCell.test.tsx @@ -114,7 +114,6 @@ describe('InstanceStatusCell off-screen gating', () => { it('shows a spinner rather than an unknown status for a row that has not loaded yet', async () => { await renderCell('RUNNING'); expect(screen.queryByText('Offline')).toBeNull(); - // The icon itself is hidden from screen readers, so the text is all they get. expect(document.querySelector('.animate-spin')?.getAttribute('aria-hidden')).toBe('true'); expect(screen.getByText('Loading status')).toBeTruthy(); }); diff --git a/src/hooks/useIsOnScreen.test.ts b/src/hooks/useIsOnScreen.test.ts index 606c210e0..5b1b8b4b5 100644 --- a/src/hooks/useIsOnScreen.test.ts +++ b/src/hooks/useIsOnScreen.test.ts @@ -6,7 +6,7 @@ import { runInNewContext } from 'node:vm'; import { afterEach, describe, expect, it, vi } from 'vitest'; import { useIsOnScreen } from './useIsOnScreen'; -/** Hands the hook an element that no test variable holds, returning whether it has been collected. */ +/** Builds the element out of the test's scope, so only the hook can keep it alive. */ function showFreshElement(rerender: (props: { element: Element | null }) => void) { const element = document.createElement('div'); const reference = new WeakRef(element); From 8c2f6a58824176416aef86f39ed85fd5a9525bdd Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Sat, 10 Oct 2026 23:14:45 -0400 Subject: [PATCH 09/21] fix(deployments): rejected deployment polls back off, and the page needs Harper 5.1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The deployment history list (2s/10s), the deployment detail (2s until terminal) and the log auto-refresh (5s) were the instance-operation pollers left outside the shared poll policy, so a 403 never stopped them and a 400 or 5xx never slowed them. The detail poll was the worst: a failed fetch has no status to end on, so a deployment id the instance rejects ("No deployment found with id …", a 400) was asked for every 2s for as long as the dialog stayed open. All three now go through pollUnlessForbidden; the two whose interval depends on the data compute it per tick and hand it over. The deployments page is hidden from the nav below Harper 5.1, but a bookmark or back-navigation still mounted it and polled list_deployments against an instance that doesn't have it. It now reads the instance's version first and says the feature needs Harper 5.1 instead of polling. DESIGN.md records the polling convention. Refs #1602 Co-Authored-By: Claude Opus 5.5 --- DESIGN.md | 1 + .../hooks/useSupportsDeploymentSSE.ts | 6 +- .../config/deployments/index.test.tsx | 63 ++++++++++++++ .../instance/config/deployments/index.tsx | 25 +++++- .../deployments/deploymentPolling.test.ts | 83 +++++++++++++++++++ .../api/instance/deployments/getDeployment.ts | 8 +- .../instance/deployments/listDeployments.ts | 3 +- .../api/instance/status/getReadLog.test.ts | 30 ++++++- .../api/instance/status/getReadLog.ts | 3 +- 9 files changed, 213 insertions(+), 9 deletions(-) create mode 100644 src/features/instance/config/deployments/index.test.tsx create mode 100644 src/integrations/api/instance/deployments/deploymentPolling.test.ts diff --git a/DESIGN.md b/DESIGN.md index a66b1bd9d..fd52bd729 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -75,6 +75,7 @@ Central manager only ever sends the boolean, never the other organizations' ids; again on the dead session. The guard redirects every signed-out dashboard load, so the spinner only covers the moment before it does (`endedSessionRouteFailures.test.tsx`, HarperFast/studio#1546). - **Data fetching**: TanStack React Query 5. Query keys must be instance-scoped when the request targets a specific instance. +- **Polling**: every `refetchInterval` on a query that can reject against an instance or a cluster goes through [`pollUnlessForbidden`](src/react-query/pollUnlessForbidden.ts), which stops the timer on a 403 and backs off a sustained 400 or 5xx; a raw number polls a failing endpoint at full rate for as long as the page is open. When the interval depends on the data, compute it and hand it over per tick: `(query) => pollUnlessForbidden(base(query))(query)` (the back-off state is per query, not per wrapper). - **Forms**: react-hook-form with a zod `zodResolver`. A form whose submit button waits on `formState.isValid` must not use the default `onSubmit` mode, which keeps `isValid` current but shows errors only after a submit the disabled button never allows, so the user is blocked with no reason given (HarperFast/studio#1550). Use `mode: 'onTouched'`: a field's error appears once it is first left and then follows each change, while `isValid` still tracks every keystroke (`onBlur` recomputes `isValid` only on blur, so the button lags the text). Enter neither submits past a disabled button nor leaves the field, so give the `
` `onKeyDown={revealFieldErrorOnEnter(form)}`, which treats Enter in a text input as leaving it. A refine that compares two fields reports on one of them; re-check that field from the other's `onChange` once it has been touched (`getFieldState(name).isTouched`, then `trigger(name)`), because `rules={{ deps }}` fires on the other field's first blur, before the user reaches it. react-hook-form only updates the `formState` fields a render has read, and `disabled={!isDirty || !isValid}` skips `isValid` while the form is pristine, so where one change can complete the form (a single field, a paste, a checkbox) the button stays disabled on a valid form. Read the fields before the JSX (`const { isDirty, isValid } = form.formState;`) and pin it with a test that makes one change and checks the button. - **Cloud permission gates** ([`usePermissions.ts`](src/hooks/usePermissions.ts)) must mirror the check the central manager endpoint runs, not the nearest role flag. An org admin appears in `/User/current` as `roles[orgId].role === 'admin'`; central manager builds every org role with `permission.super_user: false`, so the member helpers' `super_user` short-circuits never identify one — an admin gets full access there from its all-true flags. Container lifecycle ops are where the two diverge: their endpoints admit only org admins and staff holding `instance:update`, so gate them on `useContainerOpsPermission`, never on cluster `update`. - **Unit tests never read `.env` files and never open a real WebSocket.** `vitest.config.ts` sets diff --git a/src/features/instance/applications/hooks/useSupportsDeploymentSSE.ts b/src/features/instance/applications/hooks/useSupportsDeploymentSSE.ts index 89ed6eece..deedf2c6b 100644 --- a/src/features/instance/applications/hooks/useSupportsDeploymentSSE.ts +++ b/src/features/instance/applications/hooks/useSupportsDeploymentSSE.ts @@ -11,6 +11,10 @@ import { useQuery } from '@tanstack/react-query'; */ export const MIN_DEPLOYMENT_SSE_VERSION = '5.1.0'; +export function supportsDeployments(harperVersion: string | undefined): boolean { + return !!harperVersion && wasAReleasedBeforeB(MIN_DEPLOYMENT_SSE_VERSION, harperVersion); +} + /** * True when the instance/cluster is Harper >= 5.1.0, so the deployment history/detail * feature (`list_deployments` / `get_deployment`) is available. This gates feature @@ -19,7 +23,7 @@ export const MIN_DEPLOYMENT_SSE_VERSION = '5.1.0'; export function useDeploymentsAvailable(): boolean { const params = useInstanceClientIdParams(); const { data } = useQuery(getRegistrationInfoQueryOptions(params)); - return !!data?.version && wasAReleasedBeforeB(MIN_DEPLOYMENT_SSE_VERSION, data.version); + return supportsDeployments(data?.version); } /** diff --git a/src/features/instance/config/deployments/index.test.tsx b/src/features/instance/config/deployments/index.test.tsx new file mode 100644 index 000000000..b5b5be0ee --- /dev/null +++ b/src/features/instance/config/deployments/index.test.tsx @@ -0,0 +1,63 @@ +/** @vitest-environment jsdom */ +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import { cleanup, render, screen } from '@testing-library/react'; +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { ConfigDeploymentsIndex } from './index'; + +const state = vi.hoisted(() => ({ + version: undefined as string | undefined, + listEnabled: [] as (boolean | undefined)[], +})); +vi.mock('@tanstack/react-router', () => ({ useNavigate: () => vi.fn(), useParams: () => ({}) })); +vi.mock('@/hooks/usePermissions', () => ({ useInstanceManagePermission: () => true })); +vi.mock('@/config/useInstanceClient', () => ({ useInstanceClientIdParams: () => ({ entityId: 'ins-a' }) })); +vi.mock('@/integrations/api/instance/status/getRegistrationInfo', () => ({ + getRegistrationInfoQueryOptions: () => ({ + queryKey: ['registration_info', 'ins-a'], + queryFn: () => new Promise(() => {}), + ...(state.version === undefined ? {} : { initialData: { version: state.version } }), + }), +})); +vi.mock('@/integrations/api/instance/deployments/listDeployments', () => ({ + getDeploymentsQueryOptions: ({ enabled }: { enabled?: boolean }) => { + state.listEnabled.push(enabled); + return { queryKey: ['list_deployments'], queryFn: () => ({ deployments: [], total: 0 }), enabled: false }; + }, +})); +vi.mock('./modals/DeploymentDetailModal', () => ({ DeploymentDetailModal: () => null })); + +afterEach(() => { + cleanup(); + state.version = undefined; + state.listEnabled = []; +}); + +function renderPage(version: string | undefined) { + state.version = version; + render( + + + , + ); +} + +describe('ConfigDeploymentsIndex version gate', () => { + it('lists deployments on Harper 5.1 and later', () => { + renderPage('5.1.0'); + expect(state.listEnabled.at(-1)).toBe(true); + expect(screen.queryByText(/needs Harper 5\.1/)).toBeNull(); + }); + + it('never polls list_deployments on an older Harper reached by URL, and says why', () => { + renderPage('5.0.13'); + expect(state.listEnabled.every(enabled => enabled === false)).toBe(true); + expect(screen.getByText('Deployment history needs Harper 5.1 or later on this instance.')).toBeTruthy(); + }); + + it('waits for the version instead of claiming either outcome', () => { + renderPage(undefined); + expect(state.listEnabled.every(enabled => enabled === false)).toBe(true); + expect(screen.queryByText(/needs Harper 5\.1/)).toBeNull(); + expect(screen.queryByText('No deployments yet.')).toBeNull(); + }); +}); diff --git a/src/features/instance/config/deployments/index.tsx b/src/features/instance/config/deployments/index.tsx index 723f70eef..7c2c30a70 100644 --- a/src/features/instance/config/deployments/index.tsx +++ b/src/features/instance/config/deployments/index.tsx @@ -1,10 +1,12 @@ import { SimpleBrowseDataTable } from '@/components/SimpleBrowseDataTable'; import { Button } from '@/components/ui/button'; import { useInstanceClientIdParams } from '@/config/useInstanceClient'; +import { supportsDeployments } from '@/features/instance/applications/hooks/useSupportsDeploymentSSE'; import { useInstanceManagePermission } from '@/hooks/usePermissions'; import { useRefreshClick } from '@/hooks/useRefreshClick'; import { getDeploymentsQueryOptions } from '@/integrations/api/instance/deployments/listDeployments'; import { Deployment } from '@/integrations/api/instance/deployments/types'; +import { getRegistrationInfoQueryOptions } from '@/integrations/api/instance/status/getRegistrationInfo'; import { Row } from '@/lib/table'; import { useQuery } from '@tanstack/react-query'; import { useNavigate, useParams } from '@tanstack/react-router'; @@ -18,9 +20,18 @@ export function ConfigDeploymentsIndex() { const canManage = useInstanceManagePermission(); const { deploymentId }: { deploymentId?: string } = useParams({ strict: false }); const instanceParams = useInstanceClientIdParams(); + // The nav hides this page below Harper 5.1, but a bookmark or back-navigation still lands here. + const { data: registrationInfo } = useQuery(getRegistrationInfoQueryOptions(instanceParams)); + const versionKnown = registrationInfo !== undefined; + const deploymentsAvailable = supportsDeployments(registrationInfo?.version); const { data, isFetching, isRefetching, refetch, error } = useQuery( - getDeploymentsQueryOptions({ ...instanceParams, limit: 100, enabled: canManage, pollWhileOpen: true }), + getDeploymentsQueryOptions({ + ...instanceParams, + limit: 100, + enabled: canManage && deploymentsAvailable, + pollWhileOpen: true, + }), ); const deployments = useMemo(() => data?.deployments ?? [], [data]); @@ -43,11 +54,19 @@ export function ConfigDeploymentsIndex() { ); } + if (versionKnown && !deploymentsAvailable) { + return ( +
+ Deployment history needs Harper 5.1 or later on this instance. +
+ ); + } + return ( <> data={deployments} - isFetching={isFetching || isRefetching} + isFetching={isFetching || isRefetching || !versionKnown} columns={deploymentColumns} onRowClick={onRowClick} > @@ -68,7 +87,7 @@ export function ConfigDeploymentsIndex() { Could not load deployments. This instance may not support deployment history.
)} - {!error && !isFetching && deployments.length === 0 && ( + {!error && !isFetching && versionKnown && deployments.length === 0 && (
No deployments yet.
)} {!!deploymentId && ( diff --git a/src/integrations/api/instance/deployments/deploymentPolling.test.ts b/src/integrations/api/instance/deployments/deploymentPolling.test.ts new file mode 100644 index 000000000..3ba1e9f83 --- /dev/null +++ b/src/integrations/api/instance/deployments/deploymentPolling.test.ts @@ -0,0 +1,83 @@ +/** @vitest-environment jsdom */ +import type { AxiosInstance } from 'axios'; +import { AxiosError } from 'axios'; +import { describe, expect, it } from 'vitest'; +import { getDeploymentQueryOptions } from './getDeployment'; +import { getDeploymentsQueryOptions } from './listDeployments'; +import type { Deployment, DeploymentStatus, ListDeploymentsResponse } from './types'; + +type PollState = { error: unknown; errorUpdatedAt?: number; dataUpdatedAt?: number; data?: TData }; +type IntervalFn = (query: { state: PollState }) => number | false | undefined; + +function httpError(status: number): AxiosError { + const err = new AxiosError(`Request failed with status code ${status}`); + err.response = { status } as AxiosError['response']; + return err; +} + +const instance = { entityId: 'ins-test' as never, instanceClient: {} as AxiosInstance }; + +function detailInterval(pollWhileActive = true) { + const { refetchInterval } = getDeploymentQueryOptions({ ...instance, deploymentId: 'dep-1', pollWhileActive }); + return refetchInterval as unknown as IntervalFn; +} + +function listInterval(pollWhileOpen = true) { + const { refetchInterval } = getDeploymentsQueryOptions({ ...instance, pollWhileOpen }); + return refetchInterval as unknown as IntervalFn | false; +} + +const deployment = (status: DeploymentStatus) => ({ status }) as Deployment; +const list = (...statuses: DeploymentStatus[]) => + ({ deployments: statuses.map(deployment), total: statuses.length }) as ListDeploymentsResponse; + +describe('get_deployment polling', () => { + it('polls every 2s while the deployment is active and stops once it is terminal', () => { + const poll = detailInterval(); + expect(poll({ state: { error: null, data: deployment('installing') } })).toBe(2_000); + expect(poll({ state: { error: null, data: deployment('success') } })).toBe(false); + }); + + it('does not poll a detail view that never asked to', () => { + expect(detailInterval(false)({ state: { error: null, data: deployment('installing') } })).toBe(false); + }); + + it('stops on a 403 instead of retrying every 2s', () => { + expect(detailInterval()({ state: { error: httpError(403), errorUpdatedAt: 0 } })).toBe(false); + }); + + it('backs off a deployment the instance keeps rejecting, rather than polling it every 2s forever', () => { + // A failed fetch leaves no status, so "until terminal" alone never ends — Harper answers an unknown + // deployment id with a 400 ("No deployment found with id …"). + const poll = detailInterval(); + const query: { state: PollState } = { state: { error: httpError(400), errorUpdatedAt: 0 } }; + expect(poll(query)).toBe(2_000); + query.state = { ...query.state, errorUpdatedAt: 10 * 60_000 }; + expect(poll(query)).toBe(5 * 60_000); + }); +}); + +describe('list_deployments polling', () => { + it('polls fast while any deployment is active and slower otherwise', () => { + const poll = listInterval() as IntervalFn; + expect(poll({ state: { error: null, data: list('success', 'installing') } })).toBe(2_000); + expect(poll({ state: { error: null, data: list('success', 'failed') } })).toBe(10_000); + expect(poll({ state: { error: null } })).toBe(10_000); + }); + + it('does not poll unless the view is open', () => { + expect(listInterval(false)).toBe(false); + }); + + it('stops on a 403 and backs off a sustained 400', () => { + const poll = listInterval() as IntervalFn; + expect(poll({ state: { error: httpError(403), errorUpdatedAt: 0 } })).toBe(false); + + const query: { state: PollState } = { + state: { error: httpError(400), errorUpdatedAt: 0 }, + }; + poll(query); + query.state = { ...query.state, errorUpdatedAt: 60 * 60_000 }; + expect(poll(query)).toBe(5 * 60_000); + }); +}); diff --git a/src/integrations/api/instance/deployments/getDeployment.ts b/src/integrations/api/instance/deployments/getDeployment.ts index dd351cbfa..8b3cb2b76 100644 --- a/src/integrations/api/instance/deployments/getDeployment.ts +++ b/src/integrations/api/instance/deployments/getDeployment.ts @@ -9,6 +9,7 @@ import { import { parseSSEData } from '@/integrations/api/sse/parseSSEStream'; import { ResolveInstanceConnectionParams } from '@/integrations/api/sse/resolveInstanceConnection'; import { streamOperation } from '@/integrations/api/sse/streamOperation'; +import { pollUnlessForbidden } from '@/react-query/pollUnlessForbidden'; import { queryOptions } from '@tanstack/react-query'; import { Deployment, isTerminalDeploymentStatus } from './types'; @@ -33,9 +34,12 @@ export function getDeploymentQueryOptions(params: GetDeploymentParams & { pollWh queryFn: () => getDeployment(params), retry: false, // While the detail view falls back to polling (no SSE), keep refetching until the - // deployment reaches a terminal status, then stop. + // deployment reaches a terminal status, then stop. A failed fetch has no status to end + // on, so the shared policy is what stops or slows it. refetchInterval: (query) => - params.pollWhileActive && !isTerminalDeploymentStatus(query.state.data?.status) ? 2_000 : false, + pollUnlessForbidden( + params.pollWhileActive && !isTerminalDeploymentStatus(query.state.data?.status) ? 2_000 : false, + )(query), }); } diff --git a/src/integrations/api/instance/deployments/listDeployments.ts b/src/integrations/api/instance/deployments/listDeployments.ts index 4fad48531..4ca35f36a 100644 --- a/src/integrations/api/instance/deployments/listDeployments.ts +++ b/src/integrations/api/instance/deployments/listDeployments.ts @@ -1,4 +1,5 @@ import { InstanceClientIdConfig } from '@/config/instanceClientConfig'; +import { pollUnlessForbidden } from '@/react-query/pollUnlessForbidden'; import { queryOptions } from '@tanstack/react-query'; import { DeploymentStatus, isTerminalDeploymentStatus, ListDeploymentsResponse } from './types'; @@ -55,7 +56,7 @@ export function getDeploymentsQueryOptions(params: ListDeploymentsParams) { ? (query) => { const deployments = query.state.data?.deployments ?? []; const anyActive = deployments.some((d) => !isTerminalDeploymentStatus(d.status)); - return anyActive ? ACTIVE_POLL_MS : IDLE_POLL_MS; + return pollUnlessForbidden(anyActive ? ACTIVE_POLL_MS : IDLE_POLL_MS)(query); } : false, }); diff --git a/src/integrations/api/instance/status/getReadLog.test.ts b/src/integrations/api/instance/status/getReadLog.test.ts index 75075396f..8b6ae900e 100644 --- a/src/integrations/api/instance/status/getReadLog.test.ts +++ b/src/integrations/api/instance/status/getReadLog.test.ts @@ -1,5 +1,7 @@ +import type { AxiosInstance } from 'axios'; +import { AxiosError } from 'axios'; import { describe, expect, it } from 'vitest'; -import { buildReadLogBody, clampReadLogLimit, MAX_READ_LOG_LIMIT } from './getReadLog'; +import { buildReadLogBody, clampReadLogLimit, getReadLogQueryOptions, MAX_READ_LOG_LIMIT } from './getReadLog'; describe('clampReadLogLimit', () => { it('returns undefined for blank or invalid input', () => { @@ -30,3 +32,29 @@ describe('buildReadLogBody', () => { expect(buildReadLogBody({ limit: '' }, false).limit).toBeUndefined(); }); }); + +describe('read_log auto-refresh', () => { + function intervalFor(isAutoRefreshEnabled: boolean) { + const { refetchInterval } = getReadLogQueryOptions( + { + entityId: 'ins-test' as never, + instanceClient: {} as AxiosInstance, + logFilters: {}, + replicated: false, + isAutoRefreshEnabled, + } as Parameters[0], + ); + return refetchInterval as unknown as (query: { state: { error: unknown; errorUpdatedAt?: number } }) => unknown; + } + + it('refreshes every 5s only while auto-refresh is on', () => { + expect(intervalFor(true)({ state: { error: null } })).toBe(5_000); + expect(intervalFor(false)({ state: { error: null } })).toBe(false); + }); + + it('stops auto-refreshing a log the user may not read', () => { + const forbidden = new AxiosError('Request failed with status code 403'); + forbidden.response = { status: 403 } as AxiosError['response']; + expect(intervalFor(true)({ state: { error: forbidden, errorUpdatedAt: 0 } })).toBe(false); + }); +}); diff --git a/src/integrations/api/instance/status/getReadLog.ts b/src/integrations/api/instance/status/getReadLog.ts index f31851f06..1715740cb 100644 --- a/src/integrations/api/instance/status/getReadLog.ts +++ b/src/integrations/api/instance/status/getReadLog.ts @@ -1,5 +1,6 @@ import { InstanceClientIdConfig } from '@/config/instanceClientConfig'; import { LogFiltersFormSchema } from '@/integrations/api/instance/status/logFiltersFormSchema'; +import { pollUnlessForbidden } from '@/react-query/pollUnlessForbidden'; import { queryOptions } from '@tanstack/react-query'; import { z } from 'zod'; @@ -83,6 +84,6 @@ export function getReadLogQueryOptions(params: GetReadLogParams & InstanceClient ] as const, queryFn: () => getReadLog(params), retry: false, - refetchInterval: params.isAutoRefreshEnabled ? 5000 : false, + refetchInterval: pollUnlessForbidden(params.isAutoRefreshEnabled ? 5000 : false), }); } From beda7f9ad7c96418d0ce4a1952a28bbb604f0fb2 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Sat, 10 Oct 2026 23:39:17 -0400 Subject: [PATCH 10/21] fix(deployments): say the page could not load when the instance version can't be read Review follow-up: if the registration_info read failed with nothing cached, the deployments page waited on the version forever behind a spinner. It now shows the existing "Could not load deployments" line instead. The log view's auto-refresh off path stays a plain `false` rather than a wrapper that always answers false. Refs #1602 Co-Authored-By: Claude Opus 5.5 --- .../instance/config/deployments/index.test.tsx | 12 +++++++++++- src/features/instance/config/deployments/index.tsx | 9 ++++++--- .../api/instance/status/getReadLog.test.ts | 12 ++++++++---- src/integrations/api/instance/status/getReadLog.ts | 2 +- 4 files changed, 26 insertions(+), 9 deletions(-) diff --git a/src/features/instance/config/deployments/index.test.tsx b/src/features/instance/config/deployments/index.test.tsx index b5b5be0ee..4a621a354 100644 --- a/src/features/instance/config/deployments/index.test.tsx +++ b/src/features/instance/config/deployments/index.test.tsx @@ -6,6 +6,7 @@ import { ConfigDeploymentsIndex } from './index'; const state = vi.hoisted(() => ({ version: undefined as string | undefined, + registrationFails: false, listEnabled: [] as (boolean | undefined)[], })); vi.mock('@tanstack/react-router', () => ({ useNavigate: () => vi.fn(), useParams: () => ({}) })); @@ -14,7 +15,8 @@ vi.mock('@/config/useInstanceClient', () => ({ useInstanceClientIdParams: () => vi.mock('@/integrations/api/instance/status/getRegistrationInfo', () => ({ getRegistrationInfoQueryOptions: () => ({ queryKey: ['registration_info', 'ins-a'], - queryFn: () => new Promise(() => {}), + queryFn: () => (state.registrationFails ? Promise.reject(new Error('unreachable')) : new Promise(() => {})), + retry: false, ...(state.version === undefined ? {} : { initialData: { version: state.version } }), }), })); @@ -29,6 +31,7 @@ vi.mock('./modals/DeploymentDetailModal', () => ({ DeploymentDetailModal: () => afterEach(() => { cleanup(); state.version = undefined; + state.registrationFails = false; state.listEnabled = []; }); @@ -54,6 +57,13 @@ describe('ConfigDeploymentsIndex version gate', () => { expect(screen.getByText('Deployment history needs Harper 5.1 or later on this instance.')).toBeTruthy(); }); + it('says it could not load, rather than spinning forever, when the version cannot be read', async () => { + state.registrationFails = true; + renderPage(undefined); + expect(await screen.findByText(/Could not load deployments/)).toBeTruthy(); + expect(state.listEnabled.every(enabled => enabled === false)).toBe(true); + }); + it('waits for the version instead of claiming either outcome', () => { renderPage(undefined); expect(state.listEnabled.every(enabled => enabled === false)).toBe(true); diff --git a/src/features/instance/config/deployments/index.tsx b/src/features/instance/config/deployments/index.tsx index 7c2c30a70..ec984b3ab 100644 --- a/src/features/instance/config/deployments/index.tsx +++ b/src/features/instance/config/deployments/index.tsx @@ -21,8 +21,11 @@ export function ConfigDeploymentsIndex() { const { deploymentId }: { deploymentId?: string } = useParams({ strict: false }); const instanceParams = useInstanceClientIdParams(); // The nav hides this page below Harper 5.1, but a bookmark or back-navigation still lands here. - const { data: registrationInfo } = useQuery(getRegistrationInfoQueryOptions(instanceParams)); + const { data: registrationInfo, isError: registrationFailed } = useQuery( + getRegistrationInfoQueryOptions(instanceParams), + ); const versionKnown = registrationInfo !== undefined; + const versionUnavailable = !versionKnown && registrationFailed; const deploymentsAvailable = supportsDeployments(registrationInfo?.version); const { data, isFetching, isRefetching, refetch, error } = useQuery( @@ -66,7 +69,7 @@ export function ConfigDeploymentsIndex() { <> data={deployments} - isFetching={isFetching || isRefetching || !versionKnown} + isFetching={isFetching || isRefetching || (!versionKnown && !versionUnavailable)} columns={deploymentColumns} onRowClick={onRowClick} > @@ -82,7 +85,7 @@ export function ConfigDeploymentsIndex() { - {error && ( + {(error || versionUnavailable) && (
Could not load deployments. This instance may not support deployment history.
diff --git a/src/integrations/api/instance/status/getReadLog.test.ts b/src/integrations/api/instance/status/getReadLog.test.ts index 8b6ae900e..bb1bd458e 100644 --- a/src/integrations/api/instance/status/getReadLog.test.ts +++ b/src/integrations/api/instance/status/getReadLog.test.ts @@ -44,17 +44,21 @@ describe('read_log auto-refresh', () => { isAutoRefreshEnabled, } as Parameters[0], ); - return refetchInterval as unknown as (query: { state: { error: unknown; errorUpdatedAt?: number } }) => unknown; + return refetchInterval as unknown as + | ((query: { state: { error: unknown; errorUpdatedAt?: number } }) => unknown) + | false; } it('refreshes every 5s only while auto-refresh is on', () => { - expect(intervalFor(true)({ state: { error: null } })).toBe(5_000); - expect(intervalFor(false)({ state: { error: null } })).toBe(false); + const poll = intervalFor(true); + expect(poll && poll({ state: { error: null } })).toBe(5_000); + expect(intervalFor(false)).toBe(false); }); it('stops auto-refreshing a log the user may not read', () => { const forbidden = new AxiosError('Request failed with status code 403'); forbidden.response = { status: 403 } as AxiosError['response']; - expect(intervalFor(true)({ state: { error: forbidden, errorUpdatedAt: 0 } })).toBe(false); + const poll = intervalFor(true); + expect(poll && poll({ state: { error: forbidden, errorUpdatedAt: 0 } })).toBe(false); }); }); diff --git a/src/integrations/api/instance/status/getReadLog.ts b/src/integrations/api/instance/status/getReadLog.ts index 1715740cb..6df2aa1a0 100644 --- a/src/integrations/api/instance/status/getReadLog.ts +++ b/src/integrations/api/instance/status/getReadLog.ts @@ -84,6 +84,6 @@ export function getReadLogQueryOptions(params: GetReadLogParams & InstanceClient ] as const, queryFn: () => getReadLog(params), retry: false, - refetchInterval: pollUnlessForbidden(params.isAutoRefreshEnabled ? 5000 : false), + refetchInterval: params.isAutoRefreshEnabled ? pollUnlessForbidden(5_000) : false, }); } From 537c82ac248a21bbba6dd788dba121ede3c345e7 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Sun, 11 Oct 2026 02:11:06 -0400 Subject: [PATCH 11/21] feat(deploy): follow a rolling deploy's restart job, and show canary certification A rolling deploy answers once the node that received it is done and hands the other nodes to a restart_service job (restartJobId). Studio never followed it, so a node that rejected the release read as a successful deploy. The deploy mutation now polls get_job until the job ends: a failed job rejects with DeployRolloutError naming each node, and a job Studio cannot follow resolves as unfollowable, which the callers never report as success. Also reads Harper 5.4's canary certification on a deploy's result and its rejection decision on a failure, keeps the deploy's streamed warning events, and links the progress panel to the deployment's record. Refs #1785 Co-Authored-By: Claude Opus 5.5 --- DESIGN.md | 1 + src/features/instance/applications/DESIGN.md | 32 +++ .../DeployProgress/DeployProgress.test.tsx | 133 +++++++++ .../DeployProgress/DeployProgress.tsx | 137 ++++++++- .../DeployProgress/describeDeployOutcome.ts | 88 ++++++ .../useDeploymentStream.test.ts | 64 +++++ .../DeployProgress/useDeploymentStream.ts | 60 +++- .../NewApplication/useImportApplication.ts | 22 +- .../modals/RedeployApplicationModal.tsx | 51 +++- .../modals/reportDeployHealth.test.ts | 75 +++++ .../applications/modals/reportDeployHealth.ts | 76 ++++- .../applications/deployComponent.test.ts | 112 +++++++- .../instance/applications/deployComponent.ts | 28 +- .../applications/deployComponentStream.ts | 67 +++-- .../applications/deployOutcome.test.ts | 141 +++++++++ .../instance/applications/deployOutcome.ts | 61 ++++ .../instance/applications/restartJob.test.ts | 268 ++++++++++++++++++ .../api/instance/applications/restartJob.ts | 197 +++++++++++++ .../api/instance/database/getJob.test.ts | 15 +- .../api/instance/database/getJob.ts | 11 +- .../api/instance/database/importData.ts | 5 +- .../api/instance/deployments/getDeployment.ts | 24 +- src/integrations/api/sse/errors.ts | 8 +- src/integrations/api/sse/streamOperation.ts | 9 +- .../datadog/redactRelayedMessage.test.ts | 16 ++ .../datadog/redactRelayedMessage.ts | 5 +- 26 files changed, 1629 insertions(+), 77 deletions(-) create mode 100644 src/features/instance/applications/DESIGN.md create mode 100644 src/features/instance/applications/components/DeployProgress/describeDeployOutcome.ts create mode 100644 src/features/instance/applications/components/DeployProgress/useDeploymentStream.test.ts create mode 100644 src/features/instance/applications/modals/reportDeployHealth.test.ts create mode 100644 src/integrations/api/instance/applications/deployOutcome.test.ts create mode 100644 src/integrations/api/instance/applications/deployOutcome.ts create mode 100644 src/integrations/api/instance/applications/restartJob.test.ts create mode 100644 src/integrations/api/instance/applications/restartJob.ts diff --git a/DESIGN.md b/DESIGN.md index fd52bd729..fa10c50db 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -12,6 +12,7 @@ This file collects architecture and design notes for studio that aren't otherwis - [`src/features/instance/databases/DESIGN.md`](src/features/instance/databases/DESIGN.md) — when the browse grid may show its spinner, and why a table without a primary key can't be listed. +- [`src/features/instance/applications/DESIGN.md`](src/features/instance/applications/DESIGN.md) — when a rolling deploy is done: following its restart job, and why a job Studio can't follow is never a success. - [`src/features/instance/config/roles/DESIGN.md`](src/features/instance/config/roles/DESIGN.md) — why the role editor's JSON text is the only source of truth, what forces the JSON view, and what Save changes on purpose. - [`src/router/DESIGN.md`](src/router/DESIGN.md) — why a failed route load is reported and shown only by its route, and why a preload still toasts. diff --git a/src/features/instance/applications/DESIGN.md b/src/features/instance/applications/DESIGN.md new file mode 100644 index 000000000..92044799e --- /dev/null +++ b/src/features/instance/applications/DESIGN.md @@ -0,0 +1,32 @@ +# Applications — design notes + +## When a deploy is done + +Studio deploys with `restart: 'rolling'`. The node that receives the deploy answers once its own part is done and hands +the other nodes to a `restart_service` job, named in the response's `restartJobId` (Harper 4.6+). The deployment record +does not help: only the receiving node writes it, so it reads `success` while that job runs and after it fails. + +So [`deployAndFollowComponent`](../../../integrations/api/instance/applications/deployComponent.ts) follows the job with +`get_job` before the mutation settles ([`restartJob.ts`](../../../integrations/api/instance/applications/restartJob.ts)): + +- **A failed job rejects with `DeployRolloutError`.** The release is live on the receiving node; the error and the + progress panel name each node that did not take it. Its message quotes those nodes' errors, so Datadog withholds it + like `SSEOperationError`. +- **A job Studio could not follow resolves, as `rollout.state === 'unfollowable'`, and is never reported as success.** + [`reportDeployOutcome`](modals/reportDeployHealth.ts) skips the post-deploy health check for it, because that check + reads the one node Studio reaches, which can be healthy on the release a rejecting node kept. Any new caller of the + deploy mutation has to make the same distinction. +- **The job may not be there.** `hdb_job` is a non-replicating system table, so only the receiving node can answer for + it. Central manager's cluster proxy sends every operation to the first running instance, so over the proxy the deploy + and its polls reach the same node; a direct connection to a cluster's address can reach another node, which answers + `[]`. A row missing for 20 seconds counts as `not-found`. +- **Every node's outcome arrives at once.** Harper writes the job's row when it starts and when it ends, never in between. + +The job comes in two shapes. Harper 5.4's certified rollout activates the staged release on each other node, and each +certifies it with a canary of its own: `result.activated` on success, `message: { error, activated }` on failure. Older +versions, and a 5.4 deploy whose release was not armed for a canary, run the plain restart: `result.replicated`, where a +node the job could not reach is listed with its error and the job still completes, so that node reads `unconfirmed`. + +Canary certification is shown only when the response carries it, which only 5.4+ does. There is no version check on +the deploy path: `registration_info` is denied to a role with an operations allowlist, which is exactly the kind of +role that may be allowed to deploy, so a version gate there would hide the result from it. diff --git a/src/features/instance/applications/components/DeployProgress/DeployProgress.test.tsx b/src/features/instance/applications/components/DeployProgress/DeployProgress.test.tsx index 3cbd14dbc..70f582348 100644 --- a/src/features/instance/applications/components/DeployProgress/DeployProgress.test.tsx +++ b/src/features/instance/applications/components/DeployProgress/DeployProgress.test.tsx @@ -50,3 +50,136 @@ describe('DeployProgress', () => { expect(screen.queryByRole('link', { name: 'Manage SSH keys' })).toBeNull(); }); }); + +const DONE: DeploymentStreamState['phases'] = { + prepare: 'done', + load: 'done', + replicate: 'done', + restart: 'done', + success: 'done', +}; + +function deployedState(extra: Partial): DeploymentStreamState { + return { lifecycle: 'success', phases: DONE, installLog: [], peers: [], ...extra }; +} + +describe('DeployProgress canary certification (Harper 5.4+)', () => { + it('says a certified release was checked before it served', () => { + render(); + expect(screen.getByText('Certified')).toBeTruthy(); + expect(screen.getByText(/canary loaded the release before it served/)).toBeTruthy(); + }); + + it.each([ + ['uncertified', /went out unchecked: the component ran nothing as it loaded/], + ['unavailable', /restarted unchecked: no worker on this node loads the component/], + ])('says why an %s release went out unchecked', (certification, why) => { + render(); + expect(screen.getByText('Not certified')).toBeTruthy(); + expect(screen.getByText(why)).toBeTruthy(); + }); + + it('renders a status it does not know rather than hiding it', () => { + render(); + expect(screen.getByText('Not certified (withdrawn)')).toBeTruthy(); + }); + + it('shows nothing about certification for a version that does not report it', () => { + render(); + expect(screen.queryByText(/certified/i)).toBeNull(); + }); + + it('shows a rejection as the decision, with the release that went back, in place of the raw message', () => { + render( + , + ); + expect(screen.getByText(/canary rejected this release on the node that received the deploy/)).toBeTruthy(); + expect(screen.getByText('web.jsResource: boom')).toBeTruthy(); + expect(screen.getByText('Deployment a3f8, the release it replaced, is live there again.')).toBeTruthy(); + expect(screen.getByText('No other node received it.')).toBeTruthy(); + expect(screen.queryByText(/web was not deployed/)).toBeNull(); + }); + + it('says a rejected first release is failed closed', () => { + render( + , + ); + expect(screen.getByText(/restart stopped before the canary decided/)).toBeTruthy(); + expect(screen.getByText(/failed closed there until it is deployed again/)).toBeTruthy(); + }); +}); + +describe('DeployProgress warnings and rollout', () => { + it('keeps the warnings the deploy streamed', () => { + render(); + expect(screen.getByText('Warnings')).toBeTruthy(); + expect(screen.getByText('Install fingerprints differ on node-b')).toBeTruthy(); + }); + + it('says the deploy is still rolling out while the job runs', () => { + render( + , + ); + expect(screen.getByText(/Rolling out to the other nodes/)).toBeTruthy(); + }); + + it("names each node and how it took the release, including the one that didn't", () => { + render( + , + ); + expect(screen.getByText('node-b')).toBeTruthy(); + expect(screen.getByText('Certified')).toBeTruthy(); + expect(screen.getByText('node-c')).toBeTruthy(); + expect(screen.getByText('Did not take the release')).toBeTruthy(); + expect(screen.getByText('web was not deployed on this node: boom')).toBeTruthy(); + expect(screen.getByText(/was not activated on 1 of 2 peer node/)).toBeTruthy(); + }); + + it("says it can't tell, rather than claiming success, when it could not follow the job", () => { + render( + , + ); + expect(screen.getByText(/couldn't find this deploy's restart job on the node it reached/)).toBeTruthy(); + }); + + it('shows nothing for a single instance, which has no other nodes', () => { + render(); + expect(screen.queryByText('Other nodes')).toBeNull(); + }); + + it("links to the deployment's record, closing a hosting modal first", () => { + const onNavigateAway = vi.fn(); + render(); + const link = screen.getByRole('link', { name: "View this deployment's record" }); + expect(link.getAttribute('href')).toBe('/org-1/instance/ins-1/config/deployments/d1'); + fireEvent.click(link); + expect(onNavigateAway).toHaveBeenCalledOnce(); + }); +}); diff --git a/src/features/instance/applications/components/DeployProgress/DeployProgress.tsx b/src/features/instance/applications/components/DeployProgress/DeployProgress.tsx index 56d15da16..b9cf64676 100644 --- a/src/features/instance/applications/components/DeployProgress/DeployProgress.tsx +++ b/src/features/instance/applications/components/DeployProgress/DeployProgress.tsx @@ -2,12 +2,28 @@ import { Badge } from '@/components/ui/badge'; import { ScrollArea } from '@/components/ui/scrollArea'; import { Separator } from '@/components/ui/separator'; import { DEPLOY_PHASE_ORDER } from '@/integrations/api/instance/applications/deployComponentStream'; +import { CertificationDecision } from '@/integrations/api/instance/applications/deployOutcome'; +import { RolloutNode, RolloutSnapshot } from '@/integrations/api/instance/applications/restartJob'; import { cn } from '@/lib/cn'; import { errorText } from '@/lib/errorText'; import { buildAbsoluteLinkToPage } from '@/lib/urls/buildAbsoluteLinkToPage'; import { Link, useParams } from '@tanstack/react-router'; -import { CheckIcon, CircleDashedIcon, CircleIcon, Loader2Icon, XIcon } from 'lucide-react'; +import { + CheckIcon, + CircleDashedIcon, + CircleIcon, + Loader2Icon, + ShieldCheckIcon, + TriangleAlertIcon, + XIcon, +} from 'lucide-react'; import { useEffect, useRef } from 'react'; +import { + describeCertification, + describeDecision, + describeRolloutNode, + describeUnfollowable, +} from './describeDeployOutcome'; import { DeploymentStreamState, PhaseStatus } from './useDeploymentStream'; const PHASE_LABELS: Record = { @@ -122,7 +138,28 @@ export function DeployProgress({
)} - {phasesFailed && state.error && ( + {!!state.warnings?.length && ( +
+ +
Warnings
+
    + {state.warnings.map((warning, i) => ( +
  • + + {warning} +
    • + ))} +
+
+ )} + + {state.certification && } + + {state.rollout && } + + {phasesFailed && state.decision && } + + {phasesFailed && state.error && !state.decision && (
@@ -145,6 +182,102 @@ export function DeployProgress({ Live updates ended early — verifying… )} + + {state.deploymentId && ( + + View this deployment's record + + )} +
+ ); +} + +function CertificationLine({ certification }: { certification: string }) { + const { label, detail, certified } = describeCertification(certification); + return ( +
+ {certified + ? + : } + + {label} + {detail && {' '}{detail}} +
); } + +function DecisionBlock({ decision }: { decision: CertificationDecision }) { + const { title, reason, outcome } = describeDecision(decision); + return ( +
+ {title} + {reason && {reason}} + {outcome && {outcome}} + No other node received it. +
+ ); +} + +function RolloutSection({ rollout }: { rollout: RolloutSnapshot }) { + const unfollowable = describeUnfollowable(rollout); + if (rollout.state === 'complete' && rollout.nodes.length === 0) { + return null; + } + return ( +
+ +
Other nodes
+ {rollout.state === 'following' && ( +
+ + Rolling out to the other nodes, one at a time… +
+ )} + {!!rollout.nodes.length && ( +
    + {rollout.nodes.map((node, i) => { + const { label, detail } = describeRolloutNode(node); + return ( +
  • + + + {node.node} + {label} + + {detail && ( + + {detail} + + )} +
    • + ); + })} +
+ )} + {rollout.state === 'failed' && rollout.message && ( +
{rollout.message}
+ )} + {unfollowable && ( +
+ + {unfollowable} +
+ )} +
+ ); +} + +function RolloutNodeIcon({ outcome }: { outcome: RolloutNode['outcome'] }) { + if (outcome === 'failed') { + return ; + } + if (outcome === 'unconfirmed') { + return ; + } + return ; +} diff --git a/src/features/instance/applications/components/DeployProgress/describeDeployOutcome.ts b/src/features/instance/applications/components/DeployProgress/describeDeployOutcome.ts new file mode 100644 index 000000000..ee702fa42 --- /dev/null +++ b/src/features/instance/applications/components/DeployProgress/describeDeployOutcome.ts @@ -0,0 +1,88 @@ +import { CertificationDecision } from '@/integrations/api/instance/applications/deployOutcome'; +import { RolloutNode, RolloutSnapshot } from '@/integrations/api/instance/applications/restartJob'; + +export interface CertificationSummary { + label: string; + /** Why it went out unchecked, when it did. */ + detail?: string; + certified: boolean; +} + +/** Harper's `certification` string (5.4.0+), as a person reads it. */ +export function describeCertification(certification: string): CertificationSummary { + switch (certification) { + case 'certified': + return { label: 'Certified', detail: 'Its canary loaded the release before it served.', certified: true }; + case 'uncertified': + return { + label: 'Not certified', + detail: 'It went out unchecked: the component ran nothing as it loaded (dev-only loading, an absent ' + + 'if-installed component, or safe mode), or no worker was replaced.', + certified: false, + }; + case 'unavailable': + return { + label: 'Not certified', + detail: 'It restarted unchecked: no worker on this node loads the component, its isolated setting ' + + 'changed with this deploy, it was already live, or it was deployed from a local directory.', + certified: false, + }; + case 'not-requested': + return { label: 'Not certified', detail: 'The deploy did not restart, so nothing checked it.', certified: false }; + default: + return { label: `Not certified (${certification})`, certified: false }; + } +} + +export interface DecisionSummary { + title: string; + reason?: string; + outcome?: string; +} + +/** A canary's refusal of a release on the node that received the deploy. */ +export function describeDecision(decision: CertificationDecision): DecisionSummary { + const title = decision.status === 'rejected' + ? 'Its canary rejected this release on the node that received the deploy.' + : decision.status === 'interrupted' + ? 'Its restart stopped before the canary decided, on the node that received the deploy.' + : `It was not certified (${decision.status}) on the node that received the deploy.`; + const outcome = decision.restored + ? `Deployment ${decision.restored}, the release it replaced, is live there again.` + : decision.failedClosed + ? 'There was no earlier release to restore, so the component is failed closed there until it is deployed again.' + : undefined; + return { title, reason: decision.reason, outcome }; +} + +/** Why Studio could not say how the other nodes took the release. */ +export function describeUnfollowable(rollout: RolloutSnapshot): string | undefined { + switch (rollout.unfollowable) { + case 'not-found': + return "Studio couldn't find this deploy's restart job on the node it reached, so it can't tell how the " + + 'other nodes took the release. Check their status before relying on it.'; + case 'refused': + return "Studio couldn't read this deploy's restart job, so it can't tell how the other nodes took the " + + 'release. Check their status before relying on it.'; + case 'timed-out': + return 'The other nodes were still restarting after 30 minutes, so Studio stopped waiting. The restart may ' + + 'still finish; check their status before relying on it.'; + } + return undefined; +} + +/** One node's line in the rollout list. */ +export function describeRolloutNode(node: RolloutNode): { label: string; detail?: string } { + switch (node.outcome) { + case 'activated': + return node.certification + ? { label: describeCertification(node.certification).label } + : { label: 'Took the release' }; + case 'restarted': + return { label: 'Restarted' }; + case 'failed': + return { label: 'Did not take the release', detail: node.detail }; + case 'unconfirmed': + return { label: 'Restart not confirmed', detail: node.detail }; + } +} diff --git a/src/features/instance/applications/components/DeployProgress/useDeploymentStream.test.ts b/src/features/instance/applications/components/DeployProgress/useDeploymentStream.test.ts new file mode 100644 index 000000000..159988a05 --- /dev/null +++ b/src/features/instance/applications/components/DeployProgress/useDeploymentStream.test.ts @@ -0,0 +1,64 @@ +/** @vitest-environment jsdom */ +import { SSEOperationError } from '@/integrations/api/sse/errors'; +import { act, renderHook } from '@testing-library/react'; +import { describe, expect, it } from 'vitest'; +import { useDeploymentStream } from './useDeploymentStream'; + +describe('useDeploymentStream', () => { + it('keeps streamed warnings and the latest rollout snapshot', () => { + const { result } = renderHook(() => useDeploymentStream()); + act(() => { + result.current.markStarted(); + result.current.onEvent({ type: 'warning', data: { message: 'node-b installed npm:web@1.5.0' } }); + result.current.onEvent({ type: 'warning', data: {} }); + result.current.onEvent({ type: 'rollout', data: { jobId: 'j', state: 'following', nodes: [] } }); + result.current.onEvent({ type: 'rollout', data: { jobId: 'j', state: 'complete', nodes: [] } }); + }); + expect(result.current.state.warnings).toEqual(['node-b installed npm:web@1.5.0']); + expect(result.current.state.rollout?.state).toBe('complete'); + }); + + it("takes the receiving node's certification and the record id from a deploy that answered", () => { + const { result } = renderHook(() => useDeploymentStream()); + act(() => { + result.current.markStarted(); + result.current.markSettled('success', undefined, { + result: { message: 'ok', deployment_id: 'd1', certification: 'certified' }, + }); + }); + expect(result.current.state).toMatchObject({ + lifecycle: 'success', + certification: 'certified', + deploymentId: 'd1', + }); + }); + + it("takes the canary's decision and the record id from a deploy it rejected", () => { + const { result } = renderHook(() => useDeploymentStream()); + const failure = new SSEOperationError('web was not deployed: …', { + deploymentId: 'd1', + certification: { status: 'rejected', reason: 'boom', restored: 'd0', failed_closed: false }, + }); + act(() => { + result.current.markStarted(); + result.current.markSettled('error', failure.message, { failure }); + }); + expect(result.current.state).toMatchObject({ + lifecycle: 'error', + error: 'web was not deployed: …', + deploymentId: 'd1', + decision: { status: 'rejected', reason: 'boom', restored: 'd0', failedClosed: false }, + }); + }); + + it('starts each deploy clean', () => { + const { result } = renderHook(() => useDeploymentStream()); + act(() => { + result.current.onEvent({ type: 'warning', data: { message: 'old' } }); + result.current.markSettled('success', undefined, { result: { message: 'ok', deployment_id: 'd1' } }); + result.current.markStarted(); + }); + expect(result.current.state.warnings).toBeUndefined(); + expect(result.current.state.deploymentId).toBeUndefined(); + }); +}); diff --git a/src/features/instance/applications/components/DeployProgress/useDeploymentStream.ts b/src/features/instance/applications/components/DeployProgress/useDeploymentStream.ts index af6b80954..62b7424a6 100644 --- a/src/features/instance/applications/components/DeployProgress/useDeploymentStream.ts +++ b/src/features/instance/applications/components/DeployProgress/useDeploymentStream.ts @@ -1,4 +1,14 @@ -import { DeployPeerEvent, DeployStreamEvent } from '@/integrations/api/instance/applications/deployComponentStream'; +import { + DeployComponentResult, + DeployPeerEvent, + DeployStreamEvent, +} from '@/integrations/api/instance/applications/deployComponentStream'; +import { + CertificationDecision, + DeployCertification, + deployFailureDetails, +} from '@/integrations/api/instance/applications/deployOutcome'; +import { RolloutSnapshot } from '@/integrations/api/instance/applications/restartJob'; import { useCallback, useMemo, useReducer } from 'react'; export type DeploymentLifecycle = 'idle' | 'streaming' | 'success' | 'error' | 'inconclusive'; @@ -18,6 +28,13 @@ export interface DeploymentStreamState { currentPhase?: string; installLog: InstallLine[]; peers: DeployPeerEvent[]; + warnings?: string[]; + rollout?: RolloutSnapshot; + /** How the node that received the deploy checked the release, once it answered. */ + certification?: DeployCertification; + /** Why that node's canary refused the release. */ + decision?: CertificationDecision; + deploymentId?: string; error?: string; } @@ -32,11 +49,35 @@ const INITIAL_STATE: DeploymentStreamState = { // limit; the server already aggregates, this is a UI safety net. const MAX_INSTALL_LINES = 1000; +type Settled = Extract; + type Action = | { kind: 'reset' } | { kind: 'start' } | { kind: 'event'; event: DeployStreamEvent } - | { kind: 'settled'; lifecycle: Extract; error?: string }; + | { kind: 'settled'; lifecycle: Settled; error?: string; outcome?: SettledOutcome }; + +/** What the deploy answered, or failed with, beyond its lifecycle. */ +export interface SettledOutcome { + result?: DeployComponentResult; + failure?: unknown; +} + +function settledDetails(outcome: SettledOutcome | undefined): Partial { + if (outcome?.result) { + const { certification, deployment_id, rollout } = outcome.result; + return { + certification: typeof certification === 'string' ? certification : undefined, + deploymentId: deployment_id, + ...(rollout ? { rollout } : {}), + }; + } + if (outcome?.failure !== undefined) { + const { deploymentId, certification } = deployFailureDetails(outcome.failure); + return { deploymentId, decision: certification }; + } + return {}; +} function reducer(state: DeploymentStreamState, action: Action): DeploymentStreamState { switch (action.kind) { @@ -64,11 +105,20 @@ function reducer(state: DeploymentStreamState, action: Action): DeploymentStream installLog: next.length > MAX_INSTALL_LINES ? next.slice(-MAX_INSTALL_LINES) : next, }; } + if (event.type === 'warning') { + const { message } = event.data; + return typeof message === 'string' && message + ? { ...state, warnings: (state.warnings ?? []).concat(message) } + : state; + } + if (event.type === 'rollout') { + return { ...state, rollout: event.data }; + } // peer return { ...state, peers: state.peers.concat(event.data) }; } case 'settled': - return { ...state, lifecycle: action.lifecycle, error: action.error }; + return { ...state, ...settledDetails(action.outcome), lifecycle: action.lifecycle, error: action.error }; } } @@ -87,8 +137,8 @@ export function useDeploymentStream() { const markStarted = useCallback(() => dispatch({ kind: 'start' }), []); const reset = useCallback(() => dispatch({ kind: 'reset' }), []); const markSettled = useCallback( - (lifecycle: 'success' | 'error' | 'inconclusive', error?: string) => - dispatch({ kind: 'settled', lifecycle, error }), + (lifecycle: Settled, error?: string, outcome?: SettledOutcome) => + dispatch({ kind: 'settled', lifecycle, error, outcome }), [], ); diff --git a/src/features/instance/applications/components/NewApplication/useImportApplication.ts b/src/features/instance/applications/components/NewApplication/useImportApplication.ts index 23d738864..58c77e4a3 100644 --- a/src/features/instance/applications/components/NewApplication/useImportApplication.ts +++ b/src/features/instance/applications/components/NewApplication/useImportApplication.ts @@ -2,8 +2,9 @@ import { useInstanceClientIdParams } from '@/config/useInstanceClient'; import { useComponentHealthCheck } from '@/features/instance/applications/hooks/useComponentHealthCheck'; import { useEditorView } from '@/features/instance/applications/hooks/useEditorView'; import { useSupportsDeploymentSSE } from '@/features/instance/applications/hooks/useSupportsDeploymentSSE'; -import { reportDeployHealth } from '@/features/instance/applications/modals/reportDeployHealth'; +import { reportDeployOutcome } from '@/features/instance/applications/modals/reportDeployHealth'; import { useDeployComponentMutation } from '@/integrations/api/instance/applications/deployComponent'; +import { DeployRolloutError } from '@/integrations/api/instance/applications/restartJob'; import { SSEInconclusiveError } from '@/integrations/api/sse/errors'; import { invalidateEntityQueries } from '@/react-query/invalidateEntityQueries'; import { useQueryClient } from '@tanstack/react-query'; @@ -79,15 +80,21 @@ export function useImportApplication( description: event.data.line.trim().slice(0, 140), duration: 300_000, }); + } else if (event.type === 'rollout' && event.data.state === 'following') { + toast.loading(`Importing ${project}…`, { + id: toastId, + description: 'Rolling out to the other nodes…', + duration: Infinity, + }); } }, ...instanceParams, }, { - onSuccess: async () => { + onSuccess: async (result) => { toast.dismiss(toastId); onImported(project); - if (sseDeploy) { - reportDeployHealth(project, await runHealthCheck(project)); + if (sseDeploy || result?.rollout?.state === 'unfollowable') { + await reportDeployOutcome(project, result, runHealthCheck); } else { toast.success(`Imported successfully`, { description: `${project} will be available once you restart your ${instanceParams.entityType}!`, @@ -100,7 +107,12 @@ export function useImportApplication( // Live stream dropped after the deploy started — verify rather than assume failure. if (error instanceof SSEInconclusiveError) { onImported(project); - reportDeployHealth(project, await runHealthCheck(project)); + await reportDeployOutcome(project, undefined, runHealthCheck); + return; + } + if (error instanceof DeployRolloutError) { + onImported(project); + toast.error(`${project} did not reach every node`, { description: error.message }); return; } toast.error(`Failed to import ${project}`, { diff --git a/src/features/instance/applications/modals/RedeployApplicationModal.tsx b/src/features/instance/applications/modals/RedeployApplicationModal.tsx index 1e9fe6d65..7b5d25e41 100644 --- a/src/features/instance/applications/modals/RedeployApplicationModal.tsx +++ b/src/features/instance/applications/modals/RedeployApplicationModal.tsx @@ -9,11 +9,13 @@ import { FormMessage } from '@/components/ui/form/FormMessage'; import { Input } from '@/components/ui/input'; import { useInstanceClientIdParams } from '@/config/useInstanceClient'; import { DeployProgress } from '@/features/instance/applications/components/DeployProgress/DeployProgress'; +import { describeUnfollowable } from '@/features/instance/applications/components/DeployProgress/describeDeployOutcome'; import { useDeploymentStream } from '@/features/instance/applications/components/DeployProgress/useDeploymentStream'; import { useComponentHealthCheck } from '@/features/instance/applications/hooks/useComponentHealthCheck'; import { useEditorView } from '@/features/instance/applications/hooks/useEditorView'; import { useSupportsDeploymentSSE } from '@/features/instance/applications/hooks/useSupportsDeploymentSSE'; import { useDeployComponentMutation } from '@/integrations/api/instance/applications/deployComponent'; +import { DeployRolloutError } from '@/integrations/api/instance/applications/restartJob'; import { SSEInconclusiveError } from '@/integrations/api/sse/errors'; import { attemptToRestoreFocus } from '@/lib/attemptToRestoreFocus'; import { setWatchedValue, useWatchedValue } from '@/lib/events/watcher'; @@ -22,7 +24,7 @@ import { RefreshCwIcon } from 'lucide-react'; import { useCallback, useEffect, useState } from 'react'; import { useForm } from 'react-hook-form'; import { toast } from 'sonner'; -import { reportDeployHealth } from './reportDeployHealth'; +import { deployNotes, reportDeployOutcome } from './reportDeployHealth'; export function RedeployApplicationModal() { const { value: isModalOpen, trigger } = useWatchedValue('ShowRedeployApplicationModal', false); @@ -87,12 +89,14 @@ export function RedeployApplicationModal() { ...instanceParams, }, { - onSuccess: async () => { - stream.markSettled('success'); + onSuccess: async (result) => { + stream.markSettled('success', undefined, { result }); invalidateComponents(); - const health = await runHealthCheck(project); - reportDeployHealth(project, health); - closeModal(); + const known = await reportDeployOutcome(project, result, runHealthCheck); + // Left open when there is something to read in it: an outcome Studio couldn't confirm, or a warning. + if (known && !deployNotes(result).warn) { + closeModal(); + } }, onError: async (error) => { // The deploy started but the live stream dropped — verify via health check @@ -100,12 +104,20 @@ export function RedeployApplicationModal() { if (error instanceof SSEInconclusiveError) { stream.markSettled('inconclusive'); invalidateComponents(); - const health = await runHealthCheck(project); - reportDeployHealth(project, health); + await reportDeployOutcome(project, undefined, runHealthCheck); closeModal(); return; } - stream.markSettled('error', error instanceof Error ? error.message : String(error)); + if (error instanceof DeployRolloutError) { + // Live on the node that received it; the rollout section names the nodes that did not take it. + stream.markSettled('error', undefined, { failure: error }); + invalidateComponents(); + toast.error(`${project} did not reach every node`, { description: error.message }); + return; + } + stream.markSettled('error', error instanceof Error ? error.message : String(error), { + failure: error, + }); toast.error(`Failed to redeploy ${project}`, { description: error instanceof Error ? error.message : undefined, }); @@ -126,11 +138,26 @@ export function RedeployApplicationModal() { reDeployApplication( { applicationName: openedEntry.project, applicationUrl, installCommand, ...instanceParams }, { - onSuccess: () => { - toast.success(`Application ${openedEntry.project} redeployed successfully`, { id: toastId }); + onSuccess: (result) => { + if (result?.rollout?.state === 'unfollowable') { + toast.warning(`${openedEntry.project} redeployed on the node that received it`, { + id: toastId, + description: describeUnfollowable(result.rollout), + }); + } else { + toast.success(`Application ${openedEntry.project} redeployed successfully`, { id: toastId }); + } invalidateComponents(); }, - onError: () => { + onError: (error) => { + if (error instanceof DeployRolloutError) { + toast.error(`${openedEntry.project} did not reach every node`, { + id: toastId, + description: error.message, + }); + invalidateComponents(); + return; + } toast.dismiss(toastId); }, }, diff --git a/src/features/instance/applications/modals/reportDeployHealth.test.ts b/src/features/instance/applications/modals/reportDeployHealth.test.ts new file mode 100644 index 000000000..4b939e62c --- /dev/null +++ b/src/features/instance/applications/modals/reportDeployHealth.test.ts @@ -0,0 +1,75 @@ +import { afterEach, describe, expect, it, vi } from 'vitest'; + +const toast = vi.hoisted(() => ({ success: vi.fn(), warning: vi.fn(), error: vi.fn() })); +vi.mock('sonner', () => ({ toast })); + +import { DeployComponentResult } from '@/integrations/api/instance/applications/deployComponentStream'; +import { deployNotes, reportDeployOutcome } from './reportDeployHealth'; + +afterEach(() => { + vi.clearAllMocks(); +}); + +const healthy = vi.fn(async () => ({ level: 'healthy' as const })); + +describe('reportDeployOutcome', () => { + it("never runs the one-node health check, or reports success, when the other nodes' outcome is unknown", async () => { + const result: DeployComponentResult = { + message: 'Successfully deployed: web, restarting Harper', + restartJobId: 'job-1', + rollout: { jobId: 'job-1', state: 'unfollowable', nodes: [], unfollowable: 'not-found' }, + }; + expect(await reportDeployOutcome('web', result, healthy)).toBe(false); + expect(healthy).not.toHaveBeenCalled(); + expect(toast.success).not.toHaveBeenCalled(); + expect(toast.warning).toHaveBeenCalledWith('web deployed on the node that received it', { + description: expect.stringContaining("can't tell how the other nodes took the release"), + }); + }); + + it('reports health once every node took the release', async () => { + const result: DeployComponentResult = { + message: 'ok', + certification: 'certified', + rollout: { jobId: 'job-1', state: 'complete', nodes: [{ node: 'node-b', outcome: 'activated' }] }, + }; + expect(await reportDeployOutcome('web', result, healthy)).toBe(true); + expect(toast.success).toHaveBeenCalledWith('web deployed and healthy', { description: undefined }); + }); + + it('still verifies a deploy whose stream ended without saying', async () => { + expect(await reportDeployOutcome('web', undefined, healthy)).toBe(true); + expect(healthy).toHaveBeenCalledWith('web'); + }); +}); + +describe('deployNotes', () => { + it('explains an unchecked release without calling it a problem', () => { + const notes = deployNotes({ message: 'ok', certification: 'uncertified' }); + expect(notes.warn).toBe(false); + expect(notes.lines).toEqual([expect.stringMatching(/^Not certified: It went out unchecked/)]); + }); + + it('flags warnings and nodes whose restart was not confirmed', () => { + const notes = deployNotes({ + message: 'ok', + warnings: ['Install fingerprints differ on node-b'], + rollout: { + jobId: 'job-1', + state: 'complete', + nodes: [ + { node: 'node-b', outcome: 'restarted' }, + { node: 'node-c', outcome: 'unconfirmed', detail: 'connect ECONNREFUSED' }, + ], + }, + }); + expect(notes).toEqual({ + lines: ['Install fingerprints differ on node-b', 'The restart was not confirmed on node-c.'], + warn: true, + }); + }); + + it('has nothing to add for a version that reports none of it', () => { + expect(deployNotes({ message: 'Successfully deployed: web' })).toEqual({ lines: [], warn: false }); + }); +}); diff --git a/src/features/instance/applications/modals/reportDeployHealth.ts b/src/features/instance/applications/modals/reportDeployHealth.ts index 9534d26c7..0abbd7a6b 100644 --- a/src/features/instance/applications/modals/reportDeployHealth.ts +++ b/src/features/instance/applications/modals/reportDeployHealth.ts @@ -1,6 +1,42 @@ +import { + describeCertification, + describeUnfollowable, +} from '@/features/instance/applications/components/DeployProgress/describeDeployOutcome'; import { ComponentHealthResult } from '@/features/instance/applications/hooks/useComponentHealthCheck'; +import { DeployComponentResult } from '@/integrations/api/instance/applications/deployComponentStream'; import { toast } from 'sonner'; +/** What else the deploy said, beyond the component's health. `warn` when a person should look at it. */ +export interface DeployNotes { + lines: string[]; + warn: boolean; +} + +export function deployNotes(result: DeployComponentResult | undefined): DeployNotes { + const lines: string[] = []; + let warn = false; + if (typeof result?.certification === 'string') { + const { label, detail, certified } = describeCertification(result.certification); + if (!certified) { + lines.push(detail ? `${label}: ${detail}` : `${label}.`); + } + } + for (const warning of result?.warnings ?? []) { + lines.push(warning); + warn = true; + } + const unconfirmed = result?.rollout?.nodes.filter((node) => node.outcome === 'unconfirmed') ?? []; + if (unconfirmed.length) { + lines.push(`The restart was not confirmed on ${unconfirmed.map((node) => node.node).join(', ')}.`); + warn = true; + } + return { lines, warn }; +} + +function withNotes(message: string | undefined, notes: DeployNotes): string | undefined { + return [message, ...notes.lines].filter(Boolean).join(' ') || undefined; +} + /** * Surface the post-deploy health-check result as a toast. * @@ -8,19 +44,27 @@ import { toast } from 'sonner'; * threads, so a successful deploy call doesn't guarantee a working component (#1233). This * translates the polled status into clear user feedback. */ -export function reportDeployHealth(project: string, health: ComponentHealthResult): void { +export function reportDeployHealth( + project: string, + health: ComponentHealthResult, + notes: DeployNotes = { lines: [], warn: false }, +): void { switch (health.level) { case 'healthy': - toast.success(`${project} deployed and healthy`, { description: health.message }); + if (notes.warn) { + toast.warning(`${project} deployed with warnings`, { description: withNotes(health.message, notes) }); + return; + } + toast.success(`${project} deployed and healthy`, { description: withNotes(health.message, notes) }); return; case 'warning': toast.warning(`${project} deployed with warnings`, { - description: health.message ?? 'The component loaded but reported a warning.', + description: withNotes(health.message ?? 'The component loaded but reported a warning.', notes), }); return; case 'error': toast.error(`${project} deployed but failed to load`, { - description: health.message ?? 'The component reported an error after deploying.', + description: withNotes(health.message ?? 'The component reported an error after deploying.', notes), }); return; case 'loading': @@ -28,8 +72,30 @@ export function reportDeployHealth(project: string, health: ComponentHealthResul case 'indeterminate': default: toast.success(`${project} deployed`, { - description: 'Still verifying — the component is loading. Check its status shortly.', + description: withNotes('Still verifying — the component is loading. Check its status shortly.', notes), }); return; } } + +/** + * Report a deploy that resolved, or whose stream ended without saying (`result` undefined). + * + * A rollout Studio could not follow skips the health check: that reads the one node Studio reaches, + * which can be healthy on the release a rejecting node kept — exactly what following the job is for. + * Resolves `false` when the outcome is unknown. + */ +export async function reportDeployOutcome( + project: string, + result: DeployComponentResult | undefined, + runHealthCheck: (project: string) => Promise, +): Promise { + if (result?.rollout?.state === 'unfollowable') { + toast.warning(`${project} deployed on the node that received it`, { + description: describeUnfollowable(result.rollout), + }); + return false; + } + reportDeployHealth(project, await runHealthCheck(project), deployNotes(result)); + return true; +} diff --git a/src/integrations/api/instance/applications/deployComponent.test.ts b/src/integrations/api/instance/applications/deployComponent.test.ts index 0d655d978..13b54493d 100644 --- a/src/integrations/api/instance/applications/deployComponent.test.ts +++ b/src/integrations/api/instance/applications/deployComponent.test.ts @@ -1,12 +1,13 @@ -import { afterEach, describe, expect, it, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; vi.mock('./deployComponentStream', () => ({ deployComponentStream: vi.fn(), })); import { SSEOperationError, SSEUnsupportedError } from '@/integrations/api/sse/errors'; -import { onDeployComponentSubmit } from './deployComponent'; +import { deployAndFollowComponent, onDeployComponentSubmit } from './deployComponent'; import { deployComponentStream } from './deployComponentStream'; +import { DeployRolloutError } from './restartJob'; const mockStream = vi.mocked(deployComponentStream); @@ -79,3 +80,110 @@ describe('onDeployComponentSubmit', () => { expect(client.post).not.toHaveBeenCalled(); }); }); + +describe('deployAndFollowComponent', () => { + beforeEach(() => vi.useFakeTimers()); + afterEach(() => vi.useRealTimers()); + + function rolloutClient(deployed: object, jobRows: object[][]) { + let polls = 0; + return vi.fn((_url: string, body: { operation: string }) => { + if (body.operation === 'deploy_component') { + return Promise.resolve({ data: deployed }); + } + const rows = jobRows[Math.min(polls, jobRows.length - 1)]; + polls += 1; + return Promise.resolve({ data: rows }); + }); + } + + it('does not poll anything for a deploy that handed no restart to a job', async () => { + const post = rolloutClient({ message: 'Successfully deployed: my-app' }, []); + const onEvent = vi.fn(); + const result = await deployAndFollowComponent({ + ...baseArgs, + onEvent, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + instanceClient: { post } as any, + }); + expect(result).toEqual({ message: 'Successfully deployed: my-app' }); + expect(post).toHaveBeenCalledOnce(); + expect(onEvent).not.toHaveBeenCalled(); + expect(vi.getTimerCount()).toBe(0); + }); + + it('is not done until the restart job says how the other nodes took the release', async () => { + const post = rolloutClient( + { message: 'Successfully deployed: my-app, activating it', deployment_id: 'd1', restartJobId: 'job-1' }, + [ + [{ id: 'job-1', status: 'IN_PROGRESS' }], + [{ id: 'job-1', status: 'COMPLETE', result: { activated: [{ node: 'node-b', certification: 'certified' }] } }], + ], + ); + const onEvent = vi.fn(); + let settled = false; + const pending = deployAndFollowComponent({ + ...baseArgs, + onEvent, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + instanceClient: { post } as any, + }).finally(() => (settled = true)); + await vi.advanceTimersByTimeAsync(0); + expect(settled).toBe(false); + await vi.advanceTimersByTimeAsync(5_000); + const result = await pending; + expect(result.rollout).toMatchObject({ state: 'complete', nodes: [{ node: 'node-b', outcome: 'activated' }] }); + expect(post).toHaveBeenCalledWith('/', { operation: 'get_job', id: 'job-1' }); + expect(onEvent.mock.calls.map(([event]) => [event.type, event.data.state])).toEqual([ + ['rollout', 'following'], + ['rollout', 'complete'], + ]); + }); + + it("fails as a rollout failure, keeping the deployment's id, when another node rejects the release", async () => { + const post = rolloutClient({ message: 'ok', deployment_id: 'd1', restartJobId: 'job-1' }, [ + [{ + id: 'job-1', + status: 'ERROR', + message: { error: 'not activated on node-c', activated: [{ node: 'node-c', error: 'boom' }] }, + }], + ]); + const pending = deployAndFollowComponent({ + ...baseArgs, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + instanceClient: { post } as any, + }); + pending.catch(() => {}); + await vi.advanceTimersByTimeAsync(1_000); + await expect(pending).rejects.toBeInstanceOf(DeployRolloutError); + await expect(pending).rejects.toMatchObject({ message: 'not activated on node-c', deploymentId: 'd1' }); + // Following the job never deploys again. + expect(post.mock.calls.filter(([, body]) => body.operation === 'deploy_component')).toHaveLength(1); + }); + + it('resolves an unfollowable rollout as unknown rather than as a success or a failure', async () => { + const post = rolloutClient({ message: 'ok', restartJobId: 'job-1' }, [[]]); + const pending = deployAndFollowComponent({ + ...baseArgs, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + instanceClient: { post } as any, + }); + await vi.advanceTimersByTimeAsync(30_000); + expect((await pending).rollout).toMatchObject({ state: 'unfollowable', unfollowable: 'not-found' }); + }); + + it('follows a streamed deploy the same way', async () => { + mockStream.mockResolvedValue({ message: 'ok', restartJobId: 'job-1', certification: 'certified' }); + const post = rolloutClient({}, [[{ id: 'job-1', status: 'COMPLETE', result: { replicated: [] } }]]); + const pending = deployAndFollowComponent({ + ...baseArgs, + useSSE: true, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + instanceClient: { post } as any, + }); + await vi.advanceTimersByTimeAsync(1_000); + const result = await pending; + expect(result).toMatchObject({ certification: 'certified', rollout: { state: 'complete' } }); + expect(post.mock.calls.map(([, body]) => body.operation)).toEqual(['get_job']); + }); +}); diff --git a/src/integrations/api/instance/applications/deployComponent.ts b/src/integrations/api/instance/applications/deployComponent.ts index 1bad65093..a9fab9b35 100644 --- a/src/integrations/api/instance/applications/deployComponent.ts +++ b/src/integrations/api/instance/applications/deployComponent.ts @@ -4,6 +4,7 @@ import { ReplicatedResponse } from '@/integrations/api/replication'; import { SSEUnsupportedError } from '@/integrations/api/sse/errors'; import { useMutation } from '@tanstack/react-query'; import { DeployComponentResult, deployComponentStream, DeployStreamEvent } from './deployComponentStream'; +import { DeployRolloutError, followRestartJob } from './restartJob'; export interface DeployComponentFormData { applicationName: string; @@ -74,8 +75,33 @@ export async function onDeployComponentSubmit({ return data as ReplicatedResponse; } +/** + * Deploy, then — when the deploy handed its rolling restart to a job — follow that job, so the + * deploy is not done until every other node has taken the release, or said why it did not. + * + * A failed job rejects with {@link DeployRolloutError}. A job Studio could not follow resolves with + * `rollout.state === 'unfollowable'`, which no caller may report as success. + */ +export async function deployAndFollowComponent(args: DeployComponentArgs): Promise { + const result = await onDeployComponentSubmit(args); + const jobId = result?.restartJobId; + if (typeof jobId !== 'string' || !jobId) { + return result; + } + const rollout = await followRestartJob({ + instanceClient: args.instanceClient, + jobId, + signal: args.signal, + onUpdate: (snapshot) => args.onEvent?.({ type: 'rollout', data: snapshot }), + }); + if (rollout.state === 'failed') { + throw new DeployRolloutError(rollout, result.deployment_id); + } + return { ...result, rollout }; +} + export function useDeployComponentMutation() { return useMutation({ - mutationFn: onDeployComponentSubmit, + mutationFn: deployAndFollowComponent, }); } diff --git a/src/integrations/api/instance/applications/deployComponentStream.ts b/src/integrations/api/instance/applications/deployComponentStream.ts index 57cfe4bec..24537b696 100644 --- a/src/integrations/api/instance/applications/deployComponentStream.ts +++ b/src/integrations/api/instance/applications/deployComponentStream.ts @@ -1,7 +1,9 @@ import { ReplicatedResponse } from '@/integrations/api/replication'; -import { parseSSEData } from '@/integrations/api/sse/parseSSEStream'; +import { parseSSEData, SSEMessage } from '@/integrations/api/sse/parseSSEStream'; import { ResolveInstanceConnectionParams } from '@/integrations/api/sse/resolveInstanceConnection'; import { streamOperation } from '@/integrations/api/sse/streamOperation'; +import { DeployCertification } from './deployOutcome'; +import { RolloutSnapshot } from './restartJob'; /** Lifecycle phases Harper's `deploy_component` emits, in order. */ export const DEPLOY_PHASE_ORDER = ['prepare', 'load', 'replicate', 'restart', 'success'] as const; @@ -29,15 +31,52 @@ export interface DeployPeerEvent { [key: string]: unknown; } -/** A live deploy event, discriminated by `type`, surfaced to the UI via `onEvent`. */ +/** Something the deploy wants a person to know without failing, e.g. a peer that installed different code (5.3.1+). */ +export interface DeployWarningEvent { + message?: string; +} + +/** + * A live deploy event, discriminated by `type`, surfaced to the UI via `onEvent`. `rollout` is Studio's + * own: how the other nodes are taking the release, read from the restart job after the deploy answered. + */ export type DeployStreamEvent = | { type: 'phase'; data: DeployPhaseEvent } | { type: 'install'; data: DeployInstallEvent } - | { type: 'peer'; data: DeployPeerEvent }; + | { type: 'peer'; data: DeployPeerEvent } + | { type: 'warning'; data: DeployWarningEvent } + | { type: 'rollout'; data: RolloutSnapshot }; export interface DeployComponentResult extends ReplicatedResponse { deployment_id?: string; phase?: string; + certification?: DeployCertification; + /** The `restart_service` job a rolling restart hands the other nodes to. */ + restartJobId?: string; + /** The `warning` events the deploy streamed. */ + warnings?: string[]; + /** How the other nodes took the release, once Studio followed `restartJobId`. */ + rollout?: RolloutSnapshot; +} + +/** The deploy events a `deploy_component` or `get_deployment` stream carries; anything else is not one. */ +export function toDeployStreamEvent(message: SSEMessage): DeployStreamEvent | undefined { + switch (message.event) { + case 'phase': + return { type: 'phase', data: parseSSEData(message) as DeployPhaseEvent }; + case 'install': + return { type: 'install', data: parseSSEData(message) as DeployInstallEvent }; + case 'peer': + return { type: 'peer', data: parseSSEData(message) as DeployPeerEvent }; + case 'warning': { + const data = parseSSEData(message); + return { + type: 'warning', + data: typeof data === 'string' ? { message: data } : (data as DeployWarningEvent | undefined) ?? {}, + }; + } + } + return undefined; } export interface DeployComponentStreamParams { @@ -53,7 +92,7 @@ export interface DeployComponentStreamParams { } /** - * Run `deploy_component` over SSE, forwarding live phase/install/peer events to `onEvent` + * Run `deploy_component` over SSE, forwarding live phase/install/peer/warning events to `onEvent` * and resolving with the terminal `done` result. * * Throws the transport errors from `@/integrations/api/sse/errors`: @@ -71,6 +110,7 @@ export async function deployComponentStream({ signal, onEvent, }: DeployComponentStreamParams): Promise { + const warnings: string[] = []; const result = await streamOperation({ connection, body: { @@ -83,22 +123,17 @@ export async function deployComponentStream({ }, signal, onMessage: (message) => { - if (!onEvent) { + const event = toDeployStreamEvent(message); + if (!event) { return; } - switch (message.event) { - case 'phase': - onEvent({ type: 'phase', data: parseSSEData(message) as DeployPhaseEvent }); - break; - case 'install': - onEvent({ type: 'install', data: parseSSEData(message) as DeployInstallEvent }); - break; - case 'peer': - onEvent({ type: 'peer', data: parseSSEData(message) as DeployPeerEvent }); - break; + if (event.type === 'warning' && typeof event.data.message === 'string' && event.data.message) { + warnings.push(event.data.message); } + onEvent?.(event); }, }); - return (result ?? {}) as DeployComponentResult; + const deployed = (result ?? {}) as DeployComponentResult; + return warnings.length ? { ...deployed, warnings } : deployed; } diff --git a/src/integrations/api/instance/applications/deployOutcome.test.ts b/src/integrations/api/instance/applications/deployOutcome.test.ts new file mode 100644 index 000000000..5ccef5569 --- /dev/null +++ b/src/integrations/api/instance/applications/deployOutcome.test.ts @@ -0,0 +1,141 @@ +import { AxiosError } from 'axios'; +import { afterEach, describe, expect, it, vi } from 'vitest'; + +vi.mock('@/integrations/api/sse/resolveInstanceConnection', () => ({ + resolveInstanceConnection: () => ({ + url: 'https://host:9925/', + headers: { Accept: 'text/event-stream' }, + credentials: 'include' as RequestCredentials, + }), +})); + +import { deployComponentStream } from './deployComponentStream'; +import { deployFailureDetails, readCertificationDecision } from './deployOutcome'; +import { DeployRolloutError } from './restartJob'; + +// The decision a rejected deploy carries on both transports: harper `components/operations.js:1094` +// (buffered body) and `:1101-1109` (streamed `error` event), built by `certificationFailure` (:1222-1247). +const REJECTION = { + status: 'rejected', + reason: 'web.jsResource: Cannot find module ./missing.js', + failures: [{ key: 'web.jsResource', name: 'Error', message: 'Cannot find module ./missing.js', stack: '...' }], + restored: 'a3f8c2d1-0000-4000-8000-000000000000', + failed_closed: false, +}; +const MESSAGE = 'web was not deployed: release 9b2e failed to load in its canary worker: web.jsResource: ' + + 'Cannot find module ./missing.js. Deployment a3f8c2d1, the release it replaced, is live again. ' + + 'No other node received it. Deploy a fix, or activate deployment 9b2e again once the cause is fixed.'; + +function sseResponse(body: string): Response { + const stream = new ReadableStream({ + start(controller) { + controller.enqueue(new TextEncoder().encode(body)); + controller.close(); + }, + }); + return new Response(stream, { status: 200, headers: { 'content-type': 'text/event-stream' } }); +} + +function streamDeploy() { + return deployComponentStream({ + connection: { id: 'ins-1' }, + package: 'npm:web', + project: 'web', + replicated: true, + restart: 'rolling', + }); +} + +afterEach(() => { + vi.unstubAllGlobals(); +}); + +describe('deployFailureDetails', () => { + it("reads the canary's decision off a streamed rejection", async () => { + const event = { message: MESSAGE, code: 400, phase: 'load', deployment_id: '9b2e', certification: REJECTION }; + vi.stubGlobal('fetch', vi.fn().mockResolvedValue(sseResponse(`event: error\ndata: ${JSON.stringify(event)}\n\n`))); + + const failure = await streamDeploy().catch((error: unknown) => error); + expect(deployFailureDetails(failure)).toEqual({ + deploymentId: '9b2e', + certification: { + status: 'rejected', + reason: REJECTION.reason, + restored: REJECTION.restored, + failedClosed: false, + }, + }); + }); + + it('reads the same decision off a buffered rejection', () => { + const error = new AxiosError('Request failed with status code 400'); + error.response = { + status: 400, + data: { error: MESSAGE, phase: 'load', deployment_id: '9b2e', certification: REJECTION }, + } as AxiosError['response']; + expect(deployFailureDetails(error)).toEqual({ + deploymentId: '9b2e', + certification: { + status: 'rejected', + reason: REJECTION.reason, + restored: REJECTION.restored, + failedClosed: false, + }, + }); + }); + + it('keeps the record id of a deploy whose restart job failed', () => { + const error = new DeployRolloutError({ jobId: 'job-1', state: 'failed', nodes: [] }, 'd1'); + expect(deployFailureDetails(error)).toEqual({ deploymentId: 'd1' }); + }); + + it('reads nothing from a failure that carries neither', () => { + expect(deployFailureDetails(new Error('boom'))).toEqual({}); + expect(deployFailureDetails('Request failed')).toEqual({}); + expect(deployFailureDetails(undefined)).toEqual({}); + }); +}); + +describe('readCertificationDecision', () => { + it('reads a release with nothing to go back to as failed closed', () => { + expect(readCertificationDecision({ status: 'rejected', reason: 'boom', restored: null, failed_closed: true })) + .toEqual({ status: 'rejected', reason: 'boom', restored: null, failedClosed: true }); + }); + + it.each([ + ['the success string', 'certified'], + ['no status', { reason: 'boom' }], + ['an array', [REJECTION]], + ['null', null], + ])('is not fooled by %s', (_label, value) => { + expect(readCertificationDecision(value)).toBeUndefined(); + }); +}); + +describe('deployComponentStream', () => { + it('forwards and keeps the warnings the deploy streamed', async () => { + const drift = "Install fingerprints differ from this node's on 1 of 2 peer node(s): node-b (source npm:web@1.5.0)."; + vi.stubGlobal( + 'fetch', + vi.fn().mockResolvedValue( + sseResponse( + `event: warning\ndata: ${JSON.stringify({ message: drift })}\n\n` + + `event: done\ndata: ${ + JSON.stringify({ result: { message: `Successfully deployed: web ${drift}`, certification: 'certified' } }) + }\n\n`, + ), + ), + ); + + const events: unknown[] = []; + const result = await deployComponentStream({ + connection: { id: 'ins-1' }, + package: 'npm:web', + project: 'web', + replicated: true, + onEvent: (event) => events.push(event), + }); + expect(events).toEqual([{ type: 'warning', data: { message: drift } }]); + expect(result).toMatchObject({ certification: 'certified', warnings: [drift] }); + }); +}); diff --git a/src/integrations/api/instance/applications/deployOutcome.ts b/src/integrations/api/instance/applications/deployOutcome.ts new file mode 100644 index 000000000..f552037cb --- /dev/null +++ b/src/integrations/api/instance/applications/deployOutcome.ts @@ -0,0 +1,61 @@ +import { SSEOperationError } from '@/integrations/api/sse/errors'; +import { DeployRolloutError } from './restartJob'; + +/** + * How a successful, unstaged restarting deploy was checked, from Harper 5.4.0. Open-ended: Harper can + * also decide `withdrawn`, and a newer version may add more. + */ +export type DeployCertification = 'certified' | 'uncertified' | 'unavailable' | 'not-requested' | (string & {}); + +/** What a canary decided about a release it refused. A rejected deploy carries it in place of the string. */ +export interface CertificationDecision { + /** `rejected`, or `interrupted` for a restart that stopped before the canary decided. */ + status: string; + reason?: string; + /** The release put back in its place; `null` when there was none to put back. */ + restored: string | null; + /** No earlier release to restore, so no worker on that node loads the component until it is deployed again. */ + failedClosed: boolean; +} + +function isRecord(value: unknown): value is Record { + return typeof value === 'object' && value !== null && !Array.isArray(value); +} + +export function readCertificationDecision(value: unknown): CertificationDecision | undefined { + if (!isRecord(value) || typeof value.status !== 'string' || !value.status) { + return undefined; + } + return { + status: value.status, + reason: typeof value.reason === 'string' && value.reason ? value.reason : undefined, + restored: typeof value.restored === 'string' && value.restored ? value.restored : null, + failedClosed: value.failed_closed === true, + }; +} + +export interface DeployFailureDetails { + deploymentId?: string; + certification?: CertificationDecision; +} + +/** + * The record id and canary decision a failed deploy carries, from whichever transport ran it: the + * streamed `error` event, the buffered 4xx/5xx body, or a restart job that failed after the deploy. + */ +export function deployFailureDetails(error: unknown): DeployFailureDetails { + if (error instanceof SSEOperationError) { + return { deploymentId: error.deploymentId, certification: readCertificationDecision(error.certification) }; + } + if (error instanceof DeployRolloutError) { + return { deploymentId: error.deploymentId }; + } + const data: unknown = (error as { response?: { data?: unknown } } | undefined)?.response?.data; + if (!isRecord(data)) { + return {}; + } + return { + deploymentId: typeof data.deployment_id === 'string' && data.deployment_id ? data.deployment_id : undefined, + certification: readCertificationDecision(data.certification), + }; +} diff --git a/src/integrations/api/instance/applications/restartJob.test.ts b/src/integrations/api/instance/applications/restartJob.test.ts new file mode 100644 index 000000000..6a2e96161 --- /dev/null +++ b/src/integrations/api/instance/applications/restartJob.test.ts @@ -0,0 +1,268 @@ +import { AxiosError } from 'axios'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { + DeployRolloutError, + followRestartJob, + readRestartJob, + rolloutPollDelayMs, + RolloutSnapshot, +} from './restartJob'; + +type InstanceClient = Parameters[0]['instanceClient']; + +// Fixtures follow the rows Harper writes: harper `server/jobs/jobProcess.ts:72-87` records the job, and +// `bin/restart.ts` builds both result shapes — `activated` (5.4+, :232/:235/:258/:260) and `replicated` (:363/:385/:397). +const certifiedActivation = { + id: 'job-1', + status: 'COMPLETE', + message: 'Successfully completed job: job-1', + result: { + activated: [ + { node: 'node-b', certification: 'certified', message: 'Successfully deployed: web, restarting Harper' }, + { node: 'node-c', certification: 'uncertified', message: 'Successfully deployed: web, restarting Harper' }, + ], + }, +}; + +const PEER_REJECTION = 'web was not deployed on this node: release 9b2e failed to load in its canary worker: ' + + 'web.jsResource: Cannot find module ./missing.js. Deployment a3f8, the release it replaced, is live again. ' + + 'Deploy a fix, or activate deployment 9b2e again once the cause is fixed.'; + +const rejectedActivation = { + id: 'job-1', + status: 'ERROR', + message: { + error: `Deployment 9b2e of web was not activated on 1 of 2 peer node(s): node-c (${PEER_REJECTION})`, + activated: [ + { node: 'node-b', certification: 'certified', message: 'Successfully deployed: web, restarting Harper' }, + { node: 'node-c', error: PEER_REJECTION }, + ], + }, +}; + +const legacyRestart = { + id: 'job-1', + status: 'COMPLETE', + message: 'Successfully completed job: job-1', + result: { + replicated: [ + { node: 'node-b', message: 'Successfully completed job: peer-job-b' }, + { node: 'node-c', message: 'connect ECONNREFUSED 10.0.0.3:9933' }, + ], + }, +}; + +describe('readRestartJob', () => { + it('keeps following a job that has not ended', () => { + expect(readRestartJob('job-1', { id: 'job-1', status: 'CREATED' }).state).toBe('following'); + expect(readRestartJob('job-1', { id: 'job-1', status: 'IN_PROGRESS' }).state).toBe('following'); + }); + + it("reads each node's own certification from a 5.4 activation job", () => { + expect(readRestartJob('job-1', certifiedActivation)).toEqual({ + jobId: 'job-1', + state: 'complete', + nodes: [ + { node: 'node-b', outcome: 'activated', certification: 'certified' }, + { node: 'node-c', outcome: 'activated', certification: 'uncertified' }, + ], + }); + }); + + it('names the node that rejected the release, with its reason, when the activation job fails', () => { + const snapshot = readRestartJob('job-1', rejectedActivation); + expect(snapshot.state).toBe('failed'); + expect(snapshot.nodes).toEqual([ + { node: 'node-b', outcome: 'activated', certification: 'certified' }, + { node: 'node-c', outcome: 'failed', detail: PEER_REJECTION }, + ]); + expect(snapshot.message).toBe(rejectedActivation.message.error); + }); + + it('does not read a node the legacy restart job could not reach as restarted, though the job completes', () => { + expect(readRestartJob('job-1', legacyRestart)).toEqual({ + jobId: 'job-1', + state: 'complete', + nodes: [ + { node: 'node-b', outcome: 'restarted' }, + { node: 'node-c', outcome: 'unconfirmed', detail: 'connect ECONNREFUSED 10.0.0.3:9933' }, + ], + }); + }); + + it("fails with the legacy job's text when a peer's restart failed", () => { + expect( + readRestartJob('job-1', { + id: 'job-1', + status: 'ERROR', + message: 'Timed out waiting for restart job to complete', + }), + ) + .toEqual({ + jobId: 'job-1', + state: 'failed', + nodes: [], + message: 'Timed out waiting for restart job to complete', + }); + }); + + it('reads a single instance, which has no other nodes, as complete', () => { + const snapshot = readRestartJob('job-1', { id: 'job-1', status: 'COMPLETE', result: { replicated: [] } }); + expect(snapshot).toEqual({ jobId: 'job-1', state: 'complete', nodes: [] }); + }); + + it.each([ + ['a result that is not an object', { status: 'COMPLETE', result: 'done' }, 'complete'], + ['a non-array activated list', { status: 'COMPLETE', result: { activated: 'node-b' } }, 'complete'], + ['entries that are not objects', { status: 'COMPLETE', result: { activated: [null, 3, 'x'] } }, 'complete'], + ['an ERROR with no message', { status: 'ERROR' }, 'failed'], + ['a status in another case', { status: 'complete', result: {} }, 'complete'], + ['no status at all', {}, 'following'], + ])('never throws on %s', (_label, job, state) => { + expect(readRestartJob('job-1', { id: 'job-1', ...job } as never).state).toBe(state); + }); + + it('names an entry with no node rather than dropping it', () => { + const snapshot = readRestartJob('job-1', { + id: 'job-1', + status: 'ERROR', + message: { activated: [{ error: 'boom' }] }, + }); + expect(snapshot.nodes).toEqual([{ node: 'unknown node', outcome: 'failed', detail: 'boom' }]); + expect(snapshot.message).toBe('The restart job failed.'); + }); +}); + +function httpError(status: number) { + const error = new AxiosError(`Request failed with status code ${status}`); + error.response = { status } as AxiosError['response']; + return error; +} + +type Answer = { job: object } | { rows: [] } | { reject: unknown }; + +function makeClient(answers: Answer[]) { + let call = 0; + const post = vi.fn(() => { + const answer = answers[Math.min(call, answers.length - 1)]; + call += 1; + if ('reject' in answer) { + return Promise.reject(answer.reject); + } + return Promise.resolve({ data: 'rows' in answer ? [] : [answer.job] }); + }); + return { post, client: { post } as unknown as InstanceClient }; +} + +describe('followRestartJob', () => { + beforeEach(() => vi.useFakeTimers()); + afterEach(() => vi.useRealTimers()); + + it('polls get_job for that id until the job ends, reporting each snapshot', async () => { + const { post, client } = makeClient([ + { job: { id: 'job-1', status: 'IN_PROGRESS' } }, + { job: { id: 'job-1', status: 'IN_PROGRESS' } }, + { job: certifiedActivation }, + ]); + const updates: RolloutSnapshot[] = []; + const pending = followRestartJob({ instanceClient: client, jobId: 'job-1', onUpdate: (s) => updates.push(s) }); + await vi.advanceTimersByTimeAsync(10_000); + const snapshot = await pending; + expect(snapshot.state).toBe('complete'); + expect(post).toHaveBeenCalledTimes(3); + expect(post).toHaveBeenCalledWith('/', { operation: 'get_job', id: 'job-1' }); + expect(updates.map((s) => s.state)).toEqual(['following', 'complete']); + }); + + it('keeps polling through dropped connections and 5xx while a node restarts', async () => { + const { client } = makeClient([ + { reject: new AxiosError('Network Error', 'ERR_NETWORK') }, + { reject: httpError(502) }, + { reject: httpError(503) }, + { job: legacyRestart }, + ]); + const pending = followRestartJob({ instanceClient: client, jobId: 'job-1' }); + await vi.advanceTimersByTimeAsync(10_000); + expect((await pending).state).toBe('complete'); + }); + + it.each([400, 403])('stops following on a %s, which it would only get again', async (status) => { + const { post, client } = makeClient([{ reject: httpError(status) }]); + const snapshot = await followRestartJob({ instanceClient: client, jobId: 'job-1' }); + expect(snapshot).toMatchObject({ state: 'unfollowable', unfollowable: 'refused' }); + expect(post).toHaveBeenCalledOnce(); + }); + + it('gives up on a job the node it reaches does not have, but only once it has stayed missing', async () => { + const { client } = makeClient([{ rows: [] }]); + let settled: RolloutSnapshot | undefined; + void followRestartJob({ instanceClient: client, jobId: 'job-1' }).then((s) => (settled = s)); + await vi.advanceTimersByTimeAsync(18_000); + expect(settled).toBeUndefined(); + await vi.advanceTimersByTimeAsync(4_000); + expect(settled).toMatchObject({ state: 'unfollowable', unfollowable: 'not-found' }); + }); + + it('forgives a miss once the node that has the job answers again', async () => { + const { client } = makeClient([ + { rows: [] }, + ...Array.from({ length: 8 }, () => ({ rows: [] }) as Answer), + { job: { id: 'job-1', status: 'IN_PROGRESS' } }, + ...Array.from({ length: 8 }, () => ({ rows: [] }) as Answer), + { job: certifiedActivation }, + ]); + const pending = followRestartJob({ instanceClient: client, jobId: 'job-1' }); + await vi.advanceTimersByTimeAsync(60_000); + expect((await pending).state).toBe('complete'); + }); + + it('stops at its ceiling without claiming the job ended', async () => { + const { client } = makeClient([{ job: { id: 'job-1', status: 'IN_PROGRESS' } }]); + const pending = followRestartJob({ instanceClient: client, jobId: 'job-1', timeoutMs: 120_000 }); + await vi.advanceTimersByTimeAsync(130_000); + expect(await pending).toMatchObject({ state: 'unfollowable', unfollowable: 'timed-out' }); + }); + + it('slows down after the first minute instead of polling every 2s for half an hour', async () => { + expect(rolloutPollDelayMs(0, 2_000)).toBe(2_000); + expect(rolloutPollDelayMs(59_999, 2_000)).toBe(2_000); + expect(rolloutPollDelayMs(60_000, 2_000)).toBe(5_000); + expect(rolloutPollDelayMs(5 * 60_000, 2_000)).toBe(15_000); + + const { post, client } = makeClient([{ job: { id: 'job-1', status: 'IN_PROGRESS' } }]); + void followRestartJob({ instanceClient: client, jobId: 'job-1' }); + await vi.advanceTimersByTimeAsync(30 * 60_000); + // 30 in the first minute, 48 over the next four, 100 over the last 25. + expect(post.mock.calls.length).toBeLessThan(200); + }); + + it('rejects when its signal aborts', async () => { + const { client } = makeClient([{ job: { id: 'job-1', status: 'IN_PROGRESS' } }]); + const controller = new AbortController(); + const pending = followRestartJob({ instanceClient: client, jobId: 'job-1', signal: controller.signal }); + pending.catch(() => {}); + await vi.advanceTimersByTimeAsync(3_000); + controller.abort(new Error('cancelled')); + await vi.advanceTimersByTimeAsync(3_000); + await expect(pending).rejects.toThrow('cancelled'); + }); +}); + +describe('DeployRolloutError', () => { + it("carries the job's own text, which names each node that did not take the release", () => { + const rollout = readRestartJob('job-1', rejectedActivation); + const error = new DeployRolloutError(rollout, '9b2e'); + expect(error.message).toBe(rejectedActivation.message.error); + expect(error.deploymentId).toBe('9b2e'); + expect(error.rollout).toBe(rollout); + }); + + it('names the failed nodes itself when the job left no text', () => { + const error = new DeployRolloutError({ + jobId: 'job-1', + state: 'failed', + nodes: [{ node: 'node-c', outcome: 'failed', detail: 'boom' }], + }); + expect(error.message).toBe('Not every node took this release: node-c.'); + }); +}); diff --git a/src/integrations/api/instance/applications/restartJob.ts b/src/integrations/api/instance/applications/restartJob.ts new file mode 100644 index 000000000..403c31504 --- /dev/null +++ b/src/integrations/api/instance/applications/restartJob.ts @@ -0,0 +1,197 @@ +import { InstanceClientConfig } from '@/config/instanceClientConfig'; +import { getJob, HarperJob } from '@/integrations/api/instance/database/getJob'; +import { errorText } from '@/lib/errorText'; +import { sleep } from '@/lib/sleep'; +import { isDeterministicRejection } from '@/react-query/pollUnlessForbidden'; + +/** + * How a rolling deploy's other nodes took the release, read from the `restart_service` job the + * receiving node starts and names in `restartJobId`. + * + * `unfollowable` means Studio stopped without learning how the job ended, so it claims nothing. + */ +export type RolloutState = 'following' | 'complete' | 'failed' | 'unfollowable'; + +/** + * `not-found`: the node Studio reached has no such job. `hdb_job` does not replicate, so only the node + * that received the deploy can answer for it, and a cluster's address may reach another node. + */ +export type UnfollowableReason = 'not-found' | 'refused' | 'timed-out'; + +/** + * `activated` and `failed` come from a Harper 5.4+ activation job, where each node certifies the + * release itself. `restarted` and `unconfirmed` come from the plain restart job older versions run: + * there a node the job could not reach is listed with the error, and the job still completes. + */ +export type RolloutNodeOutcome = 'activated' | 'restarted' | 'failed' | 'unconfirmed'; + +export interface RolloutNode { + node: string; + outcome: RolloutNodeOutcome; + /** That node's own canary verdict (`certified`, `uncertified`, ...), on an activation job. */ + certification?: string; + detail?: string; +} + +export interface RolloutSnapshot { + jobId: string; + state: RolloutState; + nodes: RolloutNode[]; + message?: string; + unfollowable?: UnfollowableReason; +} + +// Harper's job worker records an object result under this message; a peer's restart job returns no +// result, so a peer that restarted reads exactly this, and one the job could not reach reads the error. +const PEER_RESTARTED = /^Successfully completed job\b/; + +function isRecord(value: unknown): value is Record { + return typeof value === 'object' && value !== null && !Array.isArray(value); +} + +function nodeName(entry: Record): string { + return typeof entry.node === 'string' && entry.node ? entry.node : 'unknown node'; +} + +function activatedNodes(entries: unknown[]): RolloutNode[] { + return entries.filter(isRecord).map((entry) => { + const error = errorText(entry.error); + if (error) { + return { node: nodeName(entry), outcome: 'failed', detail: error }; + } + return { + node: nodeName(entry), + outcome: 'activated', + certification: typeof entry.certification === 'string' ? entry.certification : undefined, + }; + }); +} + +function restartedNodes(entries: unknown[]): RolloutNode[] { + return entries.filter(isRecord).map((entry) => { + const message = errorText(entry.message); + return message && PEER_RESTARTED.test(message) + ? { node: nodeName(entry), outcome: 'restarted' } + : { node: nodeName(entry), outcome: 'unconfirmed', detail: message }; + }); +} + +/** Read one `get_job` row of a rolling deploy's restart job. */ +export function readRestartJob(jobId: string, job: HarperJob): RolloutSnapshot { + const status = typeof job.status === 'string' ? job.status.toUpperCase() : undefined; + if (status === 'COMPLETE') { + const result = isRecord(job.result) ? job.result : undefined; + const nodes = Array.isArray(result?.activated) + ? activatedNodes(result.activated) + : Array.isArray(result?.replicated) + ? restartedNodes(result.replicated) + : []; + return { jobId, state: nodes.some((node) => node.outcome === 'failed') ? 'failed' : 'complete', nodes }; + } + if (status === 'ERROR') { + // A 5.4+ activation job fails with its report as the message: `{ error, activated }`. + const report = isRecord(job.message) ? job.message : undefined; + return { + jobId, + state: 'failed', + nodes: Array.isArray(report?.activated) ? activatedNodes(report.activated) : [], + message: errorText(report ? report.error : job.message) ?? 'The restart job failed.', + }; + } + return { jobId, state: 'following', nodes: [] }; +} + +// Fast while a rollout is most likely to finish, then slower, so a long rollout — or a node that is +// down — costs a few requests a minute rather than thirty. +export function rolloutPollDelayMs(elapsedMs: number, fastMs: number): number { + if (elapsedMs < 60_000) { + return fastMs; + } + return elapsedMs < 5 * 60_000 ? 5_000 : 15_000; +} + +// A cluster's address can reach a node other than the one that received the deploy, and reach that one +// again on the next request, so a missing row only counts once the job has stayed missing for a while. +const NOT_FOUND_GRACE_MS = 20_000; + +export interface FollowRestartJobParams extends InstanceClientConfig { + jobId: string; + onUpdate?: (snapshot: RolloutSnapshot) => void; + signal?: AbortSignal; + pollIntervalMs?: number; + timeoutMs?: number; +} + +/** + * Poll `get_job` until the restart job ends, and say how each other node took the release. + * + * Harper writes the job's row when it starts and when it ends, so every node's outcome arrives at once. + * Rejects only when `signal` aborts: a job Studio could not follow resolves `unfollowable`. + */ +export async function followRestartJob({ + instanceClient, + jobId, + onUpdate, + signal, + pollIntervalMs = 2_000, + timeoutMs = 30 * 60_000, +}: FollowRestartJobParams): Promise { + const settle = (snapshot: RolloutSnapshot) => { + onUpdate?.(snapshot); + return snapshot; + }; + const unfollowable = (reason: UnfollowableReason) => + settle({ jobId, state: 'unfollowable', nodes: [], unfollowable: reason }); + const startedAt = Date.now(); + let missingSince: number | undefined; + onUpdate?.({ jobId, state: 'following', nodes: [] }); + for (;;) { + signal?.throwIfAborted(); + try { + const job = await getJob({ jobId, instanceClient }); + if (job) { + missingSince = undefined; + const snapshot = readRestartJob(jobId, job); + if (snapshot.state !== 'following') { + return settle(snapshot); + } + } else { + missingSince ??= Date.now(); + if (Date.now() - missingSince >= NOT_FOUND_GRACE_MS) { + return unfollowable('not-found'); + } + } + } catch (error) { + // Anything else — a dropped connection, a 5xx while that node's own workers restart — passes. + if (isDeterministicRejection(error)) { + return unfollowable('refused'); + } + } + const elapsed = Date.now() - startedAt; + if (elapsed >= timeoutMs) { + return unfollowable('timed-out'); + } + await sleep(rolloutPollDelayMs(elapsed, pollIntervalMs)); + } +} + +/** + * A rolling deploy whose release is live on the node that received it, but whose restart job failed. + * Datadog withholds its message by this `name`: it quotes each node's error, which can carry the + * component's own load failure. + */ +export class DeployRolloutError extends Error { + readonly rollout: RolloutSnapshot; + readonly deploymentId?: string; + + constructor(rollout: RolloutSnapshot, deploymentId?: string) { + const failed = rollout.nodes.filter((node) => node.outcome === 'failed').map((node) => node.node); + super( + rollout.message + ?? (failed.length ? `Not every node took this release: ${failed.join(', ')}.` : 'The restart job failed.'), + ); + this.name = 'DeployRolloutError'; + this.rollout = rollout; + this.deploymentId = deploymentId; + } +} diff --git a/src/integrations/api/instance/database/getJob.test.ts b/src/integrations/api/instance/database/getJob.test.ts index b5fe44b10..f5ebdf970 100644 --- a/src/integrations/api/instance/database/getJob.test.ts +++ b/src/integrations/api/instance/database/getJob.test.ts @@ -3,7 +3,7 @@ import { ImportJobError, waitForJob } from './getJob'; type InstanceClient = Parameters[0]['instanceClient']; -function makeClient(responses: ({ status: string; message?: string } | { reject: true })[]) { +function makeClient(responses: ({ status: string; message?: unknown } | { reject: true })[]) { let call = 0; const post = vi.fn(() => { const job = responses[Math.min(call, responses.length - 1)]; @@ -57,6 +57,19 @@ describe('waitForJob', () => { await expect(pending).rejects.toBeInstanceOf(ImportJobError); }); + // harper `server/jobs/jobProcess.ts:86` (5.3.0+) records a refused load's permission report as the message. + it('reads a structured job message as text, never "[object Object]"', async () => { + const report = { + error: 'This operation is not authorized due to role restrictions and/or invalid database items', + unauthorized_access: [{ database: 'data', table: 'dog', required_table_permissions: ['insert'] }], + }; + const instanceClient = makeClient([{ status: 'ERROR', message: report }]); + const pending = waitForJob({ jobId: 'job-1', instanceClient, pollIntervalMs: 100 }); + pending.catch(() => {}); + await vi.advanceTimersByTimeAsync(100); + await expect(pending).rejects.toThrow(JSON.stringify(report)); + }); + it('tolerates transient get_job failures and keeps polling', async () => { const instanceClient = makeClient([ { status: 'IN_PROGRESS' }, diff --git a/src/integrations/api/instance/database/getJob.ts b/src/integrations/api/instance/database/getJob.ts index 8265aa1a5..60e91214c 100644 --- a/src/integrations/api/instance/database/getJob.ts +++ b/src/integrations/api/instance/database/getJob.ts @@ -1,10 +1,17 @@ import { InstanceClientConfig } from '@/config/instanceClientConfig'; +import { errorText } from '@/lib/errorText'; export interface HarperJob { id: string; /** Lifecycle: CREATED → IN_PROGRESS → COMPLETE | ERROR. */ status: 'CREATED' | 'IN_PROGRESS' | 'COMPLETE' | 'ERROR' | string; - message?: string; + /** + * Usually text. From Harper 5.3.0 a job that fails with a structured report — a refused bulk load's + * permission report, a rolling activation's per-node outcomes — records the report object here. + */ + message?: unknown; + /** What a job that returned an object finished with; its `message` is then a generic sentence. */ + result?: unknown; job_body?: unknown; } @@ -60,7 +67,7 @@ export async function waitForJob({ // CREATED (no worker picked it up yet), so anything else means keep polling. const status = job?.status?.toUpperCase(); if (status === 'ERROR') { - throw new ImportJobError(job?.message || 'The import job failed.'); + throw new ImportJobError(errorText(job?.message) ?? 'The import job failed.'); } if (status === 'COMPLETE' && job) { return job; diff --git a/src/integrations/api/instance/database/importData.ts b/src/integrations/api/instance/database/importData.ts index 29bfe73f9..359fecaa3 100644 --- a/src/integrations/api/instance/database/importData.ts +++ b/src/integrations/api/instance/database/importData.ts @@ -1,4 +1,5 @@ import { InstanceClientConfig } from '@/config/instanceClientConfig'; +import { errorText } from '@/lib/errorText'; import { useMutation } from '@tanstack/react-query'; import { AxiosError } from 'axios'; import { onAddCSVDataSubmit } from './addCSVData'; @@ -67,7 +68,7 @@ export async function importData({ case 'csv-data': { const { job_id } = await onAddCSVDataSubmit({ database, table, fileData: source.data, instanceClient }); const job = await waitForJob({ jobId: job_id, instanceClient }); - return { message: job.message || 'CSV data imported successfully' }; + return { message: errorText(job.message) ?? 'CSV data imported successfully' }; } case 'csv-url': { const { data } = await instanceClient.post<{ message: string; job_id: string }>('/', { @@ -78,7 +79,7 @@ export async function importData({ action: 'insert', }); const job = await waitForJob({ jobId: data.job_id, instanceClient }); - return { message: job.message || 'CSV data imported successfully' }; + return { message: errorText(job.message) ?? 'CSV data imported successfully' }; } case 'json-records': { const result = await insertTableRecords({ diff --git a/src/integrations/api/instance/deployments/getDeployment.ts b/src/integrations/api/instance/deployments/getDeployment.ts index 8b3cb2b76..6249a6530 100644 --- a/src/integrations/api/instance/deployments/getDeployment.ts +++ b/src/integrations/api/instance/deployments/getDeployment.ts @@ -1,12 +1,6 @@ import { InstanceClientIdConfig } from '@/config/instanceClientConfig'; import { EntityIds } from '@/features/auth/store/authStore'; -import { - DeployInstallEvent, - DeployPeerEvent, - DeployPhaseEvent, - DeployStreamEvent, -} from '@/integrations/api/instance/applications/deployComponentStream'; -import { parseSSEData } from '@/integrations/api/sse/parseSSEStream'; +import { DeployStreamEvent, toDeployStreamEvent } from '@/integrations/api/instance/applications/deployComponentStream'; import { ResolveInstanceConnectionParams } from '@/integrations/api/sse/resolveInstanceConnection'; import { streamOperation } from '@/integrations/api/sse/streamOperation'; import { pollUnlessForbidden } from '@/react-query/pollUnlessForbidden'; @@ -70,19 +64,9 @@ export async function getDeploymentStream({ body: { operation: 'get_deployment', deployment_id: deploymentId }, signal, onMessage: (message) => { - if (!onEvent) { - return; - } - switch (message.event) { - case 'phase': - onEvent({ type: 'phase', data: parseSSEData(message) as DeployPhaseEvent }); - break; - case 'install': - onEvent({ type: 'install', data: parseSSEData(message) as DeployInstallEvent }); - break; - case 'peer': - onEvent({ type: 'peer', data: parseSSEData(message) as DeployPeerEvent }); - break; + const event = onEvent && toDeployStreamEvent(message); + if (event) { + onEvent(event); } }, }); diff --git a/src/integrations/api/sse/errors.ts b/src/integrations/api/sse/errors.ts index e67f10611..fa00d8d9d 100644 --- a/src/integrations/api/sse/errors.ts +++ b/src/integrations/api/sse/errors.ts @@ -38,12 +38,18 @@ export class SSEOperationError extends Error { readonly code?: string | number; readonly phase?: string; readonly deploymentId?: string; + /** A deploy its canary refused carries the decision here, as sent (Harper 5.4.0+). */ + readonly certification?: unknown; - constructor(message: string, details?: { code?: string | number; phase?: string; deploymentId?: string }) { + constructor( + message: string, + details?: { code?: string | number; phase?: string; deploymentId?: string; certification?: unknown }, + ) { super(message); this.name = 'SSEOperationError'; this.code = details?.code; this.phase = details?.phase; this.deploymentId = details?.deploymentId; + this.certification = details?.certification; } } diff --git a/src/integrations/api/sse/streamOperation.ts b/src/integrations/api/sse/streamOperation.ts index d0917a7b3..841e474f7 100644 --- a/src/integrations/api/sse/streamOperation.ts +++ b/src/integrations/api/sse/streamOperation.ts @@ -104,7 +104,13 @@ export async function streamOperation({ if (!data || typeof data !== 'object') { throw new SSEOperationError(typeof data === 'string' && data ? data : 'The operation failed.'); } - const err = data as { message?: string; code?: string | number; phase?: string; deployment_id?: string }; + const err = data as { + message?: string; + code?: string | number; + phase?: string; + deployment_id?: string; + certification?: unknown; + }; // A non-string message (a structured/nested error object) would stringify to // "[object Object]" in every toast that renders it — extract its nested // message (or JSON) instead of discarding the detail (#1426). @@ -112,6 +118,7 @@ export async function streamOperation({ code: err.code, phase: err.phase, deploymentId: err.deployment_id, + certification: err.certification, }); } } diff --git a/src/integrations/datadog/redactRelayedMessage.test.ts b/src/integrations/datadog/redactRelayedMessage.test.ts index 1f18a070d..da530c0aa 100644 --- a/src/integrations/datadog/redactRelayedMessage.test.ts +++ b/src/integrations/datadog/redactRelayedMessage.test.ts @@ -1,4 +1,5 @@ import { describe, expect, it } from 'vitest'; +import { DeployRolloutError } from '../api/instance/applications/restartJob'; import { ImportJobError } from '../api/instance/database/getJob'; import { SSEOperationError } from '../api/sse/errors'; import { redactRelayedMessage, redactRelayedStack } from './redactRelayedMessage'; @@ -64,6 +65,21 @@ describe('redactRelayedMessage', () => { .toBe(WITHHELD + frames); }); + it("withholds a rolling deploy's failed restart job, whose message quotes each node's error", () => { + const error = new DeployRolloutError({ + jobId: 'job-1', + state: 'failed', + nodes: [{ node: 'node-b', outcome: 'failed', detail: 'acme-billing.jsResource: Cannot find module' }], + message: 'Deployment d1 of acme-billing was not activated on 1 of 1 peer node(s): node-b (acme-billing...)', + }); + expect(error.name).toBe('DeployRolloutError'); + expect(redactRelayedMessage(error.name, error.message)).toBe(WITHHELD); + const frames = '\n at deployAndFollowComponent @ https://fabric.harper.fast/assets/index-A1b2C3d4.js:5:1234'; + const redacted = redactRelayedStack(error.name, error.message, `DeployRolloutError: ${error.message}${frames}`); + expect(redacted).not.toContain('acme-billing'); + expect(redacted).toBe(WITHHELD + frames); + }); + it('removes exactly the message and keeps what follows', () => { const message = [ 'SSEOperationError: Failed to clone package github:gh repo clone acme-corp/billing: fatal: Too many arguments.', diff --git a/src/integrations/datadog/redactRelayedMessage.ts b/src/integrations/datadog/redactRelayedMessage.ts index 98f548e5b..d279926cc 100644 --- a/src/integrations/datadog/redactRelayedMessage.ts +++ b/src/integrations/datadog/redactRelayedMessage.ts @@ -19,9 +19,10 @@ * `SSEOperationError`'s `code` and `phase`, which reach the browser but are not on the RUM event; * reporting them deliberately would restore that detail without the message. * - * `ImportJobError` is the same relay for a failed load job, whose text quotes the rejected rows. + * `ImportJobError` is the same relay for a failed load job, whose text quotes the rejected rows, and + * `DeployRolloutError` for a rolling deploy's failed restart job, whose text quotes each node's error. */ -const RELAYED_ERROR_TYPES = new Set(['SSEOperationError', 'ImportJobError']); +const RELAYED_ERROR_TYPES = new Set(['SSEOperationError', 'ImportJobError', 'DeployRolloutError']); const WITHHELD = 'Harper reported an operation failure (server message withheld).'; From 6213350033220ed83a4162d7992c5821c6a24f2b Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Sun, 11 Oct 2026 02:24:31 -0400 Subject: [PATCH 12/21] fix(deploy): never read an unrecognized or buffered rollout as a clean success A finished restart job with a result in neither shape Harper writes is now unknown rather than complete. The buffered redeploy and import paths report through the same rollout policy as the streamed one, so a node the legacy job could not reach, or a rollout Studio could not follow, is a warning there too, and their toasts link the deployment's record. Following the job aborts promptly when its signal fires mid-poll, and an unknown rollout settles the progress panel as inconclusive, not success. Refs #1785 Co-Authored-By: Claude Opus 5.5 --- src/features/instance/applications/DESIGN.md | 7 +- .../DeployProgress/DeployProgress.tsx | 5 +- .../DeployProgress/describeDeployOutcome.ts | 21 +++--- .../useDeploymentStream.test.ts | 16 +++++ .../DeployProgress/useDeploymentStream.ts | 14 ++-- .../NewApplication/useImportApplication.ts | 16 +++-- .../hooks/useDeploymentRecordAction.ts | 23 +++++++ .../modals/RedeployApplicationModal.tsx | 56 +++++++++++----- .../modals/reportDeployHealth.test.ts | 41 +++++++++++- .../applications/modals/reportDeployHealth.ts | 64 ++++++++++++++++--- .../applications/deployComponent.test.ts | 2 +- .../applications/deployComponentStream.ts | 3 +- .../instance/applications/deployOutcome.ts | 2 +- .../instance/applications/restartJob.test.ts | 42 ++++++++++-- .../api/instance/applications/restartJob.ts | 19 ++++-- .../api/instance/database/getJob.ts | 6 +- 16 files changed, 269 insertions(+), 68 deletions(-) create mode 100644 src/features/instance/applications/hooks/useDeploymentRecordAction.ts diff --git a/src/features/instance/applications/DESIGN.md b/src/features/instance/applications/DESIGN.md index 92044799e..95b77130f 100644 --- a/src/features/instance/applications/DESIGN.md +++ b/src/features/instance/applications/DESIGN.md @@ -13,9 +13,10 @@ So [`deployAndFollowComponent`](../../../integrations/api/instance/applications/ progress panel name each node that did not take it. Its message quotes those nodes' errors, so Datadog withholds it like `SSEOperationError`. - **A job Studio could not follow resolves, as `rollout.state === 'unfollowable'`, and is never reported as success.** - [`reportDeployOutcome`](modals/reportDeployHealth.ts) skips the post-deploy health check for it, because that check - reads the one node Studio reaches, which can be healthy on the release a rejecting node kept. Any new caller of the - deploy mutation has to make the same distinction. + That covers a job that ended with a result in neither shape below. [`reportDeployOutcome`](modals/reportDeployHealth.ts) + skips the post-deploy health check for it, because that check reads the one node Studio reaches, which can be healthy + on the release a rejecting node kept; `reportDeployResult` is the same policy for a caller that does not health-check. + Any new caller of the deploy mutation reports through one of the two. - **The job may not be there.** `hdb_job` is a non-replicating system table, so only the receiving node can answer for it. Central manager's cluster proxy sends every operation to the first running instance, so over the proxy the deploy and its polls reach the same node; a direct connection to a cluster's address can reach another node, which answers diff --git a/src/features/instance/applications/components/DeployProgress/DeployProgress.tsx b/src/features/instance/applications/components/DeployProgress/DeployProgress.tsx index b9cf64676..40c517ccc 100644 --- a/src/features/instance/applications/components/DeployProgress/DeployProgress.tsx +++ b/src/features/instance/applications/components/DeployProgress/DeployProgress.tsx @@ -1,6 +1,7 @@ import { Badge } from '@/components/ui/badge'; import { ScrollArea } from '@/components/ui/scrollArea'; import { Separator } from '@/components/ui/separator'; +import { deploymentRecordPath } from '@/features/instance/applications/hooks/useDeploymentRecordAction'; import { DEPLOY_PHASE_ORDER } from '@/integrations/api/instance/applications/deployComponentStream'; import { CertificationDecision } from '@/integrations/api/instance/applications/deployOutcome'; import { RolloutNode, RolloutSnapshot } from '@/integrations/api/instance/applications/restartJob'; @@ -177,7 +178,7 @@ export function DeployProgress({
)} - {state.lifecycle === 'inconclusive' && ( + {state.lifecycle === 'inconclusive' && !state.rollout && ( Live updates ended early — verifying… @@ -185,7 +186,7 @@ export function DeployProgress({ {state.deploymentId && ( diff --git a/src/features/instance/applications/components/DeployProgress/describeDeployOutcome.ts b/src/features/instance/applications/components/DeployProgress/describeDeployOutcome.ts index ee702fa42..b2bfd3261 100644 --- a/src/features/instance/applications/components/DeployProgress/describeDeployOutcome.ts +++ b/src/features/instance/applications/components/DeployProgress/describeDeployOutcome.ts @@ -1,14 +1,15 @@ import { CertificationDecision } from '@/integrations/api/instance/applications/deployOutcome'; import { RolloutNode, RolloutSnapshot } from '@/integrations/api/instance/applications/restartJob'; -export interface CertificationSummary { +interface CertificationSummary { label: string; /** Why it went out unchecked, when it did. */ detail?: string; certified: boolean; } -/** Harper's `certification` string (5.4.0+), as a person reads it. */ +// The reasons are Harper's own (documentation `reference/operations-api/operations.md`, "Certifying a release in a +// canary worker"; harper `components/operations.js:815-817`). export function describeCertification(certification: string): CertificationSummary { switch (certification) { case 'certified': @@ -34,14 +35,9 @@ export function describeCertification(certification: string): CertificationSumma } } -export interface DecisionSummary { - title: string; - reason?: string; - outcome?: string; -} - -/** A canary's refusal of a release on the node that received the deploy. */ -export function describeDecision(decision: CertificationDecision): DecisionSummary { +export function describeDecision( + decision: CertificationDecision, +): { title: string; reason?: string; outcome?: string } { const title = decision.status === 'rejected' ? 'Its canary rejected this release on the node that received the deploy.' : decision.status === 'interrupted' @@ -55,7 +51,6 @@ export function describeDecision(decision: CertificationDecision): DecisionSumma return { title, reason: decision.reason, outcome }; } -/** Why Studio could not say how the other nodes took the release. */ export function describeUnfollowable(rollout: RolloutSnapshot): string | undefined { switch (rollout.unfollowable) { case 'not-found': @@ -67,11 +62,13 @@ export function describeUnfollowable(rollout: RolloutSnapshot): string | undefin case 'timed-out': return 'The other nodes were still restarting after 30 minutes, so Studio stopped waiting. The restart may ' + 'still finish; check their status before relying on it.'; + case 'unrecognized': + return "This deploy's restart job finished, but Studio couldn't read how the other nodes took the release. " + + 'Check their status before relying on it.'; } return undefined; } -/** One node's line in the rollout list. */ export function describeRolloutNode(node: RolloutNode): { label: string; detail?: string } { switch (node.outcome) { case 'activated': diff --git a/src/features/instance/applications/components/DeployProgress/useDeploymentStream.test.ts b/src/features/instance/applications/components/DeployProgress/useDeploymentStream.test.ts index 159988a05..4983c956d 100644 --- a/src/features/instance/applications/components/DeployProgress/useDeploymentStream.test.ts +++ b/src/features/instance/applications/components/DeployProgress/useDeploymentStream.test.ts @@ -51,6 +51,22 @@ describe('useDeploymentStream', () => { }); }); + it('keeps the warnings a result carries when none streamed, without repeating streamed ones', () => { + const { result } = renderHook(() => useDeploymentStream()); + act(() => { + result.current.markStarted(); + result.current.markSettled('success', undefined, { result: { message: 'ok', warnings: ['drift'] } }); + }); + expect(result.current.state.warnings).toEqual(['drift']); + + act(() => { + result.current.markStarted(); + result.current.onEvent({ type: 'warning', data: { message: 'drift' } }); + result.current.markSettled('success', undefined, { result: { message: 'ok', warnings: ['drift'] } }); + }); + expect(result.current.state.warnings).toEqual(['drift']); + }); + it('starts each deploy clean', () => { const { result } = renderHook(() => useDeploymentStream()); act(() => { diff --git a/src/features/instance/applications/components/DeployProgress/useDeploymentStream.ts b/src/features/instance/applications/components/DeployProgress/useDeploymentStream.ts index 62b7424a6..78db60f37 100644 --- a/src/features/instance/applications/components/DeployProgress/useDeploymentStream.ts +++ b/src/features/instance/applications/components/DeployProgress/useDeploymentStream.ts @@ -57,19 +57,23 @@ type Action = | { kind: 'event'; event: DeployStreamEvent } | { kind: 'settled'; lifecycle: Settled; error?: string; outcome?: SettledOutcome }; -/** What the deploy answered, or failed with, beyond its lifecycle. */ -export interface SettledOutcome { +interface SettledOutcome { result?: DeployComponentResult; failure?: unknown; } -function settledDetails(outcome: SettledOutcome | undefined): Partial { +function settledDetails( + state: DeploymentStreamState, + outcome: SettledOutcome | undefined, +): Partial { if (outcome?.result) { - const { certification, deployment_id, rollout } = outcome.result; + const { certification, deployment_id, rollout, warnings } = outcome.result; return { certification: typeof certification === 'string' ? certification : undefined, deploymentId: deployment_id, ...(rollout ? { rollout } : {}), + // The streamed events already put them here; a result that did not stream still carries them. + ...(!state.warnings?.length && warnings?.length ? { warnings } : {}), }; } if (outcome?.failure !== undefined) { @@ -118,7 +122,7 @@ function reducer(state: DeploymentStreamState, action: Action): DeploymentStream return { ...state, peers: state.peers.concat(event.data) }; } case 'settled': - return { ...state, ...settledDetails(action.outcome), lifecycle: action.lifecycle, error: action.error }; + return { ...state, ...settledDetails(state, action.outcome), lifecycle: action.lifecycle, error: action.error }; } } diff --git a/src/features/instance/applications/components/NewApplication/useImportApplication.ts b/src/features/instance/applications/components/NewApplication/useImportApplication.ts index 58c77e4a3..30f6cccb7 100644 --- a/src/features/instance/applications/components/NewApplication/useImportApplication.ts +++ b/src/features/instance/applications/components/NewApplication/useImportApplication.ts @@ -1,9 +1,11 @@ import { useInstanceClientIdParams } from '@/config/useInstanceClient'; import { useComponentHealthCheck } from '@/features/instance/applications/hooks/useComponentHealthCheck'; +import { useDeploymentRecordAction } from '@/features/instance/applications/hooks/useDeploymentRecordAction'; import { useEditorView } from '@/features/instance/applications/hooks/useEditorView'; import { useSupportsDeploymentSSE } from '@/features/instance/applications/hooks/useSupportsDeploymentSSE'; import { reportDeployOutcome } from '@/features/instance/applications/modals/reportDeployHealth'; import { useDeployComponentMutation } from '@/integrations/api/instance/applications/deployComponent'; +import { deployFailureDetails } from '@/integrations/api/instance/applications/deployOutcome'; import { DeployRolloutError } from '@/integrations/api/instance/applications/restartJob'; import { SSEInconclusiveError } from '@/integrations/api/sse/errors'; import { invalidateEntityQueries } from '@/react-query/invalidateEntityQueries'; @@ -30,6 +32,7 @@ export function useImportApplication( const instanceParams = useInstanceClientIdParams(); const sseDeploy = useSupportsDeploymentSSE(); const runHealthCheck = useComponentHealthCheck(instanceParams); + const recordAction = useDeploymentRecordAction(); const { reloadRootEntries, setFocusedItem, setExpandedItems, setSelectedItems } = useEditorView(); const onImported = useCallback((project: string) => { @@ -93,8 +96,11 @@ export function useImportApplication( onSuccess: async (result) => { toast.dismiss(toastId); onImported(project); - if (sseDeploy || result?.rollout?.state === 'unfollowable') { - await reportDeployOutcome(project, result, runHealthCheck); + // A followed rollout restarted the other nodes, so there is a health to check however the deploy was sent. + if (sseDeploy || result?.rollout) { + await reportDeployOutcome(project, result, runHealthCheck, { + action: recordAction(result?.deployment_id), + }); } else { toast.success(`Imported successfully`, { description: `${project} will be available once you restart your ${instanceParams.entityType}!`, @@ -110,17 +116,19 @@ export function useImportApplication( await reportDeployOutcome(project, undefined, runHealthCheck); return; } + const action = recordAction(deployFailureDetails(error).deploymentId); if (error instanceof DeployRolloutError) { onImported(project); - toast.error(`${project} did not reach every node`, { description: error.message }); + toast.error(`${project} did not reach every node`, { description: error.message, action }); return; } toast.error(`Failed to import ${project}`, { description: error instanceof Error ? error.message : undefined, + action, }); }, }); - }, [mutate, sseDeploy, instanceParams, runHealthCheck, onImported]); + }, [mutate, sseDeploy, instanceParams, runHealthCheck, onImported, recordAction]); return useMemo(() => ({ isImportingApplication, diff --git a/src/features/instance/applications/hooks/useDeploymentRecordAction.ts b/src/features/instance/applications/hooks/useDeploymentRecordAction.ts new file mode 100644 index 000000000..eb549e48e --- /dev/null +++ b/src/features/instance/applications/hooks/useDeploymentRecordAction.ts @@ -0,0 +1,23 @@ +import { buildAbsoluteLinkToPage } from '@/lib/urls/buildAbsoluteLinkToPage'; +import { useNavigate, useParams } from '@tanstack/react-router'; +import { useCallback } from 'react'; + +export function deploymentRecordPath( + params: { clusterId?: string; instanceId?: string; organizationId?: string }, + deploymentId: string, +): string { + return buildAbsoluteLinkToPage(params, `config/deployments/${encodeURIComponent(deploymentId)}`); +} + +/** A toast action that opens a deployment's record, for a deploy whose result or failure named one. */ +export function useDeploymentRecordAction() { + const navigate = useNavigate(); + const params: { clusterId?: string; instanceId?: string; organizationId?: string } = useParams({ strict: false }); + return useCallback( + (deploymentId: string | undefined) => + deploymentId + ? { label: 'View record', onClick: () => void navigate({ to: deploymentRecordPath(params, deploymentId) }) } + : undefined, + [navigate, params], + ); +} diff --git a/src/features/instance/applications/modals/RedeployApplicationModal.tsx b/src/features/instance/applications/modals/RedeployApplicationModal.tsx index 7b5d25e41..6882bbed7 100644 --- a/src/features/instance/applications/modals/RedeployApplicationModal.tsx +++ b/src/features/instance/applications/modals/RedeployApplicationModal.tsx @@ -9,22 +9,24 @@ import { FormMessage } from '@/components/ui/form/FormMessage'; import { Input } from '@/components/ui/input'; import { useInstanceClientIdParams } from '@/config/useInstanceClient'; import { DeployProgress } from '@/features/instance/applications/components/DeployProgress/DeployProgress'; -import { describeUnfollowable } from '@/features/instance/applications/components/DeployProgress/describeDeployOutcome'; import { useDeploymentStream } from '@/features/instance/applications/components/DeployProgress/useDeploymentStream'; import { useComponentHealthCheck } from '@/features/instance/applications/hooks/useComponentHealthCheck'; +import { useDeploymentRecordAction } from '@/features/instance/applications/hooks/useDeploymentRecordAction'; import { useEditorView } from '@/features/instance/applications/hooks/useEditorView'; import { useSupportsDeploymentSSE } from '@/features/instance/applications/hooks/useSupportsDeploymentSSE'; import { useDeployComponentMutation } from '@/integrations/api/instance/applications/deployComponent'; +import { deployFailureDetails } from '@/integrations/api/instance/applications/deployOutcome'; import { DeployRolloutError } from '@/integrations/api/instance/applications/restartJob'; import { SSEInconclusiveError } from '@/integrations/api/sse/errors'; import { attemptToRestoreFocus } from '@/lib/attemptToRestoreFocus'; import { setWatchedValue, useWatchedValue } from '@/lib/events/watcher'; +import { describeError } from '@/react-query/queryClient'; import { useQueryClient } from '@tanstack/react-query'; import { RefreshCwIcon } from 'lucide-react'; import { useCallback, useEffect, useState } from 'react'; import { useForm } from 'react-hook-form'; import { toast } from 'sonner'; -import { deployNotes, reportDeployOutcome } from './reportDeployHealth'; +import { deployNotes, reportDeployOutcome, reportDeployResult } from './reportDeployHealth'; export function RedeployApplicationModal() { const { value: isModalOpen, trigger } = useWatchedValue('ShowRedeployApplicationModal', false); @@ -34,6 +36,7 @@ export function RedeployApplicationModal() { const sseDeploy = useSupportsDeploymentSSE(); const stream = useDeploymentStream(); const runHealthCheck = useComponentHealthCheck(instanceParams); + const recordAction = useDeploymentRecordAction(); const { openedEntry } = useEditorView(); const packageUrl = openedEntry?.package; @@ -90,9 +93,12 @@ export function RedeployApplicationModal() { }, { onSuccess: async (result) => { - stream.markSettled('success', undefined, { result }); + const known = result?.rollout?.state !== 'unfollowable'; + stream.markSettled(known ? 'success' : 'inconclusive', undefined, { result }); invalidateComponents(); - const known = await reportDeployOutcome(project, result, runHealthCheck); + await reportDeployOutcome(project, result, runHealthCheck, { + action: recordAction(result?.deployment_id), + }); // Left open when there is something to read in it: an outcome Studio couldn't confirm, or a warning. if (known && !deployNotes(result).warn) { closeModal(); @@ -108,11 +114,12 @@ export function RedeployApplicationModal() { closeModal(); return; } + const action = recordAction(deployFailureDetails(error).deploymentId); if (error instanceof DeployRolloutError) { // Live on the node that received it; the rollout section names the nodes that did not take it. stream.markSettled('error', undefined, { failure: error }); invalidateComponents(); - toast.error(`${project} did not reach every node`, { description: error.message }); + toast.error(`${project} did not reach every node`, { description: error.message, action }); return; } stream.markSettled('error', error instanceof Error ? error.message : String(error), { @@ -120,12 +127,22 @@ export function RedeployApplicationModal() { }); toast.error(`Failed to redeploy ${project}`, { description: error instanceof Error ? error.message : undefined, + action, }); }, }, ); }, - [reDeployApplication, openedEntry, instanceParams, stream, invalidateComponents, runHealthCheck, closeModal], + [ + reDeployApplication, + openedEntry, + instanceParams, + stream, + invalidateComponents, + runHealthCheck, + closeModal, + recordAction, + ], ); const redeployBuffered = useCallback( @@ -139,31 +156,40 @@ export function RedeployApplicationModal() { { applicationName: openedEntry.project, applicationUrl, installCommand, ...instanceParams }, { onSuccess: (result) => { - if (result?.rollout?.state === 'unfollowable') { - toast.warning(`${openedEntry.project} redeployed on the node that received it`, { - id: toastId, - description: describeUnfollowable(result.rollout), - }); - } else { - toast.success(`Application ${openedEntry.project} redeployed successfully`, { id: toastId }); - } + reportDeployResult( + openedEntry.project, + result, + { title: `Application ${openedEntry.project} redeployed successfully` }, + { id: toastId, action: recordAction(result?.deployment_id) }, + ); invalidateComponents(); }, onError: (error) => { + // The modal is already closed, so a toast is the only place left to link the record from. + const action = recordAction(deployFailureDetails(error).deploymentId); if (error instanceof DeployRolloutError) { toast.error(`${openedEntry.project} did not reach every node`, { id: toastId, description: error.message, + action, }); invalidateComponents(); return; } + if (action) { + toast.error(`Failed to redeploy ${openedEntry.project}`, { + id: toastId, + description: describeError(error).message, + action, + }); + return; + } toast.dismiss(toastId); }, }, ); }, - [reDeployApplication, openedEntry, instanceParams, invalidateComponents, closeModal], + [reDeployApplication, openedEntry, instanceParams, invalidateComponents, closeModal, recordAction], ); const submitForm = ({ applicationUrl, installCommand }: { diff --git a/src/features/instance/applications/modals/reportDeployHealth.test.ts b/src/features/instance/applications/modals/reportDeployHealth.test.ts index 4b939e62c..e57d40b81 100644 --- a/src/features/instance/applications/modals/reportDeployHealth.test.ts +++ b/src/features/instance/applications/modals/reportDeployHealth.test.ts @@ -4,7 +4,7 @@ const toast = vi.hoisted(() => ({ success: vi.fn(), warning: vi.fn(), error: vi. vi.mock('sonner', () => ({ toast })); import { DeployComponentResult } from '@/integrations/api/instance/applications/deployComponentStream'; -import { deployNotes, reportDeployOutcome } from './reportDeployHealth'; +import { deployNotes, reportDeployOutcome, reportDeployResult } from './reportDeployHealth'; afterEach(() => { vi.clearAllMocks(); @@ -43,6 +43,45 @@ describe('reportDeployOutcome', () => { }); }); +describe('reportDeployResult', () => { + const success = { title: 'Application web redeployed successfully' }; + const action = { label: 'View record', onClick: () => {} }; + + it("replaces the loading toast with the caller's success, and links the record", () => { + reportDeployResult('web', { message: 'ok', rollout: { jobId: 'j', state: 'complete', nodes: [] } }, success, { + id: 't1', + action, + }); + expect(toast.success).toHaveBeenCalledWith(success.title, { id: 't1', action, description: undefined }); + }); + + it('does not call a legacy rollout that left a node unconfirmed a success', () => { + reportDeployResult('web', { + message: 'ok', + rollout: { + jobId: 'j', + state: 'complete', + nodes: [{ node: 'node-c', outcome: 'unconfirmed', detail: 'connect ECONNREFUSED' }], + }, + }, success); + expect(toast.success).not.toHaveBeenCalled(); + expect(toast.warning).toHaveBeenCalledWith('web deployed with warnings', { + description: 'The restart was not confirmed on node-c.', + }); + }); + + it('does not call a rollout it could not follow a success', () => { + reportDeployResult('web', { + message: 'ok', + rollout: { jobId: 'j', state: 'unfollowable', nodes: [], unfollowable: 'refused' }, + }, success); + expect(toast.success).not.toHaveBeenCalled(); + expect(toast.warning).toHaveBeenCalledWith('web deployed on the node that received it', { + description: expect.stringContaining("couldn't read this deploy's restart job"), + }); + }); +}); + describe('deployNotes', () => { it('explains an unchecked release without calling it a problem', () => { const notes = deployNotes({ message: 'ok', certification: 'uncertified' }); diff --git a/src/features/instance/applications/modals/reportDeployHealth.ts b/src/features/instance/applications/modals/reportDeployHealth.ts index 0abbd7a6b..a79d6c173 100644 --- a/src/features/instance/applications/modals/reportDeployHealth.ts +++ b/src/features/instance/applications/modals/reportDeployHealth.ts @@ -6,12 +6,17 @@ import { ComponentHealthResult } from '@/features/instance/applications/hooks/us import { DeployComponentResult } from '@/integrations/api/instance/applications/deployComponentStream'; import { toast } from 'sonner'; -/** What else the deploy said, beyond the component's health. `warn` when a person should look at it. */ -export interface DeployNotes { +interface DeployNotes { lines: string[]; warn: boolean; } +/** A toast's link to the deployment's record, and the toast to replace, when a caller has them. */ +export interface DeployToastOptions { + id?: string | number; + action?: { label: string; onClick: () => void }; +} + export function deployNotes(result: DeployComponentResult | undefined): DeployNotes { const lines: string[] = []; let warn = false; @@ -48,22 +53,28 @@ export function reportDeployHealth( project: string, health: ComponentHealthResult, notes: DeployNotes = { lines: [], warn: false }, + options: DeployToastOptions = {}, ): void { switch (health.level) { case 'healthy': if (notes.warn) { - toast.warning(`${project} deployed with warnings`, { description: withNotes(health.message, notes) }); + toast.warning(`${project} deployed with warnings`, { + ...options, + description: withNotes(health.message, notes), + }); return; } - toast.success(`${project} deployed and healthy`, { description: withNotes(health.message, notes) }); + toast.success(`${project} deployed and healthy`, { ...options, description: withNotes(health.message, notes) }); return; case 'warning': toast.warning(`${project} deployed with warnings`, { + ...options, description: withNotes(health.message ?? 'The component loaded but reported a warning.', notes), }); return; case 'error': toast.error(`${project} deployed but failed to load`, { + ...options, description: withNotes(health.message ?? 'The component reported an error after deploying.', notes), }); return; @@ -72,6 +83,7 @@ export function reportDeployHealth( case 'indeterminate': default: toast.success(`${project} deployed`, { + ...options, description: withNotes('Still verifying — the component is loading. Check its status shortly.', notes), }); return; @@ -89,13 +101,47 @@ export async function reportDeployOutcome( project: string, result: DeployComponentResult | undefined, runHealthCheck: (project: string) => Promise, + options: DeployToastOptions = {}, ): Promise { - if (result?.rollout?.state === 'unfollowable') { - toast.warning(`${project} deployed on the node that received it`, { - description: describeUnfollowable(result.rollout), - }); + if (reportUnfollowable(project, result, options)) { + return false; + } + reportDeployHealth(project, await runHealthCheck(project), deployNotes(result), options); + return true; +} + +/** + * Report a resolved deploy whose component is not health-checked, by what its rollout said. `success` is the toast + * for a deploy with nothing to add; it is replaced by a warning when a rollout is unknown or has something to read. + */ +export function reportDeployResult( + project: string, + result: DeployComponentResult | undefined, + success: { title: string; description?: string }, + options: DeployToastOptions = {}, +): void { + if (reportUnfollowable(project, result, options)) { + return; + } + const notes = deployNotes(result); + if (notes.warn) { + toast.warning(`${project} deployed with warnings`, { ...options, description: withNotes(undefined, notes) }); + return; + } + toast.success(success.title, { ...options, description: withNotes(success.description, notes) }); +} + +function reportUnfollowable( + project: string, + result: DeployComponentResult | undefined, + options: DeployToastOptions, +): boolean { + if (result?.rollout?.state !== 'unfollowable') { return false; } - reportDeployHealth(project, await runHealthCheck(project), deployNotes(result)); + toast.warning(`${project} deployed on the node that received it`, { + ...options, + description: describeUnfollowable(result.rollout), + }); return true; } diff --git a/src/integrations/api/instance/applications/deployComponent.test.ts b/src/integrations/api/instance/applications/deployComponent.test.ts index 13b54493d..45ca0050d 100644 --- a/src/integrations/api/instance/applications/deployComponent.test.ts +++ b/src/integrations/api/instance/applications/deployComponent.test.ts @@ -133,7 +133,7 @@ describe('deployAndFollowComponent', () => { await vi.advanceTimersByTimeAsync(5_000); const result = await pending; expect(result.rollout).toMatchObject({ state: 'complete', nodes: [{ node: 'node-b', outcome: 'activated' }] }); - expect(post).toHaveBeenCalledWith('/', { operation: 'get_job', id: 'job-1' }); + expect(post).toHaveBeenCalledWith('/', { operation: 'get_job', id: 'job-1' }, undefined); expect(onEvent.mock.calls.map(([event]) => [event.type, event.data.state])).toEqual([ ['rollout', 'following'], ['rollout', 'complete'], diff --git a/src/integrations/api/instance/applications/deployComponentStream.ts b/src/integrations/api/instance/applications/deployComponentStream.ts index 24537b696..c502a7778 100644 --- a/src/integrations/api/instance/applications/deployComponentStream.ts +++ b/src/integrations/api/instance/applications/deployComponentStream.ts @@ -32,7 +32,7 @@ export interface DeployPeerEvent { } /** Something the deploy wants a person to know without failing, e.g. a peer that installed different code (5.3.1+). */ -export interface DeployWarningEvent { +interface DeployWarningEvent { message?: string; } @@ -59,7 +59,6 @@ export interface DeployComponentResult extends ReplicatedResponse { rollout?: RolloutSnapshot; } -/** The deploy events a `deploy_component` or `get_deployment` stream carries; anything else is not one. */ export function toDeployStreamEvent(message: SSEMessage): DeployStreamEvent | undefined { switch (message.event) { case 'phase': diff --git a/src/integrations/api/instance/applications/deployOutcome.ts b/src/integrations/api/instance/applications/deployOutcome.ts index f552037cb..fd14761db 100644 --- a/src/integrations/api/instance/applications/deployOutcome.ts +++ b/src/integrations/api/instance/applications/deployOutcome.ts @@ -34,7 +34,7 @@ export function readCertificationDecision(value: unknown): CertificationDecision }; } -export interface DeployFailureDetails { +interface DeployFailureDetails { deploymentId?: string; certification?: CertificationDecision; } diff --git a/src/integrations/api/instance/applications/restartJob.test.ts b/src/integrations/api/instance/applications/restartJob.test.ts index 6a2e96161..5547e59b0 100644 --- a/src/integrations/api/instance/applications/restartJob.test.ts +++ b/src/integrations/api/instance/applications/restartJob.test.ts @@ -112,16 +112,28 @@ describe('readRestartJob', () => { }); it.each([ - ['a result that is not an object', { status: 'COMPLETE', result: 'done' }, 'complete'], - ['a non-array activated list', { status: 'COMPLETE', result: { activated: 'node-b' } }, 'complete'], ['entries that are not objects', { status: 'COMPLETE', result: { activated: [null, 3, 'x'] } }, 'complete'], ['an ERROR with no message', { status: 'ERROR' }, 'failed'], - ['a status in another case', { status: 'complete', result: {} }, 'complete'], + ['a status in another case', { status: 'complete', result: { replicated: [] } }, 'complete'], ['no status at all', {}, 'following'], ])('never throws on %s', (_label, job, state) => { expect(readRestartJob('job-1', { id: 'job-1', ...job } as never).state).toBe(state); }); + it.each([ + ['no result', { status: 'COMPLETE', message: 'Successfully completed job: job-1' }], + ['a result that is not an object', { status: 'COMPLETE', result: 'done' }], + ['a result in neither shape', { status: 'COMPLETE', result: { restarted: ['node-b'] } }], + ['a non-array activated list', { status: 'COMPLETE', result: { activated: 'node-b' } }], + ])('does not read a finished job with %s as a successful rollout', (_label, job) => { + expect(readRestartJob('job-1', { id: 'job-1', ...job } as never)).toEqual({ + jobId: 'job-1', + state: 'unfollowable', + nodes: [], + unfollowable: 'unrecognized', + }); + }); + it('names an entry with no node rather than dropping it', () => { const snapshot = readRestartJob('job-1', { id: 'job-1', @@ -170,7 +182,7 @@ describe('followRestartJob', () => { const snapshot = await pending; expect(snapshot.state).toBe('complete'); expect(post).toHaveBeenCalledTimes(3); - expect(post).toHaveBeenCalledWith('/', { operation: 'get_job', id: 'job-1' }); + expect(post).toHaveBeenCalledWith('/', { operation: 'get_job', id: 'job-1' }, undefined); expect(updates.map((s) => s.state)).toEqual(['following', 'complete']); }); @@ -236,6 +248,28 @@ describe('followRestartJob', () => { expect(post.mock.calls.length).toBeLessThan(200); }); + it('rejects, without reporting the outcome, when its signal aborts while a poll is in flight', async () => { + let answer: (value: { data: object[] }) => void = () => {}; + const post = vi.fn((_url: string, _body: object, config?: { signal?: AbortSignal }) => { + expect(config?.signal).toBeInstanceOf(AbortSignal); + return new Promise<{ data: object[] }>((resolve) => (answer = resolve)); + }); + const controller = new AbortController(); + const updates: RolloutSnapshot[] = []; + const pending = followRestartJob({ + instanceClient: { post } as unknown as InstanceClient, + jobId: 'job-1', + signal: controller.signal, + onUpdate: (s) => updates.push(s), + }); + pending.catch(() => {}); + await vi.advanceTimersByTimeAsync(0); + controller.abort(new Error('cancelled')); + answer({ data: [certifiedActivation] }); + await expect(pending).rejects.toThrow('cancelled'); + expect(updates.map((s) => s.state)).toEqual(['following']); + }); + it('rejects when its signal aborts', async () => { const { client } = makeClient([{ job: { id: 'job-1', status: 'IN_PROGRESS' } }]); const controller = new AbortController(); diff --git a/src/integrations/api/instance/applications/restartJob.ts b/src/integrations/api/instance/applications/restartJob.ts index 403c31504..f526a2fe5 100644 --- a/src/integrations/api/instance/applications/restartJob.ts +++ b/src/integrations/api/instance/applications/restartJob.ts @@ -10,20 +10,21 @@ import { isDeterministicRejection } from '@/react-query/pollUnlessForbidden'; * * `unfollowable` means Studio stopped without learning how the job ended, so it claims nothing. */ -export type RolloutState = 'following' | 'complete' | 'failed' | 'unfollowable'; +type RolloutState = 'following' | 'complete' | 'failed' | 'unfollowable'; /** * `not-found`: the node Studio reached has no such job. `hdb_job` does not replicate, so only the node * that received the deploy can answer for it, and a cluster's address may reach another node. + * `unrecognized`: the job ended with a result in neither shape Harper writes, so it says nothing about the nodes. */ -export type UnfollowableReason = 'not-found' | 'refused' | 'timed-out'; +type UnfollowableReason = 'not-found' | 'refused' | 'timed-out' | 'unrecognized'; /** * `activated` and `failed` come from a Harper 5.4+ activation job, where each node certifies the * release itself. `restarted` and `unconfirmed` come from the plain restart job older versions run: * there a node the job could not reach is listed with the error, and the job still completes. */ -export type RolloutNodeOutcome = 'activated' | 'restarted' | 'failed' | 'unconfirmed'; +type RolloutNodeOutcome = 'activated' | 'restarted' | 'failed' | 'unconfirmed'; export interface RolloutNode { node: string; @@ -76,7 +77,6 @@ function restartedNodes(entries: unknown[]): RolloutNode[] { }); } -/** Read one `get_job` row of a rolling deploy's restart job. */ export function readRestartJob(jobId: string, job: HarperJob): RolloutSnapshot { const status = typeof job.status === 'string' ? job.status.toUpperCase() : undefined; if (status === 'COMPLETE') { @@ -85,7 +85,10 @@ export function readRestartJob(jobId: string, job: HarperJob): RolloutSnapshot { ? activatedNodes(result.activated) : Array.isArray(result?.replicated) ? restartedNodes(result.replicated) - : []; + : undefined; + if (!nodes) { + return { jobId, state: 'unfollowable', nodes: [], unfollowable: 'unrecognized' }; + } return { jobId, state: nodes.some((node) => node.outcome === 'failed') ? 'failed' : 'complete', nodes }; } if (status === 'ERROR') { @@ -114,7 +117,7 @@ export function rolloutPollDelayMs(elapsedMs: number, fastMs: number): number { // again on the next request, so a missing row only counts once the job has stayed missing for a while. const NOT_FOUND_GRACE_MS = 20_000; -export interface FollowRestartJobParams extends InstanceClientConfig { +interface FollowRestartJobParams extends InstanceClientConfig { jobId: string; onUpdate?: (snapshot: RolloutSnapshot) => void; signal?: AbortSignal; @@ -148,7 +151,8 @@ export async function followRestartJob({ for (;;) { signal?.throwIfAborted(); try { - const job = await getJob({ jobId, instanceClient }); + const job = await getJob({ jobId, instanceClient, signal }); + signal?.throwIfAborted(); if (job) { missingSince = undefined; const snapshot = readRestartJob(jobId, job); @@ -162,6 +166,7 @@ export async function followRestartJob({ } } } catch (error) { + signal?.throwIfAborted(); // Anything else — a dropped connection, a 5xx while that node's own workers restart — passes. if (isDeterministicRejection(error)) { return unfollowable('refused'); diff --git a/src/integrations/api/instance/database/getJob.ts b/src/integrations/api/instance/database/getJob.ts index 60e91214c..62c2d566b 100644 --- a/src/integrations/api/instance/database/getJob.ts +++ b/src/integrations/api/instance/database/getJob.ts @@ -15,11 +15,13 @@ export interface HarperJob { job_body?: unknown; } -export async function getJob({ jobId, instanceClient }: { jobId: string } & InstanceClientConfig) { +export async function getJob( + { jobId, instanceClient, signal }: { jobId: string; signal?: AbortSignal } & InstanceClientConfig, +) { const { data } = await instanceClient.post('/', { operation: 'get_job', id: jobId, - }); + }, signal ? { signal } : undefined); return data?.[0]; } From 034dd9fa46c539cdb4065f36c7e1ae43dd5855db Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Mon, 12 Oct 2026 11:14:34 -0400 Subject: [PATCH 13/21] fix(deploy): a failed poll no longer counts toward a restart job's not-found grace followRestartJob gave up on a job once it had been missing for 20s, but measured that from the first miss even when every poll in between failed. The receiving node restarts its own workers before its peers (harper bin/restart.ts:336-355), so a cluster address that reached another node could read one miss, then errors while the job's node restarted, and end the rollout as not-found though the job was running. A miss now only counts across an unbroken run of misses. Co-Authored-By: Claude Opus 5.5 --- .../api/instance/applications/restartJob.test.ts | 12 ++++++++++++ .../api/instance/applications/restartJob.ts | 3 +++ 2 files changed, 15 insertions(+) diff --git a/src/integrations/api/instance/applications/restartJob.test.ts b/src/integrations/api/instance/applications/restartJob.test.ts index 5547e59b0..294c956d4 100644 --- a/src/integrations/api/instance/applications/restartJob.test.ts +++ b/src/integrations/api/instance/applications/restartJob.test.ts @@ -228,6 +228,18 @@ describe('followRestartJob', () => { expect((await pending).state).toBe('complete'); }); + it('does not count the time no node answered toward a missing job', async () => { + const { client } = makeClient([ + { rows: [] }, + ...Array.from({ length: 12 }, () => ({ reject: httpError(503) }) as Answer), + { rows: [] }, + { job: certifiedActivation }, + ]); + const pending = followRestartJob({ instanceClient: client, jobId: 'job-1' }); + await vi.advanceTimersByTimeAsync(40_000); + expect((await pending).state).toBe('complete'); + }); + it('stops at its ceiling without claiming the job ended', async () => { const { client } = makeClient([{ job: { id: 'job-1', status: 'IN_PROGRESS' } }]); const pending = followRestartJob({ instanceClient: client, jobId: 'job-1', timeoutMs: 120_000 }); diff --git a/src/integrations/api/instance/applications/restartJob.ts b/src/integrations/api/instance/applications/restartJob.ts index f526a2fe5..d054e7c53 100644 --- a/src/integrations/api/instance/applications/restartJob.ts +++ b/src/integrations/api/instance/applications/restartJob.ts @@ -171,6 +171,9 @@ export async function followRestartJob({ if (isDeterministicRejection(error)) { return unfollowable('refused'); } + // An answer that never came is not a miss, so the grace needs an unbroken run of misses. The + // receiving node restarts its own workers first, while another node can still answer. + missingSince = undefined; } const elapsed = Date.now() - startedAt; if (elapsed >= timeoutMs) { From 8eefb692d7debc92a911a9b1ff4a000eabce4f0f Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Sun, 11 Oct 2026 02:19:55 -0400 Subject: [PATCH 14/21] feat(deployments): filter and page the list, show staged releases and how each node installed The list filters by component and status, pages by the server's total, and shows each record's id, source (package, payload hash, or the release an activation made live) and user. A staged record (Harper 5.3+) is terminal and reads as waiting to be activated, so the list no longer polls it every 2s. The detail view shows the origin's install source, the activation's source release, and each peer's install comparison (5.3.1+), and drops the Restorable and Rollback-of fields no Harper version sets. The editor's Application menu links to the component's deployments. Refs #1786 Co-Authored-By: Claude Opus 5.5 --- .../components/ContentActions.tsx | 29 +++- .../hooks/useSupportsDeploymentSSE.ts | 10 ++ .../components/DeploymentDetail.tsx | 57 ++++++-- .../components/deploymentStatusBadge.tsx | 6 +- .../deployments/constants/tableDefinition.tsx | 48 +++++-- .../deployments/deploymentsSearch.test.ts | 28 ++++ .../config/deployments/deploymentsSearch.ts | 39 +++++ .../functions/deploymentDescriptions.test.ts | 52 +++++++ .../deployments/functions/deploymentSource.ts | 25 ++++ .../functions/describePeerInstall.ts | 26 ++++ .../config/deployments/index.test.tsx | 88 +++++++++++- .../instance/config/deployments/index.tsx | 133 ++++++++++++++++-- src/features/instance/config/routes.ts | 3 + .../deployments/deploymentPolling.test.ts | 3 + .../api/instance/deployments/types.test.ts | 1 + .../api/instance/deployments/types.ts | 27 +++- 16 files changed, 525 insertions(+), 50 deletions(-) create mode 100644 src/features/instance/config/deployments/deploymentsSearch.test.ts create mode 100644 src/features/instance/config/deployments/deploymentsSearch.ts create mode 100644 src/features/instance/config/deployments/functions/deploymentDescriptions.test.ts create mode 100644 src/features/instance/config/deployments/functions/deploymentSource.ts create mode 100644 src/features/instance/config/deployments/functions/describePeerInstall.ts diff --git a/src/features/instance/applications/components/ContentActions.tsx b/src/features/instance/applications/components/ContentActions.tsx index 6638d13dd..f63f7ecad 100644 --- a/src/features/instance/applications/components/ContentActions.tsx +++ b/src/features/instance/applications/components/ContentActions.tsx @@ -16,10 +16,13 @@ import { useEditorFileContent } from '@/features/instance/applications/context/e import { isDirectory } from '@/features/instance/applications/context/isDirectory'; import { useEditorView } from '@/features/instance/applications/hooks/useEditorView'; import { useEntryActions } from '@/features/instance/applications/hooks/useEntryActions'; -import { useInstanceBrowseManagePermission } from '@/hooks/usePermissions'; +import { useDeploymentsAvailable } from '@/features/instance/applications/hooks/useSupportsDeploymentSSE'; +import { useInstanceBrowseManagePermission, useInstanceManagePermission } from '@/hooks/usePermissions'; import { useRestartInstanceClick } from '@/hooks/useRestartInstanceClick'; import { emitToListeners, useEmitToListeners } from '@/lib/events/listener'; import { useSetWatchedValue, useWatchedValue } from '@/lib/events/watcher'; +import { buildAbsoluteLinkToPage } from '@/lib/urls/buildAbsoluteLinkToPage'; +import { useNavigate, useParams } from '@tanstack/react-router'; import { ArrowLeftIcon, ArrowRightIcon, @@ -28,6 +31,7 @@ import { DownloadIcon, FilePlusIcon, FolderPlusIcon, + HistoryIcon, LockIcon, PackageIcon, PanelRightCloseIcon, @@ -73,6 +77,14 @@ export function ContentActions({ const { openedEntryContents, openedEntry, isSavingFile, restrictPackageModification } = useEditorView(); const { content: updatedFileContent } = useEditorFileContent(openedEntry?.path); const canManageBrowseInstance = useInstanceBrowseManagePermission(); + // The Deployments page itself needs super-user and Harper 5.1. + const canManageInstance = useInstanceManagePermission(); + const deploymentsAvailable = useDeploymentsAvailable(); + const canViewDeployments = canManageInstance && deploymentsAvailable; + const navigate = useNavigate(); + const routeParams: { clusterId?: string; instanceId?: string; organizationId?: string } = useParams({ + strict: false, + }); const { onRestartClick, isRestartPending } = useRestartInstanceClick({ operation: 'restart_service', instanceClient: instanceParams.instanceClient, @@ -133,7 +145,8 @@ export function ContentActions({ useEntryActions(openedEntry); const showFileMenu = canEditFile || canRename || canAddEntries || canAddTable || canDeleteEntry; - const showApplicationMenu = canDownload || canManageBrowseInstance || canRedeploy; + const deploymentsProject = canViewDeployments && openedEntry?.project ? openedEntry.project : undefined; + const showApplicationMenu = canDownload || canManageBrowseInstance || canRedeploy || !!deploymentsProject; // The "File" menu acts on whatever is selected, so name it for that thing. const fileMenuLabel = openedEntry && isDirectory(openedEntry) ? 'Directory' : 'File'; @@ -389,6 +402,18 @@ export function ContentActions({ Redeploy Package )} + {deploymentsProject && ( + + void navigate({ + to: buildAbsoluteLinkToPage(routeParams, 'config/deployments'), + search: { project: deploymentsProject }, + })} + > + + View Deployments + + )} )} diff --git a/src/features/instance/applications/hooks/useSupportsDeploymentSSE.ts b/src/features/instance/applications/hooks/useSupportsDeploymentSSE.ts index deedf2c6b..d34e7ccdd 100644 --- a/src/features/instance/applications/hooks/useSupportsDeploymentSSE.ts +++ b/src/features/instance/applications/hooks/useSupportsDeploymentSSE.ts @@ -15,6 +15,16 @@ export function supportsDeployments(harperVersion: string | undefined): boolean return !!harperVersion && wasAReleasedBeforeB(MIN_DEPLOYMENT_SSE_VERSION, harperVersion); } +/** + * Staging (`activate: false`), activation by `deployment_id`, and the `staged` status ship in 5.3.0 (harper + * `v5.3.0:components/operations.js:638`). A 5.3.0 prerelease is refused, as `supportsPutOperation` refuses one. + */ +export const MIN_STAGED_DEPLOYMENT_VERSION = '5.3.0'; + +export function supportsStagedDeployments(harperVersion: string | undefined): boolean { + return !!harperVersion && wasAReleasedBeforeB(MIN_STAGED_DEPLOYMENT_VERSION, harperVersion); +} + /** * True when the instance/cluster is Harper >= 5.1.0, so the deployment history/detail * feature (`list_deployments` / `get_deployment`) is available. This gates feature diff --git a/src/features/instance/config/deployments/components/DeploymentDetail.tsx b/src/features/instance/config/deployments/components/DeploymentDetail.tsx index 2f68691b5..02afaf76e 100644 --- a/src/features/instance/config/deployments/components/DeploymentDetail.tsx +++ b/src/features/instance/config/deployments/components/DeploymentDetail.tsx @@ -3,14 +3,18 @@ import { Separator } from '@/components/ui/separator'; import { useInstanceClientIdParams } from '@/config/useInstanceClient'; import { DeployProgress } from '@/features/instance/applications/components/DeployProgress/DeployProgress'; import { useDeploymentStream } from '@/features/instance/applications/components/DeployProgress/useDeploymentStream'; +import { deploymentRecordPath } from '@/features/instance/applications/hooks/useDeploymentRecordAction'; import { useSupportsDeploymentSSE } from '@/features/instance/applications/hooks/useSupportsDeploymentSSE'; import { getDeploymentQueryOptions, getDeploymentStream } from '@/integrations/api/instance/deployments/getDeployment'; import { Deployment, isTerminalDeploymentStatus } from '@/integrations/api/instance/deployments/types'; +import { cn } from '@/lib/cn'; import { errorText } from '@/lib/errorText'; import { humanFileSize } from '@/lib/humanFileSize'; import { translateSecondsToAgo } from '@/lib/translateSecondsToAgo'; import { useQuery, useQueryClient } from '@tanstack/react-query'; +import { Link, useParams } from '@tanstack/react-router'; import { useEffect } from 'react'; +import { describePeerInstall } from '../functions/describePeerInstall'; import { DeploymentStatusBadge } from './deploymentStatusBadge'; function Field({ label, value }: { label: string; value: React.ReactNode }) { @@ -31,6 +35,7 @@ function timestamp(ms?: number) { export function DeploymentDetail({ deploymentId }: { deploymentId: string }) { const instanceParams = useInstanceClientIdParams(); + const params: { clusterId?: string; instanceId?: string; organizationId?: string } = useParams({ strict: false }); const sseSupported = useSupportsDeploymentSSE(); const queryClient = useQueryClient(); const stream = useDeploymentStream(); @@ -113,17 +118,32 @@ export function DeploymentDetail({ deploymentId }: { deploymentId: string }) { : undefined} /> {deployment.install_fingerprint.source} + : undefined} /> {deployment.rollback_of} + label="Activation of" + value={deployment.activated_from + ? ( + + {deployment.activated_from} + + ) : undefined} />
+ {deployment.status === 'staged' && ( +
+ Staged: built and installed, but not live. Activating it makes it live with nothing to install. +
+ )} + {showLiveProgress && ( <> @@ -152,17 +172,30 @@ export function DeploymentDetail({ deploymentId }: { deploymentId: string }) { <>
-
Replication
-
    +
    Other nodes
    +
      {deployment.peer_results.map((peer, i) => { const peerError = errorText(peer.error); + const install = describePeerInstall(peer); return ( -
    • - {peer.node} - - {peer.status ?? 'ok'} - {peerError ? `: ${peerError}` : ''} +
    • + + {peer.node} + + {peer.status ?? 'ok'} + {peerError ? `: ${peerError}` : ''} + + {install && ( + + {install.label} + + )}
      • ); })} diff --git a/src/features/instance/config/deployments/components/deploymentStatusBadge.tsx b/src/features/instance/config/deployments/components/deploymentStatusBadge.tsx index eb4f0ff4b..7033913c3 100644 --- a/src/features/instance/config/deployments/components/deploymentStatusBadge.tsx +++ b/src/features/instance/config/deployments/components/deploymentStatusBadge.tsx @@ -13,9 +13,10 @@ const STATUS_VARIANT: Record = { success: 'success', failed: 'destructive', rolled_back: 'destructive', + staged: 'secondary', }; -const STATUS_LABEL: Record = { +export const DEPLOYMENT_STATUS_LABEL: Record = { pending: 'Pending', extracting: 'Extracting', installing: 'Installing', @@ -25,8 +26,9 @@ const STATUS_LABEL: Record = { success: 'Success', failed: 'Failed', rolled_back: 'Rolled back', + staged: 'Staged', }; export function DeploymentStatusBadge({ status }: { status: DeploymentStatus }) { - return {STATUS_LABEL[status] ?? status}; + return {DEPLOYMENT_STATUS_LABEL[status] ?? status}; } diff --git a/src/features/instance/config/deployments/constants/tableDefinition.tsx b/src/features/instance/config/deployments/constants/tableDefinition.tsx index 81a03dfb7..83cfc3446 100644 --- a/src/features/instance/config/deployments/constants/tableDefinition.tsx +++ b/src/features/instance/config/deployments/constants/tableDefinition.tsx @@ -2,6 +2,7 @@ import { Deployment } from '@/integrations/api/instance/deployments/types'; import { ColumnDef, createColumnHelper } from '@/lib/table'; import { translateSecondsToAgo } from '@/lib/translateSecondsToAgo'; import { DeploymentStatusBadge } from '../components/deploymentStatusBadge'; +import { deploymentSource, shortDeploymentId } from '../functions/deploymentSource'; const columnHelper = createColumnHelper(); @@ -10,8 +11,18 @@ function ago(ms?: number) { } export const deploymentColumns: Array> = [ + columnHelper.display({ + header: 'Deployment', + id: 'deployment_id', + enableSorting: false, + cell: (props) => ( + + {shortDeploymentId(props.row.original.deployment_id)} + + ), + }), { - header: 'Project', + header: 'Component', accessorKey: 'project', enableSorting: false, }, @@ -19,28 +30,35 @@ export const deploymentColumns: Array> = [ header: 'Status', id: 'status', enableSorting: false, - cell: (props) => , - }), - columnHelper.display({ - header: 'Started', - id: 'started_at', - enableSorting: false, - cell: (props) => ago(props.row.original.started_at), + cell: (props) => ( + + + {props.row.original.status === 'staged' && ( + waiting to be activated + )} + + ), }), columnHelper.display({ - header: 'Completed', - id: 'completed_at', + header: 'Source', + id: 'source', enableSorting: false, - cell: (props) => ago(props.row.original.completed_at), + cell: (props) => { + const source = deploymentSource(props.row.original); + return source + ? {source.label} + : '—'; + }, }), { header: 'User', accessorKey: 'user', enableSorting: false, }, - { - header: 'Origin', - accessorKey: 'origin_node', + columnHelper.display({ + header: 'Started', + id: 'started_at', enableSorting: false, - }, + cell: (props) => ago(props.row.original.started_at), + }), ]; diff --git a/src/features/instance/config/deployments/deploymentsSearch.test.ts b/src/features/instance/config/deployments/deploymentsSearch.test.ts new file mode 100644 index 000000000..347e1e639 --- /dev/null +++ b/src/features/instance/config/deployments/deploymentsSearch.test.ts @@ -0,0 +1,28 @@ +import { describe, expect, it } from 'vitest'; +import { filterableStatuses, validateDeploymentsSearch } from './deploymentsSearch'; + +describe('validateDeploymentsSearch', () => { + it('keeps a component, a known status and a later page', () => { + expect(validateDeploymentsSearch({ project: ' web ', status: 'staged', page: '3' })) + .toEqual({ project: 'web', status: 'staged', page: 3 }); + }); + + it.each([ + ['an empty component', { project: ' ' }], + ['a status Harper does not filter on', { status: 'bogus' }], + ['the first page, which is the default', { page: 1 }], + ['a page that is not a whole number', { page: '2.5' }], + ['a negative page', { page: -2 }], + ['a component that is not text', { project: ['web'] }], + ])('drops %s', (_label, search) => { + expect(validateDeploymentsSearch(search)).toEqual({ project: undefined, status: undefined, page: undefined }); + }); +}); + +describe('filterableStatuses', () => { + it('offers staged only where staging exists (Harper 5.3.0+)', () => { + expect(filterableStatuses(true)).toContain('staged'); + expect(filterableStatuses(false)).not.toContain('staged'); + expect(filterableStatuses(false)).toContain('success'); + }); +}); diff --git a/src/features/instance/config/deployments/deploymentsSearch.ts b/src/features/instance/config/deployments/deploymentsSearch.ts new file mode 100644 index 000000000..bf555e0e3 --- /dev/null +++ b/src/features/instance/config/deployments/deploymentsSearch.ts @@ -0,0 +1,39 @@ +import { DeploymentStatus } from '@/integrations/api/instance/deployments/types'; + +/** The Deployments page's filters and page, kept in the URL so a link can open it filtered. */ +export interface DeploymentsSearch { + project?: string; + status?: DeploymentStatus; + /** 1-based. */ + page?: number; +} + +/** What `list_deployments` can filter on. `staged` only exists from Harper 5.3.0. */ +export const FILTERABLE_STATUSES: readonly DeploymentStatus[] = [ + 'pending', + 'extracting', + 'installing', + 'loading', + 'replicating', + 'restarting', + 'success', + 'failed', + 'staged', +]; + +export function filterableStatuses(stagingSupported: boolean): readonly DeploymentStatus[] { + return stagingSupported ? FILTERABLE_STATUSES : FILTERABLE_STATUSES.filter((status) => status !== 'staged'); +} + +export const DEPLOYMENTS_PAGE_SIZE = 25; + +export function validateDeploymentsSearch(search: Record): DeploymentsSearch { + const project = typeof search.project === 'string' ? search.project.trim() : ''; + const status = FILTERABLE_STATUSES.find((candidate) => candidate === search.status); + const page = Number(search.page); + return { + project: project || undefined, + status, + page: Number.isInteger(page) && page > 1 ? page : undefined, + }; +} diff --git a/src/features/instance/config/deployments/functions/deploymentDescriptions.test.ts b/src/features/instance/config/deployments/functions/deploymentDescriptions.test.ts new file mode 100644 index 000000000..f702becd9 --- /dev/null +++ b/src/features/instance/config/deployments/functions/deploymentDescriptions.test.ts @@ -0,0 +1,52 @@ +import { Deployment } from '@/integrations/api/instance/deployments/types'; +import { describe, expect, it } from 'vitest'; +import { deploymentSource } from './deploymentSource'; +import { describePeerInstall } from './describePeerInstall'; + +const base = { deployment_id: 'd', project: 'web', status: 'success' } as Deployment; + +describe('deploymentSource', () => { + it('names the package a deploy came from', () => { + expect(deploymentSource({ ...base, package_identifier: 'github:acme/web#v2' })) + .toEqual({ label: 'github:acme/web#v2', title: 'github:acme/web#v2' }); + }); + + it("names a payload deploy by its tarball's hash", () => { + expect(deploymentSource({ ...base, package_identifier: null, payload_hash: 'abcdef0123456789abcdef' })) + .toEqual({ label: 'Payload abcdef012345', title: 'abcdef0123456789abcdef' }); + }); + + it('names the release an activation made live, which built nothing', () => { + expect(deploymentSource({ ...base, activated_from: 'a1b2c3d4-0000-4000-8000-000000000001' })) + .toEqual({ label: 'Activation of a1b2c3d4', title: 'a1b2c3d4-0000-4000-8000-000000000001' }); + }); + + it('names nothing for a record that says nothing', () => { + expect(deploymentSource(base)).toBeUndefined(); + }); +}); + +// The comparison fields Harper records per peer: harper `components/deploymentRecorder.ts` installComparisonFields. +describe('describePeerInstall', () => { + it('says nothing for a record from before installs were compared (pre-5.3.1)', () => { + expect(describePeerInstall({ node: 'node-b', status: 'success' })).toBeUndefined(); + }); + + it('names what a peer installed differently', () => { + expect(describePeerInstall({ + node: 'node-b', + install: { source: 'npm:web@1.5.0', lockfiles: {} }, + install_matches: false, + install_differs: ['source', 'package-lock.json'], + })).toEqual({ label: 'Installed differently: source npm:web@1.5.0, package-lock.json.', matches: false }); + }); + + it('says a matching install matched', () => { + expect(describePeerInstall({ node: 'node-b', install_matches: true, install_differs: [] })?.matches).toBe(true); + }); + + it('does not read missing evidence as a match', () => { + expect(describePeerInstall({ node: 'node-b', install: null, install_matches: null, install_differs: [] })) + .toEqual({ label: "Couldn't compare its install: a fingerprint is missing or unidentified.", matches: null }); + }); +}); diff --git a/src/features/instance/config/deployments/functions/deploymentSource.ts b/src/features/instance/config/deployments/functions/deploymentSource.ts new file mode 100644 index 000000000..b7641e005 --- /dev/null +++ b/src/features/instance/config/deployments/functions/deploymentSource.ts @@ -0,0 +1,25 @@ +import { Deployment } from '@/integrations/api/instance/deployments/types'; + +export function shortDeploymentId(id: string): string { + return id.length > 8 ? id.slice(0, 8) : id; +} + +/** + * Where a release came from. A package deploy records its reference and a payload deploy its tarball's hash; an + * activation (5.3.0+) builds nothing, so it names the deployment whose release it made live. + */ +export function deploymentSource(deployment: Deployment): { label: string; title: string } | undefined { + if (deployment.activated_from) { + return { + label: `Activation of ${shortDeploymentId(deployment.activated_from)}`, + title: deployment.activated_from, + }; + } + if (deployment.package_identifier) { + return { label: deployment.package_identifier, title: deployment.package_identifier }; + } + if (deployment.payload_hash) { + return { label: `Payload ${deployment.payload_hash.slice(0, 12)}`, title: deployment.payload_hash }; + } + return undefined; +} diff --git a/src/features/instance/config/deployments/functions/describePeerInstall.ts b/src/features/instance/config/deployments/functions/describePeerInstall.ts new file mode 100644 index 000000000..c66ef2378 --- /dev/null +++ b/src/features/instance/config/deployments/functions/describePeerInstall.ts @@ -0,0 +1,26 @@ +import { DeploymentPeerResult } from '@/integrations/api/instance/deployments/types'; + +/** + * Whether a node installed what the node that received the deploy did (5.3.1+). Harper adds the comparison only + * on a deploy or a stage that compared fingerprints, so a record without it says nothing either way. + */ +export function describePeerInstall( + peer: DeploymentPeerResult, +): { label: string; matches: boolean | null } | undefined { + if (!('install_matches' in peer)) { + return undefined; + } + if (peer.install_matches === true) { + return { label: 'Installed the same as the node that received the deploy.', matches: true }; + } + if (peer.install_matches === false) { + const differs = (peer.install_differs ?? []).map((field) => + field === 'source' && peer.install?.source ? `source ${peer.install.source}` : field + ); + return { + label: `Installed differently: ${differs.length ? differs.join(', ') : 'see the deploy log'}.`, + matches: false, + }; + } + return { label: "Couldn't compare its install: a fingerprint is missing or unidentified.", matches: null }; +} diff --git a/src/features/instance/config/deployments/index.test.tsx b/src/features/instance/config/deployments/index.test.tsx index 4a621a354..9e7ea9ecf 100644 --- a/src/features/instance/config/deployments/index.test.tsx +++ b/src/features/instance/config/deployments/index.test.tsx @@ -1,6 +1,6 @@ /** @vitest-environment jsdom */ import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; -import { cleanup, render, screen } from '@testing-library/react'; +import { cleanup, fireEvent, render, screen } from '@testing-library/react'; import { afterEach, describe, expect, it, vi } from 'vitest'; import { ConfigDeploymentsIndex } from './index'; @@ -8,8 +8,16 @@ const state = vi.hoisted(() => ({ version: undefined as string | undefined, registrationFails: false, listEnabled: [] as (boolean | undefined)[], + listParams: [] as Record[], + listData: undefined as { deployments: object[]; total: number } | undefined, + search: {} as Record, + navigate: vi.fn(), +})); +vi.mock('@tanstack/react-router', () => ({ + useNavigate: () => state.navigate, + useParams: () => ({}), + useSearch: () => state.search, })); -vi.mock('@tanstack/react-router', () => ({ useNavigate: () => vi.fn(), useParams: () => ({}) })); vi.mock('@/hooks/usePermissions', () => ({ useInstanceManagePermission: () => true })); vi.mock('@/config/useInstanceClient', () => ({ useInstanceClientIdParams: () => ({ entityId: 'ins-a' }) })); vi.mock('@/integrations/api/instance/status/getRegistrationInfo', () => ({ @@ -21,9 +29,15 @@ vi.mock('@/integrations/api/instance/status/getRegistrationInfo', () => ({ }), })); vi.mock('@/integrations/api/instance/deployments/listDeployments', () => ({ - getDeploymentsQueryOptions: ({ enabled }: { enabled?: boolean }) => { - state.listEnabled.push(enabled); - return { queryKey: ['list_deployments'], queryFn: () => ({ deployments: [], total: 0 }), enabled: false }; + getDeploymentsQueryOptions: (params: { enabled?: boolean }) => { + state.listEnabled.push(params.enabled); + state.listParams.push(params); + return { + queryKey: ['list_deployments'], + queryFn: () => ({ deployments: [], total: 0 }), + enabled: false, + ...(state.listData ? { initialData: state.listData } : {}), + }; }, })); vi.mock('./modals/DeploymentDetailModal', () => ({ DeploymentDetailModal: () => null })); @@ -33,6 +47,10 @@ afterEach(() => { state.version = undefined; state.registrationFails = false; state.listEnabled = []; + state.listParams = []; + state.listData = undefined; + state.search = {}; + state.navigate.mockClear(); }); function renderPage(version: string | undefined) { @@ -71,3 +89,63 @@ describe('ConfigDeploymentsIndex version gate', () => { expect(screen.queryByText('No deployments yet.')).toBeNull(); }); }); + +const row = (id: string, extra: object = {}) => ({ + deployment_id: id, + project: 'web', + status: 'success', + started_at: 1, + ...extra, +}); + +describe('ConfigDeploymentsIndex filters and paging', () => { + it('asks list_deployments for the filtered page the URL names', () => { + state.search = { project: 'web', status: 'staged', page: 3 }; + renderPage('5.3.0'); + expect(state.listParams.at(-1)).toMatchObject({ project: 'web', status: 'staged', limit: 25, offset: 50 }); + }); + + it('pages by the total the server reports, not by the rows it sent', () => { + state.search = { page: 2 }; + state.listData = { deployments: Array.from({ length: 25 }, (_, i) => row(`d-${i}`)), total: 60 }; + renderPage('5.3.0'); + expect(screen.getByText('26–50 of 60')).toBeTruthy(); + fireEvent.click(screen.getByRole('button', { name: 'Next page' })); + const update = state.navigate.mock.calls.at(-1)?.[0] as { search: (prev: object) => object }; + expect(update.search({ page: 2, project: 'web' })).toEqual({ page: 3, project: 'web' }); + fireEvent.click(screen.getByRole('button', { name: 'Previous page' })); + const back = state.navigate.mock.calls.at(-1)?.[0] as { search: (prev: object) => object }; + expect(back.search({ page: 2 })).toEqual({ page: undefined }); + }); + + it('filters by component on Enter, and starts again from the first page', () => { + state.search = { page: 4 }; + renderPage('5.3.0'); + const input = screen.getByRole('textbox', { name: 'Component' }); + fireEvent.change(input, { target: { value: ' web ' } }); + fireEvent.submit(input.closest('form')!); + const update = state.navigate.mock.calls.at(-1)?.[0] as { search: (prev: object) => object }; + expect(update.search({ page: 4 })).toEqual({ page: undefined, project: 'web' }); + }); + + it('says the filters matched nothing rather than that there are no deployments', () => { + state.search = { project: 'nope' }; + state.listData = { deployments: [], total: 0 }; + renderPage('5.3.0'); + expect(screen.getByText('No deployments match these filters.')).toBeTruthy(); + }); + + it('shows a staged release as waiting, and an activation by the release it made live', () => { + state.listData = { + deployments: [ + row('a1b2c3d4-0000-4000-8000-000000000001', { status: 'staged', package_identifier: 'npm:web@2.0.0' }), + row('e5f6a7b8-0000-4000-8000-000000000002', { activated_from: 'a1b2c3d4-0000-4000-8000-000000000001' }), + ], + total: 2, + }; + renderPage('5.3.0'); + expect(screen.getByText('waiting to be activated')).toBeTruthy(); + expect(screen.getByText('npm:web@2.0.0')).toBeTruthy(); + expect(screen.getByText('Activation of a1b2c3d4')).toBeTruthy(); + }); +}); diff --git a/src/features/instance/config/deployments/index.tsx b/src/features/instance/config/deployments/index.tsx index ec984b3ab..17fc0dc46 100644 --- a/src/features/instance/config/deployments/index.tsx +++ b/src/features/instance/config/deployments/index.tsx @@ -1,24 +1,34 @@ import { SimpleBrowseDataTable } from '@/components/SimpleBrowseDataTable'; import { Button } from '@/components/ui/button'; +import { Input } from '@/components/ui/input'; +import { Select, SelectContent, SelectItem, SelectTrigger, SelectValue } from '@/components/ui/select'; import { useInstanceClientIdParams } from '@/config/useInstanceClient'; -import { supportsDeployments } from '@/features/instance/applications/hooks/useSupportsDeploymentSSE'; +import { + supportsDeployments, + supportsStagedDeployments, +} from '@/features/instance/applications/hooks/useSupportsDeploymentSSE'; import { useInstanceManagePermission } from '@/hooks/usePermissions'; import { useRefreshClick } from '@/hooks/useRefreshClick'; import { getDeploymentsQueryOptions } from '@/integrations/api/instance/deployments/listDeployments'; -import { Deployment } from '@/integrations/api/instance/deployments/types'; +import { Deployment, DeploymentStatus } from '@/integrations/api/instance/deployments/types'; import { getRegistrationInfoQueryOptions } from '@/integrations/api/instance/status/getRegistrationInfo'; import { Row } from '@/lib/table'; -import { useQuery } from '@tanstack/react-query'; -import { useNavigate, useParams } from '@tanstack/react-router'; -import { RefreshCwIcon } from 'lucide-react'; +import { keepPreviousData, useQuery } from '@tanstack/react-query'; +import { useNavigate, useParams, useSearch } from '@tanstack/react-router'; +import { ChevronLeftIcon, ChevronRightIcon, RefreshCwIcon } from 'lucide-react'; import { useCallback, useMemo } from 'react'; +import { DEPLOYMENT_STATUS_LABEL } from './components/deploymentStatusBadge'; import { deploymentColumns } from './constants/tableDefinition'; +import { DEPLOYMENTS_PAGE_SIZE, DeploymentsSearch, filterableStatuses } from './deploymentsSearch'; import { DeploymentDetailModal } from './modals/DeploymentDetailModal'; +const ALL_STATUSES = 'all'; + export function ConfigDeploymentsIndex() { const navigate = useNavigate(); const canManage = useInstanceManagePermission(); const { deploymentId }: { deploymentId?: string } = useParams({ strict: false }); + const { project, status, page = 1 }: DeploymentsSearch = useSearch({ strict: false }); const instanceParams = useInstanceClientIdParams(); // The nav hides this page below Harper 5.1, but a bookmark or back-navigation still lands here. const { data: registrationInfo, isError: registrationFailed } = useQuery( @@ -27,21 +37,47 @@ export function ConfigDeploymentsIndex() { const versionKnown = registrationInfo !== undefined; const versionUnavailable = !versionKnown && registrationFailed; const deploymentsAvailable = supportsDeployments(registrationInfo?.version); + const statuses = filterableStatuses(supportsStagedDeployments(registrationInfo?.version)); - const { data, isFetching, isRefetching, refetch, error } = useQuery( - getDeploymentsQueryOptions({ + const { data, isFetching, isRefetching, refetch, error } = useQuery({ + ...getDeploymentsQueryOptions({ ...instanceParams, - limit: 100, + project, + status, + limit: DEPLOYMENTS_PAGE_SIZE, + offset: (page - 1) * DEPLOYMENTS_PAGE_SIZE, enabled: canManage && deploymentsAvailable, pollWhileOpen: true, }), - ); + // A filter or page change keeps the rows on screen until the next ones arrive. + placeholderData: keepPreviousData, + }); const deployments = useMemo(() => data?.deployments ?? [], [data]); + const total = data?.total ?? 0; + const firstShown = total === 0 ? 0 : (page - 1) * DEPLOYMENTS_PAGE_SIZE + 1; + const lastShown = Math.min(total, (page - 1) * DEPLOYMENTS_PAGE_SIZE + deployments.length); + const filtered = !!project || !!status; + + const setSearch = useCallback( + (patch: DeploymentsSearch) => { + void navigate({ to: '.', search: (previous: DeploymentsSearch) => ({ ...previous, ...patch }) }); + }, + [navigate], + ); + const applyProject = useCallback( + (value: string) => { + const next = value.trim() || undefined; + if (next !== project) { + setSearch({ project: next, page: undefined }); + } + }, + [project, setSearch], + ); const onSelect = useCallback( (next: string | undefined) => { const parts = [deploymentId ? '..' : '', next].filter(Boolean); - void navigate({ to: parts.join('/') || '.' }); + void navigate({ to: parts.join('/') || '.', search: true }); }, [deploymentId, navigate], ); @@ -73,6 +109,39 @@ export function ConfigDeploymentsIndex() { columns={deploymentColumns} onRowClick={onRowClick} > + { + event.preventDefault(); + applyProject(String(new FormData(event.currentTarget).get('project') ?? '')); + }} + > + applyProject(event.currentTarget.value)} + className="w-48" + /> + + + + ) + : filtered + ? 'No deployments match these filters.' + : 'No deployments yet.'} +
+ )} + {deployments.length > 0 && ( +
+ + {firstShown}–{lastShown} of {total} + + + +
)} {!!deploymentId && ( diff --git a/src/features/instance/config/routes.ts b/src/features/instance/config/routes.ts index ee2b40bef..55713d9a2 100644 --- a/src/features/instance/config/routes.ts +++ b/src/features/instance/config/routes.ts @@ -1,5 +1,6 @@ import { ConfigCertificatesIndex } from '@/features/instance/config/certificates'; import { ConfigDeploymentsIndex } from '@/features/instance/config/deployments'; +import { validateDeploymentsSearch } from '@/features/instance/config/deployments/deploymentsSearch'; import { ConfigDomainsIndex } from '@/features/instance/config/domains'; import { ConfigIndex } from '@/features/instance/config/index'; import { ConfigOverviewIndex } from '@/features/instance/config/overview'; @@ -76,12 +77,14 @@ export function createConfigRouteTree(instanceLayoutRoute: ReturnType instanceConfigRoute, path: 'deployments', + validateSearch: validateDeploymentsSearch, head: () => ({ meta: [{ title: 'Deployments — Harper Fabric' }] }), component: ConfigDeploymentsIndex, }); const instanceConfigDeploymentRoute = createRoute({ getParentRoute: () => instanceConfigRoute, path: 'deployments/$deploymentId', + validateSearch: validateDeploymentsSearch, head: () => ({ meta: [{ title: 'Deployments — Harper Fabric' }] }), component: ConfigDeploymentsIndex, }); diff --git a/src/integrations/api/instance/deployments/deploymentPolling.test.ts b/src/integrations/api/instance/deployments/deploymentPolling.test.ts index 3ba1e9f83..a1ad1a822 100644 --- a/src/integrations/api/instance/deployments/deploymentPolling.test.ts +++ b/src/integrations/api/instance/deployments/deploymentPolling.test.ts @@ -36,6 +36,8 @@ describe('get_deployment polling', () => { const poll = detailInterval(); expect(poll({ state: { error: null, data: deployment('installing') } })).toBe(2_000); expect(poll({ state: { error: null, data: deployment('success') } })).toBe(false); + // A staged record never changes again; polling it every 2s would run for as long as the view is open. + expect(poll({ state: { error: null, data: deployment('staged') } })).toBe(false); }); it('does not poll a detail view that never asked to', () => { @@ -62,6 +64,7 @@ describe('list_deployments polling', () => { const poll = listInterval() as IntervalFn; expect(poll({ state: { error: null, data: list('success', 'installing') } })).toBe(2_000); expect(poll({ state: { error: null, data: list('success', 'failed') } })).toBe(10_000); + expect(poll({ state: { error: null, data: list('success', 'staged') } })).toBe(10_000); expect(poll({ state: { error: null } })).toBe(10_000); }); diff --git a/src/integrations/api/instance/deployments/types.test.ts b/src/integrations/api/instance/deployments/types.test.ts index 3c2dd27ef..3c21e25ca 100644 --- a/src/integrations/api/instance/deployments/types.test.ts +++ b/src/integrations/api/instance/deployments/types.test.ts @@ -12,6 +12,7 @@ describe('isTerminalDeploymentStatus', () => { ['success', true], ['failed', true], ['rolled_back', true], + ['staged', true], ])('%s -> terminal=%s', (status, expected) => { expect(isTerminalDeploymentStatus(status)).toBe(expected); }); diff --git a/src/integrations/api/instance/deployments/types.ts b/src/integrations/api/instance/deployments/types.ts index f86693480..9c9254c8b 100644 --- a/src/integrations/api/instance/deployments/types.ts +++ b/src/integrations/api/instance/deployments/types.ts @@ -8,12 +8,16 @@ export type DeploymentStatus = | 'restarting' | 'success' | 'failed' - | 'rolled_back'; + | 'rolled_back' + /** Built and installed on every node, not live (5.3.0+). Activating it writes a record of its own. */ + | 'staged'; +// Harper's own set (harper `components/deploymentOperations.ts:19`): a staged record never changes again. export const TERMINAL_DEPLOYMENT_STATUSES: ReadonlySet = new Set([ 'success', 'failed', 'rolled_back', + 'staged', ]); export function isTerminalDeploymentStatus(status: DeploymentStatus | undefined): boolean { @@ -27,6 +31,15 @@ export interface DeploymentEventLogEntry { data: unknown; } +/** + * What an install came from (5.3.1+): `npm:@`, `git:`, `integrity:` or `unidentified`, + * absent for a payload; and the sha256 of each root lockfile, or why it could not be read. + */ +export interface InstallFingerprint { + source?: string; + lockfiles?: Record; +} + export interface DeploymentPeerResult { node: string; status?: string; @@ -34,6 +47,12 @@ export interface DeploymentPeerResult { error?: string | DeploymentPeerError | null; started_at?: number; completed_at?: number; + /** Present once the origin compared this peer's install with its own (5.3.1+). */ + install?: InstallFingerprint | null; + /** `false` when anything differs, `null` when the evidence is missing or unidentified, `true` when it all matches. */ + install_matches?: boolean | null; + /** `source` and/or the lockfile names that differ. */ + install_differs?: string[]; } export interface DeploymentPeerError { @@ -68,8 +87,10 @@ export interface Deployment { started_at?: number; completed_at?: number; user?: string; - restorable?: boolean; - rollback_of?: string | null; + /** On an activation (5.3.0+), the deployment whose staged or kept release it made live. */ + activated_from?: string | null; + /** The origin's own install, on a deploy or a stage (5.3.1+); an activation installs nothing. */ + install_fingerprint?: InstallFingerprint | null; error?: DeploymentError | null; } From 806599e11f90f793fc64fa3ca6ea3c03498d5660 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Sun, 11 Oct 2026 02:44:55 -0400 Subject: [PATCH 15/21] fix(deployments): no stale rows across a page change, and no staged filter where staging is absent The list no longer keeps the previous page's rows on screen, clickable, while the next page loads. A saved ?status=staged link opened against an instance older than 5.3 no longer asks for a status it cannot have. The component filter applies on Enter only, so leaving the field to click Next does not reset the page under the click, and it follows an outside change without remounting the field. Refs #1786 Co-Authored-By: Claude Opus 5.5 --- .../config/deployments/deploymentsSearch.ts | 3 +- .../config/deployments/index.test.tsx | 15 +++++++++ .../instance/config/deployments/index.tsx | 31 +++++++++++-------- .../deployments/deploymentPolling.test.ts | 1 - 4 files changed, 34 insertions(+), 16 deletions(-) diff --git a/src/features/instance/config/deployments/deploymentsSearch.ts b/src/features/instance/config/deployments/deploymentsSearch.ts index bf555e0e3..084369a24 100644 --- a/src/features/instance/config/deployments/deploymentsSearch.ts +++ b/src/features/instance/config/deployments/deploymentsSearch.ts @@ -1,6 +1,5 @@ import { DeploymentStatus } from '@/integrations/api/instance/deployments/types'; -/** The Deployments page's filters and page, kept in the URL so a link can open it filtered. */ export interface DeploymentsSearch { project?: string; status?: DeploymentStatus; @@ -8,7 +7,7 @@ export interface DeploymentsSearch { page?: number; } -/** What `list_deployments` can filter on. `staged` only exists from Harper 5.3.0. */ +// `rolled_back` is left out: no Harper version writes it, so the filter could only ever come back empty. export const FILTERABLE_STATUSES: readonly DeploymentStatus[] = [ 'pending', 'extracting', diff --git a/src/features/instance/config/deployments/index.test.tsx b/src/features/instance/config/deployments/index.test.tsx index 9e7ea9ecf..b3bf1efbe 100644 --- a/src/features/instance/config/deployments/index.test.tsx +++ b/src/features/instance/config/deployments/index.test.tsx @@ -105,6 +105,21 @@ describe('ConfigDeploymentsIndex filters and paging', () => { expect(state.listParams.at(-1)).toMatchObject({ project: 'web', status: 'staged', limit: 25, offset: 50 }); }); + it('does not ask a pre-5.3 instance for staged records a saved link names', () => { + state.search = { status: 'staged' }; + renderPage('5.2.15'); + expect(state.listParams.at(-1)).toMatchObject({ status: undefined }); + }); + + it('applies the component filter on Enter only, so leaving the field does not move the page', () => { + state.search = { page: 2 }; + renderPage('5.3.0'); + const input = screen.getByRole('textbox', { name: 'Component' }); + fireEvent.change(input, { target: { value: 'web' } }); + fireEvent.blur(input); + expect(state.navigate).not.toHaveBeenCalled(); + }); + it('pages by the total the server reports, not by the rows it sent', () => { state.search = { page: 2 }; state.listData = { deployments: Array.from({ length: 25 }, (_, i) => row(`d-${i}`)), total: 60 }; diff --git a/src/features/instance/config/deployments/index.tsx b/src/features/instance/config/deployments/index.tsx index 17fc0dc46..c5c97bd1f 100644 --- a/src/features/instance/config/deployments/index.tsx +++ b/src/features/instance/config/deployments/index.tsx @@ -13,10 +13,10 @@ import { getDeploymentsQueryOptions } from '@/integrations/api/instance/deployme import { Deployment, DeploymentStatus } from '@/integrations/api/instance/deployments/types'; import { getRegistrationInfoQueryOptions } from '@/integrations/api/instance/status/getRegistrationInfo'; import { Row } from '@/lib/table'; -import { keepPreviousData, useQuery } from '@tanstack/react-query'; +import { useQuery } from '@tanstack/react-query'; import { useNavigate, useParams, useSearch } from '@tanstack/react-router'; import { ChevronLeftIcon, ChevronRightIcon, RefreshCwIcon } from 'lucide-react'; -import { useCallback, useMemo } from 'react'; +import { useCallback, useEffect, useMemo, useRef } from 'react'; import { DEPLOYMENT_STATUS_LABEL } from './components/deploymentStatusBadge'; import { deploymentColumns } from './constants/tableDefinition'; import { DEPLOYMENTS_PAGE_SIZE, DeploymentsSearch, filterableStatuses } from './deploymentsSearch'; @@ -28,7 +28,7 @@ export function ConfigDeploymentsIndex() { const navigate = useNavigate(); const canManage = useInstanceManagePermission(); const { deploymentId }: { deploymentId?: string } = useParams({ strict: false }); - const { project, status, page = 1 }: DeploymentsSearch = useSearch({ strict: false }); + const { project, status: requestedStatus, page = 1 }: DeploymentsSearch = useSearch({ strict: false }); const instanceParams = useInstanceClientIdParams(); // The nav hides this page below Harper 5.1, but a bookmark or back-navigation still lands here. const { data: registrationInfo, isError: registrationFailed } = useQuery( @@ -38,9 +38,11 @@ export function ConfigDeploymentsIndex() { const versionUnavailable = !versionKnown && registrationFailed; const deploymentsAvailable = supportsDeployments(registrationInfo?.version); const statuses = filterableStatuses(supportsStagedDeployments(registrationInfo?.version)); + // A saved `?status=staged` opened against an instance without staging asks for nothing it can answer. + const status = requestedStatus && statuses.includes(requestedStatus) ? requestedStatus : undefined; - const { data, isFetching, isRefetching, refetch, error } = useQuery({ - ...getDeploymentsQueryOptions({ + const { data, isFetching, isRefetching, refetch, error } = useQuery( + getDeploymentsQueryOptions({ ...instanceParams, project, status, @@ -49,9 +51,7 @@ export function ConfigDeploymentsIndex() { enabled: canManage && deploymentsAvailable, pollWhileOpen: true, }), - // A filter or page change keeps the rows on screen until the next ones arrive. - placeholderData: keepPreviousData, - }); + ); const deployments = useMemo(() => data?.deployments ?? [], [data]); const total = data?.total ?? 0; const firstShown = total === 0 ? 0 : (page - 1) * DEPLOYMENTS_PAGE_SIZE + 1; @@ -84,6 +84,13 @@ export function ConfigDeploymentsIndex() { const onRowClick = useCallback((row: Row) => onSelect(row.original.deployment_id), [onSelect]); const closeModal = useCallback(() => onSelect(undefined), [onSelect]); const onRefreshClick = useRefreshClick(refetch); + const projectInput = useRef(null); + useEffect(() => { + // Follows a filter set from outside the field, such as the editor's link, without remounting it. + if (projectInput.current && projectInput.current.value.trim() !== (project ?? '')) { + projectInput.current.value = project ?? ''; + } + }, [project]); if (!canManage) { return ( @@ -117,14 +124,12 @@ export function ConfigDeploymentsIndex() { }} > applyProject(event.currentTarget.value)} - className="w-48" + className="w-56" />