Skip to content

fix(retirement): recognize controller-completed cleanup #5041

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
devantler wants to merge 2 commits into from
Closed

fix(retirement): recognize controller-completed cleanup #5041

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0d7d6a0
fix(retirement): observe controller-completed cleanup
devantler Oct 17, 2026
2585687
Merge branch 'main' into codex/retirement-race-5039
devantler Oct 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 74 additions & 0 deletions scripts/retire-backstage-stateless/fence_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,80 @@ func TestMissingOrIneffectiveAdmissionCannotAuthorizeOwnerDeletion(t *testing.T)
}
}

func TestControllerCompletedRetirementDuringAdmissionIsObservedWithoutReplay(t *testing.T) {
retirementRunner(t)
f := retirementFixture(t)
completed := false
nativeDeletes := 0
execute := func(c context.Context, p string, a ...string) ([]byte, error) {
args := strings.Join(a, " ")
if p == "kubectl" && strings.Contains(args, " delete ") && strings.Contains(args, "?dryRun=All") && strings.Contains(args, "/helmreleases/backstage?dryRun=All") && !completed {
completed = true
f.deleted = true
f.historyStatus = "uninstalled"
return nil, fmt.Errorf("target disappeared during dry-run")
}
if p == "kubectl" && strings.Contains(args, " delete ") && !strings.Contains(args, "?dryRun=All") {
nativeDeletes++
return nil, fmt.Errorf("duplicate lifecycle submission")
}
return f.execute(c, p, a...)
}
var out bytes.Buffer
if code := run([]string{"--execute"}, &out, execute); code != 0 || !completed || !f.deleted || nativeDeletes != 0 || !strings.Contains(out.String(), "RETIRED_STATELESS") {
t.Fatalf("controller-completed retirement was replayed or rejected: code=%d completed=%t deleted=%t nativeDeletes=%d output=%s", code, completed, f.deleted, nativeDeletes, &out)
}
}

func TestPartialControllerRetirementDuringAdmissionIsNeverReplayed(t *testing.T) {
retirementRunner(t)
f := retirementFixture(t)
started := false
nativeDeletes := 0
execute := func(c context.Context, p string, a ...string) ([]byte, error) {
args := strings.Join(a, " ")
if p == "kubectl" && strings.Contains(args, " delete ") && strings.Contains(args, "?dryRun=All") && strings.Contains(args, "/helmreleases/backstage?dryRun=All") && !started {
started = true
f.deleted = true
return nil, fmt.Errorf("target disappeared during dry-run")
}
if p == "kubectl" && strings.Contains(args, " delete ") && !strings.Contains(args, "?dryRun=All") {
nativeDeletes++
return nil, fmt.Errorf("duplicate lifecycle submission")
}
return f.execute(c, p, a...)
}
var out bytes.Buffer
if code := run([]string{"--execute"}, &out, execute); code != 2 || !started || nativeDeletes != 0 || !strings.Contains(out.String(), "HOLD_POSTCONDITION") {
t.Fatalf("partial controller retirement was replayed or accepted: code=%d started=%t nativeDeletes=%d output=%s", code, started, nativeDeletes, &out)
}
}

func TestForeignControllerLifecycleDuringAdmissionIsNeverAdopted(t *testing.T) {
retirementRunner(t)
f := retirementFixture(t)
replaced := false
nativeDeletes := 0
execute := func(c context.Context, p string, a ...string) ([]byte, error) {
args := strings.Join(a, " ")
if p == "kubectl" && strings.Contains(args, " delete ") && strings.Contains(args, "?dryRun=All") && strings.Contains(args, "/helmreleases/backstage?dryRun=All") && !replaced {
replaced = true
object(f.hr["metadata"])["uid"] = "replacement-release-uid"
object(f.hr["metadata"])["deletionTimestamp"] = "2026-10-17T21:30:00Z"
return nil, fmt.Errorf("target changed during dry-run")
}
if p == "kubectl" && strings.Contains(args, " delete ") && !strings.Contains(args, "?dryRun=All") {
nativeDeletes++
return nil, fmt.Errorf("duplicate lifecycle submission")
}
return f.execute(c, p, a...)
}
var out bytes.Buffer
if code := run([]string{"--execute"}, &out, execute); code != 2 || !replaced || nativeDeletes != 0 || !strings.Contains(out.String(), "HOLD_ADMISSION") {
t.Fatalf("foreign controller lifecycle was adopted or replayed: code=%d replaced=%t nativeDeletes=%d output=%s", code, replaced, nativeDeletes, &out)
}
}

