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

awaitHerdr has two return-false paths and the caller reports both as "did not answer within 30s" #498

New Issue
Closed
opened 2026-09-12 04:21:39 +02:00 by ltms · 0 comments
ltms commented 2026-09-12 04:21:39 +02:00
Owner
Copy Link
Copy Source

Found by the #494 worker on a second pass, after I had already accepted this line as a false positive. My acceptance was wrong — I read only the deadline path and agreed too quickly, in writing, twice. Correcting it here.

The code

Fleetd.java:1703 awaitHerdr returns false from two places:

} catch (HerdrException e) {
    if (System.nanoTime() >= deadline) {
        return false;                       // (1) budget genuinely exhausted
    }
    ...
    try {
        Thread.sleep(HERDR_WAIT_POLL_MILLIS);
    } catch (InterruptedException ie) {
        Thread.currentThread().interrupt();
        return false;                       // (2) interrupted — can be milliseconds in
    }
}

The single caller, Fleetd.java:279, reports both the same way:

log.warn("herdr did not answer within {}s — starting anyway; /healthz will report "
        + "degraded until it comes up. Orphaned worker panes (if any) were NOT reaped.",
        HERDR_WAIT_SECONDS);

Why it is two defects at once, not one

#494's shape — a configured value printed as a measurement. On path (2) the wait can have lasted a few milliseconds, and the line still says within 30s. An operator reads that as thirty seconds of patience that were actually never spent. Path (1) is honest; path (2) is confidently wrong.

#497's shape — one symbol for two states that need opposite handling. false means both "herdr is genuinely down" and "we were interrupted before we could find out". Those call for different responses: the first is a real degraded start, the second says nothing at all about herdr. The caller cannot tell them apart, so it asserts the first.

The consequence lands where it is least welcome. An interrupt during startup or shutdown produces a confident "herdr did not answer within 30s" plus "orphaned worker panes were NOT reaped" — and the reaping claim may also be false, since nothing was actually established. Shutdown is exactly the window #493 is about, so this line can misdirect the next person reading that part of the log.

Asked for

  1. Distinguish the two outcomes. An enum or a small record beats a boolean here — the same reasoning as #497, where the fix is a third state and not a better number.
  2. Path (1) should log the measured elapsed time next to the configured budget, per #494's rule: configured=30s elapsed=30.0s.
  3. Path (2) should log that the wait was interrupted, with how long it actually ran, and must not claim anything about whether herdr is up. It should also not claim orphan panes were not reaped, unless that is separately true.
  4. A test per path. Per #497's testing note, a test that constructs the interrupted outcome directly proves the caller handles it; it does not prove awaitHerdr ever returns it. Drive the real method and interrupt the thread.

Reachability, stated honestly

Path (2) needs a thread interrupt during the herdr wait, which in practice means startup racing a shutdown. It is rare. It is not unreachable, and "rare" is exactly when an operator most needs the log to be true, because there is no second instrument to check it against.

My error, recorded

The worker's first report listed this as a false positive, reasoning that awaitHerdr only returns false at the deadline. I read the deadline path, agreed, and wrote that agreement into a reply to the worker and into a summary. I had not read to the end of the method. A second pass by the same worker found the interrupt path and corrected it.

The lesson is the one already on #494: count the exits before judging a log line. One return false that matches the message does not mean every return false does.

Related: #494 (constant presented as measurement), #497 (sentinel conflating "no" with "could not tell"), #493 (shutdown is where this misleads).

Found by the #494 worker on a second pass, after I had already accepted this line as a false positive. **My acceptance was wrong** — I read only the deadline path and agreed too quickly, in writing, twice. Correcting it here. ## The code `Fleetd.java:1703` `awaitHerdr` returns `false` from **two** places: ```java } catch (HerdrException e) { if (System.nanoTime() >= deadline) { return false; // (1) budget genuinely exhausted } ... try { Thread.sleep(HERDR_WAIT_POLL_MILLIS); } catch (InterruptedException ie) { Thread.currentThread().interrupt(); return false; // (2) interrupted — can be milliseconds in } } ``` The single caller, `Fleetd.java:279`, reports both the same way: ```java log.warn("herdr did not answer within {}s — starting anyway; /healthz will report " + "degraded until it comes up. Orphaned worker panes (if any) were NOT reaped.", HERDR_WAIT_SECONDS); ``` ## Why it is two defects at once, not one **#494's shape — a configured value printed as a measurement.** On path (2) the wait can have lasted a few milliseconds, and the line still says `within 30s`. An operator reads that as thirty seconds of patience that were actually never spent. Path (1) is honest; path (2) is confidently wrong. **#497's shape — one symbol for two states that need opposite handling.** `false` means both "herdr is genuinely down" and "we were interrupted before we could find out". Those call for different responses: the first is a real degraded start, the second says nothing at all about herdr. The caller cannot tell them apart, so it asserts the first. The consequence lands where it is least welcome. An interrupt during startup or shutdown produces a confident "herdr did not answer within 30s" plus "orphaned worker panes were NOT reaped" — and the reaping claim may also be false, since nothing was actually established. Shutdown is exactly the window #493 is about, so this line can misdirect the next person reading that part of the log. ## Asked for 1. Distinguish the two outcomes. An enum or a small record beats a `boolean` here — the same reasoning as #497, where the fix is a third state and not a better number. 2. Path (1) should log the **measured** elapsed time next to the configured budget, per #494's rule: `configured=30s elapsed=30.0s`. 3. Path (2) should log that the wait was **interrupted**, with how long it actually ran, and must not claim anything about whether herdr is up. It should also not claim orphan panes were not reaped, unless that is separately true. 4. A test per path. Per #497's testing note, a test that constructs the interrupted outcome directly proves the caller handles it; it does not prove `awaitHerdr` ever returns it. Drive the real method and interrupt the thread. ## Reachability, stated honestly Path (2) needs a thread interrupt during the herdr wait, which in practice means startup racing a shutdown. It is rare. It is not unreachable, and "rare" is exactly when an operator most needs the log to be true, because there is no second instrument to check it against. ## My error, recorded The worker's first report listed this as a false positive, reasoning that `awaitHerdr` only returns false at the deadline. I read the deadline path, agreed, and wrote that agreement into a reply to the worker and into a summary. I had not read to the end of the method. A second pass by the same worker found the interrupt path and corrected it. The lesson is the one already on #494: **count the exits before judging a log line.** One `return false` that matches the message does not mean every `return false` does. Related: #494 (constant presented as measurement), #497 (sentinel conflating "no" with "could not tell"), #493 (shutdown is where this misleads).
ltms closed this issue 2026-09-12 05:01:33 +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#498
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?