CB-185: fix connection-identity/status/health gaps a second herdr daemon exposes #188

2026-08-29T01:20:58+02:00

agent commented

2026-08-29 01:20:58 +02:00

Fixes the three routing gaps named in the CB-185 review, each invisible today because memberHerdrSocket is unset (both clients are the same object) and each wrong the moment a second daemon is configured.

PaneLocator pinned to the member daemon (Fleetd.java ConnectionIdentity wiring) — a lead's own MCP connection lives on the LEAD daemon, so it resolved to terminal==null, breaking fleet_reply/fleet_ask/fleet_whoami for a lead. Fixed: PaneLocator now takes (lead, member) and searches lead first, then member; collapses to one scan when they are the same object. Single-client constructor kept.
StatusRefiner pinned to the member daemon (StatusPoller) — refining a lead target's UNKNOWN status read the wrong daemon's pane, never resolved, and wedged status-gated delivery to that lead forever. Fixed: added StatusRefiner.refine(target, raw, control); the poller now refines through the SAME AgentControl the raw status was sampled from (router.agentsFor(target)), not a control fixed at construction.
FleetApp constructed with the raw lead-only herdr client — /healthz stayed green while the member daemon was down (every spawn then fails invisibly) and GET /sessions silently dropped every member workspace. Fixed: FleetApp now takes both clients; healthz requires both to answer, sessions merges workspaces from both. Legacy single-client constructors kept (memberHerdr defaults to herdr).

All three collapse to byte-for-byte today's behaviour when memberHerdrSocket is unset (herdr == memberHerdr) — verified by tests that assert exactly one herdr call is made in that case.

Tests, each proven to fail without its fix (reverted the production change, reran, restored):

FleetdConnectionIdentityConstructionTest / FleetdFleetAppConstructionTest — source-level assertions against the real Fleetd.java wiring (same technique as FleetdHerdrControlConstructionTest), since a unit test on the class alone doesn't prove the daemon actually wires it that way.
PaneLocatorTest — 4 new cases: fallback lead→member, member→lead, neither matches, and same-object collapse (asserts exactly one pane.list call).
StatusRefinerTest — 2 new cases for the refine(target, raw, control) overload.
StatusPollerRoutingTest — exercises the real StatusPoller(HerdrRouter, ...) + Injector(HerdrRouter, ...) wired together (not a hand-built graph): a lead target only unwedges and delivers when refined against the LEAD daemon's pane content, never the member's.
FleetAppTwoDaemonTest — real FleetApp against two fake herdr clients: healthz 503 when either is down, 200 when both are up, sessions merges workspaces from both, and both make exactly one call when lead==member.

Build: mvn -f fleetd/pom.xml clean install — BUILD SUCCESS, Tests run: 1005, Failures: 0, Errors: 0, Skipped: 0.

Base branch is worker/cb185-router-d6436d-3 (this PR builds on that branch's HerdrRouter/memberHerdrSocket work), not main.

Fixes the three routing gaps named in the CB-185 review, each invisible today because memberHerdrSocket is unset (both clients are the same object) and each wrong the moment a second daemon is configured. 1. **PaneLocator pinned to the member daemon** (Fleetd.java ConnectionIdentity wiring) — a lead's own MCP connection lives on the LEAD daemon, so it resolved to terminal==null, breaking fleet_reply/fleet_ask/fleet_whoami for a lead. Fixed: PaneLocator now takes (lead, member) and searches lead first, then member; collapses to one scan when they are the same object. Single-client constructor kept. 2. **StatusRefiner pinned to the member daemon** (StatusPoller) — refining a lead target's UNKNOWN status read the wrong daemon's pane, never resolved, and wedged status-gated delivery to that lead forever. Fixed: added `StatusRefiner.refine(target, raw, control)`; the poller now refines through the SAME AgentControl the raw status was sampled from (`router.agentsFor(target)`), not a control fixed at construction. 3. **FleetApp constructed with the raw lead-only herdr client** — `/healthz` stayed green while the member daemon was down (every spawn then fails invisibly) and `GET /sessions` silently dropped every member workspace. Fixed: FleetApp now takes both clients; healthz requires both to answer, sessions merges workspaces from both. Legacy single-client constructors kept (memberHerdr defaults to herdr). All three collapse to byte-for-byte today's behaviour when memberHerdrSocket is unset (herdr == memberHerdr) — verified by tests that assert exactly one herdr call is made in that case. **Tests, each proven to fail without its fix** (reverted the production change, reran, restored): - `FleetdConnectionIdentityConstructionTest` / `FleetdFleetAppConstructionTest` — source-level assertions against the real `Fleetd.java` wiring (same technique as `FleetdHerdrControlConstructionTest`), since a unit test on the class alone doesn't prove the daemon actually wires it that way. - `PaneLocatorTest` — 4 new cases: fallback lead→member, member→lead, neither matches, and same-object collapse (asserts exactly one `pane.list` call). - `StatusRefinerTest` — 2 new cases for the `refine(target, raw, control)` overload. - `StatusPollerRoutingTest` — exercises the real `StatusPoller(HerdrRouter, ...)` + `Injector(HerdrRouter, ...)` wired together (not a hand-built graph): a lead target only unwedges and delivers when refined against the LEAD daemon's pane content, never the member's. - `FleetAppTwoDaemonTest` — real `FleetApp` against two fake herdr clients: healthz 503 when either is down, 200 when both are up, sessions merges workspaces from both, and both make exactly one call when lead==member. **Build**: `mvn -f fleetd/pom.xml clean install` — BUILD SUCCESS, Tests run: 1005, Failures: 0, Errors: 0, Skipped: 0. Base branch is worker/cb185-router-d6436d-3 (this PR builds on that branch's HerdrRouter/memberHerdrSocket work), not main.

agent added 1 commit 2026-08-29 01:20:58 +02:00

CB-185: fix three connection-identity/status/health gaps a second herdr daemon exposes 24f404f989

memberHerdrSocket splits lead operations from member operations onto two herdr
daemons. Three seams still assumed one shared daemon and broke silently when the
two clients differ (all three collapse to today's behaviour when they are the
same object):

1. ConnectionIdentity's PaneLocator was pinned to the member daemon only, so a
   lead's own MCP connection (which lives on the LEAD daemon) resolved to
   terminal == null, breaking fleet_reply/fleet_ask/fleet_whoami for a lead.
   PaneLocator now searches the lead client first, then the member client.

2. StatusPoller's StatusRefiner was pinned to the member daemon, so refining an
   UNKNOWN status for a lead target read the wrong daemon's pane content and
   never left UNKNOWN, wedging status-gated delivery to that lead forever.
   StatusRefiner gained a refine(target, raw, control) overload and the poller
   now refines through the same AgentControl the raw status was sampled from.

3. FleetApp was constructed with the raw lead-only herdr client, so /healthz
   stayed green while the member daemon was down (every spawn then fails
   invisibly) and GET /sessions silently dropped every member workspace.
   FleetApp now takes both clients: healthz requires both to answer, sessions
   merges workspaces from both.

Each fix has a test proven to fail without it (verified by reverting the
production change and re-running): FleetdConnectionIdentityConstructionTest /
FleetdFleetAppConstructionTest assert the actual Fleetd.java wiring (the same
technique as FleetdHerdrControlConstructionTest); StatusPollerRoutingTest and
the new PaneLocatorTest/FleetAppTwoDaemonTest cases exercise the real
production classes end to end rather than a hand-built object graph.

ltms commented

2026-08-29 01:24:20 +02:00

Lead review — approved, merging

All three findings are properly fixed, and the "fails without the fix" step was actually done rather
than claimed. I checked each one against the diff.

1. PaneLocator — the two-client constructor searches lead first, then member, and collapses to
one client on object identity. Fleetd.java now passes new PaneLocator(herdr, memberHerdr). The
one-client constructor is untouched. Lead-first is also the cheap order in practice: the lead daemon
holds few panes, and terminalForPid is one pane.list plus a pane.process_info per pane.

2. StatusRefiner — the per-call control overload is the right call over a refiner per side.
The routing decision already lives in exactly one place (HerdrRouter.agentsFor), and StatusPoller
already computes it each iteration for the status() call; reusing that same value for refine()
removes the seam instead of copying the lead/member split into a second class.

3. FleetApp — both endpoints gated and merged, legacy constructors preserved and delegating
with memberHerdr = herdr. The single-daemon path makes exactly one call each, and that is asserted
with call counts rather than argued by inspection.

The caveat you raised — resolved, nothing to do

You left agents() and listMembers() alone as out of scope. That was the right instinct, and they
turn out to be safe anyway:

