fix(axl): don't orphan the bazel client when a spawn fails partway - #1381
fix(axl): don't orphan the bazel client when a spawn fails partway#1381cristifalcas wants to merge 1 commit into
Conversation
![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".
| if let Ok(mut child) = self.child.try_borrow_mut() { | ||
| terminate_abandoned_client(&mut child); |
There was a problem hiding this comment.
When an abandoned Build reaches this Drop path, terminate_abandoned_client can reap the child, but the LiveBazelGuard remains in live_pids() until field drop, after build_event_stream, workspace_event_stream, and execlog_stream are dropped. If a shutdown signal arrives in that interval after the OS reuses the reaped PID, signal_all_for_shutdown can still SIGINT the unrelated process; the normal wait()/try_wait() paths avoid this by taking live_guard immediately after reaping. Please make this path unregister the PID immediately once the child has been observed/reaped.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and fixed in 378ffd4.
You're right, and the codebase states the invariant explicitly. live_guard is declared after build_event_stream, workspace_event_stream and execlog_stream, so field-order drop left a reaped — and therefore reusable — PID in live_pids() while those three joined their reader threads. That is what the field's RefCell<Option<_>> wrapper exists for, and what wait() / try_wait() guard by taking the guard the moment they observe the exit.
Drop for Build now does the same, immediately after terminate_abandoned_client returns.
Also applied to SpawnedClient::drop. Its window is a few instructions rather than three stream joins, so far less exposed, but one rule beats reasoning about which window is small enough to ignore.
One thing worth stating plainly: this is 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. It is verified by inspection against the pattern wait() already follows.
`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>
378ffd4 to
7fc9c84
Compare
1 participant
Three ways a bazel client outlives the invocation that started it.
The spawn window drops a live child.
Build::spawncreates the client, thendoes fallible work after it — starting the BES reader, binding iterator handles,
configuring sinks. Every
?in that stretch dropped a barestd::process::Child,which by design does not kill on drop, so the client kept running and was
reparented to init. The
LiveBazelGuardwent with it, taking the PID out of theregistry, so the signal handler that exists to stop exactly this could no longer
see it.
live.rsnames the consequence: an orphan holds the JVM-server lock andthe next invocation on that runner hangs at "Running Bazel server needs to be
killed".
An abandoned
Buildleaks the same way. AXL that starts a build and thenreturns or fails without calling
wait()owns a running client with nothing leftto reap it.
A reaped PID stayed registered while the streams drained.
live_guardisdeclared after the three event streams, so field-order drop left a reaped — and
therefore reusable — PID in
live_pids()while those joined their readerthreads. A CI cancel landing there would signal whatever now owned the PID.
wait()andtry_wait()already guard this; the drop paths now do too.Found in practice, not by inspection:
iterator_handle_rejects_reuseleaked onebasilper run, reparented toppid=1, on two separate worktrees.Changes are visible to end-users: no
Test plan
intact on the success path, and stays quiet once the child is already reaped.
live_pids()would race every other test in the binary.drop, and observing that from outside needs instrumentation out of proportion
to the fix.
cargo test -p axl-runtime— 380 passed.cargo test -p aspect-cli— 74 passed.A full run now leaves no orphaned
basil.🤖 Generated with Claude Code