From b3613ec32975dc06cac1b8047a4e5f88bc4b475f Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 4 Jan 2024 09:37:13 -0600 Subject: [PATCH 1/3] chore: prevent nil derefs in non-critical paths --- cli/cliui/agent.go | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/cli/cliui/agent.go b/cli/cliui/agent.go index 7620efa83b1e6..534cb3f6488ae 100644 --- 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(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(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(agent.ReadyAt, agent.StartedAt)) return errAgentShuttingDown } } @@ -238,13 +238,12 @@ 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 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(agent.LastConnectedAt, agent.DisconnectedAt)) } } } @@ -257,6 +256,19 @@ 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(a, b *time.Time) time.Duration { + if a == nil || b == nil { + return 0 + } + return a.Sub(*b) +} + type closeFunc func() error func (c closeFunc) Close() error { From ae7c34c3a6817816577107268be993e717001230 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 4 Jan 2024 13:15:25 -0600 Subject: [PATCH 2/3] log line when nil time detected --- cli/cliui/agent.go | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/cli/cliui/agent.go b/cli/cliui/agent.go index 534cb3f6488ae..4bb576e334457 100644 --- 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, safeDuration(agent.ReadyAt, 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, safeDuration(agent.ReadyAt, 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, safeDuration(agent.ReadyAt, agent.StartedAt)) + sw.Complete(stage, safeDuration(sw, agent.ReadyAt, agent.StartedAt)) return errAgentShuttingDown } } @@ -243,7 +243,7 @@ func Agent(ctx context.Context, writer io.Writer, agentID uuid.UUID, opts AgentO return xerrors.Errorf("fetch: %w", err) } } - sw.Complete(stage, safeDuration(agent.LastConnectedAt, agent.DisconnectedAt)) + sw.Complete(stage, safeDuration(sw, agent.LastConnectedAt, agent.DisconnectedAt)) } } } @@ -262,8 +262,14 @@ func troubleshootingMessage(agent codersdk.WorkspaceAgent, url string) string { // which are not critical, and therefor should not break things // when it fails. // A panic has been observed in a test. -func safeDuration(a, b *time.Time) time.Duration { +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 , 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 .") + } return 0 } return a.Sub(*b) From 55258d06cc3004d1f668d0131d2520eb069686bf Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Thu, 4 Jan 2024 21:32:18 +0200 Subject: [PATCH 3/3] fix --- cli/cliui/agent.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cli/cliui/agent.go b/cli/cliui/agent.go index 4bb576e334457..ab2217095e654 100644 --- a/cli/cliui/agent.go +++ b/cli/cliui/agent.go @@ -238,12 +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 for agent.Status == codersdk.WorkspaceAgentDisconnected { if agent, err = fetch(); err != nil { return xerrors.Errorf("fetch: %w", err) } } - sw.Complete(stage, safeDuration(sw, agent.LastConnectedAt, agent.DisconnectedAt)) + sw.Complete(stage, safeDuration(sw, agent.LastConnectedAt, disconnectedAt)) } } }