fix(hooks): always remove finalizers #754

agaudreault · 2025-08-12T22:09:58Z

Description

The new finalizer on hooks allow us to observe the hook state before Kubernetes tries to delete it. Multiple issue where found causing the finalizers not to be properly removed on hooks managed by the sync.

When using SyncFail phase hooks, if any hook fail to apply (sometimes happens with Replace=true or bad manifest), the finalizers will not be removed and the sync will be failed with ongoing running tasks. This is a problem in the gitops-engine logic.
When any resource fail to apply during a phase, the finalizers wont be removed from the existing hooks of that phase
When there is a "sync wave hook" error, the finalizers wont be removed from the existing hooks of that phase

Changes

ensure hook finalizers are always removed so hooks don’t get stuck by removing finalizers on any existing hook task before creating/starting a new one, and ensure the finalizer is removed before deleting BeforeHookCreation hooks.
Don’t preemptively fail a sync if failure hooks fail to apply. Wait for the failure hooks to complete before Failing. Otherwise, Argo CD will immediately retry while the SyncFail hooks are running.
Terminate ongoing hooks on errors/terminate
addresses a race between BeforeHookCreation and Replace=true.
Clarify docs
Add test for all finalizers termination case

Tests

Test sync all Delete policies & finalizers removed
Test sync existing hook with finalizers succeeds
Test sync running + terminate remove all finalizers and running hooks
Test sync fail + syncFailTasks
Test sync fail + syncFailTasks fail
test syncWaveHook fails with syncFailTasks
test race condition with BeforeHookCreation and Replace=true

Signed-off-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>

codecov · 2025-08-12T22:14:11Z

Codecov Report

❌ Patch coverage is 73.20574% with 56 lines in your changes missing coverage. Please review.
✅ Project coverage is 48.32%. Comparing base (8849c3f) to head (ef00d91).
⚠️ Report is 57 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/sync/sync_context.go	79.27%	30 Missing and 10 partials ⚠️
...kg/utils/kube/kubetest/mock_resource_operations.go	0.00%	16 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #754      +/-   ##
==========================================
- Coverage   54.26%   48.32%   -5.95%     
==========================================
  Files          64       64              
  Lines        6164     6639     +475     
==========================================
- Hits         3345     3208     -137     
- Misses       2549     3170     +621     
+ Partials      270      261       -9

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>

agaudreault · 2025-08-20T20:26:32Z

pkg/sync/helper_test.go

Move functions importing sync/common to a helper since they cannot be in utils/testing.
sync/common imports utils/kube for a type, utils/kube tests use utils/testing which would use sync/common creating a cycle.

agaudreault · 2025-08-20T20:28:40Z

pkg/sync/sync_context.go

 			sc.log.WithValues("task", nsCreateTask).Info("Creating namespace")
 			if sc.runTasks(nsSyncTasks, false) == failed {
-				sc.setOperationFailed(syncTasks{}, nsSyncTasks, "the namespace failed to apply")
+				sc.executeSyncFailPhase(syncTasks{}, nsSyncTasks, "the namespace failed to apply")


setOperationFailed was renamed to executeSyncFailPhase

agaudreault · 2025-08-20T20:30:28Z

pkg/sync/sync_context.go

+				sc.setResourceResult(task, task.syncStatus, common.OperationError, fmt.Sprintf("failed to get resource health: %v", err))
 			} else {
-				sc.setResourceResult(task, "", operationState, message)
+				sc.setResourceResult(task, task.syncStatus, operationState, message)


Always preserve the current syncStatus in resource results instead of deleting it.

agaudreault · 2025-08-20T20:32:54Z

pkg/sync/sync_context.go

 			sc.setOperationPhase(common.OperationSucceeded, "successfully synced (all tasks run)")
 		} else {
-			sc.setRunningPhase(remainingTasks, false)
+			sc.setRunningPhase(tasks, false)


fixed the incorrect message. When we are running, we care about the current running task, not the remaining tasks that are still pending.

Signed-off-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>

agaudreault · 2025-08-20T20:46:07Z

pkg/sync/sync_context.go

+	}
+}
+
+func (sc *syncContext) GetState() (common.OperationPhase, string, []common.ResourceSyncResult) {


Moved GetState(), no changes

Signed-off-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>

agaudreault · 2025-08-20T21:00:00Z

pkg/sync/sync_context.go

-	terminateSuccessful := true
-	sc.log.V(1).Info("terminating")
-	tasks, _ := sc.getSyncTasks()
-	for _, task := range tasks {


Part of terminateHooksPreemptively now

agaudreault · 2025-08-20T21:00:35Z

pkg/sync/sync_context.go

 	})
 }

