chore: prevent nil derefs in non-critical paths (#11411)

Emyrk · mafredri · web-flow · commit c6366e5b7394 · 2024-01-04T14:51:48.000-06:00
* chore: prevent nil derefs in non-critical paths

---------

Co-authored-by: Mathias Fredriksson &lt;mafredri@gmail.com&gt;
diff --git a/cli/cliui/agent.go b/cli/cliui/agent.go
@@ -200,12 +200,12 @@ func Agent(ctx context.Context, writer io.Writer, agentID uuid.UUID, opts AgentO
 
 			switch agent.LifecycleState {
 			case codersdk.WorkspaceAgentLifecycleReady:
-				sw.Complete(stage, agent.ReadyAt.Sub(*agent.StartedAt))
+				sw.Complete(stage, safeDuration(sw, agent.ReadyAt, agent.StartedAt))
 			case codersdk.WorkspaceAgentLifecycleStartTimeout:
 				sw.Fail(stage, 0)
 				sw.Log(time.Time{}, codersdk.LogLevelWarn, "Warning: A startup script timed out and your workspace may be incomplete.")
 			case codersdk.WorkspaceAgentLifecycleStartError:
-				sw.Fail(stage, agent.ReadyAt.Sub(*agent.StartedAt))
+				sw.Fail(stage, safeDuration(sw, agent.ReadyAt, agent.StartedAt))
 				// Use zero time (omitted) to separate these from the startup logs.
 				sw.Log(time.Time{}, codersdk.LogLevelWarn, "Warning: A startup script exited with an error and your workspace may be incomplete.")
 				sw.Log(time.Time{}, codersdk.LogLevelWarn, troubleshootingMessage(agent, "https://coder.com/docs/v2/latest/templates#startup-script-exited-with-an-error"))
@@ -221,7 +221,7 @@ func Agent(ctx context.Context, writer io.Writer, agentID uuid.UUID, opts AgentO
 				case agent.LifecycleState.ShuttingDown():
 					// We no longer know if the startup script failed or not,
 					// but we need to tell the user something.
-					sw.Complete(stage, agent.ReadyAt.Sub(*agent.StartedAt))
+					sw.Complete(stage, safeDuration(sw, agent.ReadyAt, agent.StartedAt))
 					return errAgentShuttingDown
 				}
 			}
@@ -238,13 +238,13 @@ func Agent(ctx context.Context, writer io.Writer, agentID uuid.UUID, opts AgentO
 			sw.Log(time.Now(), codersdk.LogLevelWarn, "Wait for it to reconnect or restart your workspace.")
 			sw.Log(time.Now(), codersdk.LogLevelWarn, troubleshootingMessage(agent, "https://coder.com/docs/v2/latest/templates#agent-connection-issues"))
 
-			disconnectedAt := *agent.DisconnectedAt
+			disconnectedAt := agent.DisconnectedAt
 			for agent.Status == codersdk.WorkspaceAgentDisconnected {
 				if agent, err = fetch(); err != nil {
 					return xerrors.Errorf("fetch: %w", err)
 				}
 			}
-			sw.Complete(stage, agent.LastConnectedAt.Sub(disconnectedAt))
+			sw.Complete(stage, safeDuration(sw, agent.LastConnectedAt, disconnectedAt))
 		}
 	}
 }
@@ -257,6 +257,25 @@ func troubleshootingMessage(agent codersdk.WorkspaceAgent, url string) string {
 	return m
 }
 
+// safeDuration returns a-b. If a or b is nil, it returns 0.
+// This is because we often dereference a time pointer, which can
+// cause a panic. These dereferences are used to calculate durations,
+// which are not critical, and therefor should not break things
+// when it fails.
+// A panic has been observed in a test.
+func safeDuration(sw *stageWriter, a, b *time.Time) time.Duration {
+	if a == nil || b == nil {
+		if sw != nil {
+			// Ideally the message includes which fields are <nil>, but you can
+			// use the surrounding log lines to figure that out. And passing more
+			// params makes this unwieldy.
+			sw.Log(time.Now(), codersdk.LogLevelWarn, "Warning: Failed to calculate duration from a time being <nil>.")
+		}
+		return 0
+	}
+	return a.Sub(*b)
+}
+
 type closeFunc func() error
 
 func (c closeFunc) Close() error {
