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

Follow-up to #275: a health-detected GONE member whose ask lapses may still strand its ticket #280

New Issue
Closed
opened 2026-09-04 05:19:53 +02:00 by ltms · 1 comment
ltms commented 2026-09-04 05:19:53 +02:00
Owner
Copy Link
Copy Source

Flagged by the #275 worker in its own report, rather than left quiet. Filing it so it is not lost.

The remaining gap

#275 fixed the definite teardown path: sessions.onRelease (an explicit fleet_stop or the idle reaper) now passes sweepAsking=true, so a member stopped while parked in fleet_ask has its ticket failed immediately.

FleetHealthMonitor deliberately still passes sweepAsking=false, and that is correct — a GONE/NEVER_READY reading is a classification from the live agent list, not a teardown the daemon performed, and an ASKING ticket must survive it (abandonDoesNotFailAnAsyncTicketWaitingForAnAnswer pins exactly that).

But that leaves a window. Suppose a member's pane dies so health reports GONE, while the session is not released:

  1. the GONE transition fires failTerminalTarget once → abandon(target, reason) with sweepAsking=false → the ASKING task is skipped, correctly.
  2. ~55–115s later the worker's own fleet_ask lapses and clears task.question to null. The task would now match abandon's filter.
  3. But nothing calls abandon again: reportTransition returns early on previous == next, and FleetHealth.decide returns GONE (from targetNotFound()) before it could ever return DELEGATION_ORPHANED, so the orphan state never surfaces either.

The ticket then sits at PENDING for good, exactly as in #275 — same destination, different road.

What I have and have not established

Established, by reading the code: the ordering in FleetHealth.decide, the previous == next early return, and terminal() being GONE || NEVER_READY only. Those three together mean nothing re-fires abandon on that target.

Not established: whether a GONE-but-unreleased session is eventually released anyway — by SessionReaper or another path. If it is, onRelease fires with sweepAsking=true and #275's fix already covers this case, making this ticket a no-op. If it is not, the ticket strands.

That question is the whole ticket. Answer it first, exactly as #275 was run: name the sequence, and if the reaper does release such a session, close this with the explanation and write no code. "Not reachable, here is why" is a good result.

If it is reachable

The worker's suggestion was to have FleetHealthMonitor retry the sweep on a non-transition tick. Be careful: the fire-once-per-transition rule exists because an earlier attempt called abandon() once per tick for as long as a member stayed terminal (CB-580), which was rejected. Any retry needs to be bounded and must not resurrect that behaviour.

A narrower option worth weighing first: once the ask has lapsed, the task is no longer ASKING, so a single delayed re-check for a target still in a terminal state may be enough — without a general per-tick retry.

Whatever the shape, keep the invariant #275 established: a health guess must never kill a ticket whose worker could still be answered by a live primary.

Flagged by the #275 worker in its own report, rather than left quiet. Filing it so it is not lost. ## The remaining gap #275 fixed the **definite teardown** path: `sessions.onRelease` (an explicit `fleet_stop` or the idle reaper) now passes `sweepAsking=true`, so a member stopped while parked in `fleet_ask` has its ticket failed immediately. `FleetHealthMonitor` deliberately still passes `sweepAsking=false`, and that is correct — a `GONE`/`NEVER_READY` reading is a classification from the live agent list, not a teardown the daemon performed, and an ASKING ticket must survive it (`abandonDoesNotFailAnAsyncTicketWaitingForAnAnswer` pins exactly that). But that leaves a window. Suppose a member's pane dies so health reports `GONE`, while the session is **not** released: 1. the GONE transition fires `failTerminalTarget` once → `abandon(target, reason)` with `sweepAsking=false` → the ASKING task is skipped, correctly. 2. ~55–115s later the worker's own `fleet_ask` lapses and clears `task.question` to `null`. The task would now match `abandon`'s filter. 3. But nothing calls `abandon` again: `reportTransition` returns early on `previous == next`, and `FleetHealth.decide` returns `GONE` (from `targetNotFound()`) before it could ever return `DELEGATION_ORPHANED`, so the orphan state never surfaces either. The ticket then sits at `PENDING` for good, exactly as in #275 — same destination, different road. ## What I have and have not established **Established, by reading the code:** the ordering in `FleetHealth.decide`, the `previous == next` early return, and `terminal()` being `GONE || NEVER_READY` only. Those three together mean nothing re-fires `abandon` on that target. **Not established:** whether a `GONE`-but-unreleased session is eventually released anyway — by `SessionReaper` or another path. If it is, `onRelease` fires with `sweepAsking=true` and #275's fix already covers this case, making this ticket a no-op. If it is not, the ticket strands. **That question is the whole ticket.** Answer it first, exactly as #275 was run: name the sequence, and if the reaper does release such a session, close this with the explanation and write no code. "Not reachable, here is why" is a good result. ## If it is reachable The worker's suggestion was to have `FleetHealthMonitor` retry the sweep on a non-transition tick. Be careful: the fire-once-per-transition rule exists because an earlier attempt called `abandon()` once per tick for as long as a member stayed terminal (CB-580), which was rejected. Any retry needs to be bounded and must not resurrect that behaviour. A narrower option worth weighing first: once the ask has lapsed, the task is no longer ASKING, so a *single* delayed re-check for a target still in a terminal state may be enough — without a general per-tick retry. Whatever the shape, keep the invariant #275 established: a health **guess** must never kill a ticket whose worker could still be answered by a live primary.
ltms commented 2026-09-04 06:33:36 +02:00
Author
Owner
Copy Link
Copy Source

