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

A dead StatusPoller or SessionReaper can now be restarted, but nothing restarts it and nothing notices #544

New Issue
Closed
opened 2026-09-12 09:03:48 +02:00 by ltms · 5 comments
ltms commented 2026-09-12 09:03:48 +02:00
Owner
Copy Link
Copy Source

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

#538 fixed the part that made the failure permanent. This is the part it deliberately left open.

What #543 changed

Both StatusPoller.loop and SessionReaper.loop now catch Throwable per work item and continue,
and both wrap the whole loop in a finally that logs at ERROR and sets running = false:

} finally {
    if (running) {
        log.error("status poller loop exited unexpectedly; it can be restarted");
    }
    running = false;
}

Before this, a dead loop left running == true, and start() opens with if (running) return;,
so a restart was impossible even in principle. Now it is possible.

What is still missing — measured on main at 4a8a780 + PR #543

Nothing calls start() a second time.

$ grep -rn "StatusPoller\|SessionReaper" fleetd/src/main/java --include='*.java' \
    | grep -v "inject/StatusPoller.java:\|session/SessionReaper.java:"
fleetd/src/main/java/dev/ltms/fleet/Fleetd.java:321:  reaper = new SessionReaper(sessions, cfg.lifecycle().idleTtlSeconds());
fleetd/src/main/java/dev/ltms/fleet/Fleetd.java:533:  StatusPoller poller = new StatusPoller(router, injector, Injector.POLL_INTERVAL_MILLIS);

Each is constructed once and started once. The remaining hits in that grep are javadoc mentions in
StatusRefiner, Injector and HerdrPeerLauncher, not call sites.

So "it can be restarted" is true and nobody does it. The operational picture is better than
before #538 but still bad:

channel what it says when the poller loop has died
/healthz green — it still does not know this thread exists
the log one ERROR line, at the moment of death, then silence
fleet_list / fleet_status nothing about loop liveness
a fresh start() call would now work — but there is no caller

The single ERROR line is a real improvement over #538's silence. It is still a line in a log nobody
is tailing, on a daemon that keeps answering /healthz and keeps accepting fleet_send while
delivering nothing.

What is wanted

Two separate things. Either is useful alone; do not let the harder one block the easier one.

  1. Observe it. Make loop liveness a fact the fleet can report, not only a log line. The health
    snapshot is the obvious home — a dead injection poller is more urgent than anything else that
    snapshot currently carries. #538 listed this as "consider surfacing it, out of scope unless
    cheap", and it was correctly left out.
  2. Restart it. A supervisor that notices running == false on a loop that was never stopped,
    and calls start() again. Decide and write down what happens if it dies immediately and
    repeatedly — an unbounded restart loop hammering a broken herdr is its own outage. A bounded
    count, or a backoff, with the give-up state reported through item 1.

Item 2 without item 1 is a trap: a supervisor that silently restarts a loop that keeps dying hides
exactly the failure that #538 was filed about.

Please read before choosing a design

  • stop() also sets running = false. A supervisor must be able to tell "stopped on purpose" from
    "died", or it will fight stop() during shutdown. That is the same
    cannot-tell-no-from-cannot-tell shape that has bitten this repo before: if one flag has to carry
    two states that need opposite handling, it needs a third state, not a cleverer reader.
  • Both loops are singletons constructed in Fleetd.main. Whatever supervises them lives there or
    alongside; it is not a per-target concern.

Acceptance

  • A test that a loop which died abnormally is restarted, and resumes doing its work. Failing before
    the fix.
  • A test that a loop stopped by stop() is not restarted. This is the one that catches the
    two-states-one-flag defect, so it is not optional.
  • If item 1 lands: a test tying the reported liveness to a loop that actually died, so inverting
    the reported value fails a test.
  • For each new test: apply a mutation that should break it, show it going red with that test's own
    message, restore, confirm byte-identical with the full shasum -a 256, and run a green control.
  • No socket, no port bind, no spawn, nothing written outside a @TempDir.

Out of scope

  • Do not reopen the per-item catch (Throwable) decision from #538. It is merged and verified.
  • Do not install a process-wide default uncaught-exception handler. #538 ruled that out for the
    same reason it still applies: it hides per-loop recovery behind a global net.

Related: #538 (the fix this follows), #413 (the route that makes NoClassDefFoundError reachable
here), #412 (the same family in the completion thread).

