diff --git a/shortcuts/apps/apps_rate_limit_hint_test.go b/shortcuts/apps/apps_rate_limit_hint_test.go new file mode 100644 index 0000000000..0bdafb5c3a --- /dev/null +++ b/shortcuts/apps/apps_rate_limit_hint_test.go @@ -0,0 +1,180 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package apps + +import ( + "encoding/json" + "errors" + "strings" + "testing" + + "github.com/larksuite/cli/errs" + "github.com/larksuite/cli/internal/errclass" + "github.com/larksuite/cli/internal/httpmock" +) + +// appsGatewayRateLimitBody is the OpenAPI gateway throttling envelope. 99991400 is +// registered in internal/errclass as api/rate_limit, retryable. +func appsGatewayRateLimitBody() map[string]interface{} { + return map[string]interface{}{ + "code": 99991400, + "msg": "request trigger frequency limit", + "error": map[string]interface{}{ + "log_id": "log-placeholder", + }, + } +} + +// A rate-limited +db-execute was answered with "verify table/column names ... +// target the dev database with --environment dev": advice about the request, for +// a failure that says nothing about the request. An agent following it sends more +// requests into the same limit. The hint must point at backing off instead. +func TestAppsDBExecute_RateLimitHintIsBackoffNotRequestFix(t *testing.T) { + for _, status := range []int{200, 429} { + t.Run(rateLimitStatusName(status), func(t *testing.T) { + factory, stdout, reg := newAppsExecuteFactory(t) + reg.Register(&httpmock.Stub{ + Method: "POST", + URL: "/open-apis/spark/v1/apps/app_x/sql_commands", + Status: status, + Body: appsGatewayRateLimitBody(), + }) + err := runAppsShortcut(t, AppsDBExecute, + []string{"+db-execute", "--yes", "--app-id", "app_x", "--sql", "select 1", "--as", "user"}, + factory, stdout) + p := requireAppsAPIProblem(t, err) + raw, _ := json.Marshal(err) + t.Logf("error envelope: %s", raw) + + // Classification is already right; only the hint is wrong. + if p.Subtype != errs.SubtypeRateLimit || p.Code != 99991400 || !p.Retryable { + t.Fatalf("subtype/code/retryable = %q/%d/%t, want rate_limit/99991400/true", p.Subtype, p.Code, p.Retryable) + } + for _, requestAdvice := range []string{"+db-table-get", "--environment dev", "table/column"} { + if strings.Contains(p.Hint, requestAdvice) { + t.Errorf("rate-limited +db-execute still tells the caller to change the request (%q): hint=%q", requestAdvice, p.Hint) + } + } + if !strings.Contains(p.Hint, "backoff") { + t.Errorf("hint = %q, want backoff guidance for a throttled request", p.Hint) + } + }) + } +} + +// The apps commands that attach a request-shaped hint do it through withAppsHint +// (79 sites), withObservabilityHint (2) or withDBSyncHint (6). Drive a real +// 99991400 classification through each helper with the request-shaped hints the +// hottest commands pass, so the fix is proven at the chokepoint and not per +// command. Commands that return the client error as is (+list, for one) keep the +// classifier's rate_limit with an empty hint, which misleads no one; commands +// with their own error helpers are outside this test. +func TestAppsHints_RateLimitDoesNotInheritRequestHint(t *testing.T) { + dbExecuteHint := "verify table/column names with `lark-cli apps +db-table-get --app-id app_x --table `; for day-to-day debugging target the dev database with `--environment dev`" + for _, tc := range []struct { + name string + requestHint string + apply func(error) error + }{ + {"+trace-get (appIDListHint)", appIDListHint, func(err error) error { return withAppsHint(err, appIDListHint) }}, + {"+db-execute", dbExecuteHint, func(err error) error { return withAppsHint(err, dbExecuteHint) }}, + {"+session-messages-list", sessionMessagesListHint, func(err error) error { return withAppsHint(err, sessionMessagesListHint) }}, + {"+metric-list / +analytics-list (withObservabilityHint)", appIDListHint, withObservabilityHint}, + {"+db-sync-list (withDBSyncHint)", dbSyncListHint, func(err error) error { return withDBSyncHint(err, dbSyncListHint) }}, + } { + t.Run(tc.name, func(t *testing.T) { + in := errclass.BuildAPIError(appsGatewayRateLimitBody(), errclass.ClassifyContext{Identity: "user"}) + out := tc.apply(in) + if out != in { + t.Fatalf("helper returned a different error value; the cause chain must survive") + } + p, ok := errs.ProblemOf(out) + if !ok { + t.Fatalf("not a typed problem: %T", out) + } + t.Logf("hint after helper: %q", p.Hint) + if p.Subtype != errs.SubtypeRateLimit || p.Code != 99991400 || !p.Retryable { + t.Fatalf("classification mutated: subtype=%q code=%d retryable=%t", p.Subtype, p.Code, p.Retryable) + } + if p.Hint == tc.requestHint { + t.Errorf("rate limit inherited the request-shaped hint %q", p.Hint) + } + if !strings.Contains(p.Hint, "backoff") { + t.Errorf("hint = %q, want backoff guidance", p.Hint) + } + }) + } +} + +// The rate-limit branch only fills an empty hint: wording the upstream sent (a +// lifted error.details value, or the stream client's retry-after sentence) wins, +// exactly as it does for every other class. +func TestWithAppsHint_RateLimitKeepsUpstreamHint(t *testing.T) { + cause := errors.New("upstream cause") + in := errs.NewAPIError(errs.SubtypeRateLimit, "HTTP 429: throttled").WithCode(429).WithRetryable(). + WithHint("wait at least 3 seconds before retrying; use exponential backoff with jitter if throttling continues"). + WithCause(cause) + out := withAppsHint(in, appIDListHint) + p, _ := errs.ProblemOf(out) + if p.Hint != "wait at least 3 seconds before retrying; use exponential backoff with jitter if throttling continues" { + t.Fatalf("hint = %q, want the upstream retry-after hint preserved", p.Hint) + } + if !errors.Is(out, cause) { + t.Fatalf("cause chain dropped") + } +} + +// Rate-limit classification is stronger evidence than message text. The same +// helpers also recognize database state from message markers, but a gateway +// throttle can carry arbitrary upstream wording and was not evaluated by the +// database service. Those markers must not turn a throttle into request advice. +func TestWithAppsHint_RateLimitWinsOverDatabaseMessageMarkers(t *testing.T) { + for _, message := range []string{ + "k_dl_4000004: request trigger frequency limit", + "no db branch: request trigger frequency limit", + } { + t.Run(message, func(t *testing.T) { + in := errs.NewAPIError(errs.SubtypeRateLimit, "%s", message). + WithCode(99991400).WithRetryable() + out := withAppsHint(in, appIDListHint) + p, _ := errs.ProblemOf(out) + if p.Subtype != errs.SubtypeRateLimit || p.Code != 99991400 || !p.Retryable { + t.Fatalf("classification mutated: subtype=%q code=%d retryable=%t", p.Subtype, p.Code, p.Retryable) + } + if p.Message != message { + t.Fatalf("message = %q, want original %q", p.Message, message) + } + if p.Hint != appRateLimitHint { + t.Fatalf("hint = %q, want rate-limit backoff hint %q", p.Hint, appRateLimitHint) + } + }) + } +} + +// The observability wrapper has its own message heuristic before withAppsHint. +// A throttle whose upstream text resembles the no-container business error must +// still keep its classification and tell the caller to back off, not deploy. +func TestWithObservabilityHint_RateLimitWinsOverContainerMessageMarker(t *testing.T) { + message := "container not exist: request trigger frequency limit" + in := errs.NewAPIError(errs.SubtypeRateLimit, "%s", message). + WithCode(99991400).WithRetryable() + out := withObservabilityHint(in) + p, _ := errs.ProblemOf(out) + if p.Subtype != errs.SubtypeRateLimit || p.Code != 99991400 || !p.Retryable { + t.Fatalf("classification mutated: subtype=%q code=%d retryable=%t", p.Subtype, p.Code, p.Retryable) + } + if p.Message != message { + t.Fatalf("message = %q, want original %q", p.Message, message) + } + if p.Hint != appRateLimitHint { + t.Fatalf("hint = %q, want rate-limit backoff hint %q", p.Hint, appRateLimitHint) + } +} + +func rateLimitStatusName(status int) string { + if status == 200 { + return "HTTP 200 envelope" + } + return "HTTP 429 envelope" +} diff --git a/shortcuts/apps/common.go b/shortcuts/apps/common.go index e411c0cb04..79af768bb4 100644 --- a/shortcuts/apps/common.go +++ b/shortcuts/apps/common.go @@ -125,10 +125,18 @@ func withObservabilityHint(err error) error { if err == nil { return nil } - if p, ok := errs.ProblemOf(err); ok && isAppNoContainerError(p) { - p.Message = appNoContainerMessage - p.Hint = appNoContainerHint - return err + if p, ok := errs.ProblemOf(err); ok { + // A gateway throttle was not evaluated by the observability service. + // Classify it before inspecting message text that can coincidentally + // resemble the service's no-container business error. + if p.Subtype == errs.SubtypeRateLimit { + return withAppsHint(err, appIDListHint) + } + if isAppNoContainerError(p) { + p.Message = appNoContainerMessage + p.Hint = appNoContainerHint + return err + } } return withAppsHint(err, appIDListHint) } @@ -158,6 +166,15 @@ func withAppsHint(err error, hint string) error { } // p points at the embedded Problem, so the mutation is reflected in err. if p, ok := errs.ProblemOf(err); ok { + // Classification beats message heuristics. A gateway throttle was not + // evaluated by the database service, so text that resembles a database + // state marker must not reclassify it or replace backoff guidance. + if p.Subtype == errs.SubtypeRateLimit { + if strings.TrimSpace(p.Hint) == "" { + p.Hint = appRateLimitHint + } + return err + } if isAppNoDatabaseError(p) { p.Message = appNoDatabaseMessage p.Hint = appNoDatabaseHint @@ -176,6 +193,13 @@ func withAppsHint(err error, hint string) error { return err } +// appRateLimitHint answers a throttled apps call (api/rate_limit — the OpenAPI +// gateway's 99991400). The server did not judge the request, so the arguments are +// not what to change; retrying at once only adds to the traffic that tripped the +// limit. The backoff wording matches the stream client's rate-limit hint +// (internal/client), so one condition reads the same on every path. +const appRateLimitHint = "the request was rate limited, not rejected for its arguments, so do not change them; stop immediate retries, then rerun the same command later with exponential backoff and jitter, and send fewer calls in parallel" + // hintExplainsFailure reports whether the caller's hint can plausibly explain this // failure. Every hint passed to withAppsHint describes something to fix about the // REQUEST — "verify --app-id", "verify table/column names with +db-table-get", "if @@ -201,15 +225,32 @@ func withAppsHint(err error, hint string) error { // 400002479 (db-sync task state) is served by withDBSyncHint, which does not // delegate here. // -// Deliberately one subtype. Gating on Category instead (deny authentication / +// SubtypeRateLimit is withheld too: a throttled call was never evaluated, so no +// sentence about the request can explain it. The OpenAPI gateway's 99991400 +// reached every apps command that passes a request hint this way — +db-execute +// was told to check table/column names and switch to --environment dev, +// +trace-get to verify --app-id — and an agent following that sent more calls into +// the same limit. The failing cases that +// proved it are TestAppsDBExecute_RateLimitHintIsBackoffNotRequestFix (through the +// command) and TestAppsHints_RateLimitDoesNotInheritRequestHint (through every +// hint helper). Unlike the precondition class the hint is not left empty: +// withAppsHint fills it with appRateLimitHint, and like every other hint only when +// it is empty, so a rate limit that arrives with its own wording keeps it +// (TestWithAppsHint_RateLimitKeepsUpstreamHint). +// +// Deliberately these two subtypes. Gating on Category instead (deny authentication / // authorization / network / ...) contradicts contracts this package asserts on // purpose: an authentication failure on +role-list (99991663) is expected to keep // the app-access hint, and a 503 on credential issuance the developer-access hint. // Those hints are broad enough to survive a caller-standing failure; only the -// precondition class is guaranteed to be misdescribed. Widen only with a failing -// case proving the hint is wrong for the new class. +// precondition and rate-limit classes are guaranteed to be misdescribed. Widen only +// with a failing case proving the hint is wrong for the new class. func hintExplainsFailure(p *errs.Problem) bool { - return p.Subtype != errs.SubtypeFailedPrecondition + switch p.Subtype { + case errs.SubtypeFailedPrecondition, errs.SubtypeRateLimit: + return false + } + return true } // appNoDatabaseMessageMarkers are lowercase substrings of the raw server message diff --git a/shortcuts/apps/db_common.go b/shortcuts/apps/db_common.go index 271eea5bb7..802ed81e30 100644 --- a/shortcuts/apps/db_common.go +++ b/shortcuts/apps/db_common.go @@ -359,6 +359,12 @@ func withDBSyncHint(err error, fallback string) error { if strings.TrimSpace(p.Hint) != "" { return err } + // A throttled call was never evaluated, so neither the code table nor the + // request-shaped fallback can explain it (see hintExplainsFailure). + if p.Subtype == errs.SubtypeRateLimit { + p.Hint = appRateLimitHint + return err + } // Subcode-specific hint takes precedence over the generic code table: the // online-DDL ban shares code 500002776 with unrelated failures, so match the // stable subcode in the msg before falling back to per-code guidance.