Say which plane decided the MCP endpoint is on, and on what port (#2389) - #2411
Conversation
One mcp object in darling.json has two owners. enabled and port are a first-run seed that config.config_service overrides forever after; the network block beside them -- listen, allowFrom, encryptedToken -- is file-only, with no store equivalent at all. Nothing in the file or the log distinguished them, so wiring up the MCP on the pgmonitor box produced a start line naming a LAN bind, and five seconds later a stop line saying the control plane had disabled it. Both lines were true. Neither said the file edit was being ignored. The store still wins; that part was never wrong. What was wrong is that `published?.Enabled ?? config.Mcp.Enabled` structurally cannot say which side it took, so a disagreement could only be reconstructed from two INFO lines with nothing tying either back to the file. The resolution now runs through a shared helper that carries its provenance, and the two places an operator actually looks say what they know: the start line names the plane each half of the bind came from and admits when it is running on file values the control plane has not weighed in on yet, and a genuine disagreement is reported as a warning at the point of override, naming both sides with the key and the column, which one wins, and the verb that changes the winning one. It fires only on the mismatch and only once per distinct state, so a steady disagreement says its piece once per start rather than every five seconds, and a later re-divergence is still reported. The port has the identical shape and gets the identical treatment -- a server on an unexpected port with no explanation is the same defect with a different symptom. The network block deliberately stays file-authoritative rather than being moved into the store to match. The token is DPAPI at LocalMachine scope, so a blob in config_service is undecryptable on any other host that reads that store, and there is no plaintext alternative worth having: a bind address, a CIDR and a bearer token in config_service would let a remote admin store connection -- the pivot the postgres.network role warning already names -- re-point this listener onto a LAN interface behind a credential of its own choosing. Exposure should require touching the host. So the split is kept and disclosed instead, in the warning itself, because the operator whose file edit is being ignored is exactly the one who needs to know the block right beside it is live. The web dashboard is the same defect on the same seam, so it shares the resolver rather than getting a parallel copy: one section argument drives the file key, the store column and the CLI verb, which is what stops the two wordings drifting apart. Three CLI notes got the same correction. All three printed only when the FILE said the endpoint was disabled, which is exactly backwards -- a file that says true while the store says false is the combination that misleads, and it printed nothing there. The wizard holds no store connection and cannot report the effective value, so what it says now is which plane decides and where the file value stops mattering. The sample config already described this split correctly. The documentation was right; the runtime was silent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| return $"{surface} configuration disagrees across the two planes and the CONTROL PLANE WINS: " | ||
| + string.Join("; ", fields) | ||
| + $". After the first run darling.json's {section}.enabled/{section}.port are only the SEED -- change them with " | ||
| + $"--enable-{section}/--disable-{section} or the Viewer's Settings, or the file values will keep being ignored. " | ||
| + $"The {section}.network block is the OPPOSITE: file-only, restart-only, no store equivalent -- so an exposure " | ||
| + "block in darling.json is live even while the control plane keeps this endpoint off."; |
There was a problem hiding this comment.
The closing sentence is unconditional but only true when toggle.Enabled == false. When the control plane enables an endpoint the file disagrees with (or when only the port differs and both planes agree it's enabled), the endpoint is actually running — this text still claims "the control plane keeps this endpoint off," which is backwards.
Concretely, DescribeToggleOverride_ReportsThePortToo_AndBothFieldsAtOnce in DarlingHostBindingTests.cs builds exactly this case: ResolveEndpointToggle((true, 5199), fileEnabled: false, filePort: 5152) → toggle.Enabled == true (control plane turned it on despite the file saying disabled). The generated report — not asserted on by that test — would still end with "...even while the control plane keeps this endpoint off," directly contradicting the state it just described a few sentences earlier ("false in darling.json ... but true in config.config_service.mcp_enabled").
A pure port-only mismatch (both planes agree enabled = true, only the port differs — plausibly the most common real case, e.g. someone changes the port via the Viewer's Settings) hits the same wrong sentence: the endpoint is running, not off.
Since this diagnostic exists specifically to stop operators from being misled about which plane is in control, this closing clause should be conditioned on toggle.Enabled (e.g. only mention "even while the control plane keeps this endpoint off" when !toggle.Enabled, and something like "even while the control plane keeps this endpoint on, exposed per darling.json's network block" otherwise — or just drop the endpoint-state claim entirely and keep the file/store precedence explanation, which is true regardless of state).
There was a problem hiding this comment.
Fixed in cfa93d3 — you're right, and the port-only case you named is the sharper half of it: both planes agreeing on enabled while only the port differs is the likeliest real mismatch, and it had no test at all, which is why nothing caught the wrong sentence.
The ownership half is now unconditional — "the control plane cannot change where this endpoint binds or what token it requires" — because that is true in every state. The consequence is emitted only in the state it describes: down, and "it is not what is keeping this endpoint down, and it takes effect as written the moment the control plane enables it"; up, and "it is what this RUNNING endpoint is bound and gated by, and no store setting can move it."
Both enabled-direction tests now assert the endpoint-is-down claim is absent, not just that the right clause is present — a presence-only assertion would have passed on the broken message, which is how it got here.
Review summaryScope: purely Darling-side C# (config resolution provenance + diagnostics for MCP/web Correctness bug found (left as an inline comment on Everything else checked out:
|
The override report closed with one unconditional sentence saying the network block was live "even while the control plane keeps this endpoint off". That is only true in the direction the issue reported. When the control plane ENABLES an endpoint the file disables, and in the port-only mismatch where both planes agree it runs and someone simply moved the port in the Viewer -- plausibly the commonest real case -- the endpoint is up, and the sentence contradicted the state the same message had just described two clauses earlier. A diagnostic that exists to stop an operator being misled about the effective state cannot itself misstate it, so the ownership half stays unconditional, because it is true in every state, and the consequence is now emitted in the state it describes: down, and the network block is not what is holding it there; up, and the network block is what it is bound and gated by. The port-only mismatch had no test at all, which is why nothing caught this. It has one now, and both enabled-direction cases assert the endpoint-is-down claim is absent rather than only asserting what is present -- an assertion on presence alone would have passed on the broken message. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Merge-order note, restated from #2409.
Safe orders:
|
|
Reviewed the diff ( Correctness — Parity — This resolves a Darling-specific defect (the Security — No secrets in the new log/warning text (only Style/schema — Column names referenced in the new messages ( No findings. Tests are thorough (26 assertions covering unpublished/agreeing/disagreeing states for both fields and both surfaces, plus source-pinning tests that guard against a future silent reversion to the null-coalesce form). |
1 participant
Closes #2389.
What the truth actually is
The first job was to establish which plane owns which field, because the issue's premise is that the same
mcpobject has two owners and nothing says so. It does. Here is every resolution site:mcp.enabledconfig.config_service.mcp_enabledpublished?.Enabled ?? config.Mcp.Enabledin the supervisor; the worker publishes on boot and every reload, so the file value applies only in the pre-publish windowmcp.portconfig.config_service.mcp_portmcp.network.listenResolveMcpBind(config.Mcp, …)off the host's ownDarlingConfig.Load()mcp.network.allowFrommcp.network.*postgres.managedwebis the byte-identical twin:web.enabled/web.portstore-authoritative,web.network.*file-only.Two details worth stating because they change how the bug reads. First, the file is not a fallback that lost an argument —
SeedServiceRowAsyncwrites it once withON CONFLICT DO NOTHING, so on a seeded store it is dead input, and the worker publishes the file values itself when the store read fails, meaningpublishedis non-null in both the healthy and the store-down case. Second, the MCP host callsDarlingConfig.Load()itself and holds that instance for the process lifetime, so the worker'sApplyToConfignever touches the host's copy: the two values genuinely coexist in one process.Why (a)/(c) and not (b)
Moving
mcp.networkinto the store to matchmcp.enabledis not safely achievable, and the DPAPI problem is only half of why.encryptedTokenisLocalMachine-scoped with entropyPerformanceMonitor.Darling.v1, so a blob inconfig_serviceis undecryptable on any other host that reads that store — and there is no plaintext alternative worth having, because a bind address, a CIDR and a bearer token living inconfig_servicewould let a remote admin store connection (the pivot thepostgres.network.rolewarning already names) re-point this listener onto a LAN interface behind a credential of its own choosing. Changing an exposure surface should require touching the host. So the split is kept, and the fix makes it visible and explicit instead.What changed
The resolution now goes through one shared helper in
DarlingHostBinding— the same anti-drift home the bind ladder already lives in — that returns the effective(enabled, port)with its provenance and whether it contradicts the file.published?.Enabled ?? …structurally cannot report which side it took; that is the whole defect, and it is why the fix is at the resolution site rather than in a decision function.Two surfaces then say what they know. The start line names the plane each half of the bind came from and admits when it is running on file values the control plane has not weighed in on yet — that is the line the operator greps and stops reading, so it had to stop presenting a provisional start as a settled one. And a real disagreement is reported as a warning at the point of override, naming both sides with the key and the column, which one wins, the verb that changes the winning one, and the opposite ownership of the network block. It fires only on the mismatch and only once per distinct state, so a steady disagreement says its piece once per service start instead of every five seconds, and a later re-divergence is still reported.
On the reported box that turns this:
into this:
The port gets the same treatment, per the issue — same shape, and a server on an unexpected port with no explanation is the same defect wearing a different symptom.
The web dashboard shares the resolver rather than getting a parallel copy. One
sectionargument drives the file key, the store column and the CLI verb, which is what keeps the two wordings from drifting.Three CLI notes had the same defect in miniature:
--configure-network's MCP and web notes andPrintNextSteps' two reminders all printed only when the file said the endpoint was disabled, which is exactly backwards — a file that saystruewhile the store saysfalseis the combination that misleads, and they printed nothing there. The wizard holds no store connection and cannot report the effective value, so what it says now is which plane decides and where the file value stops mattering.I deliberately did not defer the start log until after the first reconcile, which the issue floats as an option. The line is true when it is printed and the server really does bind; suppressing it would trade one misleading log for another, and would also hide a start that failed before the first publish. Naming the provenance fixes the misreading without making the log less complete.
Verification
Windows-only suites can't execute on macOS, so: the solution builds clean (0 warnings), and the pure resolver was run against the shipped build from a throwaway
net10.0harness — 26 assertions covering the unpublished/agreeing/disagreeing cases, both fields, both surfaces and both origin clauses, all green. The source pins were simulated in Python against the fixture copies the tests actually read, and each one was re-run againstdev's version of the two hosts to confirm it goes red without the fix rather than passing both ways. The existing pins that parse these same two files (Host-header guard ordering, the firewall check, the identity-derivation ban) were re-simulated and still pass.Not fixed here
The sweep for resolution sites turned up an adjacent defect of the same family, filed as #2414 rather than folded in: the firewall verbs (
--enable-mcp's firewall half and--configure-firewall) name the scoped rule from darling.json's port while the endpoint binds the store's, so changing the port in the Viewer leaves an exposed endpoint with no firewall path and an elevated verb that keeps creating the wrong rule. It needs a store read in the CLI, which is a different change from this one. The warning added here at least makes the precondition visible.darling.sample.jsonalready described this split correctly, on both themcpandwebblocks. The documentation was right the whole time; the runtime was silent. That is worth saying because it means no doc change would have prevented this.