diff --git a/cmd/api/api.go b/cmd/api/api.go index cae531a..64b9843 100644 --- a/cmd/api/api.go +++ b/cmd/api/api.go @@ -98,7 +98,7 @@ func runAPI(c *cobra.Command, args []string) error { if err != nil { var apiErr *client.APIError if errors.As(err, &apiErr) { - errEnv := output.ErrorEnvelope(apiErr.Code, apiErr.Message, "") + errEnv := output.ErrorEnvelope(apiErr.Code, apiErr.Message, apiErr.Suggestion) return output.Print(errEnv, resolveFormat()) } return err diff --git a/doc/changes/get-retry-and-error-consistency.md b/doc/changes/get-retry-and-error-consistency.md new file mode 100644 index 0000000..5b45005 --- /dev/null +++ b/doc/changes/get-retry-and-error-consistency.md @@ -0,0 +1,45 @@ +# 网络健壮性与错误语义一致性 + +## 背景 + +对标成熟 CLI（gh 等）的生产标准，本次补齐三个缺口： + +1. 任何网络抖动（连接失败、网关 502/503/504、限流 429）都直接失败， + AI Agent / CI 门禁场景下一次瞬态故障即中断整条工作流。 +2. HTTP 层错误（4xx/5xx）不携带修复建议，而 body 错误有——同一错误 + 两种呈现，Agent 无法统一消费。 +3. 生产实测发现：请求不存在的 API 路径时，网关回落到 Web 前端返回 + 200 + HTML 首页，CLI 误判为成功并把整页 HTML 当数据输出 `ok:true`。 +4. 生产实测发现：返回顶层 JSON 数组的遗留端点（如 + `/:owner/:repo/branches`）被降级为转义字符串输出，`--jq`/表格渲染 + 等下游能力全部失效。 + +## 变更内容 + +- `internal/client`：GET（幂等）请求遇瞬态故障自动重试，最多 2 次， + 指数退避（300ms → 600ms）：网络层错误、HTTP 429/502/503/504。 + 服务端返回 `Retry-After` 头（秒）时优先遵循，并以 5s 上限护栏保持 CLI 响应性。 + 非幂等方法（POST/PUT/DELETE/PATCH）一律不重试，避免重复副作用； + 其他状态码不重试。`--debug` 下打印每次重试原因与退避时长。 +- `APIError` 新增 `Suggestion` 字段；HTTP 层错误与 body 错误统一 + 携带 `suggestFix` 修复建议，`api` 命令错误信封透传该建议。 +- 非 JSON 响应若为 HTML 页（Content-Type 或 doctype 探测），返回 + `non_api_response` 错误信封（`ok:false` + 建议检查路径）， + 不再把 HTML 首页当成功数据。 +- 顶层 JSON 数组/标量响应按解码后的结构原样进入数据信封，不再降级为 + 转义字符串。 + +## 生产验证 + +- `api GET /nonexistent-endpoint-xyz` → `ok:false, code:non_api_response` + （修复前：`ok:true` + 整页 HTML）。 +- `api GET /gitlink/gitlink-cli/branches` → `data` 为结构化数组 + （修复前：整个数组被输出为一条转义字符串）。 +- `repo +info` 正常端点行为不变。 + +## 测试 + +`internal/client/retry_test.go` 6 个用例：503 重试后成功、超上限停止、 +POST 不重试、404 不重试、连接拒绝重试后报错、HTML 回落判错。 +`client_test.go` 新增顶层数组解码用例。 +`go test ./...` / `go vet` / `gofmt` 全绿。 diff --git a/internal/client/client.go b/internal/client/client.go index 1fb9d80..66776e5 100644 --- a/internal/client/client.go +++ b/internal/client/client.go @@ -7,7 +7,9 @@ import ( "io" "net/http" "net/url" + "strconv" "strings" + "time" "github.com/gitlink-org/gitlink-cli/internal/auth" "github.com/gitlink-org/gitlink-cli/internal/config" @@ -20,10 +22,55 @@ type Client struct { Debug bool } +// maxGetRetries is the number of extra attempts made for idempotent GET +// requests that fail with a transient network error or a retryable +// gateway status (429/502/503/504). +const maxGetRetries = 2 + +// retryBaseDelay is the initial backoff delay, doubled on each retry. +var retryBaseDelay = 300 * time.Millisecond + +// maxRetryAfter caps how long a server-provided Retry-After header can +// extend the backoff, keeping the CLI responsive. +const maxRetryAfter = 5 * time.Second + +// retryDelay returns the exponential backoff for the given attempt, honoring +// a Retry-After header (in seconds) when the server provides one. +func retryDelay(attempt int, resp *http.Response) time.Duration { + delay := retryBaseDelay << attempt + if resp == nil { + return delay + } + if ra := resp.Header.Get("Retry-After"); ra != "" { + if secs, err := strconv.Atoi(strings.TrimSpace(ra)); err == nil && secs > 0 { + d := time.Duration(secs) * time.Second + if d > maxRetryAfter { + d = maxRetryAfter + } + if d > delay { + delay = d + } + } + } + return delay +} + +func retryableStatus(code int) bool { + switch code { + case http.StatusTooManyRequests, + http.StatusBadGateway, + http.StatusServiceUnavailable, + http.StatusGatewayTimeout: + return true + } + return false +} + type APIError struct { StatusCode int Code interface{} Message string + Suggestion string } func (e *APIError) Error() string { @@ -83,15 +130,40 @@ func (c *Client) Do(method, path string, body interface{}, query url.Values) (*o fmt.Printf("→ %s %s\n", method, fullURL) } - resp, err := c.HTTP.Do(req) - if err != nil { - return nil, fmt.Errorf("request failed: %w", err) - } - defer resp.Body.Close() + var resp *http.Response + var respData []byte + for attempt := 0; ; attempt++ { + resp, err = c.HTTP.Do(req) + if err == nil { + respData, err = io.ReadAll(resp.Body) + resp.Body.Close() + if err != nil { + err = fmt.Errorf("failed to read response: %w", err) + } + } else { + err = fmt.Errorf("request failed: %w", err) + } - respData, err := io.ReadAll(resp.Body) + transient := err != nil || retryableStatus(resp.StatusCode) + if method != http.MethodGet || !transient || attempt >= maxGetRetries { + break + } + var respForDelay *http.Response + if err == nil { + respForDelay = resp + } + delay := retryDelay(attempt, respForDelay) + if c.Debug { + if err != nil { + fmt.Printf("↻ retry %d/%d in %v after error: %v\n", attempt+1, maxGetRetries, delay, err) + } else { + fmt.Printf("↻ retry %d/%d in %v after HTTP %d\n", attempt+1, maxGetRetries, delay, resp.StatusCode) + } + } + time.Sleep(delay) + } if err != nil { - return nil, fmt.Errorf("failed to read response: %w", err) + return nil, err } if c.Debug { @@ -104,12 +176,31 @@ func (c *Client) Do(method, path string, body interface{}, query url.Values) (*o StatusCode: resp.StatusCode, Code: resp.StatusCode, Message: fmt.Sprintf("HTTP %d: %s", resp.StatusCode, strings.TrimSpace(string(respData))), + Suggestion: suggestFix(resp.StatusCode), } } // Parse JSON var raw map[string]interface{} if err := json.Unmarshal(respData, &raw); err != nil { + // Some legacy endpoints (e.g. /:owner/:repo/branches) answer with a + // top-level JSON array or scalar; keep the decoded value instead of + // degrading it to an escaped string. + var nonObject interface{} + if jsonErr := json.Unmarshal(respData, &nonObject); jsonErr == nil { + return output.SuccessEnvelope(nonObject, nil), nil + } + // Unknown API paths fall through to the web frontend, which answers + // 200 with an HTML page; surface that as an error instead of data. + if isHTMLResponse(resp, respData) { + suggestion := "接口路径不存在或不是 API 端点，请检查路径是否正确" + return output.ErrorEnvelope("non_api_response", "endpoint returned an HTML page instead of API data", suggestion), &APIError{ + StatusCode: resp.StatusCode, + Code: "non_api_response", + Message: "endpoint returned an HTML page instead of API data", + Suggestion: suggestion, + } + } // Not JSON, return as-is return output.SuccessEnvelope(string(respData), nil), nil } @@ -144,6 +235,7 @@ func (c *Client) Do(method, path string, body interface{}, query url.Values) (*o StatusCode: int(bodyCode), Code: int(bodyCode), Message: bodyMsg, + Suggestion: suggestion, } } @@ -173,6 +265,14 @@ func (c *Client) Do(method, path string, body interface{}, query url.Values) (*o return output.SuccessEnvelope(raw, meta), nil } +func isHTMLResponse(resp *http.Response, body []byte) bool { + if strings.Contains(resp.Header.Get("Content-Type"), "text/html") { + return true + } + trimmed := strings.TrimSpace(string(body)) + return strings.HasPrefix(trimmed, "GitLink`) + })) + defer server.Close() + + c := &Client{HTTP: server.Client(), BaseURL: server.URL} + env, err := c.Get("/nonexistent", nil) + if err == nil { + t.Fatal("expected error for HTML fallthrough response, got nil") + } + if env == nil || env.OK { + t.Fatalf("env = %+v, want error envelope with OK=false", env) + } +} + +func TestGetRetriesConnectionError(t *testing.T) { + withZeroRetryDelay(t) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) + server.Close() // connection refused from now on + + c := &Client{HTTP: &http.Client{Timeout: 2 * time.Second}, BaseURL: server.URL} + start := time.Now() + _, err := c.Get("/thing", nil) + if err == nil { + t.Fatal("expected error, got nil") + } + if time.Since(start) > 10*time.Second { + t.Fatal("retries took too long") + } +} + +func TestRetryDelayHonorsRetryAfter(t *testing.T) { + mkResp := func(ra string) *http.Response { + h := http.Header{} + if ra != "" { + h.Set("Retry-After", ra) + } + return &http.Response{Header: h} + } + cases := []struct { + name string + attempt int + resp *http.Response + want time.Duration + }{ + {"nil response uses backoff", 0, nil, retryBaseDelay}, + {"second attempt doubles backoff", 1, nil, retryBaseDelay << 1}, + {"retry-after extends delay", 0, mkResp("2"), 2 * time.Second}, + {"retry-after capped", 0, mkResp("60"), maxRetryAfter}, + {"invalid retry-after ignored", 0, mkResp("soon"), retryBaseDelay}, + {"shorter retry-after keeps backoff", 1, mkResp("0"), retryBaseDelay << 1}, + } + for _, tc := range cases { + if got := retryDelay(tc.attempt, tc.resp); got != tc.want { + t.Errorf("%s: retryDelay = %v, want %v", tc.name, got, tc.want) + } + } +}