fix(bazel): key execlog reader on client pid; probe with startup flags - #1428
jeffpignataro wants to merge 2 commits into
Conversation
…on the client pid `Build::spawn` asked `bazel info server_pid` with no startup flags, so the probe read whatever rc files were on disk and could land on a different output base than the invocation — or the same one with different options, in which case the real invocation restarted the server and the captured pid was dead before the build started. The execlog reader used that server pid as its liveness signal and gave up at once with "process exited before the file was created", and `wait()` surfaced that stream error in place of bazel's own exit status, so a green build failed the task. - Probe with the invocation's startup flags, here and in the version probes behind version-gated flags (`query` already did this). - Start the execlog reader after `cmd.spawn()` and bound it by the client pid, as the BES reader already does; bazel closes the log before the client exits, so the client's exit is the end-of-stream signal. - In `wait()`, when bazel itself failed, report stream errors as warnings and return bazel's exit code; only a stream failure on a green build fails the call, and the message now carries the exit code. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HTa7Rk2dND4P5AXWZ7VBcV
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Aspect Workflows Tasks📅 Mon Sep 14 17:41:28 UTC 2026 Task Results
Reproduce❌ delivery (delivery-uncacheable · delivery-gha-debug · delivery-gha · delivery-uncacheable-warn)Install ⏱ Last updated Mon Sep 14 18:36:34 UTC 2026 · 📊 GitHub API quota 110/15,000 (1% used, resets in 56m) |
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. |
| pub fn spawn_with_file( | ||
| pid: u32, | ||
| out_path: Option<PathBuf>, | ||
| path: PathBuf, |
There was a problem hiding this comment.
path is a pretty horrible name here
There was a problem hiding this comment.
yeah good call - changed to execlog_path for clarity.
| rt: AsyncRuntime, | ||
| ) -> Result<Build, std::io::Error> { | ||
| let (pid, version) = super::info::server_info()?; | ||
| // Probe with the invocation's own startup flags so the answer describes |
There was a problem hiding this comment.
This seems like a pretty over detailed bloated comment
There was a problem hiding this comment.
Yeah, definitely. Tightened up the verbiage.
|
to me this sounds like we should just forward the correct startup flags that should fix the issue here? |
|
Also relying on the client_pid is not an answer here. if you have bazel/tools wrapper, bazelisk etc the client_pid is never the real bazel client, even when it is, its not the client that writes this file, its the server that does it. |
|
Closing based on #1428 (comment) - I'll try to look for a simpler solution here. |
Problem
A customer's
aspect testjobs started failing after a successful build:aspect ci bazelrc, writing--output_base=/mnt/ephemeral/output/dirt/dirtinto~/.bazelrc.Build::spawnprobedbazel info server_pid releasewith no startup flags. Bazel resolved that from the rc files, so the probe landed on the task's own output base but with rc-derived startup options, and returned that server's pid.bazel testran with the runner's explicit startup flags. Bazel saw different startup options on the running server and restarted it —Starting local Bazel server (9.2.0)three seconds afterbazel health check passed.StreamingFile::openreturnedBrokenPipeimmediately.wait()joined the streams after a green build, hit the stream error andbail!ed, never reporting bazel's exit status.Until that hook change the rc files pointed the probe at a different, long-lived output base (Rosetta's
__main__), so the wrong-but-alive pid masked the bug. It only bites with an exec-log sink registered (the artifacts feature'supload_exec_log).Fix
Build::spawnnow callsserver_info_with_startup_flags(&startup_flags), asqueryalready did. The version probes behind version-gated flags (resolve_flags_for_running_bazel,resolve_rc_version) take the startup flags too, so they no longer read rc files and never start a stray server.--execution_log_compact_filepath is chosen before spawn; the reader thread starts aftercmd.spawn()withchild.id(), mirroring the BES reader. Bazel'sCompactSpawnLogContextopens the file in its constructor and closes it inafterCommand, both before the client exits, so the client's exit is the end-of-stream signal.galvanize::StreamingFilekeys onis_pid_alive(pid)instead of "doespidhold the file open" — the server holds it, and the server may be replaced mid-invocation.wait()collects stream errors instead of bailing on the first. If bazel failed, the stream errors are printed as warnings and bazel's status is returned. If bazel succeeded and a stream failed, the call still fails (a truncated artifact must not pass silently), but the message now carries the exit code.Not changed here:
WorkspaceEventStream::spawn_with_pipeand the BESPipe'sIfOpenForPidpolicy still receive the pre-spawn server pid. With the probe fixed that pid is the right server, so they are correct unless bazel restarts for some other reason; making them client-pid-bounded is a follow-up.Test plan
bazel test //crates/galvanize:galvanize_test— two new tests: the reader gives up when the bounding pid is already dead before the file appears, and it keeps reading past EOF while the pid lives (with no process holding the file open) and ends only when the pid exits.bazel build //crates/axl-runtimebazel test //crates/axl-runtime:testaspect ci bazelrcin a pre-command hook andupload_exec_logon,aspect testcompletes and the.execlog.zstdsink is written.🤖 Generated with Claude Code
https://claude.ai/code/session_01HTa7Rk2dND4P5AXWZ7VBcV