TestWaitForCRDsEstablished_timeout races the ticker against its own deadline #3719

TestWaitForCRDsEstablished_timeout in internal/manifestutil fails at random. It is not tied to a branch, a package under change, or a runner image: the same commit produces both outcomes.

Proof

Run 31347277024 on head 4c608406f, job Unit & controller tests. Attempt 1 (job 93331525818) failed. Attempt 2 (job 93333768799), a rerun of the same attempt on the same commit with no push in between, succeeded. The failure message was:

--- FAIL: TestWaitForCRDsEstablished_timeout (2.00s)
    crd_test.go:188: error should mention stuck CRD name, got: context cancelled while waiting for CRDs to be established: context deadline exceeded

Mechanism

WaitForCRDsEstablished has two exits for a cancelled context, and only one of them names the CRD.

git show origin/main:internal/manifestutil/crd.go | sed -n '45p;48,53p;98,102p'

Line 51 is the top of the loop, a non-blocking select that returns context cancelled while waiting for CRDs to be established: %w with no CRD name. Line 100 is the bottom, a blocking select over ctx.Done() and ticker.C that returns context cancelled while waiting for CRD %q to be established: %w. The assertion at crd_test.go:188 requires the name, so the test passes only when the function leaves through the bottom exit.

The ticker is 500ms (crd.go:45) and the test deadline is 2000ms (crd_test.go:179). 2000 is an exact multiple of 500, so on the fourth tick both channels become ready at the same instant, and a Go select with more than one ready case picks uniformly at random. When it picks ticker.C, the loop runs one more iteration, the top select observes the cancellation first, and the error comes back without the name. Nothing in the test or the function makes one of the two orderings preferred; scheduling delay on the runner decides which one is even in the race.

Why this is worth fixing rather than rerunning

A failing Unit & controller tests skips the required E2E gate, so a single unlucky tick costs a whole branch run, not one test. That is the difference between an annoying flake and one worth a fix: the blast radius is the entire pipeline for that commit, and recovering costs a rerun plus the full E2E wall clock.

Suggested fix

Two halves, both small.

The top select at crd.go:48-53 is redundant. k8sClient.Get(ctx, ...) fails on a cancelled context on its own, and the bottom select already covers cancellation while naming the CRD. Deleting the top one removes the unnamed error path entirely. If it is kept for the case where the context is already cancelled on entry, it should carry the same name the previous iteration recorded.

Separately, the test deadline should not be an exact multiple of the ticker interval. 2500ms against a 500ms ticker still bounds the test at a comparable duration while keeping the deadline and the tick from becoming ready in the same instant, which is what makes the current race a coin flip rather than a rare interleaving.

What I could not reproduce

Locally the test does not fail: 15 plain runs, then 20 more with all 12 cores saturated by busy loops, all green. A run under GOMAXPROCS=1 was cut off by a time limit before it finished, so that configuration is untested. This is a boundary on where to look, not a weakness in the report: the proof above is the rerun, and the mechanism explains why a developer machine is the wrong place to hunt for it.