fix(bes): backpressure the sink instead of dying when acks fall behind - #1344
Open
cristifalcas wants to merge 3 commits into
Open
fix(bes): backpressure the sink instead of dying when acks fall behind#1344cristifalcas wants to merge 3 commits into
cristifalcas wants to merge 3 commits into
Conversation
A large build (`//workflows/...` in silo: ~66k events, since the CLI always
passes `--build_event_publish_all_actions`) outruns the backend's acks. The
replay buffer hits its 10,000-entry cap, `RetryBuffer::push` returns
`BufferOverflow`, and `BufferFull` is terminal — the sink exits mid-stream,
so `BuildFinished` never ships and the backend leaves the invocation running
forever. Observed against a dev Workflows deployment:
drive_stream returned after 35.1s: outcome=BufferFull(
retry buffer overflowed (cap=10000) while attempting to buffer seq 51705)
Treat the cap as a flow-control window rather than a cliff: gate the
event-receive arm on `!buffer.is_full()`, so a full buffer stops pulling from
`event_rx` and the loop only reads acks until they drain it. Events wait on
the unbounded `event_rx` instead of being lost. A server that stops acking
altogether is still bounded by the existing ack-progress watchdog, so this
cannot wedge the sink. The `BufferFull` arm stays as a backstop.
Also warn on `UpstreamClosed`. It means the reader closed without Bazel's
final event, which leaves the invocation in progress on the backend, but it
was reported as a clean shutdown with no user-visible signal at all.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
✨ Aspect Workflows Tasks📅 Sat Jul 25 14:16:49 UTC 2026 ❌ 1 failed task
|
…e window Review follow-ups on the backpressure change. Half-close held a flat 30s to drain whatever was outstanding, with the ack-progress watchdog explicitly disabled for the duration. That was safe when reaching the cap was terminal; now a build that ends while backpressured half-closes holding a full buffer, and a backend acking steadily but slower than the deadline gets its stream torn down and fully replayed — converging on the same give-up this change set out to remove. The deadline is a budget for silence, not for how long a drain may take, so push it out on every ack. Draining to empty already exits, so only a server that has actually gone quiet spends it, and the existing 30s bound against a silent backend is unchanged. Replay now drains acks as it goes. Reconnecting with a full buffer writes thousands of events while never reading the response stream, which lets the server's connection window fill until it stops reading requests and the replay trips `send_stall_timeout` instead. Acks are collected during the loop and applied once it releases its borrow; the few extra replayed events that costs are deduped by sequence number server-side. Tests: `events_beyond_buffer_cap_backpressure_rather_than_overflow` drives 50 events through a 4-entry buffer against an ack-delayed backend — `BufferFull` before the fix, `Done` after. `half_close_drain_survives_a_backend_slower_than_ the_deadline` covers the deadline reset the same way. Both verified to fail with their fix reverted; the previous test only exercised `is_full`'s arithmetic, which no amount of gating mistakes would have broken. Docs: three places still described the cap as terminal on overflow, including the `bes_retry_max_buffer_size` flag description users configure Workflows with. Reworded to the window semantics, and dropped the "shrink on memory-constrained runners" guidance — the cap throttles the wire and relocates the backlog onto the sink's input queue, so shrinking it does not bound memory. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
cristifalcas
commented
Jul 25, 2026
| // rather than let the build look clean and hang in the UI. A | ||
| // build that emitted nothing (Bazel failed before the BEP | ||
| // opened) is already reported by the task itself. | ||
| DriveOutcome::UpstreamClosed => { |
Contributor
Author
There was a problem hiding this comment.
we've lost bazel input
Removes `ServerMode::AckDelayed` and the two tests added in the previous commit — `events_beyond_buffer_cap_backpressure_rather_than_overflow` and `half_close_drain_survives_a_backend_slower_than_the_deadline`. Both were written against LLM review feedback rather than a maintainer's, so whether this behaviour is worth a timing-sensitive fake-backend test is a call for the people who own this suite, not one to make on their behalf. The production changes stand on their own; test coverage can be added back if reviewers want it. `buffer_is_full_gates_at_cap_and_reopens_on_ack` in retry.rs stays: it is not timing-dependent, and without it `RetryBuffer::is_full` has no coverage at all. For the record, both removed tests did fail with their fix reverted — `BufferFull(cap=4, seq 5)` and `half-close deadline (60ms) elapsed with 6 events unacked` respectively — so re-adding them is a paste, not a rewrite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
cristifalcas
marked this pull request as ready for review
July 25, 2026 15:47
![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".
Comment on lines
+771
to
+772
| 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 👍 / 👎.
Comment on lines
+912
to
+914
| 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 👍 / 👎.
Member
|
This has some conflicts. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes are visible to end-users: yes
Have the relevant documentation and comments been updated: yes
Breaking change (forces users to change their own code or config): no
The changes in this PR will be included in the release notes: yes
Problem
A large build outruns the backend's acks. The CLI always passes
--build_event_publish_all_actions, so//workflows/...insiloemits ~66k events. The replay buffer hits its 10,000-entry cap,RetryBuffer::pushreturnsBufferOverflow, andDriveOutcome::BufferFullis terminal — the sink exits mid-stream.BuildFinishednever ships, so the backend leaves the invocationin_progressindefinitely.Reproduced against a dev Workflows deployment (
ASPECT_DEBUG=1 aspect build --aspect-remote-cache --aspect-bes-backend -- //workflows/...):That run was not cancelled — the overflow hit at 35s on an ordinary build. About 15,000 of 66,539 events reached the backend;
BuildFinishedsits around event 63,800, so it never arrived. TheWARNING: … giving up, build events were not deliveredthis prints is line 3,257 of a 3,300-line build log, among dozens of unrelated Bazel warnings.Fix
Backpressure. Treat the cap as a flow-control window rather than a cliff.
drive_stream's event-receive arm is gated on!state.buffer.is_full(): at capacity the loop stops pulling fromevent_rxand only reads acks until they drain the buffer. Events wait on the unboundedevent_rxinstead of being dropped. This cannot wedge the sink — a server that stops acking altogether is still caught by the existing ack-progress watchdog, which tears the stream down for a retry exactly as before. TheBufferFullarm is kept as a backstop.Half-close deadline resets on ack progress. A build that ends while backpressured half-closes holding a full buffer, and the ack-progress watchdog is disabled once half-closed. A flat 30s to drain would tear down a stream that is acking fine, just not fast enough — reconnecting and replaying the whole buffer, converging on the same give-up. The deadline is a budget for silence, not for how long a drain may take, so each ack pushes it out. The 30s bound against a genuinely silent backend is unchanged.
Replay drains acks as it goes. Reconnecting with a full buffer writes thousands of events without ever reading the response stream, letting the server's connection window fill until it stops reading requests and the replay trips
send_stall_timeout. Acks are collected during the loop and applied once it releases its borrow of the buffer; the few redundant replayed events that costs are deduped by sequence number server-side.Warning on
UpstreamClosed. The reader closing without Bazel's final event leaves the invocation in progress on the backend, but was treated as a clean shutdown with no user-visible signal.Test coverage — reviewers' call
buffer_is_full_gates_at_cap_and_reopens_on_ackcoversRetryBuffer::is_full, the window semantics the select-arm guard depends on. The three behavioural changes are not covered.I had written two tests against a new
ServerMode::AckDelayed(Duration)fake-backend mode — one driving 50 events through a 4-entry buffer, one covering the half-close reset — and verified both fail with their fix reverted (BufferFull(cap=4, seq 5)andhalf-close deadline (60ms) elapsed with 6 events unacked). They were removed in 8d16622 because they came out of an LLM review rather than a maintainer's, and whether timing-sensitive fake-backend tests belong in this suite is a call for the people who own it. The commit records what they were; re-adding is a paste. Say the word and they come back.Worth knowing if you want them: the half-close one is the timing-sensitive of the pair (20ms ack cadence against a 60ms deadline, ~3× margin), and would be worth widening before it lands.
Test plan
bazel test //crates/...is green.buffer_is_full_gates_at_cap_and_reopens_on_ackonly; see above.aspect build --aspect-remote-cache --aspect-bes-backend -- //workflows/...against a Workflows deployment and watchASPECT_DEBUG=1output foroutcome=BufferFull. Before: the sink exits mid-stream and the invocation is stuck "Running" in the UI. After: reads pause at the cap and the stream completes.🤖 Generated with Claude Code