Normalise HTTP status-based retry policies across the agent

[moskyb]

Description

Across the agent, there are a bunch of interactions with the Buildkite Agent API that all have their own, bespoke retry policies to retry certain HTTP status codes. Many of these policies are very silly, retrying permission errors and the like.

This PR attempts to normalise these policies, adding a helper in the api package called BreakOnNonRetryableStatus, which breaks the roko retrier when the HTTP response has a status code which is non-retryable, and applying this helper across all calls to retried calls to the agent API.

Context

I was working on a thing where in my local environment i was failing meta data gets with a permission error, and had this very silly situation:

where meta data gets failing with a permission error leads to them being retried for ~5 minutes.

Changes

Where calls to the buildkite agent api are being retried through roko, ensure that when those calls receive a status that's non-retryable (any non-2xx that's not one of these)

var retryableStatuses = map[int]bool{
	http.StatusTooManyRequests:     true, // 429
	529:                            true, // Buildkite-specific "still waiting" (used by pipeline upload)
	http.StatusInternalServerError: true, // 500
	http.StatusBadGateway:          true, // 502
	http.StatusServiceUnavailable:  true, // 503
	http.StatusGatewayTimeout:      true, // 504
}

that they break immediately and don't attempt to retry.

Testing

• Tests have run locally (with go test ./...). Buildkite employees may check this if the pipeline has run automatically.
• Code is formatted (with go tool gofumpt -extra -w .)

Disclosures / Credits

[zhming0]

TIL!

[moskyb]

sigh

I misread this 🤦🏿

[DrJosh9000]

sick ✅