diff --git a/clicommand/job_promise_failure.go b/clicommand/job_promise_failure.go index 9462266e21..81a7104323 100644 --- a/clicommand/job_promise_failure.go +++ b/clicommand/job_promise_failure.go @@ -11,7 +11,6 @@ import ( "github.com/buildkite/agent/v3/api" "github.com/buildkite/agent/v3/internal/redact" - "github.com/buildkite/agent/v3/internal/socket" "github.com/buildkite/agent/v3/jobapi" "github.com/buildkite/agent/v3/logger" "github.com/urfave/cli" @@ -113,33 +112,37 @@ var JobPromiseFailureCommand = cli.Command{ return declarePromiseFailureDirectly(ctx, l, cfg, jobID, exitStatus, reason) } - outcome, err := client.DeclarePromiseFailure(ctx, exitStatus, reason) - if err == nil { - if outcome == jobapi.PromiseFailureDebounced { - // Log at debug to avoid spamming job logs on repeated calls. - l.Debugf("Promised exit status %d already declared for job %s (debounced)", exitStatus, jobID) - } else { - l.Infof("Declared promised exit status %d for job %s", exitStatus, jobID) - } - return nil + result, err := client.DeclarePromiseFailure(ctx, exitStatus, reason) + if err != nil { + // We couldn't reach or use the Job API (or its response was lost). + // Declare directly so the promise still lands; the endpoint is idempotent + // for the same exit status, so a duplicate is safe. + l.Warnf("Couldn't reach the Job API to declare the promised failure; declaring it directly: %v", err) + return declarePromiseFailureDirectly(ctx, l, cfg, jobID, exitStatus, reason) } - // The Job API returned a definitive HTTP error: the Buildkite API - // rejected the declaration (409, 422) or was unreachable after retries - // (502). Surface it rather than declaring again. - var apiErr socket.APIErr - if errors.As(err, &apiErr) { - return promiseFailureError(apiErr.StatusCode, err) + if !result.Accepted { + return promiseFailureResultError(result) } - // We couldn't reach the Job API (or its response was lost). Declare - // directly so the promise still lands; the endpoint is idempotent for the - // same exit status, so a duplicate is safe. - l.Warnf("Couldn't reach the Job API to declare the promised failure; declaring it directly: %v", err) - return declarePromiseFailureDirectly(ctx, l, cfg, jobID, exitStatus, reason) + if result.Outcome == jobapi.PromiseFailureDebounced { + // Log at debug to avoid spamming job logs on repeated calls. + l.Debugf("Promised exit status %d already declared for job %s (debounced)", exitStatus, jobID) + } else { + l.Infof("Declared promised exit status %d for job %s", exitStatus, jobID) + } + return nil }, } +func promiseFailureResultError(result *jobapi.PromiseFailureResponse) error { + err := errors.New(result.Error) + if result.Error == "" { + err = errors.New("buildkite API rejected the promised failure") + } + return promiseFailureError(result.UpstreamStatus, err) +} + // promiseFailureError wraps err with a human-readable message for the Buildkite // API status code. The command exits non-zero so the failure is visible to // scripts, which can append '|| true' to ignore it. diff --git a/internal/job/api.go b/internal/job/api.go index 2b85ae1a47..1de01ef969 100644 --- a/internal/job/api.go +++ b/internal/job/api.go @@ -9,6 +9,7 @@ import ( "github.com/buildkite/agent/v3/internal/socket" "github.com/buildkite/agent/v3/jobapi" "github.com/buildkite/agent/v3/logger" + "github.com/buildkite/agent/v3/version" ) // startJobAPI starts the job API server, iff the OS of the box supports it otherwise it returns a @@ -84,8 +85,10 @@ func (e *Executor) declarePromiseFailure(ctx context.Context, exitStatus int, re // logger.Discard keeps the access token (in HTTP debug dumps) out of the job // log; retry warnings still go to the shell logger. apiClient := api.NewClient(logger.Discard, api.Config{ - Endpoint: e.shell.Env.GetString("BUILDKITE_AGENT_ENDPOINT", ""), - Token: e.shell.Env.GetString("BUILDKITE_AGENT_ACCESS_TOKEN", ""), + Endpoint: e.shell.Env.GetString("BUILDKITE_AGENT_ENDPOINT", ""), + Token: e.shell.Env.GetString("BUILDKITE_AGENT_ACCESS_TOKEN", ""), + DisableHTTP2: e.shell.Env.GetBool("BUILDKITE_NO_HTTP2", false), + UserAgent: version.UserAgent(), }) req := &api.JobPromiseFailureRequest{ diff --git a/jobapi/client.go b/jobapi/client.go index 4f1c3ce46f..f69543a0d7 100644 --- a/jobapi/client.go +++ b/jobapi/client.go @@ -127,18 +127,18 @@ func (c *Client) RedactionCreate(ctx context.Context, text string) (string, erro // given exit status and reason to the Buildkite API, blocking until it // completes. The server debounces repeated and concurrent calls for the same // exit status: concurrent callers share one in-flight call, and once it succeeds -// later calls return from the cache. On success it returns the outcome -// (PromiseFailureDeclared or PromiseFailureDebounced). A failed declaration is -// returned as a socket.APIErr carrying the HTTP status code (the Buildkite API's -// status when it responded, otherwise 502). -func (c *Client) DeclarePromiseFailure(ctx context.Context, exitStatus int, reason string) (string, error) { +// later calls return from the cache. If the Job API handles the request, the +// returned response describes whether the Buildkite API accepted or rejected the +// declaration. Errors are reserved for Job API transport/request failures. +func (c *Client) DeclarePromiseFailure(ctx context.Context, exitStatus int, reason string) (*PromiseFailureResponse, error) { req := PromiseFailureRequest{ ExitStatus: exitStatus, Reason: reason, } var resp PromiseFailureResponse if err := c.client.Do(ctx, http.MethodPost, promiseFailureURL, &req, &resp); err != nil { - return "", err + return nil, err } - return resp.Outcome, nil + + return &resp, nil } diff --git a/jobapi/client_test.go b/jobapi/client_test.go index 0bc21c2d63..a32d77504f 100644 --- a/jobapi/client_test.go +++ b/jobapi/client_test.go @@ -65,10 +65,17 @@ func (f *fakeServer) ServeHTTP(w http.ResponseWriter, r *http.Request) { f.promised = append(f.promised, req) // Exit status 99 stands in for a declaration the Buildkite API rejects. if req.ExitStatus == 99 { - _ = socket.WriteError(w, "a different exit status was already declared", http.StatusConflict) + if err := json.NewEncoder(w).Encode(&PromiseFailureResponse{ + Outcome: PromiseFailureDeclared, + Accepted: false, + UpstreamStatus: http.StatusConflict, + Error: "a different exit status was already declared", + }); err != nil { + _ = socket.WriteError(w, fmt.Sprintf("encoding response: %v", err), http.StatusInternalServerError) + } return } - if err := json.NewEncoder(w).Encode(&PromiseFailureResponse{Outcome: PromiseFailureDeclared}); err != nil { + if err := json.NewEncoder(w).Encode(&PromiseFailureResponse{Outcome: PromiseFailureDeclared, Accepted: true}); err != nil { _ = socket.WriteError(w, fmt.Sprintf("encoding response: %v", err), http.StatusInternalServerError) } return @@ -164,23 +171,29 @@ func TestClientDeclarePromiseFailure(t *testing.T) { // A successful declaration returns the outcome and no error, and forwards the // exit status and reason to the server. - outcome, err := cli.DeclarePromiseFailure(ctx, 1, "tests failed") + result, err := cli.DeclarePromiseFailure(ctx, 1, "tests failed") if err != nil { t.Fatalf("cli.DeclarePromiseFailure(1) error = %v", err) } - if outcome != PromiseFailureDeclared { - t.Errorf("cli.DeclarePromiseFailure(1) outcome = %q, want %q", outcome, PromiseFailureDeclared) + if result.Outcome != PromiseFailureDeclared { + t.Errorf("cli.DeclarePromiseFailure(1) outcome = %q, want %q", result.Outcome, PromiseFailureDeclared) + } + if !result.Accepted { + t.Errorf("cli.DeclarePromiseFailure(1) accepted = false, want true") } - // A rejected declaration surfaces a socket.APIErr carrying the Buildkite API - // status code, so the caller can produce an accurate message. - _, err = cli.DeclarePromiseFailure(ctx, 99, "") - var apiErr socket.APIErr - if !errors.As(err, &apiErr) { - t.Fatalf("cli.DeclarePromiseFailure(99) error = %v, want a socket.APIErr", err) + // A rejected declaration is still a successful Job API response. The response + // carries the Buildkite API status code so the caller can produce an accurate + // message without guessing where the error came from. + result, err = cli.DeclarePromiseFailure(ctx, 99, "") + if err != nil { + t.Fatalf("cli.DeclarePromiseFailure(99) error = %v, want nil", err) + } + if result.Accepted { + t.Errorf("cli.DeclarePromiseFailure(99) accepted = true, want false") } - if apiErr.StatusCode != http.StatusConflict { - t.Errorf("apiErr.StatusCode = %d, want %d", apiErr.StatusCode, http.StatusConflict) + if result.UpstreamStatus != http.StatusConflict { + t.Errorf("result.UpstreamStatus = %d, want %d", result.UpstreamStatus, http.StatusConflict) } want := []PromiseFailureRequest{ diff --git a/jobapi/payloads.go b/jobapi/payloads.go index ec6ba5c69f..3f1910953f 100644 --- a/jobapi/payloads.go +++ b/jobapi/payloads.go @@ -89,4 +89,14 @@ const ( type PromiseFailureResponse struct { // Outcome is PromiseFailureDeclared or PromiseFailureDebounced. Outcome string `json:"outcome"` + + // Accepted reports whether the Buildkite API accepted the promised failure. + Accepted bool `json:"accepted"` + + // UpstreamStatus is the Buildkite API status, when one was received. + // Network errors that never received a response leave this as 0. + UpstreamStatus int `json:"upstream_status,omitempty"` + + // Error is the Buildkite API declaration error, if Accepted is false. + Error string `json:"error,omitempty"` } diff --git a/jobapi/promise_failure.go b/jobapi/promise_failure.go index 00e1ca67a2..22f60884fb 100644 --- a/jobapi/promise_failure.go +++ b/jobapi/promise_failure.go @@ -3,8 +3,10 @@ package jobapi import ( "context" "encoding/json" + "errors" "fmt" "net/http" + "sync" "github.com/buildkite/agent/v3/internal/socket" ) @@ -13,9 +15,24 @@ import ( // Buildkite API. The first caller declares it; concurrent callers wait on done // and share the result. type promiseFailure struct { - done chan struct{} // closed when the attempt completes - statusCode int // most recent Buildkite API status (0 if none received) - err error // declaration result; nil means accepted + done chan struct{} // closed when the attempt completes + result PromiseFailureResponse // declaration result +} + +// promiseFailureCoordinator owns promise-failure coalescing and caching. Each +// entry is an in-flight declaration or a cached success or terminal failure; +// transient failures are removed so a later call can retry. +type promiseFailureCoordinator struct { + mu sync.Mutex + entries map[int]*promiseFailure + declare PromiseFailureDeclarer +} + +func newPromiseFailureCoordinator(declare PromiseFailureDeclarer) *promiseFailureCoordinator { + return &promiseFailureCoordinator{ + entries: make(map[int]*promiseFailure), + declare: declare, + } } // terminalStatus reports whether an HTTP status is a definitive client error @@ -25,9 +42,8 @@ func terminalStatus(status int) bool { return status >= 400 && status < 500 && status != http.StatusTooManyRequests } -// handlePromiseFailure declares a promised failure to the Buildkite API for -// 'buildkite-agent job promise-failure', debouncing repeated and concurrent -// calls so each exit status is declared at most once successfully. The first +// Declare declares a promised failure through the configured declarer, +// coalescing repeated and concurrent calls for the same exit status. The first // caller declares it (blocking on the API so it can return an accurate result), // concurrent callers wait and share that outcome, and callers after a success or // a terminal failure return from the cache. Transient failures aren't cached, so @@ -35,6 +51,87 @@ func terminalStatus(status int) bool { // // Debouncing keys on exit status only, so for a given exit status the first // caller's reason wins and later callers' reasons are ignored. +func (c *promiseFailureCoordinator) Declare(ctx context.Context, req PromiseFailureRequest) (result PromiseFailureResponse, err error) { + if c == nil || c.declare == nil { + return PromiseFailureResponse{}, errors.New("promise-failure coordinator is not configured") + } + + c.mu.Lock() + pf, found := c.entries[req.ExitStatus] + if !found { + pf = &promiseFailure{done: make(chan struct{})} + c.entries[req.ExitStatus] = pf + } + c.mu.Unlock() + + if found { + // A declaration is already in progress or done; wait for it and share + // the outcome, unless this caller gives up first. + select { + case <-pf.done: + case <-ctx.Done(): + return PromiseFailureResponse{}, ctx.Err() + } + + result := pf.result + result.Outcome = PromiseFailureDebounced + return result, nil + } + + // We're the first caller, so we declare while others wait on pf.done. Use a + // background context so the declaration isn't abandoned if this caller + // disconnects; the waiters depend on it. + completed := false + defer func() { + if completed { + return + } + // If the declarer panics, release waiters with an error and drop the entry + // so a later call can retry. + // Otherwise pf.done never closes and callers block forever. + v := recover() + pf.result = PromiseFailureResponse{ + Outcome: PromiseFailureDeclared, + Accepted: false, + UpstreamStatus: http.StatusInternalServerError, + Error: fmt.Sprintf("declaring promised failure panicked: %v", v), + } + c.mu.Lock() + delete(c.entries, req.ExitStatus) + close(pf.done) + c.mu.Unlock() + result = pf.result + err = nil + }() + + statusCode, err := c.declare(context.Background(), req.ExitStatus, req.Reason) + pf.result = PromiseFailureResponse{ + Outcome: PromiseFailureDeclared, + Accepted: err == nil, + UpstreamStatus: statusCode, + } + if err != nil { + pf.result.Error = err.Error() + } + completed = true + terminal := terminalStatus(statusCode) + + c.mu.Lock() + if !pf.result.Accepted && !terminal { + // Evict transient failures (5xx, network, 429) so a later call can retry. + // Terminal failures (other 4xx, e.g. 409/422) stay cached so repeated + // calls don't keep hitting the Buildkite API. Waiters already hold pf and + // still see this result either way. + delete(c.entries, req.ExitStatus) + } + close(pf.done) + c.mu.Unlock() + + return pf.result, nil +} + +// handlePromiseFailure handles HTTP concerns for 'buildkite-agent job +// promise-failure'. The coordinator owns declaration, debouncing, and caching. func (s *Server) handlePromiseFailure(w http.ResponseWriter, r *http.Request) { payload := &PromiseFailureRequest{} if err := json.NewDecoder(r.Body).Decode(payload); err != nil { @@ -53,86 +150,19 @@ func (s *Server) handlePromiseFailure(w http.ResponseWriter, r *http.Request) { return } - if s.declarePromiseFailure == nil { - if err := socket.WriteError(w, fmt.Errorf("the Job API server has no promised-failure declarer configured"), http.StatusInternalServerError); err != nil { - s.Logger.Errorf("Job API: couldn't write error: %v", err) - } - return - } - - s.mtx.Lock() - pf, found := s.promiseFailures[payload.ExitStatus] - if !found { - pf = &promiseFailure{done: make(chan struct{})} - s.promiseFailures[payload.ExitStatus] = pf - } - s.mtx.Unlock() - - if found { - // A declaration is already in progress or done; wait for it and share - // the outcome, unless this caller gives up first. - select { - case <-pf.done: - case <-r.Context().Done(): + result, err := s.promiseFailures.Declare(r.Context(), *payload) + if err != nil { + if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { return } - } else { - // We're the first caller, so we declare while others wait on pf.done. - // Use a background context so the declaration isn't abandoned if this - // caller disconnects; the waiters depend on it. - completed := false - defer func() { - if completed { - return - } - // If the declarer panics, release waiters with an error and drop the - // entry so a later call can retry, then re-panic for the recovery - // middleware. Otherwise pf.done never closes and callers block forever. - v := recover() - pf.statusCode = http.StatusInternalServerError - pf.err = fmt.Errorf("declaring promised failure panicked: %v", v) - s.mtx.Lock() - delete(s.promiseFailures, payload.ExitStatus) - close(pf.done) - s.mtx.Unlock() - panic(v) - }() - - pf.statusCode, pf.err = s.declarePromiseFailure(context.Background(), payload.ExitStatus, payload.Reason) - completed = true - - s.mtx.Lock() - if pf.err != nil && !terminalStatus(pf.statusCode) { - // Evict transient failures (5xx, network, 429) so a later call can - // retry. Terminal failures (other 4xx, e.g. 409/422) stay cached so - // repeated calls don't keep hitting the Buildkite API. Waiters - // already hold pf and still see this result either way. - delete(s.promiseFailures, payload.ExitStatus) - } - close(pf.done) - s.mtx.Unlock() - } - if pf.err != nil { - status := pf.statusCode - if status < 400 || status > 599 { - // No usable error status (e.g. a network error, or an unexpected - // non-error status); report a bad gateway. - status = http.StatusBadGateway - } - if err := socket.WriteError(w, pf.err, status); err != nil { + if err := socket.WriteError(w, err, http.StatusInternalServerError); err != nil { s.Logger.Errorf("Job API: couldn't write error: %v", err) } return } - // Success. A leading caller (found == false) declared it; any other caller - // shared that result without calling the Buildkite API. - outcome := PromiseFailureDeclared - if found { - outcome = PromiseFailureDebounced - } - if err := json.NewEncoder(w).Encode(&PromiseFailureResponse{Outcome: outcome}); err != nil { + if err := json.NewEncoder(w).Encode(&result); err != nil { s.Logger.Errorf("Job API: couldn't encode or write response: %v", err) } } diff --git a/jobapi/server.go b/jobapi/server.go index 69065ad859..a67c8589b1 100644 --- a/jobapi/server.go +++ b/jobapi/server.go @@ -28,7 +28,7 @@ func WithDebug() ServerOpts { // tests), the endpoint returns an error. func WithPromiseFailureDeclarer(d PromiseFailureDeclarer) ServerOpts { return func(s *Server) { - s.declarePromiseFailure = d + s.promiseFailures.declare = d } } @@ -55,17 +55,8 @@ type Server struct { // process exits. Guarded by mtx. pendingWorkdir string - // promiseFailures coalesces concurrent and repeated promise-failure calls so - // each exit status is declared to the Buildkite API at most once - // successfully. Each entry is an in-flight declaration or a cached success or - // terminal failure; transient failures are removed so a later call can retry. - // Guarded by mtx. - promiseFailures map[int]*promiseFailure - - // declarePromiseFailure declares a promised failure to the Buildkite API. - // It's nil if the server wasn't configured with a declarer (e.g. in tests), - // in which case the /promise-failure endpoint returns an error. - declarePromiseFailure PromiseFailureDeclarer + // promiseFailures coalesces concurrent and repeated promise-failure calls. + promiseFailures *promiseFailureCoordinator token string sockSvr *socket.Server @@ -91,7 +82,7 @@ func NewServer( Logger: logger, environ: environ, redactors: redactors, - promiseFailures: make(map[int]*promiseFailure), + promiseFailures: newPromiseFailureCoordinator(nil), token: token, } diff --git a/jobapi/server_test.go b/jobapi/server_test.go index 149b5fd3df..77e49f8a37 100644 --- a/jobapi/server_test.go +++ b/jobapi/server_test.go @@ -501,8 +501,8 @@ func startPromiseFailureServer(t *testing.T, declarer jobapi.PromiseFailureDecla } // promiseFailureRequest POSTs a promise-failure declaration to the Job API and -// returns the HTTP status code and, on success, the outcome from the response body. -func promiseFailureRequest(t *testing.T, client *http.Client, token string, exitStatus int) (int, string) { +// returns the HTTP status code and, on success, the response body. +func promiseFailureRequest(t *testing.T, client *http.Client, token string, exitStatus int) (int, *jobapi.PromiseFailureResponse) { t.Helper() buf := &bytes.Buffer{} @@ -523,13 +523,13 @@ func promiseFailureRequest(t *testing.T, client *http.Client, token string, exit defer func() { _ = resp.Body.Close() }() if resp.StatusCode != http.StatusOK { - return resp.StatusCode, "" + return resp.StatusCode, nil } var body jobapi.PromiseFailureResponse if err := json.NewDecoder(resp.Body).Decode(&body); err != nil { t.Fatalf("decoding response: %v", err) } - return resp.StatusCode, body.Outcome + return resp.StatusCode, &body } func TestPromiseFailure(t *testing.T) { @@ -556,12 +556,15 @@ func TestPromiseFailure(t *testing.T) { jobapi.PromiseFailureDeclared, } for i, exitStatus := range []int{1, 1, 2} { - got, outcome := promiseFailureRequest(t, client, token, exitStatus) + got, result := promiseFailureRequest(t, client, token, exitStatus) if got != http.StatusOK { t.Errorf("promiseFailureRequest(%d) status = %d, want %d", exitStatus, got, http.StatusOK) } - if outcome != wantOutcomes[i] { - t.Errorf("promiseFailureRequest(%d) outcome = %q, want %q", exitStatus, outcome, wantOutcomes[i]) + if result.Outcome != wantOutcomes[i] { + t.Errorf("promiseFailureRequest(%d) outcome = %q, want %q", exitStatus, result.Outcome, wantOutcomes[i]) + } + if !result.Accepted { + t.Errorf("promiseFailureRequest(%d) accepted = false, want true", exitStatus) } } @@ -602,13 +605,17 @@ func TestPromiseFailureNotCachedOnTransientError(t *testing.T) { return http.StatusServiceUnavailable, fmt.Errorf("the Buildkite API is unavailable") }) - // A transient failure is surfaced with the Buildkite API's status code, and - // isn't cached: a later call for the same exit status retries. - if got, _ := promiseFailureRequest(t, client, token, 1); got != http.StatusServiceUnavailable { - t.Errorf("first promiseFailureRequest(1) status = %d, want %d", got, http.StatusServiceUnavailable) + // A transient failure is surfaced in the response body, and isn't cached: a + // later call for the same exit status retries. + if got, result := promiseFailureRequest(t, client, token, 1); got != http.StatusOK { + t.Errorf("first promiseFailureRequest(1) status = %d, want %d", got, http.StatusOK) + } else if result.Accepted || result.UpstreamStatus != http.StatusServiceUnavailable { + t.Errorf("first promiseFailureRequest(1) result = %+v, want rejected transient 503", result) } - if got, _ := promiseFailureRequest(t, client, token, 1); got != http.StatusServiceUnavailable { - t.Errorf("second promiseFailureRequest(1) status = %d, want %d", got, http.StatusServiceUnavailable) + if got, result := promiseFailureRequest(t, client, token, 1); got != http.StatusOK { + t.Errorf("second promiseFailureRequest(1) status = %d, want %d", got, http.StatusOK) + } else if result.Accepted || result.UpstreamStatus != http.StatusServiceUnavailable { + t.Errorf("second promiseFailureRequest(1) result = %+v, want rejected transient 503", result) } if got := calls.Load(); got != 2 { @@ -626,12 +633,16 @@ func TestPromiseFailureCachedOnTerminalError(t *testing.T) { }) // A terminal failure is cached: a later call for the same exit status returns - // the same status without re-hitting the Buildkite API. - if got, _ := promiseFailureRequest(t, client, token, 1); got != http.StatusConflict { - t.Errorf("first promiseFailureRequest(1) status = %d, want %d", got, http.StatusConflict) + // the same result without re-hitting the Buildkite API. + if got, result := promiseFailureRequest(t, client, token, 1); got != http.StatusOK { + t.Errorf("first promiseFailureRequest(1) status = %d, want %d", got, http.StatusOK) + } else if result.Accepted || result.UpstreamStatus != http.StatusConflict || result.Outcome != jobapi.PromiseFailureDeclared { + t.Errorf("first promiseFailureRequest(1) result = %+v, want declared terminal 409", result) } - if got, _ := promiseFailureRequest(t, client, token, 1); got != http.StatusConflict { - t.Errorf("second promiseFailureRequest(1) status = %d, want %d", got, http.StatusConflict) + if got, result := promiseFailureRequest(t, client, token, 1); got != http.StatusOK { + t.Errorf("second promiseFailureRequest(1) status = %d, want %d", got, http.StatusOK) + } else if result.Accepted || result.UpstreamStatus != http.StatusConflict || result.Outcome != jobapi.PromiseFailureDebounced { + t.Errorf("second promiseFailureRequest(1) result = %+v, want debounced terminal 409", result) } if got := calls.Load(); got != 1 { @@ -654,10 +665,12 @@ func TestPromiseFailureStatusNormalization(t *testing.T) { return http.StatusNoContent, nil }) - // Both are normalized to 502, so a caller never reads an error as success. + // Both are reported as rejected, so a caller never reads them as accepted. for _, exitStatus := range []int{1, 2} { - if got, _ := promiseFailureRequest(t, client, token, exitStatus); got != http.StatusBadGateway { - t.Errorf("promiseFailureRequest(%d) status = %d, want %d", exitStatus, got, http.StatusBadGateway) + if got, result := promiseFailureRequest(t, client, token, exitStatus); got != http.StatusOK { + t.Errorf("promiseFailureRequest(%d) status = %d, want %d", exitStatus, got, http.StatusOK) + } else if result.Accepted || result.UpstreamStatus != map[int]int{1: 0, 2: http.StatusNoContent}[exitStatus] { + t.Errorf("promiseFailureRequest(%d) result = %+v, want rejected transient", exitStatus, result) } } } @@ -678,7 +691,7 @@ func TestPromiseFailurePanicConcurrent(t *testing.T) { }) // A burst of callers coalesce onto a leader whose declaration panics. The - // leader is recovered to 500 and every waiter must also unblock with 500, + // leader is recovered to a rejected result and every waiter must also unblock, // rather than hanging on a channel that's never closed. (If any caller hung, // the wait below would never return and the test would time out.) const n = 50 @@ -687,8 +700,10 @@ func TestPromiseFailurePanicConcurrent(t *testing.T) { for range n { go func() { defer wg.Done() - if got, _ := promiseFailureRequest(t, client, token, 1); got != http.StatusInternalServerError { - t.Errorf("promiseFailureRequest(1) status = %d, want %d", got, http.StatusInternalServerError) + if got, result := promiseFailureRequest(t, client, token, 1); got != http.StatusOK { + t.Errorf("promiseFailureRequest(1) status = %d, want %d", got, http.StatusOK) + } else if result.Accepted || result.UpstreamStatus != http.StatusInternalServerError { + t.Errorf("promiseFailureRequest(1) result = %+v, want rejected panic result", result) } }() }