fix: Hydrator wipes out entire branch when multiple Applications hydrate to the same branch with different path (fixes #24179) #24185

adityaraj178 · 2025-08-18T09:02:43Z

Hydrator wipes out entire branch when multiple Applications hydrate to the same branch with different path.

Description:

Fixes an issue where the Hydrator deletes the entire branch contents when multiple Applications hydrate into the same branch but with different paths.
Previously, CommitHydratedManifests would clean the full branch before writing manifests, which caused conflicts and data loss if multiple apps targeted the same branch under different directories.
This change ensures that only the specific path for the current Application is cleaned and re-written, leaving other paths intact.

Closes:

Closes Hydrator wipes out entire branch when multiple Applications hydrate to the same branch with different path #24179

Why this PR is necessary:
Without this fix, hydrating multiple Applications to the same branch is destructive. This makes source hydrator unsafe in multi-application setups. With this fix, Applications can hydrate independently to different directories inside the same branch.

Demonstartion video: https://github.com/user-attachments/assets/c9c1a854-b7a2-4467-87f2-eeb6850dba52

Checklist:

bunnyshell · 2025-08-18T09:02:48Z

🔴 Preview Environment stopped on Bunnyshell

See: Environment Details | Pipeline Logs

Available commands (reply to this comment):

🔵 /bns:start to start the environment
🚀 /bns:deploy to redeploy the environment
❌ /bns:delete to remove the environment

codecov · 2025-08-18T09:43:57Z

Codecov Report

❌ Patch coverage is 69.23077% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.14%. Comparing base (8c8902b) to head (53fb859).
⚠️ Report is 21 commits behind head on master.

Files with missing lines	Patch %	Lines
commitserver/commit/commit.go	70.00%	2 Missing and 1 partial ⚠️
util/git/client.go	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #24185      +/-   ##
==========================================
+ Coverage   60.11%   60.14%   +0.02%     
==========================================
  Files         347      348       +1     
  Lines       59856    59894      +38     
==========================================
+ Hits        35985    36025      +40     
+ Misses      20984    20980       -4     
- Partials     2887     2889       +2

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

nitishfy · 2025-08-18T14:07:23Z

commitserver/commit/commit.go

+				return "", "", fmt.Errorf("failed to clear path %s: %w", targetPath, err)
+			}
+			logCtx.Debugf("Recreating directory %s", targetPath)
+			if err := os.MkdirAll(targetPath, 0o755); err != nil {


Doesn't WriteForPaths creates a directory already?

Yes, WriteForPaths does attempt to create the target directory for each manifest path before writing the files. However, there are two reasons I explicitly ensure the directory exists in handleCommitRequest:

Path Deletion Race: Immediately after deleting a directory with os.RemoveAll, there can be a brief moment (especially under heavy parallelization or on some filesystems) where the directory is still being cleaned up. Explicitly recreating it guarantees a safe write regardless of timing.

Top-Level Metadata: The top-level hydrator.metadata is written to the root (dirPath), and in edge cases (such as root path hydration), it’s important to be sure dirPath wasn't accidentally removed or left missing.

Path Deletion Race: Immediately after deleting a directory with os.RemoveAll, there can be a brief moment (especially under heavy parallelization or on some filesystems) where the directory is still being cleaned up. Explicitly recreating it guarantees a safe write regardless of timing.

Did you try these changes locally? Woudln't you get an error if you're trying to create the same directory again provided it has already been created once?

Hello @nitishfy, i have run it locally and it's working good. The main reason i recreates the directory is because os.RemoveAll(targetPath) will remove everything at targetPath, including all files and the directory itself. So whenever i am calling os.RemoveAll(), i am recreating the directory after that.

#24185 (review)

nitishfy

Something feels a bit off about changes....Please consider putting a demonstration video if possible OR prefer adding more tests to validate this behaviour. Thanks!

nitishfy · 2025-08-18T14:24:54Z

Not related to PR changes.....

I see someone already asked about working on the issue. Although the person wasn't assigned but please get an issue assigned beforehand and raise a fix then. There are chances that the other person might have started working on the issue that would lead to wastage of efforts at the end.

nitishfy · 2025-08-19T17:54:18Z

commitserver/commit/commit.go

+			logCtx.Debug("Using root directory for manifests, no directory removal needed")
+		} else {
+			logCtx.Debugf("Clearing path %s", targetPath)
+			if err := os.RemoveAll(targetPath); err != nil {


Suggested change

if err := os.RemoveAll(targetPath); err != nil {

if err := os.RemoveAll(targetPath); err != nil {

Why aren't we using gitClient.RemoveContents() anymore? I think we should modify the git client call than calling the os.RemoveAll function here.

nitishfy

This PR is a little bit related to fixing #23944. I'd suggest going through the PR that fixes that issue and take some inspiration from that. For now, this PR needs a lot of modification and I'd prefer having a demonstration video at the end to make sure everything is working before finally approving.

Also, if you want, you can bring this PR in Argo CD community call happening at Thursday (8:45 PM IST)

adityaraj178 · 2025-08-21T15:05:46Z

This PR is a little bit related to fixing #23944. I'd suggest going through the PR that fixes that issue and take some inspiration from that. For now, this PR needs a lot of modification and I'd prefer having a demonstration video at the end to make sure everything is working before finally approving.

Also, if you want, you can bring this PR in Argo CD community call happening at Thursday (8:45 PM IST)

Thanks, @nitishfy, for the reply and sharing the related issue #23944. I see that both issues are quite similar.

I believe stopping users from providing the root directory as a path might be restrictive since some users may want to store manifests directly in the root. Instead, focusing on cleaning only subdirectories during commits should solve the problem—since commits in the root directory will only modify files like README.md, manifests.yaml, and hydrator.metadata, leaving CI/CD config files unaffected.I will prepare and share a demonstration video to showcase this approach.

Thanks also for the insight on preferring gitClient.RemoveContents() over os.RemoveAll().
Lastly, I will ensure to get issues assigned before working on them next time.

Thanks again for the valuable feedback!

adityaraj178 · 2025-08-21T17:41:02Z

Demonstartion video: https://github.com/user-attachments/assets/c9c1a854-b7a2-4467-87f2-eeb6850dba52

Clip 1: The hydrator targets the subdirectory test, which contains .txt files. These files are properly removed and replaced during hydration.

Clip 2: The hydrator targets the root path, where a critical file config.yaml exists. This file remains unaffected by the hydration process, ensuring important root-level config files are preserved.

…ple Applications hydrate to the same branch with different path Signed-off-by: Aditya Raj <[email protected]>

Signed-off-by: Aditya Raj <[email protected]>

adityaraj178 requested a review from a team as a code owner August 18, 2025 09:02

adityaraj178 force-pushed the fix-24179 branch from 725bd4d to 02dd790 Compare August 18, 2025 09:47

nitishfy reviewed Aug 18, 2025

View reviewed changes

adityaraj178 force-pushed the fix-24179 branch 4 times, most recently from 40c0a5a to f95155e Compare August 19, 2025 17:37

nitishfy reviewed Aug 19, 2025

View reviewed changes

adityaraj178 force-pushed the fix-24179 branch from 50dacc3 to 332e638 Compare August 21, 2025 13:47

adityaraj178 force-pushed the fix-24179 branch from 332e638 to f017621 Compare August 21, 2025 20:12

adityaraj178 requested a review from a team as a code owner August 21, 2025 20:12

adityaraj178 force-pushed the fix-24179 branch 3 times, most recently from 0a185eb to aa4892c Compare August 22, 2025 04:58

adityaraj178 added 3 commits August 24, 2025 11:46

fix: issue argoproj#24179 Hydrator wipes out entire branch when multi…

92d5991

…ple Applications hydrate to the same branch with different path Signed-off-by: Aditya Raj <[email protected]>

Address review comments: updated tests and dir handling

70ec4a0

Signed-off-by: Aditya Raj <[email protected]>

using git based cleaner instead of os based

53fb859

Signed-off-by: Aditya Raj <[email protected]>

adityaraj178 force-pushed the fix-24179 branch from aa4892c to 53fb859 Compare August 24, 2025 11:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: Hydrator wipes out entire branch when multiple Applications hydrate to the same branch with different path (fixes #24179) #24185

fix: Hydrator wipes out entire branch when multiple Applications hydrate to the same branch with different path (fixes #24179) #24185

adityaraj178 commented Aug 18, 2025 •

edited

Loading

Uh oh!

bunnyshell bot commented Aug 18, 2025 •

edited

Loading

Uh oh!

codecov bot commented Aug 18, 2025 •

edited

Loading

Uh oh!

nitishfy Aug 18, 2025

Uh oh!

adityaraj178 Aug 18, 2025

Uh oh!

nitishfy Aug 18, 2025

Uh oh!

adityaraj178 Aug 18, 2025

Uh oh!

nitishfy Aug 18, 2025

Uh oh!

nitishfy left a comment •

edited

Loading

Uh oh!

nitishfy commented Aug 18, 2025

Uh oh!

nitishfy Aug 19, 2025

Uh oh!

nitishfy left a comment

Uh oh!

adityaraj178 commented Aug 21, 2025

Uh oh!

adityaraj178 commented Aug 21, 2025

Uh oh!

Uh oh!

	if err := os.RemoveAll(targetPath); err != nil {
	if err := os.RemoveAll(targetPath); err != nil {

fix: Hydrator wipes out entire branch when multiple Applications hydrate to the same branch with different path (fixes #24179) #24185

Are you sure you want to change the base?

fix: Hydrator wipes out entire branch when multiple Applications hydrate to the same branch with different path (fixes #24179) #24185

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

🔴 Preview Environment stopped on Bunnyshell

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

nitishfy Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

adityaraj178 Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

nitishfy Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

adityaraj178 Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

nitishfy Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

nitishfy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nitishfy commented Aug 18, 2025

Uh oh!

nitishfy Aug 19, 2025

Choose a reason for hiding this comment

Uh oh!

nitishfy left a comment

Choose a reason for hiding this comment

Uh oh!

adityaraj178 commented Aug 21, 2025

Uh oh!

adityaraj178 commented Aug 21, 2025

Uh oh!

Uh oh!

adityaraj178 commented Aug 18, 2025 •

edited

Loading

bunnyshell bot commented Aug 18, 2025 •

edited

Loading

codecov bot commented Aug 18, 2025 •

edited

Loading

nitishfy left a comment •

edited

Loading