Skip to content

fix(litellm): retry transient upstream 5xx at the proxy#882

Open
Yiminnn wants to merge 4 commits into
mainfrom
fix/consolidate-mimo-native
Open

fix(litellm): retry transient upstream 5xx at the proxy#882
Yiminnn wants to merge 4 commits into
mainfrom
fix/consolidate-mimo-native

Conversation

@Yiminnn

@Yiminnn Yiminnn commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator

The one narrow exception #830's rationale permits: num_retries: 0 stays (deterministic rejects never multiply), but transient upstream InternalServerErrors are absorbed (retry_policy: {InternalServerErrorRetries: 2}).

Why: some agents silently swallow a passed-through 500 into a zero-event turn — observed as the MiMo Code flake (turn ends with no error on a deepseek 500, repro ×3 in the 64-agent campaign log). With the proxy absorbing transient 500s, the agent never sees them. 4xx/auth/content classes keep zero retries and fail fast.

Companion: benchflow-ai/agents#47 — mimo consolidates on the upstream-native mimo acp adapter (manifest takes the bare mimo name via the decoupled-manifest override; core wrapper remains only as fallback).

tests/test_litellm_config.py updated + 20 green; ruff check+format clean.

…pass)

The proxy deliberately runs num_retries=0 (#830: BenchFlow owns task-level
retry classification; never multiply deterministic provider rejects). But a
transient upstream InternalServerError is not a deterministic reject — and some
agents silently swallow a passed-through 500 into a zero-event turn (observed:
MiMo Code ending turns with no error on a deepseek 500, repro x3 in the 64-agent
campaign log). Add the one narrow exception the #830 rationale permits:
retry_policy {InternalServerErrorRetries: 2}. Every deterministic class (4xx,
auth, content) keeps zero retries and fails fast.

Companion: benchflow-ai/agents#47 consolidates mimo on the upstream-native
`mimo acp` adapter (manifest takes the bare name; core wrapper = fallback).
@Yiminnn Yiminnn temporarily deployed to pypi-internal-preview July 2, 2026 19:07 — with GitHub Actions Inactive
@bingran-you bingran-you added bug Something isn't working P1 Important debt — must fix soon, but does not block the current release. area:eval Issue / PR lives primarily in the "eval" subsystem. review:pending PR is ready-for-review, no reviewer engagement yet. review:changes-requested Author needs to push more commits before this can merge. and removed review:pending PR is ready-for-review, no reviewer engagement yet. labels Jul 3, 2026

@bingran-you bingran-you 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.

Daily scan review (2026-07-03): requesting changes. The PR is correctly scoped to benchflow-ai/benchflow and CI is green, but the configured LiteLLM retry policy appears to be a no-op with the locked LiteLLM router path. Direct probing shows RetryPolicy(InternalServerErrorRetries=2) is accepted as config shape, but LiteLLM does not apply it for InternalServerError, so with BenchFlow keeping num_retries=0, transient 500s still resolve to no retry. The current tests only assert generated config shape, so they would pass even though the runtime retry behavior is unchanged. Please switch to a LiteLLM hook/patch/version path that actually retries 500s, and add behavioral coverage proving: 500 gets the intended retry count, while context-window/permanent 400 errors still follow the #830 fail-fast behavior.

@bingran-you

Copy link
Copy Markdown
Collaborator

Users Simulation review (2026-07-03): blocked.

Targeted checks passed, but the intended retry fix appears to be a no-op with the pinned LiteLLM behavior:

  • ruff format --check src/ tests/ -> passed
  • ruff check src/ tests/ -> passed
  • ty check src/ -> passed
  • python -m pytest tests/test_litellm_config.py -q -> 20 passed
  • bench --help / benchflow --help -> passed

Blocker: src/benchflow/providers/litellm_config.py serializes retry_policy: {"InternalServerErrorRetries": 2}, but a runtime Router probe on LiteLLM 1.89.0 returned None for InternalServerError; the helper only honored retry policy for other error classes such as BadRequestError. The new test checks config shape, not whether upstream 5xx retries actually happen. Please wire a supported retry path or add a runtime test that proves the Router retries transient 5xx.

No trajectory artifacts were generated.

