bugfix: ensure collision detection runs before revision linearity check#501
Merged
thetechnick merged 1 commit intopackage-operator:mainfrom Mar 31, 2026
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughRemoved the top-level early-return for revision linearity in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #501 +/- ##
==========================================
+ Coverage 72.22% 72.26% +0.03%
==========================================
Files 35 35
Lines 3017 3021 +4
==========================================
+ Hits 2179 2183 +4
Misses 712 712
Partials 126 126 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The revision linearity check in objectUpdateHandling() was evaluated before collision protection, causing a resource owned by a different parent object at a higher revision to be reported as ActionProgressed instead of ActionCollision. For example, if Revision 2 (Parent-B) created a CRD and Revision 1 (Parent-A) later reconciled the same CRD, the check `actualObjectRevision > revision` would short-circuit and return ActionProgressed before the ownership-based collision detection in the switch statement could run. The fix moves the revision linearity check from the top-level early return into the two switch cases where skipping reconciliation is safe: - ctrlSituationIsController: we already own the object - ctrlSituationPreviousIsController: a known previous owner has it The ctrlSituationUnknownController and ctrlSituationNoController cases now always evaluate their collision protection logic regardless of the object's revision, ensuring that cross-owner conflicts are properly detected as ActionCollision. Test updates: - Scoped existing "Progressed" test to withNativeOwnerMode (the withoutOwnerMode case now correctly evaluates collision protection) - Added "Progressed - previousOwner higher revision" test - Added "Collision - unknown controller higher revision" test - Added "Collision - no controller higher revision boxcutter managed" test Signed-off-by: Per G. da Silva <pegoncal@redhat.com>
perdasilva
force-pushed
the
boxcutter-fix
branch
from
ef8929e to
cb43b20
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@machinery/objects.go`:
- Around line 325-329: The current eager call to
e.getObjectRevision(actualObject) makes all paths depend on revision parsing and
causes unrelated malformed revisions to short-circuit objectUpdateHandling;
remove the early call and the actualObjectRevision variable, and instead call
getActualObjectRevision (or e.getObjectRevision) lazily only inside the branches
ctrlSituationIsController and ctrlSituationPreviousIsController where the
revision is actually needed; update those two handlers to perform the revision
parsing and handle/return errors locally so that ctrlSituationUnknownController
and ctrlSituationNoController can run and report ActionCollision without being
blocked by revision parse errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6fbf07da-cce1-4d06-93c4-3cac3047a407
📒 Files selected for processing (2)
machinery/objects.gomachinery/objects_test.go
Comment on lines
+325
to
329
| // Get actual revision to ensure revision linearity | ||
| actualObjectRevision, err := e.getObjectRevision(actualObject) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("getting revision of object: %w", err) | ||
| } |
There was a problem hiding this comment.
Defer revision parsing until a branch actually needs it.
Line 325 still makes cross-owner paths depend on parsing the revision annotation. If an unrelated object carries a malformed boxcutter revision value, objectUpdateHandling() returns "getting revision of object" before ctrlSituationUnknownController / ctrlSituationNoController can report ActionCollision.
💡 Proposed fix
- // Get actual revision to ensure revision linearity
- actualObjectRevision, err := e.getObjectRevision(actualObject)
- if err != nil {
- return nil, fmt.Errorf("getting revision of object: %w", err)
- }
+ getActualObjectRevision := func() (int64, error) {
+ actualObjectRevision, err := e.getObjectRevision(actualObject)
+ if err != nil {
+ return 0, fmt.Errorf("getting revision of object: %w", err)
+ }
+ return actualObjectRevision, nil
+ }Then call getActualObjectRevision() only inside ctrlSituationIsController and ctrlSituationPreviousIsController.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@machinery/objects.go` around lines 325 - 329, The current eager call to
e.getObjectRevision(actualObject) makes all paths depend on revision parsing and
causes unrelated malformed revisions to short-circuit objectUpdateHandling;
remove the early call and the actualObjectRevision variable, and instead call
getActualObjectRevision (or e.getObjectRevision) lazily only inside the branches
ctrlSituationIsController and ctrlSituationPreviousIsController where the
revision is actually needed; update those two handlers to perform the revision
parsing and handle/return errors locally so that ctrlSituationUnknownController
and ctrlSituationNoController can run and report ActionCollision without being
blocked by revision parse errors.
thetechnick
approved these changes
Mar 31, 2026
camilamacedo86
added a commit
to camilamacedo86/operator-controller
that referenced
this pull request
Apr 9, 2026
Boxcutter v0.13.1 includes the fix from package-operator/boxcutter#501 which ensures collision detection runs before revision linearity checks. This allows us to remove the foreignRevisionController workaround that was manually detecting ActionProgressed objects owned by foreign ClusterExtensions. Assisted-by: Claude
openshift-merge-bot Bot
pushed a commit
to operator-framework/operator-controller
that referenced
this pull request
Apr 9, 2026
#2637) Boxcutter v0.13.1 includes the fix from package-operator/boxcutter#501 which ensures collision detection runs before revision linearity checks. This allows us to remove the foreignRevisionController workaround that was manually detecting ActionProgressed objects owned by foreign ClusterExtensions. Assisted-by: Claude
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
3 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.
Summary
ActionProgressedinstead ofActionCollisionctrlSituationIsControllerandctrlSituationPreviousIsControllerswitch cases, ensuringctrlSituationUnknownControllerandctrlSituationNoControlleralways evaluate collision protection regardless of revisionDetails
The revision linearity check in
objectUpdateHandling()(actualObjectRevision > revision) was evaluated before the ownership-based collision protection switch statement. This meant that if Revision 2 (Parent-B) created a CRD and Revision 1 (Parent-A) later reconciled the same CRD, the check would short-circuit and returnActionProgressedbefore collision detection could run.The fix restricts the linearity check to the two cases where it is safe to skip reconciliation:
ctrlSituationIsController: we already own the objectctrlSituationPreviousIsController: a known previous owner has itTest plan
withNativeOwnerModeonlyActionProgressedfor known lineageActionCollisionfor cross-owner conflictActionCollisionfor orphaned boxcutter-managed objects🤖 Generated with Claude Code