Skip to content

fix: move defer out of retry loop in waitForJob #308

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ethanndickson merged 8 commits into from
Mar 3, 2026
Merged

fix: move defer out of retry loop in waitForJob #308

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
eb22c07
fix: move defer out of retry loop in waitForJob
blink-so[bot] Feb 27, 2026
09e3fd0
fix: address review feedback - fix shadowed variable, add tests
blink-so[bot] Feb 27, 2026
640b1de
fix: check conn.Close return values to satisfy errcheck linter
blink-so[bot] Feb 27, 2026
de6f5d7
refactor: address review feedback - simplify waitForJobOnce signature…
blink-so[bot] Feb 27, 2026
9b18ce0
fix: remove flaky concurrent connection test
blink-so[bot] Feb 27, 2026
f26f0fd
fix: remove extra blank lines (go fmt)
blink-so[bot] Feb 27, 2026
0741b79
provider: improve waitForJob retry flow and test readability
ethanndickson Mar 2, 2026
ebdd932
Merge remote-tracking branch 'origin/main' into fix/waitForJob-defer-…
ethanndickson Mar 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 44 additions & 34 deletions internal/provider/template_resource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1106,47 +1106,57 @@ func waitForJob(ctx context.Context, client *codersdk.Client, version *codersdk.
const maxRetries = 3
var jobLogs []codersdk.ProvisionerJobLog
Copy link
Member

@johnstcn johnstcn Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this variable here now?

Copy link
Contributor Author

@blinkagent blinkagent bot Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, removed it. waitForJobOnce now manages its own jobLogs internally and the caller accumulates via append(allLogs, logs...).

for retries := 0; retries < maxRetries; retries++ {
logs, closer, err := client.TemplateVersionLogsAfter(ctx, version.ID, 0)
jobLogs, done, err := waitForJobOnce(ctx, client, version, jobLogs)
Copy link
Member

@johnstcn johnstcn Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shadows variable

Copy link
Contributor Author

@blinkagent blinkagent bot Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — fixed. Changed to use explicit var done bool / var err error declarations with = assignment instead of := so jobLogs is no longer shadowed.

Also added 5 unit tests covering waitForJobOnce and waitForJob using httptest + WebSocket mocking:

  • Success, failure, still-active cases for waitForJobOnce
  • Retry with separate WS connections and log accumulation across retries for waitForJob

if err != nil {
return jobLogs, fmt.Errorf("begin streaming logs: %w", err)
return jobLogs, err
}
defer func() {
if err := closer.Close(); err != nil {
tflog.Warn(ctx, "error closing template version log stream", map[string]any{
"error": err,
})
}
}()
for {
logs, ok := <-logs
if !ok {
break
}
tflog.Info(ctx, logs.Output, map[string]interface{}{
"job_id": logs.ID,
"job_stage": logs.Stage,
"log_source": logs.Source,
"level": logs.Level,
"created_at": logs.CreatedAt,
})
if logs.Output != "" {
jobLogs = append(jobLogs, logs)
}
if done {
return jobLogs, nil
}
latestResp, err := client.TemplateVersion(ctx, version.ID)
if err != nil {
return jobLogs, err
}
return jobLogs, fmt.Errorf("provisioner job did not complete after %d retries", maxRetries)
}

func waitForJobOnce(ctx context.Context, client *codersdk.Client, version *codersdk.TemplateVersion, jobLogs []codersdk.ProvisionerJobLog) ([]codersdk.ProvisionerJobLog, bool, error) {
Copy link
Member

@johnstcn johnstcn Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to pass in jobLogs here?

Copy link
Contributor Author

@blinkagent blinkagent bot Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call — removed the parameter. waitForJobOnce now returns only its own logs and the caller appends them.

logs, closer, err := client.TemplateVersionLogsAfter(ctx, version.ID, 0)
if err != nil {
return jobLogs, false, fmt.Errorf("begin streaming logs: %w", err)
}
defer func() {
if err := closer.Close(); err != nil {
tflog.Warn(ctx, "error closing template version log stream", map[string]any{
"error": err,
})
}
if latestResp.Job.Status.Active() {
tflog.Warn(ctx, fmt.Sprintf("provisioner job still active, continuing to wait...: %s", latestResp.Job.Status))
continue
}()
for {
logs, ok := <-logs
if !ok {
break
}
if latestResp.Job.Status != codersdk.ProvisionerJobSucceeded {
return jobLogs, fmt.Errorf("provisioner job did not succeed: %s (%s)", latestResp.Job.Status, latestResp.Job.Error)
tflog.Info(ctx, logs.Output, map[string]interface{}{
"job_id": logs.ID,
"job_stage": logs.Stage,
"log_source": logs.Source,
"level": logs.Level,
"created_at": logs.CreatedAt,
})
if logs.Output != "" {
jobLogs = append(jobLogs, logs)
}
return jobLogs, nil
}
return jobLogs, fmt.Errorf("provisioner job did not complete after %d retries", maxRetries)
latestResp, err := client.TemplateVersion(ctx, version.ID)
if err != nil {
return jobLogs, false, err
}
if latestResp.Job.Status.Active() {
tflog.Warn(ctx, fmt.Sprintf("provisioner job still active, continuing to wait...: %s", latestResp.Job.Status))
return jobLogs, false, nil
}
if latestResp.Job.Status != codersdk.ProvisionerJobSucceeded {
return jobLogs, false, fmt.Errorf("provisioner job did not succeed: %s (%s)", latestResp.Job.Status, latestResp.Job.Error)
}
return jobLogs, true, nil
}

type newVersionRequest struct {
Expand Down