Add the replay of issue 349: a merge acted on late superseded the merge after it (hq issue 349) #290

Merged

mesh-admin merged 2 commits from fix/349-replay into main 2026-10-13 03:54:10 +00:00

Conversation 3 Commits 2 Files Changed 2

+107

mesh-admin commented 2026-10-13 03:22:07 +00:00

Contributor

Copy Link

Copy Source

What. cmd/mesh-controller/replay349_test.go (TestReplay349), a test-only file for mesh-lab's R349, listed in testdata/store-tests.

Why. Issue 349 is a core issue and resolves with a replay (ADR 0237). On 2026-10-09 two catalogue merges were acted on in reverse order: the later one (the fix) was planned first, and the earlier one, handed over late, superseded its plan and planned the fixed module again at the commit without the fix. The fix was #179; ADR 0276 (#197) later replaced its mechanism with batches, and the replay holds the outcome under both.

What it asserts. Every plan left standing that plans the fixed module plans it at the commit holding the fix, and one does. It uses only what the controller had before #179 (aMesh, RegisterModule, following.SourceMoved, asksWithPaths, RecentPlans).

How it was tested. Against postgres 17: fails on c58516f8 (#179's first parent: the plan at 8ff8197a plans notes, the fix's plan superseded), passes on 9ca7952 (#179's merge) and on main. merge-check.sh passed with the store.

For issue 349 (replay R349; mesh-lab PR on the same branch name).

**What.** `cmd/mesh-controller/replay349_test.go` (`TestReplay349`), a test-only file for mesh-lab's R349, listed in `testdata/store-tests`. **Why.** Issue 349 is a core issue and resolves with a replay (ADR 0237). On 2026-10-09 two catalogue merges were acted on in reverse order: the later one (the fix) was planned first, and the earlier one, handed over late, superseded its plan and planned the fixed module again at the commit without the fix. The fix was #179; ADR 0276 (#197) later replaced its mechanism with batches, and the replay holds the outcome under both. **What it asserts.** Every plan left standing that plans the fixed module plans it at the commit holding the fix, and one does. It uses only what the controller had before #179 (aMesh, RegisterModule, following.SourceMoved, asksWithPaths, RecentPlans). **How it was tested.** Against postgres 17: fails on c58516f8 (#179's first parent: the plan at 8ff8197a plans notes, the fix's plan superseded), passes on 9ca7952 (#179's merge) and on main. `merge-check.sh` passed with the store. For issue 349 (replay R349; mesh-lab PR on the same branch name).

mesh-admin commented 2026-10-13 03:22:18 +00:00

Author

Contributor

Copy Link

Copy Source

Delivery novox/mesh-controller@5740874ce3b9 — delivered since 2026-10-13T03:54:17Z

Delivery plan — builds nothing: the change touches no module of the mesh's graph

Group fix/349-replay, in order: novox/mesh-controller@5740874ce3 → novox/mesh-lab@d16c3db9d2

• composed together: pass — the change touches no module of the mesh's graph

Transitions

• 2026-10-13 03:48 (new) → proposed (announced): novox/mesh-controller#290's head announced
• 2026-10-13 03:53 proposed → checked (checked): a verdict names this commit and the trunk's head as its trunk commit
• 2026-10-13 03:53 checked → ready (accepted): the gate passed or warned, and the repository's own check did not fail
• 2026-10-13 03:54 ready → published (merged): on the trunk its modules follow: merged there, and its walk opened — or nothing for a walk to move
• 2026-10-13 03:54 published → delivered (done): nothing for a walk to move

The commit's note under refs/notes/mesh-plan keeps every transition: git log --notes=mesh-plan.

<!-- mesh-delivery:view --> **Delivery** `novox/mesh-controller@5740874ce3b9` — **delivered** since 2026-10-13T03:54:17Z **Delivery plan** — builds nothing: the change touches no module of the mesh's graph **Group** `fix/349-replay`, in order: novox/mesh-controller@5740874ce3b9 → novox/mesh-lab@d16c3db9d2d0 - composed together: pass — the change touches no module of the mesh's graph **Transitions** - 2026-10-13 03:48 (new) → proposed (announced): novox/mesh-controller#290's head announced - 2026-10-13 03:53 proposed → checked (checked): a verdict names this commit and the trunk's head as its trunk commit - 2026-10-13 03:53 checked → ready (accepted): the gate passed or warned, and the repository's own check did not fail - 2026-10-13 03:54 ready → published (merged): on the trunk its modules follow: merged there, and its walk opened — or nothing for a walk to move - 2026-10-13 03:54 published → delivered (done): nothing for a walk to move The commit's note under `refs/notes/mesh-plan` keeps every transition: `git log --notes=mesh-plan`.

