From f0d468e247410369ff15f68628cf27f5b623f094 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 1 May 2026 22:20:56 -0700 Subject: [PATCH] buildkite: fall back to meta_data on webhook cache miss Buildkite webhooks can land before Spawn's goroutine has persisted the build UUID to (knot, pipeline_rkey, workflow) mapping. The window is small but real: CreateBuild returns, Buildkite fires build.scheduled, the webhook handler runs LookupBuildkiteBuildByUUID and gets nothing, and the event is dropped on the floor forever. HandleWebhook now reconstructs the ref from the build's meta_data when the lookup misses. We already attach tack:knot, tack:pipeline_rkey, and tack:workflow at CreateBuild time, so a Buildkite-originated webhook for one of our builds always carries enough information to recover the tuple. Org and pipeline slug come from the payload's top-level organization and embedded build.pipeline objects. The reconstructed ref is opportunistically inserted via the existing ON CONFLICT DO UPDATE, so subsequent webhooks and any /logs request hit the cache instead of redoing the meta_data dance. If the authoritative Spawn-side insert lands afterwards it just refreshes the row. Builds without our tack:* meta_data still no-op, preserving the 'foreign build sharing this webhook URL' behavior. WebhookPayload gains an Organization field so the org slug is available without poking at Build.Pipeline. --- internal/buildkite/buildkite.go | 20 +++++-- provider_buildkite.go | 84 +++++++++++++++++++++++++++-- provider_buildkite_test.go | 94 +++++++++++++++++++++++++++++++++ 3 files changed, 190 insertions(+), 8 deletions(-) diff --git a/internal/buildkite/buildkite.go b/internal/buildkite/buildkite.go index e4c2975..1427574 100644 --- a/internal/buildkite/buildkite.go +++ b/internal/buildkite/buildkite.go @@ -255,10 +255,24 @@ func (c *Client) GetJobLog( // actually decode. Build events all wrap a "build" object; job // events wrap a "job" — keeping both around lets callers add // job-level mapping later without changing the decoder. +// +// Organization is the top-level organization object Buildkite mirrors +// onto every webhook. We decode just its slug so the provider can +// reconstruct (org, pipeline_slug, build_number) from a webhook alone +// when the local UUID→tuple mapping hasn't been persisted yet (the +// race between CreateBuild returning and InsertBuildkiteBuild landing). type WebhookPayload struct { - Event string `json:"event"` - Build Build `json:"build"` - Job Job `json:"job"` + Event string `json:"event"` + Build Build `json:"build"` + Job Job `json:"job"` + Organization Organization `json:"organization"` +} + +// Organization is the small slice of Buildkite's organization object +// we care about on webhook payloads: just the slug, used as the org +// component of REST URLs in subsequent calls. +type Organization struct { + Slug string `json:"slug"` } // WebhookMode selects how an inbound webhook request is diff --git a/provider_buildkite.go b/provider_buildkite.go index 7100f70..da4e166 100644 --- a/provider_buildkite.go +++ b/provider_buildkite.go @@ -544,13 +544,44 @@ func (p *buildkiteProvider) HandleWebhook( return fmt.Errorf("lookup build by uuid: %w", err) } if ref == nil { - // Most likely: this build was triggered outside tack and - // just happens to share our webhook URL. Nothing to do. - p.log.Debug("webhook for unknown build; ignoring", + // Cache miss. Two plausible causes: + // + // 1. Genuinely-foreign build: a Buildkite job triggered + // outside tack that just happens to share this + // webhook URL. Nothing to do. + // 2. Race: Spawn's goroutine fired CreateBuild but + // hasn't yet written the UUID→tuple row. A fast + // build.scheduled webhook can land in that window + // and would otherwise be dropped forever. + // + // We disambiguate using the Buildkite meta_data we set at + // CreateBuild time. If the tack:* keys are present the + // build is ours; we reconstruct the ref and opportunistically + // persist it so subsequent webhooks (and any Logs call) + // hit the cache rather than re-doing this work. + ref = refFromWebhook(payload) + if ref == nil { + p.log.Debug("webhook for unknown build; ignoring", + "event", payload.Event, + "build_uuid", payload.Build.ID, + ) + return nil + } + // Opportunistic cache fill. Failure here is non-fatal: + // Spawn's authoritative insert will land shortly (or has + // already, in which case our INSERT … ON CONFLICT just + // refreshes the row). We continue with the reconstructed + // ref either way so a status publish isn't lost. + if err := p.st.InsertBuildkiteBuild(ctx, *ref); err != nil { + p.log.Warn("opportunistic persist of buildkite build mapping", + "err", err, "build_uuid", ref.BuildUUID, + ) + } + p.log.Info("buildkite webhook reconstructed from meta_data", "event", payload.Event, - "build_uuid", payload.Build.ID, + "build_uuid", ref.BuildUUID, + "workflow", ref.Workflow, ) - return nil } status, ok := mapBuildkiteState(payload.Build.State) @@ -580,6 +611,49 @@ func (p *buildkiteProvider) HandleWebhook( return nil } +// refFromWebhook reconstructs a BuildkiteBuildRef directly from a +// webhook payload, using the tack:* meta_data we attach at Spawn +// time as the source of truth for (knot, pipeline_rkey, workflow). +// Org and pipeline slug come from the payload's organization and +// embedded pipeline objects, both of which Buildkite populates on +// every build.* event. +// +// Returns nil when the payload doesn't carry our meta_data: that's +// the signal the build was triggered outside tack and we should +// keep ignoring it. A partial set (one or two of the three keys) +// also returns nil; we don't want to half-reconstruct a row. +// +// This exists so HandleWebhook can recover the tuple when the +// CreateBuild→InsertBuildkiteBuild race drops a webhook on the +// floor. The caller is expected to opportunistically persist the +// returned ref so subsequent lookups hit the cache. +func refFromWebhook(payload buildkite.WebhookPayload) *BuildkiteBuildRef { + md := payload.Build.MetaData + knot := md[bkMetaKnot] + rkey := md[bkMetaPipelineRkey] + wf := md[bkMetaWorkflow] + if knot == "" || rkey == "" || wf == "" { + return nil + } + + // Pipeline slug lives on the build's embedded pipeline object. + // Decoding it as map[string]interface{} keeps the buildkite + // package's Build struct from sprouting fields we only ever + // touch on this fallback path. + pipelineSlug, _ := payload.Build.Pipeline["slug"].(string) + + return &BuildkiteBuildRef{ + BuildUUID: payload.Build.ID, + BuildNumber: payload.Build.Number, + PipelineSlug: pipelineSlug, + Org: payload.Organization.Slug, + Knot: knot, + PipelineRkey: rkey, + Workflow: wf, + PipelineURI: pipelineATURI(knot, rkey), + } +} + // mapBuildkiteState translates Buildkite's build state strings into // the Tangled spindle StatusKind enum. The mapping aligns with the // upstream constants (StatusKindRunning/Failed/Cancelled/Success); diff --git a/provider_buildkite_test.go b/provider_buildkite_test.go index dc043b6..019e046 100644 --- a/provider_buildkite_test.go +++ b/provider_buildkite_test.go @@ -463,6 +463,100 @@ func TestBuildkiteHandleWebhookIgnored(t *testing.T) { } } +// TestBuildkiteHandleWebhookMetaDataFallback covers the race where +// a webhook arrives before Spawn has persisted the UUID→tuple row. +// The handler must reconstruct the ref from the build's tack:* +// meta_data, publish a status event, and opportunistically persist +// the row so subsequent lookups hit the cache. +func TestBuildkiteHandleWebhookMetaDataFallback(t *testing.T) { + p, st, _, _ := newBuildkiteTestProvider(t, buildkite.WebhookModeToken, "secret", + func(w http.ResponseWriter, r *http.Request) { t.Fatal("buildkite shouldn't be called") }) + + // No InsertBuildkiteBuild: simulate the race window where + // CreateBuild has returned to Buildkite (so a webhook is in + // flight) but the provider hasn't yet written the mapping row. + err := p.HandleWebhook(context.Background(), buildkite.WebhookPayload{ + Event: "build.scheduled", + Build: buildkite.Build{ + ID: "uuid-race", + Number: 42, + State: "scheduled", + MetaData: map[string]string{ + bkMetaKnot: "knot.example.com", + bkMetaPipelineRkey: "rkey-race", + bkMetaWorkflow: "ci.yml", + }, + Pipeline: map[string]any{"slug": "mypipe"}, + }, + Organization: buildkite.Organization{Slug: "myorg"}, + }) + if err != nil { + t.Fatalf("HandleWebhook: %v", err) + } + + // Status was published despite the missing cache row. + rows, err := st.EventsAfter(context.Background(), 0) + if err != nil { + t.Fatalf("EventsAfter: %v", err) + } + if len(rows) != 1 { + t.Fatalf("got %d events, want 1", len(rows)) + } + var rec tangled.PipelineStatus + if err := json.Unmarshal(rows[0].EventJSON, &rec); err != nil { + t.Fatalf("decode: %v", err) + } + if rec.Status != "pending" || rec.Workflow != "ci.yml" { + t.Fatalf("bad status: %+v", rec) + } + + // Opportunistic insert means the row is now cached for any + // subsequent webhook (or Logs call), including the Org and + // PipelineSlug we recovered from the payload. + ref, err := st.LookupBuildkiteBuildByUUID(context.Background(), "uuid-race") + if err != nil { + t.Fatalf("lookup: %v", err) + } + if ref == nil { + t.Fatal("expected opportunistic InsertBuildkiteBuild, got nil row") + } + if ref.Workflow != "ci.yml" || ref.Knot != "knot.example.com" || + ref.PipelineRkey != "rkey-race" || ref.BuildNumber != 42 || + ref.PipelineSlug != "mypipe" || ref.Org != "myorg" { + t.Fatalf("ref mismatch: %+v", ref) + } +} + +// TestBuildkiteHandleWebhookForeignBuild covers the genuinely-foreign +// case: a webhook for a build that doesn't carry our tack:* meta_data +// must remain a silent no-op even though the new fallback path now +// inspects meta_data. +func TestBuildkiteHandleWebhookForeignBuild(t *testing.T) { + p, st, _, _ := newBuildkiteTestProvider(t, buildkite.WebhookModeToken, "secret", + func(w http.ResponseWriter, r *http.Request) {}) + + if err := p.HandleWebhook(context.Background(), buildkite.WebhookPayload{ + Event: "build.finished", + Build: buildkite.Build{ + ID: "uuid-foreign", + State: "passed", + // No tack:* keys: build was triggered outside tack. + MetaData: map[string]string{"someone-elses-key": "value"}, + }, + }); err != nil { + t.Fatalf("HandleWebhook: %v", err) + } + + rows, _ := st.EventsAfter(context.Background(), 0) + if len(rows) != 0 { + t.Fatalf("got %d events, want 0", len(rows)) + } + ref, _ := st.LookupBuildkiteBuildByUUID(context.Background(), "uuid-foreign") + if ref != nil { + t.Fatalf("foreign build was opportunistically persisted: %+v", ref) + } +} + // TestBuildkiteWebhookHandlerHTTP exercises the full HTTP path // including auth: a request signed with the wrong secret must be // rejected, and a correctly-signed one must reach the provider. -- 2.51.2