From f7e5d175eb6d9aa22b80aa6c8e32defff021fa6f Mon Sep 17 00:00:00 2001 From: Ryan Albrecht Date: Wed, 12 Aug 2026 15:09:39 -0700 Subject: [PATCH 1/3] fix(scanner): Retry transient LLM inference failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A single OpenRouter request timeout or transient provider error used to fail the whole pattern immediately, which — since scan-and-report treats any failed pattern as fatal — took the entire CI run down with it. analyzeWithClaude now retries the inference call plus response parsing up to 3 times with backoff before giving up, via a small shared withRetry() helper that logs each failed attempt so a recovered blip still leaves a trace. Also fixes a latent bug in the OpenRouter provider's timeout handling: AbortSignal.timeout() aborts with a DOMException, which does not extend Error, so the 'err instanceof Error' check silently skipped the intended wrapped message ('OpenRouter request timed out after Nms') and let the generic 'operation was aborted due to timeout' through instead. Refs https://github.com/getsentry/sentry/actions/runs/31643785552, where no-class-components timed out after exactly 120s with no retry. --- src/inference/openrouter.ts | 7 ++++++- src/scanner/claude.ts | 31 ++++++++++++++++++----------- src/utils/retry.ts | 39 +++++++++++++++++++++++++++++++++++++ 3 files changed, 65 insertions(+), 12 deletions(-) create mode 100644 src/utils/retry.ts diff --git a/src/inference/openrouter.ts b/src/inference/openrouter.ts index 00bfed1..a7e324a 100644 --- a/src/inference/openrouter.ts +++ b/src/inference/openrouter.ts @@ -79,7 +79,12 @@ export const openRouterProvider: InferenceProvider = { signal: AbortSignal.timeout(req.timeoutMs ?? 120_000), }); } catch (err) { - if (err instanceof Error && err.name === "TimeoutError") { + // `AbortSignal.timeout()` aborts with a `DOMException`, which does not + // extend `Error` — checking `err instanceof Error` here would silently + // skip this branch and let the generic "operation was aborted" message + // through instead of the timeout duration. + const name = err && typeof err === "object" ? (err as { name?: unknown }).name : undefined; + if (name === "TimeoutError") { throw new Error(`OpenRouter request timed out after ${req.timeoutMs ?? 120_000}ms`); } throw err; diff --git a/src/scanner/claude.ts b/src/scanner/claude.ts index a1a6ef5..eeae721 100644 --- a/src/scanner/claude.ts +++ b/src/scanner/claude.ts @@ -5,6 +5,7 @@ import { findingsJsonSchema, FindingsResponseSchema } from "../config/schemas.ts import type { FindingsResponse } from "../config/schemas.ts"; import { runInference } from "../inference/index.ts"; import { verbose } from "../utils/logger.ts"; +import { withRetry } from "../utils/retry.ts"; export interface FileContent { absolutePath: string; @@ -62,16 +63,24 @@ export async function analyzeWithClaude( verbose(`Analyzing ${files.length} files for pattern "${pattern.name}" with model "${model}"`); - const output = await runInference({ - prompt, - model, - system: systemPrompt, - jsonSchema: { - name: "findings", - schema: findingsJsonSchema as Record, - }, - timeoutMs: 120_000, - }); + // A single request timeout or a transient provider hiccup shouldn't cost the + // whole pattern (and, via scanRepo's fail-loud policy, the whole CI run) — retry + // a bounded number of times before letting the failure propagate. + return withRetry( + async () => { + const output = await runInference({ + prompt, + model, + system: systemPrompt, + jsonSchema: { + name: "findings", + schema: findingsJsonSchema as Record, + }, + timeoutMs: 120_000, + }); - return FindingsResponseSchema.parse(JSON.parse(output)); + return FindingsResponseSchema.parse(JSON.parse(output)); + }, + { label: `Pattern "${pattern.name}" inference batch`, attempts: 3, delayMs: 3000 }, + ); } diff --git a/src/utils/retry.ts b/src/utils/retry.ts new file mode 100644 index 0000000..d113d0f --- /dev/null +++ b/src/utils/retry.ts @@ -0,0 +1,39 @@ +import { setTimeout as sleep } from "node:timers/promises"; +import { log } from "./logger.ts"; + +export interface RetryOptions { + /** Total attempts, including the first. */ + attempts?: number; + /** Base delay before a retry; attempt N waits `delayMs * N`. */ + delayMs?: number; + /** Named in the retry log line so a transient blip is traceable to its call site. */ + label: string; +} + +/** + * Retry a flaky async operation (network timeouts, transient API errors) a + * bounded number of times before giving up. A retry that recovers still logs + * the attempt that failed, so a transient blip leaves a trace in the CI log + * instead of looking like it never happened. The final failure is rethrown + * as-is once attempts are exhausted, so a genuine, persistent problem still + * fails loud. + */ +export async function withRetry(fn: () => Promise, options: RetryOptions): Promise { + const attempts = options.attempts ?? 3; + const delayMs = options.delayMs ?? 2000; + let lastErr: unknown; + + for (let attempt = 1; attempt <= attempts; attempt++) { + try { + return await fn(); + } catch (err) { + lastErr = err; + if (attempt === attempts) break; + const message = err instanceof Error ? err.message : String(err); + log(` ${options.label} failed (attempt ${attempt}/${attempts}): ${message}; retrying...`); + await sleep(delayMs * attempt); + } + } + + throw lastErr; +} From f58db11da2cafa6f57e4647f85d06f84c3190286 Mon Sep 17 00:00:00 2001 From: Ryan Albrecht Date: Wed, 12 Aug 2026 15:09:48 -0700 Subject: [PATCH 2/3] fix(reporter): Retry a batch before failing on dropped Sentry events MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit assertNoDrops() failed the whole run on any transport-recorded drop, including a single one-off network_error out of thousands of events — e.g. a run reporting 2887 findings failed because 1 send hit a transient connection error, even though every other event was delivered. sendItems() now retries a chunk (or single-batch window) up to REFACTOR_TASKS_SENTRY_SEND_ATTEMPTS times (default 3) when the transport records new drops since the attempt started, logging each retry. Resending re-reports every finding in the batch; duplicates land as repeat occurrences of the same fingerprinted Sentry issue, not separate issues. Only drops that survive every attempt (unrecoveredDrops) fail the run — the transport's raw cumulative tally can't tell a recovered drop from a real one, so assertNoDrops() no longer reads it directly. Verified against a real ECONNREFUSED endpoint (retries exhaust, run fails loud with the attempt count in the message) and a flaky local server that fails once then succeeds (recovers on retry, run succeeds) — both reproduce the getsentry/sentry run's 'Sentry dropped 1 of 2887 events (network_error=1)' failure and its fix. Refs https://github.com/getsentry/sentry/actions/runs/31642621319 --- src/reporter/sentry.ts | 111 +++++++++++++++++++++++++++++++---------- 1 file changed, 86 insertions(+), 25 deletions(-) diff --git a/src/reporter/sentry.ts b/src/reporter/sentry.ts index 2041075..426ed3c 100644 --- a/src/reporter/sentry.ts +++ b/src/reporter/sentry.ts @@ -14,8 +14,10 @@ import { log, verbose } from "../utils/logger.ts"; * The `chunkSize` control tunes this: `0` (the default) sends everything in a * single batch (fast, but only viable when the volume fits under the rate * limit), while a positive value sends paced chunks of that size, flushing after - * each, to stay under it. Either way, dropped events are detected and the run - * fails loud rather than reporting success. The pacing/flush knobs below are + * each, to stay under it. Either way, dropped events are detected; a transient + * drop (e.g. a one-off `network_error`) is retried a bounded number of times + * before the run fails loud, so a single blip out of thousands of events + * doesn't turn a healthy scan red. The pacing/flush/retry knobs below are * tunable via env vars for projects with different limits. */ function envInt(name: string, fallback: number): number { @@ -35,6 +37,17 @@ function envIntOptional(name: string): number | undefined { const CHUNK_DELAY_MS = envInt("REFACTOR_TASKS_SENTRY_CHUNK_DELAY_MS", 1000); const FLUSH_TIMEOUT_MS = envInt("REFACTOR_TASKS_SENTRY_FLUSH_TIMEOUT_MS", 30_000); +/** + * Attempts per batch (chunk, or single-batch window) before a drop is treated + * as unrecoverable. A `network_error` drop is often a one-off blip — a DNS + * hiccup, a reset connection — rather than a symptom of being rate-limited, so + * it is worth resending before giving up. Resending re-captures every finding + * in the batch, including ones already delivered; duplicates land as repeat + * occurrences of the same Sentry issue (findings are fingerprinted by pattern, + * file, and line), not separate issues. + */ +const CHUNK_SEND_ATTEMPTS = envInt("REFACTOR_TASKS_SENTRY_SEND_ATTEMPTS", 3); + /** * The Sentry transport holds in-flight events in a promise buffer that defaults * to 64 slots. `captureMessage` is fire-and-forget, so once the buffer fills the @@ -58,14 +71,26 @@ function recordDrop(reason: string, count: number): void { droppedEvents.set(reason, (droppedEvents.get(reason) ?? 0) + count); } -function totalDropped(): number { - let total = 0; - for (const count of droppedEvents.values()) total += count; - return total; +/** A point-in-time copy of {@link droppedEvents}, to diff against after a send attempt. */ +function snapshotDropped(): Map { + return new Map(droppedEvents); } -function droppedSummary(): string { - return [...droppedEvents.entries()].map(([reason, count]) => `${reason}=${count}`).join(", "); +/** + * The drops recorded since `before` was snapshotted, as a total and a + * `reason=count` summary — i.e. what this specific send attempt caused, + * not the run's cumulative total. + */ +function droppedSince(before: Map): { total: number; summary: string } { + const delta = new Map(); + for (const [reason, count] of droppedEvents) { + const prior = before.get(reason) ?? 0; + if (count > prior) delta.set(reason, count - prior); + } + let total = 0; + for (const count of delta.values()) total += count; + const summary = [...delta.entries()].map(([reason, count]) => `${reason}=${count}`).join(", "); + return { total, summary }; } function initSentry(dsn: string, bufferSize: number): void { @@ -193,6 +218,11 @@ export class FindingReporter { private sent = 0; private flushTimeouts = 0; private sentAnyChunk = false; + // Drops that survived every retry attempt for their batch — as opposed to + // droppedEvents, which is the transport's raw cumulative tally and doesn't + // distinguish "dropped, then successfully resent" from "dropped for good". + private unrecoveredDrops = 0; + private unrecoveredSummaries: string[] = []; constructor(dsn: string, options: ReportOptions = {}) { this.chunkSize = resolveChunkSize(options.chunkSize); @@ -222,11 +252,8 @@ export class FindingReporter { const remainder = this.buffer.splice(0); for (let i = 0; i < remainder.length; i += this.bufferSize) { const window = remainder.slice(i, i + this.bufferSize); - for (const finding of window) { - reportFinding(finding); - } + const drained = await this.sendItems(window); this.sent += window.length; - const drained = await Sentry.flush(FLUSH_TIMEOUT_MS); if (!drained) { throw new Error( `Sentry flush timed out after ${FLUSH_TIMEOUT_MS}ms; ${this.sent} findings were enqueued but not confirmed delivered. ` + @@ -254,23 +281,61 @@ export class FindingReporter { } /** - * Fail loud if the transport rejected any events. A flush can succeed while - * events were still dropped — a rate-limit 429 is a completed request, so the - * promise resolves and the drop is only visible via the transport outcomes we - * teed into {@link droppedEvents}. Without this, a throttled run reports - * "success" while data never reached Sentry. + * Fail loud if any batch still had dropped events after exhausting its + * retries. Note this checks {@link unrecoveredDrops}, not the transport's raw + * cumulative tally — a drop that a retry successfully resent is not a + * failure, so it must not fail the run just because the transport still + * remembers the earlier, since-recovered attempt. */ private assertNoDrops(): void { - const dropped = totalDropped(); - if (dropped > 0) { + if (this.unrecoveredDrops > 0) { throw new Error( - `Sentry dropped ${dropped} of ${this.sent} events (${droppedSummary()}). ` + + `Sentry dropped ${this.unrecoveredDrops} of ${this.sent} events after ${CHUNK_SEND_ATTEMPTS} attempt(s) each (${this.unrecoveredSummaries.join("; ")}). ` + `Set REFACTOR_TASKS_SENTRY_CHUNK_SIZE to a positive value to pace sends under the rate limit, ` + `and/or raise REFACTOR_TASKS_SENTRY_CHUNK_DELAY_MS, then re-run.`, ); } } + /** + * Send `items` to Sentry, retrying the whole batch up to + * {@link CHUNK_SEND_ATTEMPTS} times if the transport records new drops (e.g. a + * transient `network_error`) since the attempt started. Resending re-reports + * every finding in the batch, including any already delivered — see + * {@link CHUNK_SEND_ATTEMPTS}'s doc comment for why that's fine. Returns + * whether the final attempt's flush drained; callers decide how to treat an + * undrained flush. Drops that survive every attempt are folded into + * {@link unrecoveredDrops}/{@link unrecoveredSummaries} for {@link assertNoDrops}. + */ + private async sendItems(items: ScanFinding[]): Promise { + let drained = true; + + for (let attempt = 1; attempt <= CHUNK_SEND_ATTEMPTS; attempt++) { + const before = snapshotDropped(); + for (const finding of items) { + reportFinding(finding); + } + drained = await Sentry.flush(FLUSH_TIMEOUT_MS); + if (!drained) return false; + + const { total, summary } = droppedSince(before); + if (total === 0) return true; + + if (attempt < CHUNK_SEND_ATTEMPTS) { + log( + ` Sentry dropped ${total} event(s) (${summary}) reporting this batch; retrying (attempt ${attempt + 1}/${CHUNK_SEND_ATTEMPTS})`, + ); + await sleep(CHUNK_DELAY_MS * attempt); + continue; + } + + this.unrecoveredDrops += total; + this.unrecoveredSummaries.push(summary); + } + + return drained; + } + private async sendChunk(chunk: ScanFinding[]): Promise { // Pace between chunks (but not before the first) so the transport can apply // 429 backoff between bursts instead of dropping a firehose of events. @@ -279,12 +344,8 @@ export class FindingReporter { } this.sentAnyChunk = true; - for (const finding of chunk) { - reportFinding(finding); - } + const drained = await this.sendItems(chunk); this.sent += chunk.length; - - const drained = await Sentry.flush(FLUSH_TIMEOUT_MS); if (!drained) { this.flushTimeouts++; verbose(`Flush timed out after ${this.sent} findings`); From 5aa9b7da7b018be2000468a7f06427e9dafbe98e Mon Sep 17 00:00:00 2001 From: Ryan Albrecht Date: Wed, 12 Aug 2026 15:23:13 -0700 Subject: [PATCH 3/3] ref(scanner): Simplify inference timeout fix to a plain duration bump MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces the withRetry() wrapper (and its now-unused utils/retry.ts) around analyzeWithClaude with a simpler fix: raise timeoutMs from 120s to 240s. The one failure we've observed (no-class-components in getsentry/sentry, run 31643785552) aborted at exactly the old 120s ceiling, which looks like a slow request under load rather than a hung one — headroom is the narrower fix for that evidence, and the retry wrapper was more machinery than the observed failure mode justified. The OpenRouter timeout-message classification fix (DOMException doesn't extend Error) stays: it's independently useful for whatever timeout duration is configured. --- src/scanner/claude.ts | 34 ++++++++++++++-------------------- src/utils/retry.ts | 39 --------------------------------------- 2 files changed, 14 insertions(+), 59 deletions(-) delete mode 100644 src/utils/retry.ts diff --git a/src/scanner/claude.ts b/src/scanner/claude.ts index eeae721..51d2b43 100644 --- a/src/scanner/claude.ts +++ b/src/scanner/claude.ts @@ -5,7 +5,6 @@ import { findingsJsonSchema, FindingsResponseSchema } from "../config/schemas.ts import type { FindingsResponse } from "../config/schemas.ts"; import { runInference } from "../inference/index.ts"; import { verbose } from "../utils/logger.ts"; -import { withRetry } from "../utils/retry.ts"; export interface FileContent { absolutePath: string; @@ -63,24 +62,19 @@ export async function analyzeWithClaude( verbose(`Analyzing ${files.length} files for pattern "${pattern.name}" with model "${model}"`); - // A single request timeout or a transient provider hiccup shouldn't cost the - // whole pattern (and, via scanRepo's fail-loud policy, the whole CI run) — retry - // a bounded number of times before letting the failure propagate. - return withRetry( - async () => { - const output = await runInference({ - prompt, - model, - system: systemPrompt, - jsonSchema: { - name: "findings", - schema: findingsJsonSchema as Record, - }, - timeoutMs: 120_000, - }); - - return FindingsResponseSchema.parse(JSON.parse(output)); + // 240s (up from the original 120s): a batch that was seen aborting at exactly + // the old ceiling in CI was likely just slow under load, not stuck — this + // gives it headroom without adding retry logic. + const output = await runInference({ + prompt, + model, + system: systemPrompt, + jsonSchema: { + name: "findings", + schema: findingsJsonSchema as Record, }, - { label: `Pattern "${pattern.name}" inference batch`, attempts: 3, delayMs: 3000 }, - ); + timeoutMs: 240_000, + }); + + return FindingsResponseSchema.parse(JSON.parse(output)); } diff --git a/src/utils/retry.ts b/src/utils/retry.ts deleted file mode 100644 index d113d0f..0000000 --- a/src/utils/retry.ts +++ /dev/null @@ -1,39 +0,0 @@ -import { setTimeout as sleep } from "node:timers/promises"; -import { log } from "./logger.ts"; - -export interface RetryOptions { - /** Total attempts, including the first. */ - attempts?: number; - /** Base delay before a retry; attempt N waits `delayMs * N`. */ - delayMs?: number; - /** Named in the retry log line so a transient blip is traceable to its call site. */ - label: string; -} - -/** - * Retry a flaky async operation (network timeouts, transient API errors) a - * bounded number of times before giving up. A retry that recovers still logs - * the attempt that failed, so a transient blip leaves a trace in the CI log - * instead of looking like it never happened. The final failure is rethrown - * as-is once attempts are exhausted, so a genuine, persistent problem still - * fails loud. - */ -export async function withRetry(fn: () => Promise, options: RetryOptions): Promise { - const attempts = options.attempts ?? 3; - const delayMs = options.delayMs ?? 2000; - let lastErr: unknown; - - for (let attempt = 1; attempt <= attempts; attempt++) { - try { - return await fn(); - } catch (err) { - lastErr = err; - if (attempt === attempts) break; - const message = err instanceof Error ? err.message : String(err); - log(` ${options.label} failed (attempt ${attempt}/${attempts}): ${message}; retrying...`); - await sleep(delayMs * attempt); - } - } - - throw lastErr; -}