Skip to content

fix(stovepipe): admit before counting the build slot - #974

Merged
sbalabanov merged 1 commit into
mainfrom
sergeyb/sp-oct-003-fail-open-admit
Oct 30, 2026
Merged

sbalabanov merged 1 commit into
mainfrom
sergeyb/sp-oct-003-fail-open-admit

Conversation

@sbalabanov

@sbalabanov sbalabanov commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Why?

process admits the latest head with two ordered CAS writes on two rows: it increments the anonymous queue.in_flight_count first, then moves the request accepted→processing. A crash between them leaves the count raised with no owner. Holds never consume retry budget and the DLQ reconciler releases only processing requests, so the queue wedges permanently.

  1. Queue at max_concurrent = 1; request R is accepted and is the latest head.
  2. process claims the slot (in_flight_count = 1) and crashes before writing processing.
  3. Every redelivery re-reads accepted, finds the gate closed by its own claim, and holds.
  4. Nothing admits again, and the best-effort unwind releases never ran because the process crashed.

What?

  • admitLatestHead gates and derives the strategy as before, then CASes the request accepted→processing first and increments in_flight_count second. A version conflict on the increment reloads and retries without re-gating or coalescing, because the admit is already durable. A lost request CAS acks without touching the counter.
  • Removes process's releaseBuildSlot helper and both unwind call sites.
  • Trade-off: the count now drifts low rather than high. A crash or hard failure between the two writes, or two heads that both pass the gate in the same window, over-admits by one. The existing zero clamp in the buildsignal and DLQ release paths brings the count back to the true value as the queue drains. The count can go high only if a delivery stalls between the two writes past the visibility timeout and wakes after its redelivery has finished the validation; the RFC records this residual and how to narrow or close it.
  • doc/rfc/stovepipe/steps/process.md: step 7 reordered; the redelivery and in_flight_count integrity sections now describe the fail-open order. Two dlq.go/buildsignal.go doc comments that described the old claim-first order are corrected (comments only).
  • This change does not repair counts that leaked before it, so a queue that is already wedged needs a one-time manual reset of in_flight_count.

Alternative to #966, which keys slots by request ID in a new in_flight_request_ids column and is the exact fix; this PR needs no schema change. Merge one or the other, not both.

Test Plan

  • make fmt && make lint && make check-tidy && make check-gazelle && make check-mocks
  • ./tool/bazel test //stovepipe/... //service/stovepipe/... --test_output=errors
  • ./tool/bazel build //test/e2e/stovepipe:go_default_test //test/integration/stovepipe/...

Audit: SP-OCT-003 and SP-OCT-005 in doc/audit/stovepipe-fable-v2.md (Sol v2 #1/#2, Opus v2 #1/#8).
🤖 Generated with Claude Code

@sbalabanov
sbalabanov requested review from a team and behinddwalls as code owners October 28, 2026 17:58
@sbalabanov

sbalabanov commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor Author

Design question (Supervisor Fable): there is one residual path that leaves in_flight_count high under this order. A delivery that stalls between the request CAS and the increment for longer than the visibility timeout, while its redelivery takes the processing branch and the build runs to terminal, wakes afterwards and increments. The zero-clamped release absorbed nothing to release, and claimBuildSlot retries version conflicts without re-gating, so the late increment lands and that slot is never released. At max_concurrent = 1 that is a wedge until an operator resets the count. It needs the stall to outlast the redelivery plus a whole validation, so it is far narrower than the wedge this PR removes, but it is not zero.

Two ways to handle it:

  1. Document only (current state): the RFC's integrity section names the residual.
  2. Re-read the request right before the increment and skip it when the request is terminal: one extra point read per admit, and the window shrinks to the gap between two reads.

#966 closes it exactly by keying the slot on the request ID. I have left the code at option 1 pending your call; say "add the re-check" and I will have the subagent add it.

Process claimed a build slot before moving the request to processing, so a
crash between the two writes left in_flight_count raised with no owner and
every redelivery held behind its own claim forever. Write the request
transition first and increment the count after it, retrying conflicts without
re-gating, and drop the unwind releases. A failure between the writes now
leaves the count low by one, which the release paths' zero clamp reconverges.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@sbalabanov
sbalabanov force-pushed the sergeyb/sp-oct-003-fail-open-admit branch from 9a60183 to 402380f Compare October 28, 2026 18:02

@mnoah1 mnoah1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do think we probably want to get to a future state that is using something more like the keyed slot claims that are present in #966 so we can have some more durable reconciliation. But good to close this gap for now.

@sbalabanov

Copy link
Copy Markdown
Contributor Author

I do think we probably want to get to a future state that is using something more like the keyed slot claims that are present in #966 so we can have some more durable reconciliation. But good to close this gap for now.

I am not sure which solution is better, I did not want the complexity of #966 so I've decided that those failures are rare enough for the gate to be made fail-open, and it is way less code to maintain.

If you think to reconsider towards #966 I will be ok with it. There are trade offs to both.

@sbalabanov
sbalabanov merged commit 5ebccb7 into main Oct 30, 2026
21 checks passed
@sbalabanov
sbalabanov deleted the sergeyb/sp-oct-003-fail-open-admit branch October 30, 2026 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants