fix(api): let the garbage collector finalize Applications by yankawai · Pull Request #4610 · cozystack/cozystack

europrinter (yankawai) · 2026-09-30T14:19:56Z

What this PR does

An application deleted with foreground or orphan propagation (kubectl delete --cascade=foreground, or any client that sends that propagationPolicy) could stay in deletion forever. Helm uninstalled the release and helm-controller removed finalizers.fluxcd.io, but the HelmRelease kept foregroundDeletion in metadata.finalizers, so the HelmRelease and its Application stayed terminating. We hit this on v1.6.4 with a Bucket and an OpenBAO application.

An Application and its HelmRelease share one UID, so the garbage collector keeps a single graph node for both and may finalize it through the Application endpoint. The API never returned finalizers on an Application, and every update restored the HelmRelease finalizers from the live object. removeFinalizer reads the owner, does not find the finalizer there and returns as if it had already been removed, so nothing ever took it off the HelmRelease.

This PR:

returns foregroundDeletion and orphan in metadata.finalizers of an Application, and only those two;
carries an add or a remove of either of them from an Application update to the HelmRelease, as a change against the finalizers the update started from, so the result is the same when helm-controller or the garbage collector changes the list during the conflict retry;
keeps controller finalizers such as finalizers.fluxcd.io owned by the live object, so helm uninstall still runs before the release goes away;
copies the deletion timestamp and grace period from the live object into the rebuilt HelmRelease, so an update of a terminating release stays valid.

rest_garbage_collection_test.go covers both finalizers with Helm finishing first and with the garbage collector finishing first, concurrent finalizer changes during the conflict retry, controller finalizers surviving an update, the cached HelmRelease not being mutated by the conversion, and an UpdatedObject that edits the object in place. All five tests fail on the current main without this change. go test -race ./pkg/registry/... passes. The tests use the controller-runtime fake client, so this has not been run against a live garbage collector with a patched cozystack-api yet.

The same bug is in release-1.6. There the Update path has no finalizer carry-over at all, because #2914 is not backported yet, and its automatic backport #4577 currently carries conflict markers. A 1.6 backport of this fix should go on top of a working backport of #2914.

Screenshots

Not a UI change.

Downstream repositories

Walked the trigger map against the diff: it only changes how cozystack-api handles metadata.finalizers of Applications in pkg/registry/apps/application. No package, values, schema, CRD, hack/ file or convention is touched.

Release note

fix(api): an application deleted with foreground or orphan propagation no longer leaves its HelmRelease stuck with the foregroundDeletion or orphan finalizer

Summary by CodeRabbit

Bug Fixes
- Application updates now preserve Kubernetes deletion behavior, including foreground deletion and orphaning, even when concurrent changes occur.
- Helm-managed protection metadata is retained during updates, and deletion timing details remain intact.

An Application and its HelmRelease share one UID, so the garbage collector keeps a single graph node for both and may finalize it through the Application endpoint. The API never returned finalizers on an Application, and every update restored the HelmRelease finalizers from the live object. When the collector came to drop foregroundDeletion or orphan through the Application, removeFinalizer found no such finalizer and returned as if it were already gone. The HelmRelease then kept foregroundDeletion after Helm had uninstalled the release, and the deletion never completed. The Application now carries the two garbage collection finalizers, and an update that adds or removes one of them applies the same change to the HelmRelease. Controller finalizers such as finalizers.fluxcd.io stay owned by the live object, so helm uninstall still runs before the release goes away. The deletion timestamp and grace period are copied from the live object, which keeps an update of a terminating release valid. Assisted-by: LLM Signed-off-by: Yan Bondarenko <[email protected]>

coderabbitai · 2026-09-30T14:20:30Z

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: cozystack/cozystack/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: daa68573-47e8-4bac-96fe-0d77ad02ce12

📥 Commits

Reviewing files that changed from the base of the PR and between 4f043b5 and 46b5a54.