Follow-up to #538, which merged as PR #543. Filed by me while verifying that merge. #538 fixed the part that made the failure permanent. This is the part it deliberately left open. ## What #543 changed Both `StatusPoller.loop` and `SessionReaper.loop` now catch `Throwable` per work item and continue, and both wrap the whole loop in a `finally` that logs at ERROR and sets `running = false`: ```java } finally { if (running) { log.error("status poller loop exited unexpectedly; it can be restarted"); } running = false; } ``` Before this, a dead loop left `running == true`, and `start()` opens with `if (running) return;`, so a restart was impossible even in principle. Now it is possible. ## What is still missing — measured on main at `4a8a780` + PR #543 Nothing calls `start()` a second time. ``` $ grep -rn "StatusPoller\|SessionReaper" fleetd/src/main/java --include='*.java' \ | grep -v "inject/StatusPoller.java:\|session/SessionReaper.java:" fleetd/src/main/java/dev/ltms/fleet/Fleetd.java:321: reaper = new SessionReaper(sessions, cfg.lifecycle().idleTtlSeconds()); fleetd/src/main/java/dev/ltms/fleet/Fleetd.java:533: StatusPoller poller = new StatusPoller(router, injector, Injector.POLL_INTERVAL_MILLIS); ``` Each is constructed once and started once. The remaining hits in that grep are javadoc mentions in `StatusRefiner`, `Injector` and `HerdrPeerLauncher`, not call sites. So `"it can be restarted"` is true and nobody does it. The operational picture is better than before #538 but still bad: | channel | what it says when the poller loop has died | |---|---| | `/healthz` | green — it still does not know this thread exists | | the log | **one** ERROR line, at the moment of death, then silence | | `fleet_list` / `fleet_status` | nothing about loop liveness | | a fresh `start()` call | would now work — but there is no caller | The single ERROR line is a real improvement over #538's silence. It is still a line in a log nobody is tailing, on a daemon that keeps answering `/healthz` and keeps accepting `fleet_send` while delivering nothing. ## What is wanted Two separate things. Either is useful alone; do not let the harder one block the easier one. 1. **Observe it.** Make loop liveness a fact the fleet can report, not only a log line. The health snapshot is the obvious home — a dead injection poller is more urgent than anything else that snapshot currently carries. #538 listed this as "consider surfacing it, out of scope unless cheap", and it was correctly left out. 2. **Restart it.** A supervisor that notices `running == false` on a loop that was never stopped, and calls `start()` again. Decide and write down what happens if it dies immediately and repeatedly — an unbounded restart loop hammering a broken herdr is its own outage. A bounded count, or a backoff, with the give-up state reported through item 1. Item 2 without item 1 is a trap: a supervisor that silently restarts a loop that keeps dying hides exactly the failure that #538 was filed about. ## Please read before choosing a design - `stop()` also sets `running = false`. A supervisor must be able to tell "stopped on purpose" from "died", or it will fight `stop()` during shutdown. That is the same cannot-tell-no-from-cannot-tell shape that has bitten this repo before: if one flag has to carry two states that need opposite handling, it needs a third state, not a cleverer reader. - Both loops are singletons constructed in `Fleetd.main`. Whatever supervises them lives there or alongside; it is not a per-target concern. ## Acceptance - A test that a loop which died abnormally is restarted, and resumes doing its work. Failing before the fix. - A test that a loop stopped by `stop()` is **not** restarted. This is the one that catches the two-states-one-flag defect, so it is not optional. - If item 1 lands: a test tying the reported liveness to a loop that actually died, so inverting the reported value fails a test. - For each new test: apply a mutation that should break it, show it going red with that test's own message, restore, confirm byte-identical with the full `shasum -a 256`, and run a green control. - No socket, no port bind, no spawn, nothing written outside a `@TempDir`. ## Out of scope - Do not reopen the per-item `catch (Throwable)` decision from #538. It is merged and verified. - Do not install a process-wide default uncaught-exception handler. #538 ruled that out for the same reason it still applies: it hides per-loop recovery behind a global net. Related: #538 (the fix this follows), #413 (the route that makes `NoClassDefFoundError` reachable here), #412 (the same family in the completion thread).
ltms commented 2026-09-12 09:14:55 +02:00
Author
Owner
Copy Link
Copy Source

Correction to my own ticket, before anyone starts it. As filed above, item 2 asks for a
supervisor that "notices running == false". That design is blind to the more likely failure.

The fleet01 lead measured the herdr call path and sent me the result. I re-measured it here rather
than take their word for it, because their tree is 91 commits behind mine and they said so
themselves.

Measured on main at 7611b69

UnixSocketHerdrClient.call() opens a SocketChannel, writes, and then reads with no deadline:

UnixSocketHerdrClient.java:71-79   try (SocketChannel ch = SocketChannel.open(StandardProtocolFamily.UNIX))
                                   ch.connect(...); writeFully(...); codec.decodeResult(readLine(ch))
UnixSocketHerdrClient.java:88-106  readLine(): while (true) { ... int n = ch.read(readBuf); ... }

No timeout mechanism exists anywhere under herdr/:

configureBlocking  0
Selector           0
setSoTimeout       0
SO_TIMEOUT         0
setOption          0
SO_RCVTIMEO        0
--- positive control, same directory, same command shape ---
SocketChannel      5
ch.read            1

The control is there because a zero match is a fact about the pattern until something proves the
search could hit. It could.

The call path has no wrapper timeout either: AgentControl.status → get(target) →
agentCall("agent.get", …) → herdr.call(…), and grep -c TimeUnit over StatusPoller.java,
AgentControl.java and UnixSocketHerdrClient.java is 0.

What that means for this ticket

StatusPoller.loop calls control.status(target) on the poller thread. A herdrd that accepts the
connection and never writes a line parks that thread forever.

Compare the two ways the loop stops working:

