Repository navigation
Conversation
@coderabbitai review |
|
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 37 minutes. View limit details
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 |
devantler
left a comment
devantler
left a comment
There was a problem hiding this comment.
🤖 Generated by the Agentic Engineer
Local review
Reviewed exact head: 0d7d6a0
CodeRabbit was requested at this head and refused because its review quota is exhausted. Codex and Cursor review lanes are also unavailable, so this is the repository-approved local fallback.
I reviewed the controller-transition helper, both call sites, and all three new regressions. The recovery branch performs reads only: it requires the captured owner to be absent or deleting, refuses a replacement UID, requires any remaining chart to retain its captured identity and ownership, verifies unchanged stored Helm content, requires the deployment source to no longer own the targets, and compares the full retained projection before observing the existing lifecycle. It never submits another delete on that branch. Partial cleanup remains a nonzero postcondition hold.
No deletion authority, target set, admission exception, data-retirement scope, or replay permission is widened.
Verdict: no P0/P1 findings.
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d7d6a0169
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2585687486
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else { | ||
| // Kubernetes owns the ongoing finalizer once DELETE may have been submitted. | ||
| // A workflow error, timeout or process stop cannot cancel its retries. | ||
| if nativeSubmitted, err = e.remove(ctx, current.owner, "/apis/helm.toolkit.fluxcd.io/v2/namespaces/backstage/helmreleases/backstage"); err != nil { | ||
| return hold("HOLD_MUTATION") |
There was a problem hiding this comment.
If Flux begins pruning after the final admission probe succeeds but before e.remove runs, admissionControls returns nil and this code skips the new observation path. The bound delete then uses current.owner's stale resource version and returns Conflict or NotFound once the controller's deletion has advanced or completed, causing an immediate HOLD_MUTATION instead of verifying the terminal state—the same false failure this change is intended to prevent. Re-snapshot and apply the lifecycle observation after an unsuccessful bound delete as well as after an admission failure.
Useful? React with 👍 / 👎.
| return hold("HOLD_MUTATION") | ||
| } | ||
| controllerLifecycle := false | ||
| if e.admissionControls(ctx, current) != nil { |
There was a problem hiding this comment.
If an admission probe fails for a reason other than target disappearance—for example, a retained-object negative control is unexpectedly accepted because webhook enforcement is ineffective—concurrent Flux deletion can still make controllerLifecycleStarted return true, so this branch converts the failed mandatory control into observation and eventual success. The fresh issue is that this new branch treats every admission error identically even though the DELETE probes now carry a correct dry-run body; only a failure specifically attributable to the captured retiring object disappearing should be adoptable, while ineffective or incomplete admission evidence must remain a hold.
AGENTS.md reference: AGENTS.md:L600-L603
Useful? React with 👍 / 👎.
| if observeErr != nil || !reflect.DeepEqual(before, kept) { | ||
| return hold("HOLD_ADMISSION") | ||
| } | ||
| controllerLifecycle = true |
There was a problem hiding this comment.
When this observation path proves that the controller lifecycle has already started but a later postcondition fails or times out, controllerLifecycle is true while nativeSubmitted remains false. Since the hold formatter only emits HOLD_NATIVE_LIFECYCLE_CONTINUES when nativeSubmitted is true, the workflow reports a plain HOLD_POSTCONDITION, hiding that deletion may still be progressing and contradicting the no-replay handling documented in docs/adr/backstage-retirement-lifecycle.md. Propagate the adopted lifecycle into the continuation flag before any subsequent hold can be returned.
Useful? React with 👍 / 👎.
Closing this repair without merge because the one-time retirement path it changes is now permanently consumed and the production outcome is terminal. The delivered completion path supersedes this branch:
This PR's three outstanding review findings therefore do not justify changing a non-replayable operation. No branch code is being merged or executed. |
Why
The Backstage stateless retirement completed through the Helm controller, but one of the workflow admission probes raced with the disappearing release and reported HOLD_ADMISSION. That false failure makes a successful protected retirement look incomplete and can invite an unsafe replay.
What
After an admission probe fails, the workflow now takes a fresh read-only snapshot. It observes the existing lifecycle only when the original release is absent or deleting, any remaining chart has the captured identity, the stored Helm release is unchanged, the deployment source no longer owns the targets, and every retained database and storage projection is unchanged. It never submits another delete on that path and reports success only after all stateless targets are absent and the stored release is uninstalled.
Regression tests cover a completed controller lifecycle, a partial lifecycle, and a replacement release with a foreign UID.
Fixes #5039
Related to #4819