📒 Files selected for processing (2)

pkg/registry/apps/application/rest.go
pkg/registry/apps/application/rest_garbage_collection_test.go

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.

📝 Walkthrough

Walkthrough

Application conversion now exposes and writes only Kubernetes garbage-collection finalizers. Update assembly merges requested finalizer changes with live HelmRelease state, including on conflict retries, and preserves the live deletion timestamp and grace period. Tests cover deletion ordering, concurrent changes, controller protection, and cached-object immutability.

Changes

Application garbage-collection finalizers

Layer / File(s)	Summary
Finalizer conversion and merge `pkg/registry/apps/application/rest.go`, `pkg/registry/apps/application/rest_garbage_collection_test.go`	Application conversion filters finalizers to `foregroundDeletion` and `orphan`. Metadata merging applies requested changes to those finalizers while retaining other live HelmRelease finalizers. Tests cover deletion ordering, controller-finalizer preservation, and conversion without changing cached finalizers.
Update metadata and conflict retries `pkg/registry/apps/application/rest.go`, `pkg/registry/apps/application/rest_garbage_collection_test.go`	Initial and retry update assembly merge finalizers using the prior Application finalizers. Metadata assembly also copies the live deletion timestamp and grace period. Tests cover concurrent finalizer changes and in-place finalizer removal.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 46b5a

No actionable merge-blocking issue is established. The change appears mergeable after normal checks; reported validation uses a fake client rather than a live garbage collector.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 46b5a

The change limits writable cleanup metadata to two Kubernetes finalizers and preserves Helm cleanup protection during concurrent updates. No introduced security bypass was established. Authorization enforcement and real-controller behavior remain incompletely verified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

inferred — The new authority concerns garbage-collection finalizers on HelmReleases handled by the shared Application adapter. Each update writes one converted namespaced object. Get resolves request-context namespace and a configured name prefix, but Update subsequently uses the updated Application's identity fields; the effective tenant boundary therefore depends on external identity and authorization enforcement that was not verified.

Trust Boundaries and Controls

observed — Caller-controlled Application finalizers now reach the HelmRelease update sink through a fixed two-value allowlist and a live-state delta merge. This is new cleanup-finalizer authority, not arbitrary controller-finalizer authority. The existing update-validation hook remains before conversion; its effective policies and the external authorizer were not inspected.

Resilience and Maintainability Implications

observed — The tests model both cleanup orders and preserve Helm's finalizer until its removal, but use a fake client rather than production API validation or controllers. Both base and head retries retain the originally converted UID without an explicit comparison to the refreshed HelmRelease; safe handling of same-name replacement remains unverified and is not established as a PR-introduced defect.

Hardening Proposals

proposed — Validate the lifecycle against a real API server and controllers, including stale-UID rejection after same-name replacement and tenant-scoped authorization for finalizer updates. These checks would close the identified proof gaps rather than demonstrate an existing vulnerability.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: enabling Kubernetes garbage collection to finalize Applications through corrected finalizer handling.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create a new PR

Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

Aleksei Sviridkin (lexfrei)

europrinter (@yankawai) LGTM.

Business context: an Application deleted with foreground or orphan propagation never finishes deleting, because the garbage collector can finalize the shared UID through the Application endpoint, where the finalizer is not visible.

The bug is real on current main. A foreground delete of an Application puts foregroundDeletion on the HelmRelease, since Delete passes the caller's options through. The collector keys its graph by UID, and both objects share one, so the node can end up with the Application identity. Then removeFinalizer reads the Application, sees no finalizer and returns without a patch, and nothing else ever removes it from the HelmRelease.

The fix is at the right place and I don't see a smaller one. Only the two collector finalizers are exposed, and a change to them is applied as a delta against the list the update started from. So finalizers.fluxcd.io stays owned by the live object, and the conflict retry keeps concurrent writes.

