From 1e1dd0d40ae0b1116d9ff4f060edbb309586ca58 Mon Sep 17 00:00:00 2001 From: Alex Leventer Date: Sun, 18 Oct 2026 13:54:38 -0700 Subject: [PATCH 1/8] Fix flaky preflight tests Fix the preflight failures caused by the tests themselves, found by classifying 258 failures across 145 failed scheduled Build runs since 2026-09-23: - TestFlyLaunchWithBuildSecrets, TestFlyWireguardCreate: the retry loop called f.Fly, so the first failed attempt failed the test, and the 10s window was shorter than a single ssh/exec attempt. Retry with FlyBestEffort and a per-attempt timeout over a longer window. - TestDeployDetach: launch's default auto-stop let the proxy stop the idle machines, and deploy leaves stopped machines stopped, so they never reached "started". Launch with --auto-stop off and wait up to 60s. - TestFlyDeploy_DeployMachinesCheck, NoServiceDeployMachinesCheck: the "Test Machine : " line is skipped once the test machine is destroyed. Assert on the "completed successfully" line instead. - App cleanup: a hung `apps destroy` failed tests that had passed and could block for 5 minutes. Give it a minute and leave stragglers to the preflight cleanup workflow. Adds FlyBestEffort, which reports failures and timeouts via ExitCode/TimedOut instead of failing the test, and guards cmdHistory with a mutex since retry goroutines can outlive EventuallyWithT. Co-Authored-By: Claude Opus 5.5 --- test/preflight/apps_v2_integration_test.go | 16 ++++--- test/preflight/fly_deploy_test.go | 8 +++- test/preflight/fly_launch_test.go | 15 +++++-- test/preflight/fly_wireguard_test.go | 13 ++++-- test/preflight/testlib/result.go | 6 +++ test/preflight/testlib/test_env.go | 52 ++++++++++++++++++---- 6 files changed, 86 insertions(+), 24 deletions(-) diff --git a/test/preflight/apps_v2_integration_test.go b/test/preflight/apps_v2_integration_test.go index 1294bc6734..41883f7a01 100644 --- a/test/preflight/apps_v2_integration_test.go +++ b/test/preflight/apps_v2_integration_test.go @@ -444,12 +444,14 @@ func testDeployDetach(t *testing.T) { f := testlib.NewTestEnvFromEnv(t) appName := f.CreateRandomAppName() - f.Fly("launch --org %s --name %s --region %s --now --internal-port 80 --image nginx --auto-confirm", f.OrgSlug(), appName, f.PrimaryRegion()) + // With auto-stop on, the proxy stops these idle machines and deploy + // leaves them stopped, so they would never reach "started". + f.Fly("launch --org %s --name %s --region %s --now --internal-port 80 --image nginx --auto-confirm --auto-stop off", f.OrgSlug(), appName, f.PrimaryRegion()) ml := f.MachinesList(appName) require.GreaterOrEqual(f, len(ml), 1) m := ml[0] - f.Fly("machine wait %s --state started --wait-timeout 15s", m.ID) + f.Fly("machine wait %s --state started --wait-timeout 60s", m.ID) res := f.Fly("deploy --detach") require.NotContains(f, res.StdOutString(), "started") @@ -457,7 +459,7 @@ func testDeployDetach(t *testing.T) { ml = f.MachinesList(appName) require.GreaterOrEqual(f, len(ml), 1) m = ml[0] - f.Fly("machine wait %s --state started --wait-timeout 15s", m.ID) + f.Fly("machine wait %s --state started --wait-timeout 60s", m.ID) res = f.Fly("deploy") require.Contains(f, res.StdOutString(), "started") @@ -467,13 +469,15 @@ func testDeployDetachBatching(t *testing.T) { f := testlib.NewTestEnvFromEnv(t) appName := f.CreateRandomAppName() - f.Fly("launch --org %s --name %s --region %s --now --internal-port 80 --image nginx --auto-confirm", f.OrgSlug(), appName, f.PrimaryRegion()) + // With auto-stop on, the proxy stops these idle machines and deploy + // leaves them stopped, so they would never reach "started". + f.Fly("launch --org %s --name %s --region %s --now --internal-port 80 --image nginx --auto-confirm --auto-stop off", f.OrgSlug(), appName, f.PrimaryRegion()) f.Fly("scale count 6 --yes") ml := f.MachinesList(appName) require.GreaterOrEqual(f, len(ml), 1) m := ml[0] - f.Fly("machine wait %s --state started --wait-timeout 15s", m.ID) + f.Fly("machine wait %s --state started --wait-timeout 60s", m.ID) res := f.Fly("deploy --detach") require.NotContains(f, res.StdOutString(), "started", false) @@ -481,7 +485,7 @@ func testDeployDetachBatching(t *testing.T) { ml = f.MachinesList(appName) require.GreaterOrEqual(f, len(ml), 1) m = ml[0] - f.Fly("machine wait %s --state started --wait-timeout 15s", m.ID) + f.Fly("machine wait %s --state started --wait-timeout 60s", m.ID) res = f.Fly("deploy") require.Contains(f, res.StdOutString(), "started", false) diff --git a/test/preflight/fly_deploy_test.go b/test/preflight/fly_deploy_test.go index dda5219ac7..f2e8e99d92 100644 --- a/test/preflight/fly_deploy_test.go +++ b/test/preflight/fly_deploy_test.go @@ -400,7 +400,9 @@ func TestFlyDeploy_DeployMachinesCheck(t *testing.T) { useShortLivedDeployToken(t, f) deployRes := f.Fly("deploy --buildkit --remote-only") output := deployRes.StdOutString() - require.Contains(f, output, "Test Machine") + // The "Test Machine : " summary is skipped once the test + // machine has been destroyed, so check for the success line instead. + require.Regexp(f, `Test machine \S+ completed successfully`, output) } func TestFlyDeploy_NoServiceDeployMachinesCheck(t *testing.T) { @@ -420,7 +422,9 @@ func TestFlyDeploy_NoServiceDeployMachinesCheck(t *testing.T) { useShortLivedDeployToken(t, f) deployRes := f.Fly("deploy --buildkit --remote-only") output := deployRes.StdOutString() - require.Contains(f, output, "Test Machine") + // The "Test Machine : " summary is skipped once the test + // machine has been destroyed, so check for the success line instead. + require.Regexp(f, `Test machine \S+ completed successfully`, output) } // TODO: This test times out after ~15 minutes in CI (hangs at deploy command) diff --git a/test/preflight/fly_launch_test.go b/test/preflight/fly_launch_test.go index 27d3e6a662..eeb41fa09c 100644 --- a/test/preflight/fly_launch_test.go +++ b/test/preflight/fly_launch_test.go @@ -308,12 +308,19 @@ func TestFlyLaunchWithBuildSecrets(t *testing.T) { RUN --mount=type=secret,id=secret1 cat /run/secrets/secret1 > /tmp/secrets.txt `) - f.Fly("launch --org %s --name %s --region %s --internal-port 80 --ha=false --now --build-secret secret1=SECRET1 --remote-only", f.OrgSlug(), appName, f.PrimaryRegion()) + f.Fly("launch --org %s --name %s --region %s --internal-port 80 --ha=false --now --build-secret secret1=SECRET1 --remote-only --auto-stop off", f.OrgSlug(), appName, f.PrimaryRegion()) + // The machine may not accept SSH connections right after launch, so a + // failed or hung attempt must be retried rather than failing the test. require.EventuallyWithT(t, func(c *assert.CollectT) { - ssh := f.Fly("ssh console -C 'cat /tmp/secrets.txt'") - assert.Equal(c, "SECRET1", ssh.StdOut().String()) - }, 10*time.Second, 1*time.Second) + // The first attempt may need to start the agent and open a WireGuard + // tunnel, so give each attempt room for that. + ssh := f.FlyBestEffort(time.Minute, "ssh console -C 'cat /tmp/secrets.txt'") + if !assert.Equal(c, 0, ssh.ExitCode(), "ssh console failed: %s", ssh.StdErrString()) { + return + } + assert.Equal(c, "SECRET1", ssh.StdOutString()) + }, 3*time.Minute, 2*time.Second) } func TestFlyLaunchBasicNodeApp(t *testing.T) { diff --git a/test/preflight/fly_wireguard_test.go b/test/preflight/fly_wireguard_test.go index 1936cc250b..1731d0e3e4 100644 --- a/test/preflight/fly_wireguard_test.go +++ b/test/preflight/fly_wireguard_test.go @@ -50,11 +50,18 @@ CMD ["sleep", "infinity"] // The backend is eventually consistent. The peer may not be immediately visible. assert.EventuallyWithT(t, func(t *assert.CollectT) { - result := f.Fly("machine exec -a %s %s 'dig +short aaaa @fdaa::3 %s._peer.internal'", appName, machineID, peerName) + // A failed or hung exec must be retried rather than failing the test. + result := f.FlyBestEffort(30*time.Second, "machine exec -a %s %s 'dig +short aaaa @fdaa::3 %s._peer.internal'", appName, machineID, peerName) + if !assert.Equal(t, 0, result.ExitCode(), "machine exec failed: %s", result.StdErrString()) { + return + } assert.Contains(t, result.StdOutString(), "fdaa:") - result = f.Fly("machine exec -a %s %s 'dig +noall +answer txt @fdaa::3 _peer.internal'", appName, machineID) + result = f.FlyBestEffort(30*time.Second, "machine exec -a %s %s 'dig +noall +answer txt @fdaa::3 _peer.internal'", appName, machineID) + if !assert.Equal(t, 0, result.ExitCode(), "machine exec failed: %s", result.StdErrString()) { + return + } assert.Contains(t, cleanupDigOutput(result.StdOutString()), peerName) - }, 10*time.Second, time.Second) + }, 2*time.Minute, 2*time.Second) }) } diff --git a/test/preflight/testlib/result.go b/test/preflight/testlib/result.go index 64e5fbb445..d7303890f2 100644 --- a/test/preflight/testlib/result.go +++ b/test/preflight/testlib/result.go @@ -20,6 +20,7 @@ type FlyctlResult struct { testIoStreams *iostreams.IOStreams stdIn, stdOut, stdErr *bytes.Buffer exitCode int + timedOut bool } func (r *FlyctlResult) CmdString() string { @@ -30,6 +31,11 @@ func (r *FlyctlResult) ExitCode() int { return r.exitCode } +// TimedOut reports whether the command was killed for exceeding its deadline. +func (r *FlyctlResult) TimedOut() bool { + return r.timedOut +} + func (r *FlyctlResult) StdOut() *bytes.Buffer { return r.stdOut } diff --git a/test/preflight/testlib/test_env.go b/test/preflight/testlib/test_env.go index a06e863139..31d43832fb 100644 --- a/test/preflight/testlib/test_env.go +++ b/test/preflight/testlib/test_env.go @@ -10,6 +10,7 @@ import ( "os/exec" "path/filepath" "strings" + "sync" "testing" "time" @@ -30,10 +31,13 @@ type FlyctlTestEnv struct { primaryRegion string otherRegions []string env map[string]string - cmdHistory []*FlyctlResult - noHistoryOnFail bool - id string - VMSize string + // historyMu guards cmdHistory, which retry goroutines that outlive an + // EventuallyWithT call can still append to. + historyMu sync.Mutex + cmdHistory []*FlyctlResult + noHistoryOnFail bool + id string + VMSize string } // Preflight flyctl commands are not supposed to take more than five minutes. @@ -213,8 +217,22 @@ func (f *FlyctlTestEnv) FlyAllowExitFailure(flyctlCmd string, vals ...interface{ return f.FlyContextAndConfig(ctx, FlyCmdConfig{NoAssertSuccessfulExit: true}, flyctlCmd, vals...) } +// FlyBestEffort runs flyctl command with the given timeout and returns the result. +// It does not fail the test because of the command's outcome: a non-zero exit +// is reported by ExitCode, and a command that exceeds the timeout is killed and +// reported by TimedOut. This makes it safe for retry loops and cleanup. +func (f *FlyctlTestEnv) FlyBestEffort(timeout time.Duration, flyctlCmd string, vals ...interface{}) *FlyctlResult { + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + + return f.FlyContextAndConfig(ctx, FlyCmdConfig{NoAssertSuccessfulExit: true, NoAssertDeadline: true}, flyctlCmd, vals...) +} + type FlyCmdConfig struct { NoAssertSuccessfulExit bool + // NoAssertDeadline reports a command that exceeds its context deadline + // through TimedOut instead of failing the test. + NoAssertDeadline bool } func (f *FlyctlTestEnv) FlyContextAndConfig(ctx context.Context, cfg FlyCmdConfig, flyctlCmd string, vals ...interface{}) *FlyctlResult { @@ -253,16 +271,24 @@ func (f *FlyctlTestEnv) FlyContextAndConfig(ctx context.Context, cfg FlyCmdConfi } err = cmd.Wait() if err != nil && ctx.Err() != nil { - f.Fatalf("command exceeded its deadline: %s [context error]: %v [stdout]: %s [stderr]: %s", res.cmdStr, ctx.Err(), res.stdOut.String(), res.stdErr.String()) - } - if err == nil { + if !cfg.NoAssertDeadline { + f.Fatalf("command exceeded its deadline: %s [context error]: %v [stdout]: %s [stderr]: %s", res.cmdStr, ctx.Err(), res.stdOut.String(), res.stdErr.String()) + } + // Record the timeout in stderr rather than logging it: this may run in a + // retry goroutine that outlives the test, where t.Logf panics. + fmt.Fprintf(res.stdErr, "\ncommand exceeded its deadline: %v", ctx.Err()) + res.exitCode = -1 + res.timedOut = true + } else if err == nil { res.exitCode = 0 } else if exitErr, ok := err.(*exec.ExitError); ok { res.exitCode = exitErr.ExitCode() } else { f.Fatalf("unexpected error waiting on command: %s [error]: %v", res.cmdStr, err) } + f.historyMu.Lock() f.cmdHistory = append(f.cmdHistory, res) + f.historyMu.Unlock() if !cfg.NoAssertSuccessfulExit { res.AssertSuccessfulExit() } @@ -282,7 +308,10 @@ func (f *FlyctlTestEnv) DebugPrintHistory() { if f.Failed() || f.noHistoryOnFail { return } - for i, r := range f.cmdHistory { + f.historyMu.Lock() + history := append([]*FlyctlResult(nil), f.cmdHistory...) + f.historyMu.Unlock() + for i, r := range history { fmt.Printf("%3d:\n", i+1) r.DebugPrintOutput() } @@ -321,7 +350,12 @@ func (f *FlyctlTestEnv) CreateRandomAppName() string { appName := randomName(f, prefix) f.Cleanup(func() { - f.FlyAllowExitFailure("apps destroy --yes %s", appName) + // A slow destroy shouldn't fail a test that already passed, or hold up + // the suite. Apps left behind are removed by the preflight cleanup workflow. + res := f.FlyBestEffort(time.Minute, "apps destroy --yes %s", appName) + if res.ExitCode() != 0 { + f.Logf("apps destroy %s failed (timed out: %t); leaving it for the preflight cleanup workflow: %s", appName, res.TimedOut(), res.StdErrString()) + } }) return appName } From 498e01d75497b36b2497008b67335c3e22613b1b Mon Sep 17 00:00:00 2001 From: Alex Leventer Date: Fri, 30 Oct 2026 18:29:13 -0700 Subject: [PATCH 2/8] Address review: log retries, tighten windows, keep destroy failures visible - Log the number, duration and exit status of every retry attempt in the build-secrets and wireguard tests, and record each command's duration on FlyctlResult so the debug history shows it too. - Shrink the retry windows. Build secrets: 30s per attempt, 90s total (was 1m / 3m). Wireguard peer lookup: 20s per attempt, 60s total (was 30s / 2m). The agent keeps the tunnel between attempts, so only the first ssh attempt pays for it. - Drop the best-effort app destroy in cleanup. A destroy that exceeds the 5-minute command deadline fails the test again, as on master, so a slow destroy stays visible instead of being logged and ignored. --- test/preflight/fly_launch_test.go | 14 ++++++++------ test/preflight/fly_wireguard_test.go | 20 ++++++++++++-------- test/preflight/testlib/result.go | 9 ++++++++- test/preflight/testlib/test_env.go | 14 ++++++-------- 4 files changed, 34 insertions(+), 23 deletions(-) diff --git a/test/preflight/fly_launch_test.go b/test/preflight/fly_launch_test.go index eeb41fa09c..ae5636b89d 100644 --- a/test/preflight/fly_launch_test.go +++ b/test/preflight/fly_launch_test.go @@ -310,17 +310,19 @@ RUN --mount=type=secret,id=secret1 cat /run/secrets/secret1 > /tmp/secrets.txt f.Fly("launch --org %s --name %s --region %s --internal-port 80 --ha=false --now --build-secret secret1=SECRET1 --remote-only --auto-stop off", f.OrgSlug(), appName, f.PrimaryRegion()) - // The machine may not accept SSH connections right after launch, so a - // failed or hung attempt must be retried rather than failing the test. + // The SSH server on a fresh machine may not be up yet, so a failed attempt + // must be retried rather than failing the test. The agent keeps the + // WireGuard tunnel between attempts, so only the first one pays for it. + attempt := 0 require.EventuallyWithT(t, func(c *assert.CollectT) { - // The first attempt may need to start the agent and open a WireGuard - // tunnel, so give each attempt room for that. - ssh := f.FlyBestEffort(time.Minute, "ssh console -C 'cat /tmp/secrets.txt'") + attempt++ + ssh := f.FlyBestEffort(30*time.Second, "ssh console -C 'cat /tmp/secrets.txt'") + t.Logf("ssh console attempt %d exited %d after %s", attempt, ssh.ExitCode(), ssh.Duration().Round(time.Millisecond)) if !assert.Equal(c, 0, ssh.ExitCode(), "ssh console failed: %s", ssh.StdErrString()) { return } assert.Equal(c, "SECRET1", ssh.StdOutString()) - }, 3*time.Minute, 2*time.Second) + }, 90*time.Second, 2*time.Second) } func TestFlyLaunchBasicNodeApp(t *testing.T) { diff --git a/test/preflight/fly_wireguard_test.go b/test/preflight/fly_wireguard_test.go index 1731d0e3e4..9a4f8f68d7 100644 --- a/test/preflight/fly_wireguard_test.go +++ b/test/preflight/fly_wireguard_test.go @@ -49,19 +49,23 @@ CMD ["sleep", "infinity"] machineID := machines[0].ID // The backend is eventually consistent. The peer may not be immediately visible. - assert.EventuallyWithT(t, func(t *assert.CollectT) { + attempt := 0 + assert.EventuallyWithT(t, func(c *assert.CollectT) { + attempt++ // A failed or hung exec must be retried rather than failing the test. - result := f.FlyBestEffort(30*time.Second, "machine exec -a %s %s 'dig +short aaaa @fdaa::3 %s._peer.internal'", appName, machineID, peerName) - if !assert.Equal(t, 0, result.ExitCode(), "machine exec failed: %s", result.StdErrString()) { + result := f.FlyBestEffort(20*time.Second, "machine exec -a %s %s 'dig +short aaaa @fdaa::3 %s._peer.internal'", appName, machineID, peerName) + t.Logf("peer AAAA lookup attempt %d exited %d after %s", attempt, result.ExitCode(), result.Duration().Round(time.Millisecond)) + if !assert.Equal(c, 0, result.ExitCode(), "machine exec failed: %s", result.StdErrString()) { return } - assert.Contains(t, result.StdOutString(), "fdaa:") + assert.Contains(c, result.StdOutString(), "fdaa:") - result = f.FlyBestEffort(30*time.Second, "machine exec -a %s %s 'dig +noall +answer txt @fdaa::3 _peer.internal'", appName, machineID) - if !assert.Equal(t, 0, result.ExitCode(), "machine exec failed: %s", result.StdErrString()) { + result = f.FlyBestEffort(20*time.Second, "machine exec -a %s %s 'dig +noall +answer txt @fdaa::3 _peer.internal'", appName, machineID) + t.Logf("peer TXT lookup attempt %d exited %d after %s", attempt, result.ExitCode(), result.Duration().Round(time.Millisecond)) + if !assert.Equal(c, 0, result.ExitCode(), "machine exec failed: %s", result.StdErrString()) { return } - assert.Contains(t, cleanupDigOutput(result.StdOutString()), peerName) - }, 2*time.Minute, 2*time.Second) + assert.Contains(c, cleanupDigOutput(result.StdOutString()), peerName) + }, time.Minute, 2*time.Second) }) } diff --git a/test/preflight/testlib/result.go b/test/preflight/testlib/result.go index d7303890f2..262ffaf388 100644 --- a/test/preflight/testlib/result.go +++ b/test/preflight/testlib/result.go @@ -7,6 +7,7 @@ import ( "bytes" "encoding/json" "fmt" + "time" "github.com/superfly/flyctl/iostreams" ) @@ -21,6 +22,7 @@ type FlyctlResult struct { stdIn, stdOut, stdErr *bytes.Buffer exitCode int timedOut bool + duration time.Duration } func (r *FlyctlResult) CmdString() string { @@ -36,6 +38,11 @@ func (r *FlyctlResult) TimedOut() bool { return r.timedOut } +// Duration is how long the command ran, from start until it exited or was killed. +func (r *FlyctlResult) Duration() time.Duration { + return r.duration +} + func (r *FlyctlResult) StdOut() *bytes.Buffer { return r.stdOut } @@ -68,5 +75,5 @@ func (r *FlyctlResult) AssertSuccessfulExit() { func (r *FlyctlResult) DebugPrintOutput() { r.t.Helper() - fmt.Printf("DBGCMD: %s\nOUTPUT:\n%s\nSTDERR:\n%s\n", r.cmdStr, r.stdOut.String(), r.stdErr.String()) + fmt.Printf("DBGCMD: %s\nEXIT: %d after %s\nOUTPUT:\n%s\nSTDERR:\n%s\n", r.cmdStr, r.exitCode, r.duration.Round(time.Millisecond), r.stdOut.String(), r.stdErr.String()) } diff --git a/test/preflight/testlib/test_env.go b/test/preflight/testlib/test_env.go index 31d43832fb..ab9ca7160d 100644 --- a/test/preflight/testlib/test_env.go +++ b/test/preflight/testlib/test_env.go @@ -265,17 +265,20 @@ func (f *FlyctlTestEnv) FlyContextAndConfig(ctx context.Context, cfg FlyCmdConfi cmd.Stdin = testIostreams.In cmd.Stdout = testIostreams.Out cmd.Stderr = testIostreams.ErrOut + start := time.Now() err = cmd.Start() if err != nil { f.Fatalf("failed to start command: %s [error]: %s", res.cmdStr, err) } err = cmd.Wait() + res.duration = time.Since(start) if err != nil && ctx.Err() != nil { if !cfg.NoAssertDeadline { f.Fatalf("command exceeded its deadline: %s [context error]: %v [stdout]: %s [stderr]: %s", res.cmdStr, ctx.Err(), res.stdOut.String(), res.stdErr.String()) } - // Record the timeout in stderr rather than logging it: this may run in a - // retry goroutine that outlives the test, where t.Logf panics. + // Record the timeout in stderr so it travels with the result. This may run + // in a retry goroutine after the test has finished, where failing the test + // would panic. fmt.Fprintf(res.stdErr, "\ncommand exceeded its deadline: %v", ctx.Err()) res.exitCode = -1 res.timedOut = true @@ -350,12 +353,7 @@ func (f *FlyctlTestEnv) CreateRandomAppName() string { appName := randomName(f, prefix) f.Cleanup(func() { - // A slow destroy shouldn't fail a test that already passed, or hold up - // the suite. Apps left behind are removed by the preflight cleanup workflow. - res := f.FlyBestEffort(time.Minute, "apps destroy --yes %s", appName) - if res.ExitCode() != 0 { - f.Logf("apps destroy %s failed (timed out: %t); leaving it for the preflight cleanup workflow: %s", appName, res.TimedOut(), res.StdErrString()) - } + f.FlyAllowExitFailure("apps destroy --yes %s", appName) }) return appName } From bed82466f8b169a9cb10b22529e2491fa6005c63 Mon Sep 17 00:00:00 2001 From: Alex Leventer Date: Fri, 30 Oct 2026 18:34:39 -0700 Subject: [PATCH 3/8] Route retry-loop commands through the collector instead of the parent test About twenty EventuallyWithT and Eventually loops in the preflight suite called f.Fly, f.MachinesList or f.VolumeList from the condition goroutine. Those helpers fail the parent test, so a transient command failure aborted the test instead of being retried, and a tick still running when the loop timed out panicked with "Fail in goroutine after Test has completed". FlyctlTestEnv.Collect(c) returns a view whose failure methods report to the assert.CollectT. Every retry loop now runs its commands through it, so a failed command only fails that tick. Command history moves behind a shared, locked pointer so the view and the parent see the same list and the struct copy stays free of the copylocks vet check. Also: - guard vl[0] / ls[0] in the volume retries, which panicked on an empty list - retry `tokens list` until the just-created app is visible to the API - retry `machine update` on the lingering "lease currently held" error - offline tests for Collect against a fake binary, run under -race --- test/preflight/fly_console_test.go | 4 +- test/preflight/fly_deploy_bluegreen_test.go | 2 +- test/preflight/fly_deploy_test.go | 19 ++--- test/preflight/fly_launch_test.go | 6 +- test/preflight/fly_machine_test.go | 15 +++- test/preflight/fly_mpg_test.go | 6 +- test/preflight/fly_postgres_test.go | 8 +- test/preflight/fly_scale_test.go | 20 ++--- test/preflight/fly_tokens_test.go | 28 +++++-- test/preflight/fly_volume_test.go | 80 +++++++++++-------- test/preflight/testlib/collect.go | 77 ++++++++++++++++++ test/preflight/testlib/collect_test.go | 86 +++++++++++++++++++++ test/preflight/testlib/test_env.go | 43 +++++++---- 13 files changed, 303 insertions(+), 91 deletions(-) create mode 100644 test/preflight/testlib/collect.go create mode 100644 test/preflight/testlib/collect_test.go diff --git a/test/preflight/fly_console_test.go b/test/preflight/fly_console_test.go index 9dbbbd5a0b..90b9d1dcc0 100644 --- a/test/preflight/fly_console_test.go +++ b/test/preflight/fly_console_test.go @@ -40,7 +40,7 @@ console_command = "/bin/echo '%s'" // Wait for machine to be started and ready require.EventuallyWithT(f, func(c *assert.CollectT) { - ml := f.MachinesList(appName) + ml := f.Collect(c).MachinesList(appName) assert.Equal(c, 1, len(ml), "should have 1 machine") if len(ml) > 0 { assert.Equal(c, "started", ml[0].State, "machine should be started") @@ -73,7 +73,7 @@ CMD ["/bin/sleep", "inf"] // All the tests above make ephemeral machines. They should be gone eventually. assert.EventuallyWithT(t, func(t *assert.CollectT) { - ml := f.MachinesList(appName) + ml := f.Collect(t).MachinesList(appName) assert.Equal(t, 1, len(ml)) }, 10*time.Second, 1*time.Second, "machines are ephemeral and eventually gone") } diff --git a/test/preflight/fly_deploy_bluegreen_test.go b/test/preflight/fly_deploy_bluegreen_test.go index e0dd17b88d..2e10623ccd 100644 --- a/test/preflight/fly_deploy_bluegreen_test.go +++ b/test/preflight/fly_deploy_bluegreen_test.go @@ -48,7 +48,7 @@ func runDetachedAppProcessMachine(f *testlib.FlyctlTestEnv, appName string) { func requireMachineCounts(f *testlib.FlyctlTestEnv, appName string, managedCount, detachedCount int) ([]*fly.Machine, []*fly.Machine) { var managedMachines, detachedMachines []*fly.Machine require.EventuallyWithT(f, func(c *assert.CollectT) { - managedMachines, detachedMachines = splitFlyLaunchMachines(f.MachinesList(appName)) + managedMachines, detachedMachines = splitFlyLaunchMachines(f.Collect(c).MachinesList(appName)) assert.Len(c, managedMachines, managedCount) assert.Len(c, detachedMachines, detachedCount) }, 2*time.Minute, 3*time.Second) diff --git a/test/preflight/fly_deploy_test.go b/test/preflight/fly_deploy_test.go index f2e8e99d92..559612cae0 100644 --- a/test/preflight/fly_deploy_test.go +++ b/test/preflight/fly_deploy_test.go @@ -81,8 +81,8 @@ func TestFlyDeployHA(t *testing.T) { result := f.Fly("volume create -a %s -r %s -s 1 data -y --json", appName, region) result.StdOutJSON(&volume) - require.Eventually(t, func() bool { - return f.FlyAllowExitFailure("volume show %s --app %s --json", volume.ID, appName).ExitCode() == 0 + require.EventuallyWithT(t, func(c *assert.CollectT) { + assert.Zero(c, f.Collect(c).FlyAllowExitFailure("volume show %s --app %s --json", volume.ID, appName).ExitCode()) }, time.Minute, 2*time.Second, "volume %s never became readable", volume.ID) } f.Fly("deploy --buildkit --remote-only") @@ -115,8 +115,8 @@ func TestFlyDeploy_AddNewMount(t *testing.T) { var volume *fly.Volume result := f.Fly("volume create -a %s -r %s -s 1 data -y --json", appName, f.PrimaryRegion()) result.StdOutJSON(&volume) - require.Eventually(t, func() bool { - return f.FlyAllowExitFailure("volume show %s --app %s --json", volume.ID, appName).ExitCode() == 0 + require.EventuallyWithT(t, func(c *assert.CollectT) { + assert.Zero(c, f.Collect(c).FlyAllowExitFailure("volume show %s --app %s --json", volume.ID, appName).ExitCode()) }, time.Minute, 2*time.Second, "volume %s never became readable", volume.ID) f.Fly("deploy --buildkit --remote-only") } @@ -189,7 +189,7 @@ ENV PREFLIGHT_TEST=true`) require.EventuallyWithT(t, func(c *assert.CollectT) { // Use FlyAllowExitFailure to handle transient WireGuard API failures (HTTP 500) - sshResult := f.FlyAllowExitFailure("ssh console -C 'printenv PREFLIGHT_TEST'") + sshResult := f.Collect(c).FlyAllowExitFailure("ssh console -C 'printenv PREFLIGHT_TEST'") if sshResult.ExitCode() != 0 { assert.Fail(c, "ssh command failed, will retry", "exit code: %d, stderr: %s", sshResult.ExitCode(), sshResult.StdErrString()) return @@ -585,13 +585,10 @@ func TestFlyDeploy_BlueGreen_StoppedMachines(t *testing.T) { for _, m := range machines { f.Fly("machine stop -a %s %s", appName, m.ID) } - require.Eventually(t, func() bool { - for _, m := range f.MachinesList(appName) { - if m.State != fly.MachineStateStopped { - return false - } + require.EventuallyWithT(t, func(c *assert.CollectT) { + for _, m := range f.Collect(c).MachinesList(appName) { + assert.Equal(c, fly.MachineStateStopped, m.State, "machine %s", m.ID) } - return true }, 30*time.Second, 2*time.Second, "timed out waiting for all machines to reach stopped state") return f, appName diff --git a/test/preflight/fly_launch_test.go b/test/preflight/fly_launch_test.go index ae5636b89d..6c6f8761a0 100644 --- a/test/preflight/fly_launch_test.go +++ b/test/preflight/fly_launch_test.go @@ -241,7 +241,7 @@ func TestFlyLaunchHA(t *testing.T) { var ml []*fly.Machine require.EventuallyWithT(t, func(c *assert.CollectT) { - ml = f.MachinesList(appName) + ml = f.Collect(c).MachinesList(appName) assert.Equal(c, 5, len(ml), "want 5 machines, which includes two standbys") }, 10*time.Second, 1*time.Second) @@ -288,12 +288,12 @@ func TestFlyLaunchSingleMount(t *testing.T) { f.Fly("launch --now --copy-config -o %s --name %s --region %s", f.OrgSlug(), appName, f.PrimaryRegion()) require.EventuallyWithT(t, func(c *assert.CollectT) { - ml := f.MachinesList(appName) + ml := f.Collect(c).MachinesList(appName) assert.Equal(c, 2, len(ml)) }, 15*time.Second, 1*time.Second, "want 2 machines, one for each process") require.EventuallyWithT(t, func(c *assert.CollectT) { - vl := f.VolumeList(appName) + vl := f.Collect(c).VolumeList(appName) assert.Equal(c, 2, len(vl)) }, 15*time.Second, 1*time.Second, "want 2 volumes, one for each process") } diff --git a/test/preflight/fly_machine_test.go b/test/preflight/fly_machine_test.go index e9ca1f94e0..6a402ab727 100644 --- a/test/preflight/fly_machine_test.go +++ b/test/preflight/fly_machine_test.go @@ -6,7 +6,9 @@ package preflight import ( "slices" "testing" + "time" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" fly "github.com/superfly/fly-go" "github.com/superfly/flyctl/test/preflight/testlib" @@ -34,7 +36,16 @@ func TestFlyMachineRun_autoStartStop(t *testing.T) { require.Nil(f, m.Config.DisableMachineAutostart) require.Equal(f, want, m.Config.Services) - f.Fly("machine update -a %s %s --autostart -y", appName, m.ID) + // The lease taken by the previous command can linger for a moment, in + // which case update fails with "lease currently held"; retry rather than + // fail the test on that. + updateMachine := func(flags string) { + require.EventuallyWithT(t, func(c *assert.CollectT) { + f.Collect(c).Fly("machine update -a %s %s %s -y", appName, m.ID, flags) + }, 30*time.Second, 2*time.Second, "machine update %s never succeeded", flags) + } + + updateMachine("--autostart") m = f.MachinesList(appName)[0] want = []fly.MachineService{{ Protocol: "tcp", @@ -49,7 +60,7 @@ func TestFlyMachineRun_autoStartStop(t *testing.T) { require.Nil(f, m.Config.DisableMachineAutostart) require.Equal(f, want, m.Config.Services) - f.Fly("machine update -a %s %s --autostart=false --autostop=false -y", appName, m.ID) + updateMachine("--autostart=false --autostop=false") m = f.MachinesList(appName)[0] want = []fly.MachineService{{ Protocol: "tcp", diff --git a/test/preflight/fly_mpg_test.go b/test/preflight/fly_mpg_test.go index 901c4fe404..c070b9f745 100644 --- a/test/preflight/fly_mpg_test.go +++ b/test/preflight/fly_mpg_test.go @@ -47,7 +47,7 @@ func TestMPG_AttachCreatesAttachmentRecord(t *testing.T) { // Wait for cluster to be ready assert.EventuallyWithT(t, func(t *assert.CollectT) { - result := f.FlyAllowExitFailure("mpg status %s --json", clusterName) + result := f.Collect(t).FlyAllowExitFailure("mpg status %s --json", clusterName) if result.ExitCode() != 0 { t.Errorf("mpg status failed: %s", result.StdErr().String()) return @@ -103,7 +103,7 @@ func TestMPG_AttachWithCustomVariableName(t *testing.T) { // Wait for cluster to be ready assert.EventuallyWithT(t, func(t *assert.CollectT) { - result := f.FlyAllowExitFailure("mpg status %s --json", clusterName) + result := f.Collect(t).FlyAllowExitFailure("mpg status %s --json", clusterName) if result.ExitCode() != 0 { return } @@ -177,7 +177,7 @@ func TestMPG_AttachFailsWhenSecretExists(t *testing.T) { // Wait for cluster to be ready assert.EventuallyWithT(t, func(t *assert.CollectT) { - result := f.FlyAllowExitFailure("mpg status %s --json", clusterName) + result := f.Collect(t).FlyAllowExitFailure("mpg status %s --json", clusterName) if result.ExitCode() != 0 { return } diff --git a/test/preflight/fly_postgres_test.go b/test/preflight/fly_postgres_test.go index b1035e7037..0893572c26 100644 --- a/test/preflight/fly_postgres_test.go +++ b/test/preflight/fly_postgres_test.go @@ -433,7 +433,7 @@ func TestPostgres_ImportSuccess(t *testing.T) { require.NoError(f, pgCreateErr, "pg create failed after 3 attempts") assert.EventuallyWithT(t, func(t *assert.CollectT) { - assertPostgresIsUp(t, f, firstAppName) + assertPostgresIsUp(t, f.Collect(t), firstAppName) }, 1*time.Minute, 10*time.Second) f.Fly( @@ -459,7 +459,7 @@ func TestPostgres_ImportSuccess(t *testing.T) { // Wait for the importer machine to be destroyed. assert.EventuallyWithT(t, func(t *assert.CollectT) { - assertMachineCount(t, f, secondAppName, 1) + assertMachineCount(t, f.Collect(t), secondAppName, 1) }, 2*time.Minute, 10*time.Second, "import machine not destroyed") } @@ -503,7 +503,7 @@ func TestPostgres_ImportFailure(t *testing.T) { require.NoError(f, pgCreateErr, "pg create failed after 3 attempts") assert.EventuallyWithT(t, func(t *assert.CollectT) { - assertPostgresIsUp(t, f, appName) + assertPostgresIsUp(t, f.Collect(t), appName) }, 1*time.Minute, 10*time.Second) result := f.FlyAllowExitFailure( @@ -515,6 +515,6 @@ func TestPostgres_ImportFailure(t *testing.T) { // Wait for the importer machine to be destroyed. assert.EventuallyWithT(t, func(t *assert.CollectT) { - assertMachineCount(t, f, appName, 1) + assertMachineCount(t, f.Collect(t), appName, 1) }, 1*time.Minute, 10*time.Second, "import machine not destroyed") } diff --git a/test/preflight/fly_scale_test.go b/test/preflight/fly_scale_test.go index 6197a424b8..215e1bafa2 100644 --- a/test/preflight/fly_scale_test.go +++ b/test/preflight/fly_scale_test.go @@ -88,7 +88,7 @@ destination = "/data" t.Logf("scale up %s to %d machines", appName, n) f.Fly("scale count -y %d", n) require.EventuallyWithT(t, func(c *assert.CollectT) { - assertMachineCount(c, f, appName, n) + assertMachineCount(c, f.Collect(c), appName, n) }, 1*time.Minute, 1*time.Second) // Ideally n, but right now we can't guarantee that. @@ -118,40 +118,40 @@ primary_region = "%s" // Extend the volume because if not found, scaling will default to 1GB. f.Fly("volume extend %s --size 4 --app %s", ml[0].Config.Mounts[0].Volume, appName) - require.Eventually(t, func() bool { - result := f.FlyAllowExitFailure("volume show %s --app %s --json", ml[0].Config.Mounts[0].Volume, appName) - if result.ExitCode() != 0 { - return false + require.EventuallyWithT(t, func(c *assert.CollectT) { + result := f.Collect(c).FlyAllowExitFailure("volume show %s --app %s --json", ml[0].Config.Mounts[0].Volume, appName) + if !assert.Zero(c, result.ExitCode(), result.StdErrString()) { + return } var volume fly.Volume result.StdOutJSON(&volume) - return volume.SizeGb == 4 + assert.Equal(c, 4, volume.SizeGb) }, time.Minute, 2*time.Second, "volume %s never reached 4 GB", ml[0].Config.Mounts[0].Volume) f.Fly("scale count -y 2") require.EventuallyWithT(t, func(c *assert.CollectT) { - ml = f.MachinesList(appName) + ml = f.Collect(c).MachinesList(appName) assert.Equal(c, 2, len(ml)) }, 10*time.Second, 2*time.Second) if f.SecondaryRegion() != "" { f.Fly("scale count -y 1 --region %s", f.SecondaryRegion()) require.EventuallyWithT(t, func(c *assert.CollectT) { - ml = f.MachinesList(appName) + ml = f.Collect(c).MachinesList(appName) assert.Equal(c, 3, len(ml)) }, 10*time.Second, 2*time.Second) } f.Fly("scale count -y 0") require.EventuallyWithT(t, func(c *assert.CollectT) { - ml = f.MachinesList(appName) + ml = f.Collect(c).MachinesList(appName) assert.Equal(c, 0, len(ml)) }, 10*time.Second, 1*time.Second) f.Fly("scale count -y 2") require.EventuallyWithT(t, func(c *assert.CollectT) { - ml = f.MachinesList(appName) + ml = f.Collect(c).MachinesList(appName) assert.Equal(c, 2, len(ml)) }, 10*time.Second, 1*time.Second) diff --git a/test/preflight/fly_tokens_test.go b/test/preflight/fly_tokens_test.go index 515759473d..b3fdec0f9e 100644 --- a/test/preflight/fly_tokens_test.go +++ b/test/preflight/fly_tokens_test.go @@ -4,10 +4,13 @@ package preflight import ( - "github.com/stretchr/testify/require" - "github.com/superfly/flyctl/test/preflight/testlib" "os" "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/superfly/flyctl/test/preflight/testlib" ) // TODO: list of things to test @@ -53,8 +56,7 @@ func TestTokensListDeterminesScopeApp(t *testing.T) { } for _, cmd_ := range commandList { - console_out := f.Fly(cmd_).StdOutString() - require.Contains(f, console_out, `Tokens for app`) + requireTokensListContains(t, f, cmd_, `Tokens for app`) } } @@ -104,10 +106,20 @@ func TestTokensListPrioritizesAppFromFlagOverToml(t *testing.T) { f.OrgSlug(), appNameB, f.PrimaryRegion(), ) // By default use the app name found in the current dir should be used ( appNameB ) - console_out := f.Fly("tokens list").StdOutString() - require.Contains(f, console_out, `Tokens for app "`+appNameB+`"`) + requireTokensListContains(t, f, "tokens list", `Tokens for app "`+appNameB+`"`) // But, the app name passed in --app should be prioritized over the toml appname ( appNameA ) - console_out = f.Fly("tokens list --app " + appNameA).StdOutString() - require.Contains(f, console_out, `Tokens for app "`+appNameA+`"`) + requireTokensListContains(t, f, "tokens list --app "+appNameA, `Tokens for app "`+appNameA+`"`) +} + +// requireTokensListContains runs a tokens list command until its output +// contains want. The apps these tests list tokens for were created moments +// earlier, and the API can briefly answer that the app "does not belong to +// selected organization" before the new app has propagated. +func requireTokensListContains(t *testing.T, f *testlib.FlyctlTestEnv, cmd, want string) { + t.Helper() + + require.EventuallyWithT(t, func(c *assert.CollectT) { + assert.Contains(c, f.Collect(c).Fly(cmd).StdOutString(), want) + }, 30*time.Second, 2*time.Second, "%s never printed %q", cmd, want) } diff --git a/test/preflight/fly_volume_test.go b/test/preflight/fly_volume_test.go index 93a38186f4..0ca0b400e9 100644 --- a/test/preflight/fly_volume_test.go +++ b/test/preflight/fly_volume_test.go @@ -60,26 +60,34 @@ primary_region = "%s" f.Fly("volume extend %s --size 4 --app %s", ml[0].Config.Mounts[0].Volume, appName) require.EventuallyWithT(f, func(c *assert.CollectT) { - vl := f.VolumeList(appName) - require.Equal(c, vl[0].SizeGb, 4) + vl := f.Collect(c).VolumeList(appName) + if assert.NotEmpty(c, vl) { + assert.Equal(c, 4, vl[0].SizeGb) + } }, 10*time.Second, 2*time.Second) f.Fly("volume extend %s --size +1 --app %s", ml[0].Config.Mounts[0].Volume, appName) require.EventuallyWithT(f, func(c *assert.CollectT) { - vl := f.VolumeList(appName) - require.Equal(c, vl[0].SizeGb, 5) + vl := f.Collect(c).VolumeList(appName) + if assert.NotEmpty(c, vl) { + assert.Equal(c, 5, vl[0].SizeGb) + } }, 10*time.Second, 2*time.Second) f.Fly("volume extend %s --size +1gb --app %s", ml[0].Config.Mounts[0].Volume, appName) require.EventuallyWithT(f, func(c *assert.CollectT) { - vl := f.VolumeList(appName) - require.Equal(c, vl[0].SizeGb, 6) + vl := f.Collect(c).VolumeList(appName) + if assert.NotEmpty(c, vl) { + assert.Equal(c, 6, vl[0].SizeGb) + } }, 10*time.Second, 2*time.Second) f.Fly("volume extend %s --size 7gb --app %s", ml[0].Config.Mounts[0].Volume, appName) require.EventuallyWithT(f, func(c *assert.CollectT) { - vl := f.VolumeList(appName) - require.Equal(c, vl[0].SizeGb, 7) + vl := f.Collect(c).VolumeList(appName) + if assert.NotEmpty(c, vl) { + assert.Equal(c, 7, vl[0].SizeGb) + } }, 10*time.Second, 2*time.Second) } @@ -94,8 +102,8 @@ func testVolumeLs(t *testing.T) { var destroyed *fly.Volume j = f.Fly("volume create test_destroy --size 1 --app %s --region %s --yes --json", appName, f.PrimaryRegion()) j.StdOutJSON(&destroyed) - require.Eventually(t, func() bool { - return f.FlyAllowExitFailure("volume show %s --app %s --json", destroyed.ID, appName).ExitCode() == 0 + require.EventuallyWithT(t, func(c *assert.CollectT) { + assert.Zero(c, f.Collect(c).FlyAllowExitFailure("volume show %s --app %s --json", destroyed.ID, appName).ExitCode()) }, time.Minute, 2*time.Second, "volume %s never became readable", destroyed.ID) // Now destroy a volume (remembering to specify the app name) @@ -103,16 +111,17 @@ func testVolumeLs(t *testing.T) { // Deleted volumes shouldn't be shown by default assert.EventuallyWithT(f, func(t *assert.CollectT) { - lsRes := f.Fly("volume ls --app %s --json", appName) + lsRes := f.Collect(t).Fly("volume ls --app %s --json", appName) var ls []*fly.Volume lsRes.StdOutJSON(&ls) - assert.Lenf(t, ls, 1, "volume %s is still visible", destroyed.ID) - assert.Equal(t, kept.ID, ls[0].ID) + if assert.Lenf(t, ls, 1, "volume %s is still visible", destroyed.ID) { + assert.Equal(t, kept.ID, ls[0].ID) + } }, 5*time.Minute, 10*time.Second) // Deleted volumes should be shown with --all. assert.EventuallyWithT(f, func(t *assert.CollectT) { - lsAllRes := f.Fly("volume ls --all --app %s --json", appName) + lsAllRes := f.Collect(t).Fly("volume ls --all --app %s --json", appName) var lsAll []*fly.Volume lsAllRes.StdOutJSON(&lsAll) @@ -135,8 +144,8 @@ func testVolumeFork(t *testing.T) { var original *fly.Volume j := f.Fly("volume create foobar --json --app %s --region %s --yes", appName, f.PrimaryRegion()) j.StdOutJSON(&original) - require.Eventually(t, func() bool { - return f.FlyAllowExitFailure("volume show %s --app %s --json", original.ID, appName).ExitCode() == 0 + require.EventuallyWithT(t, func(c *assert.CollectT) { + assert.Zero(c, f.Collect(c).FlyAllowExitFailure("volume show %s --app %s --json", original.ID, appName).ExitCode()) }, time.Minute, 2*time.Second, "volume %s never became readable", original.ID) var fork *fly.Volume @@ -154,28 +163,31 @@ func testVolumeCreateFromDestroyedVolSnapshot(tt *testing.T) { createRes := f.Fly("volume create test_destroy --size 1 --app %s --region %s --yes --json", appName, f.PrimaryRegion()) var vol *fly.Volume createRes.StdOutJSON(&vol) - require.Eventually(t, func() bool { - return f.FlyAllowExitFailure("volume show %s --app %s --json", vol.ID, appName).ExitCode() == 0 + require.EventuallyWithT(t, func(c *assert.CollectT) { + assert.Zero(c, f.Collect(c).FlyAllowExitFailure("volume show %s --app %s --json", vol.ID, appName).ExitCode()) }, time.Minute, 2*time.Second, "volume %s never became readable", vol.ID) t.Logf("Start a machine under app %s", appName) f.Fly("m run --org %s -a %s -r %s -v %s:/data --build-remote-only nginx", f.OrgSlug(), appName, f.PrimaryRegion(), vol.ID) machine := f.MachinesList(appName)[0] - require.Eventually(f, func() bool { - machines := f.MachinesList(appName) - return len(machines) == 1 && machines[0].State == "started" + require.EventuallyWithT(f, func(c *assert.CollectT) { + machines := f.Collect(c).MachinesList(appName) + if assert.Len(c, machines, 1) { + assert.Equal(c, "started", machines[0].State) + } }, 1*time.Minute, 1*time.Second, "machine %s never started", machine.ID) f.Fly("m destroy --force %s", machine.ID) - require.Eventually(f, func() bool { - return len(f.MachinesList(appName)) == 0 + require.EventuallyWithT(f, func(c *assert.CollectT) { + assert.Empty(c, f.Collect(c).MachinesList(appName)) }, 5*time.Minute, 10*time.Second, "machine %s never destroyed", machine.ID) t.Logf("machine %s is destroyed; Snapshot volume %s", machine.ID, vol.ID) f.Fly("volume snapshot create %s --app %s --json", vol.ID, appName) var snapshot *fly.VolumeSnapshot lastSnapshotStatus := "" - require.Eventually(f, func() bool { - lsRes := f.Fly("volume snapshot ls %s --json --app %s", vol.ID, appName) + require.EventuallyWithT(f, func(c *assert.CollectT) { + fc := f.Collect(c) + lsRes := fc.Fly("volume snapshot ls %s --json --app %s", vol.ID, appName) var ls []*fly.VolumeSnapshot lsRes.StdOutJSON(&ls) currentSnapshotStatus := "not found" @@ -191,11 +203,11 @@ func testVolumeCreateFromDestroyedVolSnapshot(tt *testing.T) { } } if currentSnapshotStatus != lastSnapshotStatus { - t.Logf("snapshot status: %s", currentSnapshotStatus) + fc.Logf("snapshot status: %s", currentSnapshotStatus) lastSnapshotStatus = currentSnapshotStatus } - return snapshot != nil + assert.NotNil(c, snapshot) }, 5*time.Minute, 5*time.Second, "snapshot never made it to created state") // Now destroy a volume (remembering to specify the app name) @@ -205,10 +217,11 @@ func testVolumeCreateFromDestroyedVolSnapshot(tt *testing.T) { require.EventuallyWithT(f, func(t *assert.CollectT) { var ls []*fly.Volume - j := f.Fly("volume ls --app %s --all --json", appName) + j := f.Collect(t).Fly("volume ls --app %s --all --json", appName) j.StdOutJSON(&ls) - assert.Len(t, ls, 1) - assert.Contains(t, []string{"scheduling_destroy", "pending_destroy", "destroying", "destroyed"}, ls[0].State) + if assert.Len(t, ls, 1) { + assert.Contains(t, []string{"scheduling_destroy", "pending_destroy", "destroying", "destroyed"}, ls[0].State) + } }, 5*time.Minute, 10*time.Second, "volume %s never made it to a destroy state", vol.ID) ls := f.Fly("volume snapshot ls %s --json --app %s", vol.ID, appName) @@ -224,10 +237,11 @@ func testVolumeCreateFromDestroyedVolSnapshot(tt *testing.T) { assert.EventuallyWithT(f, func(t *assert.CollectT) { var ls []*fly.Volume - j := f.Fly("volume ls --app %s --json", appName) + j := f.Collect(t).Fly("volume ls --app %s --json", appName) j.StdOutJSON(&ls) - assert.Len(t, ls, 1) - assert.Equal(t, "created", ls[0].State) + if assert.Len(t, ls, 1) { + assert.Equal(t, "created", ls[0].State) + } }, 5*time.Minute, 10*time.Second, "final volume %s never made it to created state", vol.ID) } diff --git a/test/preflight/testlib/collect.go b/test/preflight/testlib/collect.go new file mode 100644 index 0000000000..65d3fa87af --- /dev/null +++ b/test/preflight/testlib/collect.go @@ -0,0 +1,77 @@ +//go:build integration +// +build integration + +package testlib + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/assert" +) + +// Collect returns a view of the env whose failures go to c instead of the +// parent test. Use it for every flyctl command run inside a +// require.EventuallyWithT condition. +// +// EventuallyWithT runs the condition on its own goroutine and, on timeout, +// returns without waiting for a still-running tick. A command that fails +// the parent test from there either aborts the whole test on a transient +// error the retry loop should have absorbed, or panics with "Fail in +// goroutine after Test has completed" once the parent has moved on. With +// the collector, a failed command just fails that tick and the loop retries. +// +// Commands still land in the shared history. History is not printed on a +// failed tick, since failing ticks are expected; the final failure is +// reported by EventuallyWithT on the parent test as usual. +func (f *FlyctlTestEnv) Collect(c *assert.CollectT) *FlyctlTestEnv { + view := *f + view.t = &collectTB{TB: f.t, c: c} + view.noHistoryOnFail = true + + return &view +} + +// collectTB is a testing.TB whose failure methods report to an +// assert.CollectT. Everything else, such as Helper, Cleanup and TempDir, +// still goes to the parent test. Logging is written to stdout because +// the parent panics on Log calls made after the test has finished, which +// a late tick can do. +type collectTB struct { + testing.TB + c *assert.CollectT +} + +func (t *collectTB) Error(args ...any) { + t.c.Errorf("%s", fmt.Sprintln(args...)) +} + +func (t *collectTB) Errorf(format string, args ...any) { + t.c.Errorf(format, args...) +} + +func (t *collectTB) Fail() { + t.c.Errorf("failed") +} + +func (t *collectTB) FailNow() { + t.c.FailNow() +} + +func (t *collectTB) Fatal(args ...any) { + t.Error(args...) + t.c.FailNow() +} + +func (t *collectTB) Fatalf(format string, args ...any) { + t.c.Errorf(format, args...) + t.c.FailNow() +} + +func (t *collectTB) Log(args ...any) { + fmt.Print(fmt.Sprintln(args...)) +} + +func (t *collectTB) Logf(format string, args ...any) { + fmt.Printf(format+"\n", args...) +} diff --git a/test/preflight/testlib/collect_test.go b/test/preflight/testlib/collect_test.go new file mode 100644 index 0000000000..6b7cc1b727 --- /dev/null +++ b/test/preflight/testlib/collect_test.go @@ -0,0 +1,86 @@ +//go:build integration +// +build integration + +package testlib + +import ( + "fmt" + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// recordingT captures what EventuallyWithT reports without failing the +// real test. +type recordingT struct { + errors []string +} + +func (r *recordingT) Errorf(format string, args ...any) { + r.errors = append(r.errors, fmt.Sprintf(format, args...)) +} + +func (r *recordingT) FailNow() {} + +// newLocalEnv builds an env around an arbitrary binary without touching the +// platform, so the harness itself can be exercised offline. +func newLocalEnv(t *testing.T, bin string) *FlyctlTestEnv { + return &FlyctlTestEnv{ + t: t, + flyctlBin: bin, + workDir: t.TempDir(), + env: map[string]string{}, + history: &cmdHistory{}, + } +} + +func TestCollectFailedCommandOnlyFailsTheTick(t *testing.T) { + f := newLocalEnv(t, "false") + + rec := &recordingT{} + var ticks atomic.Int32 + ok := assert.EventuallyWithT(rec, func(c *assert.CollectT) { + ticks.Add(1) + f.Collect(c).Fly("anything") + }, 250*time.Millisecond, 50*time.Millisecond) + + require.False(t, ok, "a command that always fails must not satisfy the condition") + require.Greater(t, ticks.Load(), int32(1), "the failing command must be retried, not abort the loop") + require.NotEmpty(t, rec.errors) + require.Contains(t, rec.errors[0], "expected successful zero exit code") + require.False(t, t.Failed(), "the parent test must not be failed by a failing tick") +} + +func TestCollectSuccessfulCommandSatisfiesCondition(t *testing.T) { + f := newLocalEnv(t, "true") + + rec := &recordingT{} + ok := assert.EventuallyWithT(rec, func(c *assert.CollectT) { + res := f.Collect(c).Fly("anything") + assert.Zero(c, res.ExitCode()) + }, time.Second, 50*time.Millisecond) + + require.True(t, ok) + require.Empty(t, rec.errors) +} + +func TestCollectSharesCommandHistory(t *testing.T) { + f := newLocalEnv(t, "true") + + f.Fly("first") + c := new(assert.CollectT) + f.Collect(c).Fly("second") + + require.Len(t, f.history.snapshot(), 2, "commands run through the view must appear in the parent's history") +} + +func TestCollectKeepsNonFailureMethodsOnParent(t *testing.T) { + f := newLocalEnv(t, "true") + view := f.Collect(new(assert.CollectT)) + + require.Equal(t, t.Name(), view.t.Name()) + require.NotEmpty(t, view.t.TempDir()) +} diff --git a/test/preflight/testlib/test_env.go b/test/preflight/testlib/test_env.go index ab9ca7160d..bf3df3cc30 100644 --- a/test/preflight/testlib/test_env.go +++ b/test/preflight/testlib/test_env.go @@ -31,13 +31,32 @@ type FlyctlTestEnv struct { primaryRegion string otherRegions []string env map[string]string - // historyMu guards cmdHistory, which retry goroutines that outlive an - // EventuallyWithT call can still append to. - historyMu sync.Mutex - cmdHistory []*FlyctlResult - noHistoryOnFail bool - id string - VMSize string + history *cmdHistory // shared with Collect views + noHistoryOnFail bool + id string + VMSize string +} + +// cmdHistory is the list of commands a test has run. It lives behind a +// pointer so Collect views share it with the parent env, and it is locked +// because a retry goroutine that outlives an EventuallyWithT call can still +// append to it. +type cmdHistory struct { + mu sync.Mutex + results []*FlyctlResult +} + +func (h *cmdHistory) add(r *FlyctlResult) { + h.mu.Lock() + defer h.mu.Unlock() + h.results = append(h.results, r) +} + +func (h *cmdHistory) snapshot() []*FlyctlResult { + h.mu.Lock() + defer h.mu.Unlock() + + return append([]*FlyctlResult(nil), h.results...) } // Preflight flyctl commands are not supposed to take more than five minutes. @@ -153,6 +172,7 @@ func NewTestEnvFromConfig(t testing.TB, cfg TestEnvConfig) *FlyctlTestEnv { } testEnv := &FlyctlTestEnv{ id: ulid.Make().String(), + history: &cmdHistory{}, t: t, flyctlBin: flyctlBin, primaryRegion: primaryReg, @@ -289,9 +309,7 @@ func (f *FlyctlTestEnv) FlyContextAndConfig(ctx context.Context, cfg FlyCmdConfi } else { f.Fatalf("unexpected error waiting on command: %s [error]: %v", res.cmdStr, err) } - f.historyMu.Lock() - f.cmdHistory = append(f.cmdHistory, res) - f.historyMu.Unlock() + f.history.add(res) if !cfg.NoAssertSuccessfulExit { res.AssertSuccessfulExit() } @@ -311,10 +329,7 @@ func (f *FlyctlTestEnv) DebugPrintHistory() { if f.Failed() || f.noHistoryOnFail { return } - f.historyMu.Lock() - history := append([]*FlyctlResult(nil), f.cmdHistory...) - f.historyMu.Unlock() - for i, r := range history { + for i, r := range f.history.snapshot() { fmt.Printf("%3d:\n", i+1) r.DebugPrintOutput() } From 243a41892f95a416cb4f7d099f44ef912b9c2768 Mon Sep 17 00:00:00 2001 From: Alex Leventer Date: Fri, 30 Oct 2026 18:39:33 -0700 Subject: [PATCH 4/8] Log every retried command from the harness instead of the test The review asked for the number and duration of each retry attempt, here and in the other EventuallyWithT loops. The previous commit added t.Logf calls inside the build-secrets and wireguard conditions. Those run on the retry goroutine, which can outlive the test when a tick is still running at the deadline, and t.Logf panics after the test has finished, so the log line would crash the run exactly when the test was failing. Commands run through a Collect view are now logged by the harness as a numbered retry attempt with their exit status and duration, written to stdout where a late goroutine is harmless. That covers every retry loop in the suite, not just the two that logged by hand. --- test/preflight/fly_launch_test.go | 6 ++---- test/preflight/fly_wireguard_test.go | 9 +++------ test/preflight/testlib/collect.go | 8 +++++--- test/preflight/testlib/collect_test.go | 4 +++- test/preflight/testlib/test_env.go | 19 +++++++++++++++++-- 5 files changed, 30 insertions(+), 16 deletions(-) diff --git a/test/preflight/fly_launch_test.go b/test/preflight/fly_launch_test.go index 6c6f8761a0..3040d55c48 100644 --- a/test/preflight/fly_launch_test.go +++ b/test/preflight/fly_launch_test.go @@ -313,11 +313,9 @@ RUN --mount=type=secret,id=secret1 cat /run/secrets/secret1 > /tmp/secrets.txt // The SSH server on a fresh machine may not be up yet, so a failed attempt // must be retried rather than failing the test. The agent keeps the // WireGuard tunnel between attempts, so only the first one pays for it. - attempt := 0 + // Each attempt is logged by the harness with its exit status and duration. require.EventuallyWithT(t, func(c *assert.CollectT) { - attempt++ - ssh := f.FlyBestEffort(30*time.Second, "ssh console -C 'cat /tmp/secrets.txt'") - t.Logf("ssh console attempt %d exited %d after %s", attempt, ssh.ExitCode(), ssh.Duration().Round(time.Millisecond)) + ssh := f.Collect(c).FlyBestEffort(30*time.Second, "ssh console -C 'cat /tmp/secrets.txt'") if !assert.Equal(c, 0, ssh.ExitCode(), "ssh console failed: %s", ssh.StdErrString()) { return } diff --git a/test/preflight/fly_wireguard_test.go b/test/preflight/fly_wireguard_test.go index 9a4f8f68d7..d4f8e88c3e 100644 --- a/test/preflight/fly_wireguard_test.go +++ b/test/preflight/fly_wireguard_test.go @@ -49,19 +49,16 @@ CMD ["sleep", "infinity"] machineID := machines[0].ID // The backend is eventually consistent. The peer may not be immediately visible. - attempt := 0 + // Each attempt is logged by the harness with its exit status and duration. assert.EventuallyWithT(t, func(c *assert.CollectT) { - attempt++ // A failed or hung exec must be retried rather than failing the test. - result := f.FlyBestEffort(20*time.Second, "machine exec -a %s %s 'dig +short aaaa @fdaa::3 %s._peer.internal'", appName, machineID, peerName) - t.Logf("peer AAAA lookup attempt %d exited %d after %s", attempt, result.ExitCode(), result.Duration().Round(time.Millisecond)) + result := f.Collect(c).FlyBestEffort(20*time.Second, "machine exec -a %s %s 'dig +short aaaa @fdaa::3 %s._peer.internal'", appName, machineID, peerName) if !assert.Equal(c, 0, result.ExitCode(), "machine exec failed: %s", result.StdErrString()) { return } assert.Contains(c, result.StdOutString(), "fdaa:") - result = f.FlyBestEffort(20*time.Second, "machine exec -a %s %s 'dig +noall +answer txt @fdaa::3 _peer.internal'", appName, machineID) - t.Logf("peer TXT lookup attempt %d exited %d after %s", attempt, result.ExitCode(), result.Duration().Round(time.Millisecond)) + result = f.Collect(c).FlyBestEffort(20*time.Second, "machine exec -a %s %s 'dig +noall +answer txt @fdaa::3 _peer.internal'", appName, machineID) if !assert.Equal(c, 0, result.ExitCode(), "machine exec failed: %s", result.StdErrString()) { return } diff --git a/test/preflight/testlib/collect.go b/test/preflight/testlib/collect.go index 65d3fa87af..2540dae1d8 100644 --- a/test/preflight/testlib/collect.go +++ b/test/preflight/testlib/collect.go @@ -21,12 +21,14 @@ import ( // goroutine after Test has completed" once the parent has moved on. With // the collector, a failed command just fails that tick and the loop retries. // -// Commands still land in the shared history. History is not printed on a -// failed tick, since failing ticks are expected; the final failure is -// reported by EventuallyWithT on the parent test as usual. +// Commands still land in the shared history, and each one is logged with +// its exit status and duration as a numbered retry attempt. History is not +// printed on a failed tick, since failing ticks are expected; the final +// failure is reported by EventuallyWithT on the parent test as usual. func (f *FlyctlTestEnv) Collect(c *assert.CollectT) *FlyctlTestEnv { view := *f view.t = &collectTB{TB: f.t, c: c} + view.retrying = true view.noHistoryOnFail = true return &view diff --git a/test/preflight/testlib/collect_test.go b/test/preflight/testlib/collect_test.go index 6b7cc1b727..6b04cac805 100644 --- a/test/preflight/testlib/collect_test.go +++ b/test/preflight/testlib/collect_test.go @@ -73,8 +73,10 @@ func TestCollectSharesCommandHistory(t *testing.T) { f.Fly("first") c := new(assert.CollectT) f.Collect(c).Fly("second") + f.Collect(c).Fly("third") - require.Len(t, f.history.snapshot(), 2, "commands run through the view must appear in the parent's history") + require.Len(t, f.history.snapshot(), 3, "commands run through the view must appear in the parent's history") + require.Equal(t, 2, f.history.retries, "only commands run through the view count as retry attempts") } func TestCollectKeepsNonFailureMethodsOnParent(t *testing.T) { diff --git a/test/preflight/testlib/test_env.go b/test/preflight/testlib/test_env.go index bf3df3cc30..8979f98602 100644 --- a/test/preflight/testlib/test_env.go +++ b/test/preflight/testlib/test_env.go @@ -32,6 +32,7 @@ type FlyctlTestEnv struct { otherRegions []string env map[string]string history *cmdHistory // shared with Collect views + retrying bool // set on Collect views: log each command as a retry attempt noHistoryOnFail bool id string VMSize string @@ -44,12 +45,20 @@ type FlyctlTestEnv struct { type cmdHistory struct { mu sync.Mutex results []*FlyctlResult + retries int // commands run through Collect views, across all retry loops } -func (h *cmdHistory) add(r *FlyctlResult) { +// add records r and, for a retry attempt, returns its number; 0 otherwise. +func (h *cmdHistory) add(r *FlyctlResult, retry bool) int { h.mu.Lock() defer h.mu.Unlock() h.results = append(h.results, r) + if !retry { + return 0 + } + h.retries++ + + return h.retries } func (h *cmdHistory) snapshot() []*FlyctlResult { @@ -309,7 +318,13 @@ func (f *FlyctlTestEnv) FlyContextAndConfig(ctx context.Context, cfg FlyCmdConfi } else { f.Fatalf("unexpected error waiting on command: %s [error]: %v", res.cmdStr, err) } - f.history.add(res) + if retryNo := f.history.add(res, f.retrying); retryNo > 0 { + // Every command inside a retry loop is logged with its exit status and + // duration, so slow or repeatedly failing retries are visible. Written to + // stdout rather than t.Logf: this runs on the retry goroutine, which can + // outlive the test, and t.Logf panics after the test has finished. + fmt.Printf(" retry %d: %s exited %d after %s\n", retryNo, res.argsStr, res.exitCode, res.duration.Round(time.Millisecond)) + } if !cfg.NoAssertSuccessfulExit { res.AssertSuccessfulExit() } From 38eae7383b85cfb308c2bbfe5b84a053d4daf42d Mon Sep 17 00:00:00 2001 From: Alex Leventer Date: Fri, 30 Oct 2026 18:43:04 -0700 Subject: [PATCH 5/8] Remove stale test-peer WireGuard peers in the preflight cleanup job TestFlyWireguardCreate names its peer test-peer- and removes it in a defer, which never runs when the package hits the go test timeout. Dozens of peers from August onward are still in the org's _peer.internal TXT record. The cleanup job now removes test peers whose ULID timestamp is older than 30 minutes, matching the app cleanup threshold. --- scripts/clean-up-preflight-apps/main.go | 37 +++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/scripts/clean-up-preflight-apps/main.go b/scripts/clean-up-preflight-apps/main.go index bfcb7ea294..a9ed7d573b 100644 --- a/scripts/clean-up-preflight-apps/main.go +++ b/scripts/clean-up-preflight-apps/main.go @@ -11,6 +11,7 @@ import ( "time" "github.com/google/shlex" + "github.com/oklog/ulid/v2" fly "github.com/superfly/fly-go" "github.com/superfly/flyctl/gql" "github.com/superfly/flyctl/internal/flyutil" @@ -82,6 +83,42 @@ func run() error { } } + return removeStalePeers(ctx, apiClient, os.Getenv("FLY_PREFLIGHT_TEST_FLY_ORG")) +} + +// testPeerPrefix is the name prefix TestFlyWireguardCreate gives its peers, +// followed by the test env ULID, whose timestamp is the peer's creation time. +const testPeerPrefix = "test-peer-" + +// removeStalePeers removes WireGuard peers left behind by preflight runs that +// were killed before their cleanup ran. The API has no creation time for +// peers, so it is read from the ULID in the name. +func removeStalePeers(ctx context.Context, apiClient *fly.Client, orgSlug string) error { + org, err := apiClient.GetOrganizationBySlug(ctx, orgSlug) + if err != nil { + return err + } + peers, err := apiClient.GetWireGuardPeers(ctx, orgSlug) + if err != nil { + return err + } + for _, peer := range peers { + if !strings.HasPrefix(peer.Name, testPeerPrefix) { + continue + } + id, err := ulid.ParseStrict(strings.TrimPrefix(peer.Name, testPeerPrefix)) + if err != nil { + fmt.Fprintf(os.Stderr, "skipping peer %s: name does not end in a ULID: %v\n", peer.Name, err) + continue + } + if time.Since(ulid.Time(id.Time())) < 30*time.Minute { + continue + } + fmt.Fprintf(os.Stderr, "removing stale wireguard peer %s\n", peer.Name) + if err := apiClient.RemoveWireGuardPeer(ctx, org.ID, peer.Name); err != nil { + return fmt.Errorf("removing peer %s: %w", peer.Name, err) + } + } return nil } From 9ee9b04337f568d4fd37e70f09c8bc526e7df383 Mon Sep 17 00:00:00 2001 From: Alex Leventer Date: Fri, 30 Oct 2026 19:27:21 -0700 Subject: [PATCH 6/8] Satisfy nlreturn in the preflight cleanup script --- scripts/clean-up-preflight-apps/main.go | 1 + 1 file changed, 1 insertion(+) diff --git a/scripts/clean-up-preflight-apps/main.go b/scripts/clean-up-preflight-apps/main.go index a9ed7d573b..98236e3df0 100644 --- a/scripts/clean-up-preflight-apps/main.go +++ b/scripts/clean-up-preflight-apps/main.go @@ -119,6 +119,7 @@ func removeStalePeers(ctx context.Context, apiClient *fly.Client, orgSlug string return fmt.Errorf("removing peer %s: %w", peer.Name, err) } } + return nil } From 4e2f69a5e6ce1c862dacd1dcd052d4e24acc6057 Mon Sep 17 00:00:00 2001 From: Alex Leventer Date: Fri, 30 Oct 2026 19:42:39 -0700 Subject: [PATCH 7/8] Run the harness tests in CI and tighten the remaining retry loops Review follow-ups: - The offline testlib tests carry the integration tag and no preflight group matched ^TestCollect, so they never ran in CI. Add a "harness" group that runs them under -race. - The cleanup job's peer sweep stopped at the first RemoveWireGuardPeer error and failed the job; log and continue like the app loop does. - TestFlyDeploy_Dockerfile still ran ssh console with the 5-minute deadline inside a 30s window, the cold-tunnel hang fixed elsewhere in this PR. Use FlyBestEffort with a 30s attempt and a 90s window. - Give the wireguard lookup loop room for two full attempts of both 20s execs (90s window). - The "all machines stopped" loop passed vacuously on an empty machines list; require a non-empty list first. - Number retries per command line rather than across the whole test, so "retry 3" means the third attempt of that loop. - Widen the failing-tick harness test from 250ms to 2s so two process spawns are never a timing assumption. --- .github/workflows/preflight.yml | 1 + scripts/clean-up-preflight-apps/main.go | 4 +++- scripts/preflight.sh | 9 ++++++++- test/preflight/fly_deploy_test.go | 13 +++++++++---- test/preflight/fly_wireguard_test.go | 2 +- test/preflight/testlib/collect_test.go | 15 +++++++++++++-- test/preflight/testlib/test_env.go | 13 +++++++++---- 7 files changed, 44 insertions(+), 13 deletions(-) diff --git a/.github/workflows/preflight.yml b/.github/workflows/preflight.yml index 5ed2029451..c8a4239ccd 100644 --- a/.github/workflows/preflight.yml +++ b/.github/workflows/preflight.yml @@ -36,6 +36,7 @@ jobs: - tokens - wireguard - misc + - harness steps: - uses: actions/checkout@v7.0.1 - uses: actions/setup-go@v7 diff --git a/scripts/clean-up-preflight-apps/main.go b/scripts/clean-up-preflight-apps/main.go index 98236e3df0..1b32c8cf70 100644 --- a/scripts/clean-up-preflight-apps/main.go +++ b/scripts/clean-up-preflight-apps/main.go @@ -116,7 +116,9 @@ func removeStalePeers(ctx context.Context, apiClient *fly.Client, orgSlug string } fmt.Fprintf(os.Stderr, "removing stale wireguard peer %s\n", peer.Name) if err := apiClient.RemoveWireGuardPeer(ctx, org.ID, peer.Name); err != nil { - return fmt.Errorf("removing peer %s: %w", peer.Name, err) + // The test's own cleanup or a parallel run may have removed it first. + // Keep sweeping so one peer never blocks the rest. + fmt.Fprintf(os.Stderr, "removing peer %s failed; continuing cleanup: %v\n", peer.Name, err) } } diff --git a/scripts/preflight.sh b/scripts/preflight.sh index dd68443bb3..4cf1ffd620 100755 --- a/scripts/preflight.sh +++ b/scripts/preflight.sh @@ -109,14 +109,21 @@ if [[ -n "$group" ]]; then misc) test_pattern="^Test(ErrOutput|ImageLabel|NoPublicIP)" ;; + harness) + # Offline tests of the testlib harness itself against a fake binary. + test_pattern="^TestCollect" + ;; *) echo "Unknown test group: $group" - echo "Available groups: apps, deploy, deploy-detach, deploy-fixtures, bluegreen, deploy-machine-checks, launch, scale, volume, console, logs, machine, postgres, postgres-flex-failover, tokens, wireguard, misc" + echo "Available groups: apps, deploy, deploy-detach, deploy-fixtures, bluegreen, deploy-machine-checks, launch, scale, volume, console, logs, machine, postgres, postgres-flex-failover, tokens, wireguard, misc, harness" exit 1 ;; esac go_test_args=(-tags=integration -v -timeout=15m) + if [[ "$group" == "harness" ]]; then + go_test_args+=(-race) + fi if [[ -n "$test_opts" ]]; then go_test_args+=("$test_opts") fi diff --git a/test/preflight/fly_deploy_test.go b/test/preflight/fly_deploy_test.go index 559612cae0..673ba0f461 100644 --- a/test/preflight/fly_deploy_test.go +++ b/test/preflight/fly_deploy_test.go @@ -188,14 +188,15 @@ ENV PREFLIGHT_TEST=true`) f.Fly("launch --org %s --name %s --region %s --internal-port 80 --ha=false --now", f.OrgSlug(), appName, f.PrimaryRegion()) require.EventuallyWithT(t, func(c *assert.CollectT) { - // Use FlyAllowExitFailure to handle transient WireGuard API failures (HTTP 500) - sshResult := f.Collect(c).FlyAllowExitFailure("ssh console -C 'printenv PREFLIGHT_TEST'") + // A failed attempt (transient WireGuard API 500, SSH server not up yet) + // is retried, and a hung one is killed so the loop gets another try. + sshResult := f.Collect(c).FlyBestEffort(30*time.Second, "ssh console -C 'printenv PREFLIGHT_TEST'") if sshResult.ExitCode() != 0 { assert.Fail(c, "ssh command failed, will retry", "exit code: %d, stderr: %s", sshResult.ExitCode(), sshResult.StdErrString()) return } assert.Equal(c, "true", strings.TrimSpace(sshResult.StdOutString()), "expected PREFLIGHT_TEST env var to be set in machine") - }, 30*time.Second, 2*time.Second) + }, 90*time.Second, 2*time.Second) } // TestFlyDeploy_SlowBind_NoFalseListenWarning is the regression test for @@ -586,7 +587,11 @@ func TestFlyDeploy_BlueGreen_StoppedMachines(t *testing.T) { f.Fly("machine stop -a %s %s", appName, m.ID) } require.EventuallyWithT(t, func(c *assert.CollectT) { - for _, m := range f.Collect(c).MachinesList(appName) { + ml := f.Collect(c).MachinesList(appName) + if !assert.NotEmpty(c, ml, "machines list came back empty") { + return + } + for _, m := range ml { assert.Equal(c, fly.MachineStateStopped, m.State, "machine %s", m.ID) } }, 30*time.Second, 2*time.Second, "timed out waiting for all machines to reach stopped state") diff --git a/test/preflight/fly_wireguard_test.go b/test/preflight/fly_wireguard_test.go index d4f8e88c3e..f61d2d0b35 100644 --- a/test/preflight/fly_wireguard_test.go +++ b/test/preflight/fly_wireguard_test.go @@ -63,6 +63,6 @@ CMD ["sleep", "infinity"] return } assert.Contains(c, cleanupDigOutput(result.StdOutString()), peerName) - }, time.Minute, 2*time.Second) + }, 90*time.Second, 2*time.Second) // room for two full attempts of both 20s lookups }) } diff --git a/test/preflight/testlib/collect_test.go b/test/preflight/testlib/collect_test.go index 6b04cac805..74bd84d115 100644 --- a/test/preflight/testlib/collect_test.go +++ b/test/preflight/testlib/collect_test.go @@ -45,7 +45,7 @@ func TestCollectFailedCommandOnlyFailsTheTick(t *testing.T) { ok := assert.EventuallyWithT(rec, func(c *assert.CollectT) { ticks.Add(1) f.Collect(c).Fly("anything") - }, 250*time.Millisecond, 50*time.Millisecond) + }, 2*time.Second, 50*time.Millisecond) require.False(t, ok, "a command that always fails must not satisfy the condition") require.Greater(t, ticks.Load(), int32(1), "the failing command must be retried, not abort the loop") @@ -76,7 +76,7 @@ func TestCollectSharesCommandHistory(t *testing.T) { f.Collect(c).Fly("third") require.Len(t, f.history.snapshot(), 3, "commands run through the view must appear in the parent's history") - require.Equal(t, 2, f.history.retries, "only commands run through the view count as retry attempts") + require.Equal(t, map[string]int{"second": 1, "third": 1}, f.history.retries, "only commands run through the view count as retry attempts") } func TestCollectKeepsNonFailureMethodsOnParent(t *testing.T) { @@ -86,3 +86,14 @@ func TestCollectKeepsNonFailureMethodsOnParent(t *testing.T) { require.Equal(t, t.Name(), view.t.Name()) require.NotEmpty(t, view.t.TempDir()) } + +func TestCollectNumbersRetriesPerCommand(t *testing.T) { + f := newLocalEnv(t, "true") + c := new(assert.CollectT) + + f.Collect(c).Fly("first") + f.Collect(c).Fly("first") + f.Collect(c).Fly("second") + + require.Equal(t, map[string]int{"first": 2, "second": 1}, f.history.retries, "each command line gets its own attempt count") +} diff --git a/test/preflight/testlib/test_env.go b/test/preflight/testlib/test_env.go index 8979f98602..571ed77d88 100644 --- a/test/preflight/testlib/test_env.go +++ b/test/preflight/testlib/test_env.go @@ -45,10 +45,12 @@ type FlyctlTestEnv struct { type cmdHistory struct { mu sync.Mutex results []*FlyctlResult - retries int // commands run through Collect views, across all retry loops + retries map[string]int // attempts per command line run through Collect views } -// add records r and, for a retry attempt, returns its number; 0 otherwise. +// add records r and, for a retry attempt, returns how many times this exact +// command line has been retried so far; 0 otherwise. Counting per command line +// gives each retry loop its own attempt numbers. func (h *cmdHistory) add(r *FlyctlResult, retry bool) int { h.mu.Lock() defer h.mu.Unlock() @@ -56,9 +58,12 @@ func (h *cmdHistory) add(r *FlyctlResult, retry bool) int { if !retry { return 0 } - h.retries++ + if h.retries == nil { + h.retries = map[string]int{} + } + h.retries[r.argsStr]++ - return h.retries + return h.retries[r.argsStr] } func (h *cmdHistory) snapshot() []*FlyctlResult { From 609cd12490c17493c3f8808b77f8ea4418c9045f Mon Sep 17 00:00:00 2001 From: Alex Leventer Date: Fri, 30 Oct 2026 20:06:30 -0700 Subject: [PATCH 8/8] Diagnose stopped machines in TestDeployDetach and retry reads in the port test Two pre-existing flakes showed up on this branch's preflight run. TestDeployDetach/Batching failed with `machine wait` reporting the machine "currently stopped", 60s after `scale count 6`, on an app launched with --auto-stop off. That has failed 16 times on master too, and the message says nothing about who stopped the machine. The wait now runs through a helper that, on failure, appends `machine status` so the event log (exit code, requested stop, source) is in the output next time. TestFlyMachineRun_port read the machines list straight after a completed `machine update` and sometimes saw the previous services (5 master failures). Retry that read for up to 30s through the collector. --- test/preflight/apps_v2_integration_test.go | 22 ++++++++++++++++++---- test/preflight/fly_machine_test.go | 18 ++++++++++++++---- 2 files changed, 32 insertions(+), 8 deletions(-) diff --git a/test/preflight/apps_v2_integration_test.go b/test/preflight/apps_v2_integration_test.go index 41883f7a01..8e5cd40df1 100644 --- a/test/preflight/apps_v2_integration_test.go +++ b/test/preflight/apps_v2_integration_test.go @@ -451,7 +451,7 @@ func testDeployDetach(t *testing.T) { ml := f.MachinesList(appName) require.GreaterOrEqual(f, len(ml), 1) m := ml[0] - f.Fly("machine wait %s --state started --wait-timeout 60s", m.ID) + waitMachineStarted(f, m.ID) res := f.Fly("deploy --detach") require.NotContains(f, res.StdOutString(), "started") @@ -459,7 +459,7 @@ func testDeployDetach(t *testing.T) { ml = f.MachinesList(appName) require.GreaterOrEqual(f, len(ml), 1) m = ml[0] - f.Fly("machine wait %s --state started --wait-timeout 60s", m.ID) + waitMachineStarted(f, m.ID) res = f.Fly("deploy") require.Contains(f, res.StdOutString(), "started") @@ -477,7 +477,7 @@ func testDeployDetachBatching(t *testing.T) { ml := f.MachinesList(appName) require.GreaterOrEqual(f, len(ml), 1) m := ml[0] - f.Fly("machine wait %s --state started --wait-timeout 60s", m.ID) + waitMachineStarted(f, m.ID) res := f.Fly("deploy --detach") require.NotContains(f, res.StdOutString(), "started", false) @@ -485,7 +485,7 @@ func testDeployDetachBatching(t *testing.T) { ml = f.MachinesList(appName) require.GreaterOrEqual(f, len(ml), 1) m = ml[0] - f.Fly("machine wait %s --state started --wait-timeout 60s", m.ID) + waitMachineStarted(f, m.ID) res = f.Fly("deploy") require.Contains(f, res.StdOutString(), "started", false) @@ -543,3 +543,17 @@ ENV BUILT_BY_DOCKERFILE=true require.Contains(f, image["Labels"], `"Z":"ZZZ"`) } } + +// waitMachineStarted waits up to a minute for the machine to be started. When +// it isn't, the failure includes the machine's event log, so a machine that +// was stopped (and by what) shows up in the test output rather than just +// "currently stopped". +func waitMachineStarted(f *testlib.FlyctlTestEnv, machineID string) { + f.Helper() + res := f.FlyAllowExitFailure("machine wait %s --state started --wait-timeout 60s", machineID) + if res.ExitCode() == 0 { + return + } + status := f.FlyAllowExitFailure("machine status %s", machineID) + f.Fatalf("machine %s did not start: %s\n%s%s", machineID, strings.TrimSpace(res.StdErrString()), status.StdOutString(), status.StdErrString()) +} diff --git a/test/preflight/fly_machine_test.go b/test/preflight/fly_machine_test.go index 6a402ab727..602fd14646 100644 --- a/test/preflight/fly_machine_test.go +++ b/test/preflight/fly_machine_test.go @@ -163,6 +163,18 @@ func TestFlyMachineRun_port(t *testing.T) { f := testlib.NewTestEnvFromEnv(t) appName := f.CreateRandomAppMachines() + // The machines list can lag a completed update for a moment, so retry + // the read instead of asserting on the first response. + requireServices := func(want []fly.MachineService) { + require.EventuallyWithT(t, func(c *assert.CollectT) { + ml := f.Collect(c).MachinesList(appName) + if !assert.Len(c, ml, 1) { + return + } + assert.Equal(c, want, ml[0].Config.Services) + }, 30*time.Second, 2*time.Second, "machine services never matched the update") + } + f.Fly("machine run -a %s nginx --port 443:80/tcp:http:tls --region %s", appName, f.PrimaryRegion()) ml := f.MachinesList(appName) require.Equal(f, 1, len(ml)) @@ -180,7 +192,6 @@ func TestFlyMachineRun_port(t *testing.T) { require.Contains(f, []string{"created", "started"}, m.State) f.Fly("machine update -a %s %s -y --port 80/tcp:http --port 1001/udp", appName, m.ID) - m = f.MachinesList(appName)[0] want = []fly.MachineService{{ Protocol: "tcp", InternalPort: 80, @@ -198,10 +209,9 @@ func TestFlyMachineRun_port(t *testing.T) { Port: fly.Pointer(1001), }}, }} - require.Equal(f, want, m.Config.Services) + requireServices(want) f.Fly("machine update -a %s %s -y --port 80/tcp:- --port 1001/udp:tls", appName, m.ID) - m = f.MachinesList(appName)[0] want = []fly.MachineService{{ Protocol: "udp", InternalPort: 1001, @@ -210,5 +220,5 @@ func TestFlyMachineRun_port(t *testing.T) { Handlers: []string{"tls"}, }}, }} - require.Equal(f, want, m.Config.Services) + requireServices(want) }