Skip to content

fix(nim): retry transient HTTP errors and report actual status code - #2019

Open
Aftabbs wants to merge 2 commits into
NVIDIA:mainfrom
Aftabbs:fix/nim-transient-http-error-handling
Open

fix(nim): retry transient HTTP errors and report actual status code#2019
Aftabbs wants to merge 2 commits into
NVIDIA:mainfrom
Aftabbs:fix/nim-transient-http-error-handling

Conversation

@Aftabbs

@Aftabbs Aftabbs commented Aug 5, 2026

Copy link
Copy Markdown

Summary

Fixes #1967 — NIM generator aborts the entire probe on HTTP 408 (request timeout) with a misleading error message instead of retrying.

Two root causes, both fixed here:

1. openai.APIStatusError was not caught by the backoff decorator

OpenAICompatible._call_model uses @backoff.on_exception to retry on transient network failures, but the decorator only listed RateLimitError, InternalServerError, APITimeoutError, and APIConnectionError. NIM raises a bare openai.APIStatusError for HTTP 408, which fell through the decorator and bubbled up unretried.

Fix: Add openai.APIStatusError to the exception tuple, with a giveup=_is_terminal_api_error predicate that retries only transient codes (408, 502, 503, 504) and immediately surfaces terminal errors (400, 401, 403, 404, 409, 422).

2. The catch-all swallowed the HTTP status code

NIM._call_model's bare except Exception block replaced the actual error with "NIM generation failed. Is the model name spelled correctly?" — losing the HTTP status code and URL that would identify the real problem.

Fix: Split the handler: openai.APIStatusError now logs and re-raises with the actual status code and request URL; the fallback branch includes the exception type name.

Changes

File Change
garak/generators/openai.py Add _TRANSIENT_HTTP_CODES frozenset and _is_terminal_api_error() giveup predicate; extend backoff decorator to catch APIStatusError
garak/generators/nim.py Split catch-all into openai.APIStatusError (reports HTTP status) + Exception (reports type name)
tests/generators/test_nim.py 15 new unit tests for retry logic and error message content

Testing

pytest tests/generators/test_nim.py -v -k "not nim_instantiate and not nim_generate and not nim_parallel and not nim_hf and not nim_conservative"
# 16 passed, 5 deselected

The 5 deselected tests require a live NIM_API_KEY and are unchanged.

@jmartin-tech

Copy link
Copy Markdown
Collaborator

DCO signoff is missing from the commit message and the approach here has already been noted an incorrect in a previous PR.

Please see #1973, this PR duplicates the original offering in that PR and needs to account for the same feedback.

When a NIM endpoint returns HTTP 408 (timeout), 502, 503, or 504, garak
currently aborts the entire probe with a misleading generic error message
("Is the model name spelled correctly?"). Two independent problems:

1. The backoff decorator on OpenAICompatible._call_model only caught
   RateLimitError, InternalServerError, APITimeoutError, and
   APIConnectionError — leaving bare openai.APIStatusError (which NIM
   raises for 408) to fall through without any retry.

2. The catch-all except Exception block in NIM._call_model swallowed the
   original HTTP status code, making diagnostics difficult.

Fix:
- Add _TRANSIENT_HTTP_CODES frozenset {408, 502, 503, 504} and a module-
  level _is_terminal_api_error() giveup predicate in generators/openai.py.
- Extend the backoff decorator to also catch openai.APIStatusError, using
  the giveup predicate to retry only transient codes and immediately
  surface terminal errors (400, 401, 403, 404, 409, 422).
- Split the bare except Exception in NIM._call_model into an
  openai.APIStatusError branch that logs and re-raises with the actual
  HTTP status code and URL, plus a fallback branch that includes the
  exception type name.
- Add 15 unit tests covering the giveup logic and error message content.

Closes NVIDIA#1967

Signed-off-by: Aftabbs <aftabbs.wwe@gmail.com>
@Aftabbs
Aftabbs force-pushed the fix/nim-transient-http-error-handling branch from d87a596 to bd09a60 Compare August 6, 2026 06:59
@manunicholasjacob

Copy link
Copy Markdown
Contributor

Nice catch on 408 arriving as APIStatusError rather than InternalServerError.

One thing worth checking: giveup= applies to every exception in the on_exception tuple, not only the newly added APIStatusError. Since RateLimitError and InternalServerError both subclass APIStatusError, _is_terminal_api_error returns True for them whenever their status is not in _TRANSIENT_HTTP_CODES:

RateLimitError       (429) -> giveup=True    # currently retried
InternalServerError  (500) -> giveup=True    # currently retried
InternalServerError  (503) -> giveup=False
APIStatusError       (408) -> giveup=False   # the intended fix

So 429 and 500 would stop being retried, though both are listed in the existing backoff tuple. Adding the 5xx range and 429 to _TRANSIENT_HTTP_CODES, or narrowing the giveup so it only fires for classes not already in the tuple, would keep the current behaviour intact.

…or RateLimitError and InternalServerError

The _is_terminal_api_error giveup function previously returned True
for any APIStatusError whose status code was not in _TRANSIENT_HTTP_CODES.
Because RateLimitError (429) and InternalServerError (500/503) both subclass
APIStatusError, the predicate fired for them too, causing the backoff
decorator to stop retrying errors that have dedicated retry entries in the
exception tuple.

Fix: return False immediately for RateLimitError and InternalServerError
instances so the giveup predicate never overrides their dedicated retry logic.
Also add 429 to _TRANSIENT_HTTP_CODES for completeness (bare APIStatusError
with 429 should also be retried, e.g. from non-standard endpoints).

New tests verify that the predicate does not fire for:
- openai.RateLimitError (429)
- openai.InternalServerError (500, 503)

Signed-off-by: Aftabbs <aftabbs.wwe@gmail.com>

@jmartin-tech jmartin-tech left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please carefully read the comments and review code in #1973, us of giveup is not appropriate to address this issue.

Note the final approach before #1973 was retracted by the contributor was moving in the desired direction however needed closer attention from the contributor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NIM generator aborts entire probe on a single transient HTTP error (408), and reports a misleading error message

3 participants