func TestAPIDefaultedFenceMustMatchTheReviewedGetMethod(t *testing.T) {
for _, method := range []string{"GET", "POST"} {
t.Run(method, func(t *testing.T) {
Expand Down
45 changes: 39 additions & 6 deletions scripts/retire-backstage-stateless/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -807,6 +807,23 @@ func unchangedOwner(a, b, chart map[string]any) bool {
}
return true
}

func (e engine) controllerLifecycleStarted(before, after snapshot) bool {
if !sameStoredContent(before.release, after.release) {
return false
}
if after.owner != nil {
if !identityComplete(after.owner) || identity(after.owner) != identity(before.owner) || field(after.owner, "metadata", "uid") != field(before.owner, "metadata", "uid") || field(after.owner, "metadata", "generation") != field(before.owner, "metadata", "generation") || !reflect.DeepEqual(after.owner["spec"], before.owner["spec"]) || text(field(after.owner, "metadata", "deletionTimestamp")) == "" {
return false
}
}
if after.chart != nil {
if !identityComplete(after.chart) || identity(after.chart) != identity(before.chart) || field(after.chart, "metadata", "uid") != field(before.chart, "metadata", "uid") || !reflect.DeepEqual(after.chart["spec"], before.chart["spec"]) || e.chartOwned(after) != nil {
return false
}
}
return true
}
func targetResource(r map[string]any) bool {
name := text(field(r, "metadata", "name"))
kind := text(r["kind"])
Expand Down Expand Up @@ -1070,13 +1087,26 @@ func run(args []string, out io.Writer, execute command) int {
if err != nil || !reflect.DeepEqual(before, kept) {
return hold("HOLD_MUTATION")
}
controllerLifecycle := false
if e.admissionControls(ctx, current) != nil {

@chatgpt-codex-connector chatgpt-codex-connector Bot Oct 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Do not adopt arbitrary admission failures

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 👍 / 👎.

return hold("HOLD_ADMISSION")
}
// 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")
// The GitOps deletion can reach the Helm controller immediately after the
// durable journal unlocks the fence. If that happens while the admission
// probes run, observe the already-started exact lifecycle; never replay it.
observed, observeErr := e.collect(ctx)
Comment thread
devantler marked this conversation as resolved.
Resolved
if observeErr != nil || !e.controllerLifecycleStarted(current, observed) || e.deployed(observed) != nil {
return hold("HOLD_ADMISSION")
}
kept, observeErr = preserved(observed)
if observeErr != nil || !reflect.DeepEqual(before, kept) {
return hold("HOLD_ADMISSION")
}
controllerLifecycle = true

@chatgpt-codex-connector chatgpt-codex-connector Bot Oct 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Mark adopted deletion as an ongoing native lifecycle

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 👍 / 👎.

} 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")
Comment on lines +1104 to +1108

@chatgpt-codex-connector chatgpt-codex-connector Bot Oct 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Observe lifecycle races after successful probes

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 👍 / 👎.

}
}
var after snapshot
// The native uninstall is allowed five minutes. A two-minute observation
Expand Down Expand Up @@ -1105,6 +1135,9 @@ func run(args []string, out io.Writer, execute command) int {
return hold("HOLD_POSTCONDITION")
}
if field(after.chart, "metadata", "deletionTimestamp") == nil {
if controllerLifecycle {
return hold("HOLD_POSTCONDITION")
}
if _, err = e.remove(ctx, after.chart, "/apis/source.toolkit.fluxcd.io/v1/namespaces/backstage/helmcharts/backstage-backstage"); err != nil {
return hold("HOLD_MUTATION")
}
Expand Down
Loading