From 60f936a10b7a9038ea1cc9aff01f5e9db10a790c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9sar=20Arroba?= <19954079+cesararroba@users.noreply.github.com> Date: Thu, 24 Sep 2026 11:23:56 +0200 Subject: [PATCH] fix(ui): wait for the full provider connection check before reporting a result (#12869) --- ui/actions/organizations/organizations.ts | 2 +- ui/actions/providers/providers.ts | 115 ++++-- .../provider-connection-check-wait.fixed.md | 1 + .../forms/delete-organization-form.tsx | 5 +- .../use-org-account-selection-flow.test.ts | 286 +++++++++++++- .../hooks/use-org-account-selection-flow.ts | 108 +++++- .../org-account-selection.utils.test.ts | 235 +++++++++++- .../org-account-selection.utils.ts | 115 +++++- .../table/data-table-row-actions.test.tsx | 213 ++++++++++- .../table/data-table-row-actions.tsx | 64 +++- .../wizard/provider-wizard-modal.test.tsx | 8 +- .../steps/aws/aws-connect-step.test.tsx | 62 +++- .../wizard/steps/aws/aws-connect-step.tsx | 125 +++++-- .../forms/test-connection-form.test.tsx | 185 ++++++++++ .../workflow/forms/test-connection-form.tsx | 63 +++- .../shadcn/tree-view/tree-status-icon.tsx | 16 +- .../tree-view/tree-status-indicator.tsx | 8 +- ui/lib/helper.test.ts | 41 ++- ui/lib/helper.ts | 6 +- ui/lib/provider-helpers.test.ts | 348 +++++++++++++++++- ui/lib/provider-helpers.ts | 158 +++++++- ui/types/providers.ts | 12 + ui/types/tasks.ts | 15 + ui/types/tree.ts | 7 +- 24 files changed, 2024 insertions(+), 174 deletions(-) create mode 100644 ui/changelog.d/provider-connection-check-wait.fixed.md create mode 100644 ui/components/providers/workflow/forms/test-connection-form.test.tsx diff --git a/ui/actions/organizations/organizations.ts b/ui/actions/organizations/organizations.ts index 69509f0376..761c08acfb 100644 --- a/ui/actions/organizations/organizations.ts +++ b/ui/actions/organizations/organizations.ts @@ -546,7 +546,7 @@ export const applyDiscovery = async ( ); // No `include`: the apply view rejects the parameter outright and fails the // whole request. The created providers' uids are read afterwards instead, with - // `getProviderUidsByIds`. + // `getProviderUidsAndConnectionBaselines`. const attributes = buildApplyAttributes(payload); diff --git a/ui/actions/providers/providers.ts b/ui/actions/providers/providers.ts index a862c488af..76e57b30ba 100644 --- a/ui/actions/providers/providers.ts +++ b/ui/actions/providers/providers.ts @@ -151,28 +151,28 @@ export const getProvider = async (formData: FormData) => { const PROVIDERS_PAGE_MAX = 100; /** - * Uids of the given providers, keyed by provider id. A provider's `uid` is the - * candidate it was created for (AWS account id / GCP project id), so this is what - * matches an apply's created providers back to the selection. Batched with - * `filter[id__in]` rather than one `GET /providers/{id}` per id. + * Providers matching the given ids, batched with `filter[id__in]` (page size + * `PROVIDERS_PAGE_MAX`, the server max, which also bounds the id batch size) + * rather than one `GET /providers/{id}` per id. Shared by every action below + * that resolves providers by id; a batch that fails to fetch leaves its + * providers out of the result rather than failing the rest, and it is on each + * caller to say what "missing" means for its own map. Not exported: an + * exported function in this `"use server"` module becomes a callable server + * action. */ -export const getProviderUidsByIds = async ( +const fetchProvidersByIds = async ( providerIds: string[], -): Promise> => { +): Promise => { const uniqueIds = Array.from(new Set(providerIds.filter(Boolean))); if (uniqueIds.length === 0) { - return {}; + return []; } const headers = await getAuthHeaders({ contentType: false }); - const batches: string[][] = []; + const providers: ProvidersApiResponse["data"] = []; + for (let start = 0; start < uniqueIds.length; start += PROVIDERS_PAGE_MAX) { - batches.push(uniqueIds.slice(start, start + PROVIDERS_PAGE_MAX)); - } - - const uidById: Record = {}; - - for (const batch of batches) { + const batch = uniqueIds.slice(start, start + PROVIDERS_PAGE_MAX); const url = new URL(`${apiBaseUrl}/providers`); url.searchParams.set("filter[id__in]", batch.join(",")); url.searchParams.set("page[size]", String(PROVIDERS_PAGE_MAX)); @@ -183,20 +183,93 @@ export const getProviderUidsByIds = async ( | ProvidersApiResponse | undefined; - for (const provider of result?.data ?? []) { - const uid = provider?.attributes?.uid; - if (typeof provider?.id === "string" && typeof uid === "string") { - uidById[provider.id] = uid; - } - } + providers.push(...(result?.data ?? [])); } catch { - // A failed batch leaves its providers unmapped rather than failing the rest. + // A failed batch leaves its providers out of the result rather than + // failing the rest. + } + } + + return providers; +}; + +/** + * Uids of the given providers, keyed by provider id. A provider's `uid` is the + * candidate it was created for (AWS account id / GCP project id), so this is what + * matches an apply's created providers back to the selection. + */ +export const getProviderUidsByIds = async ( + providerIds: string[], +): Promise> => { + const uidById: Record = {}; + + for (const provider of await fetchProvidersByIds(providerIds)) { + const uid = provider?.attributes?.uid; + if (typeof provider?.id === "string" && typeof uid === "string") { + uidById[provider.id] = uid; } } return uidById; }; +/** + * `connection.last_checked_at` for each given provider, keyed by id. Read before + * a batch of connection checks is dispatched, so `resolveProviderConnectionState` + * can tell a check's own result apart from an older one already on record by + * comparing values, never by comparing the browser's clock against the server's + * (see that function for why). A provider missing from the response -- the batch + * read failed, or it was deleted mid-flight -- is left out of the map rather than + * defaulted, so callers can tell "no prior check" (`null`) from "unknown". + */ +export const getProviderConnectionBaselines = async ( + providerIds: string[], +): Promise> => { + const baselineById: Record = {}; + + for (const provider of await fetchProvidersByIds(providerIds)) { + if (typeof provider?.id === "string") { + baselineById[provider.id] = + provider.attributes?.connection?.last_checked_at ?? null; + } + } + + return baselineById; +}; + +/** + * Uid and `connection.last_checked_at` for each given provider, keyed by id, read + * with a single batched `filter[id__in]` request. The organization onboarding + * apply step needs both right after creating providers: the uid to match each one + * back to the candidate it was created for (see `getProviderUidsByIds`), and the + * baseline to compare a dispatched check's result against (see + * `getProviderConnectionBaselines`). Reading them together avoids fetching the + * same set of providers twice. + */ +export const getProviderUidsAndConnectionBaselines = async ( + providerIds: string[], +): Promise<{ + uidById: Record; + baselineById: Record; +}> => { + const uidById: Record = {}; + const baselineById: Record = {}; + + for (const provider of await fetchProvidersByIds(providerIds)) { + if (typeof provider?.id !== "string") { + continue; + } + const uid = provider.attributes?.uid; + if (typeof uid === "string") { + uidById[provider.id] = uid; + } + baselineById[provider.id] = + provider.attributes?.connection?.last_checked_at ?? null; + } + + return { uidById, baselineById }; +}; + export const updateProvider = async (formData: FormData) => { const headers = await getAuthHeaders({ contentType: true }); const providerId = formData.get(ProviderCredentialFields.PROVIDER_ID); diff --git a/ui/changelog.d/provider-connection-check-wait.fixed.md b/ui/changelog.d/provider-connection-check-wait.fixed.md new file mode 100644 index 0000000000..779efa5e74 --- /dev/null +++ b/ui/changelog.d/provider-connection-check-wait.fixed.md @@ -0,0 +1 @@ +Provider connection test no longer reports `Max retries exceeded` for checks that take longer than 30 seconds, such as networks where some AWS endpoints are unreachable; the wait now covers the backend task's full time limit and falls back to the provider's current connection state if it is still exhausted diff --git a/ui/components/providers/forms/delete-organization-form.tsx b/ui/components/providers/forms/delete-organization-form.tsx index 4a12c18587..c777575066 100644 --- a/ui/components/providers/forms/delete-organization-form.tsx +++ b/ui/components/providers/forms/delete-organization-form.tsx @@ -12,6 +12,7 @@ import { pollTaskCompletion } from "@/components/providers/organizations/org-acc import { Button, useToast } from "@/components/shadcn"; import { getNodeLabel } from "@/lib/organizations"; import { NodeKind, OrganizationType } from "@/types/organizations"; +import { CONNECTION_CHECK_STATUS } from "@/types/providers"; import { PROVIDERS_GROUP_KIND, ProvidersGroupKind, @@ -85,11 +86,11 @@ export function DeleteOrganizationForm({ const taskId = extractTaskId(result); const taskResult = taskId ? await pollTaskCompletion(taskId) - : { success: true as const }; + : { status: CONNECTION_CHECK_STATUS.SUCCESS }; setIsLoading(false); - if (!taskResult.success) { + if (taskResult.status !== CONNECTION_CHECK_STATUS.SUCCESS) { toast({ variant: "destructive", title: "Deletion did not complete", diff --git a/ui/components/providers/organizations/hooks/use-org-account-selection-flow.test.ts b/ui/components/providers/organizations/hooks/use-org-account-selection-flow.test.ts index 46833ae4f0..78b7e4430d 100644 --- a/ui/components/providers/organizations/hooks/use-org-account-selection-flow.test.ts +++ b/ui/components/providers/organizations/hooks/use-org-account-selection-flow.test.ts @@ -8,6 +8,7 @@ import { type GcpOrgHierarchy, ORGANIZATION_TYPE, } from "@/types/organizations"; +import { CONNECTION_CHECK_STATUS } from "@/types/providers"; import { useOrgAccountSelectionFlow } from "./use-org-account-selection-flow"; @@ -15,13 +16,25 @@ const organizationsActionsMock = vi.hoisted(() => ({ applyDiscovery: vi.fn(), })); const providersActionsMock = vi.hoisted(() => ({ - getProviderUidsByIds: vi.fn(), + getProviderConnectionBaselines: vi.fn(), + getProviderUidsAndConnectionBaselines: vi.fn(), revalidateProviders: vi.fn(), startProviderConnectionChecks: vi.fn(), })); const tasksActionsMock = vi.hoisted(() => ({ getTasksByIds: vi.fn(), })); +const providerHelpersMock = vi.hoisted(() => ({ + resolveProviderConnectionState: vi.fn(), +})); +const pollConnectionTasksMock = vi.hoisted(() => vi.fn()); +// Mutable holder for the real `pollConnectionTasks`, captured once the module +// mock factory below runs, and re-applied in `beforeEach` since +// `mockReset: true` clears `pollConnectionTasksMock`'s implementation before +// every test. +const realPollConnectionTasksHolder = vi.hoisted( + () => ({}) as { current?: (...args: unknown[]) => unknown }, +); vi.mock( "@/actions/organizations/organizations", @@ -29,6 +42,15 @@ vi.mock( ); vi.mock("@/actions/providers/providers", () => providersActionsMock); vi.mock("@/actions/task/tasks", () => tasksActionsMock); +vi.mock("@/lib/provider-helpers", () => providerHelpersMock); +vi.mock("../org-account-selection.utils", async (importOriginal) => { + const actual = + await importOriginal(); + realPollConnectionTasksHolder.current = actual.pollConnectionTasks as ( + ...args: unknown[] + ) => unknown; + return { ...actual, pollConnectionTasks: pollConnectionTasksMock }; +}); const ORGANIZATION_UID = "organizations/123456789012"; const PROJECT_UID = "projects/acme-prod"; @@ -55,6 +77,7 @@ function seedAppliedSelection() { interface RenderedFlow { onNext: ReturnType; startTesting: () => Promise; + getFooterConfig: () => WizardFooterConfig | null; } function renderFlow(): RenderedFlow { @@ -79,6 +102,7 @@ function renderFlow(): RenderedFlow { footerConfig?.onAction?.(); }); }, + getFooterConfig: () => footerConfig, }; } @@ -91,18 +115,27 @@ describe("useOrgAccountSelectionFlow", () => { ...Object.values(organizationsActionsMock), ...Object.values(providersActionsMock), ...Object.values(tasksActionsMock), + ...Object.values(providerHelpersMock), ]) { mockFn.mockReset(); } + pollConnectionTasksMock.mockReset(); + pollConnectionTasksMock.mockImplementation((...args: unknown[]) => + realPollConnectionTasksHolder.current?.(...args), + ); organizationsActionsMock.applyDiscovery.mockResolvedValue({ data: { relationships: { providers: { data: [{ id: PROVIDER_ID }] } }, }, }); - providersActionsMock.getProviderUidsByIds.mockResolvedValue({ - [PROVIDER_ID]: PROJECT_UID, - }); + providersActionsMock.getProviderUidsAndConnectionBaselines.mockResolvedValue( + { + uidById: { [PROVIDER_ID]: PROJECT_UID }, + baselineById: {}, + }, + ); + providersActionsMock.getProviderConnectionBaselines.mockResolvedValue({}); providersActionsMock.revalidateProviders.mockResolvedValue(undefined); }); @@ -156,5 +189,250 @@ describe("useOrgAccountSelectionFlow", () => { }); expect(onNext).toHaveBeenCalledTimes(1); }); + + it("resolves a still-pending task from the provider's persisted state once the wait is exhausted", async () => { + // Given the batch poll never settles the task before retries run out. + seedAppliedSelection(); + providersActionsMock.startProviderConnectionChecks.mockResolvedValue({ + [PROVIDER_ID]: { taskId: "task-1" }, + }); + providersActionsMock.getProviderUidsAndConnectionBaselines.mockResolvedValue( + { + uidById: { [PROVIDER_ID]: PROJECT_UID }, + baselineById: { [PROVIDER_ID]: "2025-01-01T00:00:00Z" }, + }, + ); + providerHelpersMock.resolveProviderConnectionState.mockResolvedValue({ + status: CONNECTION_CHECK_STATUS.SUCCESS, + error: null, + }); + pollConnectionTasksMock.mockImplementation( + async (taskIds: string[], { onSettled, resolveExhausted }) => { + for (const taskId of taskIds) { + const resolved = resolveExhausted + ? await resolveExhausted(taskId) + : null; + onSettled( + taskId, + resolved ?? { + status: CONNECTION_CHECK_STATUS.FAILED, + error: "Connection test timed out.", + }, + ); + } + }, + ); + const { onNext, startTesting } = renderFlow(); + + // When + await startTesting(); + + // Then: read from the provider's own record, not reported as a timeout, + // using the baseline captured for this provider before dispatch. + await waitFor(() => { + expect(useOrgSetupStore.getState().connectionResults[PROVIDER_ID]).toBe( + CONNECTION_TEST_STATUS.SUCCESS, + ); + }); + expect( + providersActionsMock.getProviderUidsAndConnectionBaselines, + ).toHaveBeenCalledWith([PROVIDER_ID]); + expect( + providerHelpersMock.resolveProviderConnectionState, + ).toHaveBeenCalledWith(PROVIDER_ID, "2025-01-01T00:00:00Z"); + expect(onNext).toHaveBeenCalledTimes(1); + }); + + it("does not report a still-running fallback as a connection failure", async () => { + // Given: the wait exhausts and the provider's own record cannot confirm + // an outcome either (the backend check is genuinely still running). + seedAppliedSelection(); + providersActionsMock.startProviderConnectionChecks.mockResolvedValue({ + [PROVIDER_ID]: { taskId: "task-1" }, + }); + providerHelpersMock.resolveProviderConnectionState.mockResolvedValue({ + status: CONNECTION_CHECK_STATUS.PENDING, + error: "The connection test is still running.", + }); + pollConnectionTasksMock.mockImplementation( + async (taskIds: string[], { onSettled, resolveExhausted }) => { + for (const taskId of taskIds) { + const resolved = resolveExhausted + ? await resolveExhausted(taskId) + : null; + onSettled( + taskId, + resolved ?? { + status: CONNECTION_CHECK_STATUS.FAILED, + error: "Connection test timed out.", + }, + ); + } + }, + ); + const { onNext, startTesting } = renderFlow(); + + // When + await startTesting(); + + // Then: neither a success (does not auto-advance) nor an error. + await waitFor(() => { + expect( + providerHelpersMock.resolveProviderConnectionState, + ).toHaveBeenCalled(); + }); + expect(useOrgSetupStore.getState().connectionResults[PROVIDER_ID]).toBe( + CONNECTION_TEST_STATUS.PENDING, + ); + expect(onNext).not.toHaveBeenCalled(); + }); + + it("keeps the retry control available when every unresolved account is pending, not failed", async () => { + // Given: no confirmed error, only a wait exhausted with no verdict -- + // `hasConnectionErrors` alone would hide "Test Connections" here. + seedAppliedSelection(); + providersActionsMock.startProviderConnectionChecks.mockResolvedValue({ + [PROVIDER_ID]: { taskId: "task-1" }, + }); + providerHelpersMock.resolveProviderConnectionState.mockResolvedValue({ + status: CONNECTION_CHECK_STATUS.PENDING, + error: "The connection test is still running.", + }); + pollConnectionTasksMock.mockImplementation( + async (taskIds: string[], { onSettled, resolveExhausted }) => { + for (const taskId of taskIds) { + const resolved = resolveExhausted + ? await resolveExhausted(taskId) + : null; + onSettled( + taskId, + resolved ?? { + status: CONNECTION_CHECK_STATUS.FAILED, + error: "Connection test timed out.", + }, + ); + } + }, + ); + const { startTesting, getFooterConfig } = renderFlow(); + + // When + await startTesting(); + + // Then: the action stays visible and enabled for a retry. + await waitFor(() => { + expect(useOrgSetupStore.getState().connectionResults[PROVIDER_ID]).toBe( + CONNECTION_TEST_STATUS.PENDING, + ); + }); + const footerConfig = getFooterConfig(); + expect(footerConfig?.showAction).toBe(true); + expect(footerConfig?.actionDisabled).toBe(false); + }); + + it("retries only the still-pending account, not one that already succeeded", async () => { + // Given: two accounts selected, one project and one folder-scoped project + // under the same GCP org so both resolve from a single apply. + const OTHER_UID = "projects/acme-staging"; + const hierarchyWithTwoProjects: GcpOrgHierarchy = { + ...GCP_HIERARCHY, + candidates: [ + ...GCP_HIERARCHY.candidates, + { uid: OTHER_UID, label: "Acme Staging", parentId: ORGANIZATION_UID }, + ], + }; + const OTHER_PROVIDER_ID = "provider-2"; + const store = useOrgSetupStore.getState(); + store.setOrganizationType(ORGANIZATION_TYPE.GCP); + store.setOrganization("org-1", "Acme", ORGANIZATION_UID); + store.setDiscovery("discovery-1", hierarchyWithTwoProjects); + store.setSelectedCandidateIds([PROJECT_UID, OTHER_UID]); + + organizationsActionsMock.applyDiscovery.mockResolvedValue({ + data: { + relationships: { + providers: { + data: [{ id: PROVIDER_ID }, { id: OTHER_PROVIDER_ID }], + }, + }, + }, + }); + providersActionsMock.getProviderUidsAndConnectionBaselines.mockResolvedValue( + { + uidById: { + [PROVIDER_ID]: PROJECT_UID, + [OTHER_PROVIDER_ID]: OTHER_UID, + }, + baselineById: {}, + }, + ); + providersActionsMock.startProviderConnectionChecks.mockResolvedValue({ + [PROVIDER_ID]: { taskId: "task-1" }, + [OTHER_PROVIDER_ID]: { taskId: "task-2" }, + }); + tasksActionsMock.getTasksByIds.mockResolvedValue({ + "task-1": { + data: { + attributes: { state: "completed", result: { connected: true } }, + }, + }, + "task-2": { data: { attributes: { state: "executing" } } }, + }); + providerHelpersMock.resolveProviderConnectionState.mockImplementation( + async (providerId: string) => + providerId === OTHER_PROVIDER_ID + ? { + status: CONNECTION_CHECK_STATUS.PENDING, + error: "The connection test is still running.", + } + : { status: CONNECTION_CHECK_STATUS.SUCCESS, error: null }, + ); + pollConnectionTasksMock.mockImplementation( + async (taskIds: string[], { onSettled, resolveExhausted }) => { + for (const taskId of taskIds) { + if (taskId === "task-1") { + onSettled(taskId, { status: CONNECTION_CHECK_STATUS.SUCCESS }); + continue; + } + const resolved = resolveExhausted + ? await resolveExhausted(taskId) + : null; + onSettled( + taskId, + resolved ?? { + status: CONNECTION_CHECK_STATUS.FAILED, + error: "Connection test timed out.", + }, + ); + } + }, + ); + const { startTesting } = renderFlow(); + + // First pass: one account succeeds, the other is left pending. + await startTesting(); + await waitFor(() => { + expect( + useOrgSetupStore.getState().connectionResults[OTHER_PROVIDER_ID], + ).toBe(CONNECTION_TEST_STATUS.PENDING); + }); + expect(useOrgSetupStore.getState().connectionResults[PROVIDER_ID]).toBe( + CONNECTION_TEST_STATUS.SUCCESS, + ); + providersActionsMock.startProviderConnectionChecks.mockClear(); + + // When: pressing "Test Connections" again to retry. + await startTesting(); + + // Then: only the still-pending account is re-dispatched. + await waitFor(() => { + expect( + providersActionsMock.startProviderConnectionChecks, + ).toHaveBeenCalled(); + }); + expect( + providersActionsMock.startProviderConnectionChecks, + ).toHaveBeenCalledWith([OTHER_PROVIDER_ID]); + }); }); }); diff --git a/ui/components/providers/organizations/hooks/use-org-account-selection-flow.ts b/ui/components/providers/organizations/hooks/use-org-account-selection-flow.ts index b5130760ba..65d4b4599c 100644 --- a/ui/components/providers/organizations/hooks/use-org-account-selection-flow.ts +++ b/ui/components/providers/organizations/hooks/use-org-account-selection-flow.ts @@ -5,7 +5,8 @@ import { useEffect, useRef, useState } from "react"; import { applyDiscovery } from "@/actions/organizations/organizations"; import { buildApplyPayload } from "@/actions/organizations/organizations.adapter"; import { - getProviderUidsByIds, + getProviderConnectionBaselines, + getProviderUidsAndConnectionBaselines, revalidateProviders, startProviderConnectionChecks, } from "@/actions/providers/providers"; @@ -13,12 +14,14 @@ import { WIZARD_FOOTER_ACTION_TYPE, WizardFooterConfig, } from "@/components/providers/wizard/steps/footer-controls"; +import { resolveProviderConnectionState } from "@/lib/provider-helpers"; import { useOrgSetupStore } from "@/store/organizations/store"; import { CONNECTION_TEST_STATUS, ConnectionTestStatus, PROVIDER_SECRET_STATE, } from "@/types/organizations"; +import { CONNECTION_CHECK_STATUS } from "@/types/providers"; import { TREE_ITEM_STATUS, TreeDataItem } from "@/types/tree"; import { @@ -26,6 +29,7 @@ import { canAdvanceToLaunchStep, getLaunchableProviderIds, pollConnectionTasks, + type PollConnectionTaskResult, } from "../org-account-selection.utils"; import { extractErrorMessage } from "./error-utils"; @@ -141,13 +145,22 @@ function buildTreeWithConnectionState( status = TREE_ITEM_STATUS.ERROR; errorMessage = (providerId && connectionErrors[providerId]) || "Connection failed."; - } else if ( - showPendingState || - connectionStatus === CONNECTION_TEST_STATUS.PENDING - ) { + } else if (showPendingState) { + // A batch test is actively in flight -- genuinely waiting on a response, + // so the spinner is accurate. isLoading = true; status = undefined; errorMessage = undefined; + } else if (connectionStatus === CONNECTION_TEST_STATUS.PENDING) { + // The wait was exhausted with no confirmed outcome, and nothing is + // polling this account any more -- a spinner here would be misleading. + // A static icon marks it as unresolved instead; "Test Connections" + // retries it (see `hasUnresolvedConnections`). + isLoading = false; + status = TREE_ITEM_STATUS.PENDING; + errorMessage = + (providerId && connectionErrors[providerId]) || + "The connection test is still running. Refresh in a moment to see the result."; } else if (hasAppliedProviders) { // Applied, but no outcome ever arrived for this account — typically an // unresolved provider uid. Without this the row falls back to a plain @@ -241,6 +254,13 @@ export function useOrgAccountSelectionFlow({ const hasConnectionErrors = Object.values(connectionResults).some( (status) => status === CONNECTION_TEST_STATUS.ERROR, ); + // A wait exhausted with no verdict, distinct from a confirmed error: it does + // not earn the error banner (see `org-account-selection.tsx`), but it still + // needs a way back to a resolved state, so it counts toward `canRetry` below. + const hasPendingConnections = Object.values(connectionResults).some( + (status) => status === CONNECTION_TEST_STATUS.PENDING, + ); + const hasUnresolvedConnections = hasConnectionErrors || hasPendingConnections; const willReplaceSelectedNames = sanitizedSelectedCandidateIds .map((id) => candidateLookup.get(id)) .filter( @@ -281,7 +301,10 @@ export function useOrgAccountSelectionFlow({ }; }, []); - const testAllConnections = async (providerIds: string[]) => { + const testAllConnections = async ( + providerIds: string[], + precomputedBaselines?: Record, + ) => { connectionTestAbortControllerRef.current?.abort(); const abortController = new AbortController(); connectionTestAbortControllerRef.current = abortController; @@ -296,26 +319,49 @@ export function useOrgAccountSelectionFlow({ const settleProvider = ( providerId: string, - result: { success: boolean; error?: string }, + result: PollConnectionTaskResult, ) => { if (!isMountedRef.current || signal.aborted) { return; } + + // Still running past the wait -- neither a pass nor a fail. Leaves the + // account pending rather than reporting a failure the backend never gave; + // the message is kept (not nulled) so the tree can explain the static + // pending icon it now shows once `isTesting` stops. + if (result.status === CONNECTION_CHECK_STATUS.PENDING) { + setConnectionResult(providerId, CONNECTION_TEST_STATUS.PENDING); + setConnectionError(providerId, result.error ?? null); + return; + } + + const succeeded = result.status === CONNECTION_CHECK_STATUS.SUCCESS; setConnectionResult( providerId, - result.success + succeeded ? CONNECTION_TEST_STATUS.SUCCESS : CONNECTION_TEST_STATUS.ERROR, ); setConnectionError( providerId, - result.success + succeeded ? null : result.error || "Connection failed for this account.", ); }; try { + // Read before dispatch, so the fallback below can tell each provider's own + // check result apart from whatever (possibly stale) result was already on + // record -- by comparing values, not by comparing timestamps against the + // browser's clock. See `resolveProviderConnectionState`. The initial apply + // already reads this alongside the created providers' uids (see + // `handleApplyAndTest`) and passes it in, so a retry is the only path that + // fetches it here. + const connectionBaselines = + precomputedBaselines ?? + (await getProviderConnectionBaselines(providerIds)); + // One action dispatches every check and one reads every pending task per // round: Next runs client-invoked server actions one at a time, so a loop // here would serialize the batch whatever concurrency it asked for. @@ -341,7 +387,7 @@ export function useOrgAccountSelectionFlow({ // No task id means no check ever ran, so it cannot count as passing. if (!outcome.taskId) { settleProvider(providerId, { - success: false, + status: CONNECTION_CHECK_STATUS.FAILED, error: "Connection test did not start.", }); continue; @@ -358,6 +404,17 @@ export function useOrgAccountSelectionFlow({ settleProvider(providerId, result); } }, + resolveExhausted: async (taskId) => { + const providerId = providerIdByTaskId.get(taskId); + if (!providerId) { + return null; + } + const state = await resolveProviderConnectionState( + providerId, + connectionBaselines[providerId], + ); + return { status: state.status, error: state.error ?? undefined }; + }, }); } catch { if (isMountedRef.current && !signal.aborted) { @@ -443,10 +500,21 @@ export function useOrgAccountSelectionFlow({ ) ?? []; setCreatedProviderIds(providerIds); + + // One filtered `/providers` read for both: the apply view rejects `include`, + // so the created providers' uids are read back separately, and the flow needs + // their connection baselines before dispatch anyway (see `testAllConnections`). + // Reading them together avoids fetching the same provider ids twice. + const { uidById, baselineById } = + await getProviderUidsAndConnectionBaselines(providerIds); + if (!isMountedRef.current) { + return; + } + const mapping = await buildCandidateToProviderMap({ selectedCandidateIds: currentSelectedCandidateIds, providerIds, - resolveProviderUids: getProviderUidsByIds, + resolveProviderUids: async () => uidById, }); if (!isMountedRef.current) { return; @@ -456,7 +524,7 @@ export function useOrgAccountSelectionFlow({ setIsApplying(false); lastAppliedSelectionKeyRef.current = currentSelectionKey; - await testAllConnections(providerIds); + await testAllConnections(providerIds, baselineById); }; const handleStartTesting = () => { @@ -482,12 +550,18 @@ export function useOrgAccountSelectionFlow({ return; } - const failedProviderIds = createdProviderIds.filter( + // Retries both confirmed failures and accounts a previous wait exhausted + // without a verdict -- otherwise a still-pending account has no way back to + // a resolved state once the batch that produced it has stopped polling. + const unresolvedProviderIds = createdProviderIds.filter( (providerId) => - connectionResults[providerId] === CONNECTION_TEST_STATUS.ERROR, + connectionResults[providerId] === CONNECTION_TEST_STATUS.ERROR || + connectionResults[providerId] === CONNECTION_TEST_STATUS.PENDING, ); const providerIdsToTest = - failedProviderIds.length > 0 ? failedProviderIds : createdProviderIds; + unresolvedProviderIds.length > 0 + ? unresolvedProviderIds + : createdProviderIds; void testAllConnections(providerIdsToTest); }; startTestingActionRef.current = handleStartTesting; @@ -513,7 +587,7 @@ export function useOrgAccountSelectionFlow({ return; } - const canRetry = hasConnectionErrors || Boolean(applyError); + const canRetry = hasUnresolvedConnections || Boolean(applyError); const hasSelectedAccounts = selectedCount > 0; onFooterChange({ @@ -542,7 +616,7 @@ export function useOrgAccountSelectionFlow({ }); }, [ applyError, - hasConnectionErrors, + hasUnresolvedConnections, isApplying, isTesting, isTestingView, diff --git a/ui/components/providers/organizations/org-account-selection.utils.test.ts b/ui/components/providers/organizations/org-account-selection.utils.test.ts index 366ec715b9..11c01e69bb 100644 --- a/ui/components/providers/organizations/org-account-selection.utils.test.ts +++ b/ui/components/providers/organizations/org-account-selection.utils.test.ts @@ -1,10 +1,13 @@ import { describe, expect, it, vi } from "vitest"; import { CONNECTION_TEST_STATUS } from "@/types/organizations"; +import { CONNECTION_CHECK_STATUS } from "@/types/providers"; import { buildCandidateToProviderMap, canAdvanceToLaunchStep, + CONNECTION_CHECK_DEFAULT_DELAYS_MS, + CONNECTION_CHECK_MAX_RETRIES, getLaunchableProviderIds, pollConnectionTasks, } from "./org-account-selection.utils"; @@ -89,8 +92,14 @@ describe("pollConnectionTasks", () => { // Then — the fast one is reported after round 1 and dropped from later reads, // while the slow one is still pending. expect(settled).toEqual([ - ["task-fast", { success: true }], - ["task-slow", { success: false, error: "Role trust policy mismatch." }], + ["task-fast", { status: CONNECTION_CHECK_STATUS.SUCCESS }], + [ + "task-slow", + { + status: CONNECTION_CHECK_STATUS.FAILED, + error: "Role trust policy mismatch.", + }, + ], ]); expect(rounds).toEqual([ ["task-fast", "task-slow"], @@ -150,8 +159,99 @@ describe("pollConnectionTasks", () => { // Then — the settled result stands; the pending one is reported cancelled. expect(getTasksByIds).toHaveBeenCalledTimes(1); expect(settled).toEqual([ - ["task-a", { success: true }], - ["task-b", { success: false, error: "Connection test cancelled." }], + ["task-a", { status: CONNECTION_CHECK_STATUS.SUCCESS }], + [ + "task-b", + { + status: CONNECTION_CHECK_STATUS.FAILED, + error: "Connection test cancelled.", + }, + ], + ]); + }); + + it("reports cancellation instead of accepting the resolver's result when abort lands mid-await", async () => { + // Given: the wait exhausts with one task still pending, and the caller's + // `resolveExhausted` aborts the flow while its own lookup is in flight + // (e.g. the wizard unmounted). The abort must win even though the + // resolver still returns a result. + const abortController = new AbortController(); + const getTasksByIds = vi.fn(async () => ({ + "task-a": executing, + })); + const settled: Array<[string, unknown]> = []; + const resolveExhausted = vi.fn(async () => { + abortController.abort(); + return { status: CONNECTION_CHECK_STATUS.SUCCESS }; + }); + + // When + await pollConnectionTasks(["task-a"], { + onSettled: (taskId, result) => settled.push([taskId, result]), + getTasksByIds, + sleep: async () => {}, + maxRetries: 1, + signal: abortController.signal, + resolveExhausted, + }); + + // Then: cancelled, not the resolver's (stale) success. + expect(resolveExhausted).toHaveBeenCalledWith("task-a"); + expect(settled).toEqual([ + [ + "task-a", + { + status: CONNECTION_CHECK_STATUS.FAILED, + error: "Connection test cancelled.", + }, + ], + ]); + }); + + it("stops resolving further tasks once abort lands between resolveExhausted calls", async () => { + // Given: two tasks are still pending at exhaustion; abort fires while the + // first is being resolved, so the second must never be looked up. + const abortController = new AbortController(); + const getTasksByIds = vi.fn(async () => ({ + "task-a": executing, + "task-b": executing, + })); + const settled: Array<[string, unknown]> = []; + const resolveExhausted = vi.fn(async (taskId: string) => { + if (taskId === "task-a") { + abortController.abort(); + } + return { status: CONNECTION_CHECK_STATUS.SUCCESS }; + }); + + // When + await pollConnectionTasks(["task-a", "task-b"], { + onSettled: (taskId, result) => settled.push([taskId, result]), + getTasksByIds, + sleep: async () => {}, + maxRetries: 1, + signal: abortController.signal, + resolveExhausted, + }); + + // Then + expect(resolveExhausted).toHaveBeenCalledTimes(1); + expect(resolveExhausted).toHaveBeenCalledWith("task-a"); + expect(settled).toEqual([ + [ + "task-a", + { + status: CONNECTION_CHECK_STATUS.FAILED, + error: "Connection test cancelled.", + }, + ], + [ + "task-b", + { + status: CONNECTION_CHECK_STATUS.FAILED, + error: "Connection test cancelled.", + }, + ], ]); }); @@ -173,8 +273,124 @@ describe("pollConnectionTasks", () => { // Then expect(settled).toEqual([ - ["task-a", { success: true }], - ["task-b", { success: false, error: "Connection test timed out." }], + ["task-a", { status: CONNECTION_CHECK_STATUS.SUCCESS }], + [ + "task-b", + { + status: CONNECTION_CHECK_STATUS.FAILED, + error: "Connection test timed out.", + }, + ], + ]); + }); + + it("sizes the default wait past the backend's 120s provider-connection-check time limit", () => { + // The last delay in the ladder repeats for every retry beyond it, so the + // worst-case total wait is (maxRetries - 1) * lastDelay. + const lastDelay = + CONNECTION_CHECK_DEFAULT_DELAYS_MS[ + CONNECTION_CHECK_DEFAULT_DELAYS_MS.length - 1 + ]; + const worstCaseWaitMs = (CONNECTION_CHECK_MAX_RETRIES - 1) * lastDelay; + + expect(worstCaseWaitMs).toBeGreaterThan(120_000); + }); + + it("resolves a still-pending task from the caller once the wait is exhausted", async () => { + // Given: the batch read never settles "task-b" before retries run out. + const getTasksByIds = vi.fn(async () => ({ + "task-a": completed(true), + "task-b": executing, + })); + const settled: Array<[string, unknown]> = []; + const resolveExhausted = vi.fn(async (taskId: string) => + taskId === "task-b" ? { status: CONNECTION_CHECK_STATUS.SUCCESS } : null, + ); + + // When + await pollConnectionTasks(["task-a", "task-b"], { + onSettled: (taskId, result) => settled.push([taskId, result]), + getTasksByIds, + sleep: async () => {}, + maxRetries: 2, + resolveExhausted, + }); + + // Then: the exhausted task is settled from the fallback, not a timeout. + expect(resolveExhausted).toHaveBeenCalledWith("task-b"); + expect(settled).toEqual([ + ["task-a", { status: CONNECTION_CHECK_STATUS.SUCCESS }], + ["task-b", { status: CONNECTION_CHECK_STATUS.SUCCESS }], + ]); + }); + + it("reports a still-running fallback as pending, not as a failure", async () => { + // Given: the batch read never settles "task-b", and the caller's fallback + // cannot confirm an outcome either (the backend task is still running). + const getTasksByIds = vi.fn(async () => ({ + "task-a": completed(true), + "task-b": executing, + })); + const settled: Array<[string, unknown]> = []; + const resolveExhausted = vi.fn(async (taskId: string) => + taskId === "task-b" + ? { + status: CONNECTION_CHECK_STATUS.PENDING, + error: "The connection test is still running.", + } + : null, + ); + + // When + await pollConnectionTasks(["task-a", "task-b"], { + onSettled: (taskId, result) => settled.push([taskId, result]), + getTasksByIds, + sleep: async () => {}, + maxRetries: 2, + resolveExhausted, + }); + + // Then: pending, distinct from both success and failure. + expect(settled).toEqual([ + ["task-a", { status: CONNECTION_CHECK_STATUS.SUCCESS }], + [ + "task-b", + { + status: CONNECTION_CHECK_STATUS.PENDING, + error: "The connection test is still running.", + }, + ], + ]); + }); + + it("falls back to the timeout message when the fallback cannot resolve a task", async () => { + // Given + const getTasksByIds = vi.fn(async () => ({ + "task-a": completed(true), + "task-b": executing, + })); + const settled: Array<[string, unknown]> = []; + const resolveExhausted = vi.fn(async () => null); + + // When + await pollConnectionTasks(["task-a", "task-b"], { + onSettled: (taskId, result) => settled.push([taskId, result]), + getTasksByIds, + sleep: async () => {}, + maxRetries: 2, + resolveExhausted, + }); + + // Then + expect(settled).toEqual([ + ["task-a", { status: CONNECTION_CHECK_STATUS.SUCCESS }], + [ + "task-b", + { + status: CONNECTION_CHECK_STATUS.FAILED, + error: "Connection test timed out.", + }, + ], ]); }); @@ -197,8 +413,11 @@ describe("pollConnectionTasks", () => { // Then expect(getTasksByIds).toHaveBeenCalledTimes(1); expect(settled).toEqual([ - ["task-a", { success: true }], - ["task-b", { success: false, error: "Task not found." }], + ["task-a", { status: CONNECTION_CHECK_STATUS.SUCCESS }], + [ + "task-b", + { status: CONNECTION_CHECK_STATUS.FAILED, error: "Task not found." }, + ], ]); }); }); diff --git a/ui/components/providers/organizations/org-account-selection.utils.ts b/ui/components/providers/organizations/org-account-selection.utils.ts index 59d74b90d0..23929dde94 100644 --- a/ui/components/providers/organizations/org-account-selection.utils.ts +++ b/ui/components/providers/organizations/org-account-selection.utils.ts @@ -2,8 +2,21 @@ import { CONNECTION_TEST_STATUS, ConnectionTestStatus, } from "@/types/organizations"; +import { + CONNECTION_CHECK_STATUS, + type ConnectionCheckStatus, +} from "@/types/providers"; const DEFAULT_POLL_DELAYS_MS = [2000, 3000, 5000] as const; +export const CONNECTION_CHECK_DEFAULT_DELAYS_MS = DEFAULT_POLL_DELAYS_MS; + +/** + * `provider-connection-check` has a 120s hard time limit in Celery + * (api/src/backend/config/celery.py `task_annotations`). With the delay ladder + * above -- 2s, 3s, then 5s repeating -- 32 retries cover roughly 155s, + * comfortably past the task's hard limit plus queueing/network slack. + */ +export const CONNECTION_CHECK_MAX_RETRIES = 32; interface BuildCandidateToProviderMapParams { selectedCandidateIds: string[]; @@ -27,10 +40,20 @@ interface PollConnectionTasksOptions /** Called once per task, the round it reaches a terminal state. */ onSettled: (taskId: string, result: PollConnectionTaskResult) => void; getTasksByIds?: (taskIds: string[]) => Promise>; + /** + * Called once per task still pending after `maxRetries` is exhausted, so the + * caller can re-read the provider's persisted connection state instead of + * reporting a flat timeout -- the backend task may still be running past the + * wait, or may have already finished with the UI no longer polling it. + * Returning `null` falls back to the timeout message. + */ + resolveExhausted?: ( + taskId: string, + ) => Promise; } export interface PollConnectionTaskResult { - success: boolean; + status: ConnectionCheckStatus; error?: string; } @@ -123,7 +146,10 @@ function readConnectionOutcome( taskResponse: unknown, ): PollConnectionTaskResult | null { if (isRecord(taskResponse) && typeof taskResponse.error === "string") { - return { success: false, error: taskResponse.error }; + return { + status: CONNECTION_CHECK_STATUS.FAILED, + error: taskResponse.error, + }; } const data = @@ -138,10 +164,10 @@ function readConnectionOutcome( const connected = typeof result?.connected === "boolean" ? result.connected : true; if (connected) { - return { success: true }; + return { status: CONNECTION_CHECK_STATUS.SUCCESS }; } return { - success: false, + status: CONNECTION_CHECK_STATUS.FAILED, error: (typeof result?.error === "string" && result.error) || "Connection failed for this account.", @@ -150,7 +176,7 @@ function readConnectionOutcome( if (state === "failed") { return { - success: false, + status: CONNECTION_CHECK_STATUS.FAILED, error: (typeof result?.error === "string" && result.error) || "Connection test task failed.", @@ -158,7 +184,10 @@ function readConnectionOutcome( } if (!state || !IN_PROGRESS_TASK_STATES.has(state)) { - return { success: false, error: "Unexpected task state." }; + return { + status: CONNECTION_CHECK_STATUS.FAILED, + error: "Unexpected task state.", + }; } return null; @@ -180,9 +209,10 @@ export async function pollConnectionTasks( getTasksByIds, sleep = async (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)), - maxRetries = 20, + maxRetries = CONNECTION_CHECK_MAX_RETRIES, delaysMs = [...DEFAULT_POLL_DELAYS_MS], signal, + resolveExhausted, }: PollConnectionTasksOptions, ): Promise { const pending = new Set(taskIds.filter(Boolean)); @@ -199,7 +229,7 @@ export async function pollConnectionTasks( const settleRemaining = (error: string) => { for (const taskId of Array.from(pending)) { - onSettled(taskId, { success: false, error }); + onSettled(taskId, { status: CONNECTION_CHECK_STATUS.FAILED, error }); } pending.clear(); }; @@ -239,13 +269,46 @@ export async function pollConnectionTasks( await sleepWithAbort(getPollingDelay(attempt, delaysMs), sleep, signal); } + if (resolveExhausted) { + // Sequential, not `Promise.all`: each call goes through its own + // `getProvider` server action, and client-invoked server actions run one + // at a time through Next's action queue (see `pollConnectionTasks`'s own + // batched read above) -- running them "concurrently" from here would not + // shorten the wait, only reorder it. + for (const taskId of Array.from(pending)) { + if (signal?.aborted) { + settleRemaining("Connection test cancelled."); + return; + } + + const resolved = await resolveExhausted(taskId); + + // The signal can abort while `resolveExhausted` itself is in flight; its + // result must not be accepted after that, or a check the caller has + // already moved on from could still report success. + if (signal?.aborted) { + settleRemaining("Connection test cancelled."); + return; + } + + if (resolved) { + pending.delete(taskId); + onSettled(taskId, resolved); + } + } + } + settleRemaining("Connection test timed out."); } /** * Polls a generic async task until it settles. Unlike {@link pollConnectionTasks} * it does not interpret a connection result; it is used for organization/node - * deletion, which the API answers with a `202` + task. + * deletion, which the API answers with a `202` + task. Its result is typed with + * `ConnectionCheckStatus` only because that is the connection-specific alias of + * the generic `TASK_OUTCOME` (`types/tasks.ts`) already in scope here -- the + * three outcomes (succeeded / failed / still running) apply to any polled task, + * not just a connection check. */ export async function pollTaskCompletion( taskId: string, @@ -267,16 +330,25 @@ export async function pollTaskCompletion( for (let attempt = 0; attempt < maxRetries; attempt += 1) { if (signal?.aborted) { - return { success: false, error: "Deletion cancelled." }; + return { + status: CONNECTION_CHECK_STATUS.FAILED, + error: "Deletion cancelled.", + }; } const taskResponse = await taskFetcher(taskId); if (signal?.aborted) { - return { success: false, error: "Deletion cancelled." }; + return { + status: CONNECTION_CHECK_STATUS.FAILED, + error: "Deletion cancelled.", + }; } if (isRecord(taskResponse) && typeof taskResponse.error === "string") { - return { success: false, error: taskResponse.error }; + return { + status: CONNECTION_CHECK_STATUS.FAILED, + error: taskResponse.error, + }; } const data = @@ -289,12 +361,12 @@ export async function pollTaskCompletion( const result = isRecord(attributes?.result) ? attributes.result : null; if (state === "completed") { - return { success: true }; + return { status: CONNECTION_CHECK_STATUS.SUCCESS }; } if (state === "failed") { return { - success: false, + status: CONNECTION_CHECK_STATUS.FAILED, error: (typeof result?.error === "string" && result.error) || "The deletion task failed.", @@ -303,17 +375,26 @@ export async function pollTaskCompletion( // A cancelled task is a real terminal state, not an unreadable one. if (state === "cancelled") { - return { success: false, error: "The deletion was cancelled." }; + return { + status: CONNECTION_CHECK_STATUS.FAILED, + error: "The deletion was cancelled.", + }; } if (!state || !IN_PROGRESS_TASK_STATES.has(state)) { - return { success: false, error: "Unexpected task state." }; + return { + status: CONNECTION_CHECK_STATUS.FAILED, + error: "Unexpected task state.", + }; } await sleepWithAbort(getPollingDelay(attempt, delaysMs), sleep, signal); } - return { success: false, error: "Deletion timed out." }; + return { + status: CONNECTION_CHECK_STATUS.FAILED, + error: "Deletion timed out.", + }; } export function getLaunchableProviderIds( diff --git a/ui/components/providers/table/data-table-row-actions.test.tsx b/ui/components/providers/table/data-table-row-actions.test.tsx index c65863abd2..ae0fe7c244 100644 --- a/ui/components/providers/table/data-table-row-actions.test.tsx +++ b/ui/components/providers/table/data-table-row-actions.test.tsx @@ -24,6 +24,7 @@ import { ORGANIZATION_TYPE, type OrganizationType, } from "@/types/organizations"; +import { CONNECTION_CHECK_STATUS } from "@/types/providers"; import { PROVIDERS_GROUP_KIND, PROVIDERS_ROW_TYPE, @@ -35,18 +36,36 @@ import { SCAN_SCHEDULE_CAPABILITY } from "@/types/schedules"; const { checkConnectionProviderMock, + getProviderConnectionBaselinesMock, getScheduleMock, getTasksByIdsMock, + pollConnectionTasksMock, pushMock, + realPollConnectionTasksHolder, + resolveProviderConnectionStateMock, revalidateProvidersMock, startProviderConnectionChecksMock, + testProviderConnectionMock, + toastMock, } = vi.hoisted(() => ({ checkConnectionProviderMock: vi.fn(), + getProviderConnectionBaselinesMock: vi.fn(), getScheduleMock: vi.fn(), getTasksByIdsMock: vi.fn(), + pollConnectionTasksMock: vi.fn(), pushMock: vi.fn(), + // Mutable holder for the real `pollConnectionTasks`, captured once the + // module mock factory below runs, and read fresh in `beforeEach` since + // `mockReset: true` clears `pollConnectionTasksMock`'s implementation + // before every test. + realPollConnectionTasksHolder: {} as { + current?: (...args: unknown[]) => unknown; + }, + resolveProviderConnectionStateMock: vi.fn(), revalidateProvidersMock: vi.fn(), startProviderConnectionChecksMock: vi.fn(), + testProviderConnectionMock: vi.fn(), + toastMock: vi.fn(), })); vi.mock("next/navigation", () => ({ @@ -59,6 +78,7 @@ vi.mock("@/actions/organizations/organizations", () => ({ vi.mock("@/actions/providers/providers", () => ({ checkConnectionProvider: checkConnectionProviderMock, + getProviderConnectionBaselines: getProviderConnectionBaselinesMock, revalidateProviders: revalidateProvidersMock, startProviderConnectionChecks: startProviderConnectionChecksMock, })); @@ -124,13 +144,28 @@ vi.mock("@/components/scans/schedule/edit-scan-schedule-modal", () => ({ vi.mock("@/components/shadcn", async (importOriginal) => ({ ...(await importOriginal>()), - useToast: () => ({ toast: vi.fn() }), + useToast: () => ({ toast: toastMock }), })); vi.mock("@/lib/provider-helpers", () => ({ - testProviderConnection: vi.fn(), + resolveProviderConnectionState: resolveProviderConnectionStateMock, + testProviderConnection: testProviderConnectionMock, })); +vi.mock( + "@/components/providers/organizations/org-account-selection.utils", + async (importOriginal) => { + const actual = + await importOriginal< + typeof import("@/components/providers/organizations/org-account-selection.utils") + >(); + realPollConnectionTasksHolder.current = actual.pollConnectionTasks as ( + ...args: unknown[] + ) => unknown; + return { ...actual, pollConnectionTasks: pollConnectionTasksMock }; + }, +); + import { DataTableRowActions } from "./data-table-row-actions"; const createRow = (hasSecret = false) => @@ -301,6 +336,12 @@ describe("DataTableRowActions", () => { }); beforeEach(() => { + // `mockReset: true` (vitest.config.ts) clears this before every test, so + // the real implementation is the default and tests only override it when + // they need to simulate an exhausted poll. + pollConnectionTasksMock.mockImplementation((...args: unknown[]) => + realPollConnectionTasksHolder.current?.(...args), + ); getScheduleMock.mockResolvedValue({ data: { type: "schedules", @@ -311,6 +352,7 @@ describe("DataTableRowActions", () => { }, }, }); + getProviderConnectionBaselinesMock.mockResolvedValue({}); }); it("renders Add Credentials for provider rows without credentials", async () => { @@ -780,6 +822,173 @@ describe("DataTableRowActions", () => { expect(checkConnectionProviderMock).not.toHaveBeenCalled(); }); + it("falls back to the provider's persisted state for a task still pending once the bulk wait is exhausted", async () => { + // Given: the batch poll exhausts its retries for both tasks; the component + // must re-read each provider's connection state instead of reporting a + // flat timeout. + const user = userEvent.setup(); + const testableProviderIds = ["provider-child-1", "provider-standalone"]; + startProviderConnectionChecksMock.mockResolvedValue({ + "provider-child-1": { taskId: "task-1" }, + "provider-standalone": { taskId: "task-2" }, + }); + // The baseline read before dispatch: one provider has a prior stored check, + // the other has never been checked. + getProviderConnectionBaselinesMock.mockResolvedValue({ + "provider-child-1": "2025-01-01T00:00:00Z", + "provider-standalone": null, + }); + pollConnectionTasksMock.mockImplementation( + async (taskIds: string[], { onSettled, resolveExhausted }) => { + for (const taskId of taskIds) { + const resolved = resolveExhausted + ? await resolveExhausted(taskId) + : null; + onSettled( + taskId, + resolved ?? { + status: CONNECTION_CHECK_STATUS.FAILED, + error: "Connection test timed out.", + }, + ); + } + }, + ); + resolveProviderConnectionStateMock.mockImplementation( + async (providerId: string) => + providerId === "provider-standalone" + ? { status: CONNECTION_CHECK_STATUS.SUCCESS, error: null } + : { + status: CONNECTION_CHECK_STATUS.FAILED, + error: "Connection was not confirmed. Test the connection again.", + }, + ); + + render( + , + ); + + // When + await user.click(screen.getByRole("button")); + await user.click(screen.getByText("Test Connections (2)")); + + // Then: each pending task is resolved from the provider's own record, using + // the baseline captured for that specific provider before dispatch. + await vi.waitFor(() => + expect(revalidateProvidersMock).toHaveBeenCalledTimes(1), + ); + expect(getProviderConnectionBaselinesMock).toHaveBeenCalledWith( + testableProviderIds, + ); + expect(resolveProviderConnectionStateMock).toHaveBeenCalledWith( + "provider-child-1", + "2025-01-01T00:00:00Z", + ); + expect(resolveProviderConnectionStateMock).toHaveBeenCalledWith( + "provider-standalone", + null, + ); + }); + + it("shows a neutral toast, not a failure, when a single test is still running past the wait", async () => { + // Given: the exhausted single-provider test cannot confirm an outcome yet. + const user = userEvent.setup(); + testProviderConnectionMock.mockResolvedValue({ + status: CONNECTION_CHECK_STATUS.PENDING, + error: + "The connection test is still running. Refresh in a moment to see the result.", + }); + + render( + , + ); + + // When + await user.click(screen.getByRole("button")); + await user.click(screen.getByText("Test Connection")); + + // Then: no destructive toast for a check that is merely still running. + await vi.waitFor(() => expect(toastMock).toHaveBeenCalledTimes(1)); + expect(toastMock).toHaveBeenCalledWith( + expect.not.objectContaining({ variant: "destructive" }), + ); + expect(toastMock.mock.calls[0][0].title).not.toMatch(/failed/i); + }); + + it("does not count a still-running bulk result as failed", async () => { + // Given: one provider settles successfully, the other is still running + // once the bulk wait is exhausted. + const user = userEvent.setup(); + const testableProviderIds = ["provider-child-1", "provider-standalone"]; + startProviderConnectionChecksMock.mockResolvedValue({ + "provider-child-1": { taskId: "task-1" }, + "provider-standalone": { taskId: "task-2" }, + }); + pollConnectionTasksMock.mockImplementation( + async (taskIds: string[], { onSettled, resolveExhausted }) => { + for (const taskId of taskIds) { + const resolved = resolveExhausted + ? await resolveExhausted(taskId) + : null; + onSettled( + taskId, + resolved ?? { + status: CONNECTION_CHECK_STATUS.FAILED, + error: "Connection test timed out.", + }, + ); + } + }, + ); + resolveProviderConnectionStateMock.mockImplementation( + async (providerId: string) => + providerId === "provider-standalone" + ? { status: CONNECTION_CHECK_STATUS.SUCCESS, error: null } + : { + status: CONNECTION_CHECK_STATUS.PENDING, + error: "The connection test is still running.", + }, + ); + + render( + , + ); + + // When + await user.click(screen.getByRole("button")); + await user.click(screen.getByText("Test Connections (2)")); + + // Then: not styled as a failure — no destructive toast. + await vi.waitFor(() => expect(toastMock).toHaveBeenCalledTimes(1)); + expect(toastMock).toHaveBeenCalledWith( + expect.not.objectContaining({ variant: "destructive" }), + ); + }); + it("shows selected provider count in Test Connections when OU row has active selection", async () => { const user = userEvent.setup(); render( diff --git a/ui/components/providers/table/data-table-row-actions.tsx b/ui/components/providers/table/data-table-row-actions.tsx index bcbbd8e583..95692ced80 100644 --- a/ui/components/providers/table/data-table-row-actions.tsx +++ b/ui/components/providers/table/data-table-row-actions.tsx @@ -16,6 +16,7 @@ import { useState } from "react"; import { updateOrganizationName } from "@/actions/organizations/organizations"; import { updateProvider } from "@/actions/providers"; import { + getProviderConnectionBaselines, revalidateProviders, startProviderConnectionChecks, } from "@/actions/providers/providers"; @@ -42,7 +43,10 @@ import { getNodeLabel, organizationNameFallbackHint, } from "@/lib/organizations"; -import { testProviderConnection } from "@/lib/provider-helpers"; +import { + resolveProviderConnectionState, + testProviderConnection, +} from "@/lib/provider-helpers"; import { getScanScheduleCapability } from "@/lib/schedules"; import { isCloud } from "@/lib/shared/env"; import { @@ -52,6 +56,7 @@ import { OrgFlowType, } from "@/types/organizations"; import { PROVIDER_WIZARD_MODE } from "@/types/provider-wizard"; +import { CONNECTION_CHECK_STATUS } from "@/types/providers"; import { isProvidersOrganizationRow, PROVIDERS_GROUP_KIND, @@ -394,9 +399,15 @@ export function DataTableRowActions({ // asks for. let succeeded = 0; let failed = 0; - const pendingTaskIds: string[] = []; + let pending = 0; + const providerIdByTaskId = new Map(); try { + // Read before dispatch, so the fallback below can tell each provider's own + // check result apart from whatever (possibly stale) result was already on + // record -- by comparing values, not by comparing timestamps against the + // browser's clock. See `resolveProviderConnectionState`. + const connectionBaselines = await getProviderConnectionBaselines(ids); const outcomes = await startProviderConnectionChecks(ids); for (const id of ids) { @@ -408,34 +419,54 @@ export function DataTableRowActions({ continue; } - pendingTaskIds.push(outcome.taskId); + providerIdByTaskId.set(outcome.taskId, id); } - await pollConnectionTasks(pendingTaskIds, { + await pollConnectionTasks(Array.from(providerIdByTaskId.keys()), { onSettled: (_taskId, result) => { - if (result.success) { + if (result.status === CONNECTION_CHECK_STATUS.SUCCESS) { succeeded += 1; + } else if (result.status === CONNECTION_CHECK_STATUS.PENDING) { + pending += 1; } else { failed += 1; } }, + resolveExhausted: async (taskId) => { + const id = providerIdByTaskId.get(taskId); + if (!id) { + return null; + } + const state = await resolveProviderConnectionState( + id, + connectionBaselines[id], + ); + return { status: state.status, error: state.error ?? undefined }; + }, }); } catch { - failed = ids.length - succeeded; + failed = ids.length - succeeded - pending; } await revalidateProviders(); - if (failed === 0) { + if (failed === 0 && pending === 0) { toast({ title: "Connection test completed", description: `${succeeded} ${succeeded === 1 ? "provider" : "providers"} tested successfully.`, }); + } else if (failed === 0) { + toast({ + title: "Connection test still running", + description: `${succeeded} succeeded, ${pending} still running. Refresh in a moment to see the rest.`, + }); } else { toast({ variant: "destructive", title: "Connection test completed", - description: `${succeeded} succeeded, ${failed} failed out of ${ids.length} providers.`, + description: `${succeeded} succeeded, ${failed} failed${ + pending ? `, ${pending} still running` : "" + } out of ${ids.length} providers.`, }); } @@ -454,17 +485,22 @@ export function DataTableRowActions({ const result = await testProviderConnection(providerId); setLoading(false); - if (!result.connected) { + if (result.status === CONNECTION_CHECK_STATUS.SUCCESS) { + toast({ + title: "Connection test completed", + description: "Provider tested successfully.", + }); + } else if (result.status === CONNECTION_CHECK_STATUS.PENDING) { + toast({ + title: "Connection test still running", + description: result.error ?? "Refresh in a moment to see the result.", + }); + } else { toast({ variant: "destructive", title: "Connection test failed", description: result.error ?? "Unknown error", }); - } else { - toast({ - title: "Connection test completed", - description: "Provider tested successfully.", - }); } } }; diff --git a/ui/components/providers/wizard/provider-wizard-modal.test.tsx b/ui/components/providers/wizard/provider-wizard-modal.test.tsx index 8ed98ce18d..5a1115dbd9 100644 --- a/ui/components/providers/wizard/provider-wizard-modal.test.tsx +++ b/ui/components/providers/wizard/provider-wizard-modal.test.tsx @@ -11,6 +11,7 @@ import { import { endActiveTour } from "@/lib/tours/use-driver-tour"; import { useProviderWizardStore } from "@/store/provider-wizard/store"; import { useUIStore } from "@/store/ui/store"; +import { CONNECTION_CHECK_STATUS } from "@/types/providers"; import { ProviderWizardModal } from "./provider-wizard-modal"; @@ -154,7 +155,10 @@ describe("provider wizard account creation", () => { status: "ready", options: [{ type: "acme", label: "Acme Cloud" }], }); - testProviderConnection.mockResolvedValue({ connected: true, error: null }); + testProviderConnection.mockResolvedValue({ + status: CONNECTION_CHECK_STATUS.SUCCESS, + error: null, + }); updateCredentialsProvider.mockResolvedValue({ data: { id: "secret-1" } }); updateProvider.mockResolvedValue({ data: { id: "provider-1" } }); }); @@ -351,7 +355,7 @@ describe("provider wizard account creation", () => { addProvider.mockResolvedValue({ data: { id: "provider-1" } }); addCredentialsProvider.mockResolvedValue({ data: { id: "secret-1" } }); testProviderConnection.mockResolvedValue({ - connected: false, + status: CONNECTION_CHECK_STATUS.FAILED, error: "The role could not be assumed.", }); const user = await pickAws(); diff --git a/ui/components/providers/wizard/steps/aws/aws-connect-step.test.tsx b/ui/components/providers/wizard/steps/aws/aws-connect-step.test.tsx index bf7a105f37..73cf42fea8 100644 --- a/ui/components/providers/wizard/steps/aws/aws-connect-step.test.tsx +++ b/ui/components/providers/wizard/steps/aws/aws-connect-step.test.tsx @@ -8,6 +8,10 @@ import { type ProviderFunnelDetail, } from "@/lib/provider-funnel/provider-funnel-events"; import { useProviderWizardStore } from "@/store/provider-wizard/store"; +import { + CONNECTION_CHECK_STATUS, + type ConnectionCheckStatus, +} from "@/types/providers"; import { AwsConnectStep } from "./aws-connect-step"; import type { AwsConnectUiState } from "./types"; @@ -117,7 +121,10 @@ describe("AwsConnectStep", () => { addCredentialsProvider.mockResolvedValue({ data: { id: "secret-1" } }); updateProvider.mockResolvedValue({ data: { id: "provider-1" } }); updateCredentialsProvider.mockResolvedValue({ data: { id: "secret-1" } }); - testProviderConnection.mockResolvedValue({ connected: true, error: null }); + testProviderConnection.mockResolvedValue({ + status: CONNECTION_CHECK_STATUS.SUCCESS, + error: null, + }); }); afterEach(() => { @@ -355,7 +362,7 @@ describe("AwsConnectStep", () => { it("reports the test in progress and blocks the action while it runs", async () => { // Given: a test that has not answered yet. let settle!: (result: { - connected: boolean; + status: ConnectionCheckStatus; error: string | null; }) => void; testProviderConnection.mockImplementation( @@ -378,14 +385,16 @@ describe("AwsConnectStep", () => { expect(onConnected).not.toHaveBeenCalled(); // When / Then - await act(async () => settle({ connected: true, error: null })); + await act(async () => + settle({ status: CONNECTION_CHECK_STATUS.SUCCESS, error: null }), + ); await waitFor(() => expect(onConnected).toHaveBeenCalledOnce()); }); it("ignores a result that lands after the step was closed", async () => { // Given: the wizard is closed (or switched to organizations) mid-test. let settle!: (result: { - connected: boolean; + status: ConnectionCheckStatus; error: string | null; }) => void; testProviderConnection.mockImplementation( @@ -399,7 +408,9 @@ describe("AwsConnectStep", () => { // When unmount(); - await act(async () => settle({ connected: true, error: null })); + await act(async () => + settle({ status: CONNECTION_CHECK_STATUS.SUCCESS, error: null }), + ); // Then: a reset wizard must not be pushed to the launch step. expect(onConnected).not.toHaveBeenCalled(); @@ -417,7 +428,7 @@ describe("AwsConnectStep", () => { it("stays on the keys form when the connection is refused", async () => { // Given testProviderConnection.mockResolvedValue({ - connected: false, + status: CONNECTION_CHECK_STATUS.FAILED, error: "The access keys were rejected.", }); @@ -444,7 +455,7 @@ describe("AwsConnectStep", () => { it("stays on the form and offers a retry when the connection is refused", async () => { // Given testProviderConnection.mockResolvedValue({ - connected: false, + status: CONNECTION_CHECK_STATUS.FAILED, error: "The role could not be assumed.", }); @@ -461,12 +472,35 @@ describe("AwsConnectStep", () => { ).toBeEnabled(); }); + it("shows a neutral message, not a failure, when the check is still pending", async () => { + // Given: the wait was exhausted with no confirmed outcome (the backend + // check is genuinely still running past the wait). + testProviderConnection.mockResolvedValue({ + status: CONNECTION_CHECK_STATUS.PENDING, + error: + "The connection test is still running. Refresh in a moment to see the result.", + }); + + // When + const { onConnected } = await submitRole(); + + // Then: announced neutrally, not as an alert, and the account stays + // registered rather than reporting a failure the backend never gave. + expect(await screen.findByRole("status")).toHaveTextContent( + /still running/i, + ); + expect(screen.queryByRole("alert")).not.toBeInTheDocument(); + // Nor does it advance: the outcome is still unknown. + expect(onConnected).not.toHaveBeenCalled(); + expect(screen.getByRole("button", { name: "Check again" })).toBeEnabled(); + }); + // The helper always supplies a reason today; this guards the alert against a // future contract that does not. it("falls back to a generic reason when the API gives none", async () => { // Given testProviderConnection.mockResolvedValue({ - connected: false, + status: CONNECTION_CHECK_STATUS.FAILED, error: null, }); @@ -501,7 +535,7 @@ describe("AwsConnectStep", () => { it("drops the failure as soon as the form is edited again", async () => { // Given testProviderConnection.mockResolvedValue({ - connected: false, + status: CONNECTION_CHECK_STATUS.FAILED, error: "The role could not be assumed.", }); const { user } = await submitRole(); @@ -522,8 +556,14 @@ describe("AwsConnectStep", () => { it("moves on once a retry connects", async () => { // Given testProviderConnection - .mockResolvedValueOnce({ connected: false, error: "Denied." }) - .mockResolvedValueOnce({ connected: true, error: null }); + .mockResolvedValueOnce({ + status: CONNECTION_CHECK_STATUS.FAILED, + error: "Denied.", + }) + .mockResolvedValueOnce({ + status: CONNECTION_CHECK_STATUS.SUCCESS, + error: null, + }); const { onConnected, user } = await submitRole(); // When diff --git a/ui/components/providers/wizard/steps/aws/aws-connect-step.tsx b/ui/components/providers/wizard/steps/aws/aws-connect-step.tsx index 32f8727f99..9cca93a5dd 100644 --- a/ui/components/providers/wizard/steps/aws/aws-connect-step.tsx +++ b/ui/components/providers/wizard/steps/aws/aws-connect-step.tsx @@ -20,6 +20,7 @@ import { useWatch, } from "react-hook-form"; +import { ConnectionPending } from "@/components/icons"; import { RadioCard } from "@/components/providers/radio-card"; import { CredentialsRoleHelper } from "@/components/providers/workflow/credentials-role-helper"; import { WizardInputField } from "@/components/providers/workflow/forms/fields"; @@ -48,6 +49,7 @@ import { testProviderConnection } from "@/lib/provider-helpers"; import { useProviderWizardStore } from "@/store/provider-wizard/store"; import type { AWSCredentials, AWSCredentialsRole } from "@/types"; import type { AwsConnectDraft } from "@/types/provider-wizard"; +import { CONNECTION_CHECK_STATUS } from "@/types/providers"; import { awsKeysConnectSchema, @@ -203,19 +205,27 @@ const CONNECTION_FAILED_MESSAGE = const CONNECTION_UNREACHABLE_MESSAGE = "The connection test could not be completed. The account is saved, so you can try again."; +// Fallback only: `testProviderConnection` already carries this same message on +// `error` for a pending result (see `resolveProviderConnectionState`). +const CONNECTION_PENDING_MESSAGE = + "The connection test is still running. Refresh in a moment to see the result."; + /** Footer label for the one-step form: the test and the retry share the submit. */ const resolveActionLabel = ({ isTesting, isSubmitting, hasFailed, + hasPending, }: { isTesting: boolean; isSubmitting: boolean; hasFailed: boolean; + hasPending: boolean; }) => { if (isTesting) return "Testing connection..."; if (isSubmitting) return "Connecting account..."; - return hasFailed ? "Retry connection" : "Connect account"; + if (hasFailed) return "Retry connection"; + return hasPending ? "Check again" : "Connect account"; }; /** Registers the account, stores its credentials and tests the connection in one submit. */ @@ -239,6 +249,12 @@ function useAwsConnectSubmit({ // outcome belongs to this step rather than to any form field. const [isTesting, setIsTesting] = useState(false); const [connectionError, setConnectionError] = useState(null); + // Still running past the wait -- neither a pass nor a fail. Kept separate + // from `connectionError` so it never renders with the destructive styling a + // confirmed failure gets, and never counts as one. + const [connectionPending, setConnectionPending] = useState( + null, + ); // A hook, not `form.formState.isValid` read inline: the React Compiler keys // its memo on the stable `form` object and would freeze a proxy read at false. const { isSubmitting, isValid } = useFormState({ control: form.control }); @@ -265,6 +281,7 @@ function useAwsConnectSubmit({ isTesting, isSubmitting, hasFailed: connectionError !== null, + hasPending: connectionPending !== null, }), actionDisabled: !canSubmit || isBusy, isLoading: isBusy, @@ -272,6 +289,7 @@ function useAwsConnectSubmit({ }, [ canSubmit, connectionError, + connectionPending, isBusy, isSubmitting, isTesting, @@ -279,15 +297,20 @@ function useAwsConnectSubmit({ onUiStateChange, ]); - // A past failure must not sit above the field the user is already correcting. + // A past failure or a still-pending result must not sit above the field the + // user is already correcting. useEffect(() => { - if (connectionError === null) return; - const subscription = form.watch(() => setConnectionError(null)); + if (connectionError === null && connectionPending === null) return; + const subscription = form.watch(() => { + setConnectionError(null); + setConnectionPending(null); + }); return () => subscription.unsubscribe(); - }, [connectionError, form]); + }, [connectionError, connectionPending, form]); const onSubmit = form.handleSubmit(async (values) => { setConnectionError(null); + setConnectionPending(null); const result = await connectAwsAccount({ method, values: { ...values, ...extraValues }, @@ -313,9 +336,11 @@ function useAwsConnectSubmit({ setIsTesting(true); try { const connection = await testProviderConnection(result.providerId); - connected = connection.connected; - if (!connected) { + connected = connection.status === CONNECTION_CHECK_STATUS.SUCCESS; + if (connection.status === CONNECTION_CHECK_STATUS.FAILED) { setConnectionError(connection.error || CONNECTION_FAILED_MESSAGE); + } else if (connection.status === CONNECTION_CHECK_STATUS.PENDING) { + setConnectionPending(connection.error || CONNECTION_PENDING_MESSAGE); } } catch { setConnectionError(CONNECTION_UNREACHABLE_MESSAGE); @@ -326,26 +351,31 @@ function useAwsConnectSubmit({ if (connected && isActiveRef.current) onConnected(); }); - return { onSubmit, isTesting, connectionError }; + return { onSubmit, isTesting, connectionError, connectionPending }; } -/** Progress line while the test runs, or the API's reason once it is refused. */ +/** + * Progress line while the test runs, the neutral message once the wait is + * exhausted with no verdict, or the API's reason once it is refused. + */ function ConnectionFeedback({ isTesting, error, + pending, }: { isTesting: boolean; error: string | null; + pending: string | null; }) { const alertRef = useRef(null); // The form scrolls inside the modal and the action button sits outside it, so - // an error raised from the footer can land above the fold. + // an error or a pending result raised from the footer can land above the fold. useEffect(() => { - if (!error) return; + if (!error && !pending) return; // Guarded: jsdom has no scrollIntoView, and a throw here would unmount the step. alertRef.current?.scrollIntoView?.({ block: "start", behavior: "smooth" }); - }, [error]); + }, [error, pending]); if (isTesting) { return ( @@ -359,6 +389,25 @@ function ConnectionFeedback({ ); } + if (pending) { + return ( +
+
+ ); + } + if (!error) return null; return ( @@ -417,17 +466,18 @@ function AwsRoleConnectForm({ }); const detectedAccountId = parseAwsAccountIdFromRoleArn(roleArn ?? ""); - const { onSubmit, isTesting, connectionError } = useAwsConnectSubmit({ - form, - method: AWS_ACCESS_METHOD.ROLE, - accountField: ProviderCredentialFields.ROLE_ARN, - accountResolved: detectedAccountId !== null, - // The external id is the tenant's, never user input, so it joins at submit time. - extraValues: { [ProviderCredentialFields.EXTERNAL_ID]: externalId }, - onConnected, - onBusyChange, - onUiStateChange, - }); + const { onSubmit, isTesting, connectionError, connectionPending } = + useAwsConnectSubmit({ + form, + method: AWS_ACCESS_METHOD.ROLE, + accountField: ProviderCredentialFields.ROLE_ARN, + accountResolved: detectedAccountId !== null, + // The external id is the tenant's, never user input, so it joins at submit time. + extraValues: { [ProviderCredentialFields.EXTERNAL_ID]: externalId }, + onConnected, + onBusyChange, + onUiStateChange, + }); // One template for every build: self-hosted users set the account that assumes // the role, so the AccountId parameter must stay editable in the console. @@ -437,7 +487,11 @@ function AwsRoleConnectForm({ return (
- +

1. Create the IAM role

@@ -519,19 +573,24 @@ function AwsKeysConnectForm({ }); useDraftValues(form, "keysValues"); - const { onSubmit, isTesting, connectionError } = useAwsConnectSubmit({ - form, - method: AWS_ACCESS_METHOD.CREDENTIALS, - accountField: ProviderCredentialFields.PROVIDER_UID, - onConnected, - onBusyChange, - onUiStateChange, - }); + const { onSubmit, isTesting, connectionError, connectionPending } = + useAwsConnectSubmit({ + form, + method: AWS_ACCESS_METHOD.CREDENTIALS, + accountField: ProviderCredentialFields.PROVIDER_UID, + onConnected, + onBusyChange, + onUiStateChange, + }); return ( - + ({ + pushMock: vi.fn(), + testProviderConnectionMock: vi.fn(), +})); + +vi.mock("next/navigation", () => ({ + useRouter: () => ({ push: pushMock, back: vi.fn() }), +})); + +vi.mock("@/actions/providers", () => ({ + deleteCredentials: vi.fn(), +})); + +vi.mock("@/lib/provider-helpers", () => ({ + testProviderConnection: testProviderConnectionMock, +})); + +import { CONNECTION_CHECK_STATUS } from "@/types/providers"; + +import { + TestConnectionForm, + type TestConnectionProviderData, +} from "./test-connection-form"; + +const providerData: TestConnectionProviderData = { + data: { + id: "provider-1", + type: "providers", + attributes: { + uid: "111111111111", + connection: { connected: false, last_checked_at: null }, + provider: "aws", + alias: "Production", + scanner_args: {}, + }, + relationships: { + secret: { data: { type: "provider-secrets", id: "secret-1" } }, + }, + }, +}; + +describe("TestConnectionForm", () => { + beforeEach(() => { + pushMock.mockReset(); + testProviderConnectionMock.mockReset(); + }); + + it("advances on a confirmed successful connection", async () => { + // Given + testProviderConnectionMock.mockResolvedValue({ + status: CONNECTION_CHECK_STATUS.SUCCESS, + error: null, + }); + const onSuccess = vi.fn(); + const user = userEvent.setup(); + + render( + , + ); + + // When + await user.click(screen.getByRole("button", { name: /continue/i })); + + // Then + expect(onSuccess).toHaveBeenCalledTimes(1); + expect( + screen.queryByText(/issue with your credentials/i), + ).not.toBeInTheDocument(); + }); + + it("shows a destructive failure message for a confirmed failed connection", async () => { + // Given + testProviderConnectionMock.mockResolvedValue({ + status: CONNECTION_CHECK_STATUS.FAILED, + error: "Role trust policy mismatch.", + }); + const user = userEvent.setup(); + + render( + , + ); + + // When + await user.click(screen.getByRole("button", { name: /continue/i })); + + // Then + expect(screen.getByText("Role trust policy mismatch.")).toBeInTheDocument(); + expect( + screen.getByText(/issue with your credentials/i), + ).toBeInTheDocument(); + expect( + screen.getByRole("button", { name: /reset credentials/i }), + ).toBeInTheDocument(); + // Announced to screen readers as soon as it appears, not only on focus. + expect(screen.getByRole("status")).toHaveTextContent( + "Role trust policy mismatch.", + ); + }); + + it("shows a neutral still-running message, not a failure, when the check is still pending", async () => { + // Given: the wait was exhausted and the provider's stored state could not + // confirm an outcome yet (the backend check is genuinely still running). + testProviderConnectionMock.mockResolvedValue({ + status: CONNECTION_CHECK_STATUS.PENDING, + error: + "The connection test is still running. Refresh in a moment to see the result.", + }); + const onSuccess = vi.fn(); + const user = userEvent.setup(); + + render( + , + ); + + // When + await user.click(screen.getByRole("button", { name: /continue/i })); + + // Then: the neutral message shows, but nothing reads as a credentials failure. + expect(screen.getByText(/still running/i)).toBeInTheDocument(); + // Announced to screen readers, same as the failure banner. + expect(screen.getByRole("status")).toHaveTextContent(/still running/i); + expect( + screen.queryByText(/issue with your credentials/i), + ).not.toBeInTheDocument(); + expect( + screen.queryByRole("button", { name: /reset credentials/i }), + ).not.toBeInTheDocument(); + // Does not advance either -- the outcome is still unknown. + expect(onSuccess).not.toHaveBeenCalled(); + expect(pushMock).not.toHaveBeenCalled(); + // The retry control is explicit about what pressing it does now: it is no + // longer the first check, so "Continue" would be misleading. + expect( + screen.getByRole("button", { name: /check again/i }), + ).toBeInTheDocument(); + }); + + it("re-runs the check when 'Check again' is pressed on a still-pending result", async () => { + // Given: the first attempt came back pending. + testProviderConnectionMock.mockResolvedValueOnce({ + status: CONNECTION_CHECK_STATUS.PENDING, + error: "The connection test is still running.", + }); + const user = userEvent.setup(); + + render( + , + ); + await user.click(screen.getByRole("button", { name: /continue/i })); + expect( + screen.getByRole("button", { name: /check again/i }), + ).toBeInTheDocument(); + + // When: pressing "Check again" resolves this time. + testProviderConnectionMock.mockResolvedValueOnce({ + status: CONNECTION_CHECK_STATUS.SUCCESS, + error: null, + }); + await user.click(screen.getByRole("button", { name: /check again/i })); + + // Then + expect(testProviderConnectionMock).toHaveBeenCalledTimes(2); + expect(screen.queryByText(/still running/i)).not.toBeInTheDocument(); + }); +}); diff --git a/ui/components/providers/workflow/forms/test-connection-form.tsx b/ui/components/providers/workflow/forms/test-connection-form.tsx index 687b036cce..a0beb2cb4c 100644 --- a/ui/components/providers/workflow/forms/test-connection-form.tsx +++ b/ui/components/providers/workflow/forms/test-connection-form.tsx @@ -10,11 +10,15 @@ import { useForm } from "react-hook-form"; import { z } from "zod"; import { deleteCredentials } from "@/actions/providers"; -import { CheckIcon } from "@/components/icons"; +import { CheckIcon, ConnectionPending } from "@/components/icons"; import { Button } from "@/components/shadcn"; import { Form } from "@/components/shadcn/form"; -import { testProviderConnection } from "@/lib/provider-helpers"; +import { + testProviderConnection, + type TestConnectionResult, +} from "@/lib/provider-helpers"; import { ProviderType, testConnectionFormSchema } from "@/types"; +import { CONNECTION_CHECK_STATUS } from "@/types/providers"; import { ProviderConnectionInfo } from "./provider-connection-info"; @@ -69,10 +73,8 @@ export const TestConnectionForm = ({ const providerId = searchParams.id; const [apiErrorMessage, setApiErrorMessage] = useState(null); - const [connectionStatus, setConnectionStatus] = useState<{ - connected: boolean; - error: string | null; - } | null>(null); + const [connectionStatus, setConnectionStatus] = + useState(null); const [isResettingCredentials, setIsResettingCredentials] = useState(false); const formSchema = testConnectionFormSchema; @@ -103,7 +105,7 @@ export const TestConnectionForm = ({ setConnectionStatus(result); - if (result.connected) { + if (result.status === CONNECTION_CHECK_STATUS.SUCCESS) { if (onSuccess) { onSuccess(); return; @@ -167,13 +169,17 @@ export const TestConnectionForm = ({ )} - {connectionStatus && !connectionStatus.connected && ( + {connectionStatus?.status === CONNECTION_CHECK_STATUS.FAILED && ( <> -
+
@@ -189,6 +195,30 @@ export const TestConnectionForm = ({ )} + {connectionStatus?.status === CONNECTION_CHECK_STATUS.PENDING && ( +
+
+ {/* Static, not spinning: nothing is polling any more once the wait + is exhausted, so an animated spinner would misrepresent this as + still in progress. */} +
+
+

+ {connectionStatus.error || + "The connection test is still running. Refresh in a moment to see the result."} +

+
+
+ )} + Back to providers - ) : connectionStatus?.error ? ( + ) : connectionStatus?.status === CONNECTION_CHECK_STATUS.FAILED ? (
diff --git a/ui/components/shadcn/tree-view/tree-status-icon.tsx b/ui/components/shadcn/tree-view/tree-status-icon.tsx index 1ef34024b1..c79d6e9838 100644 --- a/ui/components/shadcn/tree-view/tree-status-icon.tsx +++ b/ui/components/shadcn/tree-view/tree-status-icon.tsx @@ -1,6 +1,6 @@ "use client"; -import { CircleCheckIcon, CircleXIcon } from "lucide-react"; +import { CircleCheckIcon, CircleXIcon, InfoIcon } from "lucide-react"; import { cn } from "@/lib/utils"; import { TREE_ITEM_STATUS, TreeItemStatus } from "@/types/tree"; @@ -11,11 +11,14 @@ interface TreeStatusIconProps { } /** - * TreeStatusIcon component - displays success or error status for tree nodes. + * TreeStatusIcon component - displays success, error, or pending status for + * tree nodes. * * Features: * - CircleCheck icon for success (green) * - CircleX icon for error (red) + * - Static Info icon for pending: an item nothing is polling any more but with + * no confirmed outcome, so a spinner would misrepresent it as in progress * - Same size as TreeSpinner for consistent layout */ export function TreeStatusIcon({ status, className }: TreeStatusIconProps) { @@ -37,5 +40,14 @@ export function TreeStatusIcon({ status, className }: TreeStatusIconProps) { ); } + if (status === TREE_ITEM_STATUS.PENDING) { + return ( + + ); + } + return null; } diff --git a/ui/components/shadcn/tree-view/tree-status-indicator.tsx b/ui/components/shadcn/tree-view/tree-status-indicator.tsx index 4a7adb06e4..3dee23b30f 100644 --- a/ui/components/shadcn/tree-view/tree-status-indicator.tsx +++ b/ui/components/shadcn/tree-view/tree-status-indicator.tsx @@ -5,7 +5,7 @@ import { TooltipContent, TooltipTrigger, } from "@/components/shadcn/tooltip"; -import { TreeItemStatus } from "@/types/tree"; +import { TREE_ITEM_STATUS, TreeItemStatus } from "@/types/tree"; import { TreeStatusIcon } from "./tree-status-icon"; @@ -22,7 +22,11 @@ export function TreeStatusIndicator({ return null; } - if (status === "error" && errorMessage) { + if ( + (status === TREE_ITEM_STATUS.ERROR || + status === TREE_ITEM_STATUS.PENDING) && + errorMessage + ) { return ( diff --git a/ui/lib/helper.test.ts b/ui/lib/helper.test.ts index f59dda487b..5bc5682f66 100644 --- a/ui/lib/helper.test.ts +++ b/ui/lib/helper.test.ts @@ -1,9 +1,11 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { + checkTaskStatus, downloadScanZip, getErrorMessage, permissionFormFields, + TASK_STATUS_MAX_RETRIES_ERROR, } from "./helper"; vi.mock("@/actions/scans", () => ({ @@ -11,8 +13,9 @@ vi.mock("@/actions/scans", () => ({ getCompliancePdfReport: vi.fn(), })); +const { getTask } = vi.hoisted(() => ({ getTask: vi.fn() })); vi.mock("@/actions/task", () => ({ - getTask: vi.fn(), + getTask, })); vi.mock("@/auth.config", () => ({ @@ -140,6 +143,42 @@ describe("getErrorMessage", () => { }); }); +describe("checkTaskStatus", () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("keeps polling past a caller's default retry window and reports success once the task completes", async () => { + let calls = 0; + getTask.mockImplementation(async () => { + calls += 1; + if (calls < 25) { + return { data: { attributes: { state: "executing" } } }; + } + return { data: { attributes: { state: "completed" } } }; + }); + + // 25 retries exceeds the generic 20-retry default, simulating a task that + // outlives the caller's usual wait. + const result = await checkTaskStatus("task-id", 40, 1); + + expect(result.completed).toBe(true); + expect(calls).toBe(25); + }); + + it("reports the exhausted-retries error once maxRetries is used up", async () => { + getTask.mockResolvedValue({ data: { attributes: { state: "executing" } } }); + + const result = await checkTaskStatus("task-id", 3, 1); + + expect(result).toEqual({ + completed: false, + error: TASK_STATUS_MAX_RETRIES_ERROR, + }); + expect(getTask).toHaveBeenCalledTimes(3); + }); +}); + describe("permissionFormFields", () => { it("describes Unlimited Visibility as organization-wide", () => { // Given diff --git a/ui/lib/helper.ts b/ui/lib/helper.ts index d62ddaf65a..5612cbdd99 100644 --- a/ui/lib/helper.ts +++ b/ui/lib/helper.ts @@ -351,6 +351,10 @@ export const isGithubOAuthEnabled = !!process.env.SOCIAL_GITHUB_OAUTH_CLIENT_ID && !!process.env.SOCIAL_GITHUB_OAUTH_CLIENT_SECRET; +/** Returned by {@link checkTaskStatus} when `maxRetries` is exhausted, so callers + * can tell an exhausted wait apart from a real task failure. */ +export const TASK_STATUS_MAX_RETRIES_ERROR = "Max retries exceeded"; + /** * Polls a task until it settles. The settled task comes back with the verdict so * callers can read its result without fetching the same task again. @@ -390,7 +394,7 @@ export const checkTaskStatus = async ( } } - return { completed: false, error: "Max retries exceeded" }; + return { completed: false, error: TASK_STATUS_MAX_RETRIES_ERROR }; }; export const wait = (ms: number) => diff --git a/ui/lib/provider-helpers.test.ts b/ui/lib/provider-helpers.test.ts index 8efba9ac80..0a3a4ccc95 100644 --- a/ui/lib/provider-helpers.test.ts +++ b/ui/lib/provider-helpers.test.ts @@ -1,16 +1,155 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; -const { checkConnectionProvider, checkTaskStatus } = vi.hoisted(() => ({ - checkConnectionProvider: vi.fn(), - checkTaskStatus: vi.fn(), +const { checkConnectionProvider, checkTaskStatus, getProvider } = vi.hoisted( + () => ({ + checkConnectionProvider: vi.fn(), + checkTaskStatus: vi.fn(), + getProvider: vi.fn(), + }), +); +vi.mock("@/actions/providers/providers", () => ({ + checkConnectionProvider, + getProvider, +})); +vi.mock("./helper", () => ({ + checkTaskStatus, + TASK_STATUS_MAX_RETRIES_ERROR: "Max retries exceeded", })); -vi.mock("@/actions/providers/providers", () => ({ checkConnectionProvider })); -vi.mock("./helper", () => ({ checkTaskStatus })); -import { testProviderConnection } from "./provider-helpers"; +import { + CONNECTION_CHECK_STATUS, + PROVIDER_CONNECTION_CHECK_MAX_RETRIES, + PROVIDER_CONNECTION_CHECK_POLL_DELAY_MS, + resolveProviderConnectionState, + testProviderConnection, +} from "./provider-helpers"; + +describe("resolveProviderConnectionState", () => { + it("trusts a stored connected=true once last_checked_at differs from the baseline", async () => { + getProvider.mockResolvedValue({ + data: { + attributes: { + connection: { + connected: true, + last_checked_at: "2026-01-01T00:00:10.000Z", + }, + }, + }, + }); + + expect(await resolveProviderConnectionState("account", null)).toEqual({ + status: CONNECTION_CHECK_STATUS.SUCCESS, + error: null, + }); + }); + + it("treats a stored connected=true as pending while last_checked_at still matches the baseline", async () => { + // Given: the provider was connected from a previous check, and this check -- + // dispatched after that baseline was captured -- is still running. + const baseline = "2025-12-31T23:59:59.000Z"; + getProvider.mockResolvedValue({ + data: { + attributes: { + connection: { connected: true, last_checked_at: baseline }, + }, + }, + }); + + const result = await resolveProviderConnectionState("account", baseline); + + expect(result.status).toBe(CONNECTION_CHECK_STATUS.PENDING); + expect(result.error).toMatch(/still running/i); + }); + + it("reports success once last_checked_at changes, even when the new value sorts earlier than the baseline", async () => { + // Given: a deployment whose clocks are not synchronised -- the server writes + // a `last_checked_at` that, read as a plain string/date, sorts *before* the + // baseline captured moments earlier from the same server. A timestamp + // comparison would misread this as stale; a value comparison does not. + const baseline = "2026-06-01T00:00:00.000Z"; + getProvider.mockResolvedValue({ + data: { + attributes: { + connection: { + connected: true, + last_checked_at: "2020-01-01T00:00:00.000Z", + }, + }, + }, + }); + + const result = await resolveProviderConnectionState("account", baseline); + + expect(result).toEqual({ + status: CONNECTION_CHECK_STATUS.SUCCESS, + error: null, + }); + }); + + it("treats a missing last_checked_at as pending even when connected is true", async () => { + getProvider.mockResolvedValue({ + data: { + attributes: { + connection: { connected: true, last_checked_at: null }, + }, + }, + }); + + const result = await resolveProviderConnectionState("account", null); + + expect(result.status).toBe(CONNECTION_CHECK_STATUS.PENDING); + }); + + it("treats an unknown baseline as unresolved, never trusting a pre-existing stored result", async () => { + // Given: the pre-dispatch baseline read failed, so there is nothing to + // compare this result against. + getProvider.mockResolvedValue({ + data: { + attributes: { + connection: { + connected: true, + last_checked_at: "2026-01-01T00:00:10.000Z", + }, + }, + }, + }); + + const result = await resolveProviderConnectionState("account", undefined); + + expect(result.status).toBe(CONNECTION_CHECK_STATUS.PENDING); + }); + + it("reports a changed connected=false as a confirmed failure", async () => { + getProvider.mockResolvedValue({ + data: { + attributes: { + connection: { + connected: false, + last_checked_at: "2026-01-01T00:00:10.000Z", + }, + }, + }, + }); + + const result = await resolveProviderConnectionState("account", null); + + expect(result).toEqual({ + status: CONNECTION_CHECK_STATUS.FAILED, + error: expect.stringMatching(/test the connection again/i), + }); + }); +}); describe("provider connection confirmation", () => { beforeEach(() => { + // The baseline read (before dispatch) and the exhausted-wait fallback read + // both go through `getProvider`; default to "no prior check" unless a test + // overrides it. + getProvider.mockResolvedValue({ + data: { + attributes: { connection: { connected: null, last_checked_at: null } }, + }, + }); checkConnectionProvider.mockResolvedValue({ data: { id: "task" } }); }); it.each([undefined, {}, { connected: "true" }, { connected: false }])( @@ -20,7 +159,9 @@ describe("provider connection confirmation", () => { completed: true, task: { data: { attributes: { result } } }, }); - expect((await testProviderConnection("account")).connected).toBe(false); + expect((await testProviderConnection("account")).status).toBe( + CONNECTION_CHECK_STATUS.FAILED, + ); }, ); it("advances on an explicitly successful connection", async () => { @@ -29,8 +170,199 @@ describe("provider connection confirmation", () => { task: { data: { attributes: { result: { connected: true } } } }, }); expect(await testProviderConnection("account")).toEqual({ - connected: true, + status: CONNECTION_CHECK_STATUS.SUCCESS, error: null, }); }); + + it("reads the baseline before dispatching the check", async () => { + checkTaskStatus.mockResolvedValue({ + completed: true, + task: { data: { attributes: { result: { connected: true } } } }, + }); + + const callOrder: string[] = []; + getProvider.mockImplementation(async () => { + callOrder.push("getProvider"); + return { + data: { + attributes: { + connection: { connected: null, last_checked_at: null }, + }, + }, + }; + }); + checkConnectionProvider.mockImplementation(async () => { + callOrder.push("checkConnectionProvider"); + return { data: { id: "task" } }; + }); + + await testProviderConnection("account"); + + expect(callOrder).toEqual(["getProvider", "checkConnectionProvider"]); + }); + + it("sizes the wait past the backend's 120s provider-connection-check time limit", async () => { + checkTaskStatus.mockResolvedValue({ + completed: true, + task: { data: { attributes: { result: { connected: true } } } }, + }); + + await testProviderConnection("account"); + + expect(checkTaskStatus).toHaveBeenCalledWith( + "task", + PROVIDER_CONNECTION_CHECK_MAX_RETRIES, + PROVIDER_CONNECTION_CHECK_POLL_DELAY_MS, + ); + expect( + PROVIDER_CONNECTION_CHECK_MAX_RETRIES * + PROVIDER_CONNECTION_CHECK_POLL_DELAY_MS, + ).toBeGreaterThan(120_000); + }); + + describe("when the wait is exhausted", () => { + beforeEach(() => { + checkTaskStatus.mockResolvedValue({ + completed: false, + error: "Max retries exceeded", + }); + }); + + it("reports success when the stored state changed after the baseline was captured", async () => { + // Baseline (read before dispatch): no prior check. Fallback read: a fresh + // result landed while the UI was waiting. + getProvider + .mockResolvedValueOnce({ + data: { + attributes: { + connection: { connected: null, last_checked_at: null }, + }, + }, + }) + .mockResolvedValueOnce({ + data: { + attributes: { + connection: { + connected: true, + last_checked_at: "2999-01-01T00:00:00Z", + }, + }, + }, + }); + + expect(await testProviderConnection("account")).toEqual({ + status: CONNECTION_CHECK_STATUS.SUCCESS, + error: null, + }); + }); + + it("does not report success from a stale stored result predating this check", async () => { + // Given: the provider was already connected from a previous check -- + // captured as the baseline before this check was dispatched -- and the + // fallback read comes back with that exact same (unchanged) value, + // meaning the new check has not written a result yet. + const priorLastCheckedAt = "2025-06-01T00:00:00Z"; + getProvider.mockResolvedValue({ + data: { + attributes: { + connection: { + connected: true, + last_checked_at: priorLastCheckedAt, + }, + }, + }, + }); + + const result = await testProviderConnection("account"); + + expect(result.status).not.toBe(CONNECTION_CHECK_STATUS.SUCCESS); + expect(result.status).toBe(CONNECTION_CHECK_STATUS.PENDING); + }); + + it("reports pending, not connected, when the browser clock is behind the server and the stored result predates the check", async () => { + // Given: a deployment whose clocks are not synchronised -- the browser's + // clock is behind the server's. Under a clock-based comparison this could + // make an older stored result look newer than the check's start and be + // reported as connected. The baseline here is captured from the server's + // own prior value, so the comparison never depends on the browser's clock + // at all: an unchanged value is still unchanged. + const staleServerTimestamp = "2026-03-01T12:00:00Z"; + getProvider.mockResolvedValue({ + data: { + attributes: { + connection: { + connected: true, + last_checked_at: staleServerTimestamp, + }, + }, + }, + }); + + const result = await testProviderConnection("account"); + + expect(result.status).toBe(CONNECTION_CHECK_STATUS.PENDING); + expect(result.status).not.toBe(CONNECTION_CHECK_STATUS.SUCCESS); + }); + + it("reports the failure when the provider is confirmed not connected by this check", async () => { + getProvider + .mockResolvedValueOnce({ + data: { + attributes: { + connection: { connected: null, last_checked_at: null }, + }, + }, + }) + .mockResolvedValueOnce({ + data: { + attributes: { + connection: { + connected: false, + last_checked_at: "2999-01-01T00:00:00Z", + }, + }, + }, + }); + + const result = await testProviderConnection("account"); + + expect(result.status).toBe(CONNECTION_CHECK_STATUS.FAILED); + expect(result.error).toMatch(/test the connection again/i); + }); + + it("shows a neutral still-checking message when the provider state is undetermined", async () => { + getProvider.mockResolvedValue({ + data: { + attributes: { + connection: { connected: null, last_checked_at: null }, + }, + }, + }); + + const result = await testProviderConnection("account"); + + expect(result.status).toBe(CONNECTION_CHECK_STATUS.PENDING); + expect(result.error).toMatch(/still running|refresh/i); + expect(result.error?.toLowerCase()).not.toContain("error"); + expect(result.error?.toLowerCase()).not.toContain("failed"); + }); + }); + + it("does not re-read provider state for a real task failure", async () => { + checkTaskStatus.mockResolvedValue({ + completed: false, + error: "Unexpected task state", + }); + + const result = await testProviderConnection("account"); + + // The baseline read (before dispatch) still happens, but nothing reads + // provider state again to resolve this failure. + expect(getProvider).toHaveBeenCalledTimes(1); + expect(result).toEqual({ + status: CONNECTION_CHECK_STATUS.FAILED, + error: "Unexpected task state", + }); + }); }); diff --git a/ui/lib/provider-helpers.ts b/ui/lib/provider-helpers.ts index 329833f384..22e26edd29 100644 --- a/ui/lib/provider-helpers.ts +++ b/ui/lib/provider-helpers.ts @@ -1,12 +1,24 @@ -import { checkConnectionProvider } from "@/actions/providers/providers"; import { + checkConnectionProvider, + getProvider, +} from "@/actions/providers/providers"; +import { + CONNECTION_CHECK_STATUS, + type ConnectionCheckStatus, ProviderEntity, ProviderProps, ProvidersApiResponse, ProviderType, } from "@/types/providers"; -import { checkTaskStatus } from "./helper"; +import { checkTaskStatus, TASK_STATUS_MAX_RETRIES_ERROR } from "./helper"; + +// Re-exported so callers that only need the status enum (e.g. +// `org-account-selection.utils.ts` and its tests) can import it from +// `@/types/providers` directly, without pulling in this module's server-action +// dependencies. +export { CONNECTION_CHECK_STATUS }; +export type { ConnectionCheckStatus }; export const extractProviderUIDs = ( providersData: ProvidersApiResponse, @@ -172,10 +184,119 @@ export const requiresBackButton = (via?: string | null): boolean => { }; export interface TestConnectionResult { - connected: boolean; + status: ConnectionCheckStatus; error: string | null; } +/** + * The `provider-connection-check` Celery task has a 120s hard time limit + * (api/src/backend/config/celery.py `task_annotations`). Poll long enough to + * cover a full run plus queueing/network slack, instead of the generic 30s + * default, which cuts the wait off well before the backend gives up. + */ +export const PROVIDER_CONNECTION_CHECK_TASK_TIME_LIMIT_MS = 120_000; +const PROVIDER_CONNECTION_CHECK_POLL_BUFFER_MS = 30_000; +export const PROVIDER_CONNECTION_CHECK_POLL_DELAY_MS = 1_500; +export const PROVIDER_CONNECTION_CHECK_MAX_RETRIES = Math.ceil( + (PROVIDER_CONNECTION_CHECK_TASK_TIME_LIMIT_MS + + PROVIDER_CONNECTION_CHECK_POLL_BUFFER_MS) / + PROVIDER_CONNECTION_CHECK_POLL_DELAY_MS, +); + +const CONNECTION_NOT_CONFIRMED_MESSAGE = + "Connection was not confirmed. Test the connection again."; +const CONNECTION_STILL_RUNNING_MESSAGE = + "The connection test is still running. Refresh in a moment to see the result."; + +/** + * Reads a provider's current `connection.last_checked_at`, to be captured + * *before* a connection check is dispatched for it. Passed on to + * `resolveProviderConnectionState` as the value a later read is compared against + * -- see that function for why the comparison is by value, not by clock. + * + * `undefined` means the read failed (network error, provider not found) and is + * distinct from `null` ("no prior check exists"): `resolveProviderConnectionState` + * treats an unknown baseline as impossible to clear, never as an implicit change. + */ +export async function captureConnectionBaseline( + providerId: string, +): Promise { + const formData = new FormData(); + formData.append("id", providerId); + + const providerResponse = await getProvider(formData); + if (!providerResponse?.data) { + return undefined; + } + + return providerResponse.data.attributes?.connection?.last_checked_at ?? null; +} + +/** + * Re-reads a provider's persisted connection state from the API. Used when a + * connection-check wait is exhausted: the backend task may still be running (or + * may already have finished after the UI stopped waiting on it), so this reports + * whatever the provider record currently says instead of a flat error. + * + * The stored `connection` is the result of the *last check that finished*, not + * necessarily the one this call is following up on -- e.g. a provider was + * connected, the user changed its credentials, and the new check is still + * running past the wait. `baseline` -- the provider's `last_checked_at` captured + * (via {@link captureConnectionBaseline}) before this check was dispatched -- + * guards against reporting that stale result as current: the stored state is + * only trusted once `last_checked_at` has changed from it. + * + * This compares the two values directly rather than comparing timestamps, so it + * holds even in deployments whose clocks are not synchronised: `last_checked_at` + * is written by the server, but the previous check used a wait deadline taken + * from the browser's clock, and a browser clock that drifts from the server's + * could make an older result look newer than the check, or a finished check look + * like it is still pending. + */ +export async function resolveProviderConnectionState( + providerId: string, + baseline: string | null | undefined, +): Promise { + const formData = new FormData(); + formData.append("id", providerId); + + const providerResponse = await getProvider(formData); + const connection = providerResponse?.data?.attributes?.connection; + const lastCheckedAt = connection?.last_checked_at; + + // An unknown baseline (the pre-dispatch read failed) can never be cleared -- + // there is nothing to compare against, so the result cannot be trusted yet. + const isCurrent = + baseline !== undefined && !!lastCheckedAt && lastCheckedAt !== baseline; + + if (!isCurrent) { + // No persisted result yet, or it predates this check -- the backend task + // may still be running. + return { + status: CONNECTION_CHECK_STATUS.PENDING, + error: CONNECTION_STILL_RUNNING_MESSAGE, + }; + } + + if (connection?.connected === true) { + return { status: CONNECTION_CHECK_STATUS.SUCCESS, error: null }; + } + + if (connection?.connected === false) { + return { + status: CONNECTION_CHECK_STATUS.FAILED, + error: CONNECTION_NOT_CONFIRMED_MESSAGE, + }; + } + + // Current per its timestamp, but `connected` is null -- treat the same as + // still pending rather than guessing a pass or fail. + return { + status: CONNECTION_CHECK_STATUS.PENDING, + error: CONNECTION_STILL_RUNNING_MESSAGE, + }; +} + /** * Tests a provider's connection end-to-end: submits the task, polls until * completion, and returns the real connection result. @@ -186,6 +307,10 @@ export interface TestConnectionResult { export async function testProviderConnection( providerId: string, ): Promise { + // Captured before the check is dispatched, so any `last_checked_at` this check + // eventually writes is guaranteed to differ from it. + const baseline = await captureConnectionBaseline(providerId); + const formData = new FormData(); formData.append("providerId", providerId); @@ -193,21 +318,31 @@ export async function testProviderConnection( if (data?.errors && data.errors.length > 0) { return { - connected: false, + status: CONNECTION_CHECK_STATUS.FAILED, error: data.errors[0]?.detail ?? "Unknown error", }; } const taskId = data?.data?.id; if (!taskId) { - return { connected: false, error: "No task ID returned" }; + return { + status: CONNECTION_CHECK_STATUS.FAILED, + error: "No task ID returned", + }; } - const taskResult = await checkTaskStatus(taskId); + const taskResult = await checkTaskStatus( + taskId, + PROVIDER_CONNECTION_CHECK_MAX_RETRIES, + PROVIDER_CONNECTION_CHECK_POLL_DELAY_MS, + ); if (!taskResult.completed) { + if (taskResult.error === TASK_STATUS_MAX_RETRIES_ERROR) { + return resolveProviderConnectionState(providerId, baseline); + } return { - connected: false, + status: CONNECTION_CHECK_STATUS.FAILED, error: taskResult.error ?? "Connection test timed out", }; } @@ -217,10 +352,9 @@ export async function testProviderConnection( const connected = result?.connected === true; return { - connected, - error: connected - ? null - : result?.error || - "Connection was not confirmed. Test the connection again.", + status: connected + ? CONNECTION_CHECK_STATUS.SUCCESS + : CONNECTION_CHECK_STATUS.FAILED, + error: connected ? null : result?.error || CONNECTION_NOT_CONFIRMED_MESSAGE, }; } diff --git a/ui/types/providers.ts b/ui/types/providers.ts index f6f898ce48..272578bf32 100644 --- a/ui/types/providers.ts +++ b/ui/types/providers.ts @@ -1,4 +1,5 @@ import type { ScheduleFrequency } from "./schedules"; +import { TASK_OUTCOME, type TaskOutcome } from "./tasks"; export const PROVIDER_TYPES = [ "aws", @@ -22,6 +23,17 @@ export const PROVIDER_TYPES = [ /** The closed set of provider types this UI build ships bespoke assets for. */ export type KnownProviderType = (typeof PROVIDER_TYPES)[number]; +/** + * Outcome of a provider connection check (or a poll of one): confirmed + * connected, confirmed failed, or still running past the wait. An alias of the + * generic `TASK_OUTCOME` (see `types/tasks.ts`), which `pollTaskCompletion` also + * returns for the unrelated task it polls (organization/node deletion). Kept + * import-free (types only) so it can be used by test doubles and UI-only code + * without pulling in `lib/provider-helpers.ts`'s server-action dependencies. + */ +export const CONNECTION_CHECK_STATUS = TASK_OUTCOME; +export type ConnectionCheckStatus = TaskOutcome; + // Autocomplete for predefined + open for dynamic providers export type ProviderType = KnownProviderType | (string & {}); diff --git a/ui/types/tasks.ts b/ui/types/tasks.ts index a3ccb40b3f..838e1dc7a4 100644 --- a/ui/types/tasks.ts +++ b/ui/types/tasks.ts @@ -1,3 +1,18 @@ +/** + * Generic settle outcome for a polled async task: it succeeded, it failed, or the + * wait was exhausted with the task still running. `CONNECTION_CHECK_STATUS` in + * `types/providers.ts` is a connection-specific alias of this same shape, kept as + * its own export so a caller that only cares about a connection result does not + * have to name a generic task type to use it. + */ +export const TASK_OUTCOME = { + SUCCESS: "success", + FAILED: "failed", + PENDING: "pending", +} as const; + +export type TaskOutcome = (typeof TASK_OUTCOME)[keyof typeof TASK_OUTCOME]; + export type TaskState = | "available" | "scheduled" diff --git a/ui/types/tree.ts b/ui/types/tree.ts index d9b2f88c9f..a0ab5005f0 100644 --- a/ui/types/tree.ts +++ b/ui/types/tree.ts @@ -6,11 +6,14 @@ */ /** - * Status indicator for tree items after loading completes + * Status indicator for tree items after loading completes. `PENDING` is for an + * item whose outcome never arrived even though nothing is polling it any more -- + * distinct from `isLoading`, which is for an item actively being polled. */ export const TREE_ITEM_STATUS = { SUCCESS: "success", ERROR: "error", + PENDING: "pending", } as const; export type TreeItemStatus = @@ -33,7 +36,7 @@ export interface TreeDataItem { disabled?: boolean; /** Whether the item is in a loading state (shows spinner) */ isLoading?: boolean; - /** Status indicator shown after loading (success/error) */ + /** Status indicator shown after loading (success/error/pending) */ status?: TreeItemStatus; /** Optional error detail used by status icon tooltip */ errorMessage?: string;