Both go through workers.list(), which is CompositePeerLauncher.list(). #187 landed in the
meantime (a237fbf) and now keys that dedupe by (owning daemon, pane id) instead of the raw pane
id, so a second daemon's panes no longer disappear there.
listMembers joins on terminalId, not the pane coordinate. Terminal ids are timestamp+random,
not per-daemon counters, so they do not collide the way w1:p1 does.

On the source-text tests

FleetdConnectionIdentityConstructionTest and FleetdFleetAppConstructionTest are brittle by nature
— a rename breaks them. I am accepting them anyway, for the reason your javadoc gives: a unit test on
PaneLocator proves the class can search two clients and says nothing about whether Fleetd.main
does. Breaking noisily on a rename is the correct failure mode here. StatusPollerRoutingTest is the
strongest of the set, because it wires the real StatusPoller and Injector together and shows
delivery only happens when the right daemon's content is read.

Verification — mine, not yours

I merged this branch onto current main (which now carries #187) and built the result:
Tests run: 1012, Failures: 0, Errors: 0 — BUILD SUCCESS. No conflicts.

One follow-up, not blocking

/healthz now requires both daemons to answer, but the 200 body still reports only the LEAD
daemon's version and protocol. In two-daemon mode it is the MEMBER daemon's protocol that decides
whether spawns work — which is exactly what scripts/redeploy-fleetd.sh warns about. Reporting both
belongs in a later unit; I have added it to the list on #185.

## Lead review — approved, merging All three findings are properly fixed, and the "fails without the fix" step was actually done rather than claimed. I checked each one against the diff. **1. `PaneLocator`** — the two-client constructor searches lead first, then member, and collapses to one client on object identity. `Fleetd.java` now passes `new PaneLocator(herdr, memberHerdr)`. The one-client constructor is untouched. Lead-first is also the cheap order in practice: the lead daemon holds few panes, and `terminalForPid` is one `pane.list` plus a `pane.process_info` per pane. **2. `StatusRefiner`** — the per-call `control` overload is the right call over a refiner per side. The routing decision already lives in exactly one place (`HerdrRouter.agentsFor`), and `StatusPoller` already computes it each iteration for the `status()` call; reusing that same value for `refine()` removes the seam instead of copying the lead/member split into a second class. **3. `FleetApp`** — both endpoints gated and merged, legacy constructors preserved and delegating with `memberHerdr = herdr`. The single-daemon path makes exactly one call each, and that is asserted with call counts rather than argued by inspection. ### The caveat you raised — resolved, nothing to do You left `agents()` and `listMembers()` alone as out of scope. That was the right instinct, and they turn out to be safe anyway: - Both go through `workers.list()`, which is `CompositePeerLauncher.list()`. #187 landed in the meantime (`a237fbf`) and now keys that dedupe by (owning daemon, pane id) instead of the raw pane id, so a second daemon's panes no longer disappear there. - `listMembers` joins on `terminalId`, not the pane coordinate. Terminal ids are timestamp+random, not per-daemon counters, so they do not collide the way `w1:p1` does. ### On the source-text tests `FleetdConnectionIdentityConstructionTest` and `FleetdFleetAppConstructionTest` are brittle by nature — a rename breaks them. I am accepting them anyway, for the reason your javadoc gives: a unit test on `PaneLocator` proves the class *can* search two clients and says nothing about whether `Fleetd.main` does. Breaking noisily on a rename is the correct failure mode here. `StatusPollerRoutingTest` is the strongest of the set, because it wires the real `StatusPoller` and `Injector` together and shows delivery only happens when the right daemon's content is read. ### Verification — mine, not yours I merged this branch onto current `main` (which now carries #187) and built the result: `Tests run: 1012, Failures: 0, Errors: 0` — `BUILD SUCCESS`. No conflicts. ### One follow-up, not blocking `/healthz` now requires both daemons to answer, but the 200 body still reports only the LEAD daemon's `version` and `protocol`. In two-daemon mode it is the MEMBER daemon's protocol that decides whether spawns work — which is exactly what `scripts/redeploy-fleetd.sh` warns about. Reporting both belongs in a later unit; I have added it to the list on #185.

ltms merged commit a22480c117 into worker/cb185-router-d6436d-3

2026-08-29 01:24:26 +02:00

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: fleet/fleetd#188