kris/krisbuild
Watch
1
0
Fork
You've already forked krisbuild
 0
Code Issues Pull requests 2 Projects Releases Packages Wiki Activity Actions

Node health: don't quarantine the last node able to take work when others tripped with it #36

Open
kris wants to merge 2 commits from breaker-last-healthy-node into main
pull from: breaker-last-healthy-node
merge into: kris:main
Conversation 1 Commits 2 Files changed 9 +429 -45
kris commented 2026-10-01 00:21:06 +02:00
Owner
Copy link

What. The per-node circuit breaker is now judged across the whole fleet (scheduler/health.rs::blocked_nodes, SPEC §7.1).

The hold. It applies when the breakers of two or more able nodes have tripped and no able node is left. A node is able if it is connected, not draining and not below the disk floor; a closed breaker or one on probation leaves it able. In that case the node whose cool-off ends first is held on probation instead of being opened; the name breaks a tie. It takes one task at a time. A success closes its breaker. A failure restarts its cool-off, and the hold passes to whichever node's cool-off now ends first.

When nothing changes:

  • A lone able node that trips is not held. Nothing corroborates a shared cause, and its cool-off is what stops a black hole from failing the queue one task after another. That covers a one-node fleet, and a larger fleet whose other nodes are drained, disconnected or low on disk.
  • A single bad node beside a working one, or beside one on probation, is quarantined as before.

Details:

  • The match only counts a node as able if it is closed or on probation. A new kind of hold-back, such as a store-disk variant, therefore stays out of the able set.
  • Nodes that trip in the same pass are judged together from one snapshot, so exactly one is held. The scheduler, the queue page and /api/nodes all use the same function, and the tie-break keeps the choice stable.

Visibility:

  • GET /api/nodes shows health.held: true. While the held node has a task out, unschedulable reads on probation: one task at a time.
  • /ui/nodes shows a probation badge and "held: the last node able to take work".
  • Logging happens once per change across passes: warn! when a hold starts, debug! when it moves to another node, info! when it is released. The state lives in AppState::breaker_held.
  • load_nodes now takes the live NodeSnapshots, so /api/nodes judges connected nodes the same way the matchmaker does.

Why. Before this, a cause shared by every node could quarantine the whole fleet for quarantine_secs and stop all CI. Examples are a rev whose checkout fails everywhere, or the git proxy being down. This was the open item in docs/TODO.md from the #8 review.

The hold requires two tripped nodes. A lone node is still bounded by its cool-off, which is the throttle the breaker exists for in the 2026-09-27 black-hole case.

Discounting failures that other nodes share is not done. It needs a definition of "the same cause" that could hide a genuinely broken node, so it stays open in TODO.md. Also still open: the hold is per fleet, not per task, and a lone able node waits out its cool-off.