@bingran-you bingran-you added the status:blocked Waiting on external dependency. Add a comment explaining why. label Jul 3, 2026
@bingran-you bingran-you temporarily deployed to pypi-internal-preview July 3, 2026 13:25 — with GitHub Actions Inactive
@bingran-you

Copy link
Copy Markdown
Collaborator

Addressing blocker #882 (comment) and CHANGES_REQUESTED review #882 (review).

Pushed commit f7e3267 to fix/consolidate-mimo-native.

What changed:

  • Added a standalone LiteLLM runtime retry patch loaded by the proxy sitecustomize in both host and sandbox runtimes.
  • The patch fills LiteLLM 1.89.0 missing InternalServerError retry-policy branch only; BenchFlow keeps router_settings.num_retries=0 and disable_cooldowns=true for deterministic failures.
  • Added behavioral Router coverage proving two transient upstream 500s are retried and then succeed, while BadRequestError and ContextWindowExceededError remain fail-fast with one call.

Validation:

  • source .venv/bin/activate; python -m pytest tests/test_litellm_config.py tests/test_litellm_hardening.py -q -> 70 passed
  • uv run ruff check src/ tests/ -> All checks passed
  • uv run ty check src/ -> All checks passed

Leaving status:blocked in place until GitHub CI is green.

@bingran-you

Copy link
Copy Markdown
Collaborator

Automation status update (2026-07-04): GitHub checks are green on f7e32675 (test, pip-audit, parity, detect-scope, rollout-smoke; scope matrix/review-pack skipped as expected), and the latest follow-up comment records the LiteLLM 5xx retry behavioral fix plus local validation. Moving labels from status:blocked / review:changes-requested to status:ready / review:pending; the prior CHANGES_REQUESTED review still needs human re-review before merge.

@bingran-you bingran-you added status:ready Triaged, unassigned, available to claim. review:pending PR is ready-for-review, no reviewer engagement yet. and removed status:blocked Waiting on external dependency. Add a comment explaining why. review:changes-requested Author needs to push more commits before this can merge. labels Jul 4, 2026
@bingran-you

Copy link
Copy Markdown
Collaborator

Users Simulation automation review (2026-07-04): ready by simulation on current head f7e32675.

The retry fix was validated at runtime rather than only by config shape:

  • uv run --extra dev python -m pytest tests/test_litellm_hardening.py::test_runtime_retry_patch_retries_only_transient_5xx tests/test_litellm_config.py::test_proxy_config_registers_plain_and_openai_aliases tests/test_bedrock_thinking.py::test_bedrock_thinking_effort_is_threaded_into_route_params tests/test_bedrock_thinking.py::test_every_requestable_bedrock_effort_survives_real_litellm_transform -q -> 4 passed.
  • A subprocess probe from a generated LiteLLM runtime dir confirmed sitecustomize loads the patch, transient 500 retries happen, and BadRequestError still fails fast.
  • Focused ruff passed; touched provider source passed ty.

Shared sweep canary produced a healthy Docker OpenHands + DeepSeek artifact bundle with ACP+LLM trajectories, results.jsonl, 37,265 tokens, $0.00273728, 71.4s total, and validator healthy. Keep status:ready / review:pending; the existing CHANGES_REQUESTED review still needs human re-review before merge.

@bingran-you

Copy link
Copy Markdown
Collaborator

Users Simulation automation review (2026-07-05): ready by simulation on head f7e32675b334171e09d87682ac1404f3f00413ab.

Validated CLI help, SDK LiteLLM config construction, ruff, ty, and the LiteLLM-focused pytest slice (70 passed). The runtime patch path is active and focused tests confirm transient upstream 500s retry while 400-class errors still fail fast. No blocking thermo-nuclear issue found.

Shared gates passed: uv sync, ruff, ty, 71-test CLI/task/usage slice, bench help/task check/SDK smoke. Live Docker/OpenHands + Gemini flash-lite canary is healthy: ACP+LLM trajectories, results.jsonl training_ready=1, reward 1.0, 24,869 tokens, $0.00079715, 69.8s at /tmp/benchflow-users-simulation-20260705/shared-openhands-gemini-flash-lite-default/2026-07-05__05-11-14/hello-world-task__40312589. OpenHands max-effort was quarantined because that ACP agent does not declare reasoning_effort=max.

Labels: keep status:ready + review:pending; stale human CHANGES_REQUESTED still needs human dismissal or re-review.