mesh-admin reviewed 2026-10-13 03:34:49 +00:00

mesh-admin left a comment

Author

Contributor

Copy Link

Copy Source

Verdict: request changes at f796dc0b73

1. The test uses only code from before #179. cmd/mesh-controller/replay349_test.go calls only aMesh, inv.RegisterModule, following{open: open}.SourceMoved, asksWithPaths, and inv.RecentPlans. None of these, nor the link.SourceMoved fields it sets (MergedAt, Paths, ModuleDirs, ModuleDirsSaid), were touched by #179 (c58516f8..9ca7952d changes only cmd/mesh-controller/{gate.go,issue348_test.go,issue349_test.go,machine_network.go,probes.go,release_plan.go,upgrades.go}, internal/catalogue/resolver_manifests_test.go, internal/conditions/*, migration 0086, and internal/inventory/plans.go, none of which this test file calls into for anything #179 added, such as NewestMergeOf or Plan.Merged). It compiled and ran unchanged at c58516f8.

2. Against a throwaway postgres 17 (MESH_TEST_POSTGRES), I laid the PR's test file over worktrees of all three commits and ran go test -run TestReplay349 -v ./cmd/mesh-controller/.... At c58516f8 (#179's parent): "--- FAIL: TestReplay349", with the log showing "plan-... building at 8ff8197a: app notes" -- the fixed module (notes) replanned at the earlier commit, without the fix. At 9ca7952d (#179's merge): "--- PASS: TestReplay349", the log showing the correction ("novox/mesh-catalog 8ff8197a was merged before a082615b ... what it moved is built from a082615b, which contains it") and the standing plan "building at a082615b: app notes". At main (584ee99, which already contains #197/ADR 0276's batches): "--- PASS: TestReplay349". This matches the PR's own report exactly, and on c58516f8 and 9ca7952d it genuinely replays the original fault -- a late, older merge superseding the newer merge's plan and building the older commit -- not something weaker.

3. The "passes ... on main" result is weaker evidence than the PR's "the replay holds the outcome under both" suggests, and this is why I am asking for a change. Under #197, following.SourceMoved no longer computes a plan synchronously; it only queues the merge into a batch (hearMerge). In the run against main, the first merge (notes' fix) is cut into its own plan at once and stays "building"; the second, earlier merge (app) ends up "queued behind" it with an empty Modules map and the note "plan not yet calculated", because ADR 0276 keeps one walk open at a time. TestReplay349's own assertion loop skips any plan that does not hold "notes", so it never looks at that still-uncalculated plan -- it passes because nothing currently holds "notes" except the already-correct first plan, not because the scenario was driven to the point where the batch mechanism's real decision (what commit the queued batch is cut at, once the first walk ends) was exercised. I confirmed main's actual mechanism does handle this case correctly by probing it directly: marking the first plan inventory.PlanDone and then calling cutBatchesHeld, the same way cmd/mesh-controller/batches_test.go's TestALateMergeIsAnsweredByTheWalkOfTheLaterOne does, cuts the second plan as "building at 8ff8197a: app" alone, while notes stays "done at a082615b: notes". But TestReplay349 itself never reaches that point, so it would not catch a regression there -- it is testing a different, and weaker, thing on main than it tests on 9ca7952d.

