amadan
cli: ship survives its own slow uploads, 404s keep their real diagnosis
Fixes from review round 1 on the console API client:
- CRITICAL: doMultipart no longer inherits the JSON verbs' 30s absolute
http.Client Timeout — a large ship could be aborted client-side while
the console's own 10-minute read / 2-minute store budget was still
live, reporting a healthy upload as failed. It now runs on its own
Timeout-free client bounded only by ship's context (a new 15-minute
ctx), with shipHTTPClientTimeout as an injectable seam so a test can
prove the failure mode fast, without a real multi-minute sleep.
- IMPORTANT: apiErrorFromBody tries the JSON {"error":...} decode first
and only falls back to the generic membership message when a 404 body
has no such field — promote's unshipped-version 404 and other real
diagnoses no longer get silently rewritten into "check -account and
-app". channels() now reads its own raw body (getJSON) instead of
going through do() to make the same decision.
- IMPORTANT (brief amendment): promote and ship report noop/created
parity via typed results (apiPromoteResult, apiShipResult) instead of
bare errors — promote keys off the server's "result" field, ship keys
off status (201 vs 200) since apicli.go's identical-reship response
still says "shipped" and only the status code marks it a no-op.
- MINOR: an all-expired (not absent) credential now gets its own
"expired — run: carlos auth login" message instead of being folded
into the direct-mode-or-login suggestion meant for a truly empty
credentials file.
- MINOR: doMultipart closes its pipe reader on the request-construction
error path, so the writer goroutine can't block forever on a pipe
nobody will ever read.
- MINOR: tests clear CARLOS_CONSOLE alongside the two deployment vars.
- MINOR: httptest handlers report via t.Errorf + 500 instead of
t.Fatalf, which is unsafe to call from a goroutine other than the
test's own.
3 files changed,
+423
−49
apimode.go+156 −37apimode_test.go+241 −11auth.go+26 −1
diff --git a/apimode.go b/apimode.go| index a7e8dd4ea..4c6e46ee9 100644 |
| --- a/apimode.go |
| +++ b/apimode.go |
| @@ -28,6 +28,7 @@ import ( |
| "os" |
| "path/filepath" |
| "sort" |
| + "time" |
| ) |
| // apiSession is the resolved API mode: which console, as whom. console and |
| @@ -62,6 +63,17 @@ func resolveMode(consoleFlag string) (*apiSession, error) { |
| // bucket/dir nor a login has TWO ways to fix it, and the naked "not |
| // logged in" message only offers one. |
| if err.Error() == noCredentialsMsg { |
| + // But "not logged in" is also what pick() says when a credential |
| + // EXISTS but every entry on disk has expired — its live-credential |
| + // filter treats an expired entry as absent, so that case reaches |
| + // this same string. Those are different problems for a person to |
| + // fix ("configure a target" vs. "log back in"), and conflating |
| + // them here would tell someone who has in fact logged in before |
| + // that nothing is configured at all. |
| + if expired := expiredCredentialFirst(); expired != nil { |
| + return nil, fmt.Errorf("the credential for %s expired — run: carlos auth login -console %s", |
| + expired.Console, expired.Console) |
| + } |
| return nil, fmt.Errorf("no deployment target configured: " + |
| "set CARLOS_DEPLOYMENT_BUCKET (or CARLOS_DEPLOYMENT_DIR) for direct bucket access, " + |
| "or run carlos auth login -console <url> to work through a console") |
| @@ -76,13 +88,41 @@ func resolveMode(consoleFlag string) (*apiSession, error) { |
| } |
| // noCredentialsMsg is (*credentialFile).pick's exact wording (auth.go) for |
| -// "no -console given and there is nothing to pick from" — the one pick() |
| -// refusal that never mentions direct mode, since pick has no idea direct |
| -// mode exists. Matched by exact string rather than a sentinel error because |
| -// pick constructs a fresh error each call; this is the one pick() message |
| -// with no interpolated value, so an exact match is precise, not fragile. |
| +// "no -console given (and no CARLOS_CONSOLE either) and there is nothing |
| +// live to pick from" — the one pick() refusal that never mentions direct |
| +// mode, since pick has no idea direct mode exists. Matched by exact string |
| +// rather than a sentinel error because pick constructs a fresh error each |
| +// call; this is the one pick() message with no interpolated value, so an |
| +// exact match is precise, not fragile. |
| +// |
| +// It is ALSO the one case pick() reaches both when the credentials file is |
| +// genuinely empty and when it holds only expired entries (pick's "live" |
| +// filter excludes expired credentials before counting), which is why |
| +// resolveMode calls expiredCredentialFirst before treating it as "nothing |
| +// configured". |
| const noCredentialsMsg = "not logged in — run: carlos auth login -console <url>" |
| +// expiredCredentialFirst reports whether the credentials file holds at |
| +// least one entry and every one of them is expired, returning one such |
| +// entry if so (and nil otherwise, including when the file is empty or |
| +// unreadable). Only ever consulted from the noCredentialsMsg branch above, |
| +// where consoleFlag and CARLOS_CONSOLE are already known to both be empty — |
| +// pick() would have taken its console!="" branch, and returned its OWN |
| +// (already specific) expired-credential message, otherwise. |
| +func expiredCredentialFirst() *credential { |
| + f, err := loadCredentials() |
| + if err != nil || len(f.Credentials) == 0 { |
| + return nil |
| + } |
| + now := time.Now() |
| + for i := range f.Credentials { |
| + if !f.Credentials[i].Expired(now) { |
| + return nil |
| + } |
| + } |
| + return &f.Credentials[0] |
| +} |
| + |
| // apiChannel is one row of a channel ladder read back through the console |
| // API, mirroring the runChannels per-row ERROR behaviour: a channel whose |
| // pointer failed verification is a row with Err set, not a failed call. |
| @@ -94,6 +134,22 @@ type apiChannel struct { |
| Err string |
| } |
| +// apiPromoteResult and apiShipResult carry the one fact Task 5 needs to |
| +// print a parity message: whether the console did nothing because the |
| +// request already matched what's live (a promote of a version already on |
| +// that channel; a re-ship of bytes already shipped under that version). |
| +// Every other field of the console's response (rung names, artifact |
| +// hashes) is deliberately not modelled here — the CLI already knows the |
| +// version/channel it asked for and prints those, never the server's echo of |
| +// them. |
| +type apiPromoteResult struct { |
| + NoOp bool |
| +} |
| + |
| +type apiShipResult struct { |
| + NoOp bool |
| +} |
| + |
| // apiPath builds one /api/cli/apps/{account}/{app}/... path. account is |
| // passed through verbatim (sqid or name, exactly what -account carried, |
| // default "ops") — the console resolves it against the caller's own |
| @@ -102,32 +158,63 @@ func apiPath(account, app, tail string) string { |
| return "/api/cli/apps/" + url.PathEscape(account) + "/" + url.PathEscape(app) + tail |
| } |
| -// notFoundErr is what every call here returns for a 404. apiAccountApp |
| -// (internal/console/apicli.go) answers an unknown account, an unknown app, |
| -// and someone else's account with the SAME http.NotFound — a byte-identical |
| -// text/plain body, not JSON, by design (a token holder must not learn which |
| -// of those three it was). This is the one sane message for all three. |
| +// notFoundErr is what every call here returns for a 404 whose body carries |
| +// no JSON "error" field. apiAccountApp (internal/console/apicli.go) answers |
| +// an unknown account, an unknown app, and someone else's account with the |
| +// SAME http.NotFound — a byte-identical text/plain body, not JSON, by design |
| +// (a token holder must not learn which of those three it was). This is the |
| +// one sane message for all three. A 404 that DOES carry an "error" field |
| +// (an unshipped version on promote, a channel with no history on rollback) |
| +// is a genuine server diagnosis, and apiErrorFromBody surfaces THAT text |
| +// instead — never rewritten into this one. |
| func notFoundErr() error { |
| return errors.New("not found: check -account and -app, and that your token's address is a member") |
| } |
| -// apiErrorFromBody turns a non-2xx status and its raw body into an error |
| -// for the plain {"error": "..."} shape every door but the stable-refusal |
| -// uses. 404 is special-cased because that body is text/plain, not JSON — |
| -// json.Unmarshal-ing it would silently produce no error field at all. |
| +// apiErrorFromBody turns a non-2xx status and its raw body into an error. |
| +// The JSON {"error": "..."} shape is tried FIRST, whatever the status code — |
| +// several doors (promote's unshipped-version 404, rollback's no-history 409) |
| +// answer a real diagnosis worth showing verbatim. Only when the body has no |
| +// such field at all — exactly what apiAccountApp's gate sends, a text/plain |
| +// "404 page not found" — does a 404 fall back to the generic membership |
| +// message; every other unreadable body just reports its status. |
| func apiErrorFromBody(status int, body []byte) error { |
| - if status == http.StatusNotFound { |
| - return notFoundErr() |
| - } |
| var e struct { |
| Error string `json:"error"` |
| } |
| if json.Unmarshal(body, &e) == nil && e.Error != "" { |
| return errors.New(e.Error) |
| } |
| + if status == http.StatusNotFound { |
| + return notFoundErr() |
| + } |
| return fmt.Errorf("unexpected response (status %d)", status) |
| } |
| +// getJSON runs one JSON GET and hands back the status and raw body without |
| +// interpreting a non-200 as an error itself, for the same reason postJSON |
| +// doesn't: a caller here needs the FULL body (to try apiErrorFromBody's |
| +// json-first decode), not just a pre-picked message. |
| +func (s *apiSession) getJSON(ctx context.Context, path string) (int, []byte, error) { |
| + req, err := http.NewRequestWithContext(ctx, http.MethodGet, s.c.base+path, nil) |
| + if err != nil { |
| + return 0, nil, err |
| + } |
| + if s.c.token != "" { |
| + req.Header.Set("Authorization", "Bearer "+s.c.token) |
| + } |
| + res, err := s.c.httpc.Do(req) |
| + if err != nil { |
| + return 0, nil, err |
| + } |
| + defer res.Body.Close() |
| + body, err := io.ReadAll(io.LimitReader(res.Body, 1<<20)) |
| + if err != nil { |
| + return res.StatusCode, nil, err |
| + } |
| + return res.StatusCode, body, nil |
| +} |
| + |
| // postJSON runs one JSON POST and hands back the status and raw body |
| // without interpreting a non-200 as an error itself. Unlike |
| // (*consoleClient).do, which keeps only a bare "error" string, promote and |
| @@ -177,29 +264,39 @@ func stableRefusal(status int, body []byte) (error, bool) { |
| return fmt.Errorf("%s\n %s", refusal.Error, refusal.ConsoleURL), true |
| } |
| -// promote asks the console to promote version onto channel. The response |
| -// carries RUNG names (From/To), not versions or channels — deliberately |
| -// unused here: the caller already knows the version and channel it asked |
| -// for and prints those, never res.From/res.To. |
| -func (s *apiSession) promote(ctx context.Context, account, app, version, channel string) error { |
| +// promote asks the console to promote version onto channel. The response's |
| +// "result" is the one field read here — "noop" when the ladder already had |
| +// this version on this channel — because everything else the response |
| +// carries (RUNG names in From/To) is not what the caller prints: the CLI |
| +// already knows the version/channel it asked for and prints those, never |
| +// res.From/res.To. |
| +func (s *apiSession) promote(ctx context.Context, account, app, version, channel string) (apiPromoteResult, error) { |
| in := struct { |
| Version string `json:"version"` |
| Channel string `json:"channel"` |
| }{version, channel} |
| status, body, err := s.postJSON(ctx, apiPath(account, app, "/promote"), in) |
| if err != nil { |
| - return err |
| + return apiPromoteResult{}, err |
| } |
| if status == http.StatusOK { |
| - return nil |
| + var res struct { |
| + Result string `json:"result"` |
| + } |
| + if err := json.Unmarshal(body, &res); err != nil { |
| + return apiPromoteResult{}, fmt.Errorf("promote: unreadable response: %w", err) |
| + } |
| + return apiPromoteResult{NoOp: res.Result == "noop"}, nil |
| } |
| if refErr, ok := stableRefusal(status, body); ok { |
| - return refErr |
| + return apiPromoteResult{}, refErr |
| } |
| - return apiErrorFromBody(status, body) |
| + return apiPromoteResult{}, apiErrorFromBody(status, body) |
| } |
| // rollback asks the console to roll channel back to its previous version. |
| +// There is no noop case for a rollback — it either has a previous version to |
| +// land on (409 if not) or it doesn't — so this stays error-only. |
| func (s *apiSession) rollback(ctx context.Context, account, app, channel string) error { |
| in := struct { |
| Channel string `json:"channel"` |
| @@ -219,6 +316,13 @@ func (s *apiSession) rollback(ctx context.Context, account, app, channel string) |
| // channels reads one app's channel ladder through the console. |
| func (s *apiSession) channels(ctx context.Context, account, app string) ([]apiChannel, error) { |
| + status, body, err := s.getJSON(ctx, apiPath(account, app, "/channels")) |
| + if err != nil { |
| + return nil, err |
| + } |
| + if status != http.StatusOK { |
| + return nil, apiErrorFromBody(status, body) |
| + } |
| var res struct { |
| Channels []struct { |
| Channel string `json:"channel"` |
| @@ -228,12 +332,8 @@ func (s *apiSession) channels(ctx context.Context, account, app string) ([]apiCh |
| Error string `json:"error"` |
| } `json:"channels"` |
| } |
| - status, err := s.c.do(ctx, http.MethodGet, apiPath(account, app, "/channels"), nil, &res) |
| - if err != nil { |
| - if status == http.StatusNotFound { |
| - return nil, notFoundErr() |
| - } |
| - return nil, err |
| + if err := json.Unmarshal(body, &res); err != nil { |
| + return nil, fmt.Errorf("channels: unreadable response: %w", err) |
| } |
| out := make([]apiChannel, len(res.Channels)) |
| for i, ch := range res.Channels { |
| @@ -245,11 +345,22 @@ func (s *apiSession) channels(ctx context.Context, account, app string) ([]apiCh |
| return out, nil |
| } |
| +// shipTimeout bounds ship's own context — generous on purpose. The console |
| +// gives itself up to 10 minutes to read the request body and 2 minutes to |
| +// store it (internal/console/apicli.go), well past the 30s absolute Timeout |
| +// the JSON verbs' shared client carries; ship must never inherit that 30s |
| +// cap (doMultipart, auth.go, uses its own client for exactly this reason), |
| +// but it still needs SOME bound of its own rather than running forever. |
| +const shipTimeout = 15 * time.Minute |
| + |
| // ship uploads one release's artifacts as a multipart POST: the meta part |
| // ({"version","kind"}) first, then one file part per entry in files (partName |
| // -> local path), in map-key-sorted order — sorted so the wire bytes are |
| // deterministic, since Go's map iteration order is not. |
| -func (s *apiSession) ship(ctx context.Context, account, app, version, kind string, files map[string]string) error { |
| +func (s *apiSession) ship(ctx context.Context, account, app, version, kind string, files map[string]string) (apiShipResult, error) { |
| + ctx, cancel := context.WithTimeout(ctx, shipTimeout) |
| + defer cancel() |
| + |
| names := make([]string, 0, len(files)) |
| for name := range files { |
| names = append(names, name) |
| @@ -279,12 +390,20 @@ func (s *apiSession) ship(ctx context.Context, account, app, version, kind strin |
| return nil |
| }) |
| if err != nil { |
| - return err |
| + return apiShipResult{}, err |
| } |
| - if status == http.StatusOK || status == http.StatusCreated { |
| - return nil |
| + // handleAPIShip (internal/console/apicli.go) sends "result": "shipped" |
| + // on BOTH paths — status is the only signal that distinguishes them: 201 |
| + // for bytes actually written, 200 when the request matched an identical |
| + // version already on record and nothing new was stored. |
| + switch status { |
| + case http.StatusCreated: |
| + return apiShipResult{NoOp: false}, nil |
| + case http.StatusOK: |
| + return apiShipResult{NoOp: true}, nil |
| + default: |
| + return apiShipResult{}, apiErrorFromBody(status, body) |
| } |
| - return apiErrorFromBody(status, body) |
| } |
| // writeFilePart streams one local file into a multipart part named name, so |
diff --git a/apimode_test.go b/apimode_test.go| index b551b9bd2..5fdf686d3 100644 |
| --- a/apimode_test.go |
| +++ b/apimode_test.go |
| @@ -17,13 +17,16 @@ import ( |
| "time" |
| ) |
| -// clearDeploymentEnv ensures neither direct-mode env var leaks in from the |
| -// process that's running the test binary — resolveMode must see a clean |
| -// slate to make its own env check meaningful. |
| +// clearDeploymentEnv ensures none of the three env vars resolveMode/pick |
| +// read leak in from the process running the test binary — resolveMode must |
| +// see a clean slate to make its own env check meaningful, and CARLOS_CONSOLE |
| +// left set would silently change which pick() branch a "" -console flag |
| +// takes. |
| func clearDeploymentEnv(t *testing.T) { |
| t.Helper() |
| t.Setenv("CARLOS_DEPLOYMENT_BUCKET", "") |
| t.Setenv("CARLOS_DEPLOYMENT_DIR", "") |
| + t.Setenv("CARLOS_CONSOLE", "") |
| } |
| func TestResolveModeDirectWins(t *testing.T) { |
| @@ -104,6 +107,41 @@ func TestResolveModeNothing(t *testing.T) { |
| } |
| } |
| +// TestResolveModeExpiredCredentialOnly pins the minor fix: a credential that |
| +// EXISTS but has expired is a different situation from nothing configured at |
| +// all, and deserves a message that says so instead of offering to set up |
| +// direct mode from scratch. |
| +func TestResolveModeExpiredCredentialOnly(t *testing.T) { |
| + useTempCredentials(t) |
| + clearDeploymentEnv(t) |
| + |
| + f, _ := loadCredentials() |
| + f.put(credential{Console: "https://a.example", Address: "a@keymail.dev", Token: "t1", ExpiresAt: time.Now().Add(-time.Hour)}) |
| + if err := f.save(); err != nil { |
| + t.Fatal(err) |
| + } |
| + |
| + sess, err := resolveMode("") |
| + if sess != nil { |
| + t.Fatalf("resolveMode with only an expired credential = %+v, want nil session", sess) |
| + } |
| + if err == nil { |
| + t.Fatal("resolveMode with only an expired credential must error") |
| + } |
| + if !strings.Contains(err.Error(), "https://a.example") { |
| + t.Fatalf("error %q does not name the expired console", err) |
| + } |
| + if !strings.Contains(err.Error(), "expired") { |
| + t.Fatalf("error %q does not say the credential expired", err) |
| + } |
| + if !strings.Contains(err.Error(), "carlos auth login") { |
| + t.Fatalf("error %q does not name the fix", err) |
| + } |
| + if strings.Contains(err.Error(), "CARLOS_DEPLOYMENT_BUCKET") { |
| + t.Fatalf("error %q wrongly offers direct mode for an expired (not absent) credential", err) |
| + } |
| +} |
| + |
| func newTestSession(base string) *apiSession { |
| return &apiSession{c: newConsoleClient(base, "cli-token"), console: base, address: "a@keymail.dev"} |
| } |
| @@ -120,7 +158,9 @@ func TestAPISessionShipMultipart(t *testing.T) { |
| gotAuth = r.Header.Get("Authorization") |
| mr, err := r.MultipartReader() |
| if err != nil { |
| - t.Fatalf("request was not multipart: %v", err) |
| + t.Errorf("request was not multipart: %v", err) |
| + w.WriteHeader(http.StatusInternalServerError) |
| + return |
| } |
| for { |
| part, err := mr.NextPart() |
| @@ -128,11 +168,15 @@ func TestAPISessionShipMultipart(t *testing.T) { |
| break |
| } |
| if err != nil { |
| - t.Fatalf("reading part: %v", err) |
| + t.Errorf("reading part: %v", err) |
| + w.WriteHeader(http.StatusInternalServerError) |
| + return |
| } |
| b, err := io.ReadAll(part) |
| if err != nil { |
| - t.Fatalf("reading part body: %v", err) |
| + t.Errorf("reading part body: %v", err) |
| + w.WriteHeader(http.StatusInternalServerError) |
| + return |
| } |
| if part.FormName() == "meta" { |
| metaBody = string(b) |
| @@ -157,13 +201,16 @@ func TestAPISessionShipMultipart(t *testing.T) { |
| } |
| s := newTestSession(ts.URL) |
| - err := s.ship(context.Background(), "acct", "app", "1.2.3", "binary", map[string]string{ |
| + res, err := s.ship(context.Background(), "acct", "app", "1.2.3", "binary", map[string]string{ |
| "zeta-amd64": fileB, |
| "alpha-amd64": fileA, |
| }) |
| if err != nil { |
| t.Fatalf("ship: %v", err) |
| } |
| + if res.NoOp { |
| + t.Fatal("a fresh (201) ship must not report NoOp") |
| + } |
| if gotAuth != "Bearer cli-token" { |
| t.Fatalf("Authorization header = %q, want %q", gotAuth, "Bearer cli-token") |
| } |
| @@ -181,6 +228,51 @@ func TestAPISessionShipMultipart(t *testing.T) { |
| } |
| } |
| +// TestAPISessionShipNoOp pins handleAPIShip's identical-reship response |
| +// exactly as apicli.go sends it: still "result": "shipped" — 200 rather |
| +// than 201 is the ONLY signal that nothing new was written, so NoOp must be |
| +// keyed off status, not the result string. |
| +func TestAPISessionShipNoOp(t *testing.T) { |
| + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
| + mr, err := r.MultipartReader() |
| + if err != nil { |
| + t.Errorf("request was not multipart: %v", err) |
| + w.WriteHeader(http.StatusInternalServerError) |
| + return |
| + } |
| + for { |
| + _, err := mr.NextPart() |
| + if err == io.EOF { |
| + break |
| + } |
| + if err != nil { |
| + t.Errorf("reading part: %v", err) |
| + w.WriteHeader(http.StatusInternalServerError) |
| + return |
| + } |
| + } |
| + w.WriteHeader(http.StatusOK) |
| + json.NewEncoder(w).Encode(map[string]any{ |
| + "result": "shipped", "version": "1.2.3", "artifacts": map[string]any{}, |
| + }) |
| + })) |
| + defer ts.Close() |
| + |
| + dir := t.TempDir() |
| + file := filepath.Join(dir, "a.bin") |
| + if err := os.WriteFile(file, []byte("aaa"), 0o600); err != nil { |
| + t.Fatal(err) |
| + } |
| + s := newTestSession(ts.URL) |
| + res, err := s.ship(context.Background(), "acct", "app", "1.2.3", "binary", map[string]string{"alpha-amd64": file}) |
| + if err != nil { |
| + t.Fatalf("ship: %v", err) |
| + } |
| + if !res.NoOp { |
| + t.Fatalf("ship result = %+v, want NoOp true for a 200 identical re-ship", res) |
| + } |
| +} |
| + |
| func TestAPISessionShipErrorSurfacesServerMessage(t *testing.T) { |
| ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
| w.WriteHeader(http.StatusConflict) |
| @@ -196,12 +288,82 @@ func TestAPISessionShipErrorSurfacesServerMessage(t *testing.T) { |
| t.Fatal(err) |
| } |
| s := newTestSession(ts.URL) |
| - err := s.ship(context.Background(), "acct", "app", "1.2.3", "binary", map[string]string{"alpha-amd64": file}) |
| + _, err := s.ship(context.Background(), "acct", "app", "1.2.3", "binary", map[string]string{"alpha-amd64": file}) |
| if err == nil || !strings.Contains(err.Error(), "already shipped with different content") { |
| t.Fatalf("ship error = %v, want the server's conflict message", err) |
| } |
| } |
| +// TestAPISessionShipSurvivesSlowServer is the CRITICAL fix's proof: a slow |
| +// server holds the connection open longer than an old-style absolute client |
| +// Timeout would tolerate, but well within ship's own generous context. |
| +// shipHTTPClientTimeout (auth.go) is the injectable seam that lets both |
| +// halves of the point run fast, without an actual multi-minute sleep: |
| +// shrunk to a few milliseconds, the request fails exactly the way the |
| +// pre-fix code (which reused the JSON verbs' 30s-Timeout client) would have |
| +// on a real large upload; restored to its production zero value, the same |
| +// slow server succeeds because only the context bounds the request. |
| +func TestAPISessionShipSurvivesSlowServer(t *testing.T) { |
| + t.Run("an absolute client Timeout fails against a slow server", func(t *testing.T) { |
| + release := make(chan struct{}) |
| + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
| + <-release |
| + io.Copy(io.Discard, r.Body) |
| + w.WriteHeader(http.StatusCreated) |
| + json.NewEncoder(w).Encode(map[string]any{"result": "shipped", "version": "1.2.3", "artifacts": map[string]any{}}) |
| + })) |
| + defer func() { |
| + close(release) |
| + ts.Close() |
| + }() |
| + |
| + dir := t.TempDir() |
| + file := filepath.Join(dir, "a.bin") |
| + if err := os.WriteFile(file, []byte("aaa"), 0o600); err != nil { |
| + t.Fatal(err) |
| + } |
| + s := newTestSession(ts.URL) |
| + |
| + old := shipHTTPClientTimeout |
| + shipHTTPClientTimeout = 30 * time.Millisecond |
| + defer func() { shipHTTPClientTimeout = old }() |
| + |
| + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) |
| + defer cancel() |
| + _, err := s.ship(ctx, "acct", "app", "1.2.3", "binary", map[string]string{"alpha-amd64": file}) |
| + if err == nil { |
| + t.Fatal("ship with a tiny absolute client Timeout against a slow-to-answer server must fail") |
| + } |
| + }) |
| + |
| + t.Run("no absolute timeout survives the same slow server", func(t *testing.T) { |
| + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
| + time.Sleep(150 * time.Millisecond) // longer than the tiny timeout above; well inside ship's context |
| + io.Copy(io.Discard, r.Body) |
| + w.WriteHeader(http.StatusCreated) |
| + json.NewEncoder(w).Encode(map[string]any{"result": "shipped", "version": "1.2.3", "artifacts": map[string]any{}}) |
| + })) |
| + defer ts.Close() |
| + |
| + dir := t.TempDir() |
| + file := filepath.Join(dir, "a.bin") |
| + if err := os.WriteFile(file, []byte("aaa"), 0o600); err != nil { |
| + t.Fatal(err) |
| + } |
| + s := newTestSession(ts.URL) |
| + // shipHTTPClientTimeout is back at its zero production default here |
| + // (the sibling subtest above restored it) — only the context bounds |
| + // this call. |
| + res, err := s.ship(context.Background(), "acct", "app", "1.2.3", "binary", map[string]string{"alpha-amd64": file}) |
| + if err != nil { |
| + t.Fatalf("ship against a slow-but-healthy server = %v, want success", err) |
| + } |
| + if res.NoOp { |
| + t.Fatal("a fresh (201) ship must not report NoOp") |
| + } |
| + }) |
| +} |
| + |
| // TestAPISessionStableRefusal pins handleAPIPromote's 403 shape: the CLI |
| // never resolves stable itself — it surfaces the console's own error AND the |
| // console_url it hands back, because do() only keeps the bare "error" |
| @@ -217,7 +379,7 @@ func TestAPISessionStableRefusal(t *testing.T) { |
| defer ts.Close() |
| s := newTestSession(ts.URL) |
| - err := s.promote(context.Background(), "acct", "app", "1.2.3", "canary") |
| + _, err := s.promote(context.Background(), "acct", "app", "1.2.3", "canary") |
| if err == nil { |
| t.Fatal("promote against a stable refusal must error") |
| } |
| @@ -255,16 +417,84 @@ func TestAPISessionPromoteSuccess(t *testing.T) { |
| var req struct{ Version, Channel string } |
| json.NewDecoder(r.Body).Decode(&req) |
| if req.Version != "1.2.3" || req.Channel != "canary/a" { |
| - t.Fatalf("promote body = %+v", req) |
| + t.Errorf("promote body = %+v", req) |
| + w.WriteHeader(http.StatusInternalServerError) |
| + return |
| } |
| json.NewEncoder(w).Encode(map[string]any{"result": "promoted", "from": "", "to": "canary"}) |
| })) |
| defer ts.Close() |
| s := newTestSession(ts.URL) |
| - if err := s.promote(context.Background(), "acct", "app", "1.2.3", "canary/a"); err != nil { |
| + res, err := s.promote(context.Background(), "acct", "app", "1.2.3", "canary/a") |
| + if err != nil { |
| + t.Fatalf("promote: %v", err) |
| + } |
| + if res.NoOp { |
| + t.Fatal(`promote result = NoOp true, want false for a "promoted" result`) |
| + } |
| +} |
| + |
| +// TestAPISessionPromoteNoOp pins the noop-parity fix: PromoteOnLadder |
| +// already found this version on this channel, and handleAPIPromote answers |
| +// {"result": "noop"} (still 200) — promote() must report that as NoOp true |
| +// rather than folding it into an ordinary success indistinguishable from an |
| +// actual promotion. |
| +func TestAPISessionPromoteNoOp(t *testing.T) { |
| + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
| + json.NewEncoder(w).Encode(map[string]any{"result": "noop"}) |
| + })) |
| + defer ts.Close() |
| + |
| + s := newTestSession(ts.URL) |
| + res, err := s.promote(context.Background(), "acct", "app", "1.2.3", "canary/a") |
| + if err != nil { |
| t.Fatalf("promote: %v", err) |
| } |
| + if !res.NoOp { |
| + t.Fatalf(`promote result = %+v, want NoOp true for a "noop" result`, res) |
| + } |
| +} |
| + |
| +// TestAPISessionPromoteUnshippedVersionSurfacesServerText pins the |
| +// IMPORTANT fix: a 404 with a real JSON diagnosis (an unshipped version, |
| +// handleAPIPromote's release.GetManifest check) must reach the caller |
| +// verbatim, not be swallowed into the generic membership message that only |
| +// belongs to apiAccountApp's gate. |
| +func TestAPISessionPromoteUnshippedVersionSurfacesServerText(t *testing.T) { |
| + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
| + w.WriteHeader(http.StatusNotFound) |
| + json.NewEncoder(w).Encode(map[string]string{ |
| + "error": "Version 9.9.9 is not a shipped release of app.", |
| + }) |
| + })) |
| + defer ts.Close() |
| + |
| + s := newTestSession(ts.URL) |
| + _, err := s.promote(context.Background(), "acct", "app", "9.9.9", "canary/a") |
| + if err == nil || !strings.Contains(err.Error(), "not a shipped release") { |
| + t.Fatalf("promote 404 with a JSON diagnosis = %v, want the server's own text", err) |
| + } |
| + if strings.Contains(err.Error(), "check -account and -app") { |
| + t.Fatalf("promote 404 with a real diagnosis must not be replaced by the membership message: %v", err) |
| + } |
| +} |
| + |
| +// TestAPISessionPromoteAccountGateNotFound is the companion case: the SAME |
| +// 404 status, but apiAccountApp's gate (an unknown account/app or one the |
| +// caller doesn't belong to) — a text/plain http.NotFound body with no |
| +// "error" field at all — must still get the generic membership message. |
| +func TestAPISessionPromoteAccountGateNotFound(t *testing.T) { |
| + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
| + http.NotFound(w, r) |
| + })) |
| + defer ts.Close() |
| + |
| + s := newTestSession(ts.URL) |
| + _, err := s.promote(context.Background(), "nope", "app", "1.2.3", "canary/a") |
| + if err == nil || !strings.Contains(err.Error(), "not found") || !strings.Contains(err.Error(), "-account") { |
| + t.Fatalf("promote against the account/app gate 404 = %v, want the membership message", err) |
| + } |
| } |
| // TestAPISessionChannelsNotFound pins the 404 case the parent task called |
diff --git a/auth.go b/auth.go| index 44a5d46d2..fba2b8a39 100644 |
| --- a/auth.go |
| +++ b/auth.go |
| @@ -395,6 +395,22 @@ func (c *consoleClient) do(ctx context.Context, method, path string, in, out any |
| return res.StatusCode, nil |
| } |
| +// shipHTTPClientTimeout is the ABSOLUTE per-request Timeout doMultipart's own |
| +// client carries — zero (the production default) means none: a ship relies |
| +// solely on its caller's context deadline (apimode.go's ship gives itself a |
| +// generous 15 minutes), because the console's own budget for a big upload — |
| +// up to 10 minutes to read the body, up to 2 more to store it |
| +// (internal/console/apicli.go) — already exceeds the 30s absolute Timeout |
| +// c.httpc carries for the JSON verbs. Reusing that client here would abort a |
| +// large, still-healthy upload CLIENT-side while the server was still working |
| +// on it, reporting a live ship as failed. |
| +// |
| +// A var only so a test can shrink it far below what any real upload takes — |
| +// set it small against a deliberately slow server and the request fails |
| +// fast, proving the failure mode the zero-Timeout client is built to avoid, |
| +// without an actual multi-minute sleep to demonstrate it. |
| +var shipHTTPClientTimeout time.Duration |
| + |
| // doMultipart runs one multipart/form-data POST, streaming the body through |
| // an io.Pipe rather than buffering it whole: write feeds the multipart.Writer |
| // from a goroutine while http.Client reads the other end of the pipe as it |
| @@ -417,13 +433,22 @@ func (c *consoleClient) doMultipart(ctx context.Context, path string, write func |
| }() |
| req, err := http.NewRequestWithContext(ctx, http.MethodPost, c.base+path, pr) |
| if err != nil { |
| + // The write goroutine above is already running (or about to run) and |
| + // will otherwise block forever trying to write to a pipe nobody ever |
| + // reads: closing our end unblocks it with io.ErrClosedPipe instead of |
| + // leaking it. |
| + pr.Close() |
| return 0, nil, err |
| } |
| req.Header.Set("Content-Type", mw.FormDataContentType()) |
| if c.token != "" { |
| req.Header.Set("Authorization", "Bearer "+c.token) |
| } |
| - res, err := c.httpc.Do(req) |
| + // A client of its own — see shipHTTPClientTimeout above — never c.httpc, |
| + // whose 30s absolute Timeout is sized for the JSON verbs, not a large |
| + // upload. |
| + client := &http.Client{Timeout: shipHTTPClientTimeout} |
| + res, err := client.Do(req) |
| if err != nil { |
| return 0, nil, err |
| } |