From 4a1cacfcaf286cbad156abefe9daf55ca12bb63a Mon Sep 17 00:00:00 2001 From: jonnii Date: 2026年6月22日 22:47:16 -0400 Subject: [PATCH] refactor(api): extract Syncer.SyncRepo for per-repo sync triggers Pull the per-repo unit of work out of the interval loop so a future webhook receiver can trigger a refresh for a single repo by its GitHub coordinates, not just the ticker iterating over managed entries. - Registry.FindManaged(owner, name) resolves a managed mirror case-insensitively; it deliberately never returns the unmanaged -cwd working repo, which must not be mirror-fetched (mirror-fetch detaches HEAD). - Syncer.syncEntry now returns an error instead of logging-and-swallowing; syncOnce logs failures, and the new SyncRepo(owner, name) wrapper resolves the entry and delegates, returning ErrRepoNotManaged for un-onboarded repos. Pure refactor: the interval loop behaves identically. Sets up the webhook-driven refresh stack. --- internal/api/registry/registry.go | 20 +++++++++++ internal/api/registry/registry_test.go | 26 ++++++++++++++ internal/api/reposync/syncer.go | 34 ++++++++++++++---- internal/api/reposync/syncer_test.go | 48 ++++++++++++++++++++++++++ 4 files changed, 122 insertions(+), 6 deletions(-) diff --git a/internal/api/registry/registry.go b/internal/api/registry/registry.go index 8e492423f..b02a131a3 100644 --- a/internal/api/registry/registry.go +++ b/internal/api/registry/registry.go @@ -12,6 +12,7 @@ import ( "log/slog" "regexp" "sort" + "strings" "sync" "time" @@ -225,6 +226,25 @@ func (r *Registry) Get(id string) (*RepoEntry, bool) { return e, ok } +// FindManaged returns the managed entry whose GitHub owner/name match the +// arguments case-insensitively, or false when none does. The interval sync +// loop and the webhook receiver use it to map a remote-change signal (a push +// for owner/name) onto the local checkout to refresh. +// +// Only managed mirrors are considered: the unmanaged -cwd working repo must +// never be selected here, because the sync path mirror-fetches into a detached +// HEAD and would corrupt a real working tree (see safety-invariants.md). +func (r *Registry) FindManaged(owner, name string) (*RepoEntry, bool) { + r.mu.RLock() + defer r.mu.RUnlock() + for _, e := range r.entries { + if e.Managed && strings.EqualFold(e.Owner, owner) && strings.EqualFold(e.Name, name) { + return e, true + } + } + return nil, false +} + // List returns every entry sorted by ID so callers (and tests) get a stable // ordering for serialization. func (r *Registry) List() []*RepoEntry { diff --git a/internal/api/registry/registry_test.go b/internal/api/registry/registry_test.go index d261f6768..0e15424dd 100644 --- a/internal/api/registry/registry_test.go +++ b/internal/api/registry/registry_test.go @@ -79,6 +79,32 @@ func TestRegistry_ListReturnsSortedByID(t *testing.T) { require.Equal(t, "c", got[2].ID) } +func TestRegistry_FindManaged(t *testing.T) { + t.Parallel() + r := New() + require.NoError(t, r.Add(&RepoEntry{ID: "managed", Managed: true, Owner: "Octo", Name: "Widget"})) + require.NoError(t, r.Add(&RepoEntry{ID: "unmanaged", Owner: "octo", Name: "dev"})) + + t.Run("matches owner/name case-insensitively", func(t *testing.T) { + t.Parallel() + e, ok := r.FindManaged("octo", "widget") + require.True(t, ok) + require.Equal(t, "managed", e.ID) + }) + + t.Run("skips unmanaged entries", func(t *testing.T) { + t.Parallel() + _, ok := r.FindManaged("octo", "dev") + require.False(t, ok, "the unmanaged -cwd repo must never be selected for mirror-fetch") + }) + + t.Run("returns false when no repo matches", func(t *testing.T) { + t.Parallel() + _, ok := r.FindManaged("octo", "missing") + require.False(t, ok) + }) +} + func TestRegistry_Len(t *testing.T) { t.Parallel() r := New() diff --git a/internal/api/reposync/syncer.go b/internal/api/reposync/syncer.go index 2c88b72d4..99da45d4c 100644 --- a/internal/api/reposync/syncer.go +++ b/internal/api/reposync/syncer.go @@ -6,6 +6,8 @@ package reposync import ( "context" + "errors" + "fmt" "log/slog" "time" @@ -14,6 +16,11 @@ import ( "github.com/getstackit/stackit/internal/utils" ) +// ErrRepoNotManaged is returned by SyncRepo when no managed registry entry +// matches the requested owner/name — e.g. a webhook arrives for a repo that +// was never onboarded, or for the unmanaged -cwd dev repo. +var ErrRepoNotManaged = errors.New("reposync: repo not managed") + // defaultWorkers bounds concurrent fetches per pass. Fetches are network-bound, // so a small pool is gentle on the remote and the file-descriptor table. const defaultWorkers = 4 @@ -82,23 +89,38 @@ func (s *Syncer) syncOnce(ctx context.Context) { if ctx.Err() != nil { return } - s.syncEntry(ctx, e) + if err := s.syncEntry(ctx, e); err != nil { + slog.Warn("sync: entry failed", "repo", e.ID, "error", err) + } }) } -func (s *Syncer) syncEntry(ctx context.Context, e *registry.RepoEntry) { +// SyncRepo mirror-fetches and refreshes the single managed entry matching +// owner/name. It is the per-repo unit of work shared by the interval loop and +// the webhook receiver: the loop calls syncEntry directly over the entries it +// already holds, while the webhook path arrives with only GitHub coordinates +// and resolves the entry here. Returns ErrRepoNotManaged when no managed repo +// matches, so a webhook for an un-onboarded repo is a clean no-op. +func (s *Syncer) SyncRepo(ctx context.Context, owner, name string) error { + e, ok := s.reg.FindManaged(owner, name) + if !ok { + return fmt.Errorf("%w: %s/%s", ErrRepoNotManaged, owner, name) + } + return s.syncEntry(ctx, e) +} + +func (s *Syncer) syncEntry(ctx context.Context, e *registry.RepoEntry) error { token := "" if s.provider != nil { t, err := s.provider.InstallationToken(ctx, e.Owner, e.Name) if err != nil { - slog.Warn("sync: installation token unavailable; skipping", "repo", e.ID, "error", err) - return + return fmt.Errorf("installation token: %w", err) } token = t } if err := s.fetch(ctx, e.RepoRoot, e.Remote, token); err != nil { - slog.Warn("sync: fetch failed", "repo", e.ID, "error", err) - return + return fmt.Errorf("fetch: %w", err) } s.refresh(e) + return nil } diff --git a/internal/api/reposync/syncer_test.go b/internal/api/reposync/syncer_test.go index d2313057f..f4bb05907 100644 --- a/internal/api/reposync/syncer_test.go +++ b/internal/api/reposync/syncer_test.go @@ -95,6 +95,54 @@ func TestSyncEntryNilProviderUsesEmptyToken(t *testing.T) { require.Equal(t, "", gotToken, "no provider means an unauthenticated (public) fetch") } +func TestSyncRepoFetchesMatchingManagedEntry(t *testing.T) { + t.Parallel() + reg := registry.New() + require.NoError(t, reg.Add(managedEntry("m", "Octo", "Widget", "/r/m"))) + + var gotToken string + refreshed := false + s := New(reg, fakeProvider{token: "inst-token"}, time.Minute) + s.fetch = func(_ context.Context, _, _, token string) error { gotToken = token; return nil } + s.refresh = func(*registry.RepoEntry) { refreshed = true } + + // Casing differs from the entry to prove the lookup is case-insensitive, + // matching how a webhook payload may present the coordinates. + require.NoError(t, s.SyncRepo(context.Background(), "octo", "widget")) + require.Equal(t, "inst-token", gotToken) + require.True(t, refreshed) +} + +func TestSyncRepoUnknownRepoReturnsErrNotManaged(t *testing.T) { + t.Parallel() + reg := registry.New() + require.NoError(t, reg.Add(managedEntry("m", "o", "n", "/r"))) + + fetchCalled := false + s := New(reg, fakeProvider{token: "t"}, time.Minute) + s.fetch = func(context.Context, string, string, string) error { fetchCalled = true; return nil } + s.refresh = func(*registry.RepoEntry) {} + + err := s.SyncRepo(context.Background(), "other", "repo") + require.ErrorIs(t, err, ErrRepoNotManaged) + require.False(t, fetchCalled, "an un-onboarded repo must not trigger a fetch") +} + +func TestSyncRepoPropagatesFetchError(t *testing.T) { + t.Parallel() + reg := registry.New() + require.NoError(t, reg.Add(managedEntry("m", "o", "n", "/r"))) + + refreshed := false + s := New(reg, fakeProvider{token: "t"}, time.Minute) + s.fetch = func(context.Context, string, string, string) error { return errors.New("boom") } + s.refresh = func(*registry.RepoEntry) { refreshed = true } + + err := s.SyncRepo(context.Background(), "o", "n") + require.Error(t, err) + require.False(t, refreshed, "a failed fetch must not refresh stale state") +} + func TestRunStopsOnContextCancel(t *testing.T) { t.Parallel() s := New(registry.New(), nil, time.Hour)

AltStyle によって変換されたページ (->オリジナル) /