4. Ask: either drive the scenario to an actual resolution on main before asserting (mark the first plan Done and call cutBatchesHeld, accepting that this leans on post-#197 code for that half of the check), or narrow the PR's and the replay's claim to the pre-batches mechanism and point to the existing TestALateMergeIsAnsweredByTheWalkOfTheLaterOne (the same-module case) for main's coverage, rather than implying TestReplay349 itself proves main is safe.

5. Mechanically the rest is fine: testdata/store-tests gains exactly the one line "./cmd/mesh-controller TestReplay349", and go vet ./... is clean on main with the test file present.

Verdict: request changes at f796dc0b73075be13fefe80fe10d556ec9fa899e 1. The test uses only code from before #179. cmd/mesh-controller/replay349_test.go calls only aMesh, inv.RegisterModule, following{open: open}.SourceMoved, asksWithPaths, and inv.RecentPlans. None of these, nor the link.SourceMoved fields it sets (MergedAt, Paths, ModuleDirs, ModuleDirsSaid), were touched by #179 (c58516f8..9ca7952d changes only cmd/mesh-controller/{gate.go,issue348_test.go,issue349_test.go,machine_network.go,probes.go,release_plan.go,upgrades.go}, internal/catalogue/resolver_manifests_test.go, internal/conditions/*, migration 0086, and internal/inventory/plans.go, none of which this test file calls into for anything #179 added, such as NewestMergeOf or Plan.Merged). It compiled and ran unchanged at c58516f8. 2. Against a throwaway postgres 17 (MESH_TEST_POSTGRES), I laid the PR's test file over worktrees of all three commits and ran `go test -run TestReplay349 -v ./cmd/mesh-controller/...`. At c58516f8 (#179's parent): "--- FAIL: TestReplay349", with the log showing "plan-... building at 8ff8197a: [[app notes]]" -- the fixed module (notes) replanned at the earlier commit, without the fix. At 9ca7952d (#179's merge): "--- PASS: TestReplay349", the log showing the correction ("novox/mesh-catalog 8ff8197a was merged before a082615b ... what it moved is built from a082615b, which contains it") and the standing plan "building at a082615b: [[app notes]]". At main (584ee99, which already contains #197/ADR 0276's batches): "--- PASS: TestReplay349". This matches the PR's own report exactly, and on c58516f8 and 9ca7952d it genuinely replays the original fault -- a late, older merge superseding the newer merge's plan and building the older commit -- not something weaker. 3. The "passes ... on main" result is weaker evidence than the PR's "the replay holds the outcome under both" suggests, and this is why I am asking for a change. Under #197, following.SourceMoved no longer computes a plan synchronously; it only queues the merge into a batch (hearMerge). In the run against main, the first merge (notes' fix) is cut into its own plan at once and stays "building"; the second, earlier merge (app) ends up "queued behind" it with an empty Modules map and the note "plan not yet calculated", because ADR 0276 keeps one walk open at a time. TestReplay349's own assertion loop skips any plan that does not hold "notes", so it never looks at that still-uncalculated plan -- it passes because nothing currently holds "notes" except the already-correct first plan, not because the scenario was driven to the point where the batch mechanism's real decision (what commit the queued batch is cut at, once the first walk ends) was exercised. I confirmed main's actual mechanism does handle this case correctly by probing it directly: marking the first plan inventory.PlanDone and then calling cutBatchesHeld, the same way cmd/mesh-controller/batches_test.go's TestALateMergeIsAnsweredByTheWalkOfTheLaterOne does, cuts the second plan as "building at 8ff8197a: [[app]]" alone, while notes stays "done at a082615b: [[notes]]". But TestReplay349 itself never reaches that point, so it would not catch a regression there -- it is testing a different, and weaker, thing on main than it tests on 9ca7952d. 4. Ask: either drive the scenario to an actual resolution on main before asserting (mark the first plan Done and call cutBatchesHeld, accepting that this leans on post-#197 code for that half of the check), or narrow the PR's and the replay's claim to the pre-batches mechanism and point to the existing TestALateMergeIsAnsweredByTheWalkOfTheLaterOne (the same-module case) for main's coverage, rather than implying TestReplay349 itself proves main is safe. 5. Mechanically the rest is fine: testdata/store-tests gains exactly the one line "./cmd/mesh-controller TestReplay349", and `go vet ./...` is clean on main with the test file present.

jschoubben referenced this issue from a commit 2026-10-13 03:42:20 +00:00

Drive the replay of issue 349 to where main decides (review of #290)

mesh-admin commented 2026-10-13 03:43:05 +00:00

Author

Contributor

Copy Link

Copy Source

Merge check at ddbc02d4
Judged: head ddbc02d44c6d merged into trunk 1c9bc9cf0041

• mesh/merge-gate: FAIL — head ddbc02d44c does not merge cleanly into trunk 1c9bc9cf00: in conflict testdata/store-tests (modules: build-agent)
• mesh/repo-check: FAIL — head ddbc02d44c does not merge cleanly into trunk 1c9bc9cf00: in conflict testdata/store-tests

Delivery plan — builds build-agent → ace, g14, novox, shanks; no bus step

• tier 0: build-agent
• ace: receives build-agent
• g14: receives build-agent
• novox: receives build-agent
• shanks: receives build-agent
• build-agent keeps data (ADR 0233): what it holds is backed up before it moves
• grants step: if this changes what the bus's user list says (a new tool, subject or user), novox is sent that list alone, every build there kept, before build-agent is judged on its first machine

Run by the build seat on g14 as build-1791862968165642750 (builds --log build-1791862968165642750).

head ddbc02d44c6d does not merge cleanly into trunk 1c9bc9cf0041: in conflict testdata/store-tests

**Merge check** at `ddbc02d4` Judged: head `ddbc02d44c6d` merged into trunk `1c9bc9cf0041` - `mesh/merge-gate`: **FAIL** — head ddbc02d44c6d does not merge cleanly into trunk 1c9bc9cf0041: in conflict testdata/store-tests (modules: build-agent) - `mesh/repo-check`: **FAIL** — head ddbc02d44c6d does not merge cleanly into trunk 1c9bc9cf0041: in conflict testdata/store-tests **Delivery plan** — builds build-agent → ace, g14, novox, shanks; no bus step - tier 0: build-agent - ace: receives build-agent - g14: receives build-agent - novox: receives build-agent - shanks: receives build-agent - build-agent keeps data (ADR 0233): what it holds is backed up before it moves - grants step: if this changes what the bus's user list says (a new tool, subject or user), novox is sent that list alone, every build there kept, before build-agent is judged on its first machine Run by the build seat on g14 as `build-1791862968165642750` (`builds --log build-1791862968165642750`). ``` head ddbc02d44c6d does not merge cleanly into trunk 1c9bc9cf0041: in conflict testdata/store-tests ```

jschoubben referenced this issue from a commit 2026-10-13 03:48:40 +00:00

Drive the replay of issue 349 to where main decides (review of #290)

jschoubben added 2 commits 2026-10-13 03:48:40 +00:00

Add the replay of issue 349: a merge acted on late superseded the merge after it (hq issue 349) ... bbf70fbda4

Two catalogue merges acted on in reverse order left the fixed module planned at
the commit without the fix. The replay uses only what the controller had before
Listed among the store tests, so a check without a database cannot pass its skip.

Drive the replay of issue 349 to where main decides (review of #290) ... 5740874ce3

Since ADR 0276 the earlier merge waits in a queued batch with no modules, and the
replay skipped it, so its pass on main proved nothing. The replay now ends the
later merge's walk and has another repository's merge heard, which cuts the batch,
and fails on any plan of the two merges left standing with nothing planned.

jschoubben force-pushed fix/349-replay from ddbc02d44c to 5740874ce3 2026-10-13 03:48:40 +00:00 Compare

mesh-admin reviewed 2026-10-13 03:53:31 +00:00

mesh-admin left a comment

Author

Contributor

Copy Link

Copy Source

Verdict: approve at 5740874ce3

This addresses the gap from my previous review of this PR. I re-ran the test against a throwaway postgres 17 on worktrees of all three commits:

1. At c58516f8 (#179's parent): "--- FAIL: TestReplay349", log "plan-... building at 8ff8197a: app notes" -- the same original fault as before (notes replanned at the stale commit).

2. At 9ca7952d (#179's merge): "--- PASS: TestReplay349", log shows the plan corrected to "building at a082615b: app notes" via the fix's own commit substitution.

3. At main 1c9bc9c (which already contains #197/ADR 0276's batches): "--- PASS: TestReplay349". This time the pass is for the right reason: the test now ends the fix's plan (marks it inventory.PlanDone) and hears a merge of a different repository (novox/other), which drives following.SourceMoved's own internal batch-cutting to look again -- the log shows the queued batch for the earlier merge actually being cut ("plan-... cut: novox/mesh-catalog 8ff8197a ... building at 8ff8197a: app"), with notes staying "done at a082615b: notes". Both plans that matter are non-empty, so the assertion now exercises the real decision rather than skipping it.

4. I confirmed the new step is load-bearing: removing it (reverting to only the two original SourceMoved calls, same assertion) and rerunning on main 1c9bc9c reproduces the gap I originally flagged -- "--- FAIL: TestReplay349", with "plan-..., of the merge at 8ff8197aearlier, is left standing and plans nothing" and "no plan left standing builds app". So the stricter assertion (erroring on an empty Modules map, not skipping it) and the main-side step together are what make the pass on main meaningful.

5. No main-only functions are used: the test still only calls aMesh, inv.RegisterModule, following{open: open}.SourceMoved, asksWithPaths, inv.RecentPlans, inv.SavePlan, Plan.Open(), and the PlanDone/PlanSuperseded constants -- all of which predate #179 and compiled and ran unchanged at c58516f8 in step 1. It reaches main's batch cut only by calling the same pre-#179 SourceMoved entry point a third time (for another repository's merge), never by calling batches.go internals (e.g. cutBatchesHeld) directly.

6. go vet ./... is clean on main with the test file present, and testdata/store-tests gains exactly the one line "./cmd/mesh-controller TestReplay349" (git diff against main 1c9bc9c shows a clean, non-conflicting one-line addition).

Verdict: approve at 5740874ce3b99b47c27403f33e6c6d2a334ad3d7 This addresses the gap from my previous review of this PR. I re-ran the test against a throwaway postgres 17 on worktrees of all three commits: 1. At c58516f8 (#179's parent): "--- FAIL: TestReplay349", log "plan-... building at 8ff8197a: [[app notes]]" -- the same original fault as before (notes replanned at the stale commit). 2. At 9ca7952d (#179's merge): "--- PASS: TestReplay349", log shows the plan corrected to "building at a082615b: [[app notes]]" via the fix's own commit substitution. 3. At main 1c9bc9c (which already contains #197/ADR 0276's batches): "--- PASS: TestReplay349". This time the pass is for the right reason: the test now ends the fix's plan (marks it inventory.PlanDone) and hears a merge of a different repository (novox/other), which drives following.SourceMoved's own internal batch-cutting to look again -- the log shows the queued batch for the earlier merge actually being cut ("plan-... cut: novox/mesh-catalog 8ff8197a ... building at 8ff8197a: [[app]]"), with notes staying "done at a082615b: [[notes]]". Both plans that matter are non-empty, so the assertion now exercises the real decision rather than skipping it. 4. I confirmed the new step is load-bearing: removing it (reverting to only the two original SourceMoved calls, same assertion) and rerunning on main 1c9bc9c reproduces the gap I originally flagged -- "--- FAIL: TestReplay349", with "plan-..., of the merge at 8ff8197aearlier, is left standing and plans nothing" and "no plan left standing builds app". So the stricter assertion (erroring on an empty Modules map, not skipping it) and the main-side step together are what make the pass on main meaningful. 5. No main-only functions are used: the test still only calls aMesh, inv.RegisterModule, following{open: open}.SourceMoved, asksWithPaths, inv.RecentPlans, inv.SavePlan, Plan.Open(), and the PlanDone/PlanSuperseded constants -- all of which predate #179 and compiled and ran unchanged at c58516f8 in step 1. It reaches main's batch cut only by calling the same pre-#179 SourceMoved entry point a third time (for another repository's merge), never by calling batches.go internals (e.g. cutBatchesHeld) directly. 6. `go vet ./...` is clean on main with the test file present, and testdata/store-tests gains exactly the one line "./cmd/mesh-controller TestReplay349" (git diff against main 1c9bc9c shows a clean, non-conflicting one-line addition).

mesh-admin merged commit b12c4b82e8 into main 2026-10-13 03:54:10 +00:00

mesh-admin deleted branch fix/349-replay 2026-10-13 03:54:10 +00:00

Sign in to join this conversation.

Reviewers

No Reviewers

mesh-admin

Labels

No items

No labels

Milestone

No items

No Milestone

Assignees

Clear assignees

jschoubben miel.truyen

No Assignees

2 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: novox/mesh-controller#290