fcd11d960e
werkator build succeeded after 01:39
"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
74 lines
4.6 KiB
Markdown
74 lines
4.6 KiB
Markdown
> **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:
|
|
|
|
- `startBuild` submits each build to `branchWorkers.computeIfAbsent(repo to branch)`, a single-thread worker **per branch**, so the two builds run on two independent threads.
|
|
- Each thread calls `slotsFor(...).acquire()` inside `execute`, and only then records `RUNNING`.
|
|
- 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-a` that is `RUNNING`, holding the only slot
|
|
- **When** a build of `branch-b` is enqueued
|
|
- **Then** it is recorded `PENDING`
|
|
- **And when** `branch-a` finishes
|
|
- **Then** `branch-b` starts, and its `RUNNING` transition is recorded after `branch-a`'s `SUCCESS`
|
|
|
|
##### Verified by
|
|
|
|
- [BuildExecutorTest — with maxConcurrent 1 a second branch stays PENDING until the first finished](../../src/test/kotlin/de/hoennig/werkator/build/BuildExecutorTest.kt)
|
|
|
|
## 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.
|