fix(bes): bound the CLI's BES replay buffer by bytes, not event count - #1353
Merged
Conversation
A CLI-streamed BES sink retains unacked events so it can replay them if the stream reconnects. That buffer was capped at 10,000 events and overflow was terminal: the sink stopped uploading mid-build and reported a partial upload. An event count is the wrong bound. BEP event sizes span orders of magnitude — most events are a few hundred bytes, but an action's captured stdout can reach tens of megabytes — so a count caps neither memory nor stream length. A build emitting many small events trips it while consuming little memory; a build emitting a few huge ones stays far under it while consuming a lot. Observed on a fully-cached silo lint run: Bazel emitted ~20.7k analysis events in 65s (7,137 targets, 34,801 aspect applications, --build_event_publish_all_- actions). Acks could not keep 10,000 slots free, the buffer filled, and the sink terminated with 11,615 of 21,616 events uploaded. Nothing was wrong with the build, the network, or the backend. Bound the buffer by bytes (default 256 MiB) and make overflow non-fatal: evict the oldest retained events instead of tearing the stream down. Retention exists only to enable replay, so eviction costs replay coverage for the evicted range and nothing else — delivery of the live stream is unaffected. The server tolerates the resulting sequence gap (it acks by position and readers page forward past missing seqs), and a reconnect that replays an incomplete range now warns instead of failing silently. An event larger than the whole budget is sent but not retained. `retry_max_buffer_size` is renamed to `retry_max_buffer_bytes` on both the `bazel.build_events.grpc()` Starlark surface and the Workflows feature's `--bes-retry-max-buffer-bytes` flag, since the unit changed. Test plan: - 6 new RetryBuffer tests: many small events retained without eviction, oldest-first eviction on overflow, one large event displacing several small, oversized event not retained, prune reclaims bytes. - cargo test --workspace: 597 passed, 0 failed. - All 42 `aspect dev test-*` AXL suites pass. - clippy warning count unchanged vs main (153 → 153). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
✨ Aspect Workflows Tasks📅 Tue Jul 28 04:16:05 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: 0ec147656e
ℹ️ 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
+167
to
+170
| if size > self.cap_bytes { | ||
| self.evict_all(); | ||
| self.evicted += 1; | ||
| return; |
There was a problem hiding this comment.
When retry_max_buffer_bytes is configured below a single BES request size, this branch evicts the existing buffer and returns without retaining the oversized request. The gRPC state machine uses state.buffer.is_empty() to decide that there are no outstanding acks, including the half-close path that returns Done when the buffer is empty, so an oversized last_message sent to a backend that never acks will be reported as a successful upload after half_close_timeout instead of surfacing a retry/loss condition. Please keep separate outstanding-ack accounting, or otherwise avoid treating a non-retained sent event as already acked.
Useful? React with 👍 / 👎.
The right buffer budget is a property of the machine, not of the build: a memory-constrained runner may want less than 256 MiB, and a fleet that streams unusually large events may want more. Threading a flag through every task definition to express that is awkward, so read it from the environment. `ASPECT_BES_RETRY_MAX_BUFFER_BYTES` accepts a plain byte count or a suffixed size (`512MB`, `1GiB`; all binary multiples, case-insensitive). It sets the *default*, so an explicit `retry_max_buffer_bytes` on `bazel.build_events.grpc()` still wins — precedence is explicit arg > env var > built-in 256 MiB. Resolution is cached per-process so two sinks in one build cannot disagree, and a malformed or zero value warns and falls back rather than failing: BES upload is best-effort, and a typo'd tuning knob should not lose a CI run. The Workflows `--bes-retry-max-buffer-bytes` flag now defaults to 0 meaning "unset" and is omitted from the `grpc()` call in that case; previously its hardcoded default would have silently outranked the environment. Test plan: - New tests: byte-size parsing (plain, all suffixes, case, whitespace) and its rejections (no leading number, unknown unit, fractional, negative, overflow); override resolution for valid / absent / invalid values, asserting the fallback warns and names the variable. - New Starlark test that omitting the knob validates — that is the path which now consults the environment. - cargo test --workspace: 605 passed, 0 failed. - All 42 `aspect dev test-*` AXL suites pass. - clippy warning count unchanged vs main (153). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ER_BYTES The `CLI_` infix marks which uploader this tunes. Bazel has its own BES uploader driven by `--bes_backend`, with its own buffering that this setting does not affect; the runner also already exports `ASPECT_WORKFLOWS_BES_BACKEND` and `ASPECT_WORKFLOWS_BES_RESULTS_URL`, so an unqualified `ASPECT_BES_*` would read as applying to BES generally rather than to the CLI-streamed sink alone. Also adds a test that reads the variable through the real environment under its documented name. The existing tests cover the fallback rules but pass a string directly to the resolver, so they would not catch a misspelled constant or a `default_retry_max_buffer_bytes` that stopped consulting the environment. Test plan: - cargo test --workspace: 604 passed, 0 failed. - All 42 `aspect dev test-*` AXL suites pass. - clippy warning count unchanged vs main (153). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review pass over the byte-bounded buffer. Bug: the eviction warning fired on every reconnect. `drive_stream` runs once per attempt and read a cumulative counter, so a build that evicted once and then reconnected three times warned three times, each with a growing count and each claiming the events "could not be replayed after reconnecting" — including ones a previous warning had already reported. `evicted()` becomes `take_evicted()`, draining the counter so each reconnect reports only what was lost since the last one. The warning text also overstated the loss: an evicted event was streamed, it just cannot be re-sent. Reworded, and it now names the environment variable so the message carries its own remedy. Other cleanups: - `parse_byte_size` and `parse_duration` shared a number/suffix split; extracted `split_scalar_unit`. `parse_duration` picks up case-insensitivity and internal whitespace as a side effect, both now covered by tests. - `push` no longer needs a separate `evict_all` path or an unreachable match arm; one `evict_oldest` helper drives both cases. - Dropped `#[allow(dead_code)]` from `len()` (used by six debug-log sites) and `bytes()` (now used by one). - `drive_stream`'s entry log reports buffered bytes alongside the event count — bytes are the bound, so a user tuning the budget can see the headroom. - Trimmed doc comments that argued for the design rather than describing the code, and repaired a module docstring paragraph split mid-sentence. New tests: `take_evicted` drain semantics, partial prune reclaiming budget proportionally, and an acked stream pushing 100 events through a 4-event budget without evicting. Test plan: - cargo test --workspace: 606 passed, 0 failed. - All 42 `aspect dev test-*` AXL suites pass. - cargo fmt --check clean; clippy unchanged vs main (153, none in changed files). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
cristifalcas
added a commit
that referenced
this pull request
Aug 8, 2026
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>
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
1 participant
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.
The CLI's BES sink retains unacked events so it can replay them if the stream reconnects. That buffer was capped at 10,000 events, and exceeding it was terminal — the sink stopped uploading mid-build and reported a partial upload.
An event count is the wrong bound. BEP event sizes span orders of magnitude: most are a few hundred bytes, but one carrying an action's stdout can reach tens of megabytes. A count bounds neither memory nor stream length, so it trips on builds that are using very little memory while failing to protect against the ones that aren't.
A fully-cached run hit exactly that. Bazel emitted ~20.7k analysis events in 65s (7,137 targets, 34,801 aspect applications,
--build_event_publish_all_actions); acks could not keep 10,000 slots free, and the sink gave up withUploaded 11615 of 21616 build events … before the stream failed. Nothing was wrong with the build, the network, or the backend —ASPECT_DEBUG=1showed a singledrive_streamcall atattempt=0, so no disconnect and no retry, just the cap.Changes
Bound the buffer by bytes (default 256 MiB), tracked incrementally via
encoded_len().Make overflow non-fatal. Retention exists only to enable replay, so exceeding the budget evicts the oldest entries instead of ending the stream. The cost is that a later reconnect cannot replay the evicted range; the server tolerates the resulting sequence gap, acking by position and paging forward past missing seqs. An event larger than the whole budget is sent but not retained. When a reconnect does find evicted events, the sink warns — the only signal that data may be incomplete, since BES upload never fails the build.
Allow per-runner configuration via
ASPECT_CLI_BES_RETRY_MAX_BUFFER_BYTES, which accepts a plain byte count or a size suffix (512MB,1GiB; binary multiples, case-insensitive):export ASPECT_CLI_BES_RETRY_MAX_BUFFER_BYTES=512MBPrecedence is explicit
retry_max_buffer_bytesargument → environment variable → 256 MiB. Resolution is cached per process so two sinks in one build cannot disagree, and a malformed or zero value warns and falls back rather than failing the build. TheCLIin the name distinguishes this from Bazel's own BES uploader (--bes_backend), whose buffering it does not affect.Notes for reviewers
ivy-bep-etlvalidates onlysequence_number >= 1; it does not require contiguity.Changes are visible to end-users: yes
Because the unit changed,
retry_max_buffer_sizeis renamed toretry_max_buffer_bytesonbazel.build_events.grpc(), and--bes-retry-max-buffer-sizeto--bes-retry-max-buffer-byteson the Workflows feature. The old names are errors rather than silent misinterpretations — 10000 bytes would be a pathologically small budget.Suggested release notes
ASPECT_CLI_BES_RETRY_MAX_BUFFER_BYTES(accepts512MB,1GiB, or a plain byte count).bazel.build_events.grpc(retry_max_buffer_size = …)is nowretry_max_buffer_bytes, and--bes-retry-max-buffer-sizeis now--bes-retry-max-buffer-bytes. The value is a byte budget, not an event count.Test plan
RetryBuffer: 20k small events retained without eviction, oldest-first eviction on overflow, one large event displacing several small ones, oversized events not retained,take_evicteddraining so consecutive reconnects don't re-report the same losses, partial prune reclaiming budget proportionally, and an acked stream pushing 100 events through a 4-event budget without evicting.Configuration: byte-size parsing across suffixes, case, and whitespace plus its rejections (no leading number, unknown unit, fractional, negative, overflow); override resolution for valid, absent, and invalid values; and a test that reads the variable from the real environment under its documented name.
Starlark surface: an explicit value validates,
0is rejected, and omitting the argument — the path that consults the environment — validates.cargo test --workspace— 606 passed, 0 failedaspect dev test-*AXL suites passcargo fmt --checkclean;cargo clippy -p axl-runtimeunchanged vsmain(153 warnings, none in changed files)