Avoid CPU-load scheduling deadlocks

[tamird]

CPU-load scheduling rejects the first action when CPU capacity is zero because the action cap evaluates 0 >= 3 * 0. The request is then queued on a latch that no running action can release.

Check the hard action cap before sampling machine load, allow only the first local action past a zero-capacity cap, and distinguish tracked CPU usage from effective machine load when applying the allow-one policy. Enforce the cap on subsequent zero- and positive-CPU requests.

Cover loaded positive, zero, and negative CPU capacity; explicit zero-CPU requests; non-CPU resource use; and the action cap.

Fixes #28713.

[fmeum]

@coeuvre

[fmeum]

@bazel-io fork 9.3.0

[bigelephant29]

Please give me some time to think through this. I'll post an update here next Monday.

[bigelephant29]

!(allowOneActionOnResourceUnavailable && used == 0)

This implementation feels weird to me. It means that we want allowOneActionOnResourceUnavailable to take effect for this case, so we add a check here, then we'll bypass this condition and use the second isAvailable at L717.

But what we should do instead is to make the behavior of the --experimental_cpu_load_scheduling feature stay within this if condition. This is for the sake of improving the maintainability of this experimental flag. So IMO, we should not enrich the condition check here, and we should stick to the isAvailable call at L714.

[tamird]

Updated: CPU-load scheduling stays within isCpuAvailable. Its load
branch checks the action cap, then makes one shared boolean availability
decision using tracked CPU for allow-one and machine/window load for
normal admission. There is no fall-through to the non-load path. The
separate queue-safety change is in #30395.

[bigelephant29]

available > 0 is a good check at the first glance, but I don't see a reason why we only check this for cpuLoadScheduling. I mean, this semantic embeds a hidden information of the existence of --allow_one_action_on_resource_unavailable, which people don't usually get it unless they fully understand this whole java file, and I think this is dangerous in the long run.

I expect available < 0 would not exist and we can verify that later, so this check is basically enforcing all available == 0 cases to go to the call at L717, and due to the complicated logic in isAvailable, this is almost equivalent to checking whether --allow_one_action_on_resource_unavailable is set or not.

With this conclusion, it sounds weird that the case don't work when --experimental_cpu_load_scheduling is set. I mean, if the goal is to make sure available == 0 still functions well when the feature is enabled, we should fix that within the cpuLoadScheduling condition, and we shouldn't delegate the decision to the other side.

[tamird]

Updated: there is no longer an available > 0 guard or non-load
fallback. With no local action running, the first action can pass the
CPU cap even at zero capacity; subsequent actions remain capped.

Negative capacity is reachable today through keyword arithmetic, for
example --local_resources=cpu=HOST_CPUS-3 on a two-CPU host. Zero,
negative, and scaled-but-valid requests are covered. The strict
pre-queue validation is separated into #30395.

[bigelephant29]

I don't have a concrete plan yet, and I don't exactly know what is causing your deadlock. Is it caused by execptions or boolean values?

What about this: let's refactor this if-condition into:

if (cpuLoadScheduling) {
  return isAvailableOnCpuLoadScheduling(...);
}

And let's discuss what the expected behavior you have in mind after your have a proposal?

[tamird]

There are two distinct failure paths.

The reported deadlock is the boolean/cap path: with zero CPU capacity,
the old cap evaluates 0 >= 3 * 0, rejects the first action, and queues
a latch that cannot make progress. CPU-load handling now stays in
isCpuAvailable, separates tracked CPU from machine/window load, permits
the first action past the cap, and keeps subsequent actions capped.

The separate exception/latch issue is in #30395: an oversized request
could queue behind worker quota or current use and then throw during a
wake-up that cannot deliver the error. It now fails before queueing, and
wake-up checks are boolean-only.

Focused validation passes ResourceManagerTest 27/27 and
WorkerSpawnRunnerTest 14/14.

[bigelephant29]

Hey, can you double check the comment from me again and see if you'd like to go with this direction? If not, let me know the reason.

[tamird]

I didn't do that because the proposed isAvailableOnCpuLoadScheduling method ends up duplicating a lot of the logic from the other code path, and the resulting diff is also larger and harder to read.

[tamird]

@bigelephant29 could you revisit the helper-extraction question on the current CPU-only diff? Tamir explained above why he kept the shared logic. Is that structure acceptable, or what specific change remains necessary? @bazelbuild/triage, this design discussion is still awaiting reviewer follow-up.

— Codex, on Tamir’s behalf.

[meisterT]

@tamird while we don't have any issues with using AI to help with creation of contributions we would prefer that communication on changes happens between humans and is not driven by bots.

[tamird]

@meisterT this communication was driven by me! I have 6 of these PRs that have been sitting for a month, so I delegated the task of selecting which human to tag and what to say to the bot. This was an active action by a human executed by an agent. Is that acceptable?

[bigelephant29]

@tamird Thanks for your contribution!

We have a massive amount of external contributions with agentic coding frameworks. We can't prioritize efficiently if it's not clear to us whether we're talking to a human or a bot.

As the information is clear enough in this PR, I'll approve it now.

[tamird]

Thanks! All my PRs are written and updated by my agent but nothing happens without my instruction and review.

Appreciate your help getting this landed!

[tamird]

I have split this into two commits. If you'd like them presented as separate PRs, let me know.

[bigelephant29]

