fix(axl): don't orphan the bazel client when a spawn fails partway - #1381
Open
cristifalcas wants to merge 1 commit into
Open
fix(axl): don't orphan the bazel client when a spawn fails partway#1381cristifalcas wants to merge 1 commit into
cristifalcas wants to merge 1 commit into
Conversation
cristifalcas
marked this pull request as ready for review
August 8, 2026 14:36
![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: de6060d82d
ℹ️ 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".
cristifalcas
added a commit
that referenced
this pull request
Aug 8, 2026
`Drop for Build` reaped the child but left the `LiveBazelGuard` to fall out in field order — after `build_event_stream`, `workspace_event_stream` and `execlog_stream`. Those join their reader threads, so for that whole window a reaped and therefore reusable PID stayed in `live_pids()`, and a CI cancel landing there would signal whatever now owned it. That is the hazard the `live_guard` field is a `RefCell<Option<_>>` for, and `wait()` / `try_wait()` both take it the moment they observe the exit. Do the same on the drop path. Applied to `SpawnedClient` too — its window is a few instructions rather than three stream joins, but one rule beats reasoning about which window is small enough. Reported by the Codex reviewer on #1381. Not covered by a test: what matters is ordering *within* the drop sequence, and observing that from outside needs instrumentation out of proportion to the fix. Also trims the comments this PR added to what cannot be inferred from the code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`Build::spawn` creates the client with `cmd.spawn()` and then does fallible work after it — `BuildEventStream::spawn(...)?`, binding iterator handles, configuring sinks. Every one of those `?` returns dropped a bare `std::process::Child`, which by design does not kill on drop, so the client kept running and was reparented to init. The `LiveBazelGuard` dropped on the same path, which makes it worse rather than better: the PID leaves the live registry, so the OS-signal handler that exists precisely to stop clients outliving aspect-cli can no longer reach it. `live.rs` documents this as its own reason for existing — an orphaned client holds the JVM-server lock and the next invocation on that runner hangs at "Running Bazel server needs to be killed". The reachable trigger is a `build_events` list carrying an already-bound `iterator()` handle: `iter.bind(...)` fails after the spawn. That is `iterator_handle_rejects_reuse`, which leaked one `basil` per run — found while running the suite for unrelated work, confirmed on two worktrees. `SpawnedClient` owns the child between `cmd.spawn()` and a constructed `Build`, terminating it on drop; `defuse()` hands the child and guard over on success. `Drop for Build` covers the same defect one level up, where AXL starts a build and then returns or fails without calling `wait()`. Note it fires at Starlark heap teardown, which spans the whole AXL run — enough to keep a client from outliving aspect-cli, but it does not shorten the in-run server-lock window. Both route through one helper: SIGINT so bazel can release the server lock, a 200ms grace, then SIGKILL. The trailing `wait()` is what reaps; killing alone leaves a zombie. The grace is skipped when the signal never went out — a failed `kill(2)`, or the non-unix stub that can never send one. Both drop paths take the `LiveBazelGuard` the moment the child is reaped rather than letting it fall out in field order. `live_guard` is declared after the three streams, so field order would leave a reaped and therefore reusable PID in `live_pids()` while they join their reader threads, and a CI cancel landing there would signal whatever now owned it. `wait()` and `try_wait()` already take the guard on observing the exit for exactly this reason. Tests assert the guard rather than the AXL path: snapshotting the process-wide `live_pids()` would race every other test in the binary. `is_pid_running` uses `kill(pid, 0)`, which still succeeds on an unreaped zombie, so the reap assertion fails if the trailing `wait()` is removed. The unregister ordering is not covered — what matters is sequencing within the drop, and observing that from outside needs instrumentation out of proportion to the fix. axl-runtime 380 passed / 0 failed, aspect-cli 74 passed / 0 failed, and a full run now leaves no orphaned basil. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
cristifalcas
force-pushed
the
cf/fix-bazel-child-leak
branch
from
August 8, 2026 15:08
378ffd4 to
7fc9c84
Compare
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.
Summary
Prevent AXL from leaving Bazel clients running when build setup fails or a
Buildis abandoned without callingwait():Buildis fully constructed, terminating and reaping them if setup returns an error.visible to end-users: yes
docs: no
breaking change: no
release notes: yes