fix(auxiliary): skip same-provider retry on a vision full-budget timeout

[xmhuangzhijun-hue]

What does this PR do?

Issue #54465 established that a same-provider retry after a full-budget timeout costs a second whole timeout window before the fallback chain is reached — doubling the user-visible stall — and that compression must not pay it because it sits on a critical path.

The guard added for that is spelled task == "compression", at both retry sites:

if task == "compression" and _is_timeout_error(transient_err):

So vision still retries, and vision sits on the interactive path. The cost is identical and the stall is more visible: the turn holding the image cannot answer, and because turns are serialised the following user messages queue behind it. Two sequential full-budget timeouts against an unhealthy vision provider is a long stall for something the fallback chain could have served immediately.

This replaces the string comparison at both sites (sync call_llm and async_call_llm) with a shared _TIMEOUT_NO_RETRY_TASKS = frozenset({"compression", "vision"}), so the two paths cannot drift apart again.

Behaviour is unchanged for every other task. Fast blips (a streaming-close or a 5xx) still retry — only full-budget timeouts on those two tasks skip straight to fallback.

Found while running a deployment with a configured auxiliary.vision provider on a link that times out intermittently.

Related Issue

Precedent and rationale: #54465

Not a duplicate of #51513 — that fixes five separate defects inside the vision fallback chain (capability detection, sync/async client misuse, geo-block and RemoteProtocolError classification, chain iteration). This is about the wasted timeout window before that chain is reached, and the two changes are independent.

Type of Change

• 🐛 Bug fix (non-breaking change that fixes an issue)
• ✨ New feature (non-breaking change that adds functionality)
• 🔒 Security fix
• 📝 Documentation update
• ✅ Tests (adding or improving test coverage)
• ♻️ Refactor (no behavior change)
• 🎯 New skill (bundled or hub)

Changes Made

• agent/auxiliary_client.py — added _TIMEOUT_NO_RETRY_TASKS; both retry sites now test membership instead of comparing to the literal "compression"; comments updated to state why vision qualifies. Log lines carry the task name rather than hardcoding it.
• tests/agent/test_auxiliary_client.py — two tests in TestTransientTransportRetry:
  • test_vision_skips_same_provider_retry_on_timeout — primary is tried exactly once, then fallback.
  • test_non_critical_task_still_retries_same_provider_on_timeout — an ordinary task keeps its one same-provider retry, proving the change is scoped rather than blanket.

Testing

The vision test has teeth: reverting the source change fails it while leaving the scoping test green.

pytest tests/agent/test_auxiliary_client.py:

• without this change: 5 failed, 180 passed
• with this change: 5 failed, 182 passed (the +2 are the new tests)

The 5 pre-existing failures are unrelated to this path and reproduce on a clean checkout of main.

One note for review: vision resolves its client through resolve_vision_provider_client() rather than _get_cached_client(), so the new test patches that instead — the retry block under test is shared by both paths.

[Enough1122]

AI code review — automated review for reference; please use your judgment.

Overall: Sound extension of the "skip same-provider retry on full-budget timeout" policy from compression to vision — both sit on user-visible critical paths where a second full timeout window stalls the turn.

What it does

• Introduces a module-level _TIMEOUT_NO_RETRY_TASKS = frozenset({"compression", "vision"}) and uses task in _TIMEOUT_NO_RETRY_TASKS and _is_timeout_error(...) in both the sync (_call_llm_impl) and async (_acreate) paths, replacing the previous task == "compression" checks.
• Log lines updated to include the task name, keeping the sync/async messages consistent.
• Tests add a vision-timeout fallback case (asserts exactly one primary attempt + fallback) and a negative control proving non-critical tasks (title) still retry the same provider once — confirming this is not a blanket behavior change.

Non-blocking notes

• The frozenset is keyed on the exact task string "vision". If any call site passes a variant (e.g. "image", "vision_analysis") it silently misses the skip. Worth a quick grep that every vision-path call uses the literal task="vision"; a unit test that exercises the actual call-site constant would pin it.
• The compression vs vision critical-path reasoning is documented well; the comment references the same issue Preflight context compression can wedge resumed sessions when auxiliary compression times out #54465 for both, which is accurate for compression but the vision rationale is broader (serialised turn stall) — the inline comment covers this correctly.

No blocking issues; the change is minimal, symmetric across sync/async, and well-tested.

Non-blocking — please use your judgment.

[xmhuangzhijun-hue]

Confirmed against the PR tree: the production vision entry points use the exact "vision" task value. tools/vision_tools.py builds call_kwargs with "task": "vision" for both image and video analysis, and tools/browser_camofox.py passes task="vision". I found no production call site using an image or vision_analysis variant. The test also reaches call_llm(task="vision"), so the frozenset key matches the real path.

[kshitijk4poor]

Triage note (perf/P2 sweep, verified against origin/main 30b83ab7b1):

Canonical for the vision-timeout pair (#97572 / #87087). Premise live on main: only task == "compression" skips the same-provider retry, at both the sync (~10632) and async (~11465) sites; tools/vision_tools.py drives async_call_llm(task="vision").

Your diff no longer applies cleanly because f50b5bb added a carve-out on the sync site after your base (a no-progress timeout — dead stream, zero output within 60 s — is cheap and still retries; only stalls/hard-ceiling skip). Salvaged onto current main as #101570 with your commit cherry-picked (authorship preserved; conflict resolved keeping that carve-out) plus two follow-ups of ours: mirror the carve-out on the async site (it never had it — a stillborn async vision stream would have skipped to fallback where sync retries), and fold the three-clause decision into one _should_skip_same_provider_retry(task, exc) predicate next to _TIMEOUT_NO_RETRY_TASKS so the two sites can't drift again.

Benchmark (macOS / Apple Silicon; stubbed network — primary sleeps TIMEOUT_S=0.5 then raises an APITimeoutError look-alike, fallback answers instantly; task="vision", ×5, median):

	sync	async	primary attempts
main	1015 ms	1010 ms	2
#101570	517 ms	511 ms	1

What actually improved: one timeout window instead of two before fallback runs — with the real vision timeout that is on the order of minutes saved per timed-out image, on the path that blocks the next user message. Tests: your two tests + test_codex_aux_no_progress_timeout.py 15/15 on the salvage; reverting the change fails the vision-skip tests.

[kshitijk4poor]

Merged via #101570 (rebase-merge, your commit cherry-picked with authorship preserved — merge commit c4e394cdf8 on main). Thanks @xmhuangzhijun-hue.

On the review notes here: the literal "vision" task key you confirmed is also what I verified on current main (tools/vision_tools.py, tools/browser_camofox.py). The sync/async "same reasoning" comment concern is resolved structurally — both sites now call one _should_skip_same_provider_retry(task, exc) predicate that carries the rationale (and the no-progress carve-out from f50b5bb) in a single place. Closing this PR since the change is on main.