What I ran on a trial merge with main (merges clean):

go test -race ./pkg/registry/apps/application/ passes.
With rest.go from main, all five new tests fail, as the description says.
Replacing the delta with the absolute list from the Application fails TestApplicationGarbageCollectionConflict. Dropping the deletion timestamp copy fails TestApplicationGarbageCollection and TestApplicationGarbageCollectionWithInPlaceUpdate.

Sign-off matches the author email, the only trailer is Assisted-by: LLM, and the description matches the diff, including the note that it was not run against a live garbage collector yet.

…pplications (#4683) ## What this PR does Backport of #4610 to `release-1.6`. `Update` on this branch has no `applyLiveMetadata`, so the finalizer merge and the copy of the deletion metadata are done inline, in `Update` and in its conflict retry. The tests are the same as on main. An Application and its HelmRelease share one UID, so the garbage collector keeps one graph node for both and may finalize it through the Application endpoint. The API never returned finalizers on an Application, so when the collector dropped `foregroundDeletion` or `orphan` through it, the HelmRelease kept `foregroundDeletion` after Helm had uninstalled the release and the deletion never completed. The Application now carries the two garbage collection finalizers, and an update that adds or removes one of them applies the same change to the HelmRelease. Controller finalizers such as `finalizers.fluxcd.io` stay owned by the live object. ### Validation - `go test ./pkg/registry/apps/application/...` passes on this branch. - Making the finalizer merge return the live list turns four tests red, the conflict-retry test among them. - No API types change, so code generation is not affected. ### Screenshots Not a UI change. ### Downstream repositories Walked the trigger map against the diff. It changes the Application REST handler and its test, with no chart, values, schema, CRD or `hack/` file, so no companion repository needs an update. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note fix(api): an application deleted with foreground or orphan propagation no longer leaves its HelmRelease stuck with the foregroundDeletion or orphan finalizer ```

europrinter (yankawai) requested review from IvanHunters, Andrei Kvapil (kvaps), Aleksei Sviridkin (lexfrei), Timofei Larkin (lllamnyp), myasnikovdaniil and Kirill Ilin (sircthulhu) as code owners September 30, 2026 14:19

github-actions Bot added size/L This PR changes 100-499 lines, ignoring generated files area/api Issues or PRs related to the cozystack-api aggregated API server kind/bug Categorizes issue or PR as related to a bug labels Sep 30, 2026

Aleksei Sviridkin (lexfrei) approved these changes Sep 30, 2026

View reviewed changes

Aleksei Sviridkin (lexfrei) enabled auto-merge September 30, 2026 15:10

Aleksei Sviridkin (lexfrei) merged commit 52c74be into cozystack:main Sep 30, 2026
55 checks passed

europrinter (yankawai) mentioned this pull request Oct 1, 2026

[Backport release-1.6] fix(api): let the garbage collector finalize Applications #4683

Merged

1 of 12 tasks

myasnikovdaniil mentioned this pull request Oct 14, 2026

[Backport release-1.6] fix(api): preserve foreign HelmRelease metadata and repair empty required fields #4919

Merged

1 of 11 tasks

Navigation Menu

Repository navigation

fix(api): let the garbage collector finalize Applications - #4610

fix(api): let the garbage collector finalize Applications#4610
Aleksei Sviridkin (lexfrei) merged 1 commit into
cozystack:mainfrom
yankawai:fix/application-garbage-collection

europrinter (yankawai) commented Sep 30, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Sep 30, 2026 •

edited

Loading

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

Aleksei Sviridkin (lexfrei) left a comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

europrinter (yankawai) commented Sep 30, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does

Screenshots

Downstream repositories

Release note

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Sep 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Security Architecture Review

❌ Failed checks (1 warning)

Uh oh!

Aleksei Sviridkin (lexfrei) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

europrinter (yankawai) commented Sep 30, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Sep 30, 2026 •

edited

Loading