chore: address review nits on skipped and canceled script outcomes

Comment, message and test fixes from the automated review. The tooltip
now names the cause for a skipped or canceled script and no longer
shows a placeholder exit code for a timed out or open-pipe script.

agent/proto/agent.pb.go

@@ -529,11 +529,13 @@ const (
 	Timing_TIMED_OUT       Timing_Status = 2
 	Timing_PIPES_LEFT_OPEN Timing_Status = 3
 	// The script never ran because a prerequisite did not reach the
-	// outcome it needed. Start and end are both the moment of the skip.
+	// outcome it needed. Start and end are both the moment of the skip
+	// and exit_code is 0.
 	Timing_SKIPPED Timing_Status = 4
-	// The lifecycle execution ended before the script finished. A script
-	// that was running keeps its real interval; one that never started
-	// has start equal to end at the moment of cancellation.
+	// The workspace stopped before the script finished. A script that
+	// was running keeps its real interval and the exit code its process
+	// returned; one that never started has start equal to end at the
+	// moment of cancellation and exit_code 0.
 	Timing_CANCELED Timing_Status = 5
 )
 

agent/proto/agent.proto

@@ -349,11 +349,13 @@ message Timing {
         TIMED_OUT = 2;
         PIPES_LEFT_OPEN = 3;
         // The script never ran because a prerequisite did not reach the
-        // outcome it needed. Start and end are both the moment of the skip.
+        // outcome it needed. Start and end are both the moment of the skip
+        // and exit_code is 0.
         SKIPPED = 4;
-        // The lifecycle execution ended before the script finished. A script
-        // that was running keeps its real interval; one that never started
-        // has start equal to end at the moment of cancellation.
+        // The workspace stopped before the script finished. A script that
+        // was running keeps its real interval and the exit code its process
+        // returned; one that never started has start equal to end at the
+        // moment of cancellation and exit_code 0.
         CANCELED = 5;
     }
     Status status = 6;

coderd/agentapi/scripts.go

@@ -68,7 +68,7 @@ func (s *ScriptsAPI) ScriptCompleted(ctx context.Context, req *agentproto.Worksp
 	default:
 		// Without this the empty value reaches Postgres and fails with an
 		// enum error that does not say where it came from.
-		return nil, xerrors.Errorf("unknown script timing status %q", req.Timing.Status.String())
+		return nil, xerrors.Errorf("unknown script timing status %q: the agent is newer than coderd", req.Timing.Status.String())
 	}
 
 	//nolint:gocritic // We need permissions to write to the DB here and we are in the context of the agent.

coderd/agentapi/scripts_test.go

