feat: add shared platform buildkite/githubactions BuildRunner clients by roychying · Pull Request #423 · uber/submitqueue

roychying · 2026-07-22T01:42:27Z

Summary

Extract shared Buildkite and GitHub Actions clients into platform/extension/buildrunner/ and refactor the existing submitqueue adapters to use them.
Add stovepipe-side Buildkite and GitHub Actions BuildRunner adapters on top of the shared clients.

Test plan

go build ./...
go test ./platform/extension/buildrunner/... ./submitqueue/extension/buildrunner/... ./stovepipe/extension/buildrunner/...
make gazelle (no drift)

…client

…orm client

sbalabanov · 2026-07-22T18:39:45Z

+		// Already terminal — no-op per BuildRunner.Cancel contract.
+		return nil
+	default:
+		return fmt.Errorf("unexpected status %d from cancel", resp.StatusCode)


would be nice to also include the message into the error, typically in a body of a response; can be truncated if needed.

done, already included the body in the cancel error

sbalabanov · 2026-07-22T18:50:55Z

+	}
+	defer resp.Body.Close()
+
+	respBody, err := io.ReadAll(resp.Body)


this may not properly respond to context cancellation. AI analysis:

Behavior: Vulnerable to blocking/hanging.

Why: io.ReadAll reads from resp.Body until it hits io.EOF. However, once httpClient.Do() successfully receives response headers, the HTTP transport hands the raw stream body to you. Standard response bodies (net/http.bodyEOFSignal or TCP socket readers) do not automatically monitor the request ctx for subsequent reads.

If the server stalls, sends data infinitely slowly, or hangs while streaming the payload after headers have been sent, calling io.ReadAll(resp.Body) will block the goroutine until the connection times out at the network level—even if your ctx was canceled long ago.

The poor man's alternative is buffered read with cancellation checks:
https://github.com/uber/tango/blob/1ed2b86888fc4afc59494d9d97fdd6dc23c2638e/core/storage/ctxreader.go#L25
At least it cancels somewhere in between for long transfers.

good call. dug a bit on this one. Per the stdlib docs, Request's context "controls the entire lifetime of a request and its response: obtaining a connection, sending the request, and reading the response headers and body" , and Client.Timeout (implemented as context cancellation internally) is documented to "interrupt reading of the Response.Body" . So I believe a canceled ctx does unblock an in-progress io.ReadAll(resp.Body) here, not wait for a network-level timeout.

one thing is that nothing sets a deadline on ctx or a Timeout on the http.Client for this path. platform/http.NewClient's doc already calls that out as the caller's job, but no actual service wiring exists yet to enforce it (Buildkite isn't wired into any main.go today).

I prefer to leave that to whichever change wires up a real client, but happy to follow tango's implementation since it's a cheap safe guard. lmk if anything is wrong on my understanding

sbalabanov · 2026-07-22T18:54:05Z

+	}
+
+	if resp.StatusCode == http.StatusNotFound {
+		return fmt.Errorf("build not found")


http helper function do() is not necessarily about build. You may want to define sentinel errors to transform http codes into.

Done, do() now returns ErrNotFound instead of a hardcoded string.

sbalabanov · 2026-07-22T19:03:20Z

+	if !ok || raw == "" {
+		return meta
+	}
+	_ = json.Unmarshal([]byte(raw), &meta)


let's not swallow errors without proper visibility
because this is an exported function, probably better design is to expose an error and let caller handle it as they see fit

sbalabanov · 2026-07-22T19:05:12Z

+}
+
+func buildJSONWithEnv(number int, state, webURL string, env map[string]string) []byte {
+	b, _ := json.Marshal(BuildResponse{Number: number, State: state, WebURL: webURL, Env: env})


func buildJSONWithEnv(t *testing.T, number int, state, webURL string, env map[string]string) []byte { t.Helper() b, err := json.Marshal(BuildResponse{Number: number, State: state, WebURL: webURL, Env: env}) require.NoError(t, err) return b }

sbalabanov · 2026-07-22T19:07:25Z

+		capturedMethod = req.Method
+		capturedBody, _ = io.ReadAll(req.Body)
+		w.Header().Set("Content-Type", "application/json")
+		_, _ = w.Write(buildJSON(42, "scheduled", "https://buildkite.com/test-org/my-pipeline/builds/42"))


require.NoError()

sbalabanov · 2026-07-22T19:11:17Z

+	)
+}
+
+func (c *Client) do(ctx context.Context, method, rawURL string, body []byte, out any) error {


duplicated code with buildkite impl, can deduce?

sure, now both use extracted platform/http.SendRequest

sbalabanov · 2026-07-22T19:13:45Z

+	}
+	if params.Logger == nil {
+		return nil, fmt.Errorf("logger is required")
+	}


Submitqueue design assumes all wiring to be non-nil, so there is no need to explicitly check it. Panic will do just fine.

I see, removed those unnecessary check

Also applied the same fix to submitqueue's buildkite and githubaction's NewBuildRunners, which had identical checks.

sbalabanov · 2026-07-22T19:14:52Z

+		EnvKeyQueue:   r.cfg.QueueName,
+	}
+	if len(metadata) > 0 {
+		metaJSON, _ := json.Marshal(metadata)


plz do not swallow errors

the metadata marshal error is now propagated. Also fixed the same swallowed-marshal-error pattern in submitqueue's buildkite Trigger

sbalabanov · 2026-07-22T19:18:48Z

+
+	_, err := r.Trigger(context.Background(), "", "github://repo/head/aaa", nil)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "response missing workflow_run_id")


Do not assume on error messages - messages are not part of application logic and it makes test unnecessary fragile. AGENTS.md has an instruction to avoid it, surprising it was not picked up.

dropped the message assertion, kept require.Error.

…dupe and clean up the buildkite/githubactions HTTP clients.

…wing it, logging a warning at each call site.

…succeeds instead of discarding its error.

…ding it.

…validating it in NewBuildRunner.

… instead of discarding it.

…igger test.

…ite Trigger instead of discarding them.

…f validating it in NewBuildRunner.

…ead of validating it in NewBuildRunner.

…ad of hardcoded not-found strings, matching buildkite.

roychying added 7 commits July 22, 2026 01:40

feat(platform): add shared buildkite extension client

52e64ce

refactor(submitqueue): rebase buildkite adapter onto shared platform …

7826743

…client

feat(stovepipe): add buildkite-backed BuildRunner adapter

4f58d41

feat(platform): add shared githubactions extension client

a31f44d

refactor(submitqueue): rebase githubactions adapter onto shared platf…

fa7875f

…orm client

feat(stovepipe): add githubactions-backed BuildRunner adapter

85fc006

fix(stovepipe): match reversed BuildRunner.Trigger arg order in backends

3c10122

roychying requested review from a team, behinddwalls and sbalabanov as code owners July 22, 2026 01:42

sbalabanov requested changes Jul 22, 2026

View reviewed changes

roychying added 10 commits July 23, 2026 01:20

Extract platform/http.SendRequest and add buildkite.ErrNotFound to de…

f10d7d3

…dupe and clean up the buildkite/githubactions HTTP clients.

Propagate DecodeMetadataEnv's decode error instead of silently swallo…

d1d1a08

…wing it, logging a warning at each call site.

Extract a buildJSONWithEnv test helper that asserts the JSON marshal …

8042513

…succeeds instead of discarding its error.

Check the w.Write error in TestCreateBuild instead of silently discar…

f2a70a6

…ding it.

Let stovepipe's Buildkite BuildRunner panic on nil wiring instead of …

e8d7e12

…validating it in NewBuildRunner.

Propagate the metadata marshal error in stovepipe's Buildkite Trigger…

e605145

… instead of discarding it.

Stop asserting on error message text in stovepipe's GitHub Actions Tr…

84f125d

…igger test.

Propagate the URI and metadata marshal errors in submitqueue's Buildk…

9a475f8

…ite Trigger instead of discarding them.

Let submitqueue's Buildkite BuildRunner panic on nil wiring instead o…

e0aac07

…f validating it in NewBuildRunner.

Let submitqueue's GitHub Actions BuildRunner panic on nil wiring inst…

0e6f18b

…ead of validating it in NewBuildRunner.

roychying requested a review from sbalabanov July 23, 2026 01:38

Return a githubactions.ErrNotFound sentinel from do()/CancelRun inste…

504c9f0

…ad of hardcoded not-found strings, matching buildkite.

Uh oh!

Conversation

roychying commented Jul 22, 2026

Summary

Test plan

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roychying Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roychying Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roychying Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roychying Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

roychying Jul 22, 2026 •

edited

Loading

roychying Jul 23, 2026 •

edited

Loading

roychying Jul 23, 2026 •

edited

Loading

roychying Jul 23, 2026 •

edited

Loading