fix(manifestutil): report one message when the CRD wait is cancelled - #3742

What this PR does

WaitForCRDsEstablished reported a cancelled context two different ways. The check at the top of the poll loop returned a message with no CRD name, the select at the bottom named the CRD the poll had stopped on. When a tick and the deadline come ready in the same instant, select picks between the ready cases at random, so one cancellation reached the operator either way.

Both callers wrap this as CRDs not established after apply, so the CRD name is what makes that log line actionable, and whether it is there was a coin toss. TestWaitForCRDsEstablished_timeout asserts on the name, which is where this surfaced: its 2s deadline is an exact multiple of the 500ms ticker, so the two channels come ready together on the fourth tick and the assertion failed at random.

The message is now built in one place and returned from both exits, so the two are indistinguishable by construction rather than by inspection. The name is carried across iterations, because a cancellation noticed at the top of the loop has no poll of its own to take one from, and it starts as the first CRD of the set for a wait cancelled before any poll observed anything. That start value is a stand-in rather than an observation, and the comment on it says so; the alternative, an empty name and a second phrasing for it, would leave the loop-top message with nothing to assert against and no test able to catch its return to the unnamed form.

Deleting the top check instead, so there is literally one exit, is the smaller diff and it moves the name to the head of the list in the very interleaving this PR is about. A client refuses Get on a dead context at the first name in the list whichever CRD is actually lagging, so a poll entered on a cancelled context renames the report to that first name. That premise is about the production client, client.New at cmd/cozystack-operator/main.go:176, uncached, so the request is rejected before it is issued, and it is worth saying that nothing in the test surface pins it: replacing the ctx passed to Get with context.Background() survives the whole package, and the fixtures that need a context-honouring client check ctx.Err() by hand. That line predates this change and is untouched by it. Measured with a client that honours the context: with the check, a wait that re-enters the loop after the deadline keeps the name its last poll settled on; without it, the report moves to the head of the list. The check closes that window and no more than that. A context that dies part-way through a poll still records the first Get to fail, exactly as merge base does, because this branch cannot tell a refused request from a CRD status. That is unchanged here and written up separately.

Four tests cover the ways this wait can end on a cancelled context: before any poll has observed the set, cut off inside a poll, cancelled while waiting for the next tick, and re-entering the loop on a context that died during that wait. Two of them are red on merge base and are the regression pins for the defect above, both deterministically rather than by repetition: merge base answers a wait that starts cancelled, and a wait that re-enters the loop cancelled, with the unnamed message every time. Its loop-top exit is a select whose Done case is ready, and a ready case always beats default. The middle two are green on merge base, which gets those cases right; they are here because each catches a specific way of breaking the fix. Every one of the four is the sole killer of at least one mutation, which is the property worth having: it says none of them is redundant. They compare the whole message rather than searching it for a name, because a second exit that happens to carry a name is still the defect being fixed here, and a test that only looks for the name cannot see it.

The re-entry test needs the bottom select to take the tick while cancellation is already pending, which against a real context is the same coin toss the bug rides on. The double it uses hands out an open Done for the single read that select makes in that iteration and a closed one for every read after, so the tick wins by construction and the next iteration sees a context cancelled through both Done and Err, the way a real one is. That pairing is guaranteed rather than incidental: cancelCtx.Err reads <-c.Done() before returning a non-nil error, so a double that reported cancellation through Err alone would be modelling a state no context can be in, and would pin the shape of the check rather than its behaviour. One property of that double is worth knowing before editing the loop: it hands out its open and closed channels per read, so a Done() read added inside the poll would consume the open one and leave the bottom select a closed channel, quietly turning this into a second bottom-exit test that still passes. The failure direction there is false-green, which is the worse half, and the cheap guard is a lower bound on elapsed time inside that test, a run that leaves through the loop top spends a ticker interval getting there, one that leaves at the bottom of the first iteration does not. Not done here, because the test is correct today and provably kills the mutation it exists for, so the hazard is conditional on a future edit.

The timeout test does reproduce locally under load: 6 failures across 400 runs on the unfixed code while I was measuring, spread over five batches, the last of which saw none. #3719 reports that it does not reproduce locally, so this is worth writing down before someone spends an evening re-testing that. That spread is also why the number is an order of magnitude and not a rate.

TestInstall_crdNotEstablished in internal/crdinstall has the same 2s deadline against the same 500ms ticker. It asserts on the CRDs not established wrapper the caller adds, and both messages carried that wrapper, so this coin flip could not reach it and nothing here touches it.

I left the 2s deadline where it is. Moving it off the tick boundary would have hidden the coin flip and left the second message in place. The new tests catch a reintroduced second message deterministically, which is what that job now rests on rather than the timeout test's roughly one-in-sixty.

Screenshots

Downstream repositories

• No downstream repository is affected by this change
• cozystack/website - follow-up:
• cozystack/terraform-provider-cozystack - follow-up:
• cozystack/ansible-cozystack - follow-up:
• cozystack/ccp - follow-up:
• cozystack/talm - follow-up:
• cozystack/cozyhr - follow-up:
• cozystack/cozy-proxy - follow-up:
• cozystack/cozystack-telemetry-server - follow-up:
• cozystack/external-apps-example - follow-up:
• cozystack/examples - follow-up:

Release note

fix(manifestutil): a CRD wait cut short by a cancelled context now reports one error naming the CRD it was waiting on, where before the name was present or missing depending on which of two exits the scheduler happened to pick

Summary by CodeRabbit

• Bug Fixes

  • Improved cancellation handling while waiting for custom resources to become established.
  • Cancelled operations now stop before issuing unnecessary requests and identify the affected resource.
  • Timeout errors remain distinguishable from cancellation errors.

• Tests

  • Added coverage for cancellation before, during, after, and between polling attempts.