Let a requirement be optional, binding only a provider that already runs (hq issue 166, ADR 0330) #281
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: novox/mesh-controller#281
Reference in New Issue
Block a user
What
A manifest's requirement may be written
{"name": "influxdb-api", "optional": true}(ADR 0330 §1). A bare name stays hard.internal/catalogue/resolve.go): a name only optional consumers want is settled after the walk. It binds exactly as a hard requirement when a provider is assigned or reachable (§2). Otherwise it is absent: not refused, and nothing is installed for it, not even the only catalogue candidate, on this machine or elsewhere (§3). A name another module wants hard (including one pulled in later) still resolves and installs as before.Resolution.Absentlists each absent requirement by consumer.declaration.go,bound_into_files.go):${bound:<provision>:<key>|default:<value>}renders its default while the requirement is absent (§4). A file markedonly-when-bound: <requirement>is left out while it is absent (§5), and the field is dropped from every resource before sending. Binding, credential and grant files of an absent requirement are not composed (§6).optional_requirements.go): refuses an optional${bound:…}placeholder without a default outside an only-when-bound file, and a${secret:…}of an optional requirement outside one. It also refusesonly-when-boundon a hard or unrequired name or on a non-file, and a default on a hard requirement.cmd/mesh-controller/unbound.go,plan.go,nodes.go): each listunbound <requirement>, optional for <consumer>. Forplan --json/--diffthe line goes to stderr, which the verb answer carries, because the declaration envelope is parsed strictly by the node-engine (§7).modulesmarks a requirement(optional).Why
Issue 166: soft dependencies became hard provisions, so grafana could not be assigned on a mesh without influxdb, and with influxdb in the catalogue it would have been installed for grafana.
How it was tested
internal/catalogue/optional_requirements_test.gocovers:cmd/mesh-controller/unbound_test.go(store test, added totestdata/store-tests): with grafana assigned and influxdb only in the catalogue,plan laptopandnode show laptopboth sayinfluxdb-apiis unbound and influxdb is not installed. Once influxdb is assigned on anchor, grafana binds to it with no act on grafana.MESH_TEST_POSTGRES=… sh merge-check.shpasses locally.Design limits chosen
only-when-boundapplies to files only (ADR 0330 §5 also says "contribution"). Acontributesentry to an optional requirement is already composed only while it binds, because it goes to whoever answers it. No seat contribution in the catalogue needs the mark yet.|default:}is not a placeholder, and the sweep refuses it as a misspelling.Fixes issue 166.
The catalogue PRs (mesh-catalog: grafana, home-assistant; mesh-media-catalog: ombi) depend on this one shipping first.
Delivery
novox/mesh-controller@79a3396340a7— delivered since 2026-10-13T00:15:31ZDelivery 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
bc81029aJudged: head
bc81029a030amerged into trunk56d6313262e8; read beside it: mesh-catalog8f80cd471634, mesh-controller-main56d6313262e8, mesh-host7a41e120635b, 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: FAIL — its merge-check.sh failed: --- FAIL: TestTheInstallersFirstUserListIsWhatTheControllerWouldCompose (0.00s)Delivery plan — builds build-agent, mesh-controller → ace, g14, novox, shanks; no bus step
Run by the build seat on shanks as
build-1791844826150324029(builds --log build-1791844826150324029).Verdict: request changes at
bc81029a03Independent review for hq issue 166 (ADR 0330). I am asking for one change: the form a registered manifest is stored in (finding 4). Everything else I checked holds, and the evidence is below.
1. Today's mesh composes byte-identically. Proven. I merged this head into main
56d6313locally; it merges cleanly. I took the facts snapshot of 2026-10-12 22:43:48Z and composed every machine twice with main and once with the merge. Each run used the gate's own path (raiseFromFacts, then composedAndValidated, thensendable.Body()) against a throwaway Postgres. Both controllers produced the same resource ids in the same order on all 4 machines: 385, 718, 540 and 361 resources. The only bytes that differ are those that also differ between two runs of main: sealed values, generated secrets, and 5 file contents whose only difference is a random token. With those masked, main and the merge are identical. The node-engine's validator refuses nothing in either.2. Resolution works as decided. The tests cover each case, and I mutated the code to show each test catches its case. Optional with no provider: absent, nothing installed, on both passes. Optional with a provider elsewhere: binds as a hard requirement would. One consumer's optional want and another's hard want: still refused, or installed when a module pulled in later wants it hard. On real data, I made a copy of the snapshot with influxdb, lidarr, mosquitto and nodered unassigned on ace, and laid catalogue #259 and media #36 over it. ace composes and validates, and
planlists four unbound lines. With nodered left assigned, mqtt-topic is refused because nodered wants it hard. That is correct.3.
only-when-boundis stripped before sending.withoutAbsentdrops the field from every resource it keeps. In every composed body (the snapshot as it is, with #259 and #36, and the absent copy) no resource carriesonly-when-bound.4. Rollback to an older controller: not acceptable as written. Keep a plain-name copy.
Inventory.Catalogue()unmarshals every stored manifest and returns an error on the first one it cannot read. I checked the controller at main against both forms:cannot unmarshal object into manifestFields.requires.1 of type string;{"requires":["a","b"],"optional-requires":["b"]}reads cleanly, because the extra key goes tounknown.So once #259 or #36 is registered, a controller older than this one cannot resolve any machine. Every plan and every push fails, and the controller cannot heal itself. A merge gate catches a revert, but a person's
pushor a rebuild of an older controller build does not go through one, and on 2026-10-02 an older controller build did reach the control node.The fix is small and stays inside the controller: have
MarshalJSONwriterequiresas plain names plus anoptional-requireslist, and haveUnmarshalJSONread that as well as the long form. The catalogue keeps the long form in its source files. An older controller would then treat the optional requirements as hard, which composes today, because each one is bound on ace. At worst it refuses or installs for that one module, as it did before ADR 0330. That is far less than every machine failing to compose. Please add a test that stores a manifest with an optional requirement and reads it back through the old field shape.5. Grafana's data source on ace is covered on #259: it keeps its uid, and the files move with the old paths removed as undeclared.
6. The tests fail without the changes. I took out each part of the change in turn and ran the tests:
withoutAbsentturned off: 2 tests fail;OptionalProblemstaken out of the module check: the module-check test fails;unboundLinesemptied: TestPlanAndNodeListAnOptionalRequirementLeftUnbound fails, for both plan and node.Checks. I read every status on this head.
mesh/merge-gatepassed at 22:46 against trunk56d6313("builds build-agent, mesh-controller → ace, g14, novox, shanks"). It has been marked out of date since trunk moved tocbac703.mesh/repo-checkfailed at 22:46 onTestTheInstallersFirstUserListIsWhatTheControllerWouldCompose, and this change did not cause it. mesh-host 37947ca (hq issue 146, ADR 0331) removedexamples/foundation-first-node-nats.lock, butinternal/broker/genesis_template_test.goon controller main (cbac703too) still reads that file. Against mesh-host main as the clone beside the check, main fails the same way. Every controller pull request will be red on this until that test follows mesh-host. Locally, without that clone (the captured copy in testdata/beside), merge-check.sh against a throwaway Postgres 18 passes: gofmt, go vet, andgo test -raceover the whole suite, with all 37 listed store tests passing, including the new one. The first full run failed once oninternal/leaseTestKeepingRenewsAcrossManyAges ("context deadline exceeded" under load). That package passed 3 of 3 runs on its own, and the second full run passed. It is a flake unrelated to this change.Not blocking:
because[want], which is the first module that queued the name. On the absent copy it said: nothing in this mesh provides "mqtt-topic", wanted by home-assistant. home-assistant wants it only optionally, and nodered is the module that wants it hard. Name a hard wanter when there is one.only-when-boundfile, or the bound and secret ids of an absent requirement. The node-engine accepts this and reads an empty digest, so binding later restarts the container, which is the behaviour we want. A line in the module-check comment would stop the next reader wondering.only-when-bound. The check refuses the field on anything but a file. A contribution to an absent requirement has no provider to receive it, so the result is the same, but the comment in OptionalProblems should say so.Review 112 addressed at
f0a62c1.MarshalJSONnow writesrequiresas a plain list of names, with the optional ones in a separate key,optional-requires.UnmarshalJSONreads both shapes: the{name, optional}entry and the separate list. The module check refuses anoptional-requiresentry the module does not require.TestTheStoredFormReadsWhereRequiresIsAListOfNamesunmarshals the stored form into main'sstruct{ Requires []string }. It also reads it back here, both leniently and throughParseManifest.catalogue.Manifest. The error is nil and requires is[route influxdb-api]. The unknownoptional-requireskey is set aside, so only that module is left out and the catalogue still reads.Catalogue,Catalogued, snapshots for the gate,upgradeFrom), and the store always holds this controller'sMarshalJSONform. A catalogue file is only read through the strictParseManifest. On an older controller that refuses the file whichever form it uses (a type error or an unknown key), and only that one registration. So the files keep ADR 0330's{name, optional}form.wanted by telegraf, tested). Before, it named the optional consumer.only-when-boundon contributions: not built. Acontributesentry is keyed by the requirement and goes only to whoever answers it, so it is already composed only while the requirement is bound. A seat contribution is not tied to a requirement, and none in the catalogues needs it. I can add it when one does.MESH_TEST_POSTGRES=… sh merge-check.shexit 0, including the store testTestPlanAndNodeListAnOptionalRequirementLeftUnbound. I judged it locally because repo-check is red on main for the unrelated genesis bundle.Merge check at
f0a62c1bJudged: head
f0a62c1b473fmerged into trunkcbac703e817d; read beside it: mesh-catalog8f80cd471634, mesh-controller-maincbac703e817d, mesh-host7a41e120635b, 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: FAIL — its merge-check.sh failed: --- FAIL: TestTheInstallersFirstUserListIsWhatTheControllerWouldCompose (0.00s)Delivery plan — builds build-agent, mesh-controller → ace, g14, novox, shanks; no bus step
Run by the build seat on shanks as
build-1791847135848669029(builds --log build-1791847135848669029).Out of date when it landed: out of date: judged against trunk
cbac703e81, the trunk is now 7b2a9eae8020; judged again on a recheck or when it is mergedVerdict: approve at
f0a62c1b47Re-review against my review 112, for hq issue 166 (ADR 0330). The rollback finding is resolved, and nothing new came up.
The stored form now reads on an older controller. I proved it end to end. This head strict-parsed the files of #259 (grafana, home-assistant) and #36 (ombi), long form included, and marshalled them as registration stores them. The result is a plain
requireslist plusoptional-requires(grafana:["route","oidc-client","influxdb-api"]and["influxdb-api"]). I then read those bytes with the controller at main (cbac703) through its ownManifest.UnmarshalJSON, the decoderInventory.Catalogue()uses. All three read without error, with the fullrequires, andUnknownField()namesoptional-requires. After a rollback, those three modules are therefore left out of every machine by name under ADR 0262: nothing of them changes on their machines, and a condition is raised. The rest of the catalogue resolves. That is the rollback behaviour the mesh already designed for, and much better than every plan failing. (It is not "read as hard", as I suggested in 112. Leaving the module out is the safer of the two.)The builder's claim holds: no path in main reads a catalogue file leniently. At main, every place that reads a module.json from a tree uses the strict
catalogue.ParseManifest: the builder (internal/builder/builder.go:224),module check(check.go, modules.go), the merge gate'sshelfWithChange, and build results (build.go:185, 614, 692, 783). After a rollback, those paths refuse the long form loudly, for that module alone. The lenientjson.Unmarshalreads are all of stored bytes:Inventory.Catalogue,upgradeFromand the bus-provider check in inventory/catalogue.go, and the facts snapshot'sshelfOfFactsandgraphOfFacts. With this head, every stored byte is written byMarshalJSONin the plain form.withReferencesreads into a generic map and hands back the bytes unchanged, andlink.ModuleOfreads onlymodule. So keeping the long form in #259 and #36's files is fine.Today's mesh still composes byte-identically. I merged this head into main
cbac703locally; it merges cleanly. I took a fresh facts snapshot (23:09:52Z), composed all 4 machines twice with main and once with the merge, and compared the wire bodies. The resource ids and their order are the same (385, 718, 540 and 361 resources). With the values that differ between two runs of main masked (sealed values, generated secrets, and 5 contents that differ only in a random token), there are 0 differences. The node-engine's validator refuses nothing.The refusal now names a hard wanter. On my copy of the snapshot with influxdb, lidarr and mosquitto unassigned on ace (#259 and #36 laid over it), it reads: nothing in this mesh provides "mqtt-topic", wanted by nodered. It used to name home-assistant. With nodered unassigned as well, ace composes and validates, with the same 4 unbound lines as before. The rename only fires when the first wanter wanted the name optionally, because
because[r]is set fromWants(), so a mesh without optional requirements is untouched. The byte-identity result confirms this.The new tests fail without the change. Each time I removed one part and ran the tests:
optional-requiresnot written, TestTheStoredFormReadsWhereRequiresIsAListOfNames and TestARequirementIsHardUnlessMarkedOptional fail;The new check that refuses an
optional-requiresentry the module does not require is covered by the same test.Checks. merge-check.sh against a throwaway Postgres 18 passes locally: gofmt, go vet, and
go test -raceover the whole suite, with all 37 listed store tests passing. It used the captured mesh-host copy in testdata/beside, so the genesis-template test that main fails was not hit. The first full run failed once: TestAWalkLetGoByAVerbIsSaidAsPlanMoved gave a NATS "context deadline exceeded" while I was composing in parallel. It passed 3 of 3 runs on its own, and the full rerun passed. On the forge, both required checks on this head read "out of date: judged against trunkcbac703e81, the trunk is now 7b2a9eae8020". Read them green on the recheck before merging.The notes from 112 about restart-on naming absent ids and contributions still stand. Neither blocks.
Merge check at
f0a62c1bJudged: head
f0a62c1b473fmerged into trunk454a3db69a1emesh/merge-gate: FAIL — headf0a62c1b47does not merge cleanly into trunk454a3db69a: in conflict testdata/store-tests (modules: build-agent, mesh-controller)mesh/repo-check: FAIL — headf0a62c1b47does not merge cleanly into trunk454a3db69a: in conflict testdata/store-testsDelivery plan — builds build-agent, mesh-controller → ace, g14, novox, shanks; no bus step
Run by the build seat on g14 as
build-1791848235636367152(builds --log build-1791848235636367152).Making every soft dependency a provision made it hard: a mesh without influxdb could not assign grafana, and one with influxdb in its catalogue would install it for grafana. A requirement may now be written {"name": ..., "optional": true}. It resolves as a hard one when a provider is assigned or reachable, and is otherwise absent: never refused, nothing installed for it. Its binding, credential and only-when-bound files are left out, its ${bound:...|default:...} placeholders take their defaults, the module check refuses one that could render empty, and plan and node list it unbound.f0a62c1b47to79a3396340Verdict: approve at
79a3396340The range-diff between the approved head (
f0a62c1b, review 117) and the new head (79a3396), against their respective bases, shows a single difference confined to the testdata/store-tests hunk in the first commit: the new base (main454a3db) had independently gained its own line (TestAStoredBuildWithASilentMergeableFileIsStillReadAndComposed) since the approval, and the rebase's conflict resolution kept that line alongside the PR's own addition (TestPlanAndNodeListAnOptionalRequirementLeftUnbound). Nothing else differs; the second commit's patch is identical (=) on both sides. This matches the builder's account exactly.Both test names exist as functions in the new head: TestPlanAndNodeListAnOptionalRequirementLeftUnbound in cmd/mesh-controller/unbound_test.go, and TestAStoredBuildWithASilentMergeableFileIsStillReadAndComposed in internal/inventory/stored_silent_file_test.go.
GOTMPDIR=/var/tmp go build ./..., go vet ./internal/catalogue ./cmd/mesh-controller, and go test -race ./internal/catalogue/ all passed cleanly at
79a3396.The rebase is clean; the approval at
f0a62c1bstill holds at the new head.Merge check at
79a33963Judged: head
79a3396340a7merged into trunk454a3db69a1e; read beside it: mesh-catalog39e7a06e6293, mesh-controller-main454a3db69a1e, mesh-host7a41e120635b, 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-1791849278130813815(builds --log build-1791849278130813815).