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.