Issue 341: a merge check judges the pull request's head, not what merging it makes
Node-engine #66 passed both required statuses after #64/#65 had merged, and its merge did not compile: each side added Applied.Declared. The check clones the head alone and nothing marks a verdict stale when the trunk moves; ADR 0237/0238 say the head, so the fix needs a decision.
This commit is contained in:
jochen
@@ -0,0 +1,102 @@
|
||||
---
|
||||
status: located
|
||||
opened: 2026-10-09
|
||||
located-in: [mesh-controller cmd/mesh-controller (checks.go, the check request names the pull request's head as the ref), mesh-controller internal/builder (check.go, the check clones and checks out that ref and never merges the base), mesh-catalog modules/mesh-delivery (table.go, a ready delivery is rechecked only when a person asks or its group changes; holder.go, a merge on the trunk leaves the repository's other open deliveries as they were)]
|
||||
fixed-by:
|
||||
amended-design:
|
||||
---
|
||||
|
||||
# 341. A merge check judges the pull request's head, not what merging it makes
|
||||
|
||||
## Symptom
|
||||
|
||||
On 2026-10-09 a node-engine pull request passed its merge check, and its merge broke the trunk. The
|
||||
trunk then did not compile, so no build of it could be made.
|
||||
|
||||
What happened, in order (UTC):
|
||||
|
||||
1. **00:15.** Node-engine #64 and #65 (issue 339's follow-ups) merged. #65's merge commit, `d28ca95`, is
|
||||
the node-engine build the nodes run. Both pull requests added a field `Applied.Declared` to the
|
||||
node-engine's store record, a string, under the JSON key `declared`.
|
||||
2. **00:49.** Node-engine #66 (ADR 0269) was checked, at head `dfb1415`. That check was asked after the
|
||||
trunk had moved, so the check was not out of date by the clock. `mesh/merge-gate` passed ("4 of 4
|
||||
compose") and `mesh/repo-check` passed (its `merge-check.sh`, every package `ok`). Read with
|
||||
`mesh-delivery.checks` for the commit: both statuses `success`, set at 00:49:42, and "mergeable — every
|
||||
status main requires is a success".
|
||||
3. **00:54.** #66 merged, as `690966d`. #66 had added its own `Applied.Declared` field, a `*FoundUnit`,
|
||||
**under the same JSON key `declared`**. The two edits touched different lines, so git merged them
|
||||
without a conflict. The result does not compile:
|
||||
`internal/store/store.go:145:2: Declared redeclared`.
|
||||
4. **00:55.** The walk the merge started (`plan-1791507264215942736`) failed: "`mesh-host` failed to build
|
||||
in tier 0". `mesh/delivery` on the head reads `failure`. Nothing was sent to a node, and every node kept
|
||||
`d28ca95`.
|
||||
5. **01:09.** Node-engine #67 repaired the trunk. It renamed #66's field to `DeclaredUnit`, key
|
||||
`declared_unit`, and added a test that pins both keys.
|
||||
|
||||
The facts behind the "different lines" claim, read from the node-engine's repository: `dfb1415` does not
|
||||
contain `d28ca95`. Their common ancestor is `a2c2c35` (#63's merge). `dfb1415` holds one `Declared`
|
||||
(store.go line 107), `d28ca95` holds the other (line 139), and `690966d` holds both (lines 107 and 145).
|
||||
|
||||
No node was harmed, because a walk builds before it sends. But the merge check said "this may merge" of
|
||||
a merge that did not build, and both required statuses were green when it merged.
|
||||
|
||||
## The cost before this: checks that went stale
|
||||
|
||||
The same night, the session that merged the pull requests below had to ask for their checks again by
|
||||
hand, or rebase a branch, before the verdict was right (as that session reported it). Each time, another
|
||||
merge or the order of merges had changed what the check would see. What each pull request's own text
|
||||
shows:
|
||||
|
||||
- **Controller #165** (issue 335). Its body says `mesh/repo-check` "fails on
|
||||
`TestTheInstallersFirstUserListIsWhatTheControllerWouldCompose` until #159 merges". The head held the
|
||||
trunk of before #159. A check of the head alone stays red until the branch is rebased, even after #159
|
||||
is on the trunk.
|
||||
- **Controller #159** (issue 330). Node-engine #60's body says the controller's repository check "failed
|
||||
on exactly this test without it": the check reads the node-engine's trunk beside the controller, so #159
|
||||
waited for #60 to merge and then for a new check.
|
||||
- **Controller #148**. It was rebased so that its check would run on the current trunk.
|
||||
- **Controller #169 and catalogue #145** (ADR 0268), and **controller #171 and catalogue #147** (the zfs
|
||||
module). Each pair names a merge order in its body ("#169 first", "Do not merge before mesh-controller
|
||||
#171"). The second of each pair needed its check asked again once the first had merged.
|
||||
|
||||
In every case a person kept track of which verdicts the last merge had made stale. That is the work the
|
||||
merge check exists to remove.
|
||||
|
||||
## Why it is a design issue
|
||||
|
||||
The merge check is the rule that "a change is judged before it merges"
|
||||
([ADR 0237](../../02-DECISIONS/0237-a-change-is-judged-against-the-mesh-that-runs-before-it-merges-on-the-build-seat.md)).
|
||||
The forge requires its two statuses on the trunk
|
||||
([ADR 0238](../../02-DECISIONS/0238-a-commit-is-the-build-at-hand-one-commit-one-change-plan-checked-off-the-trunk-and-published-only-on-it.md)
|
||||
decision 5). A person and an agent both read "both green" as "this merge is safe". But the check judges
|
||||
something else: the pull request's head, alone. Two pull requests that each pass can break the trunk
|
||||
together, and a recheck after the trunk moved judges the same head and passes again.
|
||||
|
||||
Nothing says that a verdict stops counting when the trunk moves. The delivery's state table moves a ready
|
||||
delivery back to proposed only when a person asks or its group changes
|
||||
([ADR 0239](../../02-DECISIONS/0239-a-delivery-is-owned-by-the-mesh-delivery-module-and-runs-from-commit-to-delivered.md)).
|
||||
A merge of another pull request into the same trunk changes neither.
|
||||
|
||||
## What is expected
|
||||
|
||||
- A merge check judges exactly what merging would make: the pull request's head merged into the current
|
||||
trunk. The forge keeps no merge ref for a pull request (`git ls-remote` lists only
|
||||
`refs/pull/<n>/head`), so the build seat makes that merge itself in its clone. A head that does not
|
||||
merge cleanly is a failure that says so.
|
||||
- A verdict is tied to the trunk commit it was judged against. When the trunk moves, every open pull
|
||||
request's verdict into it is stale. The check either runs again by itself or is marked out of date (the
|
||||
required statuses back to pending), so that a merge is never let through on a verdict for a trunk that
|
||||
no longer exists.
|
||||
- The two required statuses then mean "this merge is green", which is what everyone already reads them as.
|
||||
|
||||
## The records say the head
|
||||
|
||||
ADR 0237 decision 4 has the build seat check "the head". ADR 0238 decision 1 says "A commit not on the
|
||||
trunk — a pull request's head, any branch — is only checked", and decision 2 computes the delivery plan
|
||||
from "a repository, the branch it merges into, the commit at hand". Neither record says the head is merged
|
||||
with its trunk first. **So the fix needs a decision record** that extends ADR 0237 §4 and ADR 0238 §1–§2
|
||||
(what is checked is the merge of the head into the trunk at a named trunk commit) and ADR 0239's state
|
||||
table (a trunk move makes a ready verdict stale). ADR 0238's publishing rule is not touched: a merge made
|
||||
to check a pull request is still a commit off the trunk, only checked, never registered.
|
||||
|
||||
The diagnosis is in [01-diagnosis.md](01-diagnosis.md).
|
||||
+102
@@ -0,0 +1,102 @@
|
||||
# 341 — Diagnosis
|
||||
|
||||
## 2026-10-09
|
||||
|
||||
**The records first.** `records_search` for "redeclared", "refs/pull" and "merged into its base" found
|
||||
nothing. "recheck" found [issue 305](../305-a-recheck-left-the-old-verdict-standing/00-report.md), which
|
||||
is close but not the same. 305 made a recheck put both statuses back to pending before the new check
|
||||
runs. It also recorded that "some of the rechecked pull requests turned red: judged against the trunk of
|
||||
that day, they no longer composed". So stale verdicts were seen on 2026-10-07 too. 305 fixed how a
|
||||
recheck is asked; it did not change what a check judges or when a verdict goes stale.
|
||||
|
||||
### What the check builds (mesh-controller at `36008e0`)
|
||||
|
||||
- `cmd/mesh-controller/checks.go`, around line 259: the build request for a pull request's check sets
|
||||
`Ref: p.Commit`, the pull request's head. The trunk goes in as `Check.Base`, a branch name.
|
||||
- `internal/builder/check.go`, line 52, the check's own description: it "clones the repository at the
|
||||
pull request's head".
|
||||
- `internal/builder/check.go`, lines 249–256 and 265: `clone` runs `git clone` and then
|
||||
`git checkout <ref>` with `spec.Ref`. Nothing merges the base into that tree.
|
||||
- `spec.Base` (declared at line 83, "what the gate compares the change against") is read only to pick
|
||||
the check script from the trunk (`TheCheckThatJudges`, line 270, issue 310's fix) and to show the
|
||||
touched manifests as they are on the trunk (around line 582). Neither builds the trunk with the change.
|
||||
- A delivery group's other heads (lines 297–310) are cloned beside the head, each at its own head. They
|
||||
are composed together for the merge gate. The repository's own `merge-check.sh` still runs on the head
|
||||
alone.
|
||||
|
||||
So `mesh/repo-check` ran `go test` on `dfb1415`, a tree with one `Declared`, and passed. The tree that
|
||||
merging made, with two, was never built before the merge.
|
||||
|
||||
### When a verdict goes stale (mesh-catalog at `63de373`)
|
||||
|
||||
- `modules/mesh-delivery/cmd/mesh-delivery/table.go`, line 60 and line 167: `EvRecheck` takes a rejected
|
||||
or ready delivery back to proposed. Its comment says "a person asked for it again, or its group
|
||||
changed".
|
||||
- `holder.go`, `PullMerged` (line 347): a merge records the merge commit on that pull request's own
|
||||
delivery and links its walk. It does not touch the repository's other open deliveries into the same
|
||||
trunk.
|
||||
- The forge marks a head pending only when it announces a new head (issue 305). A merge into the trunk is
|
||||
not a new head of any other pull request.
|
||||
|
||||
So after #64/#65 merged, #66's verdict was not marked stale by anything. The recheck at 00:49 was asked by
|
||||
a person. It judged the same head and passed again.
|
||||
|
||||
### What the records decided
|
||||
|
||||
- [ADR 0237](../../02-DECISIONS/0237-a-change-is-judged-against-the-mesh-that-runs-before-it-merges-on-the-build-seat.md)
|
||||
decision 4: the controller "asks the build seat a check: the head, and beside it the controller the mesh
|
||||
runs and its main, the catalogue, the node-engine it runs, and mesh-lab".
|
||||
- [ADR 0238](../../02-DECISIONS/0238-a-commit-is-the-build-at-hand-one-commit-one-change-plan-checked-off-the-trunk-and-published-only-on-it.md)
|
||||
decision 1: "A commit not on the trunk — a pull request's head, any branch — is only checked".
|
||||
Decision 2: a delivery plan is computed from "a repository, the branch it merges into, the commit at
|
||||
hand".
|
||||
- [ADR 0239](../../02-DECISIONS/0239-a-delivery-is-owned-by-the-mesh-delivery-module-and-runs-from-commit-to-delivered.md)'s
|
||||
state table: `ready → recheck → proposed` when "a person asked, or its group changed".
|
||||
|
||||
The code does what the records say. The records name the head and never the merge, and say nothing of
|
||||
a verdict going stale when the trunk moves. **This is a design gap, and its fix needs a decision record**
|
||||
(playbook 02), not only a code change.
|
||||
|
||||
### Ruled out
|
||||
|
||||
- **A flaky test or a toolchain difference.** The failure is a compile error that exists only in the
|
||||
merge. `dfb1415` and `d28ca95` each compile; `690966d` does not.
|
||||
- **A check run before the trunk moved.** The check that let #66 merge was set at 00:49:42, 34 minutes
|
||||
after #64 and #65 merged.
|
||||
- **The forge merging badly.** git merged the two edits without a conflict because they touch different
|
||||
lines. That is correct for git. A semantic conflict like this one is found only by building the merge.
|
||||
|
||||
### Owners
|
||||
|
||||
- **mesh-controller**: the check request and the builder's clone. The check builds the merge of the head
|
||||
into the trunk commit it names, and says that trunk commit with its verdict.
|
||||
- **mesh-catalog modules/mesh-delivery**: a trunk move makes the ready and rejected verdicts of the
|
||||
repository's other open deliveries into that trunk stale. They are proposed again, or held out of date,
|
||||
with the statuses back to pending (issue 305's path).
|
||||
- **The forge's module** (mesh-catalog modules/gitea), possibly: if the fix chooses the forge's own
|
||||
"outdated branch" protection instead, the protection is set there.
|
||||
|
||||
### Directions for the decision (not chosen here)
|
||||
|
||||
1. **The build seat merges, the delivery owner rechecks on a trunk move.** The check clones the trunk at
|
||||
a named commit, merges the head into it (refusing a conflict), and runs both layers on that tree. Every
|
||||
merge into a trunk proposes the open deliveries into it again. Cost: one check per open pull request
|
||||
into that trunk per merge, on the build seat. How many that is on a busy night is not measured yet.
|
||||
2. **The forge refuses an outdated branch.** Gitea's branch protection can block a merge when the pull
|
||||
request's branch is behind its base. The check then stays a check of the head, and the head always
|
||||
contains the trunk. Cost: every merge forces every other open pull request to rebase or merge the
|
||||
trunk, by a person or an agent, and each of those is a new head and a new check. That is the manual
|
||||
work the report lists, made mandatory.
|
||||
3. **Both**: the merge is what is checked, and a verdict carries the trunk commit it was judged against.
|
||||
The merge is refused while that commit is not the trunk's current one, until the automatic recheck
|
||||
answers.
|
||||
|
||||
A delivery group already composes its members' heads for the merge gate. Whatever is chosen should say
|
||||
how a group's composed check meets the trunk: merged into it, member by member in the group's order.
|
||||
|
||||
### Replay
|
||||
|
||||
This is a core issue (its `located-in` names the controller). It resolves with a replay in mesh-lab's
|
||||
register, or a `replay-none:` saying why none is possible. A replay is possible: two branches off one
|
||||
trunk commit, each adding the same field to one struct, each passing alone, and the second checked after
|
||||
the first merged. Before the fix the second passes; after it, the second fails with the compile error.
|
||||
Reference in New Issue
Block a user