Skip to content

Eliminate double fetch per sync cycle#577

Merged
rdimitrov merged 1 commit intomainfrom
fix-double-fetch-sync-pipeline
Mar 17, 2026
Merged

Eliminate double fetch per sync cycle#577
rdimitrov merged 1 commit intomainfrom
fix-double-fetch-sync-pipeline

Conversation

@lujunsan
Copy link
Copy Markdown
Contributor

Summary

  • Remove CurrentHash from RegistryHandler interface — it was a broken
    abstraction that promised a lightweight hash check but performed a full
    fetch in all three implementations (git, file, API)
  • IsDataChanged now calls FetchRegistry directly and returns the
    FetchResult alongside the changed/unchanged boolean
  • ShouldSync threads the FetchResult through to PerformSync, which
    reuses it instead of fetching again — reducing fetches per sync cycle
    from two to one for all source types
  • The fallback path (prefetched == nil) is preserved for error cases

Test plan

  • Unit tests pass (task test)
  • Manually verified with docker-compose using a git source:
    • First sync: one clone, prefetch reused, data stored
    • Subsequent sync, no upstream change: one clone for hash check, shouldSync:false, no store
    • Subsequent sync, different source data: one clone, prefetch reused, new hash and server count stored

FIxes #548

Signed-off-by: lujunsan <luisjuncaldev@gmail.com>
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 53.73134% with 31 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.59%. Comparing base (4ba9a30) to head (cada809).

Files with missing lines Patch % Lines
internal/sync/manager.go 66.66% 13 Missing and 2 partials ⚠️
internal/sync/coordinator/coordinator.go 0.00% 8 Missing ⚠️
internal/sync/mocks/mock_manager.go 0.00% 7 Missing ⚠️
internal/sync/detectors.go 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #577      +/-   ##
==========================================
+ Coverage   57.54%   57.59%   +0.05%     
==========================================
  Files          94       94              
  Lines        8460     8443      -17     
==========================================
- Hits         4868     4863       -5     
+ Misses       3261     3249      -12     
  Partials      331      331              

☔ 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.

@rdimitrov rdimitrov merged commit 5f301c7 into main Mar 17, 2026
12 checks passed
@rdimitrov rdimitrov deleted the fix-double-fetch-sync-pipeline branch March 17, 2026 11:53
rdimitrov added a commit that referenced this pull request Mar 19, 2026
Reimplements main's #577 for the v1 branch. Previously each sync cycle
fetched registry data twice: once in IsDataChanged (via CurrentHash) to
check for changes, and again in PerformSync (via FetchRegistry). Now
IsDataChanged calls FetchRegistry directly and threads the result
through ShouldSync → PerformSync, eliminating the redundant fetch.

- Remove CurrentHash from RegistryHandler interface and all implementations
- IsDataChanged returns (bool, *FetchResult, error)
- ShouldSync returns (Reason, *FetchResult)
- PerformSync accepts optional prefetched *FetchResult

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
rdimitrov added a commit that referenced this pull request Mar 19, 2026
## Summary
- Reimplements main's #577 for the v1 branch line
- Removes redundant `CurrentHash` method from `RegistryHandler`
interface — `IsDataChanged` now calls `FetchRegistry` directly and
returns the fetched data
- Threads prefetched `FetchResult` from `ShouldSync` → `PerformSync`,
eliminating the second fetch per sync cycle
- Adds `TestDefaultSyncManager_PerformSync_WithPrefetched` to verify
prefetched data path

## Test plan
- [x] Build passes
- [x] Lint passes (0 issues)
- [x] All sources and sync tests pass
- [ ] CI passes

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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.

Optimise the cloning of git registry sources

4 participants