Tests.

  • Unit tests in health.rs:
    • Two nodes trip in the same pass: one is held, the other stays quarantined. The held node is Probing while it has a task out, and its success releases the hold.
    • A bad node beside a healthy peer, or beside a peer on probation, is quarantined.
    • A lone able node is quarantined, not held. A draining or low-disk peer is never held, and never counted, even when its cool-off ends first.
    • The hold passes to the node whose cool-off ends first, and the name breaks a tie.
    • The hold is tracked across passes.
  • it::node_health::a_black_hole_node_is_quarantined, restored:
    • Alone, alpha is quarantined and not held.
    • Then a push on another branch queues work alpha never failed. Alpha gets none of it, and /api/queue why_not.eliminated.alpha says "quarantined".
    • Once beta connects, beta runs the new work and the retries.
    • Checked by mutation: letting quarantined nodes through the matchmaker fails this test.
  • it::node_health::a_shared_cause_holds_one_node_on_probation, new:
    • Alpha and beta fail every checkout until both trip. One is then held and the other quarantined.
    • The held node gets exactly one task at a time and the why-not says "probation".
    • Its success closes its breaker, the other node stays quarantined, and the held node runs the rest.
  • cargo test --workspace and cargo clippy --workspace --all-targets -- -D warnings pass. I have not run the nix gate (nix build .#ci.*) locally because nix is not installed in the VM. No files were added, so no fileset changes are needed.

Deploy notes.

  • No config, schema or protocol changes, and no new files. /api/nodes gains one field, health.held.
  • Behaviour changes only when two or more able nodes have tripped and none is left. On nuxbox + ares, that means both trip while both are connected and above the disk floor.
  • A one-node fleet, or a node whose peer is drained or down, behaves exactly as before.
  • docs/forgejo-setup.md describes the hold.
  • The SPEC §6.3 pseudocode line ("breaker closed") is left for after #35, since both PRs edit it.

🤖 Generated with Claude Code

**What.** The per-node circuit breaker is now judged across the whole fleet (`scheduler/health.rs::blocked_nodes`, SPEC §7.1). **The hold.** It applies when the breakers of two or more able nodes have tripped and no able node is left. A node is able if it is connected, not draining and not below the disk floor; a closed breaker or one on probation leaves it able. In that case the node whose cool-off ends first is **held on probation** instead of being opened; the name breaks a tie. It takes one task at a time. A success closes its breaker. A failure restarts its cool-off, and the hold passes to whichever node's cool-off now ends first. **When nothing changes:** - A lone able node that trips is not held. Nothing corroborates a shared cause, and its cool-off is what stops a black hole from failing the queue one task after another. That covers a one-node fleet, and a larger fleet whose other nodes are drained, disconnected or low on disk. - A single bad node beside a working one, or beside one on probation, is quarantined as before. **Details:** - The match only counts a node as able if it is closed or on probation. A new kind of hold-back, such as a store-disk variant, therefore stays out of the able set. - Nodes that trip in the same pass are judged together from one snapshot, so exactly one is held. The scheduler, the queue page and `/api/nodes` all use the same function, and the tie-break keeps the choice stable. **Visibility:** - `GET /api/nodes` shows `health.held: true`. While the held node has a task out, `unschedulable` reads `on probation: one task at a time`. - `/ui/nodes` shows a `probation` badge and "held: the last node able to take work". - Logging happens once per change across passes: `warn!` when a hold starts, `debug!` when it moves to another node, `info!` when it is released. The state lives in `AppState::breaker_held`. - `load_nodes` now takes the live `NodeSnapshot`s, so `/api/nodes` judges connected nodes the same way the matchmaker does. **Why.** Before this, a cause shared by every node could quarantine the whole fleet for `quarantine_secs` and stop all CI. Examples are a rev whose checkout fails everywhere, or the git proxy being down. This was the open item in docs/TODO.md from the #8 review. The hold requires two tripped nodes. A lone node is still bounded by its cool-off, which is the throttle the breaker exists for in the 2026-09-27 black-hole case. Discounting failures that other nodes share is not done. It needs a definition of "the same cause" that could hide a genuinely broken node, so it stays open in TODO.md. Also still open: the hold is per fleet, not per task, and a lone able node waits out its cool-off. **Tests.** - Unit tests in health.rs: - Two nodes trip in the same pass: one is held, the other stays quarantined. The held node is `Probing` while it has a task out, and its success releases the hold. - A bad node beside a healthy peer, or beside a peer on probation, is quarantined. - A lone able node is quarantined, not held. A draining or low-disk peer is never held, and never counted, even when its cool-off ends first. - The hold passes to the node whose cool-off ends first, and the name breaks a tie. - The hold is tracked across passes. - `it::node_health::a_black_hole_node_is_quarantined`, restored: - Alone, alpha is quarantined and not held. - Then a push on another branch queues work alpha never failed. Alpha gets none of it, and `/api/queue` `why_not.eliminated.alpha` says "quarantined". - Once beta connects, beta runs the new work and the retries. - Checked by mutation: letting quarantined nodes through the matchmaker fails this test. - `it::node_health::a_shared_cause_holds_one_node_on_probation`, new: - Alpha and beta fail every checkout until both trip. One is then held and the other quarantined. - The held node gets exactly one task at a time and the why-not says "probation". - Its success closes its breaker, the other node stays quarantined, and the held node runs the rest. - `cargo test --workspace` and `cargo clippy --workspace --all-targets -- -D warnings` pass. I have **not** run the nix gate (`nix build .#ci.*`) locally because nix is not installed in the VM. No files were added, so no fileset changes are needed. **Deploy notes.** - No config, schema or protocol changes, and no new files. `/api/nodes` gains one field, `health.held`. - Behaviour changes only when two or more able nodes have tripped and none is left. On nuxbox + ares, that means both trip while both are connected and above the disk floor. - A one-node fleet, or a node whose peer is drained or down, behaves exactly as before. - docs/forgejo-setup.md describes the hold. - The SPEC §6.3 pseudocode line ("breaker closed") is left for after #35, since both PRs edit it. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Node health: never quarantine the last node able to take work
All checks were successful
krisbuild/kris/krisbuild/nix/hello cached
Details
krisbuild/kris/krisbuild/nix/test-deps cached
Details
krisbuild/kris/krisbuild/nix/workspace-deps cached
Details
krisbuild/kris/krisbuild/nix/world cached
Details
krisbuild/kris/krisbuild/nix/clippy succeeded
Details
krisbuild/kris/krisbuild/nix/test succeeded
Details
krisbuild/kris/krisbuild/nix/build succeeded
Details
krisbuild/kris/krisbuild/nix/kb-check succeeded
Details
krisbuild/kris/krisbuild/nix/deployed succeeded
Details
krisbuild/kris/krisbuild krisbuild kris/krisbuild: all tasks succeeded
Details
ca587e5c97 
The breaker is judged across the fleet: when every connected node that is
not draining or below the disk floor would be quarantined, the one whose
cool-off ends first is held on probation instead, so a cause they share (a
rev every checkout fails on) slows the fleet to one task at a time rather
than stopping it for a cool-off. A single bad node beside a working one is
quarantined as before. /api/nodes shows the hold as health.held, /ui/nodes
badges it, and a warning is logged once per hold.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
kris commented 2026-10-01 00:30:59 +02:00
Author
Owner
Copy link

Automated review (reviewer subagent) of ca587e5c97

Verdict: request changes. The mechanics are sound, but two things block.

Gates: cargo clippy --workspace --all-targets -j2 -- -D warnings is clean. cargo test --workspace is not verified: the sandbox VM's disk went read-only mid-run because of host block-I/O errors. Mutation checks were argued from the code, not executed.

Blocking

  1. A lone able node is held, which drops the breaker's throttle in the black-hole case it exists for. scheduler/health.rs:403-419, SPEC.md:945-959.
    • When only one node can take work, it stays on probation indefinitely. Each failure restarts its cool-off, but it is still the minimum, so it is re-held on the next pass.
    • A spawn-error black hole (the 2026-09-27 pattern from #8) now fails tasks serially at millisecond pace, where before it made about one attempt per quarantine_secs.
    • "Lone able node" also covers the two-node fleet whenever its peer is drained, disconnected or low on disk, i.e. during maintenance.
    • A platform-wide cause (e.g. the git proxy down) now drains the queue into red builds instead of pausing.
    • Fix: hold only when corroborated, i.e. open.len() >= 2 (at least two able nodes tripped, so a shared cause is plausible); a lone node keeps the bounded cool-off. Update SPEC §7.1, the deploy notes, and the "a lone node is held" leg of peers_that_take_no_work_do_not_count (health.rs:856). Optionally pace held probes.
  2. The integration test no longer proves "a quarantined node gets no new work". tests/it/node_health.rs:198-222.
    • The final alpha.next_assign(1s).is_none() runs after beta has finished a, b and c, so it is vacuous.
    • "beta runs all three retries" is explained by avoided_nodes alone.
    • Removing Quarantined from matchmaking's blocked would likely still pass.
    • The queue why-not "quarantined" is no longer asserted anywhere.
    • Fix: split the test. a_lone_node_is_held_on_probation becomes whatever #1 settles on. Restore a_black_hole_node_is_quarantined with work queued that alpha has not failed (e.g. beta busy or too small). Assert that alpha gets nothing while that task waits, and that /api/queue why_not.eliminated.alpha contains "quarantined".

Non-blocking

  1. health.rs:430-445: the warning is deduped per node, not per hold, so with two nodes a shared cause warns on every failed probe, and a release is silent. Dedupe on "a hold exists", info! when it ends, debug! on a move.
  2. health.rs:~793: nothing tests the hold passing to the peer whose cool-off ends first, or the name tie-break. Add both.
  3. docs/forgejo-setup.md:209-214 still says a tripped node "gets no new work". Mention the hold.
  4. (follow-up) SPEC.md:599 §6.3 pseudocode says "breaker closed". Reconcile it with probation and the hold once #35 lands, since both PRs edit that line.

Checked and fine

  • Read-only callers have no side effects: load_nodes, load_queue and /api/nodes each mutate a fresh map; warn_held is called only from matchmaking.
  • Nothing persisted: the hold is derived per pass.
  • Races and ordering: same-pass double trips give one hold. Draining, low-disk and disconnected peers are excluded. A returning peer releases the hold. With two or more nodes the hold can't pin to one.
  • Probation with infra retries: interacts correctly.
  • Conventions and deploy: follow the house style; health.held is additive.
## Automated review (reviewer subagent) of ca587e5c97396f6bc2638bdda2e834da748f228f **Verdict: request changes.** The mechanics are sound, but two things block. Gates: `cargo clippy --workspace --all-targets -j2 -- -D warnings` is clean. `cargo test --workspace` is **not verified**: the sandbox VM's disk went read-only mid-run because of host block-I/O errors. Mutation checks were argued from the code, not executed. ### Blocking 1. **A lone able node is held, which drops the breaker's throttle in the black-hole case it exists for.** `scheduler/health.rs:403-419`, SPEC.md:945-959. - When only one node can take work, it stays on probation indefinitely. Each failure restarts its cool-off, but it is still the minimum, so it is re-held on the next pass. - A spawn-error black hole (the 2026-09-27 pattern from #8) now fails tasks serially at millisecond pace, where before it made about one attempt per `quarantine_secs`. - "Lone able node" also covers the two-node fleet whenever its peer is drained, disconnected or low on disk, i.e. during maintenance. - A platform-wide cause (e.g. the git proxy down) now drains the queue into red builds instead of pausing. - **Fix:** hold only when corroborated, i.e. `open.len() >= 2` (at least two able nodes tripped, so a shared cause is plausible); a lone node keeps the bounded cool-off. Update SPEC §7.1, the deploy notes, and the "a lone node is held" leg of `peers_that_take_no_work_do_not_count` (health.rs:856). Optionally pace held probes. 2. **The integration test no longer proves "a quarantined node gets no new work".** `tests/it/node_health.rs:198-222`. - The final `alpha.next_assign(1s).is_none()` runs after beta has finished a, b and c, so it is vacuous. - "beta runs all three retries" is explained by `avoided_nodes` alone. - Removing Quarantined from matchmaking's `blocked` would likely still pass. - The queue why-not "quarantined" is no longer asserted anywhere. - **Fix:** split the test. `a_lone_node_is_held_on_probation` becomes whatever #1 settles on. Restore `a_black_hole_node_is_quarantined` with work queued that alpha has *not* failed (e.g. beta busy or too small). Assert that alpha gets nothing while that task waits, and that `/api/queue` `why_not.eliminated.alpha` contains "quarantined". ### Non-blocking 3. `health.rs:430-445`: the warning is deduped per node, not per hold, so with two nodes a shared cause warns on every failed probe, and a release is silent. Dedupe on "a hold exists", `info!` when it ends, `debug!` on a move. 4. `health.rs:~793`: nothing tests the hold passing to the peer whose cool-off ends first, or the name tie-break. Add both. 5. `docs/forgejo-setup.md:209-214` still says a tripped node "gets no new work". Mention the hold. 6. (follow-up) SPEC.md:599 §6.3 pseudocode says "breaker closed". Reconcile it with probation and the hold once #35 lands, since both PRs edit that line. ### Checked and fine - **Read-only callers have no side effects:** `load_nodes`, `load_queue` and `/api/nodes` each mutate a fresh map; `warn_held` is called only from matchmaking. - **Nothing persisted:** the hold is derived per pass. - **Races and ordering:** same-pass double trips give one hold. Draining, low-disk and disconnected peers are excluded. A returning peer releases the hold. With two or more nodes the hold can't pin to one. - **Probation with infra retries:** interacts correctly. - **Conventions and deploy:** follow the house style; `health.held` is additive.
Node health: hold a node on probation only when two or more tripped
All checks were successful
krisbuild/kris/krisbuild/nix/test-deps cached
Details
krisbuild/kris/krisbuild/nix/workspace-deps cached
Details
krisbuild/kris/krisbuild/nix/hello cached
Details
krisbuild/kris/krisbuild/nix/world cached
Details
krisbuild/kris/krisbuild/nix/clippy succeeded
Details
krisbuild/kris/krisbuild/nix/test succeeded
Details
krisbuild/kris/krisbuild/nix/build succeeded
Details
krisbuild/kris/krisbuild/nix/deployed succeeded
Details
krisbuild/kris/krisbuild/nix/kb-check succeeded
Details
krisbuild/kris/krisbuild krisbuild kris/krisbuild: all tasks succeeded
Details
c160e82723 
A lone able node that trips keeps its cool-off: nothing corroborates a
shared cause, and the cool-off is what keeps a black hole from failing the
queue serially. The hold now needs at least two able nodes with an open
breaker. A hold is logged when it starts, moves and ends rather than per
probe.

The black-hole integration test is restored with work alpha never failed
(a push on another branch) and asserts the queue names the quarantine; the
probation path gets its own two-node test. docs/forgejo-setup.md describes
the hold.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
kris changed title from Node health: never quarantine the last node able to take work to Node health: don't quarantine the last node able to take work when others tripped with it 2026-10-02 17:58:59 +02:00
All checks were successful
krisbuild/kris/krisbuild/nix/test-deps cached
Details
krisbuild/kris/krisbuild/nix/workspace-deps cached
Details
krisbuild/kris/krisbuild/nix/hello cached
Details
krisbuild/kris/krisbuild/nix/world cached
Details
krisbuild/kris/krisbuild/nix/clippy succeeded
Details
krisbuild/kris/krisbuild/nix/test succeeded
Details
krisbuild/kris/krisbuild/nix/build succeeded
Details
krisbuild/kris/krisbuild/nix/deployed succeeded
Details
krisbuild/kris/krisbuild/nix/kb-check succeeded
Details
krisbuild/kris/krisbuild krisbuild kris/krisbuild: all tasks succeeded
Details
This pull request can be merged automatically.
This branch is out-of-date with the base branch
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin breaker-last-healthy-node:breaker-last-healthy-node
git switch breaker-last-healthy-node
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
kris/krisbuild!36
No description provided.