fix(workflows): don't signal runner unhealthy mid-job on sandbox poisoning - #1402
Merged
Merged
Conversation
…oning The `still_poisoned` branch of the bazel#23880 sandbox recovery hook ran as `bazel_attempt_end` and called `signal_instance_unhealthy` immediately, including after non-final attempts. That signal is a hard reclaim, not a drain request — on AWS it flips the instance Unhealthy with `--no-should-respect-grace-period`, on GCP it calls `delete-instances`. The retry loop, which knows nothing about the hook's side effect, then started the next attempt on an instance the ASG/MIG was already tearing down. A couple of minutes later the agent took a cancellation signal and the job died with SIGTERM/143 instead of reporting its real Bazel exit code. The retry was doomed regardless: we mark unhealthy precisely because the sandbox could not be cleaned, and every subsequent invocation on that output base crashes the same way. Coordinate the three seams through one `sandbox_recovery.new(environment)` state holder. `attempt_end` now records the poisoning verdict instead of signalling; `build_retry` declines to retry once it is set; `build_end` performs the deferred signal. The job ends on its real exit code and the runner still leaves the pool before it can take the next job. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
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: Pro Plus 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 |
![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: fd781302a1
ℹ️ 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".
✨ Aspect Workflows Tasks📅 Thu Aug 27 18:49:48 UTC 2026 🔄 4 in progress tasks
❌ 1 failed task
|
…d_retry Addresses review feedback on the first pass, which moved the signal from `bazel_attempt_end` to `build_end`. That seam is per-*build*, not per-task: `delivery.axl` fires `build_end` after phases 1/2 and then keeps working for minutes (phase 3 plus the deliverable subprocesses), and phase 3 deliberately fires only `bazel_attempt_end`. So the mid-job reclaim race survived for delivery, and a phase-3-only poisoning verdict never signalled at all. Both signal paths now register a `ctx.defer`, which runs after the task impl returns (even on a fatal Starlark error) and is the only true end-of-task seam. A `signal_pending` latch keeps it to one reclaim per task however many attempts or builds report trouble. `build_retry` is a single-valued attr, so overwriting it dropped any predicate a custom task or another feature had installed and silently reinstated `default_retry`. The current predicate is now passed into `sandbox_recovery.new` and delegated to unless poisoning forces False. The server-sick classification in `build_end` deliberately stays on `default_retry` — a task that declines to retry 37 has not thereby made its runner healthy. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
A customer's Buildkite job died with
status 143a couple of minutes after a Bazel server crash (Socket closed), instead of reporting the real Bazel exit code.The
still_poisonedbranch of the bazel#23880 sandbox recovery hook calledsignal_instance_unhealthyinline, frombazel_attempt_end. That signal is a hard reclaim, not a drain request: on AWS it flips the instanceUnhealthywith--no-should-respect-grace-period, on GCP it callsinstance-groups managed delete-instances. Meanwhile the retry loop, which knows nothing about the hook's side effect, started the next attempt. The ASG/MIG reclaimed the box out from under the running job, the agent took a cancellation signal, and the job exited 143.That retry was doomed regardless: we mark the runner unhealthy precisely because the sandbox could not be cleaned, and every subsequent Bazel invocation against the same output base crashes the same way.
What changed
The hooks now share one
sandbox_recovery.new(environment, retry_predicate)state holder:attempt_endrecords the poisoning verdict instead of signalling.build_retrydeclines to retry once that verdict is set, delegating to the task's existing predicate otherwise.build_endhandles the separate "final exit is server-internal-error class" policy.Both signal paths register a
ctx.deferrather than signalling inline.build_endis a per-build seam, not per-task —delivery.axlfires it after phases 1/2 and then keeps working for minutes (phase 3 plus the deliverable subprocesses), and phase 3 fires onlybazel_attempt_end.ctx.deferruns after the task impl returns, even on a fatal Starlark error, so it is the only seam that reliably means "the job is over". Asignal_pendinglatch keeps it to one reclaim per task however many attempts or builds report trouble.build_retryis a single-valued attr, so the previous predicate is passed into.new()and delegated to rather than overwritten — otherwise enabling this feature would silently reinstatedefault_retryover any predicate a custom task or another feature installed. The server-sick classification inbuild_enddeliberately stays ondefault_retry: a task that declines to retry 37 has not thereby made its runner healthy.Net effect: the job ends on its real exit code, and the runner still leaves the pool before it can pick up the next job.
Changes are visible to end-users: yes
Suggested release notes
Test plan
aspect dev test-sandbox-recovery— 15 sections, covering:build_retrystops retrying once poisoning is recorded, including over a permissive task predicatebuild_retrydelegates to the task's predicate when no poisoning is recordedbuild_endclassification ignores the task predicate and usesdefault_retryclean/repairedstay quiet, final 36/37 signal, final 0/1/2/3/4/6/7/8 do not, and a missing signal binary is a no-opEach new regression test was confirmed to fail against the pre-fix behavior. The full AXL suite (~45
dev test-*tasks) and the Rustsandbox_recoveryunit tests pass.Not reproduced end-to-end on a live runner — the diagnosis is from the customer's log plus the
signal_instance_unhealthyimplementation in silo (lib.sh:1548-1565). The ~2 min gap between the unhealthy signal and the cancellation matches ASG reaction latency plus the deliberatesleep 10for CloudWatch flushing.🤖 Generated with Claude Code