refactor(chat): guard the blocking HTTPException arm so it can't log status 200

[hasitpbhatt]

Orca-Code-Review — push 2

Severity	Count	Δ vs previous push
P0	0	0
P1	0	0
P2	0	0
P3	0	0

✅ no blocking findings

Not a live bug — defensive guard

Closes #158, which has been reworded (and typed Task) accordingly.

#158 claimed an HTTPException from inside the blocking try already lands in requests_log as status_code = 200. That premise was wrong — @xizhuomengcontin showed it in review by driving the #153 hosted-fallback 429 end-to-end on main, where the row is already (429, 'rate_limit_error', 0).

I verified it independently, and the arm is unreachable for a stronger reason than "the adapter doesn't raise HTTPException":

• packages/litellm_adapter/client.py:201-204 — acompletion wraps the router call in a blanket except Exception as exc: raise _translate_error(exc) from exc, so whatever litellm raises arrives as UpstreamProviderError. HTTPException subclasses Exception, so it would be translated too.
• packages/litellm_adapter/client.py:76 — the hosted-fallback rate-limit signal becomes UpstreamProviderError(http_status=429, error_type="rate_limit_error") → the except UpstreamProviderError arm, which already records exc.http_status.
• No HTTPException anywhere in packages/; every raise HTTPException in the engine is pre-dispatch (:403 allowlist, :442/:509/:517 auto-resolution, :549 budget — all before the try) or is an except arm itself.

So this PR changes behaviour on no reachable path — hence refactor, not fix.

Why merge it anyway

except HTTPException is the only one of the three arms that leaves the handler-local status_code at its initial 200, so its finally would write a success-shaped row (cost ~0, error_type NULL) for a failed request. It becomes live the first time anything inside that try raises HTTPException, which is plausible near-term given where the hosted-fallback work is heading. #91's blocking budget settlement already has to work around the same shape of problem (status_code < 400 plus a "was a completion dict actually delivered" gate) — this removes that footgun at the root.

Fix

Unchanged: record status_code = exc.status_code in the HTTPException arm before re-raising, matching the sibling arms. Two lines + one integration test.

Push 2 (986aac6) is comment and docstring only: it deletes the wrong rationale ("Raised from inside the try today by the hosted-fallback signal (HTTPException(429))") from the new code comment and from the test docstring, and states the accurate one. No behaviour change; suite re-run green, ruff clean.

Deliberately out of scope:

• error_type from ERROR_TYPE_HEADER — a follow-up if there's appetite; the guard doesn't need it.
• The genuinely-wrong sibling gaps are missing rows, not mis-recorded ones, and are written up under "Out of scope" in Blocking chat path's except HTTPException arm cannot record its status (defensive guard) #158: pre-dispatch rejections (allowlist 403, auto-resolution 422/403, budget-exhausted 429) raise before the try so no row is written at all, and the streaming path's twin writes no row on a pre-stream HTTPException either.

Test plan

• test_chat_completion_blocking_httpexception_logs_real_status — injected HTTPException(429) → response 429 and log row status_code == 429, cost_microcents == 0. The injection is a shape the real adapter never produces, so this pins the handler's arm logic; it is not evidence the path is reachable.
• Full suite green — 703 passed, re-run on 986aac6
• ruff check app packages tests clean
• No pre-existing hosted-fallback 429 test asserted a 200 row, which is consistent with the arm being dead code

Interaction with #91

Discovered while reviewing #91's blocking budget settlement, which keys off status_code < 400: #91 had to additionally gate on "a completion dict was actually delivered" so an early failure couldn't be charged the full remaining allowance (push 6). Landing this first removes that footgun at the root; #91's rebase is a 3-line touch on the adjacent arm. Both orders work; this is the smaller change.

One follow-up that rebase should pick up: #91's settlement comment (eef1eff) justifies its response gate partly by "the re-raised HTTPException above, whose status_code never left 200". Once this lands that parenthetical is stale — the arm does record the status now, and the gate's remaining justification is the no-usage-delivered case.

[hasitpbhatt]

Spun off from the #91 self-review: while auditing the blocking budget settlement for P1 threads, this arm surfaced as the root cause of the empty-response/status-200 footgun. Small diff, safe to prioritize before #91 — see #158 for the analysis.

[orcacode-review]

🐳 OrcaCode Review

✅ No findings — nothing to flag in this PR. Great work!

_{OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 249 calls · 6.8M tokens · 94% cached}

❤️ Share · Install OrcaCode Review

Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.

Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter

[xizhuomengcontin]

Happy to take this one. It merges clean onto main (7d2ce05), ruff is clean and the suite is 703 passed (×2), and the change itself is right — all three except arms should record their outcome.

But I can't reproduce the bug as described, and I think the premise in #158 is wrong.

#158 says:

Any HTTPException raised from inside the try — today that means the adapter's hosted-fallback signal from #153, which raises HTTPException(429, ...) — lands in requests_log as status_code = 200

I drove exactly that path end-to-end: two fake upstreams (local BYOK on one port, hosted on another), local pinned to 429, blocking (non-stream) requests, then read the rows back out of requests_log. On main, without this PR:

client status sequence : [429, 200, 200, 200, 200, 200, 200, 200, 200, 200]
requests_log row       : (429, 'rate_limit_error', 0)

The 429 is already recorded correctly. Two runs, same result.

The reason is that the adapter doesn't raise HTTPException at all:

$ grep -rn "HTTPException" packages/
(no matches)

_translate_error returns UpstreamProviderError(msg, http_status=429, error_type="rate_limit_error") (packages/litellm_adapter/client.py:75-76), which lands in the except UpstreamProviderError arm — the one that already does status_code = exc.http_status. The blocking path's try only wraps client.acompletion(...) and the actual_resolved assignment, and every raise HTTPException in app/routes/chat.py sits outside it (the ones at :1026 and :1037 are those except arms). So except HTTPException on the blocking path is currently unreachable.

