Skip to content

fix(applicationset): prevent prune state leakage in RollingSync (fixes #27261)#27284

Closed
sahanajc4 wants to merge 2 commits intoargoproj:masterfrom
sahanajc4:fix/applicationset-prune-state-leak
Closed

fix(applicationset): prevent prune state leakage in RollingSync (fixes #27261)#27284
sahanajc4 wants to merge 2 commits intoargoproj:masterfrom
sahanajc4:fix/applicationset-prune-state-leak

Conversation

@sahanajc4
Copy link
Copy Markdown
Contributor

@sahanajc4 sahanajc4 commented Apr 10, 2026

## Problem

When using ApplicationSet with RollingSync strategy, changing `syncPolicy.automated.prune` from `true` to `false` does not take effect.

Subsequent sync operations continue to prune resources because the previous value (`prune: true`) persists in `status.operationState.operation.sync.prune`.

## Root Cause

- `SyncOperation.Prune` is defined as `bool`, which cannot distinguish between:
  - unset (field omitted)
  - explicitly set to false

- During RollingSync, if `prune` is not explicitly set in a new operation, the previous value from `operationState` may be reused.

## Solution

This PR fixes the issue by:

- Changing `SyncOperation.Prune` from `bool` to `*bool` to distinguish unset vs explicitly false
- Explicitly setting `prune` in every RollingSync-triggered sync operation:
  ```go
  Prune: &prune
  • Safely converting *bool to bool when passing to the sync engine to maintain compatibility:

    prune := false
    if syncOp.Prune != nil {
        prune = *syncOp.Prune
    }
  • Updating unit tests to use pointer semantics

  • Adding a test case covering prune toggle (true → false)

Result

  • prune=false is now correctly respected in subsequent RollingSync operations
  • Prevents unintended resource pruning after configuration changes
  • No changes to existing sync behavior beyond fixing the bug

Scope

  • No changes to reconciliation logic
  • No changes to gitops-engine APIs
  • Fix is limited to correct propagation of prune values

Testing

  • Updated existing unit tests for pointer semantics
  • Added test case: "Prune toggles from true to false"

Checklist

  • This is a bug fix
  • The title of the PR states what changed and the related issue number
  • The title of the PR conforms to the project conventions
  • I've included "Fixes ApplicationSet RollingSync cannot set operation.sync.prune from true to false #27261" in the description
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR
  • I have signed off all my commits as required by DCO
  • I have written unit tests for my change
  • My build is green (troubleshooting builds)
  • My new feature complies with the feature status guidelines
  • I have added a description of what this PR solves
  • Optional. My organization is added to USERS.md
  • Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into

Fixes #27261
Signed-off-by: sahanjc4 < sahanajavgal7@gmail.com >

@sahanajc4 sahanajc4 requested a review from a team as a code owner April 10, 2026 17:17
@bunnyshell
Copy link
Copy Markdown

bunnyshell Bot commented Apr 10, 2026

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

  • 🚀 /bns:deploy to deploy the environment

@sahanajc4 sahanajc4 changed the title fix(applicationset): prevent prune state leakage in RollingSync- chan… fix(applicationset): prevent prune state leakage in RollingSync (fixes #27261) Apr 10, 2026
@sahanajc4 sahanajc4 force-pushed the fix/applicationset-prune-state-leak branch 6 times, most recently from 49427b8 to 1700dc9 Compare April 14, 2026 07:28
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 14, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 63.37%. Comparing base (0737418) to head (1700dc9).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #27284      +/-   ##
==========================================
+ Coverage   63.34%   63.37%   +0.02%     
==========================================
  Files         415      415              
  Lines       56694    56701       +7     
==========================================
+ Hits        35914    35934      +20     
+ Misses      17396    17380      -16     
- Partials     3384     3387       +3     

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

…ge SyncOperation.Prune from bool to *bool to distinguish unset vs false- explicitly set prune in every RollingSync sync operation- safely convert *bool to bool when passing to sync engine- update tests to use pointer semantics- add test case for prune toggle (true -> false)Fixes issue where previous prune=true persisted across operations and caused unintended pruning.

Signed-off-by: Sahana <sahanajavgal7@gmail.com>
@sahanajc4 sahanajc4 force-pushed the fix/applicationset-prune-state-leak branch from 1700dc9 to db4ec78 Compare April 14, 2026 08:13
Signed-off-by: Sahana <sahanajavgal7@gmail.com>
@sahanajc4 sahanajc4 force-pushed the fix/applicationset-prune-state-leak branch from 0e9f3c4 to b91c8e9 Compare April 14, 2026 09:45
@sahanajc4 sahanajc4 closed this Apr 14, 2026
@sahanajc4 sahanajc4 deleted the fix/applicationset-prune-state-leak branch April 14, 2026 09:52
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.

ApplicationSet RollingSync cannot set operation.sync.prune from true to false

1 participant