fix(bes): stream an invocation once when bazel and the CLI share a backend - #1349
Merged
Conversation
…s to When a CLI-streamed BES backend names the same endpoint as Bazel's own `--bes_backend`, both upload the same invocation: the backend records a duplicate, and the two streams race to close — losing that race is how the CLI-side stream ends up truncated. Drop the CLI sink in that case. Bazel wins the tie because it is the more explicit request: the CLI's backends are auto-wired (a Workflows runner's `ASPECT_WORKFLOWS_BES_BACKEND`, a `--deployment`'s advertised BES host) while `--bes_backend` is set by hand. Each drop prints an INFO line so the suppression is visible. Bazel's effective backend is read through the run command, so it is caught wherever it came from — `--bazel-flag`, a `.bazelrc`, or an expanded `--config`. Endpoints are matched on `host:port` with the scheme's default port made explicit, so `grpcs://h`, `grpcs://h:443`, and `h:443` all compare equal. Scheme is ignored at equal ports, which is required rather than merely lenient: the runtime rewrites `grpcs://` to `https://` on the sink, so the runner's sink URI and the user's flag never share one. `grpc://h` and `grpcs://h` still differ, their default ports being 80 and 443. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![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: e238ec1005
ℹ️ 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".
✨ Aspect Workflows Tasks📅 Tue Jul 28 01:12:43 UTC 2026 ❌ 1 failed task
|
…pressing Addresses two review findings on the duplicate-stream suppression. Suppressing the CLI's sink to the Aspect backend left the build with no gRPC sink at all, so `Build::spawn` minted no `sink_invocation_id` and the Aspect Web UI link vanished from every status surface — for exactly the users who wired BES up twice. The backend does still hold the invocation in that case, indexed under Bazel's own UUID because Bazel is the uploader, so `resolve_aspect_url` now falls back to `data["bazel"]["invocation_id"]` when `bes_streamed_by_bazel` is set. The flag is gated on the suppressed backend being the runner's own `ASPECT_WORKFLOWS_BES_BACKEND`: dropping a third-party BES says nothing about what the Aspect UI has. Like `sink_invocation_id`, it survives the `build_started` reset and a retry's `init_data()`. Endpoint normalization also treated a schemeless URI as having no default port, so `--bes_backend=bes.example.com` missed a `grpcs://bes.example.com` sink and both streamed. Bazel documents these as `[SCHEME://]HOST[:PORT]` and assumes grpcs when the scheme is omitted, so the default now applies there too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`bazel.build_events.grpc` rewrote `grpcs://` to `https://` before storing the
URI on the sink, but that string is also what `sink.uri` returns and what
every user-facing line names. On a runner with
ASPECT_WORKFLOWS_BES_BACKEND=grpcs://remote.silo-aws.aspect.build
the CLI reported
INFO: Streaming build events to https://remote.silo-aws.aspect.build.
naming an endpoint the user never configured. The rewrite exists because
tonic's `Channel::from_shared` only understands http/https, so it is a
transport concern; it now lives in `build_event_stream::client`, at the one
point the string becomes a channel. Everything upstream — `sink.uri`, the
announce and summary lines, the `BES sink <endpoint>` debug/warning prefixes —
keeps the spelling the user wrote.
`grpc://` is mapped to `http://` for the same reason, which it previously was
not: it reached tonic unmapped.
Verified against a live TLS backend that `grpcs://` still negotiates TLS and
the server acks (`build_enqueued ack'd in 115ms`) while the logs read
`grpcs://`, and against a local listener that `grpc://` still opens a plaintext
connection.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… task Follow-up cleanup to the duplicate-stream fix; no behavior change. Each of the six bazel-spawning tasks had grown the same four-line ritual — resolve Bazel's backend, filter two sink groups through two different helpers, set a data key, repeat the same comment. That is now `collect_bes_sinks(ctx, bazel_trait, rc)` plus `bes_streamed_by_bazel(ctx, rc)`, and the two filter helpers collapse into one `_drop_bazel_streamed` taking a `uri_of` accessor, since they differed only in how they read a URI. `endpoint_host` and `endpoint_authority` were parsing URIs separately; both now go through `_split_endpoint`, which incidentally fixes `endpoint_host` not trimming surrounding whitespace. `_DEFAULT_SCHEME` and `SCHEME_ALIASES` replace the schemes that were spelled inline. `process_event`'s build_started reset now restores `_TASK_OWNED_KEYS` rather than naming each preserved field twice, so a future task-owned key does not have to remember to add itself in two places. Tests exercise the public entry points instead of the now-private helpers, and cover `collect_bes_sinks` directly — including the all-sinks-suppressed case that costs the Web UI its sink id. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
gregmagolan
changed the title
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.
Configuring one BES endpoint on both upload paths — Bazel's
--bes_backendand the CLI's own sink — makes both stream the same invocation. The backend records a duplicate, and the two streams race to close; the CLI-side one typically loses and shows up truncated. A customer hit this on a Workflows runner by setting--bes_backendto the URL the runner already injects asASPECT_WORKFLOWS_BES_BACKEND.The CLI now drops its sink to an endpoint Bazel is already uploading to, so the invocation is streamed once. Bazel wins the tie because it's the more explicit request: the CLI's backends are auto-wired (the runner's env var, a
--deployment's advertised host) while--bes_backendis set by hand. Each drop prints anINFO:line. Both sink groups are covered — those built from--bes-backend/--deployment, and those features push ontoBazelTrait.build_event_sinks(the runner's env-injected one, the customer's actual path). A file sink is a local BEP dump, not an upload, so it always survives.Bazel's effective backend is read through the run command, catching it from
--bazel-flag, a.bazelrc, or an expanded--config. Endpoints match onhost:portwith the scheme's default port made explicit, sogrpcs://h,grpcs://h:443,h, andh:443are one endpoint (Bazel documents these as[SCHEME://]HOST[:PORT]and assumesgrpcswhen the scheme is omitted). Scheme is ignored at equal ports;grpc://handgrpcs://hstill differ, their defaults being 80 and 443.Suppression can leave a build with no gRPC sink at all, and the id the Aspect Web UI keys on is only minted when one exists — so a naive drop would delete the "✨ Aspect Workflows" link for exactly these users. The backend still holds the invocation, indexed under Bazel's UUID because Bazel uploaded it, so
resolve_aspect_urlfalls back todata["bazel"]["invocation_id"]when the suppressed backend was the runner's ownASPECT_WORKFLOWS_BES_BACKEND. Dropping a third-party BES says nothing about what the Aspect UI holds, so the link stays suppressed there.Endpoints are also now reported as configured.
bazel.build_events.grpcrewrotegrpcs://tohttps://before storing the URI on the sink, but that string is also whatsink.urireturns and what every user-facing line names, so a runner set togrpcs://remote.foobar.aspect.buildloggedStreaming build events to https://remote.foobar.aspect.build. The rewrite is a transport concern — tonic'sChannel::from_sharedonly understands http/https — so it moved tobuild_event_stream::client, at the one point the string becomes a channel.grpc://maps tohttp://for the same reason, which it previously did not.Changes are visible to end-users: yes
Suggested release notes
--bes_backend. Configuring the same endpoint on both paths (for example, setting--bes_backendto the URL a Workflows runner injects asASPECT_WORKFLOWS_BES_BACKEND) previously produced two streams for one invocation, recorded a duplicate on the backend, and could leave the CLI-side stream truncated. Matching accounts for default ports and an omitted scheme, sogrpcs://bes.example.com,grpcs://bes.example.com:443, andbes.example.comare recognized as one endpoint. The Aspect Web UI link is preserved, and anINFO:line reports each suppressed stream.grpcs://backend was previously logged ashttps://— naming an endpoint that was never set — because the scheme mapping tonic needs was applied to the stored URI rather than only at dial time.Test plan
bes_sinks_test.axlcovers sink collection, duplicate dropping across both sink groups (including when every gRPC sink is suppressed), and the Aspect-backend gate;aspect_endpoint_auth_test.axlcovers authority normalization and endpoint equivalence (default ports in both spellings, omitted scheme, differing ports, empty inputs);bazel_results_test.axlcovers the Web UI link fallback, its third-party negative case, sink-id precedence, and survival of thebuild_startedreset;client.rscovers the scheme mapping, http pass-through, leading-scheme-only rewriting, and that every spelling survivesChannel::from_shared.aspect dev test-*suites and 414 Rust tests acrossaspect-cli,axl-runtime, andbuild-event-streampass.🤖 Generated with Claude Code