Code review fixes

Signed-off-by: Spike Curtis <spike@coder.com>
coder · spikecurtis · Aug 25, 2023 · Aug 23, 2023 · Aug 23, 2023 · Aug 23, 2023
commit d1a9b2a7c2032c89286a670531639fd1ac3129d1
diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go
@@ -468,10 +468,12 @@ func NewProvisionerDaemon(t testing.TB, coderAPI *coderd.API) io.Closer {
 		_ = echoServer.Close()
 		cancelFunc()
 	})
+	// seems t.TempDir() is not safe to call from a different goroutine
+	workDir := t.TempDir()
 	go func() {
 		err := echo.Serve(ctx, &provisionersdk.ServeOptions{
 			Listener:      echoServer,
-			WorkDirectory: t.TempDir(),
+			WorkDirectory: workDir,
 			Logger:        coderAPI.Logger.Named("echo").Leveled(slog.LevelDebug),
 		})
 		assert.NoError(t, err)

@@ -45,9 +45,9 @@ message AcquiredJob {
         TemplateImport template_import = 7;
         TemplateDryRun template_dry_run = 8;
     }
-	// trace_metadata is currently used for tracing information only. It allows
-	// jobs to be tied to the request that created them.
-	map<string, string> trace_metadata = 9;
+    // trace_metadata is currently used for tracing information only. It allows
+    // jobs to be tied to the request that created them.
+    map<string, string> trace_metadata = 9;
 }
 
 message FailedJob {
@@ -113,15 +113,15 @@ message UpdateJobRequest {
     string job_id = 1;
     repeated Log logs = 2;
     repeated provisioner.TemplateVariable template_variables = 4;
-	repeated provisioner.VariableValue user_variable_values = 5;
+    repeated provisioner.VariableValue user_variable_values = 5;
     bytes readme = 6;
 }
 
 message UpdateJobResponse {
     reserved 2;
 
     bool canceled = 1;
-	repeated provisioner.VariableValue variable_values = 3;
+    repeated provisioner.VariableValue variable_values = 3;
 }
 
 message CommitQuotaRequest {

@@ -82,8 +82,8 @@ message InstanceIdentityAuth {
 }
 
 message GitAuthProvider {
-	string id = 1;
-	string access_token = 2;
+    string id = 1;
+    string access_token = 2;
 }
 
 // Agent represents a running agent on the workspace.
@@ -110,15 +110,15 @@ message Agent {
         string token = 9;
         string instance_id = 10;
     }
-	int32 connection_timeout_seconds = 11;
-	string troubleshooting_url = 12;
-	string motd_file = 13;
-	// Field 14 was bool login_before_ready = 14, now removed.
-	int32 startup_script_timeout_seconds = 15;
-	string shutdown_script = 16;
-	int32 shutdown_script_timeout_seconds = 17;
+    int32 connection_timeout_seconds = 11;
+    string troubleshooting_url = 12;
+    string motd_file = 13;
+    // Field 14 was bool login_before_ready = 14, now removed.
+    int32 startup_script_timeout_seconds = 15;
+    string shutdown_script = 16;
+    int32 shutdown_script_timeout_seconds = 17;
     repeated Metadata metadata = 18;
-	string startup_script_behavior = 19;
+    string startup_script_behavior = 19;
 }
 
 enum AppSharingLevel {
@@ -207,12 +207,12 @@ message ParseRequest {
 message ParseComplete {
     string error = 1;
     repeated TemplateVariable template_variables = 2;
-	bytes readme = 3;
+    bytes readme = 3;
 }
 
 // PlanRequest asks the provisioner to plan what resources & parameters it will create
 message PlanRequest {
-	Metadata metadata = 1;
+    Metadata metadata = 1;
     repeated RichParameterValue rich_parameter_values = 2;
     repeated VariableValue variable_values = 3;
     repeated GitAuthProvider git_auth_providers = 4;
@@ -229,7 +229,7 @@ message PlanComplete {
 // ApplyRequest asks the provisioner to apply the changes.  Apply MUST be preceded by a successful plan request/response
 // in the same Session.  The plan data is not transmitted over the wire and is cached by the provisioner in the Session.
 message ApplyRequest {
-	Metadata metadata = 1;
+    Metadata metadata = 1;
 }
 
 // ApplyComplete indicates a request to apply completed.

diff --git a/provisionersdk/session.go b/provisionersdk/session.go
@@ -36,14 +36,23 @@ func (p *protoServer) Session(stream proto.DRPCProvisioner_SessionStream) error
 	}
 	sessDir := fmt.Sprintf("Session%p", s)
 	s.WorkDirectory = filepath.Join(p.opts.WorkDirectory, sessDir)
-	err := os.MkdirAll(s.WorkDirectory, 0o700)
+	// the WorkDirectory shouldn't exist, but it's possible it does from a restarted process or failed cleanup
+	err := os.RemoveAll(s.WorkDirectory)
+	if err != nil {
+		// RemoveAll returns nil if (as expected) the path doesn't exist.  So, if we hit an error, the WorkDirectory
+		// exists AND we failed to clean it up.
+		s.Logger.Error(s.Context(), "failed to pre-clean work directory", slog.Error(err))
+		return xerrors.Errorf("failed to pre-clean work directory: %w", err)
+	}
+	err = os.MkdirAll(s.WorkDirectory, 0o700)
 	if err != nil {
 		return xerrors.Errorf("create work directory %q: %w", s.WorkDirectory, err)
 	}
 	defer func() {
+		var err error
 		// Cleanup the work directory after execution.
 		for attempt := 0; attempt < 5; attempt++ {
-			err := os.RemoveAll(s.WorkDirectory)
+			err = os.RemoveAll(s.WorkDirectory)
 			if err != nil {
 				// On Windows, open files cannot be removed.
 				// When the provisioner daemon is shutting down,
@@ -54,8 +63,10 @@ func (p *protoServer) Session(stream proto.DRPCProvisioner_SessionStream) error
 				continue
 			}
 			s.Logger.Debug(s.Context(), "cleaned up work directory")
-			break
+			return
 		}
+		s.Logger.Error(s.Context(), "failed to clean up work directory after multiple attempts",
+			slog.F("path", s.WorkDirectory), slog.Error(err))
 	}()
 	req, err := stream.Recv()
 	if err != nil {
@@ -70,7 +81,7 @@ func (p *protoServer) Session(stream proto.DRPCProvisioner_SessionStream) error
 		s.logLevel = proto.LogLevel_value[strings.ToUpper(s.Config.ProvisionerLogLevel)]
 	}
 
-	err = s.ExtractArchive()
+	err = s.extractArchive()
 	if err != nil {
 		return xerrors.Errorf("extract archive: %w", err)
 	}
@@ -176,7 +187,7 @@ func (s *Session) Context() context.Context {
 	return s.stream.Context()
 }
 
-func (s *Session) ExtractArchive() error {
+func (s *Session) extractArchive() error {
 	ctx := s.Context()
 
 	s.Logger.Info(ctx, "unpacking template source archive",