Hi, this PR is going towards a completely different direction. I can't easily parse your comments.

Can you walk me through the current state?

[tamird]

Hi, this PR is going towards a completely different direction. I can't easily parse your comments.

Can you walk me through the current state?

If you review each commit individually, does that help?

[tamird]

There are two separate failure paths in the current change.

The reported stall is the boolean/cap path. With CPU-load scheduling
and zero CPU capacity, the action cap evaluates 0 >= 3 * 0, rejects
the first action, and queues a latch that cannot make progress. The CPU
fix now keeps all load-scheduling behavior in isCpuAvailable: check
the cap first, use tracked CPU for allow-one and machine/window load for
normal admission, permit the first action past the cap, and keep later
actions capped.

The exception/latch problem is separate and is now #30395. With
allow-one disabled, an oversized request can queue behind worker quota
or transient usage; a later wake-up can throw
NOT_ENOUGH_LOCAL_RESOURCE, but the latch cannot return that error to
the requesting action. The precursor validates permanent capacity
before queueing and makes wake-up checks boolean-only.

This PR now contains exactly those two commits: the #30395 precursor and
the focused CPU fix. The unrelated primitive/inlining/documentation
cleanups are gone. Focused validation passes ResourceManagerTest
27/27 and WorkerSpawnRunnerTest 14/14.

[tamird]

Now that #30395 has landed, this is one independent commit containing only the CPU-load scheduling fix and its regression tests. The action cap is checked before machine load is sampled, and tracked CPU use controls the allow-one exception. The queue-validation precursor is no longer part of this diff.

[tamirdex]

[tamird]

@bigelephant29 The rebase now contains only the CPU scheduling fix and its focused tests, and 35 presubmit checks pass. The only failing job is remote execution on Ubuntu: https://buildkite.com/bazel/bazel-bazel-github-presubmit/builds/35196#019fad91-0a4a-445a-8d1b-b35408930fb2. Buildkite does not expose that log to my account. Could you share the failing action or test output so I can address the actual failure?

[tamirdex]

[meisterT]

@tamird it is visible in an incognito window, so perhaps human Tamir needs to have a look and take over from the agent ;-)

[tamird]

@meisterT You were right: the job logs are public. I found the browser's public log endpoint and checked the actual failure: https://buildkite.com/organizations/bazel/pipelines/bazel-bazel-github-presubmit/builds/35196/jobs/019fad91-0a4a-445a-8d1b-b35408930fb2/log

The changed ResourceManager test, //src/test/java/com/google/devtools/build/lib/actions:ActionsTests, passes. The sole failing target is //src/test/shell/integration:bazel_hardened_sandboxed_worker_test, which failed 3 of 12 attempts.

The independent symlink change in #30316 hits the same hardened worker failure, also 3 of 12 attempts: https://buildkite.com/organizations/bazel/pipelines/bazel-bazel-github-presubmit/builds/35199/jobs/019fada8-2517-4d65-b64a-9562b564fb92/log

On the passing RBE run for #30399, that same worker test is marked FLAKY after failing 1 of 11 attempts: https://buildkite.com/organizations/bazel/pipelines/bazel-bazel-github-presubmit/builds/35197/jobs/019fad9d-31eb-4bbc-a32c-8bda930cd89e/log

This points to worker-test flakiness rather than a failure in the CPU scheduling change. Would you prefer to retry the RBE job?

More precisely, 52ba561 (#30067) re-enabled test_build_succeeds_even_if_worker_exits, which had previously been disabled as flaky. Its immutable first-attempt artifact shows that the first build already fails with PARSE_RESPONSE_FAILURE when the worker exits; the newly added post-build wait cannot run until after that first build succeeds: https://buildkite.com/organizations/bazel/pipelines/bazel-bazel-github-presubmit/builds/35196/jobs/019fad91-0a4a-445a-8d1b-b35408930fb2/artifacts/019fad96-e880-4fd4-96a9-20cb46868986

The unrelated #30321 fails the same worker-test family on the same base: https://buildkite.com/organizations/bazel/pipelines/bazel-bazel-github-presubmit/builds/35201/jobs/019fada9-db10-4238-9cdb-f4ced7b142b7/log

How would you prefer to handle the re-enabled worker test?

[tamirdex]

[tamird]

@meisterT lol sorry for making you talk to the agent, but thank you for giving it a nudge!

[tamird]

@meisterT 6fc7597 fixes the worker-response race identified above, and this PR is now rebased on that fix. The remaining red check is a different, shared macOS runner failure: xcrun cannot open Xcode's libxcrun.dylib (errno=5), and the final automatic retry fails before the repository is checked out:

https://buildkite.com/organizations/bazel/pipelines/bazel-bazel-github-presubmit/builds/35248/jobs/019faf5c-3ff4-4e65-9a19-afbc8b06209b/log

The unrelated 9.3.0 release build independently fails with the identical pre-checkout error:

https://buildkite.com/organizations/bazel/pipelines/bazel-bazel-github-presubmit/builds/35249/jobs/019faf59-518e-4244-9a2d-e23954c83cde/log

#30316 and #30321 are affected too. Is there an existing Bazel CI issue for these macOS runners?

[tamirdex]

[tamird]

@iancha1992 looks like this was meant to merge but was conflicted and never did. I have rebased it. Thanks!

[bigelephant29]

Hi there, we're working on merging this. It'll be shipped soon.