bazelbuild / bazelbuild/bazel

The server may keep a command running and its lock held when the client is killed

Open
#30,954 0 comments 0 reactions 0 assignees View on GitHub
P2 team-Core team-Local-Exec type: bug
Dominant language
Java
Stars
25.8k
Forks
4.6k
Avg merge
2d 16h
Merged PRs (30d)
72

Description

### Description of the bug:

When a `bazel` client is killed while its command is running, the workspace stays locked: the next invocation waits, or fails outright with `--noblock_for_lock`, for a command whose client no longer exists:
```console
$ bazel build //... # in another terminal: kill -9 that client
$ bazel build //...
Another command (pid=41944) is running. Waiting for it to complete on the server (server_pid=1457)...
```
`41944` is gone, and nothing will ever collect its result.

The wait lasts as long as the abandoned command would have taken, so a 10-minute test run blocks the workspace for 10 minutes, and in the case of #30435 it lasted until the server was killed by hand.

Meanwhile the abandoned command keeps building, running tests and writing to the output base.
Anything that kills a client triggers it: a canceled CI job whose runner escalates to SIGKILL, the OOM killer, etc.
Getting out of it means killing the server, for instance by deleting `$(bazel info output_base)/server/server.pid.txt`, since `bazel shutdown` queues on the same lock.

The server only ever learns that its client is gone in `GrpcCommandServerImpl.BlockingStreamObserver`, and `BlazeCommandDispatcher` keeps the command lock until the command it dispatched returns.
Any command is affected: we hit it with `build`, `test` and `coverage`.

058adaacae08 fixed #30435 in one place, the coverage report evaluation, and the fix works, as far as it goes: `SkyframeBuildView.analyzeAndExecuteTargets` now bails out when the thread is already interrupted, and evaluates the coverage report artifacts through the interruptible `SkyframeExecutor.evaluate(...)` rather than `evaluateSkyKeys`, so that particular retry can no longer be restarted forever.
The CPU-pinned loop reported in #30435, with its `DeletingNodeVisitor` and `ForkJoinPool` churn, is gone.

There's however a missing counterpart in `BlockingStreamObserver`, which is the only place where the server acts on a client that prematurely closed the connection:
```java
} finally {
// Restore the interrupt bit.
if (interrupted || observer.isCancelled()) {
Thread.currentThread().interrupt();
}
}
```

This causes 3 distinct problems in which the command runs on, the lock stays held, and the next invocation on the same output base blocks:
1. a command that stops writing is never interrupted: the interrupt is delivered from `onNext`, so it takes an output write to happen.
A command that produces nothing more after its client is killed runs to completion, holding the lock for its whole duration,
2. the interrupt goes to whichever thread writes: that is frequently `cli-update-thread`, the progress refresh thread started by `UiEventHandler`, which catches `InterruptedException` and keeps refreshing the progress bar.
It absorbs the only interrupt, so the main thread of the command is never interrupted and the command never terminates,
3. the interrupt is repeated on every write: the observer sets the interrupt bit again at each progress report, so code retrying an interruptible step while the command terminates never gets a clean attempt and never finishes.
058adaacae08 took one such retry out of the way, the coverage report evaluation reported in #30435, but the repetition that starved it is untouched and the same retry pattern stays reachable: `SkyframeExecutor.evaluateSkyKeys` still goes through `Uninterruptibles.callUninterruptibly`, and so does `SequencedSkyframeExecutor.dropConfiguredTargetsNow`, which runs the invalidator when a previous invocation discarded the analysis cache.

Cases 1 and 2 are reproduced below on current master.

I don't have a reproducer for case 3: the coverage report evaluation was the only victim I know of, and it is now fixed.
What is left to look at is the repetition itself, plus the two call sites named above that still retry the same way.

### Which category does this issue belong to?

Core

### What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

`BUILD.bazel`:
```starlark
genrule(name = "slow", outs = ["slow.out"], cmd = "sleep 120; touch $@")
```

```console
$ bazel build --noshow_progress --curses=no //:slow & # case 1, silent
$ # or, for case 2, the same without --noshow_progress
$ kill -9
$ bazel --noblock_for_lock info
Another command (pid=) is running. Exiting immediately.
```

The lock stays held until the action completes on its own, 120 seconds here.
With `sleep 3600` it stays held for an hour.

`kill -9` matters: SIGTERM lets the client send a `Cancel` RPC, which the server has always acted upon, so the same scenario recovers cleanly with SIGTERM.

### Which operating system are you running Bazel on?

Reproduced on Linux (Ubuntu 24.04). Originally observed on macOS CI runners.

### What is the output of `bazel info release`?

`release 9.2.0` for the reproduction above. Also reproduced against master built from source, which includes 058adaacae08.

### If `bazel info release` returns `development version` or `(@non-git)`, tell us how you built Bazel.

`bazel build //src:bazel` at the revision below.

### What's the output of `git remote get-url origin; git rev-parse HEAD` ?

```text
https://github.com/bazelbuild/bazel.git
4e8f5549b3c34454e5ed863724558d362e5de10e
```

### If this is a regression, please try to identify the Bazel commit where the bug was introduced with bazelisk --bisect.

_No response_

### Have you found anything relevant by searching the web?

- #30435, the coverage livelock, fixed by 058adaacae08 as described above,
- #2337, `bazel run` releasing the lock before launching, which is why `bazel run` cannot be used to reproduce this: the client execs the binary itself, so the command ends before the binary starts.

### Any other information, logs, or outputs that you want to share?

I have a fix with tests at 3 levels, each failing on master:
- killing a client must leave the lock free (shell integration),
- a command whose client left must be able to finish (`CommandServerTest`),
- no write past the cancellation may set the interrupt bit again (`GrpcCommandServerImplTest`).

Please take a look:
- #30960.

Contributor guide

Open the contributing guide

Research direction

Start with GrpcCommandServerImpl.BlockingStreamObserver and BlazeCommandDispatcher to trace cancellation, interruption, and lock ownership. Then read the retry paths in SkyframeExecutor.evaluateSkyKeys and SequencedSkyframeExecutor.dropConfiguredTargetsNow, along with CommandServerTest and GrpcCommandServerImplTest. Done means the named client-death cases release the lock, allow the command to finish, and prevent repeated post-cancellation interruption.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
build-system
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.