fleet/fleetd
3
0
Fork 0
Code Issues 73 Pull Requests Actions Packages Projects Releases 1 Wiki Activity

LoopWatchdog.health() is public and nothing reads it — surface it without breaking the two scripts that parse /healthz #562

New Issue
Closed
opened 2026-09-12 11:15:10 +02:00 by ltms · 3 comments
ltms commented 2026-09-12 11:15:10 +02:00
Owner
Copy Link
Copy Source

Follow-up to #544, which merged as PR #559. Filed by me while verifying that merge.

#544 shipped the observability half: StatusPoller.health() and SessionReaper.health() each return a LoopWatchdog.State of RUNNING / STALLED / STOPPED, and both are proven by tests. Nothing in production calls either one.

Measured on the merged tree (main 7a3b2bb + bfac141, identical to main at dab697f):

$ grep -rnE '(poller|reaper|statusPoller|sessionReaper)\.health\(\)' fleetd/src/main/java --include='*.java'
$ echo $?
1

A zero match proves nothing on its own, so the same pattern against the test sources, as a positive control that the search really runs:

$ grep -rnE '(poller|reaper)\.health\(\)' fleetd/src/test/java --include='*.java' | head -4
.../inject/StatusPollerWatchdogTest.java:43:  assertEquals(LoopWatchdog.State.RUNNING, poller.health(),
.../inject/StatusPollerWatchdogTest.java:47:  assertEquals(LoopWatchdog.State.STALLED, poller.health(),
.../inject/StatusPollerWatchdogTest.java:68:  assertEquals(LoopWatchdog.State.STALLED, poller.health(),
.../inject/StatusPollerWatchdogTest.java:83:  assertEquals(LoopWatchdog.State.STOPPED, poller.health(),

And the declarations themselves, so the zero above is a zero about callers and not about the method existing:

$ grep -rn 'LoopWatchdog.State health()' fleetd/src/main/java --include='*.java'
fleetd/src/main/java/dev/ltms/fleet/inject/StatusPoller.java:97:    public LoopWatchdog.State health() {
fleetd/src/main/java/dev/ltms/fleet/session/SessionReaper.java:82:    public LoopWatchdog.State health() {

Do not use the pattern \.health() for this. It returns 7 hits in fleetd/src/main/java, every one of them cfg.health() / config.get().health() — the config accessor in Fleetd.java and ConfigRef.java, an unrelated method with the same name. Anyone re-measuring with the loose pattern will read those hits as production callers of the watchdog and conclude this ticket is already done.

So a StatusPoller loop that dies or parks now has a correct three-state fact about it, sitting in memory, that no operator and no tool can read. That is better than #538's silence and it is not yet observability.

What is wanted

Make health() reachable from outside the process. At least one of these; they are independent and the easiest one alone is worth shipping:

  1. fleet_list and/or fleet_status — a field per loop, alongside the existing capacity and coordinator rows. Probably the cheapest, and it reaches a lead directly.
  2. /healthz — see the hard constraint below before designing anything here.
  3. The health snapshot (HealthSnapshot / FleetHealthMonitor) — note healthCoverage currently reports detection-only, so check what that promise already means before adding to it.

The constraint that decides the /healthz design — measured, 2026-09-12

503 is the only safe non-200 status code /healthz can return. Both scripts that parse it treat every other non-200 as fatal:

script 503 any other non-200
scripts/redeploy-fleetd.sh:1082 warn and continue :1087 — die
scripts/rename-checkout.sh:448 treated as OK :452 — die

Re-measure before relying on this:

grep -n '503' scripts/redeploy-fleetd.sh scripts/rename-checkout.sh

If both files still branch on 503 with a die on the else, this section still applies. If either stops parsing the status code, delete this section.

That leaves a real dilemma, and whoever takes this should decide it deliberately rather than discover it:

  • A new status code for a stalled loop breaks both scripts on the day it first fires — which is the day the fleet is already degraded.
  • Reusing 503 makes a stalled StatusPoller indistinguishable from an unreachable herdr, which is the exact two-states-one-symbol shape (#512) that LoopWatchdog exists to avoid. Putting it back one layer out would be a poor trade.
  • Keeping /healthz at 200 and putting the state in the body costs nothing for either script, because neither reads the body. This is probably the right answer, but it means the loop state does not reach anything that only checks the status code.

FleetApp.java:293-330 is the endpoint. Today it reports herdr reachability and nothing else.

A design warning from the fleet01 lead

Worth writing down before anyone chooses an alerting shape, because the failure direction here is the unusual one:

The symptom is INVERTED from the usual one — stop() then start() reports STOPPED forever on a loop that is actually running, so the watchdog you are building would itself report a false alarm on a healthy daemon. A monitoring component whose failure mode is a false POSITIVE gets muted by whoever is on call, and then the real alarm is muted too.

That specific bug is fixed and pinned — aRestartedLoopReportsRunningAgainNotStoppedForever in both StatusPollerWatchdogTest and SessionReaperWatchdogTest, each proven by deleting watchdog.reset() from start(). The general point survives the fix: whatever surfaces this must be at least as trustworthy as the thing it reports on, because a monitor that cries wolf is worse than no monitor. A surfaced STALLED that turns out to be a bug in the surfacing is the failure to design against.

Acceptance

  • A test that a loop reported as STALLED by health() is reported as stalled through the new surface, and that inverting the reported value fails a test.
  • A test that a loop stopped on purpose is not reported as an alarm through that surface. STOPPED is not STALLED; collapsing the two at the boundary throws away the whole point of the three-state design.
  • If /healthz is touched: a test pinning that the status code for a stalled loop is one the two scripts above survive, with the decision above stated in the PR.
  • If fleet_list / fleet_status is touched: the intent→tool table in CLAUDE.md and the wiki copy stay in sync, and McpContractDocTest passes. A visible new behaviour also earns an entry in wiki/11-Features.md — what it does, the knob, why it exists, the gotcha.
  • Each new test proven by a mutation: line-anchored sed only, the pristine full line counted by exact string equality (awk '$0==p', not a regex — a regex can match inside the comment the mutation inserted, which produced a false "not applied" on #544), red with that test's own assertion message, restored byte-identical under a full shasum -a 256, green control re-run.
  • No socket, no port bind, no spawn, nothing written outside a @TempDir.

Out of scope

  • Restarting a dead loop. That is the other half of #544 and is still unfiled. Do not build a supervisor here; a monitor that silently restarts hides the failure it was built to show.
  • Adding a deadline to UnixSocketHerdrClient.call(). Separate, real, wanted — and this watchdog exists because that deadline does not exist yet.
  • Reopening the package placement of LoopWatchdog. It lives in inject to avoid an inject → health → session → inject cycle; PackageCyclesTest enforces it.

Related: #544 (the half that merged), #538 / #543 (the catch (Throwable) work underneath), #512 (one symbol carrying two states).

Follow-up to #544, which merged as PR #559. Filed by me while verifying that merge. #544 shipped the observability **half**: `StatusPoller.health()` and `SessionReaper.health()` each return a `LoopWatchdog.State` of `RUNNING` / `STALLED` / `STOPPED`, and both are proven by tests. Nothing in production calls either one. Measured on the merged tree (`main` `7a3b2bb` + `bfac141`, identical to `main` at `dab697f`): ``` $ grep -rnE '(poller|reaper|statusPoller|sessionReaper)\.health\(\)' fleetd/src/main/java --include='*.java' $ echo $? 1 ``` A zero match proves nothing on its own, so the same pattern against the test sources, as a positive control that the search really runs: ``` $ grep -rnE '(poller|reaper)\.health\(\)' fleetd/src/test/java --include='*.java' | head -4 .../inject/StatusPollerWatchdogTest.java:43: assertEquals(LoopWatchdog.State.RUNNING, poller.health(), .../inject/StatusPollerWatchdogTest.java:47: assertEquals(LoopWatchdog.State.STALLED, poller.health(), .../inject/StatusPollerWatchdogTest.java:68: assertEquals(LoopWatchdog.State.STALLED, poller.health(), .../inject/StatusPollerWatchdogTest.java:83: assertEquals(LoopWatchdog.State.STOPPED, poller.health(), ``` And the declarations themselves, so the zero above is a zero about callers and not about the method existing: ``` $ grep -rn 'LoopWatchdog.State health()' fleetd/src/main/java --include='*.java' fleetd/src/main/java/dev/ltms/fleet/inject/StatusPoller.java:97: public LoopWatchdog.State health() { fleetd/src/main/java/dev/ltms/fleet/session/SessionReaper.java:82: public LoopWatchdog.State health() { ``` **Do not use the pattern `\.health()` for this.** It returns 7 hits in `fleetd/src/main/java`, every one of them `cfg.health()` / `config.get().health()` — the **config** accessor in `Fleetd.java` and `ConfigRef.java`, an unrelated method with the same name. Anyone re-measuring with the loose pattern will read those hits as production callers of the watchdog and conclude this ticket is already done. So a `StatusPoller` loop that dies or parks now has a correct three-state fact about it, sitting in memory, that no operator and no tool can read. That is better than #538's silence and it is not yet observability. ## What is wanted Make `health()` reachable from outside the process. **At least one** of these; they are independent and the easiest one alone is worth shipping: 1. `fleet_list` and/or `fleet_status` — a field per loop, alongside the existing capacity and coordinator rows. Probably the cheapest, and it reaches a lead directly. 2. `/healthz` — see the hard constraint below before designing anything here. 3. The health snapshot (`HealthSnapshot` / `FleetHealthMonitor`) — note `healthCoverage` currently reports `detection-only`, so check what that promise already means before adding to it. ## The constraint that decides the `/healthz` design — measured, 2026-09-12 **503 is the only safe non-200 status code `/healthz` can return.** Both scripts that parse it treat every *other* non-200 as fatal: | script | 503 | any other non-200 | |---|---|---| | `scripts/redeploy-fleetd.sh:1082` | warn and continue | `:1087` — `die` | | `scripts/rename-checkout.sh:448` | treated as OK | `:452` — `die` | Re-measure before relying on this: ```bash grep -n '503' scripts/redeploy-fleetd.sh scripts/rename-checkout.sh ``` If both files still branch on `503` with a `die` on the else, this section still applies. If either stops parsing the status code, delete this section. That leaves a real dilemma, and whoever takes this should decide it deliberately rather than discover it: - A **new** status code for a stalled loop breaks both scripts on the day it first fires — which is the day the fleet is already degraded. - **Reusing 503** makes a stalled `StatusPoller` indistinguishable from an unreachable herdr, which is the exact two-states-one-symbol shape (#512) that `LoopWatchdog` exists to avoid. Putting it back one layer out would be a poor trade. - **Keeping `/healthz` at 200 and putting the state in the body** costs nothing for either script, because neither reads the body. This is probably the right answer, but it means the loop state does not reach anything that only checks the status code. `FleetApp.java:293-330` is the endpoint. Today it reports herdr reachability and nothing else. ## A design warning from the fleet01 lead Worth writing down before anyone chooses an alerting shape, because the failure direction here is the unusual one: > The symptom is INVERTED from the usual one — `stop()` then `start()` reports STOPPED forever on a loop that is actually running, so the watchdog you are building would itself report a false alarm on a healthy daemon. A monitoring component whose failure mode is a false POSITIVE gets muted by whoever is on call, and then the real alarm is muted too. That specific bug is fixed and pinned — `aRestartedLoopReportsRunningAgainNotStoppedForever` in both `StatusPollerWatchdogTest` and `SessionReaperWatchdogTest`, each proven by deleting `watchdog.reset()` from `start()`. The general point survives the fix: **whatever surfaces this must be at least as trustworthy as the thing it reports on**, because a monitor that cries wolf is worse than no monitor. A surfaced `STALLED` that turns out to be a bug in the surfacing is the failure to design against. ## Acceptance - A test that a loop reported as `STALLED` by `health()` is reported as stalled through the new surface, and that inverting the reported value fails a test. - A test that a loop stopped on purpose is **not** reported as an alarm through that surface. `STOPPED` is not `STALLED`; collapsing the two at the boundary throws away the whole point of the three-state design. - If `/healthz` is touched: a test pinning that the status code for a stalled loop is one the two scripts above survive, with the decision above stated in the PR. - If `fleet_list` / `fleet_status` is touched: the intent→tool table in `CLAUDE.md` and the wiki copy stay in sync, and `McpContractDocTest` passes. A visible new behaviour also earns an entry in `wiki/11-Features.md` — what it does, the knob, **why it exists**, the gotcha. - Each new test proven by a mutation: line-anchored `sed` only, the pristine full line counted by exact string equality (`awk '$0==p'`, **not** a regex — a regex can match inside the comment the mutation inserted, which produced a false "not applied" on #544), red with that test's own assertion message, restored byte-identical under a full `shasum -a 256`, green control re-run. - No socket, no port bind, no spawn, nothing written outside a `@TempDir`. ## Out of scope - **Restarting** a dead loop. That is the other half of #544 and is still unfiled. Do not build a supervisor here; a monitor that silently restarts hides the failure it was built to show. - Adding a deadline to `UnixSocketHerdrClient.call()`. Separate, real, wanted — and this watchdog exists *because* that deadline does not exist yet. - Reopening the package placement of `LoopWatchdog`. It lives in `inject` to avoid an `inject → health → session → inject` cycle; `PackageCyclesTest` enforces it. Related: #544 (the half that merged), #538 / #543 (the `catch (Throwable)` work underneath), #512 (one symbol carrying two states).
ltms commented 2026-09-12 14:14:17 +02:00
Author
Owner
Copy Link
Copy Source

Decision, and a re-measurement of every number this ticket rests on

Measured on main at 204da67, 2026-09-12. I re-ran the ticket's own commands rather than trust them, and two of the line numbers had already drifted.

The zero still holds

grep -rnE '(poller|reaper|statusPoller|sessionReaper)\.health\(\)' fleetd/src/main/java --include='*.java'
  → exit 1, no output

Positive control, same idea against the test sources — non-empty, so the search really runs:

fleetd/src/test/java/dev/ltms/fleet/inject/StatusPollerWatchdogTest.java:6
fleetd/src/test/java/dev/ltms/fleet/session/SessionReaperWatchdogTest.java:6

And the declarations, so the zero is about callers and not about the method existing: StatusPoller.java:97 and SessionReaper.java:82. Still no production caller.

The 503 constraint still holds, but the line numbers moved

The ticket says scripts/redeploy-fleetd.sh:1082 / :1087. They are now :1009 / :1014. scripts/rename-checkout.sh:448 is still :448. Locate them fresh; do not trust either number, including mine.

The constraint itself is intact, and the second script's half is worse than the ticket describes:

  • redeploy-fleetd.sh report_health(): 503 → warn and continue (:1010). Every other non-200 → die (:1014).
  • rename-checkout.sh :445-452: a case on the code with arms for 200 and 503 only. An unknown code does not die immediately — it keeps polling until HEALTH_WAIT runs out and then dies with "/healthz never answered within Ns". That message is wrong: the daemon answered every time. So a new status code here does not just break the script, it breaks it with a misleading diagnosis, on the day the fleet is already degraded.

The body is free, and I measured why

The ticket assumes neither script reads the body. Half true, and the true half is better than assumed. redeploy-fleetd.sh's green path is a body predicate, not a code check:

health_is_up() {
  [ -n "$1" ]
}

Body non-empty. That is all. So adding fields to the /healthz body costs nothing for that script, and rename-checkout.sh reads only the status code. The body really is free.

The decision

1. /healthz keeps its status codes exactly as they are. 200 for ok, 503 for herdr unreachable. The loop states go in the body only. No new status code, and do not reuse 503 for a stalled loop — that is the one-symbol-two-states shape (#512) that LoopWatchdog was built to avoid, and putting it back one layer out would be a bad trade.

2. fleet_list is the primary surface. It is the cheapest of the three, it reaches a lead directly, and it carries no script risk at all. Do this one properly; it is the deliverable.

3. STOPPED is never reported as an alarm. Collapsing STOPPED into STALLED at the boundary throws away the whole reason the design has three states. A loop stopped on purpose is a fact, not a fault.

4. Option 3 — HealthSnapshot / FleetHealthMonitor — is out of scope for this unit. healthCoverage still reports detection-only; changing what that word promises is its own decision and does not belong in the same PR.

On the fleet01 lead's warning

Keeping it, and it shapes the acceptance rather than just sitting in the ticket. A monitor whose failure mode is a false positive gets muted, and then the real alarm is muted too. So the test that a deliberately stopped loop is not surfaced as an alarm is not a nice-to-have — it is the test that decides whether anyone will still be listening the first time this fires for real.

One thing the member cannot do, so it must not be asked of them

The ticket's acceptance says the CLAUDE.md block and the wiki copy must stay in sync, and that a visible new behaviour earns a wiki/11-Features.md entry. A member's provisioned worktree has wiki/ uninitialized — measured here in three worker worktrees — so the sync script dies with FileNotFoundError and the Features file does not exist for them. Asking a worker for either is asking them to invent a pass.

So the split is: the member edits CLAUDE.md if the tool's described output changes, writes the Features entry as text in the PR body, and says plainly that it could not run the sync check. I run the sync check in the main clone and push the wiki myself before merging.

Delegating now.

## Decision, and a re-measurement of every number this ticket rests on Measured on `main` at 204da67, 2026-09-12. I re-ran the ticket's own commands rather than trust them, and **two of the line numbers had already drifted.** ### The zero still holds ``` grep -rnE '(poller|reaper|statusPoller|sessionReaper)\.health\(\)' fleetd/src/main/java --include='*.java' → exit 1, no output ``` Positive control, same idea against the test sources — non-empty, so the search really runs: ``` fleetd/src/test/java/dev/ltms/fleet/inject/StatusPollerWatchdogTest.java:6 fleetd/src/test/java/dev/ltms/fleet/session/SessionReaperWatchdogTest.java:6 ``` And the declarations, so the zero is about callers and not about the method existing: `StatusPoller.java:97` and `SessionReaper.java:82`. Still no production caller. ### The 503 constraint still holds, but the line numbers moved The ticket says `scripts/redeploy-fleetd.sh:1082` / `:1087`. They are now **`:1009` / `:1014`**. `scripts/rename-checkout.sh:448` is still `:448`. Locate them fresh; do not trust either number, including mine. The constraint itself is intact, and the second script's half is worse than the ticket describes: - `redeploy-fleetd.sh` `report_health()`: `503` → warn and continue (`:1010`). **Every other non-200 → `die`** (`:1014`). - `rename-checkout.sh` `:445-452`: a `case` on the code with arms for `200` and `503` only. An unknown code does **not** die immediately — it keeps polling until `HEALTH_WAIT` runs out and then dies with **"/healthz never answered within Ns"**. That message is wrong: the daemon answered every time. So a new status code here does not just break the script, it breaks it with a **misleading diagnosis**, on the day the fleet is already degraded. ### The body is free, and I measured why The ticket assumes neither script reads the body. Half true, and the true half is better than assumed. `redeploy-fleetd.sh`'s green path is a **body** predicate, not a code check: ```bash health_is_up() { [ -n "$1" ] } ``` Body non-empty. That is all. So **adding fields to the `/healthz` body costs nothing** for that script, and `rename-checkout.sh` reads only the status code. The body really is free. ## The decision **1. `/healthz` keeps its status codes exactly as they are.** 200 for ok, 503 for herdr unreachable. The loop states go in the **body only**. No new status code, and **do not reuse 503 for a stalled loop** — that is the one-symbol-two-states shape (#512) that `LoopWatchdog` was built to avoid, and putting it back one layer out would be a bad trade. **2. `fleet_list` is the primary surface.** It is the cheapest of the three, it reaches a lead directly, and it carries no script risk at all. Do this one properly; it is the deliverable. **3. `STOPPED` is never reported as an alarm.** Collapsing `STOPPED` into `STALLED` at the boundary throws away the whole reason the design has three states. A loop stopped on purpose is a fact, not a fault. **4. Option 3 — `HealthSnapshot` / `FleetHealthMonitor` — is out of scope for this unit.** `healthCoverage` still reports `detection-only`; changing what that word promises is its own decision and does not belong in the same PR. ### On the fleet01 lead's warning Keeping it, and it shapes the acceptance rather than just sitting in the ticket. **A monitor whose failure mode is a false positive gets muted, and then the real alarm is muted too.** So the test that a deliberately stopped loop is *not* surfaced as an alarm is not a nice-to-have — it is the test that decides whether anyone will still be listening the first time this fires for real. ### One thing the member cannot do, so it must not be asked of them The ticket's acceptance says the `CLAUDE.md` block and the wiki copy must stay in sync, and that a visible new behaviour earns a `wiki/11-Features.md` entry. **A member's provisioned worktree has `wiki/` uninitialized** — measured here in three worker worktrees — so the sync script dies with `FileNotFoundError` and the Features file does not exist for them. Asking a worker for either is asking them to invent a pass. So the split is: the member edits `CLAUDE.md` if the tool's described output changes, writes the Features entry **as text in the PR body**, and says plainly that it could not run the sync check. I run the sync check in the main clone and push the wiki myself before merging. Delegating now.
ltms commented 2026-09-12 15:04:54 +02:00
Author
Owner
Copy Link
Copy Source

HOLD on PR #579 — I mutated the production wiring and it survived. One more test is required before merge.

Everything the worker reported checks out. This is not a correction of their work; it is a gap their tests do not cover, found by the mutation I run on every PR against the half the worker did not change.

What I verified first (all of it good)

  • Merge is clean: 1 commit, 6 files, wiki pointer identical on both sides.
  • Baseline Tests run: 1771, Failures: 0, Errors: 0, Skipped: 0, confirmed twice — from the Maven Results: block, and from an independent sum over 131 surefire report files. That is the 1766 baseline plus the worker's 5 new tests.
  • FleetMcp.java and FleetApp.java restored byte-identical, as reported.

The mutation that survived

Fleetd.java:669 — located fresh, not from the ticket, because line numbers drift. Pristine anchor counted with grep -Fxc = 1.

// pristine
FleetMcp.LoopHealthSource loopHealth = new FleetMcp.LoopHealthSource(poller::health,
        () -> reaper == null ? LoopWatchdog.State.STOPPED : reaper.health());

// mutated — poller::health replaced by a constant
FleetMcp.LoopHealthSource loopHealth = new FleetMcp.LoopHealthSource(() -> LoopWatchdog.State.RUNNING,
        () -> reaper == null ? LoopWatchdog.State.STOPPED : reaper.health());

Anchor count 1 → 0, marker present. Result:

[INFO] Tests run: 1771, Failures: 0, Errors: 0, Skipped: 0
--- failure count: 0
[INFO] BUILD SUCCESS

The mutation survived. Fleetd.java has been restored; shasum -a 256 matches the pristine 75262d31753ea626.

What that means in production terms

The daemon can be changed to always report the StatusPoller as RUNNING — so the watchdog can never fire and a stalled poller is invisible — and all 1771 tests stay green.

That is precisely the false-negative this ticket exists to prevent. Worth naming the direction: the fleet01 lead's warning already on this ticket was about a monitoring component whose failure mode is a false positive getting muted. This is the mirror. A false negative is worse, because there is no noise for anyone to notice and then silence.

Why the tests miss it (I ruled out the alternatives)

Not a dead line, and not an excluded test group. The cause is instantiation:

  • All five new tests build their own LoopHealthSource with fixed lambdas, e.g. new FleetMcp.LoopHealthSource(() -> statusPoller, () -> sessionReaper). grep -rln 'LoopHealthSource' across the branch's test tree returns only FleetMcpTest.java and FleetAppTest.java.
  • So the tests prove the seam — LoopHealthSource reports what it is given — and prove nothing about what Fleetd.main gives it. This is the hand-built vs config-wired shape, the same one as #561.

What is required to merge

Add FleetdLoopHealthSourceWiringTest, following the 12 sibling wiring tests already in this project — specifically FleetdHealthCoverageSourceWiringTest, which is the direct analogue for healthCoverageSource(config).

Acceptance, and please read the last point before starting:

  1. The test fails when poller::health at Fleetd.java:669 is replaced by a constant () -> LoopWatchdog.State.RUNNING. Run that mutation yourself, paste the failure, then restore and paste the matching shasum -a 256. A test that is not red under that exact mutation is not the test being asked for.
  2. Same for the reaper half: the test fails when reaper.health() is replaced by a constant. Two halves means two assertions — one invariant wired at two places needs one assertion per place. A single combined assertion whose total is non-zero is how a gap at one half hides.
  3. Keep the reaper == null branch working. That null check is real behaviour (STOPPED when there is no reaper), so do not delete it to make the test easier — assert it instead, as a third case.
  4. loopHealth is built inline, and the sibling factories are not. capacitySource(...) at Fleetd.java:1003 and healthCoverageSource(...) at :1037 are package-private named factory methods, which is exactly what makes their wiring tests able to call them directly (Fleetd.healthCoverageSource(config)). loopHealth is an inline new inside main, so there is nothing for a test to call. Extract it to a package-private factory in the same style as those two, then test the factory. That extraction is the substance of this unit — without it there is no seam to pin, and the same mutation stays green whatever test you write.

The rest of PR #579 is good and stays as it is. This adds one test and one small extraction; do not widen it further.

## HOLD on PR #579 — I mutated the production wiring and it survived. One more test is required before merge. Everything the worker reported checks out. This is not a correction of their work; it is a gap their tests do not cover, found by the mutation I run on every PR against **the half the worker did not change**. ### What I verified first (all of it good) - Merge is clean: 1 commit, 6 files, wiki pointer identical on both sides. - Baseline **`Tests run: 1771, Failures: 0, Errors: 0, Skipped: 0`**, confirmed twice — from the Maven `Results:` block, and from an independent sum over 131 surefire report files. That is the 1766 baseline plus the worker's 5 new tests. - `FleetMcp.java` and `FleetApp.java` restored byte-identical, as reported. ### The mutation that survived `Fleetd.java:669` — located fresh, not from the ticket, because line numbers drift. Pristine anchor counted with `grep -Fxc` = 1. ```java // pristine FleetMcp.LoopHealthSource loopHealth = new FleetMcp.LoopHealthSource(poller::health, () -> reaper == null ? LoopWatchdog.State.STOPPED : reaper.health()); // mutated — poller::health replaced by a constant FleetMcp.LoopHealthSource loopHealth = new FleetMcp.LoopHealthSource(() -> LoopWatchdog.State.RUNNING, () -> reaper == null ? LoopWatchdog.State.STOPPED : reaper.health()); ``` Anchor count 1 → 0, marker present. Result: ``` [INFO] Tests run: 1771, Failures: 0, Errors: 0, Skipped: 0 --- failure count: 0 [INFO] BUILD SUCCESS ``` **The mutation survived.** `Fleetd.java` has been restored; `shasum -a 256` matches the pristine `75262d31753ea626`. ### What that means in production terms The daemon can be changed to **always report the `StatusPoller` as `RUNNING`** — so the watchdog can never fire and a stalled poller is invisible — and all 1771 tests stay green. That is precisely the false-negative this ticket exists to prevent. Worth naming the direction: the fleet01 lead's warning already on this ticket was about a monitoring component whose failure mode is a false **positive** getting muted. This is the mirror. A false **negative** is worse, because there is no noise for anyone to notice and then silence. ### Why the tests miss it (I ruled out the alternatives) Not a dead line, and not an excluded test group. The cause is instantiation: - **All five new tests build their own `LoopHealthSource`** with fixed lambdas, e.g. `new FleetMcp.LoopHealthSource(() -> statusPoller, () -> sessionReaper)`. `grep -rln 'LoopHealthSource'` across the branch's test tree returns only `FleetMcpTest.java` and `FleetAppTest.java`. - So the tests prove the *seam* — `LoopHealthSource` reports what it is given — and prove nothing about **what `Fleetd.main` gives it**. This is the hand-built vs config-wired shape, the same one as #561. ### What is required to merge Add **`FleetdLoopHealthSourceWiringTest`**, following the 12 sibling wiring tests already in this project — specifically `FleetdHealthCoverageSourceWiringTest`, which is the direct analogue for `healthCoverageSource(config)`. Acceptance, and please read the last point before starting: 1. The test **fails** when `poller::health` at `Fleetd.java:669` is replaced by a constant `() -> LoopWatchdog.State.RUNNING`. Run that mutation yourself, paste the failure, then restore and paste the matching `shasum -a 256`. **A test that is not red under that exact mutation is not the test being asked for.** 2. Same for the reaper half: the test fails when `reaper.health()` is replaced by a constant. Two halves means **two assertions** — one invariant wired at two places needs one assertion per place. A single combined assertion whose total is non-zero is how a gap at one half hides. 3. Keep the `reaper == null` branch working. That null check is real behaviour (`STOPPED` when there is no reaper), so do not delete it to make the test easier — assert it instead, as a third case. 4. **`loopHealth` is built inline, and the sibling factories are not.** `capacitySource(...)` at `Fleetd.java:1003` and `healthCoverageSource(...)` at `:1037` are package-private named factory methods, which is exactly what makes their wiring tests able to call them directly (`Fleetd.healthCoverageSource(config)`). `loopHealth` is an inline `new` inside `main`, so there is nothing for a test to call. **Extract it to a package-private factory in the same style as those two, then test the factory.** That extraction is the substance of this unit — without it there is no seam to pin, and the same mutation stays green whatever test you write. The rest of PR #579 is good and stays as it is. This adds one test and one small extraction; do not widen it further.
ltms commented 2026-09-12 15:30:43 +02:00
Author
Owner
Copy Link
Copy Source

Merged. The survivor is dead — I re-ran my own mutation and it is now caught.

Merged to main as part of 634d33b, via PR #584, which supersedes #579 (its branch is merged in as the base).

My own verification:

  • Fleetd.java:669 now reads loopHealthSource(poller, reaper) — the extracted factory is genuinely wired into main, not a parallel copy sitting beside an unchanged inline new. That was the thing most worth checking, because an extraction that leaves the original call site alone passes its own test and changes nothing.
  • Merged build with #571 and #581: Tests run: 1784, Failures: 0, Errors: 0, confirmed from the Results: block and an independent sum over 132 report files.
  • The exact mutation that survived before now fails. Replacing poller::health with () -> LoopWatchdog.State.RUNNING inside the factory is killed by FleetdLoopHealthSourceWiringTest.statusPollerHalfReflectsThePollersRealHealth:61. One test, not a crowd.

The worker improved on the brief and said so. I asked for "replace reaper.health() with a constant". They first did the literal thing — collapsing the whole ternary — found it killed both the reaper test and the null-reaper test, and recognised that as a less isolating mutation. They then did a surgical version that replaces only the reaper.health() sub-expression, keeps the reaper == null branch intact, and kills exactly one test, which additionally proves the third assertion is independent. They reported both, and chose the discriminating one. That is the right instinct: a kill that takes a crowd with it proves less than a kill of exactly one.

The framing I got wrong, corrected by the fleet01 lead. I described the five original tests as weak coverage of the wiring question. They are not weak — they are structurally zero. Every one of them built its own LoopHealthSource with fixed lambdas, and a test that supplies its own dependency is a test of the consumer that can never be evidence about the producer. So the survivor was not a puzzle to explain; it was the expected result. That is now written into the Features entry, so the next reader does not try to strengthen the five.

Context worth keeping. The same peer measured that a tree 91 commits back has only 2 Fleetd*WiringTest classes against 12 on main today. Ten arrived in those 91 commits. So this convention is new and actively being swept — #562 did not miss an old rule, it landed wiring while the sweep was in progress and did not join it. A clean sweep of Fleetd.java's constructor-arg method refs finds roughly 17 wiring sites against 12 test classes whose names do not map one-to-one. Worth its own ticket; not yet filed.

CLAUDE.md ↔ wiki/7-Use-Cases.md sync check run in the main clone: in sync after re-syncing the block, and the wiki is pushed and verified by ref. Features entry written.

Closing.

## Merged. The survivor is dead — I re-ran my own mutation and it is now caught. Merged to `main` as part of `634d33b`, via PR #584, which supersedes #579 (its branch is merged in as the base). **My own verification:** - `Fleetd.java:669` now reads `loopHealthSource(poller, reaper)` — the extracted factory is genuinely wired into `main`, not a parallel copy sitting beside an unchanged inline `new`. That was the thing most worth checking, because an extraction that leaves the original call site alone passes its own test and changes nothing. - Merged build with #571 and #581: **`Tests run: 1784, Failures: 0, Errors: 0`**, confirmed from the `Results:` block and an independent sum over 132 report files. - **The exact mutation that survived before now fails.** Replacing `poller::health` with `() -> LoopWatchdog.State.RUNNING` inside the factory is killed by `FleetdLoopHealthSourceWiringTest.statusPollerHalfReflectsThePollersRealHealth:61`. One test, not a crowd. **The worker improved on the brief and said so.** I asked for "replace `reaper.health()` with a constant". They first did the literal thing — collapsing the whole ternary — found it killed *both* the reaper test and the null-reaper test, and recognised that as a less isolating mutation. They then did a surgical version that replaces only the `reaper.health()` sub-expression, keeps the `reaper == null` branch intact, and kills exactly one test, which additionally proves the third assertion is independent. **They reported both, and chose the discriminating one.** That is the right instinct: a kill that takes a crowd with it proves less than a kill of exactly one. **The framing I got wrong, corrected by the fleet01 lead.** I described the five original tests as *weak* coverage of the wiring question. They are not weak — they are **structurally zero**. Every one of them built its own `LoopHealthSource` with fixed lambdas, and a test that supplies its own dependency is a test **of the consumer** that can never be evidence about **the producer**. So the survivor was not a puzzle to explain; it was the expected result. That is now written into the Features entry, so the next reader does not try to strengthen the five. **Context worth keeping.** The same peer measured that a tree 91 commits back has only **2** `Fleetd*WiringTest` classes against 12 on `main` today. Ten arrived in those 91 commits. So this convention is new and actively being swept — #562 did not miss an old rule, it landed wiring while the sweep was in progress and did not join it. A clean sweep of `Fleetd.java`'s constructor-arg method refs finds roughly 17 wiring sites against 12 test classes whose names do not map one-to-one. Worth its own ticket; not yet filed. `CLAUDE.md` ↔ `wiki/7-Use-Cases.md` sync check run in the main clone: **in sync** after re-syncing the block, and the wiki is pushed and verified by ref. Features entry written. Closing.
ltms closed this issue 2026-09-12 15:31:21 +02:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
blocked
Cannot start until something else lands. The body says what.
 needs-live-proof
Merged and green, but never shown working on the running daemon. Not the same as done.
 ready-to-delegate
Scope, files and acceptance criteria are written. A worker can be briefed from the body alone.
 silent-default
A feature that compiles, passes tests, and ships turned off. Nine recurrences and counting.
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
agent kevin ltms
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#562
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?