@@ -24,6 +24,7 @@ func TestScriptCompleted(t *testing.T) {
 		scriptID     uuid.UUID
 		timing       *agentproto.Timing
 		expectInsert bool
+		expectStatus database.WorkspaceAgentScriptTimingStatus
 		expectError  string
 	}{
 		{
@@ -36,6 +37,7 @@ func TestScriptCompleted(t *testing.T) {
 				ExitCode: 0,
 			},
 			expectInsert: true,
+			expectStatus: database.WorkspaceAgentScriptTimingStatusOk,
 		},
 		{
 			scriptID: uuid.New(),
@@ -47,6 +49,7 @@ func TestScriptCompleted(t *testing.T) {
 				ExitCode: 0,
 			},
 			expectInsert: true,
+			expectStatus: database.WorkspaceAgentScriptTimingStatusOk,
 		},
 		{
 			scriptID: uuid.New(),
@@ -58,6 +61,7 @@ func TestScriptCompleted(t *testing.T) {
 				ExitCode: 0,
 			},
 			expectInsert: true,
+			expectStatus: database.WorkspaceAgentScriptTimingStatusOk,
 		},
 		{
 			scriptID: uuid.New(),
@@ -69,6 +73,7 @@ func TestScriptCompleted(t *testing.T) {
 				ExitCode: 255,
 			},
 			expectInsert: true,
+			expectStatus: database.WorkspaceAgentScriptTimingStatusTimedOut,
 		},
 		{
 			scriptID: uuid.New(),
@@ -80,10 +85,10 @@ func TestScriptCompleted(t *testing.T) {
 				ExitCode: 1,
 			},
 			expectInsert: true,
+			expectStatus: database.WorkspaceAgentScriptTimingStatusExitFailure,
 		},
 		{
-			// A skipped script has a zero duration, which passes the
-			// start-after-end check.
+			// Equal start and end passes the start-after-end check.
 			scriptID: uuid.New(),
 			timing: func() *agentproto.Timing {
 				at := timestamppb.New(dbtime.Now())
@@ -96,9 +101,9 @@ func TestScriptCompleted(t *testing.T) {
 				}
 			}(),
 			expectInsert: true,
+			expectStatus: database.WorkspaceAgentScriptTimingStatusSkipped,
 		},
 		{
-			// A script canceled while running keeps its real interval.
 			scriptID: uuid.New(),
 			timing: &agentproto.Timing{
 				Stage:    agentproto.Timing_START,
@@ -108,9 +113,9 @@ func TestScriptCompleted(t *testing.T) {
 				ExitCode: 255,
 			},
 			expectInsert: true,
+			expectStatus: database.WorkspaceAgentScriptTimingStatusCanceled,
 		},
 		{
-			// A script canceled before it started has a zero duration.
 			scriptID: uuid.New(),
 			timing: func() *agentproto.Timing {
 				at := timestamppb.New(dbtime.Now())
@@ -123,16 +128,18 @@ func TestScriptCompleted(t *testing.T) {
 				}
 			}(),
 			expectInsert: true,
+			expectStatus: database.WorkspaceAgentScriptTimingStatusCanceled,
 		},
 		{
-			// A status this coderd does not know is refused by name
-			// before the insert.
+			// A status this coderd does not know fails before the insert.
+			// The value sits far outside the enum so a seventh status
+			// does not turn this row into a known one.
 			scriptID: uuid.New(),
 			timing: &agentproto.Timing{
 				Stage:    agentproto.Timing_START,
 				Start:    timestamppb.New(dbtime.Now()),
 				End:      timestamppb.New(dbtime.Now().Add(time.Second)),
-				Status:   agentproto.Timing_Status(6),
+				Status:   agentproto.Timing_Status(1000),
 				ExitCode: 0,
 			},
 			expectInsert: false,
@@ -209,7 +216,7 @@ func TestScriptCompleted(t *testing.T) {
 			mDB.EXPECT().InsertWorkspaceAgentScriptTimings(gomock.Any(), database.InsertWorkspaceAgentScriptTimingsParams{
 				ScriptID:  tt.scriptID,
 				Stage:     protoScriptTimingStageToDatabase(tt.timing.Stage),
-				Status:    protoScriptTimingStatusToDatabase(tt.timing.Status),
+				Status:    tt.expectStatus,
 				StartedAt: tt.timing.Start.AsTime(),
 				EndedAt:   tt.timing.End.AsTime(),
 				ExitCode:  tt.timing.ExitCode,
@@ -240,22 +247,3 @@ func protoScriptTimingStageToDatabase(stage agentproto.Timing_Stage) database.Wo
 	}
 	return dbStage
 }
-
-func protoScriptTimingStatusToDatabase(stage agentproto.Timing_Status) database.WorkspaceAgentScriptTimingStatus {
-	var dbStatus database.WorkspaceAgentScriptTimingStatus
-	switch stage {
-	case agentproto.Timing_OK:
-		dbStatus = database.WorkspaceAgentScriptTimingStatusOk
-	case agentproto.Timing_EXIT_FAILURE:
-		dbStatus = database.WorkspaceAgentScriptTimingStatusExitFailure
-	case agentproto.Timing_TIMED_OUT:
-		dbStatus = database.WorkspaceAgentScriptTimingStatusTimedOut
-	case agentproto.Timing_PIPES_LEFT_OPEN:
-		dbStatus = database.WorkspaceAgentScriptTimingStatusPipesLeftOpen
-	case agentproto.Timing_SKIPPED:
-		dbStatus = database.WorkspaceAgentScriptTimingStatusSkipped
-	case agentproto.Timing_CANCELED:
-		dbStatus = database.WorkspaceAgentScriptTimingStatusCanceled
-	}
-	return dbStatus
-}

coderd/database/migrations/000625_workspace_agent_script_timing_skipped_canceled.down.sql

@@ -1,6 +1,7 @@
--- An older dashboard throws on a status it has no legend for, so rows must
--- not keep the two values after a rollback. exit_failure with the runner's
--- "unknown" exit code is the nearest older meaning.
+-- The old type cannot hold skipped or canceled, so the cast below fails on
+-- any row that still has them. exit_failure with the runner's "unknown"
+-- exit code is the nearest older meaning, and an older dashboard also
+-- throws on a status it has no legend for.
 UPDATE workspace_agent_script_timings
 SET status = 'exit_failure', exit_code = 255
 WHERE status IN ('skipped', 'canceled');

coderd/workspacebuilds_test.go

@@ -2234,8 +2234,8 @@ func TestWorkspaceBuildTimings(t *testing.T) {
 		scripts := dbgen.WorkspaceAgentScripts(t, db, 5, database.WorkspaceAgentScript{
 			WorkspaceAgentID: agent.ID,
 		})
-		// A skipped script and a canceled one that never started
-		// both have a zero duration. The remaining three keep the defaults.
+		// Starting an hour ago puts these two rows before the default
+		// timings once sorted by StartedAt below.
 		skippedAt := dbtime.Now().Add(-time.Hour)
 		canceledAt := skippedAt.Add(time.Second)
 		agentScriptTimings := []database.WorkspaceAgentScriptTiming{

site/src/modules/workspaces/WorkspaceTiming/ScriptsChart.tsx

@@ -113,6 +113,7 @@ export const ScriptsChart: React.FC<ScriptsChartProps> = ({
 									<Tooltip>
 										<TooltipTrigger asChild>
 											<Bar
+												role="img"
 												aria-label={`${t.name}: ${legend.label}`}
 												tabIndex={0}
 												value={duration}
@@ -125,7 +126,7 @@ export const ScriptsChart: React.FC<ScriptsChartProps> = ({
 											side="bottom"
 											className="border-surface-quaternary text-content-primary"
 										>
-											{tooltipText(t)}
+											{tooltipContent(t)}
 										</TooltipContent>
 									</Tooltip>
 
@@ -140,12 +141,18 @@ export const ScriptsChart: React.FC<ScriptsChartProps> = ({
 	);
 };
 
-function tooltipText(t: ScriptTiming): React.ReactNode {
+// The runner reports 255 when it has no exit code, so a timeout or an open
+// pipe must not be shown as an exit code.
+function tooltipContent(t: ScriptTiming): React.ReactNode {
 	switch (t.status) {
+		case "timed_out":
+			return "Script timed out.";
+		case "pipes_left_open":
+			return "Script exited without closing its output pipes.";
 		case "skipped":
-			return "Script was skipped.";
+			return "Script was skipped because a prerequisite did not reach the required outcome.";
 		case "canceled":
-			return "Script was canceled.";
+			return "Script was canceled because the workspace stopped before it finished.";
 		default:
 			return (
 				<>

site/src/modules/workspaces/WorkspaceTiming/WorkspaceTimings.stories.tsx

@@ -83,7 +83,7 @@ const skippedScriptTiming = {
 	status: "skipped" as const,
 };
 
-const canceledWaitingScriptTiming = {
+const canceledNotStartedScriptTiming = {
 	...skippedScriptTiming,
 	display_name: "Post setup",
 	status: "canceled" as const,
@@ -123,7 +123,9 @@ export const SkippedScript: Story = {
 		const { user, canvas, body } = await openStartupScripts(canvasElement);
 		const bar = await canvas.findByLabelText("Install tools: skipped");
 		await user.hover(bar);
-		await body.findByText("Script was skipped.");
+		await body.findByText(
+			"Script was skipped because a prerequisite did not reach the required outcome.",
+		);
 	},
 };
 
@@ -131,7 +133,7 @@ export const CanceledScript: Story = {
 	args: {
 		agentScriptTimings: [
 			canceledRunningScriptTiming,
-			canceledWaitingScriptTiming,
+			canceledNotStartedScriptTiming,
 		],
 	},
 	play: async ({ canvasElement }) => {
@@ -139,7 +141,9 @@ export const CanceledScript: Story = {
 		await canvas.findByLabelText("Clone repo: canceled");
 		const bar = await canvas.findByLabelText("Post setup: canceled");
 		await user.hover(bar);
-		await body.findByText("Script was canceled.");
+		await body.findByText(
+			"Script was canceled because the workspace stopped before it finished.",
+		);
 	},
 };
 
@@ -155,8 +159,31 @@ export const TimedOutScript: Story = {
 		],
 	},
 	play: async ({ canvasElement }) => {
-		const { canvas } = await openStartupScripts(canvasElement);
-		await canvas.findByLabelText("Startup Script: timed out");
+		const { user, canvas, body } = await openStartupScripts(canvasElement);
+		const bar = await canvas.findByLabelText("Startup Script: timed out");
+		await user.hover(bar);
+		await body.findByText("Script timed out.");
+	},
+};
+
+export const PipesLeftOpenScript: Story = {
+	args: {
+		agentScriptTimings: [
+			{
+				...WorkspaceTimingsResponse.agent_script_timings[0],
+				status: "pipes_left_open",
+				exit_code: 255,
+			},
+			WorkspaceTimingsResponse.agent_script_timings[1],
+		],
+	},
+	play: async ({ canvasElement }) => {
+		const { user, canvas, body } = await openStartupScripts(canvasElement);
+		const bar = await canvas.findByLabelText(
+			"Startup Script: output pipes left open",
+		);
+		await user.hover(bar);
+		await body.findByText("Script exited without closing its output pipes.");
 	},
 };
 
@@ -174,7 +201,7 @@ export const MixedScriptOutcomes: Story = {
 				exit_code: 1,
 			},
 			skippedScriptTiming,
-			canceledWaitingScriptTiming,
+			canceledNotStartedScriptTiming,
 		],
 	},
 	play: async ({ canvasElement }) => {

site/src/modules/workspaces/health.test.ts

@@ -132,6 +132,18 @@ describe("getAgentScriptIssues", () => {
 		);
 	});
 
+	it("does not return issues for skipped or canceled scripts", () => {
+		const issues = getAgentScriptIssues(
+			buildAgent(MockWorkspaceAgentStartError),
+		);
+		expect(issues).not.toContainEqual(
+			expect.objectContaining({ title: expect.stringContaining("skipped") }),
+		);
+		expect(issues).not.toContainEqual(
+			expect.objectContaining({ title: expect.stringContaining("canceled") }),
+		);
+	});
+
 	it("does not return connectivity issues", () => {
 		const issues = getAgentScriptIssues(buildAgent({ status: "disconnected" }));
 		expect(issues).not.toContainEqual(

tailnet/proto/version.go

@@ -104,10 +104,10 @@ import (
 //   - A v2.14 agent runs start and stop scripts in dependency order.
 //
 // API v2.15:
-//   - Added SKIPPED and CANCELED to Timing.Status on the Agent API. A v2.15
-//     agent reports a skipped lifecycle script as a zero-duration SKIPPED
-//     timing and a canceled one as CANCELED, and writes a cancellation line
-//     to the script's log.
+//   - Added SKIPPED and CANCELED to Timing.Status on the Agent API, for a
+//     lifecycle script that never ran because of a prerequisite and one the
+//     workspace stopped before it finished. Agents do not send either value
+//     yet.
 const (
 	CurrentMajor = 2
 	CurrentMinor = 15