Skip to content

Commit d1a9b2a

Browse files
spikecurtisspikecurtis
committed
Code review fixes
Signed-off-by: Spike Curtis <spike@coder.com>
1 parent 83deae6 commit d1a9b2a

File tree

4 files changed

+37
-24
lines changed

4 files changed

+37
-24
lines changed

‎coderd/coderdtest/coderdtest.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -468,10 +468,12 @@ func NewProvisionerDaemon(t testing.TB, coderAPI *coderd.API) io.Closer {
468468
_ = echoServer.Close()
469469
cancelFunc()
470470
})
471+
// seems t.TempDir() is not safe to call from a different goroutine
472+
workDir := t.TempDir()
471473
go func() {
472474
err := echo.Serve(ctx, &provisionersdk.ServeOptions{
473475
Listener: echoServer,
474-
WorkDirectory: t.TempDir(),
476+
WorkDirectory: workDir,
475477
Logger: coderAPI.Logger.Named("echo").Leveled(slog.LevelDebug),
476478
})
477479
assert.NoError(t, err)

‎provisionerd/proto/provisionerd.proto

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -45,9 +45,9 @@ message AcquiredJob {
4545
TemplateImport template_import = 7;
4646
TemplateDryRun template_dry_run = 8;
4747
}
48-
// trace_metadata is currently used for tracing information only. It allows
49-
// jobs to be tied to the request that created them.
50-
map<string, string> trace_metadata = 9;
48+
// trace_metadata is currently used for tracing information only. It allows
49+
// jobs to be tied to the request that created them.
50+
map<string, string> trace_metadata = 9;
5151
}
5252

5353
message FailedJob {
@@ -113,15 +113,15 @@ message UpdateJobRequest {
113113
string job_id = 1;
114114
repeated Log logs = 2;
115115
repeated provisioner.TemplateVariable template_variables = 4;
116-
repeated provisioner.VariableValue user_variable_values = 5;
116+
repeated provisioner.VariableValue user_variable_values = 5;
117117
bytes readme = 6;
118118
}
119119

120120
message UpdateJobResponse {
121121
reserved 2;
122122

123123
bool canceled = 1;
124-
repeated provisioner.VariableValue variable_values = 3;
124+
repeated provisioner.VariableValue variable_values = 3;
125125
}
126126

127127
message CommitQuotaRequest {

‎provisionersdk/proto/provisioner.proto

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -82,8 +82,8 @@ message InstanceIdentityAuth {
8282
}
8383

8484
message GitAuthProvider {
85-
string id = 1;
86-
string access_token = 2;
85+
string id = 1;
86+
string access_token = 2;
8787
}
8888

8989
// Agent represents a running agent on the workspace.
@@ -110,15 +110,15 @@ message Agent {
110110
string token = 9;
111111
string instance_id = 10;
112112
}
113-
int32 connection_timeout_seconds = 11;
114-
string troubleshooting_url = 12;
115-
string motd_file = 13;
116-
// Field 14 was bool login_before_ready = 14, now removed.
117-
int32 startup_script_timeout_seconds = 15;
118-
string shutdown_script = 16;
119-
int32 shutdown_script_timeout_seconds = 17;
113+
int32 connection_timeout_seconds = 11;
114+
string troubleshooting_url = 12;
115+
string motd_file = 13;
116+
// Field 14 was bool login_before_ready = 14, now removed.
117+
int32 startup_script_timeout_seconds = 15;
118+
string shutdown_script = 16;
119+
int32 shutdown_script_timeout_seconds = 17;
120120
repeated Metadata metadata = 18;
121-
string startup_script_behavior = 19;
121+
string startup_script_behavior = 19;
122122
}
123123

124124
enum AppSharingLevel {
@@ -207,12 +207,12 @@ message ParseRequest {
207207
message ParseComplete {
208208
string error = 1;
209209
repeated TemplateVariable template_variables = 2;
210-
bytes readme = 3;
210+
bytes readme = 3;
211211
}
212212

213213
// PlanRequest asks the provisioner to plan what resources & parameters it will create
214214
message PlanRequest {
215-
Metadata metadata = 1;
215+
Metadata metadata = 1;
216216
repeated RichParameterValue rich_parameter_values = 2;
217217
repeated VariableValue variable_values = 3;
218218
repeated GitAuthProvider git_auth_providers = 4;
@@ -229,7 +229,7 @@ message PlanComplete {
229229
// ApplyRequest asks the provisioner to apply the changes. Apply MUST be preceded by a successful plan request/response
230230
// in the same Session. The plan data is not transmitted over the wire and is cached by the provisioner in the Session.
231231
message ApplyRequest {
232-
Metadata metadata = 1;
232+
Metadata metadata = 1;
233233
}
234234

235235
// ApplyComplete indicates a request to apply completed.

‎provisionersdk/session.go

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -36,14 +36,23 @@ func (p *protoServer) Session(stream proto.DRPCProvisioner_SessionStream) error
3636
}
3737
sessDir := fmt.Sprintf("Session%p", s)
3838
s.WorkDirectory = filepath.Join(p.opts.WorkDirectory, sessDir)
39-
err := os.MkdirAll(s.WorkDirectory, 0o700)
39+
// the WorkDirectory shouldn't exist, but it's possible it does from a restarted process or failed cleanup
40+
err := os.RemoveAll(s.WorkDirectory)
41+
if err != nil {
42+
// RemoveAll returns nil if (as expected) the path doesn't exist. So, if we hit an error, the WorkDirectory
43+
// exists AND we failed to clean it up.
44+
s.Logger.Error(s.Context(), "failed to pre-clean work directory", slog.Error(err))
45+
return xerrors.Errorf("failed to pre-clean work directory: %w", err)
46+
}
47+
err = os.MkdirAll(s.WorkDirectory, 0o700)
4048
if err != nil {
4149
return xerrors.Errorf("create work directory %q: %w", s.WorkDirectory, err)
4250
}
4351
defer func() {
52+
var err error
4453
// Cleanup the work directory after execution.
4554
for attempt := 0; attempt < 5; attempt++ {
46-
err := os.RemoveAll(s.WorkDirectory)
55+
err = os.RemoveAll(s.WorkDirectory)
4756
if err != nil {
4857
// On Windows, open files cannot be removed.
4958
// When the provisioner daemon is shutting down,
@@ -54,8 +63,10 @@ func (p *protoServer) Session(stream proto.DRPCProvisioner_SessionStream) error
5463
continue
5564
}
5665
s.Logger.Debug(s.Context(), "cleaned up work directory")
57-
break
66+
return
5867
}
68+
s.Logger.Error(s.Context(), "failed to clean up work directory after multiple attempts",
69+
slog.F("path", s.WorkDirectory), slog.Error(err))
5970
}()
6071
req, err := stream.Recv()
6172
if err != nil {
@@ -70,7 +81,7 @@ func (p *protoServer) Session(stream proto.DRPCProvisioner_SessionStream) error
7081
s.logLevel = proto.LogLevel_value[strings.ToUpper(s.Config.ProvisionerLogLevel)]
7182
}
7283

73-
err = s.ExtractArchive()
84+
err = s.extractArchive()
7485
if err != nil {
7586
return xerrors.Errorf("extract archive: %w", err)
7687
}
@@ -176,7 +187,7 @@ func (s *Session) Context() context.Context {
176187
return s.stream.Context()
177188
}
178189

179-
func (s *Session) ExtractArchive() error {
190+
func (s *Session) extractArchive() error {
180191
ctx := s.Context()
181192

182193
s.Logger.Info(ctx, "unpacking template source archive",

0 commit comments

Comments
 (0)