From ab5b79b19592bf730a523b251bbfc5d944f704d1 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 1 May 2026 22:02:52 -0700 Subject: [PATCH] buildkite: persist resolved org on build mapping Spawn previously stored cfg.Org on the buildkite_builds row, leaving empty when the workflow YAML didn't set tack.buildkite.org and relying on the read path to fall back to the provider's defaultOrg. That coupling let historical lookups drift: if defaultOrg ever changed, log fetches and webhook joins for older builds would silently target the wrong organisation. --- provider_buildkite.go | 26 ++++++----- provider_buildkite_test.go | 93 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+), 12 deletions(-) diff --git a/provider_buildkite.go b/provider_buildkite.go index 8007916..7100f70 100644 --- a/provider_buildkite.go +++ b/provider_buildkite.go @@ -259,16 +259,18 @@ func (p *buildkiteProvider) spawnWorkflow( ) pipelineURI := pipelineATURI(knot, pipelineRkey) - // Persist the org we actually used. cfg.Org (not the resolved - // `org`) is intentional: empty means "fall back to defaultOrg - // at read time", which keeps the row meaningful even if the - // provider's defaultOrg changes later — and matches what older - // rows scan as before this column existed. + // Persist the *resolved* org — the one we actually issued the + // CreateBuild against — rather than cfg.Org. If we stored only + // cfg.Org, a later change to the provider's defaultOrg would + // silently retarget historical lookups (logs, webhook joins) at + // the wrong organisation. Legacy rows written before this fix + // may still have an empty Org; the read path keeps the + // defaultOrg fallback for those (see Logs). if err := p.st.InsertBuildkiteBuild(ctx, BuildkiteBuildRef{ BuildUUID: build.ID, BuildNumber: build.Number, PipelineSlug: cfg.Pipeline, - Org: cfg.Org, + Org: org, Knot: knot, PipelineRkey: pipelineRkey, Workflow: wf.Name, @@ -383,12 +385,12 @@ func (p *buildkiteProvider) Logs( return nil, ErrLogsNotFound } - // Resolve the org against which we should pull jobs/logs. The - // workflow YAML can override the spindle's default at Spawn - // time (tack.buildkite.org), so we honour whatever the row - // recorded; an empty value means the workflow didn't override, - // which is the same as "use defaultOrg" — including for rows - // written before the org column was added. + // Resolve the org against which we should pull jobs/logs. + // Spawn now persists the *resolved* org used at create time, so + // for any row written by current code ref.Org is authoritative + // and we use it verbatim. The empty-string fallback to + // defaultOrg only exists for legacy rows on disk that predate + // persisting the resolved org; new rows should never hit it. org := ref.Org if org == "" { org = p.defaultOrg diff --git a/provider_buildkite_test.go b/provider_buildkite_test.go index 52ff622..dc043b6 100644 --- a/provider_buildkite_test.go +++ b/provider_buildkite_test.go @@ -253,6 +253,99 @@ func TestBuildkiteSpawnWorkflowConfig(t *testing.T) { } } +// TestBuildkiteSpawnPersistsResolvedOrg pins that Spawn writes the +// *resolved* org to the buildkite_builds row when the workflow YAML +// omits `tack.buildkite.org`. Storing cfg.Org (empty) here would let +// historical lookups silently follow a later change to defaultOrg +// into the wrong organisation; storing the value we actually issued +// the CreateBuild against keeps each row self-describing. +func TestBuildkiteSpawnPersistsResolvedOrg(t *testing.T) { + bk := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // The default org in newBuildkiteTestProvider is "myorg"; + // the URL must reflect the resolved fallback so we know + // what value the row is expected to capture. + if !strings.Contains(r.URL.Path, "/organizations/myorg/") { + t.Errorf("CreateBuild path = %q; want /organizations/myorg/", r.URL.Path) + } + w.WriteHeader(http.StatusCreated) + _ = json.NewEncoder(w).Encode(buildkite.Build{ID: "uuid-r", Number: 3}) + }) + p, st, _, _ := newBuildkiteTestProvider(t, buildkite.WebhookModeToken, "s", bk) + + p.Spawn(context.Background(), "knot.example.com", "rkey-r", + &tangled.Pipeline_TriggerMetadata{ + Push: &tangled.Pipeline_PushTriggerData{ + NewSha: "abc", Ref: "refs/heads/main", + }, + }, + // No `org:` under tack.buildkite — must fall back to the + // provider's defaultOrg AND persist that resolved value. + []*tangled.Pipeline_Workflow{ + {Name: "ci.yml", Raw: "tack:\n buildkite:\n pipeline: mypipe\n"}, + }, + ) + + deadline := time.Now().Add(2 * time.Second) + var ref *BuildkiteBuildRef + for time.Now().Before(deadline) { + var err error + ref, err = st.LookupBuildkiteBuildByUUID(context.Background(), "uuid-r") + if err != nil { + t.Fatalf("lookup: %v", err) + } + if ref != nil { + break + } + time.Sleep(20 * time.Millisecond) + } + if ref == nil { + t.Fatal("buildkite build row not persisted within deadline") + } + // The whole point of the change: ref.Org records the org the + // build was created against, not "" (which would hand off to + // whatever defaultOrg happens to be at read time). + if ref.Org != "myorg" { + t.Fatalf("ref.Org = %q; want %q (resolved defaultOrg)", + ref.Org, "myorg") + } + + // Belt-and-braces: simulate defaultOrg drifting after the row + // was written. Logs() should still target the org persisted on + // the row, not the new default. We swap defaultOrg out and + // stand up a sibling httptest server that fails the test if + // anything but /organizations/myorg/ is requested. + gotPath := make(chan string, 1) + logSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotPath <- r.URL.Path + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(buildkite.Build{ + ID: "uuid-r", Number: 3, Jobs: nil, + }) + })) + t.Cleanup(logSrv.Close) + prev := buildkite.APIBase + buildkite.APIBase = logSrv.URL + t.Cleanup(func() { buildkite.APIBase = prev }) + + p.defaultOrg = "newdefault" + ch, err := p.Logs(context.Background(), "knot.example.com", "rkey-r", "ci.yml") + if err != nil { + t.Fatalf("Logs: %v", err) + } + // Drain so the goroutine completes; we only care about the + // initial GetBuild path. + for range ch { + } + select { + case path := <-gotPath: + if !strings.Contains(path, "/organizations/myorg/") { + t.Fatalf("Logs hit %q; want path containing /organizations/myorg/", path) + } + case <-time.After(2 * time.Second): + t.Fatal("Logs did not call GetBuild") + } +} + // TestBuildkiteSpawnInvalidYAML proves a workflow without the // required `tack.buildkite.pipeline` field is skipped — no API // call, no DB row, no status. A misconfigured workflow shouldn't -- 2.51.2