thread is running is isAlive() says what a liveness watchdog does
Error escapes the loop (#538) dead false (since #543) false finds it, restarts it
herdrd accepts and never answers alive, parked true true finds nothing, reports green

So a watchdog built to item 2 as I wrote it closes the door #538 came through and leaves this one
open, while saying everything is fine. That is a negative check over a channel that cannot carry the
failure — the same shape as #512.

Item 2 must be a PROGRESS watchdog, not a liveness watchdog. Record a last-completed-round
timestamp from inside each loop, and alarm on the timestamp being stale. That detects both cases: a
dead loop stops updating it, and a parked loop stops updating it.

The hang needs no Error at all, so it does not depend on #413 or on any of #538's reachability
argument. A herdrd that is up but wedged is an ordinary operational state.

One thing to check before designing the recovery

SocketChannel is an interruptible channel, so Thread.interrupt() on a thread blocked in
ch.read should close the channel and raise ClosedByInterruptException, which becomes an
IOException and then a HerdrException. If that holds, stop() followed by start() is a
working recovery lever for the parked case, not only the dead one.

I have not measured this. It is the documented JDK contract, not something I ran. Verify it with
a real test before building recovery on top of it.

Revised acceptance for item 2

Replacing the item 2 bullets above:

  • Each loop records a monotonic last-completed-round timestamp.
  • A test where the loop is parked in a herdr call that never returns, asserting the watchdog
    reports it. This is the test the original wording would have let a worker skip.
  • A test where the loop died, asserting the watchdog also reports that.
  • A test that a loop stopped by stop() is reported as stopped, not as stalled. stop() also sets
    running = false, so one flag carries two states that need opposite handling; that needs a third
    state, not a cleverer reader.
  • Report the exit code alongside any failure count.
**Correction to my own ticket, before anyone starts it.** As filed above, item 2 asks for a supervisor that "notices `running == false`". **That design is blind to the more likely failure.** The fleet01 lead measured the herdr call path and sent me the result. I re-measured it here rather than take their word for it, because their tree is 91 commits behind mine and they said so themselves. ## Measured on `main` at `7611b69` `UnixSocketHerdrClient.call()` opens a `SocketChannel`, writes, and then reads with no deadline: ``` UnixSocketHerdrClient.java:71-79 try (SocketChannel ch = SocketChannel.open(StandardProtocolFamily.UNIX)) ch.connect(...); writeFully(...); codec.decodeResult(readLine(ch)) UnixSocketHerdrClient.java:88-106 readLine(): while (true) { ... int n = ch.read(readBuf); ... } ``` No timeout mechanism exists anywhere under `herdr/`: ``` configureBlocking 0 Selector 0 setSoTimeout 0 SO_TIMEOUT 0 setOption 0 SO_RCVTIMEO 0 --- positive control, same directory, same command shape --- SocketChannel 5 ch.read 1 ``` The control is there because a zero match is a fact about the pattern until something proves the search could hit. It could. The call path has no wrapper timeout either: `AgentControl.status` → `get(target)` → `agentCall("agent.get", …)` → `herdr.call(…)`, and `grep -c TimeUnit` over `StatusPoller.java`, `AgentControl.java` and `UnixSocketHerdrClient.java` is **0**. ## What that means for this ticket `StatusPoller.loop` calls `control.status(target)` on the poller thread. A herdrd that accepts the connection and never writes a line parks that thread **forever**. Compare the two ways the loop stops working: | | thread is | `running` is | `isAlive()` says | what a liveness watchdog does | |---|---|---|---|---| | `Error` escapes the loop (#538) | dead | `false` (since #543) | `false` | finds it, restarts it | | herdrd accepts and never answers | **alive, parked** | **`true`** | **`true`** | **finds nothing, reports green** | So a watchdog built to item 2 as I wrote it closes the door #538 came through and leaves this one open, while saying everything is fine. That is a negative check over a channel that cannot carry the failure — the same shape as #512. **Item 2 must be a PROGRESS watchdog, not a liveness watchdog.** Record a last-completed-round timestamp from inside each loop, and alarm on the timestamp being stale. That detects both cases: a dead loop stops updating it, and a parked loop stops updating it. The hang needs no `Error` at all, so it does not depend on #413 or on any of #538's reachability argument. A herdrd that is up but wedged is an ordinary operational state. ## One thing to check before designing the recovery `SocketChannel` is an interruptible channel, so `Thread.interrupt()` on a thread blocked in `ch.read` should close the channel and raise `ClosedByInterruptException`, which becomes an `IOException` and then a `HerdrException`. If that holds, `stop()` followed by `start()` is a working recovery lever for the parked case, not only the dead one. **I have not measured this.** It is the documented JDK contract, not something I ran. Verify it with a real test before building recovery on top of it. ## Revised acceptance for item 2 Replacing the item 2 bullets above: - Each loop records a monotonic last-completed-round timestamp. - A test where the loop is **parked in a herdr call that never returns**, asserting the watchdog reports it. This is the test the original wording would have let a worker skip. - A test where the loop **died**, asserting the watchdog also reports that. - A test that a loop stopped by `stop()` is reported as stopped, not as stalled. `stop()` also sets `running = false`, so one flag carries two states that need opposite handling; that needs a third state, not a cleverer reader. - Report the exit code alongside any failure count.
ltms commented 2026-09-12 10:29:48 +02:00
Author
Owner
Copy Link
Copy Source

One note for whoever implements this, and it is an argument for the obvious choice rather than against it — so it belongs in a comment in the code, not just here.

System.nanoTime() is the right clock for this, and it is worth saying why

A progress watchdog compares "now" against a last-completed-round timestamp. The natural worry is that nanoTime is not wall-clock, and someone will later "fix" it to System.currentTimeMillis() or an Instant. That would be a regression.

nanoTime does not advance while this host is asleep. Measured here previously: ps etime reported 60,541s for the fleetd process while jcmd VM.uptime reported 2,966s — a 16-hour gap, all of it sleep. Every fleetd TTL and stall threshold already freezes with the host for that reason.

For this watchdog that behaviour is correct, not a bug:

  • While the host sleeps, now and lastRoundNanos freeze together, so the delta does not grow and the state stays RUNNING. The loop genuinely was not running, and alarming about it would be a false positive — nobody was being served either.
  • After wake, the delta resumes growing in awake-time. A loop that is really dead or parked crosses the threshold on real elapsed awake time, which is the quantity anyone reading the alarm actually cares about.

A wall-clock implementation would do the opposite: every laptop lid-close longer than the threshold would report STALLED on wake for a perfectly healthy daemon. That is the alarm-fatigue failure, and it would very likely get "fixed" by raising the threshold until the watchdog stops detecting anything.

So: use a monotonic LongSupplier (System::nanoTime live, a controllable stub in tests), and put the reason in a javadoc line on the field. The next person to look at it will otherwise see a non-wall-clock timestamp and assume it was an oversight.

Two smaller things while in there

  • Reset on start, not only in the constructor. If the watchdog is constructed well before start() runs, the first staleness judgement is measured from construction. A reset() called from start() — clearing any previous stop mark and re-stamping the timestamp — gives the loop a fresh grace period before its first round completes.
  • Derive the threshold, do not pick a number. From the loop's own interval (Injector.POLL_INTERVAL_MILLIS for the poller, the reaper's own) with generous headroom, and say in a comment how it was derived. A bare constant invites exactly the "just raise it" fix described above.