@bingran-you

Copy link
Copy Markdown
Collaborator

Users Simulation automation review (2026-07-06): ready by simulation on head f7e32675; old GitHub review gate still blocks merge.

Evidence:

  • Focused checks passed: LiteLLM retry config/runtime tests (70 passed), ruff, ruff format --check, ty check src/, and CLI help smoke.
  • Real local SDK canary passed on openhands + deepseek/deepseek-v4-flash, docker, usage_tracking=required: reward 1.0, 2 tool calls, 37,312 tokens, timing total 67.3s, agent_result.cost_usd=0.00357686, price_source=litellm.
  • The local canary was revalidated with benchflow-experiment-review: 1/1 rollout healthy, ACP + LLM trajectories present, results.jsonl training-ready.

No code blocker found. Live GitHub still shows reviewDecision=CHANGES_REQUESTED / mergeStateStatus=BLOCKED, so human review cleanup is still needed before merge.

@bingran-you bingran-you temporarily deployed to pypi-internal-preview July 8, 2026 08:45 — with GitHub Actions Inactive
@bingran-you

Copy link
Copy Markdown
Collaborator

Daily scan follow-up (2026-07-08): fixed the thermo-nuclear file-size blocker on current branch head 1e4bb7f75.

The retry behavioral coverage added by this PR pushed tests/test_litellm_hardening.py over the 1k-line threshold. I split that focused retry-patch regression into tests/test_litellm_retry_patch.py without changing production behavior.

Validation in isolated worktree /tmp/benchflow-daily-pr882:

  • uv sync --extra dev --locked
  • uv run python -m pytest tests/test_litellm_retry_patch.py tests/test_litellm_hardening.py tests/test_litellm_config.py -q -> 70 passed
  • focused ruff check -> passed
  • ruff format --check tests/test_litellm_retry_patch.py tests/test_litellm_hardening.py -> passed
  • line counts: tests/test_litellm_hardening.py 981 lines; tests/test_litellm_retry_patch.py 132 lines

Keeping status:ready / review:pending; refreshed CI and reviewer re-review are still required before merge because GitHub still has the old CHANGES_REQUESTED review gate.

@bingran-you bingran-you added review:changes-requested Author needs to push more commits before this can merge. and removed review:pending PR is ready-for-review, no reviewer engagement yet. labels Jul 8, 2026
@bingran-you

Copy link
Copy Markdown
Collaborator

Users Simulation automation review (2026-07-08): simulation ready, waiting on human re-review on head 1e4bb7f75eeefb94cdc2de52030a36ed29077e90.

Evidence:

  • LiteLLM retry canary passed: simulated InternalServerError twice, then succeeded on the third call; BadRequestError and ContextWindowExceededError still fail fast.
  • Sandbox runtime probe passed: _start_sandbox_litellm uploads benchflow_litellm_retry_patch.py, and sitecustomize.py imports both runtime patch modules.
  • Focused checks passed: tests/test_litellm_config.py, tests/test_litellm_hardening.py, tests/test_litellm_retry_patch.py, focused ruff, focused ty.
  • Shared base sweep passed: ruff check ., ty check src, and 149 CLI/SDK/task/usage/skill tests.
  • Current-head integration-light-jobs artifact passed benchflow-experiment-review: ACP + LLM trajectories and training-ready results.jsonl; reward 1.0, 268,393 tokens, 16 tool calls, total timing 394.3s.

Thermo-nuclear review: no blocker. The retry patch is isolated and direct. Minor cleanup opportunity only: _write_runtime_files now writes a fourth runtime file while still returning a 3-tuple.

Metadata caveat: GLM artifact token/timing coverage is healthy, but result.json.agent_result.cost_usd / price_source are null. This is a shared GLM user-endpoint cost-accounting gap, not a retry regression.

Labels: status:ready; changed review label to review:changes-requested because GitHub still reports reviewDecision=CHANGES_REQUESTED. Human re-review is still required before merge.

@bingran-you

Copy link
Copy Markdown
Collaborator

Users Simulation automation review (2026-07-10): blocked on head 1e4bb7f75eeefb94cdc2de52030a36ed29077e90.

What passed:

  • bench --help, bench agent list --help, bench tasks check --help
  • bench tasks check tests/conformance/acp_smoke
  • pytest tests/test_litellm_config.py tests/test_litellm_retry_patch.py -q (21 passed)
  • ruff check on the touched LiteLLM runtime/retry/config files and tests

Artifact gate:

  • A model-backed demo run was attempted with codex-acp + openai/gpt-5.4-mini, but the artifact is unhealthy: the run ended in proxy/auth failure before any tokened completion (ACP error -32000: Authentication required, tokens=0, tool_calls=0, results.jsonl has training_ready=false). That is local auth/process evidence, not a healthy rollout.

Blocking thermo-nuclear finding:

  • src/benchflow/providers/litellm_retry_patch.py applies the 5xx retry behavior as an import-time monkeypatch and catches all setup/import failures by returning silently. src/benchflow/providers/litellm_runtime.py imports it through sitecustomize, but there is no fail-closed startup assertion that the LiteLLM helper was actually patched. If LiteLLM internals drift or the runtime file stops loading, the proxy can still boot while silently losing the retry behavior.

Please add an explicit startup probe/fail-closed check, similar in spirit to the Bedrock preflight, or move the retry behavior into a normal startup path that cannot disappear silently.

@bingran-you bingran-you added the status:blocked Waiting on external dependency. Add a comment explaining why. label Jul 10, 2026
@bingran-you bingran-you removed the status:ready Triaged, unassigned, available to claim. label Jul 10, 2026
@bingran-you bingran-you temporarily deployed to pypi-internal-preview July 10, 2026 17:11 — with GitHub Actions Inactive
@bingran-you

Copy link
Copy Markdown
Collaborator

Users Simulation automation follow-up (2026-07-10): current head 3eaba85c37e8c8f9bdde4c43ed432adb506b16bb appears to address the earlier fail-open retry-shim blocker.

New evidence by inspection:

  • litellm_retry_preflight.py adds a fail-closed probe that verifies the patched helper is active, checks InternalServerErrorRetries, and confirms permanent BadRequestError does not retry.
  • litellm_runtime.py now runs preflight_host_retry_patch(...) for the host proxy and uploads/runs preflight_sandbox_retry_patch(...) for sandbox proxies before agent launch.

I am leaving status:blocked for now because current-head test and rollout-smoke are still pending. Once those pass, this specific automation blocker should be cleared and labels can move back to status:ready / review:pending for human re-review.

@bingran-you

Copy link
Copy Markdown
Collaborator

Users Simulation automation follow-up (2026-07-10): ready by simulation on current head 3eaba85c37e8c8f9bdde4c43ed432adb506b16bb.

The retry-patch fail-open blocker from #882 (comment) is addressed by 3eaba85c3:

  • added a fail-closed LiteLLM retry preflight that probes the active runtime helper without importing the patch module and masking load failures;
  • host and sandbox LiteLLM startup now run that preflight after proxy health and before agent launch;
  • added regression coverage proving the preflight passes when sitecustomize loads the retry patch, fails when the patch is absent, and the sandbox path tears down instead of returning a proxy when the retry patch is inactive.

Validation:

  • uv run python -m pytest tests/test_litellm_retry_patch.py tests/test_litellm_hardening.py tests/test_litellm_config.py -q -> 75 passed
  • focused ruff check / ruff format --check on touched retry/runtime files -> passed
  • uv run ty check src/ -> passed
  • git diff --check -> passed
  • GitHub CI is green on 3eaba85c3: test, pip-audit, manifest-parity, detect-scope, and integration-light / rollout-smoke.
  • The latest integration-light-jobs artifact validates healthy with benchflow-experiment-review: ACP trajectory, LLM trajectory, results.jsonl, reward present, 265661 tokens, 15 tool calls, and training_ready=1.

Moving the PR back to status:ready; keeping review:changes-requested because GitHub still reports the prior CHANGES_REQUESTED review and human re-review is required before merge.

@bingran-you bingran-you added status:ready Triaged, unassigned, available to claim. and removed status:blocked Waiting on external dependency. Add a comment explaining why. labels Jul 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:eval Issue / PR lives primarily in the "eval" subsystem. bug Something isn't working P1 Important debt — must fix soon, but does not block the current release. review:changes-requested Author needs to push more commits before this can merge. status:ready Triaged, unassigned, available to claim.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants