4323527699
werkator build succeeded after 01:38
A commit that landed while a build of the same pool was still PENDING or RUNNING was skipped -- and then never built. The watcher read a branch's dueness from its lagging local ref, but fast-forwarded that very ref at the end of the same cycle, so from the next cycle on the skipped branch no longer looked changed and the "later cycle" its doc comment promised never came. The branch stayed behind origin until another push arrived or recoverOnStartup healed it at the next restart. The dueness is now remembered where it is decided: RepoWatch.deferredBranches holds the branches startBuildIfDue refused for a busy pool, and the next enqueue takes that set over and reconsiders them alongside the changed ones. A branch still waiting is put back by that same call, one gone from origin yields no head commit and drops out -- so the memory clears itself. WatcherTest covered only that *other* branches keep going while a build runs, never the next cycle of the same branch; both the queue-behind case and the forgetting of a vanished branch are covered now. release 1.11.2 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: mhoennig <michael@hoennig.de> Reviewed-on: #57
110 lines
6.1 KiB
Markdown
110 lines
6.1 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
|
|
|
|
A commit that lands while a build of the same pool is still `PENDING` or `RUNNING` was never built.
|
|
|
|
The watcher's poll cycle does three things in this order:
|
|
it works out which branches are due, it enqueues them, and it then fast-forwards the primary checkout's local branch refs to origin.
|
|
Dueness was read from those very refs: a local ref lagging behind its origin counterpart meant "this branch has new commits".
|
|
|
|
`startBuildIfDue` refuses a branch whose pool is still busy, and its doc comment promised that the branch waits for a later cycle.
|
|
But the fast-forward at the end of the same cycle moves the skipped branch's ref up to origin regardless.
|
|
From the next cycle on, the branch no longer looks changed, nothing calls `startBuildIfDue` for it, and the promised later cycle never comes.
|
|
The only record that the branch was due had just been erased by the step that was supposed to be harmless.
|
|
|
|
The branch then stays behind origin until either another push arrives or the instance restarts — `recoverOnStartup` heals the state, which is why the bug was easy to miss.
|
|
|
|
Observed on repository `werkjournal`, 2026-09-12:
|
|
|
|
| Commit | Pushed | Result |
|
|
|---|---|---|
|
|
| `3494793` | 17:30 | built, green, deployed |
|
|
| `6406853` | ~40 s later, while `3494793` was building | **never built** |
|
|
| `f20fc73` | later, pool idle | enqueued immediately |
|
|
|
|
The existing comment on `fastForwardLocalRefs` shows the coupling was known:
|
|
it explains why syncing *earlier* in the cycle would suppress the build it prepares.
|
|
That reasoning covers syncing too early within one cycle, not syncing after an enqueue was skipped.
|
|
|
|
## Non-Goals
|
|
|
|
- No change to the queue-behind policy itself: one build per pool at a time stays as it is, and a commit still waits rather than starting a second build.
|
|
- No change to `fastForwardLocalRefs`: the local refs keep catching up to origin every cycle, which build tools in the shared `.git` rely on.
|
|
- No persistence of the new memory across a restart — `recoverOnStartup` already re-enqueues branches whose latest build never finished.
|
|
- No queue of intermediate commits: as before, the branch is built at its current origin head, not once per skipped commit.
|
|
|
|
## The Scenarios
|
|
|
|
### Feature: a push during a running build is not lost
|
|
|
|
#### Background
|
|
|
|
- A *pool* is one branch and one build definition (`BuildDefinition.poolName`); the watcher runs at most one build per pool at a time.
|
|
- *Fast-forwarding local refs* moves the primary checkout's `refs/heads/*` up to `refs/remotes/origin/*` at the end of each poll cycle.
|
|
- A branch is *deferred* when its enqueue was refused solely because its pool was still `PENDING` or `RUNNING`.
|
|
|
|
#### Scenario#57.01: a commit pushed during a running build is built afterwards
|
|
|
|
So that no push is silently dropped just because it arrived at a busy moment.
|
|
|
|
- **Given** branch `main` has a `RUNNING` build at commit `commit-1`
|
|
- **and** origin's head of `main` is `commit-2`
|
|
- **When** a poll cycle runs
|
|
- **Then** no build is enqueued
|
|
- **And when** the running build has finished
|
|
- **and** the local ref of `main` has meanwhile been fast-forwarded to origin, so the ref comparison no longer reports new commits
|
|
- **and** a further poll cycle runs
|
|
- **Then** a build of `main` at `commit-2` is enqueued
|
|
|
|
##### Verified by
|
|
|
|
- [WatcherTest: a commit pushed during a running build is built in a later cycle, fast-forwarded ref or not](../../src/test/kotlin/de/hoennig/werkator/watcher/WatcherTest.kt)
|
|
|
|
#### Scenario#57.02: a deferred branch that disappears is forgotten
|
|
|
|
So that the new memory cannot grow without bound or keep asking about branches that no longer exist.
|
|
|
|
- **Given** branch `main` was deferred behind a running build
|
|
- **When** `main` is gone from origin
|
|
- **and** further poll cycles run
|
|
- **Then** no build is enqueued for it
|
|
- **and** the watcher stops carrying it as deferred
|
|
|
|
##### Verified by
|
|
|
|
- [WatcherTest: a branch deferred behind a running build is forgotten once it is gone from origin](../../src/test/kotlin/de/hoennig/werkator/watcher/WatcherTest.kt)
|
|
|
|
## The Solution
|
|
|
|
The dueness of a branch is no longer derived from the local ref alone; it is remembered where it is decided.
|
|
|
|
`Watcher.RepoWatch` — the per-repository poll memory — gains `deferredBranches`.
|
|
`startBuildIfDue` adds the branch to it in exactly the one branch of the method that refuses for a busy pool, and nowhere else:
|
|
a refusal for an already-built commit, a missing origin head or the pull-request gate is final, not a wait.
|
|
|
|
`enqueueDueBranches` takes the set over at the start of each cycle and clears it, then adds those branches to the changed ones.
|
|
They run through the same definition selection and the same `startBuildIfDue` as any changed branch, so nothing about the decision differs — they are merely reconsidered.
|
|
A branch whose pool is still busy is put back by that very call; a branch that is gone from origin yields no head commit, is refused, and simply drops out.
|
|
That makes the memory self-clearing: a branch is remembered exactly as long as it is waiting.
|
|
|
|
This is the second of the two options the bug report weighed, and it was chosen over excluding the skipped branches from the fast-forward.
|
|
It states the invariant instead of protecting an accidental one:
|
|
the watcher now knows a branch is due, rather than inferring it from a ref that other things are allowed to move —
|
|
which also holds with `watcher.fastForwardLocalRefs: false` and against ref movements the watcher did not cause.
|
|
|
|
The doc comments of `startBuildIfDue` and `fastForwardLocalRefs` are corrected in the same move.
|
|
The first promised a later cycle it could not keep; the second explained only half of its ordering constraint.
|
|
|
|
## Additional Changes
|
|
|
|
- The test gap the bug report names is closed:
|
|
`WatcherTest`'s "a poll cycle completes while a build is running and still enqueues other branches" only ever checked that *other* branches keep going,
|
|
never the next cycle of the *same* branch.
|
|
|
|
## Follow-up PRs
|
|
|
|
- None.
|