One note for whoever implements this, and it is an argument **for** the obvious choice rather than against it — so it belongs in a comment in the code, not just here. ## `System.nanoTime()` is the right clock for this, and it is worth saying why A progress watchdog compares "now" against a last-completed-round timestamp. The natural worry is that `nanoTime` is not wall-clock, and someone will later "fix" it to `System.currentTimeMillis()` or an `Instant`. That would be a regression. **`nanoTime` does not advance while this host is asleep.** Measured here previously: `ps etime` reported 60,541s for the fleetd process while `jcmd VM.uptime` reported 2,966s — a 16-hour gap, all of it sleep. Every fleetd TTL and stall threshold already freezes with the host for that reason. For this watchdog that behaviour is **correct, not a bug**: - While the host sleeps, `now` and `lastRoundNanos` freeze together, so the delta does not grow and the state stays `RUNNING`. The loop genuinely was not running, and alarming about it would be a false positive — nobody was being served either. - After wake, the delta resumes growing in awake-time. A loop that is really dead or parked crosses the threshold on real elapsed **awake** time, which is the quantity anyone reading the alarm actually cares about. A wall-clock implementation would do the opposite: every laptop lid-close longer than the threshold would report `STALLED` on wake for a perfectly healthy daemon. That is the alarm-fatigue failure, and it would very likely get "fixed" by raising the threshold until the watchdog stops detecting anything. **So: use a monotonic `LongSupplier` (`System::nanoTime` live, a controllable stub in tests), and put the reason in a javadoc line on the field.** The next person to look at it will otherwise see a non-wall-clock timestamp and assume it was an oversight. ## Two smaller things while in there - **Reset on start, not only in the constructor.** If the watchdog is constructed well before `start()` runs, the first staleness judgement is measured from construction. A `reset()` called from `start()` — clearing any previous stop mark and re-stamping the timestamp — gives the loop a fresh grace period before its first round completes. - **Derive the threshold, do not pick a number.** From the loop's own interval (`Injector.POLL_INTERVAL_MILLIS` for the poller, the reaper's own) with generous headroom, and say in a comment how it was derived. A bare constant invites exactly the "just raise it" fix described above.
ltms commented 2026-09-12 10:37:13 +02:00
Author
Owner
Copy Link
Copy Source

Follow-up, measured on main at 26f380a. This comment is newer than any brief you were sent — where it disagrees, this wins.

A watchdog nothing reads is a watchdog that cannot raise an alarm. state() has to reach a surface. Below is what I measured about where it can go and what it must not do when it gets there.

1. It does not belong in HealthSnapshot

fleetd/src/main/java/dev/ltms/fleet/health/HealthSnapshot.java:7-11
  public record HealthSnapshot(MemberSession.State sessionState, AgentStatus liveStatus, ...)

Every field there is per member, built once per member per tick at FleetHealthMonitor.java:192. A loop's progress is one daemon-wide fact. Putting it in a per-member record makes N copies of one fact and invites a reader to attribute a daemon-wide stall to whichever member it happened to read.

2. /healthz is the right surface — and 503 is the only safe non-200 code

FleetApp.java:293-330 reports herdr reachability and nothing else. A parked StatusPoller leaves it fully green today.

Before changing it, I measured both consumers the javadoc at :280-291 names:

scripts/redeploy-fleetd.sh:1074   curl -fsS ...            -> -f makes 503 a failure
scripts/redeploy-fleetd.sh:1079   if [ -z "$HEALTH_BODY" ]
scripts/redeploy-fleetd.sh:1082     if [ "$CODE" = "503" ]  -> warn, continues
scripts/redeploy-fleetd.sh:1087     else die               -> ANY OTHER non-200 kills the redeploy
scripts/rename-checkout.sh:448    503) HEALTH_OK=1; warn   -> also tolerated
scripts/rename-checkout.sh:452    otherwise die

So the javadoc's "both only check the HTTP status code" is understated: both special-case 503 as non-fatal and treat every other non-200 as fatal.

