fix(bes): keep the stream alive through long replays and slow drains - #1344
fix(bes): keep the stream alive through long replays and slow drains#1344cristifalcas wants to merge 1 commit into
Conversation
✨ Aspect Workflows Tasks📅 Sat Jul 25 14:16:49 UTC 2026 ❌ 1 failed task
|
![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: 8d16622162
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| while let Some(Some(Ok(resp))) = response_stream.next().now_or_never() { | ||
| replayed_acks.push(resp.sequence_number); |
There was a problem hiding this comment.
When the backend returns an error while buffered events are being replayed, this while let polls and consumes the ready Some(Err(status)) but retains only Some(Ok(...)). The normal response handler therefore never sees or classifies the status; for example, a non-retryable PERMISSION_DENIED is discarded and the subsequent EOF is reported as a retryable premature close, causing unnecessary reconnects and hiding the actual backend failure. Match all ready stream outcomes here and return the corresponding fatal or transient result instead of dropping non-ack items.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, and it is ours — that drain loop is new in this PR.
now_or_never() resolves the future, so a ready Some(Some(Err(status))) is consumed and then dropped by the failed pattern match. The stream is finished after that, the main loop reads None, and the existing handling classifies it as a premature close. A non-retryable status would be retried four times and never reported.
Fix follows the shape already there: stash the outcome like replay_failure does — Err classified through is_retryable, Some(None) treated as a closed stream — break out, and act on it once the state.buffer borrow is released.
| if half_close_deadline.is_some() && !state.buffer.is_empty() { | ||
| half_close_deadline = | ||
| Some(tokio::time::Instant::now() + retry.half_close_timeout); |
There was a problem hiding this comment.
When a reconnect leaves duplicate replay acknowledgements queued, or a backend repeatedly sends an old/out-of-order sequence number, record_ack makes no progress but this still pushes out the half-close deadline. A slow series of stale acknowledgements can therefore extend the drain far beyond half_close_timeout, and a backend continuously repeating one acknowledgement can prevent sink.wait() from ever completing despite the documented bound. Compare the acknowledged sequence or buffer length before and after record_ack and reset the deadline only when outstanding events were actually pruned.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, and also introduced here.
Duplicate or out-of-order acks prune nothing, so !state.buffer.is_empty() still holds and each one buys another full half_close_timeout. With bes_timeout defaulting to 0s there is no outer bound, so a backend repeating a single ack keeps sink.wait() alive indefinitely — precisely what half_close_timeout exists to prevent. It needs a misbehaving backend to trigger, but it punches a hole in a bound documented as absolute.
Fix: snapshot max_acked around record_ack and push the deadline out only when it advances.
Worth noting the ack_deadline assignment directly above resets unconditionally in the same way and predates this PR. Milder consequence, same weakness — it should probably get the same treatment while we are in here.
|
This has some conflicts. |
8d16622 to
e3cf514
Compare
A reconnect replays every retained event before resuming the live stream. That loop never reads the response side, so on a large buffer the server's flow-control window fills, it stops reading requests, and the replay trips `send_stall_timeout` — reconnecting into the same wall each time. Acks are now drained as they arrive and applied once the loop releases its borrow of the buffer; the handful of extra replayed events that costs are deduped by sequence number server-side. Separately, half-close held a flat 30s to drain whatever was outstanding. That deadline is a budget for *silence*, not for how long a drain may take: a build that ends holding a large unacked backlog, against a backend acking steadily but slower than 30s, gets its stream torn down and fully replayed at the end of the build. It is now pushed out on every ack. Draining to empty already exits, so only a backend that has actually gone quiet spends it, and the 30s bound against a silent one is unchanged. Both paths got more exposed with #1353, which replaced the 10,000-event cap with a 256 MiB byte budget — the replay these guard is now up to two orders of magnitude larger. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
c44a27e to
072de45
Compare
cristifalcas
changed the title
2 participants
Two ways the BES sink tears down a stream that is working fine.
Replay deadlocks itself. A reconnect replays the retained buffer without
reading the response side, so the server's flow-control window fills, it stops
reading requests, and the replay trips
send_stall_timeout— then reconnectsinto the same wall. Acks are now read as they land.
Half-close mistakes slow for silent. The 30s post-half-close deadline was a
flat budget for the whole drain, so a build ending with a large backlog gets
torn down and fully replayed at the end, even though the backend is acking
steadily. The deadline now resets on each ack, bounding silence rather than
drain time.
Changes are visible to end-users: no
Test plan
cargo test -p axl-runtime— 377 passed.