chore: review feedback

Signed-off-by: Danny Kopping <dannykopping@gmail.com>
coder · dannykopping · May 14, 2025 · Apr 25, 2025 · Apr 25, 2025 · Apr 25, 2025
commit d9c906a49331da79f2ee8af982b563f448ac28b7
diff --git a/coderd/provisionerdserver/provisionerdserver.go b/coderd/provisionerdserver/provisionerdserver.go
@@ -26,6 +26,7 @@ import (
 	protobuf "google.golang.org/protobuf/proto"
 
 	"cdr.dev/slog"
+
 	"github.com/coder/coder/v2/codersdk/drpcsdk"
 
 	"github.com/coder/quartz"
@@ -1729,8 +1730,8 @@ func (s *server) CompleteJob(ctx context.Context, completed *proto.CompletedJob)
 			// Track resource replacements, if there are any.
 			orchestrator := s.PrebuildsOrchestrator.Load()
 			if resourceReplacements := completed.GetWorkspaceBuild().GetResourceReplacements(); orchestrator != nil && len(resourceReplacements) > 0 {
-				// Fire and forget.
-				go (*orchestrator).TrackResourceReplacement(context.Background(), workspace.ID, workspaceBuild.ID, resourceReplacements)
+				// Fire and forget. Bind to the lifecycle of the server so shutdowns are handled gracefully.
+				go (*orchestrator).TrackResourceReplacement(s.lifecycleCtx, workspace.ID, workspaceBuild.ID, resourceReplacements)
 			}
 		}
 

diff --git a/enterprise/coderd/prebuilds/reconcile.go b/enterprise/coderd/prebuilds/reconcile.go
@@ -3,6 +3,7 @@ package prebuilds
 import (
 	"context"
 	"database/sql"
+	"errors"
 	"fmt"
 	"math"
 	"strings"
@@ -631,7 +632,6 @@ func (c *StoreReconciler) provision(
 }
 
 func (c *StoreReconciler) TrackResourceReplacement(ctx context.Context, workspaceID, buildID uuid.UUID, replacements []*sdkproto.ResourceReplacement) {
-	// Set authorization context since this may be called in the background (i.e. with a bare context).
 	// nolint:gocritic // Necessary to query all the required data.
 	ctx = dbauthz.AsSystemRestricted(ctx)
 	// Since this may be called in a fire-and-forget fashion, we need to give up at some point.
@@ -716,8 +716,7 @@ func (c *StoreReconciler) trackResourceReplacement(ctx context.Context, workspac
 		return xerrors.Errorf("fetch template admins: %w", err)
 	}
 
-	var errs multierror.Error
-
+	var notifErr error
 	for _, templateAdmin := range templateAdmins {
 		if _, err := (*c.notifEnq.Load()).EnqueueWithData(ctx, templateAdmin.ID, notifications.TemplateWorkspaceResourceReplaced,
 			map[string]string{
@@ -735,10 +734,10 @@ func (c *StoreReconciler) trackResourceReplacement(ctx context.Context, workspac
 			// Associate this notification with all the related entities.
 			workspace.ID, workspace.OwnerID, workspace.TemplateID, templateVersion.ID, prebuildPreset.ID, workspace.OrganizationID,
 		); err != nil {
-			errs.Errors = append(errs.Errors, xerrors.Errorf("send notification to %q: %w", templateAdmin.ID.String(), err))
+			notifErr = errors.Join(xerrors.Errorf("send notification to %q: %w", templateAdmin.ID.String(), err))
 			continue
 		}
 	}
 
-	return errs.ErrorOrNil()
+	return notifErr
 }
diff --git a/provisioner/terraform/executor.go b/provisioner/terraform/executor.go
@@ -416,7 +416,7 @@ func (e *executor) parsePlan(ctx, killCtx context.Context, planfilePath string)
 // logDrift must only be called while the lock is held.
 // It will log the output of `terraform show`, which will show which resources have drifted from the known state.
 func (e *executor) logDrift(ctx, killCtx context.Context, planfilePath string, logr logSink) {
-	stdout, stdoutDone := resourceReplaceLogWriter(logr)
+	stdout, stdoutDone := resourceReplaceLogWriter(logr, e.logger)
 	stderr, stderrDone := logWriter(logr, proto.LogLevel_ERROR)
 	defer func() {
 		_ = stdout.Close()
@@ -436,9 +436,9 @@ func (e *executor) logDrift(ctx, killCtx context.Context, planfilePath string, l
 //
 // The WriteCloser must be closed by the caller to end logging, after which the returned channel will be closed to
 // indicate that logging of the written data has finished.  Failure to close the WriteCloser will leak a goroutine.
-func resourceReplaceLogWriter(sink logSink) (io.WriteCloser, <-chan any) {
+func resourceReplaceLogWriter(sink logSink, logger slog.Logger) (io.WriteCloser, <-chan struct{}) {
 	r, w := io.Pipe()
-	done := make(chan any)
+	done := make(chan struct{})
 
 	go func() {
 		defer close(done)
@@ -455,6 +455,9 @@ func resourceReplaceLogWriter(sink logSink) (io.WriteCloser, <-chan any) {
 
 			sink.ProvisionLog(level, string(line))
 		}
+		if err := scanner.Err(); err != nil {
+			logger.Error(context.Background(), "failed to read terraform log", slog.Error(err))
+		}
 	}()
 	return w, done
 }

diff --git a/provisioner/terraform/resource_replacements.go b/provisioner/terraform/resource_replacements.go
@@ -59,21 +59,22 @@ func findResourceReplacements(plan *tfjson.Plan) resourceReplacements {
 		}
 
 		// Replacements found, problem!
-		for _, p := range ch.Change.ReplacePaths {
-			var path string
-			switch p := p.(type) {
+		for _, val := range ch.Change.ReplacePaths {
+			var pathStr string
+			// Each path needs to be coerced into a string. All types except []interface{} can be coerced using fmt.Sprintf.
+			switch path := val.(type) {
 			case []interface{}:
-				segs := p
-				list := make([]string, 0, len(segs))
-				for _, s := range segs {
-					list = append(list, fmt.Sprintf("%v", s))
+				// Found a slice of paths; coerce to string and join by ".".
+				segments := make([]string, 0, len(path))
+				for _, seg := range path {
+					segments = append(segments, fmt.Sprintf("%v", seg))
 				}
-				path = strings.Join(list, ".")
+				pathStr = strings.Join(segments, ".")
 			default:
-				path = fmt.Sprintf("%v", p)
+				pathStr = fmt.Sprintf("%v", path)
 			}
 
-			replacements[ch.Address] = append(replacements[ch.Address], path)
+			replacements[ch.Address] = append(replacements[ch.Address], pathStr)
 		}
 	}