Report streaming failures with the real status, and fail over locally - #18
maiphucgiang merged 9 commits into
Conversation
A streaming request that fails before the first byte reaches the client used to answer HTTP 200 with a single in-band `error` event. `StreamingResponse` commits the response headers as soon as it is iterated, while the upstream call happens inside the generator, so an upstream 429, a connect/write timeout or a content-filter rejection had nowhere to go but the SSE body. Clients parse per chunk: an OpenAI SDK sees a chunk without `choices`, Codex never receives `response.completed`, and the turn silently ends as "the model answered with nothing" - no retry, no error, and the audit log records a success. Non-streaming requests of the same failure already returned the real status, so the two paths disagreed about what failed means. - Prefetch the first segment before committing to 200, so a failure that happened before any byte went out is surfaced with the real HTTP status on all three endpoints; a stream that breaks after bytes were already sent still reports in band, because its status can no longer be recalled. - Treat a write timeout like a connect failure in `open_backend_stream`: the request body was never accepted, so replaying it on a fresh connection cannot double-bill. Large cross-border sessions hit the 60 s write timeout far more often than they hit connection setup. - Add `--failover-max` (default 0, off): retry such a failure on another credential, bounded by N, so the client still sees one normal response. Only upstream HTTP rejections (401/403/429/502/503/504) and never-accepted request bodies are replayed; content-filter rejections, 502s synthesized from an already-open stream, read timeouts and protocol errors never are. When no other credential can be picked, the first real status is surfaced instead of masking it with a 503. - `observe_recovery()` keeps the audit outcome honest after a saved replay: the failed attempt stays in `attempts` (plus a `failover_recovered` marker) without leaving an `error` + 200 row behind. `tests/test_refusal.py` and `tests/test_workbuddy_filter.py` pinned the old 200-plus-in-band-error contract on the streaming paths; they now assert that both stream modes report the same status. `test_runtime_endpoints.py` counted exactly three pooled `_route_chat` call sites; the failover paths add two more, and the invariant it protects (never select credentials on the event loop) is asserted as a minimum now.
Reviewer's GuideThe PR fixes misleading HTTP 200 responses for streaming failures by prefetching the first output segment, then optionally performs bounded, billing-safe credential failover for failures that are known to occur before request acceptance or are explicit upstream HTTP rejections. It unifies streaming and non-streaming routing, status handling, credential exclusion, threadpool execution, and audit recovery across all three generation endpoints while preserving in-band reporting for post-output failures. Sequence diagram for preflight streaming status and credential failoversequenceDiagram
participant Client
participant Endpoint
participant CredentialPool
participant Upstream
Client->>Endpoint: streaming request
Endpoint->>CredentialPool: _route_chat(payload, body, rid)
CredentialPool-->>Endpoint: credential and upstream route
Endpoint->>Upstream: open_backend_stream()
alt first segment arrives
Upstream-->>Endpoint: first output segment
Endpoint-->>Client: HTTP 200 and SSE stream
Upstream-->>Endpoint: later output or in-band error
else replayable failure before first segment
Upstream-->>Endpoint: UpstreamHTTPError or ConnectError/WriteTimeout
alt failover enabled and another credential available
Endpoint->>CredentialPool: _route_chat(..., tried=...)
CredentialPool-->>Endpoint: alternate credential and route
Endpoint->>Upstream: open_backend_stream()
Upstream-->>Endpoint: first output segment
Endpoint-->>Client: HTTP 200 and SSE stream
Endpoint->>Endpoint: observe_recovery()
else no failover
Endpoint-->>Client: original HTTP error status
end
else non-replayable failure before first segment
Upstream-->>Endpoint: upstream or synthesized stream failure
Endpoint-->>Client: real failure status
end
Flow diagram for streaming failure classificationflowchart TD
A[Start streaming request] --> B[_preflight_stream reads first segment]
B -->|segment received| C[Commit HTTP 200 and stream output]
C --> D{Failure after output?}
D -->|yes| E[Emit in-band error event]
D -->|no| F[Complete normally]
B -->|failure before output| G{_failover_safe}
G -->|no| H[Return original HTTP status]
G -->|yes| I{failover_max and alternate credential?}
I -->|no| H
I -->|yes| J[run_in_threadpool _route_chat with tried credentials]
J --> K[Replay request]
K -->|success| L[observe_recovery and return normal stream]
K -->|failure| I
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="app/upstream_io.py" line_range="199" />
<code_context>
yield response
return
- except (httpx.ConnectError, httpx.ConnectTimeout) as error:
+ except (httpx.ConnectError, httpx.ConnectTimeout, httpx.WriteTimeout) as error:
if opened or attempt == 1:
raise
if on_retry is not None:
</code_context>
<issue_to_address>
**issue (broader_impact):** `httpx.WriteTimeout` is retried as if no request bytes were accepted, but a write timeout only proves that the complete request body was not written; the upstream can already have received and processed a partial body. The retry can therefore submit the same billable request twice.
**Triggers:** When the upstream accepts and processes the request before the client-side write operation times out.
**Suggested fix:** Do not replay `WriteTimeout` without transport-level evidence that zero request bytes were accepted, or track the upload boundary and only retry before any body bytes are sent.
```suggestion
except (httpx.ConnectError, httpx.ConnectTimeout) as error:
```
</issue_to_address>Sourcery assessment
Needs a human reviewer. 1 finding to address first, and when failover is enabled, a write timeout or 502/503/504 can occur after the upstream has accepted or processed the POST, so replaying it with another credential could duplicate a billable request or consume credits twice. Reverting stops future replays but cannot undo an upstream operation or its charge.
Blocking findings: app/upstream_io.py:199
Review feedback on this PR asked whether replaying `httpx.WriteTimeout` can double-bill: a write timeout proves the client did not finish writing the body, not that the upstream accepted no bytes of it. Same question for the 502/504 in `FAILOVER_CODES` — the upstream gateway can give up on a backend that had already run the request. Both are true, and neither changes what the operator should do, because a failure at this point has no response headers and no result: the client must retry regardless. Refusing to replay does not return the credit that attempt may already have spent; it only turns a paid-for attempt into a session that ends with nothing. The choice is one billed attempt and a broken session, or two attempts and an answer. So keep the behaviour and make the cost visible instead: - `POSSIBLY_CHARGED_CODES` (502/504) marks the class where the upstream may already have processed the request; `_replay_cost_note()` appends "上游可能已处理该请求" to those replay log lines and only those. 401/403/ 429/503 and the transport failures happen at admission time, so they stay untagged. Reconciliation against the official usage detail is now a grep away rather than a guess. - Say why `WriteTimeout` still counts as "body never accepted": the request carries a `Content-Length`, so a write timeout means the declared body was not fully written — the upstream holds no complete request to run. - `--failover-max` stays default 0, so all of this is opt-in and bounded. Docs (en + zh) and `.env.example` now state the billing boundary per status class instead of claiming none of it can ever cost anything.
|
Follow-up after the Sourcery finding on The point is valid — a write timeout, and a 502/504, can follow an upstream that already accepted or processed the request. ce373e7 does not revert the behaviour, because at that stage the request has no response headers and no result: the client must retry regardless, and declining to replay returns no credit. What ce373e7 adds is honesty about the cost:
Tests: |
A write timeout proves that the declared body was not fully sent. It does not prove that the upstream ignored the bytes it did receive: what a peer does with a truncated body is upstream behaviour and cannot be observed from this side. Asserting otherwise was the weak point of the previous revision, so stop treating WriteTimeout as proof of an unaccepted request. - default replay set is back to ConnectError/ConnectTimeout only, in both the connection retry (`open_backend_stream`) and credential failover (`_failover_safe`); - `--retry-write-timeout` / `CODEBUDDY2API_RETRY_WRITE_TIMEOUT` opts in, because long cross-border sessions hit the write timeout far more often than the handshake (in one deployment's logs 100% of transport failures were write timeouts) and operators who have confirmed their upstream does not bill partial bodies should be able to recover those sessions; opt-in replays carry the same "may already have been billed" log tag; - both knobs are now hot entries in the management console's settings page, so failover can be tuned without a restart; - docs and CLI help reworded to match what the exception actually proves.
The two knobs are only useful if an operator can flip them while traffic is running, so pin the whole path: listed as hot and unlocked, defaults still match the upstream behaviour, PATCH writes through to CONFIG and persists, and the gateway is told to apply them without a restart.
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
|
Heads-up on the failed It is reachable now — Current state, for convenience:
|
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a7df12d3bf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| attempt = await run_in_threadpool(_route_chat, payload, body, rid, | ||
| tried={_cred_manager(item) for item in tried}) |
There was a problem hiding this comment.
When an auto request first selects an international credential, _route_chat rewrites its body model to default-model. This retry passes that already-mutated body back into _route_chat, so credential selection evaluates default-model rather than auto: domestic fallback credentials that support auto are excluded, and model-policy restrictions configured for auto no longer apply, allowing another international credential outside the permitted set to receive the replay. Re-route with a pristine resolved source-model body (or restore its model) instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
已修（d045bf1），与维护者那条 auto 意见同一根因，详细复现见 #18 (comment) 下的回复。
_routed_stream/_routed_fetch 的第二个参数改名为 canonical：三个端点在 _route_chat 改写模型之前留存规范请求体并传进来，重路由只用它；routed 降级成「本轮真正发给上游的报文」。_nonstream_adapted 增加 canonical=，把它透传给 _routed_fetch。
新增 4 条回归（tests/test_stream_failover.py），把 converter.py 退回上一提交后前 3 条按预期变红：
test_auto_is_rewritten_on_the_international_site：夹具自检，证明国际站确实把auto发成了default-model，否则后面几条等于没测；test_failover_cannot_widen_the_credential_binding：auto独占绑 A + A 回 503 +failover_max=1，修复前上游收到 2 次请求（A→B），修复后 1 次、下游如实 503；test_failover_cannot_widen_the_site_binding：region=intl且靶子账号的目录里也有default-model，修复前第二跳跑到国内站，修复后不再放宽；test_reroute_uses_the_pristine_body_model：直接给_route_chat装 spy，断言每一轮入参的body["model"]都仍然是auto。
全量：781 passed / 2724 subtests。
maiphucgiang
left a comment
maiphucgiang
left a comment
There was a problem hiding this comment.
Reviewed a7df12d against the current base and checked the remaining issues with small isolated reproductions. The separate, default-off write-timeout opt-in addresses the earlier default-replay concern. Two correctness blockers remain: failover can lose the original model policy, and first-chunk prefetch no longer observes client disconnects. There is also a billing-risk labeling gap in the lower retry layer. Details and suggested regression cases are attached inline.
| if limit <= 0 or len(tried) > limit or not _failover_safe(failure.error, failure.raw): | ||
| raise surface from None | ||
| try: | ||
| attempt = await run_in_threadpool(_route_chat, payload, body, rid, |
There was a problem hiding this comment.
[P1] Preserve the canonical model when selecting a failover credential
The endpoints overwrite body with the first _route_chat() result before passing it into this loop. For an international first account, auto has therefore already become default-model. Rerouting that body makes _cred_for() / model_policy.route_allowed() look up the policy for default-model, not the original auto rule. Account/profile restrictions on the requested model can be bypassed; an otherwise valid domestic fallback can also be missed.
Source-level reproduction: bind auto exclusively to international account A, with another international account B supporting the same upstream model. Let A return 503 and enable one failover. The attempts become A(default-model) -> B(default-model), even though route_allowed(..., B, "auto") is false. _routed_fetch() has the same issue.
Please retain the canonical prepared body separately from each attempt's wire body, and use its logical model for every candidate/policy check. Add an auto failover regression that proves account/profile bindings cannot widen after the first rewrite.
There was a problem hiding this comment.
已修（d045bf1）。改成两份正文分开带：
_routed_stream(payload, canonical, ...)/_routed_fetch(payload, canonical, ...)：第二参数明确是「改写前的规范请求体」，只用于候选与策略判定；routed只是本轮 wire body，负责发给上游。三个端点都在await run_in_threadpool(_route_chat, ...)之前先留prepared = body，之后传prepared。_nonstream_adapted()加canonical=None并透传给_routed_fetch（responses/messages 的非流式聚合走的就是这条），避免它继续把已经改写的body当基准。- 注释里写清为什么必须分开：
_upstream_model()只在国际站改写auto，所以第一跳之后body["model"]已经不是客户端请求的模型了。
您给的源码级复现我按形状落成了 4 条测试（tests/test_stream_failover.py）。夹具要点：两个账号的目录里都要有 default-model —— 否则国际站的 auto 根本没有资格，改写不发生，测试就变成空转。
| 测试 | 绑定 | 修复前 | 修复后 |
|---|---|---|---|
test_auto_is_rewritten_on_the_international_site |
无（自检） | 上游收到 default-model |
同 |
test_failover_cannot_widen_the_credential_binding |
credential_ids=[A] |
A→B 两枪，下游 200/多烧一次 | 只 1 枪，下游 503 |
test_failover_cannot_widen_the_site_binding |
region="intl" + 目录含 default-model 的国内站靶子 |
第二跳落到 cn | 只 1 枪，下游 503 |
test_reroute_uses_the_pristine_body_model |
spy _route_chat |
第二轮入参是 default-model |
每轮都是 auto |
红→绿是实测的：git stash push -- converter.py 之后前 3 条失败，恢复实现后全绿；全量 781 passed / 2724 subtests。
站点绑定那条一开始我拿两个国际站账号当靶子，结果「第二跳换到另一个 intl 账号」本来就是合规的，测试断言 len(sent)==1 直接挂了 —— 放宽只会发生在「改写后的模型名查不到规则」这个缝隙上，所以靶子必须是另一个站点。
| while True: | ||
| stream = make(routed, cred, headers, url) | ||
| try: | ||
| first = await _preflight_stream(stream, model_name, t0, rid) |
There was a problem hiding this comment.
[P1] Keep disconnect handling active while prefetching the first chunk
This await runs inside the endpoint, before StreamingResponse.__call__() starts its disconnect listener. If the upstream stalls before its first chunk and the client disconnects, nothing consumes http.disconnect during preflight. The upstream operation keeps running and ConcurrencyLimitMiddleware retains its slot until the upstream completes or times out. Aggregated Responses/tool streams can keep this window open for an entire generation.
Isolated ASGI reproduction with max_concurrent=1 and a generator blocked before its first yield: after http.disconnect, the base implementation closes the generator and releases the slot; this implementation leaves both live, and the next request gets 503.
Please run preflight under a disconnect-aware ASGI/response lifecycle while still deferring response headers, cancel the pending upstream read on disconnect, and explicitly close the generator in finally. Cover cancellation before the first chunk as well as after streaming starts.
There was a problem hiding this comment.
已修（d045bf1）。流式响应换成一个把预取搬进 ASGI 生命周期内部的 StreamingResponse 子类：
class _DeferredStreamResponse(StreamingResponse):
async def stream_response(self, send):
agen, first = await self._plan() # 预取 + 换凭证重放，都在 listen_for_disconnect 的任务组里
try:
await send({"type": "http.response.start", ...}) # 响应头仍然推迟到这里之后
...
finally:
await _close_stream(agen)_routed_stream() 现在只构造响应、什么都不 await（注释里写了为什么）；预取失败仍然抛 HTTPException，它从 route.handle 里穿到 ExceptionMiddleware，那一刻一个字节都没发出去 —— 所以「还原真实状态码」和 /v1/* 的协议化错误体（_protocol_http_exception）都原样保留，没有复制一份错误成形逻辑。
只把取消打进来还不够。 按您的复现做完之后我发现另一件事：anyio 的取消作用域会在每个检查点重复投递取消，于是取消落在生成器帧内部时，帧自己的 finally 做到一半就被打断 —— 也就是「生成器被关掉了」其实只是被杀掉了，httpx 关连接的那段 await 从来没跑完。最小实验（同样的作用域、finally 里一个 await）：不加屏蔽 upstream-closed 从不出现，加了才出现。所以：
- 每一段读取都放进子任务（
_first_segment）：外层取消打断的是我们的await，我们只取消子任务一次，然后在anyio.CancelScope(shield=True)里等它把finally走完； _stream_segments()用同一个原语逐段读，所以「首段之前」和「已经开始流式之后」两种取消都会走到同一套收尾；_close_stream()覆盖「取消落在两段之间、帧还停在 yield 上」的情况，屏蔽着 aclose。
测试（tests/test_stream_status_contract.py，用真实 ConcurrencyLimitMiddleware 手工驱动 ASGI，max_concurrent=1，spec_version 2.3 与 uvicorn 一致）：
test_disconnect_before_first_segment_aborts_without_a_response：断连后http.response.start一条都没发，上游生成器被关闭；test_disconnect_after_streaming_started_closes_the_upstream：响应头已发出的窗口里断连，同样关干净；test_slot_is_returned_so_the_next_request_still_runs：您复现的最终症状 —— 断连之后紧接着的请求必须还是 200，不是 503；ShieldedCloseTests.test_cleanup_await_completes_inside_a_cancelled_scope：钉住「不屏蔽就收尾跑不完」。
4 条在退回上一提交后全部按预期红（其中三条是 wait_for(task, 2) 超时 = 名额被占到读超时的缩影）。全量 781 passed / 2724 subtests。
两点如实交代：
- 为此多了
import anyio，我把anyio写进requirements.in成为声明的直接依赖；版本仍是 starlette/httpx 已经锁死的 4.14.2，requirements.txt只动了via块，哈希行没变。 - 您提到「聚合 Responses/tool 流会把这段窗口拉长到整代」，那是本 PR 之前就有的形状（
stream=false的聚合一直在端点里 await，_routed_fetch那条），不是这次预取引入的。这次我只修自己引入的回归；如果同意，我另开一个 PR 把聚合路径也搬进同一种延迟响应里（它没有「第一段」这个里程碑，得先想清楚断连时给下游回什么）。
There was a problem hiding this comment.
更正上一条回复里的实现方式（f63285c）：结论一样，做法不再需要 anyio，也不动依赖清单。
上一版用 anyio.CancelScope(shield=True) 包住收尾，并把 anyio 写进 requirements.in。base 这中间合了 #19，依赖改由 pyproject.toml + uv.lock 管，requirements.in 变成 scripts/export_requirements.py 生成的文件，tests/test_build_lock.py 会把每个传递依赖对回 lock —— 为一个 finally 扩大直接依赖集不划算，已经撤掉。
换成纯 stdlib 的等价做法，依据是读了一遍 anyio 的投递机制：_deliver_cancellation 用 call_soon 自循环，对作用域里的宿主任务每个事件循环周期重投一次取消，而它只遍历 self._tasks。所以关键不是「等得更用力」，而是把收尾放到不属于那个取消作用域的任务里：
_first_segment已经把生成器帧放进子任务，child.cancel()只投递一次，帧自己的finally（httpx 在这里关连接）没人再打断；_teardown_finished()再用最多TEARDOWN_GRACE_CYCLES=100轮、每轮asyncio.wait(..., timeout=0.01)尽量当场等完 —— 上界用轮数而不是墙上时间，因为被反复取消时每次 await 都立刻返回，按秒等就变成忙等；等不到就挂 done 回调交给后台做完，异常照样取走；_close_stream同一个套路，覆盖「取消落在两段之间、帧还停在 yield 上」那种（ShieldedClose→TeardownCloseTests钉的就是这个形状）。
四条回归不变，仍然红→绿（test_disconnect_before_first_segment_aborts_without_a_response、test_slot_is_returned_so_the_next_request_still_runs、test_disconnect_after_streaming_started_closes_the_upstream、test_cleanup_await_completes_inside_a_cancelled_scope），连跑 3 遍稳定通过；合上游后全量 836 passed / 2772 subtests。
如果您更看重「收尾必须在返回前确定完成」，那 anyio.CancelScope(shield=True) 是唯一干净的写法，而且 anyio 4.14.2 本来就在锁里 —— 说一声，我在后续提交里把 pyproject.toml 的直接依赖补上并重跑 scripts/export_requirements.py。
| try: | ||
| async with open_backend_stream(url, headers, body, read_timeout=timeout, on_retry=retry) as response: | ||
| async with open_backend_stream(url, headers, body, read_timeout=timeout, on_retry=retry, | ||
| retry_write_timeout=bool(CONFIG.get("retry_write_timeout"))) as response: |
There was a problem hiding this comment.
[P2] Tag the billing ambiguity of same-credential write-timeout retries
The opt-in now reaches the lower transport retry, but its retry() callback still records connect_retry and logs only 建连失败，重试 1/1. _replay_cost_note() is called only by credential failover. If the first connection raises WriteTimeout and the second succeeds, the operation never reaches those failover log sites, so the potentially charged replay has no billing-risk marker.
Reproduced with retry_write_timeout=true: two POSTs, a successful response, and only the generic connection-retry log. This also occurs with failover_max=0.
Please distinguish the write-timeout retry in this callback and apply the same risk note (or an equivalent structured attempt flag). Add a WriteTimeout -> 200 regression without credential rotation so both retry layers honor the documented observability contract.
There was a problem hiding this comment.
已修（d045bf1）。retry() 回调现在自己带标记，不再只有换凭证那行有：
timeout_on_write = isinstance(error, WRITE_TIMEOUT_TRANSPORT)
observe_attempt("write_timeout_retry" if timeout_on_write else "connect_retry", ...)
_log(f"[{rid}] {'写超时重放' if timeout_on_write else '建连失败'}，重试 1/1 | {model_name}"
f" | {_network_error_text(error)}{_replay_cost_note(error)}")- 结构化标记走
stage：safe_attempt()有键白名单（stage/code/error_code/...），加一个新布尔键会在落库时被丢掉，所以用write_timeout_retry与connect_retry区分两类，审计里一眼能看出是哪一层重放、是否可能已计费； - 只有写超时追加
_replay_cost_note()。建连失败/建连超时上游手里没有正文，加风险标记反而是噪音，这点与换凭证那侧的口径一致； - 日志文案也跟着分开：写超时发生在已经建好的连接上，再说「建连失败」是错的。
新回归 test_same_credential_write_timeout_replay_carries_the_risk_note：retry_write_timeout=True + failover_max=0，第一枪 WriteTimeout、第二枪同一凭证成功。断言 4 件事：上游恰好 2 枪；两枪 x-user-id 相同（没换凭证）；换凭证重放 日志为空（证明这条路径到不了那行）；重放日志含 上游可能已处理该请求，且审计 attempts 里出现 write_timeout_retry。另加 test_connect_retry_stays_untagged 钉住「建连类不加风险标记」，避免以后被顺手全打上。
退回上一提交后第一条按预期失败（只有 建连失败，重试 1/1、无标记、无 stage），第二条仍绿 —— 它守的是不该变的行为。
文档不需要改：docs/advanced.md、docs/advanced.zh-CN.md 和 .env.example 本来就写着「显式打开后两层都生效」「这类重放同样带 上游可能已处理该请求 日志标记」，是代码没兑现，现在对齐了。
Three findings from the re-review. Each comes with a regression that is red on the previous commit and green on this one. - Re-route with the client's model, not the rewritten one. `_route_chat` rewrites `auto` into the site's `default-model` before sending, and the failover loop fed that rewritten body straight back into `_route_chat`. The second round then looked up the policy for `default-model`, so a rule binding `auto` to one credential or one site stopped applying exactly when the gateway starts choosing a new backend. `_routed_stream`/`_routed_fetch` now take the canonical (pre-rewrite) body separately from the per-round wire body, and all three endpoints pass what the client actually asked for. - Do the preflight inside the ASGI lifecycle. It was awaited in the endpoint, i.e. before `StreamingResponse.__call__` installs `listen_for_disconnect`, so across that whole window nobody consumed `http.disconnect`: a stuck first segment plus a client that already left held a `ConcurrencyLimitMiddleware` slot until the 300s read timeout -- at `max_concurrent=1` one stranded client took the whole gateway down. Streaming replies are now a `StreamingResponse` subclass that runs the preflight (and any credential failover) inside `stream_response`, so the disconnect cancels the pending upstream read while the response headers stay deferred. Failures still raise `HTTPException` from inside that call, below `ExceptionMiddleware`, so the status-code contract and the protocol error bodies are unchanged. Getting the cancellation to land is not enough on its own: anyio re-delivers it at every checkpoint, which aborts the generator's own `finally` -- measured directly, the httpx teardown there never finished. Each segment is therefore read in a child task (`_first_segment`), so the frame is never unwound by the outer scope, and the close runs in a shielded scope. Covered at the ASGI level with the real gate layer: disconnect before the first segment, disconnect after streaming started, and "the next request still gets a slot". - Tag the same-credential write replay. `--retry-write-timeout` applies to the connection retry as well, but only the credential-switch log line carried `上游可能已处理该请求`, so a first WriteTimeout followed by a same-credential success was a possibly-billed replay with no marker -- the docs already promised the tag for both layers. The retry callback now separates the two classes, logs the note for write timeouts, and records a `write_timeout_retry` attempt stage so the audit says which kind of replay it was. anyio becomes a declared direct dependency: same 4.14.2 that starlette/httpx already pinned, so the locked hash line is unchanged and only the `via` block moves. 781 passed, 2724 subtests.
Follow-up to d045bf1, which shielded the teardown with `anyio.CancelScope` and declared anyio as a direct dependency. Since the base moved to a pyproject + `uv.lock` regime where `requirements.in` is generated and `tests/test_build_lock.py` pins every transitive to the lock, growing the direct-dependency set for one `finally` is the wrong trade. The guarantee is kept with stdlib only: - anyio re-delivers cancellation to the host task once per event loop cycle (`_deliver_cancellation` re-arms with `call_soon`), which is why a bare `await agen.aclose()`/`await task` in that context never finishes: the fix is not to wait harder, it is to run the teardown in a task that is *not* a member of the cancelled scope. `_first_segment` already moved the generator frame there, so `child.cancel()` is delivered exactly once and the frame's own `finally` (where httpx closes the connection) completes. - `_teardown_finished()` then waits up to `TEARDOWN_GRACE_CYCLES` poll cycles for that task so the common case stays synchronous, and otherwise leaves it to run in the background with its result drained. The bound is cycles, not wall time: under re-cancellation every `await` returns immediately, so a time bound would busy-wait instead. Measured with the same regressions: the ASGI disconnect tests and the cancel-scope teardown test all still pass, repeatedly.
The base moved to `720b433` (PR maiphucgiang#19: scoped model mappings, glass dashboard, per-account check-in, and a pyproject + `uv.lock` dependency regime), which made this branch unmergeable. Everything auto-merged except `requirements.in`, where d045bf1's `anyio` line was already obsolete: f63285c dropped that dependency in favour of a stdlib teardown, so the generated file keeps upstream's content and `requirements.txt` is byte-identical to the base again. 836 passed, 2772 subtests.
|
评审这一轮的 4 条都已处理，分支也跟上 base（
新增 8 条回归，全部按「退回上一提交则红、当前实现则绿」实测过；全量 836 passed / 2772 subtests。逐条细节在原线程里回复。 一条仍然开着的取舍，想听您的意见： |
本地部署（完整控制台 + 用户名密码鉴权 + 双站点路由/自动优先免费 + 站点日限额 + 统计面板等）与上游 f929eda 的合并结果，并把 maiphucgiang#18（流式失败状态码与换凭证重放）同步到 a7df12d。这是升级过程的中间态，下一步合入上游 720b433 与 maiphucgiang#18 的最终版本。
`requirements.txt` is exported by `scripts/export_requirements.py` (`uv export --locked --no-dev --no-emit-project`). With the revert to a stdlib-only teardown, anyio is no longer a direct requirement, so the `# -r requirements.in` attribution under it is dead: direct deps are exported as `# via codebuddy2api`, and anyio's own block already names httpx/starlette/watchfiles. Removing the line makes this file identical to upstream again (no diff), so the PR touches nothing dependency-related.
|
One more cleanup while I'm here: this PR now touches nothing dependency-related.
dc93817 removes it, which makes |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc938172bf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| collected = await fetch(routed, cred, headers, url) | ||
| if tried: | ||
| observe_recovery() # 同上：换凭证后成功的请求不该记成失败 |
There was a problem hiding this comment.
When a non-streaming request first fails over (for example, 429) and the replacement credential returns a completed response whose finish_reason is content_filter but includes a refusal/partial output, _fetch_checked_chat deliberately calls observe_failure("content_filter") and returns the response. Because tried is non-empty, this call immediately clears that newer failure, so AuditMiddleware persists the filtered request as outcome="success" with no error code. Only clear the original retryable failure, or defer recovery until the final response has been confirmed not to be filtered.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Conceded and fixed in 23fcf35 — it reproduces exactly as described, and the aggregated stream has the same ordering bug.
Root cause. Recovery was scoped by sequence position instead of by identity: observe_recovery() cleared whichever error_code happened to be current when it ran. On the non-streaming path _fetch_checked_chat returns normally for a filtered completion — it calls observe_failure("content_filter") and hands the refusal back — so the if tried: observe_recovery() right after it erased a failure the replay had nothing to do with:
[{"stage": "upstream_http", "status_code": 429},
{"stage": "upstream_http", "status_code": 200},
{"stage": "content_filter", "error_code": "content_filter"},
{"stage": "failover_recovered", "code": "content_filter"}] // outcome=success, error_code=nullA plain streamed refusal was never affected: there the filter is observed after the preflight, i.e. after recovery has already run, which is why only the aggregated paths were wrong.
Fix. Recovery is now scoped to the failure that triggered the replay. _Observation keeps a per-request failure_seq that fail() bumps; both failover loops snapshot it when a round fails (observe_failure_seq()) and pass it as observe_recovery(through=…), which clears only while that is still the newest failure. A newer failure stands on its own, so the filtered row keeps error_code=content_filter under HTTP 200 — the same row the identical refusal produces at --failover-max=0. through=None keeps the previous semantics for any caller that has no sequence to name.
Regressions (tests/test_stream_failover.py, all three red on dc93817, green here):
- non-streaming chat:
429→ replay → filtered200keepsoutcome=error+error_code=content_filter, and no longer carries afailover_recoveredmarker; - the same shape through
stream=truewithtools, where the whole aggregate runs inside the preflight; - parity: one filtered request audits identically with and without a replay (
outcome,error_code,status_code), so failover can never move the billing label.
The existing recovery test now also pins the marker's own code (upstream_429), so a recovery cannot be credited with clearing something else again.
Scope stays at three files (converter.py, app/observability.py, that test file) — no dependency files touched. tests/test_stream_failover.py is 67/67, and 136 pass across failover + status-contract + observability + region-routing.
Codex's re-review of dc93817 (P2, converter.py:3054) is right: the recovery marker was cleared by sequence position, not by identity. `observe_recovery()` unset whatever `error_code` happened to be current, so a replay could swallow a failure recorded *after* it. The reachable case is the non-streaming aggregate. `_fetch_checked_chat` returns normally for a filtered completion -- it calls `observe_failure("content_filter")` and hands the refusal back -- and the `if tried: observe_recovery()` right after it then erased that, persisting a blocked request as `outcome=success` with no error code. The `failover_recovered` attempt even named `content_filter` as the failure it had recovered from. The aggregate-inside-preflight stream (a `tools` request on `/v1/chat/completions`) had the same ordering, and a plain streamed refusal did not: there the filter lands after the preflight, so it survived. Recovery is now scoped to the failure that triggered the replay. `_Observation` keeps a per-request `failure_seq`, bumped on every `fail()`; the failover loops snapshot it when a round fails and pass it as `observe_recovery(through=...)`, which only clears while that is still the newest failure. A newer one stands on its own, so the filtered row reads `outcome=error` + `error_code=content_filter` + HTTP 200 -- exactly what the same refusal records with `--failover-max=0`. The default `through=None` keeps the old semantics for any future caller that has no sequence to name. Three regressions in tests/test_stream_failover.py, all red on dc93817: the non-streaming chat row, the aggregated stream row, and a parity case asserting a filtered request audits identically with and without a replay. The existing recovery test now also pins the marker's code (`upstream_429`), so a recovery can never again be credited with clearing something else.
|
Head is now 23fcf35, which answers the latest Codex finding (a replay could clear a failure recorded after it, so a content-filtered response was persisted as Nothing else in the branch changed: the four earlier findings remain addressed by d045bf1, and the PR still touches no dependency files. I also split the next piece of work out of this review rather than growing this one: the non-streaming (aggregated) endpoints never observe |
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
2 participants
Problem
A streaming request that fails before the first byte reaches the client answers
HTTP 200with a single in-banderrorevent. The request is a failure, but the transport says success, so nothing downstream can tell.Concretely, on a rate-limited upstream (
429) withstream=true:200429data: {"error": ...}frame, nochoices, no[DONE]Clients parse per chunk, so each one reads this differently — and all wrong:
choices→ an empty completion.response.completednever arrives → the turn ends silently. The session "hallucinates": the model appears to have answered nothing, the agent stops, and neither the client nor the operator ever sees an error or a retry.Non-streaming requests of the same failure already returned the real status, so the two paths disagreed about what "failed" means, and only the path agents actually use was the lying one.
Root cause
StreamingResponsecommits the response headers as soon as it is iterated, while the upstream call happens inside that generator. Once iteration starts there is no status left to choose, so every upstream outcome —429, connect failure, write timeout, content-filter rejection — had nowhere to go but the SSE body.What changes
_preflight_stream). A failure that happened before any byte went out is now surfaced with the real HTTP status on all three endpoints (/v1/chat/completions,/v1/messages,/v1/responses), so streaming andstream=falseagree. A stream that genuinely breaks after bytes were sent still reports in band — that status can no longer be recalled, and pretending otherwise is what caused this.open_backend_streamnow replayshttpx.WriteTimeoutalongside connect failures. The request body was never accepted, so a fresh connection cannot double-bill. Large cross-border sessions hit the 60 s write timeout far more often than they hit connection setup, and previously each one was a hard failure.--failover-max(default0, off) —CODEBUDDY2API_FAILOVER_MAX, hot-reloadable via CONFIG. Such a failure is retried on another credential within the replay window, so the client still sees exactly one normal response. Excluded credentials thread throughCredentialPool._candidates/pick/headers_forand_cred_for, and routing stays off the event loop (run_in_threadpool).UpstreamHTTPErrorvs a synthesized 502 — an upstream answer carried on an HTTP status is now distinguishable from a 502 the aggregator invented from a 200 body. This is what makes replay billing-safe rather than guesswork.observe_recovery()keeps the audit honest after a saved replay: the failed attempt stays inattemptsplus afailover_recoveredmarker, but the row no longer ends up asoutcome=errorwithstatus_code=200.What is deliberately never replayed
Replaying these either double-bills or wastes quota on a deterministic answer:
is_filter_error) — that is the model's real answer; another account hits the same wall;ReadError/ReadTimeout/WriteError/RemoteProtocolError— the body may have been accepted and processed;400/404/413— deterministic refusals; a different account answers the same way.When no other credential can be picked, the first real status is surfaced instead of degrading it into a generic 503.
Changes to three existing assertions
These pinned the old contract, so they had to move — flagging them explicitly for review:
test_refusal::test_empty_stop_done_errors_on_all_six_paths...— empty-termination is knowable before the first byte, so all six combinations now assert a real502instead of200+ an in-band error frame.test_workbuddy_filter— (a) filter rejections now assert the same status asstream=false; (b) in the "refusal followed by transport error" test, status depends on whether bytes actually went out:/v1/responsesalways aggregates before emitting, so it gets a real502, while genuinely streamingchat/messageshas already sent refusal text and keeps in-band reporting. Theresponse.incompleteprotocol semantics from Align protocol semantics with client contracts #12 are untouched.test_runtime_endpoints::test_credential_selection_runs_off_the_event_loop— it grep-counted exactly 3 pooled_route_chatsites; the two failover paths add more. The invariant it protects (no credential selection on the event loop —direct == []) is still asserted; the count is now a minimum.Tests
tests/test_stream_failover.py(replay windows, excluded codes, credential exclusion, audit recovery) andtests/test_stream_status_contract.py(streaming vs non-streaming must agree, per endpoint and per failure mode).main+ this branch: 762 passed, 1 failed, 2708 subtests. The single failure istest_audit_store::test_normal_write_work_does_not_scale_with_detail_population, a pre-existing timing benchmark — it passes in isolation on this branch and on a cleanmainworktree, so it is unrelated to these changes.Compatibility
--failover-maxdefaults to0, so failover is entirely opt-in and no traffic pattern changes by default. The always-on behaviour change is the intended fix: failures before the first byte stop being reported as200. Docs and.env.exampleupdated in bothdocs/advanced.mdanddocs/advanced.zh-CN.md.Summary by Sourcery
Make pre-first-byte streaming failures report truthful HTTP errors and optionally recover eligible requests by replaying them on alternate credentials.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: