Repository navigation
Fix flaky preflight tests - #5310
Open
alexleventer wants to merge 1 commit into
Open
alexleventer wants to merge 1 commit into
alexleventer wants to merge 1 commit into
Conversation
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 <id>: <state>" 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 <noreply@anthropic.com>
1 of 3 tasks
| 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) |
Contributor
There was a problem hiding this comment.
I'm unsure about this one. A destroy that takes a long time is a bug and we should fix it instead of ignoring it.
| // 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. |
Contributor
There was a problem hiding this comment.
I think it would help to log the number and duration of each retry; here and in the other EventuallyWithT.
| 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 |
Contributor
There was a problem hiding this comment.
Hmm, it looks like the original EventuallyWithT was already very generous, time-wise. I'm a bit worried that there are bugs hiding here and by making the tests even more generous, we're simply ignoring them.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Change Summary
What and Why:
About 40% of scheduled Build runs on master fail in the preflight jobs, often alternating pass/fail on the same commit. I classified all 258 test failures across the 145 failed scheduled runs since 2026-09-23. This PR fixes the ones caused by the tests themselves (~50 failures):
TestFlyLaunchWithBuildSecretsf.Fly, so the first failedssh consoleattempt failed the test. The 10s window was also shorter than one attempt.TestDeployDetach(Simple, Batching)shouldSkipLaunch), so they never reachedstarted.TestFlyDeploy_DeployMachinesCheck,NoServiceDeployMachinesCheckTest Machine <id>: <state>, which is skipped once the test machine has been destroyed.TestFlyWireguardCreateapps destroyfailed tests that had passed and could block for 5 minutes against the 15mgo testtimeout.How:
FlyBestEffort(timeout, cmd)testlib helper. It reports failures throughExitCode()and timeouts throughTimedOut()instead of failing the test, so it's safe inEventuallyWithTloops and cleanup. Commands that assert success still fail on a timeout.--auto-stop off.TestDeployDetachlaunches with--auto-stop offand waits up to 60s.Test machine <id> completed successfully, which is always printed on success.FlyBestEffortwith a 1-minute timeout, logs failures, and leaves leftover apps topreflight_cleanup.yml.cmdHistoryis guarded by a mutex, since a retry attempt still running whenEventuallyWithTgives up can append after the test returns.Not fixed here: about 200 failures come from platform or infra errors. The largest group (~75) is
app not found/failed to get app: sql: no rows,volume not foundandmachine not foundreturned right after the resource was created. The rest are 502s fromdocker-hub-mirror.fly.io, 408/5xx errors, IP allocation failures and commands that hang past 5 minutes. These happen insidelaunch/deploy/scale, so users hit them too, and retrying in the tests would hide them. The bluegreencanPerformBluegreenDeployment504s (19) were already fixed by #5292.Verified with
go vetand with throwaway testlib tests against a fake flyctl binary under-race: the timeout, exit-code, assert-on-timeout and concurrent-history cases all pass. The preflight run on this PR is the real check.Related to:
Documentation
🤖 Generated with Claude Code