-func (sc *syncContext) updateResource(task *syncTask) error {


pkg/sync/doc.go

Co-authored-by: Dan Garfield <dan@codefresh.io>

Signed-off-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>

sonarqubecloud · 2025-09-02T17:12:08Z

Quality Gate passed

Issues
31 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
28.2% Duplication on New Code

See analysis details on SonarQube Cloud

ppapapetrou76 · 2025-09-11T08:09:44Z

The gitops-engine repository is migrating to https://github.com/argoproj/argo-cd.

Can you please update the PR by resolving the conflicts / If it's not relevant anymore, feel free to close it
You can always re-open it when the migration is over

agaudreault added 5 commits August 11, 2025 13:24

clarify doc

12baa16

Signed-off-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>

do not preemptively fail sync if failure hooks fail to apply

03d77eb

Signed-off-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>

make sure finalizer is removed before deleting BeforeHookCreation hooks

60c298f

Signed-off-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>

terminate hooks on error/failures

6654e51

Signed-off-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>

always remove finalizers on existing task before creation

9f490be

Signed-off-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>

agaudreault added 20 commits August 13, 2025 16:13

cleanup status update on terminate

412d96b

Signed-off-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>

handle sync failure on manifest apply

230a1c0

Signed-off-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>

fix test

cccd178

Signed-off-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>

clarify doc

8c43f14

Signed-off-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>

typo

5399bd1

Signed-off-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>

Merge remote-tracking branch 'upstream/master' into fix-hook-finalizer

eda44c9

race condition on BeforeHookCreation and replace

dc9311d

Signed-off-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>

log

4416a7e

Signed-off-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>

review

fa0bdc3

Signed-off-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>

unit tests

b652e65

Signed-off-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>

clean tests

a0bf17e

Signed-off-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>

check deleted

3286e06

Signed-off-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>

some tests

f633932

Signed-off-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>

move tests

0201c5a

Signed-off-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>

cleanup tests

3d2aa95

Signed-off-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>

docs

b39a9c8

Signed-off-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>

import cycle

9da34cc

Signed-off-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>

fix cycle in import

13fa1f3

Signed-off-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>

fix unit test

9c694ed

Signed-off-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>

revert hook finalizer const move

1cec50b

Signed-off-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>

agaudreault commented Aug 20, 2025

View reviewed changes

fix pending msg

bc80f53

Signed-off-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>

agaudreault commented Aug 20, 2025

View reviewed changes

more comments

9bd0a63

Signed-off-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>

agaudreault commented Aug 20, 2025

View reviewed changes

pkg/sync/sync_context.go

})

}

func (sc *syncContext) updateResource(task *syncTask) error {

Copy link

Member Author

agaudreault Aug 20, 2025

Choose a reason for hiding this comment

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

moved

agaudreault marked this pull request as ready for review August 20, 2025 21:04

agaudreault requested a review from a team as a code owner August 20, 2025 21:04

agaudreault mentioned this pull request Aug 20, 2025

test: updating e2e hook finalizer argoproj/argo-cd#24215

Draft

todaywasawesome reviewed Aug 21, 2025

View reviewed changes

pkg/sync/doc.go Show resolved Hide resolved

Update pkg/sync/doc.go

8af49be

Co-authored-by: Dan Garfield <dan@codefresh.io>

This was referenced Aug 22, 2025

fix(hooks): always remove finalizers on create if hook exists #761

Merged

fix(hooks): always remove finalizers on create if hook exists #762

Merged

agaudreault mentioned this pull request Sep 2, 2025

fix(hooks): always remove finalizers on create if hook exists #770

Merged

remove finalizer from completed hooks on terminate

ef00d91

Signed-off-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>

agaudreault marked this pull request as draft September 9, 2025 21:30

agaudreault mentioned this pull request Sep 15, 2025

ArgoCD fails on hook-finalizer (after upgrade to 3.x) argoproj/argo-cd#23226

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(hooks): always remove finalizers #754

fix(hooks): always remove finalizers #754

Uh oh!

agaudreault commented Aug 12, 2025 •

edited

Loading

Uh oh!

codecov bot commented Aug 12, 2025 •

edited

Loading

Uh oh!

agaudreault Aug 20, 2025

Uh oh!

agaudreault Aug 20, 2025

Uh oh!

agaudreault Aug 20, 2025

Uh oh!

agaudreault Aug 20, 2025

Uh oh!

agaudreault Aug 20, 2025

Uh oh!

agaudreault Aug 20, 2025

Uh oh!

agaudreault Aug 20, 2025

Uh oh!

Uh oh!

sonarqubecloud bot commented Sep 2, 2025

Uh oh!

ppapapetrou76 commented Sep 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fix(hooks): always remove finalizers #754

Are you sure you want to change the base?

fix(hooks): always remove finalizers #754

Uh oh!

Conversation

agaudreault commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

Tests

Uh oh!

codecov bot commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

agaudreault Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

agaudreault Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

agaudreault Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

agaudreault Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

agaudreault Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

agaudreault Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

agaudreault Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sonarqubecloud bot commented Sep 2, 2025

Quality Gate passed

Uh oh!

ppapapetrou76 commented Sep 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

agaudreault commented Aug 12, 2025 •

edited

Loading

codecov bot commented Aug 12, 2025 •

edited

Loading