Skip to content

Commit 5299f29

Browse files
anandgupta42claude
andcommitted
fix(telemetry): classify 5xx + rate-limit errors; harden DoS test thresholds
Two follow-ups from the v1.4.0 pre-merge ritual, both surfaced as [KNOWN GAP] tests in earlier rounds. Neither blocks the merge; both strengthen the test suite and improve telemetry diagnostics. ## (1) classifyError ERROR_PATTERNS coverage Round 2 fuzz testing pinned that real provider errors like "APIError: Service unavailable (503)" and "Rate limit exceeded. Retry after 60s" classified as `"unknown"` because ERROR_PATTERNS only matched the prefixed forms ("status code: 5", "http 429"). That defeated the diagnostic value of `agent_outcome.error_class` for two of the most common provider failure modes. Adds these phrases to the existing `http_error` class so they classify into the right bucket: "service unavailable", "rate limit", "rate_limit", "retry after", "too many requests", "503", "502", "504" Why `http_error` and not a new `rate_limit` class: the upstream event schema uses `http_error` for any non-2xx HTTP failure; rate-limit responses ARE HTTP 429s, just stated in different prose. Keeping the same bucket avoids fragmenting telemetry dashboards. Tests: - v140-merge-fuzz.test.ts: replace the 2 `[KNOWN GAP]` tests with positive assertions: - "APIError 503 classifies as 'http_error' (Service unavailable)" - "rate-limit error classifies as 'http_error' (Retry after)" ## (2) DoS test threshold hardening Round 3 chaos tests asserted regex DoS resistance with fixed `<100ms` wall-clock thresholds. CI runners under load can flake on absolute thresholds (slow scheduling, contended I/O, etc.), even though the actual runtime is fine relative to a baseline. Replaces fixed thresholds with a relative-budget approach: - Take the min of 5 baseline runs on a trivial input (filters scheduler jitter, 1ms floor). - Run a single warmup invocation before the timed adversarial call (so JIT-cold doesn't penalize first-run). - Adversarial input must complete within `50 × baseline`. Why 50× and not the original implicit 100ms-on-a-fast-runner (maybe ~200×): catastrophic backtracking on these regexes would be 1000×+ baseline, so 50× is a generous yet still-strict budget. Real measurements stayed within ~3× baseline locally. Tests still pass 5/5 runs locally. The new code prints a descriptive error message ("X took Yms (baseline Zms × 50 budget = Wms)") so a future regression has a clear diagnostic. Combined verification: - All 4 telemetry/test/upstream suites pass: 87 tests, 3593 assertions, 0 fail. - Chaos suite alone: 24/24 pass × 5 consecutive runs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 0a9a67d commit 5299f29

3 files changed

Lines changed: 68 additions & 24 deletions

File tree

packages/opencode/src/altimate/telemetry/index.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -925,6 +925,17 @@ export namespace Telemetry {
925925
"http 429",
926926
"http 451",
927927
"http 403",
928+
// R3 audit: real provider 5xx + rate-limit messages don't carry "status code:" prefix.
929+
// Add bare phrases so 503 / 502 / 504 + Retry-After / "rate limit exceeded" classify
930+
// out of "unknown" into http_error (preserving diagnostic specificity in agent_outcome.error_class).
931+
"service unavailable",
932+
"rate limit",
933+
"rate_limit",
934+
"retry after",
935+
"too many requests",
936+
"503",
937+
"502",
938+
"504",
928939
],
929940
},
930941
// altimate_change end

packages/opencode/test/upstream/v140-merge-chaos.test.ts

Lines changed: 47 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -26,33 +26,65 @@ import path from "path"
2626
const repoRoot = path.resolve(import.meta.dir, "..", "..", "..", "..")
2727