Merged as b3f917e, corrected in ece2091. Real merge built green at 1301 tests.

Step 1 was answered, and the answer is "reachable"

This was the whole ticket, and the worker did the work rather than jumping to code. The chain, which I re-read myself:

  • reapIdle (SessionManager.java:844) skips anything that is not READY or DONE.
  • A member parked in fleet_ask is mid-turn, so it is BUSY — onDelivered (:731-744) sets it and nothing clears it until the turn ends.
  • FleetHealthMonitor's failTarget is wired straight to messages::abandon (Fleetd.java:559-561). It never touches SessionManager, never calls release.

So a GONE-but-never-stopped member stays BUSY forever, the reaper never reaches it, and #275's onRelease sweep never fires. Not a no-op.

The fix, and why the narrower shape was right

The worker took the second option — one bounded delayed re-check, not a per-tick retry. reportTransition's previous == next early return is untouched, so CB-580's rejected shape stays rejected.

I checked the delay arithmetic rather than taking it on trust. Both ask ceilings are 115s (FleetMcp.ASK_MAX_TIMEOUT_MS and FleetApp.MAX_ASK_TIMEOUT_MS, measured, not remembered), and the delay is 120s. The 5-second margin looked thin until I worked out the ordering: the ask always starts before GONE is observed, so the real margin is 5s plus however long the member had already been asking. It is guaranteed by the ordering, not by luck.

Two independent protections keep the extra attempt safe, and that redundancy is the right call:

  • sweepAsking stays false, so a member genuinely asking again is skipped exactly as on attempt one.
  • the re-check fires only if the target is still classified in that same terminal state.

My mutation — a different one from the worker's

The worker neutered recheckTerminalTarget's whole body, which proves the functional half (the ticket does get swept). It does not prove the safety half. So I removed only the states.get(target) != state guard and left the rest:

FleetHealthMonitorTest.recheckIsANoOpForATargetItNeverObserved:485
  an untracked/released target must not be reached into ==> expected: <0> but was: <1>
FleetHealthMonitorTest.recheckIsANoOpOnceTheTargetHasRecovered:473
  a recovered target must not be reached into again ==> expected: <1> but was: <2>

The guard is load-bearing, not decoration — the worker said so and it is true. Between the two mutations both halves are pinned.

The end-to-end test is the one that matters and it is honest work: it drives the real MessageService, proves the first sweep skips a genuinely-ASKING ticket, lets the ask lapse on its own, proves an unchanged tick still does not refire, and only then sweeps to FAILED.

My correction — a second task type now reads a plain HashMap

states is a bare HashMap (:41). That was safe while tick was the only thing touching it. This change adds a second, independently scheduled task that reads it.

They do not race today: Fleetd.java:554 builds the monitor's scheduler with Executors.newSingleThreadScheduledExecutor, so both tasks are serialised on one thread. I checked that rather than assuming it.

But nothing in FleetHealthMonitor enforces it, and the failure mode if someone ever swaps in a pool is bad in a specific way: an unsynchronised HashMap read racing a resize can spin a CPU forever rather than fail visibly. Made it a ConcurrentHashMap and wrote the reason at the field, including which maps are not concurrent and why — priors and orphanStreaks stay plain, because tick is still their only toucher.

