From 9e1c502ecee50f743ae4af04d388f3e2b53d3639 Mon Sep 17 00:00:00 2001 From: Atif Ali Date: Thu, 11 Jun 2026 09:30:15 +0500 Subject: [PATCH] fix(site): delink agent health from script failures (#26124) --- .../modules/resources/AgentRow.stories.tsx | 2 +- site/src/modules/resources/AgentRow.tsx | 49 ++++--- .../modules/resources/AgentStatus.stories.tsx | 27 ++-- site/src/modules/resources/AgentStatus.tsx | 31 +---- site/src/modules/workspaces/health.test.ts | 128 +++++++++++++++++- site/src/modules/workspaces/health.ts | 57 ++++++-- 6 files changed, 214 insertions(+), 80 deletions(-) diff --git a/site/src/modules/resources/AgentRow.stories.tsx b/site/src/modules/resources/AgentRow.stories.tsx index 30002d60a8..a8a1249c0b 100644 --- a/site/src/modules/resources/AgentRow.stories.tsx +++ b/site/src/modules/resources/AgentRow.stories.tsx @@ -216,7 +216,7 @@ export const ConnectingWithStartupLogs: Story = { play: async ({ canvasElement }) => { const canvas = within(canvasElement); - // Agent is connecting (hasAgentIssues=true) but no script has failed. + // Agent is connecting (hasConnectivityIssues=true) but no script has failed. // Old code snapped to the Startup Script tab; the fix keeps us on All Logs. const allLogsTab = await canvas.findByRole("tab", { name: "All Logs" }); await waitFor(() => diff --git a/site/src/modules/resources/AgentRow.tsx b/site/src/modules/resources/AgentRow.tsx index bc58ddf9d4..a0e07db1d8 100644 --- a/site/src/modules/resources/AgentRow.tsx +++ b/site/src/modules/resources/AgentRow.tsx @@ -58,7 +58,7 @@ import { import { useProxy } from "#/contexts/ProxyContext"; import { useClipboard } from "#/hooks/useClipboard"; import { useFeatureVisibility } from "#/modules/dashboard/useFeatureVisibility"; -import { getAgentHealthIssues } from "#/modules/workspaces/health"; +import { getAgentConnectivityIssues } from "#/modules/workspaces/health"; import { AgentAlert } from "#/pages/WorkspacePage/AgentAlert"; import { AppStatuses } from "#/pages/WorkspacePage/AppStatuses"; import { cn } from "#/utils/cn"; @@ -104,9 +104,11 @@ const statusBorderClassByLifecycle: Partial< starting: "border-border-pending", shutting_down: "border-border-pending", ready: "border-border-success", - start_timeout: "border-border-warning", + // Script errors and timeouts do not affect agent connectivity; they are + // surfaced in the per-script log tabs instead. + start_timeout: "border-border-success", shutdown_timeout: "border-border-warning", - start_error: "border-border-warning", + start_error: "border-border-success", shutdown_error: "border-border-warning", off: "border-border", }; @@ -161,13 +163,16 @@ export const AgentRow: FC = ({ const runningScriptsCount = agent.scripts.filter( (s) => s.run_on_start && !s.status, ).length; - const healthIssues = getAgentHealthIssues(agent); - const hasAgentIssues = healthIssues.length > 0; - const hasWarningIssues = healthIssues.some((i) => i.severity === "warning"); + // Use connectivity issues for agent panel styling and visibility decisions. + // Script issues are handled separately in the log tabs. + const connectivityIssues = getAgentConnectivityIssues(agent); + const hasConnectivityIssues = connectivityIssues.length > 0; + const hasWarningConnectivityIssues = connectivityIssues.some( + (i) => i.severity === "warning", + ); const { proxy } = useProxy(); const [showLogs, setShowLogs] = useState( - (["starting", "start_timeout"].includes(agent.lifecycle_state) || - hasAgentIssues) && + (agent.lifecycle_state !== "ready" || hasConnectivityIssues) && hasStartupFeatures, ); const agentLogs = useAgentLogs({ agentId: agent.id, enabled: showLogs }); @@ -177,10 +182,10 @@ export const AgentRow: FC = ({ useEffect(() => { setShowLogs( - (agent.lifecycle_state !== "ready" || hasAgentIssues) && + (agent.lifecycle_state !== "ready" || hasConnectivityIssues) && hasStartupFeatures, ); - }, [agent.lifecycle_state, hasAgentIssues, hasStartupFeatures]); + }, [agent.lifecycle_state, hasConnectivityIssues, hasStartupFeatures]); // This is a layout effect to remove flicker when we're scrolling to the bottom. // biome-ignore lint/correctness/useExhaustiveDependencies: consider refactoring @@ -331,7 +336,8 @@ export const AgentRow: FC = ({ ...sortedSourceLogTabs, ]; const hasAnyLogs = agentLogs.length > 0; - const shouldExpandLogs = showLogs || (!hasStartupFeatures && hasAgentIssues); + const shouldExpandLogs = + showLogs || (!hasStartupFeatures && hasConnectivityIssues); const shouldShowLogsTabs = hasStartupFeatures && hasAnyLogs; const logTabsMeasureEnabled = shouldShowLogsTabs && showLogs; const { @@ -542,7 +548,7 @@ export const AgentRow: FC = ({ Logs {agent.lifecycle_state === "starting" && runningScriptsCount > 0 && - healthIssues.length === 0 && ( + connectivityIssues.length === 0 && ( = ({ {runningScriptsCount} )} - {hasAgentIssues && ( + {hasConnectivityIssues && ( - {hasWarningIssues ? ( + {hasWarningConnectivityIssues ? ( ) : ( )} - {healthIssues.length} + {connectivityIssues.length} )} @@ -579,12 +585,13 @@ export const AgentRow: FC = ({ Collapse's `in` condition is needed here, or else the Spinner will also show as Collapse is closing */} - {shouldExpandLogs && !(hasAgentIssues || shouldShowLogsTabs) && ( - - )} - {hasAgentIssues && ( + {shouldExpandLogs && + !(hasConnectivityIssues || shouldShowLogsTabs) && ( + + )} + {hasConnectivityIssues && (
- {healthIssues.map((issue) => ( + {connectivityIssues.map((issue) => ( { - await expectTooltip( - "Startup script failed", - agentScriptMessages.start_error.title, - agentScriptMessages.start_error.detail, - true, - ); + expect(screen.getByRole("status", { name: "Ready" })).toBeInTheDocument(); }, }; +// start_timeout no longer affects the status dot; the agent shows as Ready +// because the timeout is surfaced in the per-script log tabs instead. export const StartupScriptTimeout: Story = { args: { agent: { @@ -81,12 +80,7 @@ export const StartupScriptTimeout: Story = { }, }, play: async () => { - await expectTooltip( - "Startup script timeout", - agentScriptMessages.start_timeout.title, - agentScriptMessages.start_timeout.detail, - true, - ); + expect(screen.getByRole("status", { name: "Ready" })).toBeInTheDocument(); }, }; @@ -143,6 +137,8 @@ export const ConnectionTimeout: Story = { }, }; +// start_error renders as Ready regardless of whether a troubleshoot URL is +// provided, because script errors are surfaced in the per-script log tabs. export const StartupScriptFailedNoTroubleshootURL: Story = { args: { agent: { @@ -153,12 +149,7 @@ export const StartupScriptFailedNoTroubleshootURL: Story = { }, }, play: async () => { - await expectTooltip( - "Startup script failed", - agentScriptMessages.start_error.title, - agentScriptMessages.start_error.detail, - false, - ); + expect(screen.getByRole("status", { name: "Ready" })).toBeInTheDocument(); }, }; diff --git a/site/src/modules/resources/AgentStatus.tsx b/site/src/modules/resources/AgentStatus.tsx index fc0e8f65c8..2bb1cfbe74 100644 --- a/site/src/modules/resources/AgentStatus.tsx +++ b/site/src/modules/resources/AgentStatus.tsx @@ -126,25 +126,6 @@ interface DevcontainerStatusProps { agent?: WorkspaceAgent; } -const StartTimeoutLifecycle: FC = ({ agent }) => ( - -); - -const StartErrorLifecycle: FC = ({ agent }) => ( - -); - const ShuttingDownLifecycle: FC = () => { return ( @@ -203,11 +184,13 @@ const ConnectedStatus: FC = ({ agent }) => { if (agent.lifecycle_state === "ready") { return ; } - if (agent.lifecycle_state === "start_timeout") { - return ; - } - if (agent.lifecycle_state === "start_error") { - return ; + // Script errors and timeouts do not affect agent connectivity. + // These states are surfaced in the per-script log tabs instead. + if ( + agent.lifecycle_state === "start_timeout" || + agent.lifecycle_state === "start_error" + ) { + return ; } if (agent.lifecycle_state === "shutting_down") { return ; diff --git a/site/src/modules/workspaces/health.test.ts b/site/src/modules/workspaces/health.test.ts index b94572803b..5a34235be9 100644 --- a/site/src/modules/workspaces/health.test.ts +++ b/site/src/modules/workspaces/health.test.ts @@ -9,7 +9,11 @@ import { MockWorkspaceAgentStartError, MockWorkspaceAgentStartTimeout, } from "#/testHelpers/entities"; -import { getAgentHealthIssues } from "./health"; +import { + getAgentConnectivityIssues, + getAgentHealthIssues, + getAgentScriptIssues, +} from "./health"; interface AgentOverrides { status?: WorkspaceAgentStatus; @@ -24,6 +28,128 @@ function buildAgent(overrides: AgentOverrides): WorkspaceAgent { }; } +describe("getAgentConnectivityIssues", () => { + it("returns disconnected issue for a disconnected agent", () => { + expect( + getAgentConnectivityIssues(buildAgent({ status: "disconnected" })), + ).toContainEqual( + expect.objectContaining({ + title: "Workspace agent has disconnected", + severity: "warning", + prominent: false, + }), + ); + }); + + it("returns timeout issue for a timed-out agent", () => { + expect( + getAgentConnectivityIssues(buildAgent({ status: "timeout" })), + ).toContainEqual( + expect.objectContaining({ + title: "Agent is taking longer than expected to connect", + severity: "warning", + prominent: false, + }), + ); + }); + + it("does not return script issues", () => { + const issues = getAgentConnectivityIssues( + buildAgent(MockWorkspaceAgentStartError), + ); + expect(issues).not.toContainEqual( + expect.objectContaining({ + title: `"Startup Script" failed`, + }), + ); + }); + + it("returns empty list for healthy ready connected agent", () => { + expect( + getAgentConnectivityIssues( + buildAgent({ status: "connected", lifecycle_state: "ready" }), + ), + ).toEqual([]); + }); + + it("returns connecting issue for a connecting agent", () => { + expect( + getAgentConnectivityIssues( + buildAgent({ status: "connecting", lifecycle_state: "starting" }), + ), + ).toContainEqual( + expect.objectContaining({ + title: "Workspace agent is connecting", + severity: "info", + prominent: false, + }), + ); + }); + + it("returns shutdown issue for shutdown lifecycle states", () => { + for (const lifecycle_state of [ + "shutting_down", + "shutdown_error", + "shutdown_timeout", + ] as const) { + expect( + getAgentConnectivityIssues(buildAgent({ lifecycle_state })), + ).toContainEqual( + expect.objectContaining({ + title: "Workspace agent is shutting down", + severity: "info", + }), + ); + } + }); +}); + +describe("getAgentScriptIssues", () => { + it("returns script issues", () => { + const issues = getAgentScriptIssues( + buildAgent(MockWorkspaceAgentStartError), + ); + expect(issues).toContainEqual( + expect.objectContaining({ + title: `"Startup Script" failed`, + severity: "warning", + prominent: false, + }), + ); + expect(issues).toContainEqual( + expect.objectContaining({ + title: `"time" is taking longer than expected`, + severity: "warning", + prominent: false, + }), + ); + expect(issues).toContainEqual( + expect.objectContaining({ + title: `"pipe" left pipes open`, + severity: "warning", + prominent: false, + }), + ); + }); + + it("does not return connectivity issues", () => { + const issues = getAgentScriptIssues(buildAgent({ status: "disconnected" })); + expect(issues).not.toContainEqual( + expect.objectContaining({ + title: "Workspace agent has disconnected", + }), + ); + }); + + it("returns empty list when no scripts failed", () => { + expect( + getAgentScriptIssues( + buildAgent({ status: "connected", lifecycle_state: "ready" }), + ), + ).toEqual([]); + }); +}); + describe("getAgentHealthIssues", () => { it("returns disconnected issue for a disconnected agent", () => { expect( diff --git a/site/src/modules/workspaces/health.ts b/site/src/modules/workspaces/health.ts index ffe52f7af0..b894171809 100644 --- a/site/src/modules/workspaces/health.ts +++ b/site/src/modules/workspaces/health.ts @@ -4,7 +4,7 @@ import type { WorkspaceAgent } from "#/api/typesGenerated"; * Canonical messages for startup and shutdown script issues. * Used by the per-agent-row tooltips in AgentStatus; the * start-related entries are also shared with per-agent health - * classification in getAgentHealthIssues. + * classification in getAgentScriptIssues. */ export const agentScriptMessages = { start_error: { @@ -62,9 +62,12 @@ interface AgentHealthIssue { } /** - * Classifies all health issues for an individual agent. + * Classifies connectivity-related health issues for an individual agent. + * These issues affect the agent panel border color and warning visibility. + * Script failures are excluded from this function to prevent them from + * incorrectly suggesting agent connectivity problems. */ -export function getAgentHealthIssues( +export function getAgentConnectivityIssues( agent: WorkspaceAgent, ): AgentHealthIssue[] { const issues: AgentHealthIssue[] = []; @@ -87,6 +90,10 @@ export function getAgentHealthIssues( }); } + // Shutdown lifecycle states are treated as connectivity concerns rather than + // script concerns because the workspace is actively becoming unavailable; + // unlike startup script failures, the agent is no longer reachable once + // shutdown begins. if ( agent.lifecycle_state === "shutting_down" || agent.lifecycle_state === "shutdown_error" || @@ -100,9 +107,28 @@ export function getAgentHealthIssues( }); } - // Ignore `start_error` and `start_timeout`, as these will eventually be - // removed from agent health. Instead, figure out if a script failed to start - // by looking directly at the scripts. + if (agent.status === "connecting") { + issues.push({ + title: agentConnectionMessages.connecting.title, + detail: agentConnectionMessages.connecting.detail, + severity: "info", + prominent: false, + }); + } + + return issues; +} + +/** + * Classifies script-related health issues for an individual agent. + * These issues are shown only in the script tabs, not in the agent panel header. + */ +export function getAgentScriptIssues( + agent: WorkspaceAgent, +): AgentHealthIssue[] { + const issues: AgentHealthIssue[] = []; + + // Check for script failures directly from the scripts array. for (const script of agent.scripts) { switch (script.status) { case "timed_out": @@ -141,14 +167,15 @@ export function getAgentHealthIssues( } } - if (agent.status === "connecting") { - issues.push({ - title: agentConnectionMessages.connecting.title, - detail: agentConnectionMessages.connecting.detail, - severity: "info", - prominent: false, - }); - } - return issues; } + +/** + * Classifies all health issues for an individual agent, combining both + * connectivity and script issues. + */ +export function getAgentHealthIssues( + agent: WorkspaceAgent, +): AgentHealthIssue[] { + return [...getAgentConnectivityIssues(agent), ...getAgentScriptIssues(agent)]; +}