2828
// ---------- Regex DoS resistance ----------
29+
//
30+
// We measure adversarial-input runtime relative to a baseline of a benign
31+
// short input. This is more robust against CI runner variability than a
32+
// fixed wall-clock threshold: a slow runner that takes 50ms on the baseline
33+
// gets a 50× budget (2500ms) for adversarial input; a fast runner that takes
34+
// 0.5ms gets a 25ms budget. Either way, catastrophic-backtracking regressions
35+
// (which would be 100×–1000× slower than baseline) get caught.
36+
//
37+
// We also include a single warmup invocation before each timed run so JIT
38+
// optimization doesn't artificially penalize the first call.
2939
describe("v1.4.0 chaos — maskString regex DoS resistance", () => {
30-
test("10000 backslashes processed in <100ms", () => {
31-
const input = '"' + "\\\\".repeat(10000) + '"'
40+
// Baseline: trivial input. Computed once per describe block.
41+
const BASELINE_INPUT = "no secrets here, just plain text"
42+
const BUDGET_MULTIPLIER = 50 // adversarial input must be at most 50× baseline
43+
44+
function baselineMs(): number {
45+
// Take min of 5 runs to filter out scheduler jitter
46+
const samples: number[] = []
47+
for (let i = 0; i < 5; i++) {
48+
const t0 = performance.now()
49+
Telemetry.maskString(BASELINE_INPUT)
50+
samples.push(performance.now() - t0)
51+
}
52+
// 1ms floor so we don't divide-by-zero on very-fast runners
53+
return Math.max(1, Math.min(...samples))
54+
}
55+
56+
function probe(input: string, label: string): void {
57+
Telemetry.maskString(input) // warmup
3258
const t0 = performance.now()
3359
Telemetry.maskString(input)
34-
expect(performance.now() - t0).toBeLessThan(100)
60+
const dt = performance.now() - t0
61+
const baseline = baselineMs()
62+
const budget = baseline * BUDGET_MULTIPLIER
63+
if (dt > budget) {
64+
throw new Error(
65+
`DoS regression: "${label}" took ${dt.toFixed(2)}ms (baseline ${baseline.toFixed(2)}ms × ${BUDGET_MULTIPLIER} budget = ${budget.toFixed(2)}ms)`,
66+
)
67+
}
68+
}
69+
70+
test("10000 backslashes — runtime stays within 50× baseline", () => {
71+
const input = '"' + "\\\\".repeat(10000) + '"'
72+
expect(() => probe(input, "10000 backslashes")).not.toThrow()
3573
})
3674

37-
test("alternating quotes (10k) processed in <100ms", () => {
75+
test("alternating quotes (10k) — runtime stays within 50× baseline", () => {
3876
const input = '""""'.repeat(2500)
39-
const t0 = performance.now()
40-
Telemetry.maskString(input)
41-
expect(performance.now() - t0).toBeLessThan(100)
77+
expect(() => probe(input, "alternating quotes")).not.toThrow()
4278
})
4379

44-
test("evil escape pattern (1k×3) processed in <100ms", () => {
80+
test("evil escape pattern (1k×3) — runtime stays within 50× baseline", () => {
4581
const input = '"' + "\\\\.".repeat(1000) + '"'
46-
const t0 = performance.now()
47-
Telemetry.maskString(input)
48-
expect(performance.now() - t0).toBeLessThan(100)
82+
expect(() => probe(input, "evil escape pattern")).not.toThrow()
4983
})
5084

51-
test("1000 concurrent sk-ant prefixes processed in <100ms", () => {
85+
test("1000 concurrent sk-ant prefixes — runtime stays within 50× baseline", () => {
5286
const input = ("sk-ant-" + "a".repeat(20) + " ").repeat(1000)
53-
const t0 = performance.now()
54-
Telemetry.maskString(input)
55-
expect(performance.now() - t0).toBeLessThan(100)
87+
expect(() => probe(input, "sk-ant prefix flood")).not.toThrow()
5688
})
5789
})
5890

packages/opencode/test/upstream/v140-merge-fuzz.test.ts

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -232,31 +232,32 @@ describe("v1.4.0 failure injection — synthetic provider errors flow through di
232232
expect(out.error_class).not.toBe("unknown")
233233
})
234234

235-
// KNOWN GAP: classifyError patterns lack "503" / "Service unavailable" /
236-
// "Rate limit" / "Retry after" keywords. Real provider errors get
237-
// classified as "unknown" — diagnostic value lost. Fix: extend
238-
// ERROR_PATTERNS in altimate/telemetry/index.ts. Tests below pin the
239-
// current behavior so a future fix flips them automatically.
240-
test("[KNOWN GAP] APIError 503 classifies as 'unknown' (pattern coverage hole)", () => {
235+
// Once gapped: classifyError patterns lacked "503" / "Service unavailable" /
236+
// "Rate limit" / "Retry after" / "Too many requests" keywords, so real
237+
// provider 5xx and rate-limit errors classified as "unknown" — defeating
238+
// the diagnostic value of agent_outcome.error_class. ERROR_PATTERNS now
239+
// includes those phrases under the http_error class. Tests below pin
240+
// the new classifications.
241+
test("APIError 503 classifies as 'http_error' (Service unavailable)", () => {
241242
const out = Telemetry.deriveAgentOutcomeReason({
242243
outcome: "error",
243244
lastToolName: "edit",
244245
lastMessageError: "APIError: Service unavailable (503)",
245246
abortReason: null,
246247
lastErrorClass: "",
247248
})
248-
expect(out.error_class).toBe("unknown") // flip when pattern added
249+
expect(out.error_class).toBe("http_error")
249250
expect(out.reason).toContain("Service unavailable")
250251
})
251252

252-
test("[KNOWN GAP] rate-limit error classifies as 'unknown' (pattern coverage hole)", () => {
253+
test("rate-limit error classifies as 'http_error' (Retry after)", () => {
253254
const out = Telemetry.deriveAgentOutcomeReason({
254255
outcome: "error",
255256
lastToolName: null,
256257
lastMessageError: "Rate limit exceeded. Retry after 60s",
257258
abortReason: null,
258259
lastErrorClass: "",
259260
})
260-
expect(out.error_class).toBe("unknown") // flip when pattern added
261+
expect(out.error_class).toBe("http_error")
261262
})
262263
})

0 commit comments

Comments
 (0)