The worker's own caveat, accepted

ASK_LAPSE_RECHECK_DELAY_SECONDS = 120 is a constant, not a config knob. The worker flagged this itself. I am leaving it as a constant — nothing here is worth a knob — but it is now in the wiki entry as a maintenance note, because raising either ask ceiling past 120s would make the re-check fire while the question is still open, find nothing, and spend the single attempt.

Wiki entry added: A ticket is not stranded when a dead member's question lapses.

Merged as `b3f917e`, corrected in `ece2091`. Real merge built green at **1301 tests**. ## Step 1 was answered, and the answer is "reachable" This was the whole ticket, and the worker did the work rather than jumping to code. The chain, which I re-read myself: - `reapIdle` (`SessionManager.java:844`) skips anything that is not `READY` or `DONE`. - A member parked in `fleet_ask` is mid-turn, so it is `BUSY` — `onDelivered` (`:731-744`) sets it and nothing clears it until the turn ends. - `FleetHealthMonitor`'s `failTarget` is wired straight to `messages::abandon` (`Fleetd.java:559-561`). It never touches `SessionManager`, never calls `release`. So a `GONE`-but-never-stopped member stays `BUSY` forever, the reaper never reaches it, and #275's `onRelease` sweep never fires. Not a no-op. ## The fix, and why the narrower shape was right The worker took the second option — one bounded delayed re-check, not a per-tick retry. `reportTransition`'s `previous == next` early return is untouched, so CB-580's rejected shape stays rejected. I checked the delay arithmetic rather than taking it on trust. Both ask ceilings are 115s (`FleetMcp.ASK_MAX_TIMEOUT_MS` and `FleetApp.MAX_ASK_TIMEOUT_MS`, measured, not remembered), and the delay is 120s. The 5-second margin looked thin until I worked out the ordering: the ask always *starts* before `GONE` is observed, so the real margin is 5s plus however long the member had already been asking. It is guaranteed by the ordering, not by luck. Two independent protections keep the extra attempt safe, and that redundancy is the right call: - `sweepAsking` stays `false`, so a member genuinely asking again is skipped exactly as on attempt one. - the re-check fires only if the target is *still* classified in that same terminal state. ## My mutation — a different one from the worker's The worker neutered `recheckTerminalTarget`'s whole body, which proves the *functional* half (the ticket does get swept). It does not prove the *safety* half. So I removed only the `states.get(target) != state` guard and left the rest: ``` FleetHealthMonitorTest.recheckIsANoOpForATargetItNeverObserved:485 an untracked/released target must not be reached into ==> expected: <0> but was: <1> FleetHealthMonitorTest.recheckIsANoOpOnceTheTargetHasRecovered:473 a recovered target must not be reached into again ==> expected: <1> but was: <2> ``` The guard is load-bearing, not decoration — the worker said so and it is true. Between the two mutations both halves are pinned. The end-to-end test is the one that matters and it is honest work: it drives the real `MessageService`, proves the first sweep skips a genuinely-ASKING ticket, lets the ask lapse on its own, proves an unchanged tick still does not refire, and only then sweeps to `FAILED`. ## My correction — a second task type now reads a plain HashMap `states` is a bare `HashMap` (`:41`). That was safe while `tick` was the only thing touching it. This change adds a **second, independently scheduled task** that reads it. They do not race today: `Fleetd.java:554` builds the monitor's scheduler with `Executors.newSingleThreadScheduledExecutor`, so both tasks are serialised on one thread. I checked that rather than assuming it. But nothing in `FleetHealthMonitor` enforces it, and the failure mode if someone ever swaps in a pool is bad in a specific way: an unsynchronised `HashMap` read racing a resize can spin a CPU forever rather than fail visibly. Made it a `ConcurrentHashMap` and wrote the reason at the field, including which maps are *not* concurrent and why — `priors` and `orphanStreaks` stay plain, because `tick` is still their only toucher. ## The worker's own caveat, accepted `ASK_LAPSE_RECHECK_DELAY_SECONDS = 120` is a constant, not a config knob. The worker flagged this itself. I am leaving it as a constant — nothing here is worth a knob — but it is now in the wiki entry as a maintenance note, because raising either ask ceiling past 120s would make the re-check fire while the question is still open, find nothing, and spend the single attempt. Wiki entry added: *A ticket is not stranded when a dead member's question lapses*.
ltms closed this issue 2026-09-04 06:33:48 +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#280
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?