From d8b76831ffb3a518b69443e487f87756f9aee481 Mon Sep 17 00:00:00 2001 From: "J. Scott Miller" Date: Mon, 15 Jun 2026 16:56:08 -0500 Subject: [PATCH] test(testutil): retry terraform provider cache population on transient failures (#26196) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem Tests that run real Terraform (e.g. `enterprise/coderd.TestWorkspaceTemplateParamsChange`, `provisioner/terraform.TestProvision`) intermittently fail when populating the shared provider cache. On a cache miss, `DownloadTFProviders` shells out to `terraform init` and `terraform providers mirror` against the live registry, which periodically returns transient 5xx errors from the registry/GitHub (504, 500). The cache-population helper had no application-level retry, so a single transient failure failed the whole test. Terraform's own registry client only retries each request once ("the request failed after 2 attempts"), which is insufficient for these bursts. ## Fix `runCmd` now retries on any non-zero exit using `github.com/coder/retry`, logging each failed attempt and preserving the original failure message format. Retry-all is safe here because `terraform init` and `terraform providers mirror` are idempotent: each run reconciles the existing state in the working directory. The backoff window is deliberately wide: 5 attempts with a 5s floor and 30s ceiling. `coder/retry` grows the delay by phi (~1.618) from the floor and caps it at the ceiling, so attempts start at roughly t=0s, 8s, 21s, 42s, and 72s (waits of ~8.1s, ~13.1s, ~21.2s, and 30s capped, plus command runtime). Registry/GitHub incidents typically last seconds to minutes rather than a single unlucky request, so a narrow window would only survive an isolated blip, while the early second attempt (~8s) still recovers quickly from brief ones. This is affordable because the network path runs only on a cache miss, not on every test: a populated cache short-circuits via `os.Stat` and is reused within and across runs (persisted by `.github/actions/test-cache`). The wait is therefore rarely incurred and is negligible against the 20m per-package test timeout. The only downside is a slower failure on a genuinely doomed run. This only affects the test provider-cache helper. Production provisioner code, the Windows no-op path, and the CI cache strategy are unchanged. Refs https://github.com/coder/internal/issues/1201
Investigation notes The CI cache (`~/.cache/coderv2-test`, via `.github/actions/test-cache`) is persisted across runs and keyed by a hash of a stable caller-supplied label + template file contents, so cache hits avoid the network entirely. The flake only surfaces on a cache miss (provider version bump, monthly cache reset, or new label/template), where the populating `terraform init` was the sole unprotected network call. This change closes that gap without weakening the "use real Terraform" intent of the tests.
--- 🤖 Generated with Coder Agents on behalf of @jscottmiller. --- testutil/terraform_cache.go | 51 ++++++++++++++++++++++++++++++++----- 1 file changed, 44 insertions(+), 7 deletions(-) diff --git a/testutil/terraform_cache.go b/testutil/terraform_cache.go index 1d6f27ede2..2b8baec2c2 100644 --- a/testutil/terraform_cache.go +++ b/testutil/terraform_cache.go @@ -13,8 +13,11 @@ import ( "slices" "strings" "testing" + "time" "github.com/stretchr/testify/require" + + "github.com/coder/retry" ) const ( @@ -85,17 +88,51 @@ func WriteTFCliConfig(t *testing.T, dir string) string { return cliConfigPath } +const ( + runCmdMaxAttempts = 5 + runCmdRetryFloor = 5 * time.Second + runCmdRetryCeil = 30 * time.Second +) + +// runCmd runs the given command, retrying on any non-zero exit. The provider +// cache population commands hit the network and intermittently fail with +// transient registry/GitHub 5xx errors; retrying is safe because `terraform +// init` and `terraform providers mirror` are idempotent. The backoff window is +// wide because registry incidents last seconds to minutes, and the wait is +// only incurred on a cache miss (see DownloadTFProviders). func runCmd(t *testing.T, dir string, args ...string) { t.Helper() - stdout, stderr := bytes.NewBuffer(nil), bytes.NewBuffer(nil) - cmd := exec.Command(args[0], args[1:]...) //#nosec - cmd.Dir = dir - cmd.Stdout = stdout - cmd.Stderr = stderr - if err := cmd.Run(); err != nil { - t.Fatalf("failed to run %s: %s\nstdout: %s\nstderr: %s", strings.Join(args, " "), err, stdout.String(), stderr.String()) + ctx := t.Context() + var ( + attempt int + lastErr error + lastStdout, lastStderr string + ) + for r := retry.New(runCmdRetryFloor, runCmdRetryCeil); attempt < runCmdMaxAttempts && r.Wait(ctx); attempt++ { + stdout, stderr := bytes.NewBuffer(nil), bytes.NewBuffer(nil) + // #nosec G204 - args are test-controlled (the terraform binary plus fixed + // subcommands and a cache dir path), never external input. + cmd := exec.CommandContext(ctx, args[0], args[1:]...) + cmd.Dir = dir + cmd.Stdout = stdout + cmd.Stderr = stderr + err := cmd.Run() + if err == nil { + return + } + lastErr = err + lastStdout, lastStderr = stdout.String(), stderr.String() + t.Logf("attempt %d/%d to run %s failed: %s\nstdout: %s\nstderr: %s", + attempt+1, runCmdMaxAttempts, strings.Join(args, " "), err, lastStdout, lastStderr) } + if lastErr == nil { + // The loop exited without running a command, which means r.Wait saw the + // context canceled before the first attempt. Report that, not a nil error. + t.Fatalf("failed to run %s: %v", strings.Join(args, " "), ctx.Err()) + } + t.Fatalf("failed to run %s after %d attempts: %s\nstdout: %s\nstderr: %s", + strings.Join(args, " "), attempt, lastErr, lastStdout, lastStderr) } // GetTestTFCacheDir returns a unique cache directory path based on the test name and template files.