From 45f81a7cd5a07d435f3f3f203a3df14b599d4bff Mon Sep 17 00:00:00 2001
From: Dean Sheather <dean@deansheather.com>
Date: Thu, 10 Nov 2022 05:40:52 +1000
Subject: [PATCH] fix: prevent terraform init races (#4985)

---
 provisioner/terraform/executor.go             | 22 ++++++++++++++-----
 ...utor_test.go => executor_internal_test.go} |  1 -
 provisioner/terraform/serve.go                | 13 +++--------
 3 files changed, 20 insertions(+), 16 deletions(-)
 rename provisioner/terraform/{executor_test.go => executor_internal_test.go} (98%)

diff --git a/provisioner/terraform/executor.go b/provisioner/terraform/executor.go
index 3061f94519..b52b230459 100644
--- a/provisioner/terraform/executor.go
+++ b/provisioner/terraform/executor.go
@@ -14,16 +14,28 @@ import (
 	"strings"
 	"sync"
 
-	"golang.org/x/xerrors"
-
 	"github.com/hashicorp/go-version"
 	tfjson "github.com/hashicorp/terraform-json"
+	"golang.org/x/xerrors"
 
 	"github.com/coder/coder/provisionersdk/proto"
 )
 
+// initMut is a global mutex that protects the Terraform cache directory from
+// concurrent usage by path. Only `terraform init` commands are guarded by this
+// mutex.
+//
+// When cache path is set, we must protect against multiple calls to
+// `terraform init`.
+//
+// From the Terraform documentation:
+//
+//	Note: The plugin cache directory is not guaranteed to be concurrency
+//	safe. The provider installer's behavior in environments with multiple
+//	terraform init calls is undefined.
+var initMut = &sync.Mutex{}
+
 type executor struct {
-	initMu     sync.Locker
 	binaryPath string
 	cachePath  string
 	workdir    string
@@ -181,8 +193,8 @@ func (e executor) init(ctx, killCtx context.Context, logr logger) error {
 	//     concurrency safe. The provider installer's behavior in
 	//     environments with multiple terraform init calls is undefined.
 	if e.cachePath != "" {
-		e.initMu.Lock()
-		defer e.initMu.Unlock()
+		initMut.Lock()
+		defer initMut.Unlock()
 	}
 
 	return e.execWriteOutput(ctx, killCtx, args, e.basicEnv(), outWriter, errWriter)
diff --git a/provisioner/terraform/executor_test.go b/provisioner/terraform/executor_internal_test.go
similarity index 98%
rename from provisioner/terraform/executor_test.go
rename to provisioner/terraform/executor_internal_test.go
index 6d091947ec..e23a13e354 100644
--- a/provisioner/terraform/executor_test.go
+++ b/provisioner/terraform/executor_internal_test.go
@@ -1,4 +1,3 @@
-// nolint:testpackage
 package terraform
 
 import (
diff --git a/provisioner/terraform/serve.go b/provisioner/terraform/serve.go
index 765eef1681..3950b7d15f 100644
--- a/provisioner/terraform/serve.go
+++ b/provisioner/terraform/serve.go
@@ -3,7 +3,6 @@ package terraform
 import (
 	"context"
 	"path/filepath"
-	"sync"
 	"time"
 
 	"github.com/cli/safeexec"
@@ -100,20 +99,14 @@ func Serve(ctx context.Context, options *ServeOptions) error {
 }
 
 type server struct {
-	// initMu protects against executors running `terraform init`
-	// concurrently when cache path is set.
-	initMu sync.Mutex
-
-	binaryPath string
-	cachePath  string
-	logger     slog.Logger
-
+	binaryPath  string
+	cachePath   string
+	logger      slog.Logger
 	exitTimeout time.Duration
 }
 
 func (s *server) executor(workdir string) executor {
 	return executor{
-		initMu:     &s.initMu,
 		binaryPath: s.binaryPath,
 		cachePath:  s.cachePath,
 		workdir:    workdir,