Skip to content

fix: git sync is not pulling full folders, no longer deploy on creation of sync#2239

Merged
kmendell merged 1 commit intomainfrom
fix/git-folder-sync-issues
Apr 6, 2026
Merged

fix: git sync is not pulling full folders, no longer deploy on creation of sync#2239
kmendell merged 1 commit intomainfrom
fix/git-folder-sync-issues

Conversation

@kmendell
Copy link
Copy Markdown
Member

@kmendell kmendell commented Apr 6, 2026

Checklist

  • This PR is not opened from my fork’s main branch

What This PR Implements

Fixes: #2224

Changes Made

Testing Done

  • Development environment started: ./scripts/development/dev.sh start
  • Frontend verified at http://localhost:3000
  • Backend verified at http://localhost:3552
  • Manual testing completed (describe):
  • No linting errors (e.g., just lint all)
  • Backend tests pass: just test backend

AI Tool Used (if applicable)

AI Tool:
Assistance Level:
What AI helped with:
I reviewed and edited all AI-generated output:
I ran all required tests and manually verified changes:

Additional Context

Disclaimer Greptiles Reviews use AI, make sure to check over its work.

To better help train Greptile on our codebase, if the comment is useful and valid Like the comment, if its not helpful or invalid Dislike

To have Greptile Re-Review the changes, mention greptileai.

Greptile Summary

[Linus Torvalds Mode] Someone actually read their previous review comments — I'm as shocked as you are. This PR fixes two real problems: the directory sync was only pulling the compose file instead of the full directory tree, and a newly-created sync was unconditionally triggering a redeploy even before any containers existed to redeploy. Both bugs traced back to missing abstractions and a lack of change-detection logic that this PR now introduces via a staging-area pattern.

What changed:

  • prepareSyncSource is extracted from PerformSync, and the deferred cleanup is now correctly registered before the error check — fixing the repo-dir leak on compose-file-not-found paths flagged in a previous review
  • A new staging flow (stageDirectorySyncInternal) copies the live project to a temp dir, applies removals, writes the new repo files, validates the compose tree, and only then atomically promotes the stage to the live path with a backup/rollback
  • directorySyncContentsChangedInternal compares repo content against the live project directory to gate redeployments; a new project is Stopped so redeployIfRunningAfterSync no-ops correctly
  • updateDirectorySyncProjectInternal now removes the backup after a confirmed DB update — fixing the backup-deleted-before-DB-write ordering bug from a prior review thread
  • WalkDirectory uses fs.WalkDir over os.OpenRoot(syncDir).FS() to recursively traverse the full directory tree, fixing the "not pulling full folders" root cause
  • Tests added for both project-creation and project-update directory sync paths

Remaining concern:

  • In createDirectorySyncProjectInternal, three sequential DB writes (Create, two Updates) operate with no wrapping transaction. A failure on the second or third write leaves an orphaned Project row in the database that will cause duplicate projects on the next sync run — worth fixing before this sees heavy use.

Confidence Score: 5/5

[Linus Torvalds Mode] Congratulations, the author actually addressed prior review feedback — color me reluctantly impressed. The two P1s from previous rounds (leaked repo dir and backup-deleted-before-DB-write) are fixed, the Internal suffix convention is now followed on all new package-level helpers, and the staging logic is structurally sound. Safe to merge; the one remaining DB partial-failure scenario is a low-probability P2.

Normally I'd be sharpening my red pen right now, but the previously flagged critical issues are genuinely resolved. The only remaining finding is a low-probability DB split-brain during project creation (three sequential writes, no transaction) that degrades gracefully — the sync retries and creates a new project, leaving an orphaned row. That's P2, not a blocker. By the scoring guidance, all-P2 lands at 5/5.

Stare the hardest at createDirectorySyncProjectInternal (~line 1130) in gitops_sync_service.go — the transaction-less triple DB write is the only spot that can silently corrupt state under a partial failure.

Fix All in Codex

Prompt To Fix All With AI
This is a comment left during a code review.
Path: backend/internal/services/gitops_sync_service.go
Line: 1162-1173

Comment:
**Orphaned project row on partial DB failure**

Three sequential DB writes stitched together with string and prayers — bold move for code that touches both the filesystem and the database. Did we lose a bet, or did someone just really hate the concept of atomicity?

After `s.db.Create(project)` succeeds at line 1162, if `Update("project_id")` at line 1167 or `Update("gitops_managed_by")` at line 1171 fails, the function returns an error with no rollback of the already-created project record. The orphaned `Project` row stays in the DB with the filesystem intact at `projectPath`. On the next sync run `sync.ProjectID` is still `nil`, so `createDirectorySyncProjectInternal` runs again — creating a second project at a deduplicated path and permanently orphaning the first.

Wrapping all three writes in `s.db.WithContext(ctx).Transaction(func(tx *gorm.DB) error { ... })` fixes this: a failure on either update rolls back the `Create` atomically, and the `os.RemoveAll` cleanup path at line 1163 can be applied uniformly on any transaction error.

The orphaned rows will haunt the `projects` table until someone writes a janitor migration — and nobody wants to write that incident report.

**Rule Used:** # Go Development Patterns

**What:** Code should p... ([source](https://app.greptile.com/review/custom-context?memory=c1082b6a-5fdc-4db8-8419-8a71ccd57636))

How can I resolve this? If you propose a fix, please make it concise.

Reviews (3): Last reviewed commit: "fix: git sync is not pulling full folder..." | Re-trigger Greptile

Context used:

  • Rule used - # Go Development Patterns

What: Code should p... (source)

  • Rule used - # Golang Pro

Senior Go developer with deep expert... (source)

Copy link
Copy Markdown
Member Author

kmendell commented Apr 6, 2026

This stack of pull requests is managed by Graphite. Learn more about stacking.

@kmendell
Copy link
Copy Markdown
Member Author

kmendell commented Apr 6, 2026

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@kmendell kmendell marked this pull request as ready for review April 6, 2026 02:03
@kmendell kmendell requested a review from a team April 6, 2026 02:03
@getarcaneappbot
Copy link
Copy Markdown
Contributor

getarcaneappbot commented Apr 6, 2026

Container images for this PR have been built successfully!

  • Manager: ghcr.io/getarcaneapp/arcane:pr-2239
  • Agent: ghcr.io/getarcaneapp/arcane-headless:pr-2239

Built from commit 48bc996

@kmendell kmendell force-pushed the fix/git-folder-sync-issues branch 2 times, most recently from 5d755e2 to ca2f548 Compare April 6, 2026 02:18
@kmendell kmendell force-pushed the fix/git-folder-sync-issues branch from ca2f548 to eb5f627 Compare April 6, 2026 02:35
@kmendell kmendell force-pushed the fix/git-folder-sync-issues branch from eb5f627 to 48bc996 Compare April 6, 2026 02:49
@kmendell kmendell merged commit 6ceefc8 into main Apr 6, 2026
21 checks passed
@kmendell kmendell deleted the fix/git-folder-sync-issues branch April 6, 2026 02:49
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.

🐞 Bug: Git Sync Folder Issue [next-bfc13d9e]

2 participants