Send an approved settings layer by a walk at once, so the operator's Approve takes effect (issue 641, ADR 0344) #305
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: novox/mesh-controller#305
Reference in New Issue
Block a user
What. On the operator's Approve of a settings proposal (ADR 0277), the warrant now sets the layer and opens a settings walk (
cmd/mesh-controller/settings_walk.go): a walk of the release kind (PlanRelease.Settings, kept in the plan's release JSON, no schema change) whose order is every machine running the module that the layer reaches (the named machine, or all for a whole-mesh layer), the control node last. No machine running it: no walk, and the warrant's words say there is nothing to send.Each step:
judgingIn);push <node> --move <module>(settingsDataMoves, composed as the send composes);sendJudgedunderkeepingBuildsForSettings(recorded_kept.go), which keeps every module there at the build it was last sent, whatever its policy. Arecordbuild (betterbird) stays held; a build waiting for a gate, or held after a failed release (ADR 0236 §24), is not released by it;judgeMoveswith the module as a judged pair), then the next machine.A machine not heard from is skipped and named; a walk that reached no machine ends failed, never delivered. A failed gate stops the walk, keeps the layer as approved, and names the machines not sent. A failed settings walk does not hold back the backlog's release (
releaseBacklogskips it); an open one makes it wait. The walk's start and end are said onplan-moved(ADR 0308), so the messenger tells its end ("Delivered: betterbird on g14, in 1m", or the failure with why).The ask's explanation and its Approve option now say the layer is set and sent at once to where it reaches, each machine judged, every build kept, and that its end is told as every walk's is;
settings proposeanswers the same. Nopushis named. A walk that cannot be opened fails the act loudly, withpush <node>as the hand remedy.settings setby hand opens no walk.Why. Issue 641: proposal s09bf8c99da7ac8b9 (betterbird,
calendar.alarms.showmissedfalse) was approved on Telegram at 16:13; the layer was set and nothing sent. ADR 0277 §18 kept the push a separate act. ADR 0344 (proposed, needs the operator's Approve) changes that.Not here. Betterbird reads prefs only at its start and declares no service, so
restart-ondoes not apply; a running Betterbird keeps the old values until restarted: issue 643 (mesh-catalog modules/betterbird).Tests (
settings_walk_test.go; they fail on main: the identifiers do not exist, and the ask test alone, run on main, fails every wording check):TestASettingsWalkGoesToTheMachinesTheLayerReachesTheControlNodeLast,TestASettingsWalkWaitsWhileAnotherWalkJudgesOnTheMachine(pure)TestTheAskSaysAnApprovedLayerIsSentAtOnceTestAnApprovedLayerOpensTheWalkThatSendsIt(store): terminalsettings setopens none; the warrant opens one with the right order; a layer nobody runs opens none; an unopened walk fails the actTestASettingsWalkSendsOneMachineAtATimeEachJudged(store)TestASettingsWalkSendsTheLayerAloneEveryBuildKept(store): every send under the keep; a waiting rolled-out build kept at c1TestASettingsWalkStopsBeforeADataMoveAndPassesOverAMachineAway(store)TestAFailedSettingsWalkStopsAndHoldsNothingBack(store): the layer stays; a waiting build is still releasedRun locally on g14 (niced, no race): gofmt clean,
go vet ./...;go testof./cmd/mesh-controller/and./internal/inventory/against a throwaway PostgreSQL 17 pass on the head; the rest of the suite passed on the first head (cmd/mesh-builder flaked once under load, passed on rerun; untouched).Overlaps. #202 (issue 383) rewrites the same ask sentence in
proposals.gointo the Details; whichever merges second rebases (keeponApprove()'s wording). #303 (issue 630) removes whole-mesh module layers;settingsWalkOrderhandles both.Design: novox/hq#346. Fixes issue 641.
Delivery
novox/mesh-controller@8bbb1b19ae63— delivered since 2026-10-13T17:30:14ZDelivery plan — builds build-agent, mesh-controller → ace, g14, novox, shanks; no bus step
Machines
Transitions
The commit's note under
refs/notes/mesh-plankeeps every transition:git log --notes=mesh-plan.Merge check at
e324f5d8Judged: head
e324f5d82162merged into trunk6f1a05366c1d; read beside it: mesh-catalog5f491e0c097d, mesh-controller-main6f1a05366c1d, mesh-host0e92ef3d1b21, mesh-tools954e1a53eef9mesh/merge-gate: PASS — every machine composes with the change as it did without (4 of 4 compose) (modules: build-agent, mesh-controller)mesh/repo-check: PASS — its merge-check.sh passedDelivery plan — builds build-agent, mesh-controller → ace, g14, novox, shanks; no bus step
Run by the build seat on ace as
build-1791902410496412263(builds --log build-1791902410496412263).Merge check at
1219ec82Judged: head
1219ec822943merged into trunk6f1a05366c1d; read beside it: mesh-catalog4d15a155e634, mesh-controller-main6f1a05366c1d, mesh-host0e92ef3d1b21, mesh-tools954e1a53eef9mesh/merge-gate: PASS — every machine composes with the change as it did without (4 of 4 compose) (modules: build-agent, mesh-controller)mesh/repo-check: PASS — its merge-check.sh passedDelivery plan — builds build-agent, mesh-controller → ace, g14, novox, shanks; no bus step
Run by the build seat on shanks as
build-1791903030659654048(builds --log build-1791903030659654048).Merge check at
e32ca00aJudged: head
e32ca00aed9dmerged into trunk6f1a05366c1d; read beside it: mesh-cataloge209d46faf3d, mesh-controller-main6f1a05366c1d, mesh-host0e92ef3d1b21, mesh-tools954e1a53eef9mesh/merge-gate: PASS — every machine composes with the change as it did without (4 of 4 compose) (modules: build-agent, mesh-controller)mesh/repo-check: PASS — its merge-check.sh passedDelivery plan — builds build-agent, mesh-controller → ace, g14, novox, shanks; no bus step
Run by the build seat on novox as
build-1791903374200877480(builds --log build-1791903374200877480).Verdict: approve at
e32ca00aedIndependent review of issue 641 under ADR 0344 (accepted, operator-approved, v8).
Checks at this head: mesh/merge-gate success (PASS, 4 of 4 compose; modules build-agent, mesh-controller), mesh/repo-check success (merge-check.sh passed), mesh/delivery success (ready: waits for its merge). No mesh/delivery-group status is set on this head. Locally (niced): go vet ./... clean; go test ./... all ok. Without MESH_TEST_POSTGRES the five store-backed settings-walk tests SKIP locally (TestAnApprovedLayerOpensTheWalkThatSendsIt, ...OneMachineAtATimeEachJudged, TestAFailedSettingsWalkStopsAndHoldsNothingBack, ...TheLayerAloneEveryBuildKept, ...StopsBeforeADataMove...); the three store-free ones pass. The repo-check ran the store.
What holds:
push <node> --move <module>, the later machines named not sent (settings_walk.go:180-195). Nothing is moved.settings proposeno longer names push.Findings, none blocking; I suggest one follow-up issue for 1 and 2:
Overlap with #304 (issue 639): one conflict hunk in settingsProposal.ask (proposals.go ~line 160-190). #304's new summary() also says 'approved, the layer is set at once and takes it at its next push', and its tests assert that text. That sentence becomes false under ADR 0344 §5. My recommendation: merge this PR first, then rebase #304. The rebase is small but not mechanical. It resolves the hunk, rewrites summary() for both set and clear to the walk wording (p.onApprove / p.reaches), and updates #304's expected strings. #304's reviewer should check that no 'next push' wording is left.
Merge check at
8bbb1b19Judged: head
8bbb1b19ae63merged into trunk6f1a05366c1d; read beside it: mesh-catalog9c166398f204, mesh-controller-main6f1a05366c1d, mesh-host0e92ef3d1b21, mesh-tools954e1a53eef9mesh/merge-gate: PASS — every machine composes with the change as it did without (4 of 4 compose) (modules: build-agent, mesh-controller)mesh/repo-check: PASS — its merge-check.sh passedDelivery plan — builds build-agent, mesh-controller → ace, g14, novox, shanks; no bus step
Run by the build seat on shanks as
build-1791905603074859821(builds --log build-1791905603074859821).Verdict: approve at
8bbb1b19aeRe-review of the diff since
e32ca00a(one commit,8bbb1b1).Checks at this head: mesh/merge-gate success (PASS, 4 of 4 compose; build-agent, mesh-controller; judged on shanks), mesh/repo-check success (merge-check.sh passed), mesh/delivery success (ready: waits for its merge). No mesh/delivery-group status is set. Locally (niced): go vet ./... is clean. go test ./... passes against a throwaway PostgreSQL (MESH_TEST_POSTGRES set), and all 18 settings tests pass with the store, the two new ones included.
My earlier findings:
Nit, not blocking: the errors.Is(err, errSettingsCannotKeep) check after sendJudged cannot match. sendToEach flattens a machine's refusal into a string ("%s:\n%v"), so the wrap is lost. The same composition runs first in settingsDataMoves and catches the condition there, so this costs nothing today. Either drop the check or wrap with %w in sendToEach if it should hold on its own.
The finding 4 note and the merge order with #304 stand as in my earlier review: merge this PR first, then rebase #304 and reword its 'takes it at its next push' summary.
Verdict: request changes at
8bbb1b19aeThis replaces my approve in review 219. The code still stands as approved; one change is needed to its check.
merge-check.sh has no name filter or build tag that excludes the settings-walk tests. The repo-check sets MESH_TEST_POSTGRES, so the whole-suite step (
go test -race -count=1 ./..., merge-check.sh:58) runs them against the store, and a failure there would fail the check. The repo-check's 'store tests, each run against the store' section names only the tests in testdata/store-tests (merge-check.sh:64-116), and this PR adds none to that list. So nothing proves that the seven store-backed tests ran rather than skipped. A skip passes unseen, which is exactly what issue 459 guards against. The script itself says: 'A pull request adds its own store tests to the list.'Requested: add these lines to testdata/store-tests:
./cmd/mesh-controller TestAnApprovedLayerOpensTheWalkThatSendsIt
./cmd/mesh-controller TestASettingsWalkSendsOneMachineAtATimeEachJudged
./cmd/mesh-controller TestAFailedSettingsWalkStopsAndHoldsNothingBack
./cmd/mesh-controller TestASettingsWalkSendsTheLayerAloneEveryBuildKept
./cmd/mesh-controller TestASettingsWalkStopsBeforeADataMoveAndPassesOverAMachineAway
./cmd/mesh-controller TestACrossingSendSupersedesASettingsGate
./cmd/mesh-controller TestASettingsWalkFailsOnARefusalThatCannotClear
All seven pass locally against a throwaway PostgreSQL at this head. Once they are listed and the repo-check names each as PASS, I will approve the new head. Nothing else changes from review 219.