That gives a hard rule:

  • Adding a key to the 200 body is safe. Neither script parses a field.
  • Reporting a stalled loop as 503 is safe, and it is the honest code — the daemon is up and something in it is not working, which is exactly what 503 already means here.
  • Any other status code is not safe. A new 500 on a stalled poller would make redeploy-fleetd.sh die at :1087 and report a successful redeploy as a failure. That is precisely the #552 defect, re-created one layer up. Do not introduce one.

If you report 503, extend the detail/herdr wording so a reader can tell "herdr unreachable" from "a loop stalled". Both scripts print the body verbatim, so the distinction reaches a human for free. I would also add the loop states to the 200 body, so a RUNNING reading is positively visible and not merely the absence of an alarm.

3. One claim I checked and had wrong — do not re-derive it

I expected a parked StatusPoller to blind FleetHealthMonitor. It does not:

FleetHealthMonitor.java:165  agentsNow = agents.list();   // its own herdr call, per tick
FleetHealthMonitor.java:178  AgentStatus status = agent == null ? AgentStatus.UNKNOWN : agent.status();

liveStatus comes from the monitor's own agents.list(), independent of the poller.

The real coupling is session.state(). Session states move on injector.onStatus(...), which only the poller calls. So a parked poller freezes every session at BUSY, and then:

FleetHealthMonitor.java:184-185
  boolean stalled = session.state() == MemberSession.State.BUSY
      && stallElapsedNanos(...) >= workingSuspectAfterNanos;

turns true for every member at once. The failure is not silence — it is a fleet-wide false-positive storm with no field anywhere naming the single common cause. That is the strongest argument for surfacing state(): it converts N misleading per-member alarms into one true daemon-level fact.

4. Two smaller things

  • Write the sleep rationale on the clock field. LoopWatchdog.java:38 says "a monotonic elapsed-time clock", which says what it is and not why it must stay that way. nanoTime does not advance while this host sleeps — measured here: ps etime 60,541s against jcmd VM.uptime 2,966s. For this watchdog that is correct, because a frozen delta during sleep is a true RUNNING (nobody was being served either), while a wall clock would report STALLED after every lid-close. Without that sentence someone "fixes" it to an Instant and the alarm becomes noise.
  • The threshold derivations at StatusPoller.java:30-38 and SessionReaper.java:20-28 are right and I am not asking you to change them. Deriving from each loop's own interval with the reasoning written down is exactly what this needed.
Follow-up, measured on `main` at `26f380a`. **This comment is newer than any brief you were sent — where it disagrees, this wins.** A watchdog nothing reads is a watchdog that cannot raise an alarm. `state()` has to reach a surface. Below is what I measured about where it can go and what it must not do when it gets there. ## 1. It does not belong in `HealthSnapshot` ``` fleetd/src/main/java/dev/ltms/fleet/health/HealthSnapshot.java:7-11 public record HealthSnapshot(MemberSession.State sessionState, AgentStatus liveStatus, ...) ``` Every field there is **per member**, built once per member per tick at `FleetHealthMonitor.java:192`. A loop's progress is one daemon-wide fact. Putting it in a per-member record makes N copies of one fact and invites a reader to attribute a daemon-wide stall to whichever member it happened to read. ## 2. `/healthz` is the right surface — and 503 is the only safe non-200 code `FleetApp.java:293-330` reports herdr reachability and nothing else. A parked `StatusPoller` leaves it fully green today. Before changing it, I measured both consumers the javadoc at `:280-291` names: ``` scripts/redeploy-fleetd.sh:1074 curl -fsS ... -> -f makes 503 a failure scripts/redeploy-fleetd.sh:1079 if [ -z "$HEALTH_BODY" ] scripts/redeploy-fleetd.sh:1082 if [ "$CODE" = "503" ] -> warn, continues scripts/redeploy-fleetd.sh:1087 else die -> ANY OTHER non-200 kills the redeploy scripts/rename-checkout.sh:448 503) HEALTH_OK=1; warn -> also tolerated scripts/rename-checkout.sh:452 otherwise die ``` So the javadoc's "both only check the HTTP status code" is understated: **both special-case 503 as non-fatal and treat every other non-200 as fatal.** That gives a hard rule: - **Adding a key to the 200 body is safe.** Neither script parses a field. - **Reporting a stalled loop as 503 is safe**, and it is the honest code — the daemon is up and something in it is not working, which is exactly what 503 already means here. - **Any other status code is not safe.** A new 500 on a stalled poller would make `redeploy-fleetd.sh` `die` at `:1087` and report a *successful* redeploy as a failure. That is precisely the #552 defect, re-created one layer up. Do not introduce one. If you report 503, extend the `detail`/`herdr` wording so a reader can tell "herdr unreachable" from "a loop stalled". Both scripts print the body verbatim, so the distinction reaches a human for free. I would also add the loop states to the 200 body, so a `RUNNING` reading is positively visible and not merely the absence of an alarm. ## 3. One claim I checked and had wrong — do not re-derive it I expected a parked `StatusPoller` to blind `FleetHealthMonitor`. It does not: ``` FleetHealthMonitor.java:165 agentsNow = agents.list(); // its own herdr call, per tick FleetHealthMonitor.java:178 AgentStatus status = agent == null ? AgentStatus.UNKNOWN : agent.status(); ``` `liveStatus` comes from the monitor's own `agents.list()`, independent of the poller. The real coupling is `session.state()`. Session states move on `injector.onStatus(...)`, which only the poller calls. So a parked poller freezes every session at `BUSY`, and then: ``` FleetHealthMonitor.java:184-185 boolean stalled = session.state() == MemberSession.State.BUSY && stallElapsedNanos(...) >= workingSuspectAfterNanos; ``` turns true for **every** member at once. The failure is not silence — it is a fleet-wide false-positive storm with no field anywhere naming the single common cause. That is the strongest argument for surfacing `state()`: it converts N misleading per-member alarms into one true daemon-level fact. ## 4. Two smaller things - **Write the sleep rationale on the clock field.** `LoopWatchdog.java:38` says "a monotonic elapsed-time clock", which says what it is and not why it must stay that way. `nanoTime` does not advance while this host sleeps — measured here: `ps etime` 60,541s against `jcmd VM.uptime` 2,966s. For this watchdog that is correct, because a frozen delta during sleep is a true `RUNNING` (nobody was being served either), while a wall clock would report `STALLED` after every lid-close. Without that sentence someone "fixes" it to an `Instant` and the alarm becomes noise. - **The threshold derivations at `StatusPoller.java:30-38` and `SessionReaper.java:20-28` are right and I am not asking you to change them.** Deriving from each loop's own interval with the reasoning written down is exactly what this needed.
ltms commented 2026-09-12 10:56:27 +02:00
Author
Owner
Copy Link
Copy Source

