fcd11d960e
"with maxConcurrent 1 a second branch stays PENDING until the first
finished" failed on CI on a branch that does not touch BuildExecutor, and
the message named the cause: expected:<PENDING> but was:<SUCCESS>. Not
merely started -- already finished.
The test started two builds of two branches back to back and assumed the
first one started would get the single slot. Nothing provides that. Each
branch has a worker thread of its own, both call slot.acquire() inside
execute, and RUNNING is recorded only afterwards; which thread arrives
first is the scheduler's business. Semaphore fairness orders threads
already waiting, not two threads racing to arrive. So branch-b took the
slot, ran `echo ok` to the end, released it, branch-a went RUNNING -- the
state the test was waiting for -- and the next line found branch-b
finished. Starting the two in the other order reproduces it every time.
The gate file from 9221550 could never cover this: it holds branch-a only
once branch-a already has the slot.
branch-b is now enqueued after branch-a is observed RUNNING, which is
proof the slot is taken, and the gate keeps it taken. No window is left,
and nothing is assumed about scheduling. It is also the case the test is
named after and the one that happens in production: a push arriving while
another branch builds. The transitions assertion at the end, which is the
real requirement, is unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: mhoennig <michael@hoennig.de>
Reviewed-on: #60
4.6 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.
The Problem
BuildExecutorTest's "with maxConcurrent 1 a second branch stays PENDING until the first finished" fails now and then.
It failed on 2026-09-13 on the CI instance, on the branch of PR#59, which does not touch BuildExecutor at all.
The same machine built the commit before it and the commit after it green, and the test class passes five forced runs in a row locally.
The recorded failure names the cause exactly:
org.opentest4j.AssertionFailedError: expected:<PENDING> but was:<SUCCESS>
at BuildExecutorTest.kt:571
SUCCESS, not RUNNING: the second build had not merely started, it had already finished.
The test starts two builds of two different branches back to back and then waits for the first to be RUNNING before asserting that the second is still PENDING.
That assumes the build started first gets the single executor slot, and nothing provides that:
startBuildsubmits each build tobranchWorkers.computeIfAbsent(repo to branch), a single-thread worker per branch, so the two builds run on two independent threads.- Each thread calls
slotsFor(...).acquire()insideexecute, and only then recordsRUNNING. - Which of the two threads reaches
acquire()first is up to the scheduler.Semaphore(maxConcurrent, true)is fair, but fairness orders threads that are already waiting; it does not order two threads racing to arrive.
So on the CI machine branch-b acquired the slot first, ran echo ok to completion and released it; branch-a then acquired it and went RUNNING, which is what the test was waiting for; and the assertion in the next line read branch-b as SUCCESS.
The gate file that commit 9221550 introduced ("test: gate the maxConcurrent-1 test instead of racing a sleep") cannot prevent this.
It holds branch-a only once branch-a already has the slot, and the failure happens before that.
The race was reproduced deterministically here by starting the two builds in the other order, which fails with the very same message.
Non-Goals
- No change to the executor. Two builds of different branches competing for the last free slot have no promised order, and none is needed: a CI does not owe callers first-come-first-served across branches, and the per-branch worker already keeps one branch's builds in order.
- No fix by waiting longer or by asserting repeatedly. The old assertion was not too early, it was asserting something the executor never guaranteed.
- No change to the sibling tests. "with maxConcurrent 2 two branches build at the same time" has a slot for each build, and "a second build of the same branch waits even when a slot is free" enqueues both builds on the same per-branch worker, which is FIFO by construction.
The Scenarios
Feature: one build slot, a second branch waiting
Scenario#60.01: a build enqueued while another one runs stays PENDING
So that the promise of executor.maxConcurrent is tested, not the thread scheduler.
- Given a build of
branch-athat isRUNNING, holding the only slot - When a build of
branch-bis enqueued - Then it is recorded
PENDING - And when
branch-afinishes - Then
branch-bstarts, and itsRUNNINGtransition is recorded afterbranch-a'sSUCCESS
Verified by
The Solution
The second build is enqueued only once the first is observed RUNNING.
RUNNING is recorded after slot.acquire() returns, so observing it is proof that the slot is taken, and the gate file keeps it taken until the test says otherwise.
There is no longer a window in which branch-b could acquire anything, and no assumption about which thread wins a race.
That is also the case the test is named after, and the one that happens in production: a push arrives while a build of another branch is running.
The final assertion is unchanged and remains the actual requirement — the recorded transitions must show branch-b starting after branch-a finished.
The comment above the test now says which race the gate does and does not cover, so the next reader does not have to rediscover it from a CI log.
Open Questions
- A test-only change still takes a version and a release-notes entry, per the rule introduced with v1.8.0. The comparable earlier fix, PR#46, predates that rule and deliberately took neither.