Worth noting the test here passes because it injects the exception directly:

fake.acompletion = AsyncMock(side_effect=HTTPException(status_code=429, ...))

That's a shape the real adapter never produces — so the test confirms the handler's logic, but not that the path is reachable.

None of that makes the change wrong. Leaving one of three arms able to leak status_code = 200 is a latent trap, and it becomes live the moment anything inside that try starts raising HTTPException — which is a plausible near-future change given where the hosted-fallback work is heading. I'd merge it as a defensive guard.

What I'd like to adjust is the framing: #158 is written as a live data-integrity bug ("Analytics and spend history count failed requests as successes"), and on current main that isn't happening. Could you reword the issue — or let me know if you have a repro I'm missing, in which case I'd rather understand it than merge on a wrong rationale. If you'd rather I just merge it with a note in the squash message, say so and I'll do that.

[hasitpbhatt]

No repro on my side either — you're right, and the premise in #158 was wrong. I verified it the same way you did, plus the structural reason it can't happen at all: acompletion wraps the router call in a blanket except Exception as exc: raise _translate_error(exc) from exc (packages/litellm_adapter/client.py:201-204), so anything litellm raises — HTTPException included, since it subclasses Exception — already arrives translated as UpstreamProviderError. Your grep was stronger than it looked.

Reworded rather than merged on a wrong rationale:

• Blocking chat path's except HTTPException arm cannot record its status (defensive guard) #158 retitled and rewritten to lead with the correction (the reachability proof, your end-to-end (429, 'rate_limit_error', 0) result, the client.py:76 translation), and typed Task so the classification is explicit. The live-bug claim is gone.
• refactor(chat): guard the blocking HTTPException arm so it can't log status 200 #159 is now refactor(chat): guard the blocking HTTPException arm so it can't log status 200 — refactor, because nothing changes on a reachable path. That's also the squash subject, so the old fix(chat) headline never reaches main.
• push 2 (986aac6) fixes the two places my wrong claim had already gotten into the code: the rationale comment on the arm and the new test's docstring. Comment/docstring only — re-ran the full suite at 703 passed, ruff check app packages tests clean.

Please do merge it as a defensive guard. The reason I'd keep on the record is the one in the reworded #158: it's the only one of the three arms that can leave status_code at its initial 200, so the first HTTPException from inside that try — plausible soon, given where hosted fallback is heading — logs a failed request as a success, and #91 already carries a workaround keyed to exactly that shape.

One thing your audit surfaced that is real, now listed under "Out of scope" in #158 instead of growing this diff: every pre-dispatch rejection (allowlist 403 at :403, auto-resolution 422/403 at :442/:509/:517, budget-exhausted 429 at :549) raises before the try, so those requests write no row at all — same class as the streaming pre-stream gap you noted. That's a missing-row problem, not a mis-recorded one; happy to take it as its own issue if you want it tracked.

[orcacode-review]

🐳 OrcaCode Review

✅ No findings — nothing to flag in this PR. Great work!

_{OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 195 calls · 4.4M tokens · 93% cached}

❤️ Share · Install OrcaCode Review

Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.

Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter

[xizhuomengcontin]

Merged. And thanks for the correction on my correction — your structural argument is stronger than my grep, and I've confirmed it:

# packages/litellm_adapter/client.py:201-204
try:
    resp = await self._router.acompletion(**kwargs)
except Exception as exc:
    raise _translate_error(exc) from exc

A blanket except Exception, and HTTPException subclasses Exception, so even if litellm did raise one it would arrive as UpstreamProviderError. That makes the arm unreachable structurally, not just "nothing happens to raise it today" — which is a better reason to keep the guard than the one I gave.

I also verified it at the branch level rather than by inference this time. Instrumented all three arms and ran every blocking-path failure mode, on main and on this branch:

scenario	client	logged row	arm taken
upstream 429	429	(429, 'rate_limit_error')	UpstreamProviderError
upstream 500	503	(503, 'upstream_error')	UpstreamProviderError
upstream 503	503	(503, 'upstream_error')	UpstreamProviderError
upstream unreachable	503	(503, 'upstream_error')	UpstreamProviderError
success	200	(200, None)	—

ARM=HTTPException never fires, and client status matches the logged status in all five. Identical on both sides, so this is a behavioural no-op on every reachable path — which is what the reworded comment and docstring now say. ruff clean, 703 passed across 4 runs.

Withdrawing one thing from my own comment: I wrote that an HTTPException from inside that try was "a plausible near-future change given where the hosted-fallback work is heading." That was speculation about your roadmap with nothing behind it. The structural reason you gave is the real argument; mine was hand-waving.

---

On the out-of-scope item — it reproduces, and it's worth its own issue.

Your line numbers are from #91, so I located the equivalents on main: the allowlist 403 at chat.py:366, and the auto-resolution 422/403 at :390, :457, :465. All of them raise before the try at :1010, so the finally that writes the row never runs.

Confirmed end-to-end, 3 identical runs:

scenario	client	rows written
model="auto", no provider configured → 422	422	0
model outside the key's model_allowlist → 403	403	0
model inside the allowlist → 200 (control)	200	1
upstream 429 (control, reaches the try)	429	1

So an operator can't see that a key is being blocked by its allowlist, or that auto-routing is failing to resolve — those requests simply don't exist in analytics. Different class from a mis-recorded row, as you said, and arguably worse for the allowlist case, since that's exactly the signal you'd want when debugging why a key "doesn't work".

I'll open an issue with this repro unless you'd rather own it — say the word. It'll want the streaming pre-dispatch gap folded in too, since it's the same shape.