feat: add shared platform buildkite/githubactions BuildRunner clients - #423
feat: add shared platform buildkite/githubactions BuildRunner clients#423roychying wants to merge 18 commits into
Conversation
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| respBody, err := io.ReadAll(resp.Body) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
…dupe and clean up the buildkite/githubactions HTTP clients.
…wing it, logging a warning at each call site.
…succeeds instead of discarding its error.
…validating it in NewBuildRunner.
… instead of discarding it.
…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.
3 participants
Summary
platform/extension/buildrunner/and refactor the existing submitqueue adapters to use them.BuildRunneradapters 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)