Skip to content

Conversation

@majiayu000
Copy link
Contributor

While fixing the "stuck finalizer" issue in PR #658, we identified that the root cause was the deletion order.

This PR enforces that all HealthRecords associated with a Promise are fully deleted (and their finalizers removed) BEFORE the Promise controller deletes the RBAC resources (ClusterRole/Bindings). This prevents HealthRecords from getting stuck in a "zombie" state where they cannot be finalized due to missing permissions.

Closes the architectural gap identified in #658 discussion.

@ChunyiLyu
Copy link
Member

ChunyiLyu commented Jan 8, 2026

Thanks for creating a PR @majiayu000. Is this one meant to fix issue #628? It that's the case, would you mind closing the previous PR you have for the same issue #658?

Linter has failed for spelling check. Could you please fit it? If you would like to run the linter and the unit tests locally, you can simply run them with make targets test and lint within the project root.

@majiayu000
Copy link
Contributor Author

Thanks @ChunyiLyu! Yes, this PR fixes issue #628. Creates a cleaner deletion order. I have fixed the linter spelling error and closed PR #658 as requested.

@ChunyiLyu ChunyiLyu self-assigned this Jan 12, 2026
@ChunyiLyu ChunyiLyu force-pushed the fix/deletion-ordering branch from d2edfd9 to fdabd84 Compare January 12, 2026 16:04
@ChunyiLyu
Copy link
Member

ChunyiLyu commented Jan 13, 2026

Hi @majiayu000 thanks for submitting a PR. After a closer look at the issue, I think the behavior documented in issue #628 is not what happens. I've updated the issue with the current behavior I have seen today. Feel free to take a look.

For this PR, I don't think it makes sense to block deletion of a promise for health records. We want the deletion of promise to be able to go through, and health records controller should be able to handle the case of a deleted promise gracefully. I'm going to close this PR for now because I think the fix will need to change how we handle delete of health records in general, and it's not suitable as a "good first issue".

@ChunyiLyu ChunyiLyu closed this Jan 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants