fix(llm): retry transient provider failures

[rng1995]

Summary

NVCARPS-125 exposed a retry-classification gap in LLM-backed scans. Transient provider failures such as HTTP 5xx responses, timeouts, throttling, and Bedrock service errors were not consistently recognized unless their exception class was exactly APIConnectionError. A recoverable provider problem could therefore end a semantic batch immediately and make an otherwise useful scan incomplete.

This change adds one bounded, provider-neutral retry policy while preserving deterministic request failures as non-retryable.

Root cause

The coordinator matched a single exception class name. Provider SDKs expose the same transient conditions through several exception shapes, status codes, nested causes, and Bedrock error codes. Native SDK retry behavior also differed by adapter, which left gaps for dynamic deadlines and could create overlapping retry layers.

Solution

• Recognize common transient OpenAI, Anthropic, HTTP, and Bedrock error shapes, including wrapped causes.
• Retry transport/service failures with bounded 0.5s, 1s, and 2s backoff.
• Retry rate limits with bounded 5s, 15s, and 30s backoff and honor Retry-After up to 60 seconds.
• Respect an explicit provider x-should-retry decision.
• Keep deterministic client errors, including context-window overflow, non-retryable.
• Use a single retry owner: configurable OpenAI/Anthropic native retries remain available for static timeouts, while the coordinator owns dynamic-deadline and Bedrock retries.
• Preserve the initiating provider error class and classify exhausted transient failures consistently in the inspection ledger.

Key files

• src/skillspector/llm_analyzer_base.py
• src/skillspector/inspection_ledger.py
• src/skillspector/providers/bedrock/provider.py
• Provider-neutral sync, async, deadline, native-retry, and Bedrock regression tests

Validation

• make lint format-check test-ci
  • Ruff lint and format checks passed.
  • 4,867 tests passed, 14 skipped, 4 expected failures, 38 deselected.
• pytest -m integration tests/
  • 28 passed; 7 live agent CLI tests skipped because external services were not configured.
• Package build completed for wheel and source distribution.
• twine check passed for both artifacts.

Compatibility and risk

The change is internal and does not alter the public API or report schema. The main behavior change is bounded extra latency during a transient provider failure. Retry counts and delay caps prevent unbounded work, and deadline-aware calls stop before a retry that cannot fit. Authentication, validation, malformed-request, and context-overflow errors are not retried.

Rollback is a straight revert of this commit. CI-side failure taxonomy and alerting are separate follow-up work and are not changed here.

Tracking: NVCARPS-125

[rng1995]

[SkillSpector Review]

Manual Review Needed — this PR is authored by the authenticated reviewer, so an independent reviewer must supply the approval.

Reviewed head: 1c91cd339c17b9f355f24bf2afabedb4bf3640d6

No required code changes were identified in this head. I reviewed the complete change and surrounding retry architecture, including provider-neutral exception/status classification, wrapped causes and explicit retry overrides, bounded Retry-After handling, dynamic-deadline behavior, native-versus-coordinator retry ownership, Bedrock SDK retry configuration, ledger/error sanitization, synchronous and asynchronous paths, and the added boundary tests.

Merge gates still open:

• Independent approval is required because the author and authenticated reviewer are both rng1995.
• GitHub reports no CI/check results for the current head, so validation cannot be independently confirmed.
• GitHub reports the PR as mergeable but BLOCKED; do not merge until branch-protection requirements are satisfied.

Priority: P0 — fixes customer-reported semantic-scan failures caused by transient provider errors.

[riygarg17]

[P1] Preserve retries for all Bedrock transport failures

Setting total_max_attempts to 1 disables botocore’s native retry handling and makes the coordinator the sole retry owner. However, the coordinator does not currently recognize botocore ProxyConnectionError, SSLError, or its base ConnectionError as transient. A temporary proxy or TLS failure that botocore previously retried will therefore terminate the semantic batch immediately.

[riygarg17]

[P2] Do not delegate HTTP 425 to native retries
The coordinator classifies HTTP 425 as retryable, but it skips coordinator retries whenever OpenAI or Anthropic native retries are enabled. The locked OpenAI and Anthropic SDK versions retry 408, 409, 429, and 5xx, but not 425. Consequently, a static-timeout OpenAI/Anthropic request receiving 425 is recorded as exhausted after only one outer attempt.

[rng1995]

Addressed both review findings in commit 34cba965e1c2f855436daf70b0fb3cb3ec176099:

• P1: botocore ConnectionError, ProxyConnectionError, and SSLError are now classified as transient so the coordinator preserves Bedrock transport retries.
• P2: native retry ownership now matches the locked OpenAI/Anthropic SDK policies; HTTP 425 remains coordinator-owned and receives bounded retries in both sync and async paths.
• Merged current main (92e8e650) and resolved the test import conflict while retaining the deadline-client reuse coverage.

Validation: 263 focused tests passed; full suite 5,459 passed, 14 skipped, 39 deselected, 4 expected xfailed; Ruff lint and format checks passed. No merge or approval performed.

[chrisknvidia]

Validated inline review findings.

[chrisknvidia]

Re-reviewed exact head fc783f3148f8e3fa53e94375aa6277517a0afab2. The prior retry findings are addressed, focused and full local suites pass, and the combined #555/#556 focused integration is clean. Approved for code review; exact-head CI is still action_required with zero jobs and must be authorized and pass before merge.

[rng1995]

[SkillSpector Review]

Manual Review Needed — a retry-outcome correction is required

Re-reviewed current head fc783f3148f8e3fa53e94375aa6277517a0afab2 against all prior threads, the complete provider-neutral retry implementation, Bedrock retry ownership, synchronous/asynchronous paths, surrounding ledger reporting, and focused regressions.

The three blocking review findings are substantially addressed: Bedrock's remaining standard transient shapes are covered, retry-after-ms takes precedence with fallback, and an explicit provider retry veto now records a generic batch failure. One path still violates the same public-ledger contract. In llm_analyzer_base.py, _invoke_batch_with_retries() can re-raise a retryable provider error before any retry when the remaining workflow time cannot fund the first delay (and can reach the final combined attempt after only structured-response retries). run_batches_detailed() then classifies the error solely by its retryable type as LLM_CONNECTION_RETRIES_EXHAUSTED, falsely stating that transient retries were exhausted. Track the actual provider retry count/outcome and report a deadline/generic failure when zero provider retries ran; cover sync and async deadline-exhaustion and mixed-attempt cases.

The authenticated reviewer rng1995 is also the PR author, so this COMMENT cannot approve the change. Five exact-head checks pass and test-unit is still running; GitHub reports BLOCKED. The existing jitter suggestion remains non-blocking.

Priority: P0 — customer-facing retry reliability and failure telemetry affect semantic-scan trustworthiness.

[rng1995]

[SkillSpector Review]

Manual Review Needed — the retry-outcome correction remains required

Re-reviewed current head c100cbd2f9a6a9eafc5e37f098da7a2b0fb69a20 after the final synchronization with settled main d162d9b343e559be13df8ebba093df3bc9d58c90. I inspected the complete authored delta since the prior review, current retry loops and ledger projection, prior threads, and the merged-main interaction.

The new bounded full-jitter implementation correctly treats Retry-After as a minimum, caps total delay, checks the sampled delay against the workflow deadline, and adds synchronous/asynchronous coverage. It resolves the prior non-blocking jitter suggestion. It does not resolve the blocking ledger defect: when the deadline cannot fund the first sampled delay—or the final combined attempt follows only structured-response retries—the loop re-raises before any provider retry, but run_batches_detailed() still classifies solely by retryable exception type as LLM_CONNECTION_RETRIES_EXHAUSTED. Track the actual provider retry count/outcome and emit deadline/generic failure when zero provider retries ran, with sync/async outcome-level regressions.

The authenticated reviewer rng1995 is also the PR author, so this COMMENT cannot approve it. GitHub currently reports no checks and mergeStateStatus=UNSTABLE; the required telemetry fix, exact-head CI, and independent approval remain open.

Priority: P0 — retry behavior and public failure telemetry directly affect semantic-scan trustworthiness.