Repository navigation
[Backport release-1.6] fix(manifestutil): report one message when the CRD wait is cancelled - #4475
Merged
Merged
Conversation
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. Which one ran was left to the scheduler: when the deadline and a tick come ready in the same instant, a select picks between the ready cases at random, so one cancellation reached the operator either way. Build the message in one place and return it from both exits, so the two are indistinguishable by construction. The name is carried across iterations, since a cancellation noticed at the top of the loop has no poll of its own to take one from, and starts as the first CRD of the set for a wait cancelled before any poll observed anything. Keeping the check at the top is what makes carrying the name worthwhile. A client refuses Get on a dead context at the first name in the list whichever CRD is actually lagging, so a loop that re-entered and polled anyway would overwrite the name its last poll settled on. The check closes that window and no more than that: a context dying part-way through a poll still records the first Get to fail, because this branch cannot tell a refused request from a CRD status. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]> (cherry picked from commit 5747747) Signed-off-by: Myasnikov Daniil <[email protected]>
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: cozystack/cozystack/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
size/L
myasnikovdaniil
added a commit
that referenced
this pull request
Sep 25, 2026
The audit started from labels and dropped every backport PR whose original was not a candidate for the branch. On release-1.6 that hid nine hand backports of unlabelled main PRs (#4431 to #4435, #4437, #4438, #4456 and #4475), and it had no way to notice two open backport PRs for the same originals: #4421 sat open next to #4456 for #3938 and #4280, and nothing reported it. Read the PRs on the branch from the side of the originals they claim as well, and report two more sections per branch. UNLABELLED lists each original claimed by backport PRs on the branch that is not a candidate for it, with every backport PR claiming it and its state, and says whether they backported it, only claim it with a PR still open, or were all closed. It never moves the exit code: the gate answers whether everything labelled landed, and an unlabelled backport can only add to a branch, never leave a labelled change off it. DUPLICATE lists each original claimed by two open backport PRs, or by an open one after another already merged, labelled or not. Both make the audit exit 1. One of the PRs is redundant, or the open one is the rest of a split backport; either way someone has to decide before the cut, and no verdict can say so, since a verdict settles on the first merged backport or reports the first open one as pending. A closed PR next to an open one is how a conflicting bot backport gets redone by hand and is not flagged. The titles of originals that no listing carries come from one GraphQL request for the whole run. A failed lookup costs the titles and nothing else: the URL is derived locally and the exit code is already settled. --json now emits an object per branch holding candidates, unlabelled and duplicates arrays, in place of the bare array of verdicts. On release-1.6 today this lists 13 unlabelled originals, the nine above among them, and three duplicates that are open right now: the bot's conflict drafts for #3936, #4254 and #4292 were left open next to their hand backports, and #4254 and #4292 each also have a fork PR open next to its reopening from a branch in this repository. #4421 is closed and shows up only as a closed claim on #4280. Assisted-by: LLM Signed-off-by: Myasnikov Daniil <[email protected]>
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.
Backport of #3742 to
release-1.6.TestWaitForCRDsEstablished_timeoutfailsUnit & controller testson this branch at random, last time on #4402:error should mention stuck CRD name, got: context cancelled while waiting for CRDs to be established: context deadline exceeded.WaitForCRDsEstablishedhere still has two cancellation exits with different messages, and when the deadline and a ticker tick are ready together the select picks one of them at random. I ran the timeout test as 16 concurrent processes to get the timing jitter CI has: 3 failures out of 160 runs on the current branch, 0 out of 320 with this change.Cherry-picked clean with
-x.Testing
make unit-testsandmake test-controllersgreen, POSIX sh sweep clean.