d48feb30d7
PR#65 set the draining flag after releasing the concurrency slot, so a build waiting on that semaphore could be woken by the release and start before it could read the flag — under the version on its way out, to be cut short by the restart and re-run by the recovery. That is the window the drain exists to close, one layer further in than the one PR#65 closed. The flag is now set before the slot is released; the wait for the executing builds stays after it. Found by Scenario#65.05 on the production host, which is slower and busier than the machine the change was written on, where it had passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: mhoennig <michael@hoennig.de> Reviewed-on: #66
3.2 KiB
WARNING: This document describes only the change applied in this PR. It may already be outdated once the next PR is merged. Historic PR-documentation is not maintained along with new PRs — treat it as a snapshot, not as current documentation.
Related Links
The Problem
PR#65 exists to close a window: a build that starts in the moment between "is the instance idle?" and the stop is cut short and re-run by the recovery. It left a smaller one of the same kind, one layer further in.
The executor set its draining flag after releasing the concurrency slot. A build waiting on that semaphore is woken by the release, and it reads the flag on the other side of the wake-up. Between the two lies a handful of instructions, and a worker that gets there first starts a build under the version that is on its way out — the outcome the drain exists to prevent.
The test for the queued build (Scenario#65.05) caught it on the first run on the production host, where the machine is slower and busier than the laptop the change was written on. It had passed locally.
Non-Goals
- No new behaviour: the drain, the pinned setting and the deployment script are unchanged.
The Scenarios
Feature: nothing starts under the outgoing version
Scenario#66.01: The flag is set before the slot is free
So that a build waiting for a slot can never observe it free while the drain is not yet declared.
- Given a deployment holds the only concurrency slot and another build is waiting for it
- When the deployment ends
SUCCESS - Then the waiting build sees the instance draining as soon as it acquires the slot
- and it stays
PENDINGand prepares no workspace
- and it stays
Verified by
The Solution
The flag is now set in the same finally, before builds.remove and before slot.release(), and the waiting for the executing builds stays where it was — after both.
beginDrain therefore splits along the line the ordering demands: the declaration is a compareAndSet in the release path, the wait is awaitDrained.
The order of the three statements is now load-bearing in both directions, which is worth saying out loud: the flag must come before the release, and the removal from the current builds must stay after the terminal status and the artifacts, for the reason it always did.
The scenario's test now makes the deployment hold the slot long enough for the second build to be genuinely waiting when it ends. That makes the situation real on every run; it does not make the old ordering fail deterministically, because the window is a few instructions wide. The fix is correct by construction, and the test is the scenario's check rather than a proof of the race.
Additional Changes
- None.
Prerequisite PRs
Follow-up PRs
- The two measurements RFC 0005 left open: CPU time per build, and
/proc/pressure/io.