Lead verification of PR #559 at 735b6af. This comment is newer than any brief — where it disagrees, this wins.

The PR is good. Verified on the merged tree (origin/main + 735b6af), not on the branch:

mvn clean install   MVN_EXIT=0
Tests run: 1742, Failures: 0, Errors: 0, Skipped: 0   (130 surefire report files)

The package move to dev.ltms.fleet.inject is right and I confirmed the reason independently before reading the worker's note: health imports session.MemberSession, and session imports inject.TurnListener / inject.MemberPresence, while inject imports neither. So LoopWatchdog in health would have closed inject → health → session → inject. Good catch, and PackageCyclesTest earning its keep is worth recording.

The threshold derivations (40x / 12x, each from its own loop's interval, with the reasoning written down) are exactly right and I am not asking for changes there.

One surviving mutant

I ran a mutation the worker did not: remove watchdog.reset() from StatusPoller.start() (:105).

exact-line anchor, pristine file : 1
exact-line anchor, mutated file  : 0      <- the mutation genuinely applied

(My first anchor was a regex on watchdog\.reset\(\), which still matched inside the comment I had inserted and read 1 — a false not-applied. Re-counted with exact full-line equality against a pristine control. Recording that because it is the same trap the suite's own \Q incident was.)

Result — the whole suite survives it:

mvn -q test    MVN_EXIT=0
report files: 130   Tests run: 1742, Failures: 0, Errors: 0
failing: none

Harness is live: StatusPollerWatchdogTest does go red for the worker's own M4 on stop(), so this is a real survivor and not a dead harness.

What the survivor costs

stoppedByCaller is sticky, and reset() is the only thing that clears it:

public void reset()               { stoppedByCaller = false; lastRoundNanos = nowNanos.getAsLong(); }
public void markStoppedByCaller() { stoppedByCaller = true; }
public State state()              { if (stoppedByCaller) { return State.STOPPED; } ... }

start() is reset()'s only caller. So without that one line, stop() then start() leaves health() reporting STOPPED forever while the loop is genuinely running. Both classes document start() as idempotent, and loop()'s own error line says "it can be restarted" — so restart is an anticipated path, and the watchdog would quietly lie about a live daemon on exactly that path.

The reset() call is already there and already correct. It simply has nothing pinning it.

The missing test — written and verified, not proposed

@Test
void aRestartedLoopReportsRunningAgainNotStoppedForever() throws Exception {
    AtomicLong clock = new AtomicLong(0);
    StatusPoller poller = new StatusPoller(new AgentControl(new IdleHerdr()), new Injector(new AgentControl(new IdleHerdr())),
            new StatusRefiner(new AgentControl(new IdleHerdr())), INTERVAL_MILLIS, clock::get);
    poller.start();
    poller.stop();
    poller.start(); // start() is documented idempotent, and loop() logs "it can be restarted"

    assertEquals(LoopWatchdog.State.RUNNING, poller.health(), "...");
}

pristine tree : Tests run: 5, Failures: 0          PRISTINE_TREE_EXIT=0
mutant        : Tests run: 5, Failures: 1          MUTANT_EXIT=1
  expected: <RUNNING> but was: <STOPPED>

Restored, shasum -a 256 -c OK.

SessionReaper has the identical sticky-flag shape and needs the twin test.

The shape, which is the reason this is worth a comment rather than a silent fix

This is the invariant-not-idiom case the fleet01 lead named, in its other direction. The reset() call is load-bearing, its absence has no symptom in any existing test, and the invariant it maintains is nameable in one sentence: an intentional stop must not outlive the restart that follows it. That sentence is the test. Had it been unnameable, the call would have been the suspicious thing instead.

Not in this PR, and that is fine

health() still has no production caller — I grepped the diff, and nothing touches FleetApp or /healthz. The worker scoped this as "the observability half only" and I agree that is a clean cut, so I am not holding the PR for it. The surfacing has real consumer constraints (measured in my earlier comment: both script consumers tolerate 503 specifically and die on any other non-200) and deserves its own unit. I will file it.

Lead verification of PR #559 at `735b6af`. **This comment is newer than any brief — where it disagrees, this wins.** The PR is good. Verified on the **merged** tree (`origin/main` + `735b6af`), not on the branch: ``` mvn clean install MVN_EXIT=0 Tests run: 1742, Failures: 0, Errors: 0, Skipped: 0 (130 surefire report files) ``` The package move to `dev.ltms.fleet.inject` is right and I confirmed the reason independently before reading the worker's note: `health` imports `session.MemberSession`, and `session` imports `inject.TurnListener` / `inject.MemberPresence`, while `inject` imports neither. So `LoopWatchdog` in `health` would have closed `inject → health → session → inject`. Good catch, and `PackageCyclesTest` earning its keep is worth recording. The threshold derivations (40x / 12x, each from its own loop's interval, with the reasoning written down) are exactly right and I am not asking for changes there. ## One surviving mutant I ran a mutation the worker did not: **remove `watchdog.reset()` from `StatusPoller.start()`** (`:105`). ``` exact-line anchor, pristine file : 1 exact-line anchor, mutated file : 0 <- the mutation genuinely applied ``` (My first anchor was a regex on `watchdog\.reset\(\)`, which still matched inside the comment I had inserted and read `1` — a false *not-applied*. Re-counted with exact full-line equality against a pristine control. Recording that because it is the same trap the suite's own `\Q` incident was.) Result — **the whole suite survives it**: ``` mvn -q test MVN_EXIT=0 report files: 130 Tests run: 1742, Failures: 0, Errors: 0 failing: none ``` Harness is live: `StatusPollerWatchdogTest` does go red for the worker's own M4 on `stop()`, so this is a real survivor and not a dead harness. ## What the survivor costs `stoppedByCaller` is **sticky**, and `reset()` is the **only** thing that clears it: ```java public void reset() { stoppedByCaller = false; lastRoundNanos = nowNanos.getAsLong(); } public void markStoppedByCaller() { stoppedByCaller = true; } public State state() { if (stoppedByCaller) { return State.STOPPED; } ... } ``` `start()` is `reset()`'s only caller. So without that one line, **`stop()` then `start()` leaves `health()` reporting `STOPPED` forever while the loop is genuinely running.** Both classes document `start()` as idempotent, and `loop()`'s own error line says *"it can be restarted"* — so restart is an anticipated path, and the watchdog would quietly lie about a live daemon on exactly that path. The `reset()` call is already there and already correct. It simply has nothing pinning it. ## The missing test — written and verified, not proposed ```java @Test void aRestartedLoopReportsRunningAgainNotStoppedForever() throws Exception { AtomicLong clock = new AtomicLong(0); StatusPoller poller = new StatusPoller(new AgentControl(new IdleHerdr()), new Injector(new AgentControl(new IdleHerdr())), new StatusRefiner(new AgentControl(new IdleHerdr())), INTERVAL_MILLIS, clock::get); poller.start(); poller.stop(); poller.start(); // start() is documented idempotent, and loop() logs "it can be restarted" assertEquals(LoopWatchdog.State.RUNNING, poller.health(), "..."); } ``` ``` pristine tree : Tests run: 5, Failures: 0 PRISTINE_TREE_EXIT=0 mutant : Tests run: 5, Failures: 1 MUTANT_EXIT=1 expected: <RUNNING> but was: <STOPPED> ``` Restored, `shasum -a 256 -c` **OK**. `SessionReaper` has the identical sticky-flag shape and needs the twin test. ## The shape, which is the reason this is worth a comment rather than a silent fix This is the **invariant-not-idiom** case the fleet01 lead named, in its other direction. The `reset()` call is load-bearing, its absence has no symptom in any existing test, and the invariant it maintains is nameable in one sentence: **an intentional stop must not outlive the restart that follows it.** That sentence is the test. Had it been unnameable, the call would have been the suspicious thing instead. ## Not in this PR, and that is fine `health()` still has **no production caller** — I grepped the diff, and nothing touches `FleetApp` or `/healthz`. The worker scoped this as "the observability half only" and I agree that is a clean cut, so I am not holding the PR for it. The surfacing has real consumer constraints (measured in my earlier comment: both script consumers tolerate **503** specifically and `die` on any other non-200) and deserves its own unit. I will file it.
ltms commented 2026-09-12 11:13:21 +02:00
Author
Owner
Copy Link
Copy Source

Merged as part of #559. Verified by the lead, on a merged tree — not on the branch.

Setup

main at 7a3b2bb + branch bfac141, merged in a throwaway worktree = 5de807f.
Baseline: mvn clean install → MVN_EXIT=0, Tests run: 1744, Failures: 0, Errors: 0, Skipped: 0, 130 surefire report files.

Mutations — three, all killed

Each one deleted or changed one line, counted the pristine full line by exact string equality (awk -v p="$PRISTINE" '$0==p'), kept a pristine copy as a control, and restored to a byte-identical file before the next run. This matters here: an earlier round of this same ticket produced a false "not applied" because the anchor regex matched inside the comment the mutation itself had inserted (comment 16944). Exact full-line equality has no such failure mode.

# Mutation Anchor count Killed by Observed value
M1 StatusPoller.java:105 — delete watchdog.reset(); from start() 1 → 0 (control still 1) StatusPollerWatchdogTest.aRestartedLoopReportsRunningAgainNotStoppedForever an intentional stop must not outlive the restart that follows it ==> expected: <RUNNING> but was: <STOPPED>
M2 LoopWatchdog.java:73 — delete lastRoundNanos = nowNanos.getAsLong(); from reset() 2 → 1 (control still 2) LoopWatchdogTest.resetClearsAPreviousStopAndTheStaleClock reset() must clear both the stop mark and the stale timestamp ==> expected: <RUNNING> but was: <STALLED>
M3 LoopWatchdog.java:89 — >= → > 1 → 0 (control still 1) LoopWatchdogTest.reportsStalledOnceTheLastRoundAgesPastTheThreshold expected: <STALLED> but was: <RUNNING>

M1 is the surviving mutant this rework was sent back for. It is now dead, and it dies with the invariant named in the failure message rather than a bare expected/was.

M2 and M3 are mutations the worker did not run. M2 in particular is a line whose count is 2, not 1 — recordRoundComplete() at :63 holds the identical text — so a naive 1 → 0 check would have read as "not applied" and scored a false survivor. It is 2 → 1, with the control still reading 2.

M2 was also a prediction I got wrong, and that is the useful part. I picked it expecting a survivor: the javadoc on reset() claims it "resets the clock, so the loop gets a fresh grace period", and the new restart test freezes its clock at 0, so that test cannot possibly see the difference. The harness caught it anyway, from a different test file. A comment claiming an invariant is a free test case — and here the assertion the sentence describes already existed.

Control build after all three restores: MVN_EXIT=0, Tests run: 1744, Failures: 0. git status --short empty in the verification worktree.

Scope, unchanged

Still the observability half only. health() is public and returns the three-state fact; nothing reads it yet. The surfacing unit — wiring health() into /healthz, fleet_list or fleet_status — is not filed yet and carries one measured constraint worth writing down before anyone designs it:

503 is the only safe non-200 code /healthz can return. Measured on main: scripts/redeploy-fleetd.sh:1082 treats 503 as a warning and :1087 calls die on any other non-200; rename-checkout.sh:448 treats 503 as OK and :452 calls die otherwise. So a new /healthz status code for a STALLED loop would break both scripts, while reusing 503 would make a stalled poller indistinguishable from an unreachable herdr.

Closing.

Merged as part of #559. Verified by the lead, on a **merged tree** — not on the branch. ## Setup `main` at `7a3b2bb` + branch `bfac141`, merged in a throwaway worktree = `5de807f`. Baseline: `mvn clean install` → `MVN_EXIT=0`, `Tests run: 1744, Failures: 0, Errors: 0, Skipped: 0`, 130 surefire report files. ## Mutations — three, all killed Each one deleted or changed **one** line, counted the pristine full line by exact string equality (`awk -v p="$PRISTINE" '$0==p'`), kept a pristine copy as a control, and restored to a byte-identical file before the next run. This matters here: an earlier round of this same ticket produced a **false "not applied"** because the anchor regex matched inside the comment the mutation itself had inserted (comment 16944). Exact full-line equality has no such failure mode. | # | Mutation | Anchor count | Killed by | Observed value | |---|---|---|---|---| | M1 | `StatusPoller.java:105` — delete `watchdog.reset();` from `start()` | 1 → 0 (control still 1) | `StatusPollerWatchdogTest.aRestartedLoopReportsRunningAgainNotStoppedForever` | `an intentional stop must not outlive the restart that follows it ==> expected: <RUNNING> but was: <STOPPED>` | | M2 | `LoopWatchdog.java:73` — delete `lastRoundNanos = nowNanos.getAsLong();` from `reset()` | 2 → 1 (control still 2) | `LoopWatchdogTest.resetClearsAPreviousStopAndTheStaleClock` | `reset() must clear both the stop mark and the stale timestamp ==> expected: <RUNNING> but was: <STALLED>` | | M3 | `LoopWatchdog.java:89` — `>=` → `>` | 1 → 0 (control still 1) | `LoopWatchdogTest.reportsStalledOnceTheLastRoundAgesPastTheThreshold` | `expected: <STALLED> but was: <RUNNING>` | **M1 is the surviving mutant this rework was sent back for.** It is now dead, and it dies with the invariant named in the failure message rather than a bare `expected/was`. **M2 and M3 are mutations the worker did not run.** M2 in particular is a line whose count is **2, not 1** — `recordRoundComplete()` at `:63` holds the identical text — so a naive 1 → 0 check would have read as "not applied" and scored a false survivor. It is 2 → 1, with the control still reading 2. **M2 was also a prediction I got wrong, and that is the useful part.** I picked it expecting a survivor: the javadoc on `reset()` claims it "resets the clock, so the loop gets a fresh grace period", and the new restart test freezes its clock at `0`, so that test cannot possibly see the difference. The harness caught it anyway, from a *different* test file. A comment claiming an invariant is a free test case — and here the assertion the sentence describes already existed. Control build after all three restores: `MVN_EXIT=0`, `Tests run: 1744, Failures: 0`. `git status --short` empty in the verification worktree. ## Scope, unchanged Still the observability half only. `health()` is public and returns the three-state fact; **nothing reads it yet.** The surfacing unit — wiring `health()` into `/healthz`, `fleet_list` or `fleet_status` — is not filed yet and carries one measured constraint worth writing down before anyone designs it: > **503 is the only safe non-200 code `/healthz` can return.** Measured on `main`: `scripts/redeploy-fleetd.sh:1082` treats 503 as a warning and `:1087` calls `die` on any other non-200; `rename-checkout.sh:448` treats 503 as OK and `:452` calls `die` otherwise. So a new `/healthz` status code for a STALLED loop would break both scripts, while reusing 503 would make a stalled poller indistinguishable from an unreachable herdr. Closing.
ltms closed this issue 2026-09-12 11:13:27 +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#544
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?