From 4b7774d32432d0d7f8c3f46079a0387e9e037b50 Mon Sep 17 00:00:00 2001 From: dvcdsys Date: Thu, 4 Jun 2026 19:49:39 +0100 Subject: [PATCH 1/2] fix(server): make GitHub webhook registration idempotent (#68) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Auto-registration always POSTed a new hook instead of checking whether one with the same delivery URL already existed, so re-adds, reindexes, and restarts accumulated duplicate hooks on a repo — GitHub then fanned every push out N times. Add githubapi.ListWebhooks + EnsureWebhook: list existing hooks, match on config.url, PATCH the match (and prune same-URL duplicates) instead of creating, and only POST when none match. Wire both registration paths (tryAutoRegisterWebhook, reconciler.reconcileOne) through it. The reconciler now reuses an existing hook even when the stored WebhookID was lost, so a reregister sweep never leaves old + new hooks side by side. Covered by new githubapi tests (create / reuse / prune-duplicates / ignore-non-matching-URL) and a reconciler reuse test. Docs updated. Co-Authored-By: Claude Opus 4.8 --- doc/WEBHOOKS.md | 20 ++- server/internal/githubapi/githubapi.go | 145 +++++++++++++++- server/internal/githubapi/githubapi_test.go | 178 ++++++++++++++++++++ server/internal/httpapi/gitrepos.go | 5 +- server/internal/tunnels/reconciler.go | 17 +- server/internal/tunnels/tunnels_test.go | 41 ++++- 6 files changed, 385 insertions(+), 21 deletions(-) diff --git a/doc/WEBHOOKS.md b/doc/WEBHOOKS.md index 1edd98f3..aee62b66 100644 --- a/doc/WEBHOOKS.md +++ b/doc/WEBHOOKS.md @@ -82,10 +82,17 @@ When `webhook_mode=auto` and the PAT scope check passes: 1. Operator submits the add-repo form. The server clones the repo (`clone_repo` job) and starts indexing. -2. In parallel, the server calls `POST /repos/{owner}/{repo}/hooks` - on GitHub via `server/internal/githubapi/`. The hook payload sets - `events: ["push"]`, `content_type: json`, and embeds the - server-generated `webhook_secret`. +2. In parallel, the server registers the hook **idempotently** via + `server/internal/githubapi/` (`EnsureWebhook`): it first + `GET /repos/{owner}/{repo}/hooks` and looks for a hook whose + `config.url` already equals this server's delivery URL. If one + exists it is reused (PATCHed to refresh the secret/events) and any + extra duplicates pointing at the same URL are deleted; only when + none match does it `POST /repos/{owner}/{repo}/hooks`. The hook + payload sets `events: ["push"]`, `content_type: json`, and embeds + the server-generated `webhook_secret`. This is what prevents + duplicate hooks accumulating across re-adds, reindexes, and server + restarts (issue #68). 3. GitHub responds with the hook id. The id is stored on the `git_repos` row so a later DELETE can call `DELETE /repos/{owner}/{repo}/hooks/{id}` cleanly. @@ -154,7 +161,10 @@ On boot the server runs a one-shot audit This is also why rotating `CIX_PUBLIC_URL` should be paired with a "reregister all" sweep in the dashboard — there's no automatic -follow-up. +follow-up. The reconcile/reregister path is idempotent: a repo whose +hook id is still known is PATCHed in place; a repo whose stored id was +lost is matched by `config.url` and reused rather than re-created, so a +sweep never leaves a repo with old **and** new hooks side by side. ## 7. What gets re-indexed on a push diff --git a/server/internal/githubapi/githubapi.go b/server/internal/githubapi/githubapi.go index 52a1c8b8..1472aced 100644 --- a/server/internal/githubapi/githubapi.go +++ b/server/internal/githubapi/githubapi.go @@ -73,10 +73,10 @@ func New() *Client { // repo-picker UI actually renders — so we don't bloat the JSON payload // (a single user can have several hundred repos visible via a PAT). type Repo struct { - FullName string `json:"full_name"` // "owner/name" - DefaultBranch string `json:"default_branch"` // used to auto-fill the branch input - Private bool `json:"private"` // shown as a lock icon in the dropdown - HTMLURL string `json:"html_url"` // canonical https://github.com/... form + FullName string `json:"full_name"` // "owner/name" + DefaultBranch string `json:"default_branch"` // used to auto-fill the branch input + Private bool `json:"private"` // shown as a lock icon in the dropdown + HTMLURL string `json:"html_url"` // canonical https://github.com/... form Description string `json:"description,omitempty"` Owner RepoOwner `json:"owner"` } @@ -359,11 +359,22 @@ type CreateWebhookOptions struct { Insecure bool // mostly for tests against http:// origins } -// HookResponse is the slice of the GitHub response we care about. +// HookConfig mirrors the `config` slice of a GitHub hook. We only read +// `url` — the delivery target — which is what makes registration +// idempotent: a hook whose config.url already equals our delivery URL is +// the cix hook, so we reuse it instead of POSTing a duplicate. +type HookConfig struct { + URL string `json:"url"` +} + +// HookResponse is the slice of the GitHub response we care about. The +// top-level URL is the hook's own API URL (…/hooks/{id}); Config.URL is the +// delivery target the hook POSTs to. type HookResponse struct { - ID int64 `json:"id"` - URL string `json:"url"` - Active bool `json:"active"` + ID int64 `json:"id"` + URL string `json:"url"` + Active bool `json:"active"` + Config HookConfig `json:"config"` } // CreateWebhook calls POST /repos/{owner}/{repo}/hooks. Returns the @@ -422,6 +433,124 @@ func (c *Client) CreateWebhook(ctx context.Context, opts CreateWebhookOptions) ( } } +// ListWebhooks calls GET /repos/{owner}/{repo}/hooks and returns the repo's +// configured hooks (with their delivery config.url). Used to make +// registration idempotent — callers match on config.url to find an existing +// cix hook before creating a new one. Walks Link rel=next up to maxPages +// (0 = no cap); a repo with >100 hooks is pathological, so the default +// caller passes a small ceiling. +func (c *Client) ListWebhooks(ctx context.Context, owner, repo, pat string, maxPages int) ([]HookResponse, error) { + if owner == "" || repo == "" { + return nil, fmt.Errorf("owner/repo required") + } + if pat == "" { + return nil, fmt.Errorf("PAT required") + } + out := []HookResponse{} + page := 0 + pageURL := c.BaseURL + "/repos/" + url.PathEscape(owner) + "/" + url.PathEscape(repo) + "/hooks?per_page=100" + for pageURL != "" { + page++ + req, err := http.NewRequestWithContext(ctx, http.MethodGet, pageURL, nil) + if err != nil { + return nil, err + } + c.signRequest(req, pat) + resp, err := c.HTTPClient.Do(req) + if err != nil { + return nil, fmt.Errorf("github API: %w", err) + } + body, _ := io.ReadAll(resp.Body) + resp.Body.Close() + switch resp.StatusCode { + case http.StatusOK: + var batch []HookResponse + if err := json.Unmarshal(body, &batch); err != nil { + return nil, fmt.Errorf("parse hooks page: %w", err) + } + out = append(out, batch...) + case http.StatusUnauthorized, http.StatusForbidden: + return nil, fmt.Errorf("%w: %s", ErrUnauthorized, githubMessage(body)) + case http.StatusNotFound: + return nil, fmt.Errorf("%w: %s", ErrNotFound, githubMessage(body)) + default: + return nil, fmt.Errorf("github API %d: %s", resp.StatusCode, githubMessage(body)) + } + if maxPages > 0 && page >= maxPages { + break + } + pageURL = parseNextLink(resp.Header.Get("Link")) + } + return out, nil +} + +// EnsureWebhook is the idempotent registration entry point. It lists the +// repo's existing hooks and, if one already targets opts.URL, PATCHes it in +// place (refreshing the secret/events/active flag) and returns its id; +// otherwise it POSTs a fresh hook. Any *additional* hooks pointing at the +// same delivery URL are deleted, so a repo that already accumulated +// duplicates (issue #68) converges to exactly one cix hook. +// +// The returned bool reports whether a new hook was created (false = an +// existing one was reused). Callers persist the returned id either way. +func (c *Client) EnsureWebhook(ctx context.Context, opts CreateWebhookOptions) (HookResponse, bool, error) { + hooks, err := c.ListWebhooks(ctx, opts.Owner, opts.Repo, opts.PAT, 10) + if err != nil { + return HookResponse{}, false, err + } + var matches []HookResponse + for _, h := range hooks { + if sameDeliveryURL(h.Config.URL, opts.URL) { + matches = append(matches, h) + } + } + if len(matches) == 0 { + hr, cerr := c.CreateWebhook(ctx, opts) + if cerr != nil { + return HookResponse{}, false, cerr + } + return hr, true, nil + } + + // Reuse the first match; PATCH to refresh delivery config. If GitHub + // reports it gone (raced delete between list and patch), create instead. + keep := matches[0] + hr, uerr := c.UpdateWebhook(ctx, UpdateWebhookOptions{ + Owner: opts.Owner, + Repo: opts.Repo, + PAT: opts.PAT, + HookID: keep.ID, + URL: opts.URL, + Secret: opts.Secret, + Events: opts.Events, + Insecure: opts.Insecure, + }) + if uerr != nil { + if errors.Is(uerr, ErrNotFound) { + hr2, cerr := c.CreateWebhook(ctx, opts) + if cerr != nil { + return HookResponse{}, false, cerr + } + return hr2, true, nil + } + return HookResponse{}, false, uerr + } + + // Prune any leftover duplicates so the repo ends up with one cix hook. + for _, dup := range matches[1:] { + _ = c.DeleteWebhook(ctx, opts.Owner, opts.Repo, opts.PAT, dup.ID) + } + return hr, false, nil +} + +// sameDeliveryURL compares two hook delivery URLs for idempotency. Exact +// match after trimming a trailing slash — cix builds delivery URLs +// deterministically, so anything subtler would risk treating a genuinely +// different endpoint as the same hook. +func sameDeliveryURL(a, b string) bool { + return strings.TrimRight(a, "/") == strings.TrimRight(b, "/") && a != "" +} + // UpdateWebhookOptions parameterises a hook update. HookID identifies the // existing hook; URL/Secret are the new delivery config. Events defaults // to ["push"] when nil. diff --git a/server/internal/githubapi/githubapi_test.go b/server/internal/githubapi/githubapi_test.go index 713af789..61c78aa0 100644 --- a/server/internal/githubapi/githubapi_test.go +++ b/server/internal/githubapi/githubapi_test.go @@ -7,6 +7,7 @@ import ( "io" "net/http" "net/http/httptest" + "strconv" "strings" "testing" ) @@ -467,6 +468,183 @@ func TestListReposForAccountUsesCorrectEndpoint(t *testing.T) { } } +// hooksServer is a tiny stateful in-memory fake of GitHub's +// /repos/{owner}/{repo}/hooks endpoints — enough to exercise the +// list→match→create/update/delete dance EnsureWebhook performs. The map is +// id → delivery (config.url). Counters let tests assert no duplicate POSTs. +type hooksServer struct { + hooks map[int64]string + nextID int64 + posts int + patches int + deletes int +} + +func newHooksServer(t *testing.T, seed map[int64]string) (*Client, *hooksServer) { + t.Helper() + st := &hooksServer{hooks: map[int64]string{}, nextID: 100} + for id, u := range seed { + st.hooks[id] = u + if id >= st.nextID { + st.nextID = id + 1 + } + } + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + // Path is /repos/o/r/hooks or /repos/o/r/hooks/{id}. + parts := strings.Split(strings.Trim(r.URL.Path, "/"), "/") + var hookID int64 + if len(parts) == 5 { // repos/o/r/hooks/{id} + hookID, _ = strconv.ParseInt(parts[4], 10, 64) + } + switch r.Method { + case http.MethodGet: + type hook struct { + ID int64 `json:"id"` + Active bool `json:"active"` + Config HookConfig `json:"config"` + } + out := []hook{} + for id, u := range st.hooks { + out = append(out, hook{ID: id, Active: true, Config: HookConfig{URL: u}}) + } + _ = json.NewEncoder(w).Encode(out) + case http.MethodPost: + st.posts++ + raw, _ := io.ReadAll(r.Body) + var body struct { + Config struct { + URL string `json:"url"` + } `json:"config"` + } + _ = json.Unmarshal(raw, &body) + id := st.nextID + st.nextID++ + st.hooks[id] = body.Config.URL + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(`{"id":` + strconv.FormatInt(id, 10) + `,"active":true,"config":{"url":"` + body.Config.URL + `"}}`)) + case http.MethodPatch: + st.patches++ + if _, ok := st.hooks[hookID]; !ok { + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"message":"Not Found"}`)) + return + } + raw, _ := io.ReadAll(r.Body) + var body struct { + Config struct { + URL string `json:"url"` + } `json:"config"` + } + _ = json.Unmarshal(raw, &body) + st.hooks[hookID] = body.Config.URL + _, _ = w.Write([]byte(`{"id":` + strconv.FormatInt(hookID, 10) + `,"active":true,"config":{"url":"` + body.Config.URL + `"}}`)) + case http.MethodDelete: + st.deletes++ + delete(st.hooks, hookID) + w.WriteHeader(http.StatusNoContent) + default: + w.WriteHeader(http.StatusMethodNotAllowed) + } + })) + t.Cleanup(srv.Close) + c := New() + c.BaseURL = srv.URL + return c, st +} + +func TestEnsureWebhookCreatesWhenNoneExist(t *testing.T) { + c, st := newHooksServer(t, nil) + hr, created, err := c.EnsureWebhook(context.Background(), CreateWebhookOptions{ + Owner: "o", Repo: "r", PAT: "ghp_x", + URL: "https://cix.test/api/v1/webhooks/github/abc", Secret: "s", + }) + if err != nil { + t.Fatalf("EnsureWebhook: %v", err) + } + if !created { + t.Fatalf("expected created=true on empty repo") + } + if st.posts != 1 { + t.Fatalf("expected 1 POST, got %d", st.posts) + } + if st.hooks[hr.ID] != "https://cix.test/api/v1/webhooks/github/abc" { + t.Fatalf("hook not registered with expected url: %v", st.hooks) + } +} + +func TestEnsureWebhookReusesMatchingURL(t *testing.T) { + const url = "https://cix.test/api/v1/webhooks/github/abc" + c, st := newHooksServer(t, map[int64]string{55: url}) + hr, created, err := c.EnsureWebhook(context.Background(), CreateWebhookOptions{ + Owner: "o", Repo: "r", PAT: "ghp_x", URL: url, Secret: "s", + }) + if err != nil { + t.Fatalf("EnsureWebhook: %v", err) + } + if created { + t.Fatalf("expected created=false (reuse), got true") + } + if hr.ID != 55 { + t.Fatalf("expected reused id 55, got %d", hr.ID) + } + if st.posts != 0 { + t.Fatalf("must not POST a duplicate, got %d POSTs", st.posts) + } + if st.patches != 1 { + t.Fatalf("expected 1 PATCH to refresh config, got %d", st.patches) + } + if len(st.hooks) != 1 { + t.Fatalf("expected exactly one hook to remain, got %d", len(st.hooks)) + } +} + +func TestEnsureWebhookPrunesDuplicates(t *testing.T) { + const url = "https://cix.test/api/v1/webhooks/github/abc" + // Three identical hooks already exist (the issue #68 state). + c, st := newHooksServer(t, map[int64]string{1: url, 2: url, 3: url}) + _, created, err := c.EnsureWebhook(context.Background(), CreateWebhookOptions{ + Owner: "o", Repo: "r", PAT: "ghp_x", URL: url, Secret: "s", + }) + if err != nil { + t.Fatalf("EnsureWebhook: %v", err) + } + if created { + t.Fatalf("expected created=false, got true") + } + if st.posts != 0 { + t.Fatalf("must not POST, got %d", st.posts) + } + if st.deletes != 2 { + t.Fatalf("expected 2 duplicate deletes, got %d", st.deletes) + } + if len(st.hooks) != 1 { + t.Fatalf("repo should converge to a single hook, got %d", len(st.hooks)) + } +} + +func TestEnsureWebhookIgnoresNonMatchingURL(t *testing.T) { + // A hook for a *different* delivery URL must not be reused — that would + // hijack an unrelated webhook. EnsureWebhook creates a new one alongside. + c, st := newHooksServer(t, map[int64]string{9: "https://other.test/hook"}) + _, created, err := c.EnsureWebhook(context.Background(), CreateWebhookOptions{ + Owner: "o", Repo: "r", PAT: "ghp_x", + URL: "https://cix.test/api/v1/webhooks/github/abc", Secret: "s", + }) + if err != nil { + t.Fatalf("EnsureWebhook: %v", err) + } + if !created { + t.Fatalf("expected created=true for a non-matching existing hook") + } + if st.posts != 1 { + t.Fatalf("expected 1 POST, got %d", st.posts) + } + if len(st.hooks) != 2 { + t.Fatalf("expected both hooks to coexist, got %d", len(st.hooks)) + } +} + func TestParseOwnerRepo(t *testing.T) { cases := map[string][2]string{ "https://github.com/spf13/cobra": {"spf13", "cobra"}, diff --git a/server/internal/httpapi/gitrepos.go b/server/internal/httpapi/gitrepos.go index 4000451f..4954ce45 100644 --- a/server/internal/httpapi/gitrepos.go +++ b/server/internal/httpapi/gitrepos.go @@ -602,7 +602,10 @@ func (s *Server) tryAutoRegisterWebhook(ctx context.Context, g gitrepos.GitRepo, if perr != nil { return false, "github_url is not a parseable owner/repo URL" } - hr, herr := githubapi.New().CreateWebhook(ctx, githubapi.CreateWebhookOptions{ + // EnsureWebhook is idempotent: if the repo already has a hook pointing at + // this delivery URL it reuses (and PATCHes) it instead of POSTing a + // duplicate, and prunes any accumulated duplicates (issue #68). + hr, _, herr := githubapi.New().EnsureWebhook(ctx, githubapi.CreateWebhookOptions{ Owner: owner, Repo: repo, PAT: pat, diff --git a/server/internal/tunnels/reconciler.go b/server/internal/tunnels/reconciler.go index dc3ef069..ea05ab3b 100644 --- a/server/internal/tunnels/reconciler.go +++ b/server/internal/tunnels/reconciler.go @@ -33,8 +33,11 @@ type TokenRevealer interface { // WebhookClient is the GitHub webhook API surface. Satisfied by // *githubapi.Client. type WebhookClient interface { - CreateWebhook(ctx context.Context, opts githubapi.CreateWebhookOptions) (githubapi.HookResponse, error) UpdateWebhook(ctx context.Context, opts githubapi.UpdateWebhookOptions) (githubapi.HookResponse, error) + // EnsureWebhook registers idempotently: it reuses an existing hook whose + // delivery URL already matches (deduping accumulated duplicates) or + // creates one. The bool reports whether a new hook was created. + EnsureWebhook(ctx context.Context, opts githubapi.CreateWebhookOptions) (githubapi.HookResponse, bool, error) } // Reconciler re-points every webhook_mode=auto repo at the current public @@ -172,7 +175,11 @@ func (r *Reconciler) reconcileOne(ctx context.Context, g gitrepos.GitRepo, baseU r.logger.Warn("webhook gone on GitHub side, recreating", "project", g.ProjectPath) } - hr, cerr := r.gh.CreateWebhook(ctx, githubapi.CreateWebhookOptions{ + // No (usable) stored hook id: register idempotently. EnsureWebhook reuses + // a hook already pointing at this delivery URL — and prunes duplicates — + // rather than blindly POSTing a new one, which is how repos accumulated + // duplicate hooks before (issue #68). + hr, created, cerr := r.gh.EnsureWebhook(ctx, githubapi.CreateWebhookOptions{ Owner: owner, Repo: repo, PAT: pat, @@ -187,6 +194,10 @@ func (r *Reconciler) reconcileOne(ctx context.Context, g gitrepos.GitRepo, baseU if serr := r.repos.SetWebhookID(ctx, g.ProjectPath, hr.ID); serr != nil { r.logger.Warn("could not persist webhook id", "project", g.ProjectPath, "err", serr) } - out.Action = "created" + if created { + out.Action = "created" + } else { + out.Action = "updated" + } return out } diff --git a/server/internal/tunnels/tunnels_test.go b/server/internal/tunnels/tunnels_test.go index 3ed4b18f..7e65350f 100644 --- a/server/internal/tunnels/tunnels_test.go +++ b/server/internal/tunnels/tunnels_test.go @@ -196,12 +196,11 @@ func (fakeTokens) Touch(_ context.Context, _ string) error { return n type fakeGH struct { created, updated int failUpdateNotFound bool + // existing simulates GitHub-side hooks keyed by delivery URL, so + // EnsureWebhook can model the idempotent reuse path. + existing map[string]int64 } -func (f *fakeGH) CreateWebhook(_ context.Context, _ githubapi.CreateWebhookOptions) (githubapi.HookResponse, error) { - f.created++ - return githubapi.HookResponse{ID: 999}, nil -} func (f *fakeGH) UpdateWebhook(_ context.Context, opts githubapi.UpdateWebhookOptions) (githubapi.HookResponse, error) { if f.failUpdateNotFound { return githubapi.HookResponse{}, fmt.Errorf("%w: gone", githubapi.ErrNotFound) @@ -209,6 +208,14 @@ func (f *fakeGH) UpdateWebhook(_ context.Context, opts githubapi.UpdateWebhookOp f.updated++ return githubapi.HookResponse{ID: opts.HookID}, nil } +func (f *fakeGH) EnsureWebhook(_ context.Context, opts githubapi.CreateWebhookOptions) (githubapi.HookResponse, bool, error) { + if id, ok := f.existing[opts.URL]; ok { + f.updated++ + return githubapi.HookResponse{ID: id, Config: githubapi.HookConfig{URL: opts.URL}}, false, nil + } + f.created++ + return githubapi.HookResponse{ID: 999, Config: githubapi.HookConfig{URL: opts.URL}}, true, nil +} func i64(v int64) *int64 { return &v } @@ -256,6 +263,32 @@ func TestReconcileRecreatesWhenHookGone(t *testing.T) { } } +func TestReconcileReusesExistingHookWithoutStoredID(t *testing.T) { + // A repo with no stored WebhookID but an existing hook already pointing + // at the delivery URL must be reused (updated), not duplicated. This is + // the idempotency guarantee of issue #68. + repos := &fakeRepos{repos: []gitrepos.GitRepo{ + {ProjectPath: "github.com/o/a@main", PathHash: "aaa", GitHubURL: "https://github.com/o/a", TokenID: "t1", WebhookSecret: "s", WebhookMode: gitrepos.WebhookModeAuto}, + }} + delivery := "https://x.trycloudflare.com" + WebhookPathPrefix + "aaa" + gh := &fakeGH{existing: map[string]int64{delivery: 77}} + r := NewReconciler(repos, fakeTokens{}, gh, nil) + + res, err := r.Reconcile(context.Background(), "https://x.trycloudflare.com") + if err != nil { + t.Fatalf("Reconcile: %v", err) + } + if res.Created != 0 || res.Updated != 1 { + t.Fatalf("expected reuse (updated), got created=%d updated=%d (%+v)", res.Created, res.Updated, res.Outcomes) + } + if gh.created != 0 { + t.Fatalf("must not POST a duplicate hook, got created=%d", gh.created) + } + if repos.setIDs["github.com/o/a@main"] != 77 { + t.Fatalf("reused hook id should be persisted, got %v", repos.setIDs) + } +} + func TestReconcileNoBaseURLIsNoOp(t *testing.T) { repos := &fakeRepos{repos: []gitrepos.GitRepo{ {ProjectPath: "github.com/o/a@main", PathHash: "aaa", GitHubURL: "https://github.com/o/a", TokenID: "t1", WebhookMode: gitrepos.WebhookModeAuto, WebhookID: i64(42)}, From e7994c79f3c6ec3c3571ece645d4a55ea661f62d Mon Sep 17 00:00:00 2001 From: dvcdsys Date: Thu, 4 Jun 2026 19:56:13 +0100 Subject: [PATCH 2/2] fix(server): prune duplicates on the EnsureWebhook race-create path; doc caveat MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address PR review: - When the matched hook is deleted between list and PATCH (404), the create-replacement branch now also prunes any other same-URL duplicates we listed, instead of leaving them behind. - WEBHOOKS.md §6: note that a reconcile sweep does NOT prune pre-existing same-URL duplicates when the stored hook id is still valid (it PATCHes that one and returns); re-adding the repo collapses them via EnsureWebhook. Co-Authored-By: Claude Opus 4.8 --- doc/WEBHOOKS.md | 7 +++++++ server/internal/githubapi/githubapi.go | 6 ++++++ 2 files changed, 13 insertions(+) diff --git a/doc/WEBHOOKS.md b/doc/WEBHOOKS.md index aee62b66..cf72aa0f 100644 --- a/doc/WEBHOOKS.md +++ b/doc/WEBHOOKS.md @@ -166,6 +166,13 @@ hook id is still known is PATCHed in place; a repo whose stored id was lost is matched by `config.url` and reused rather than re-created, so a sweep never leaves a repo with old **and** new hooks side by side. +One caveat for repos that *already* accumulated same-URL duplicates +before this fix: when the stored hook id is still valid, reconcile +PATCHes that one hook and returns — it does **not** list and prune the +sibling duplicates. Re-adding the repo (which routes through the +`EnsureWebhook` list→match→prune path) collapses them back to a single +hook; a plain reconcile sweep does not. + ## 7. What gets re-indexed on a push Each accepted `push` enqueues a `clone_repo` job, which: diff --git a/server/internal/githubapi/githubapi.go b/server/internal/githubapi/githubapi.go index 1472aced..81ac7628 100644 --- a/server/internal/githubapi/githubapi.go +++ b/server/internal/githubapi/githubapi.go @@ -527,10 +527,16 @@ func (c *Client) EnsureWebhook(ctx context.Context, opts CreateWebhookOptions) ( }) if uerr != nil { if errors.Is(uerr, ErrNotFound) { + // matches[0] was deleted out from under us between list and + // patch. Create a replacement, but still prune any other + // same-URL duplicates we listed so we don't leave them behind. hr2, cerr := c.CreateWebhook(ctx, opts) if cerr != nil { return HookResponse{}, false, cerr } + for _, dup := range matches[1:] { + _ = c.DeleteWebhook(ctx, opts.Owner, opts.Repo, opts.PAT, dup.ID) + } return hr2, true, nil } return HookResponse{}, false, uerr