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

Investigate: abandon() skips a task in ASKING, so a torn-down member mid-fleet_ask may strand its ticket forever #275

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

Found by a bug-hunt fan-out. Unlike #272/#273/#274, I have NOT confirmed this one is reachable — I confirmed only that the code shape the reporter describes is really there. Reachability is the whole question, so this ticket is an investigation first and a fix only if the answer is yes.

The claim

MessageService.abandon() matches tasks with task.question == null:

for (Task task : tasks.values()) {
    if (target.equals(task.target) && task.question == null && !task.future.isDone()) {
        matching.add(task);
    }
}

hasOrphanedDelegation (line ~369) carries the same question == null guard.

So the claim is: if a member is torn down (pane crash, or fleet_stop) while its async ticket is parked in fleet_ask (Phase.ASKING), abandon() skips that task. resolveQuestion has already closed the forward waiter, so no waiter is failed either. The ticket's future is never completed: it stays pending forever, pruneTerminalTickets will not remove it because it is not terminal, and hasOrphanedDelegation cannot flag it because of the same guard.

The reporter adds that the worker's own fleet_ask eventually times out (~55s) and clears question to null, but by then FleetHealthMonitor.failTerminalTarget has already fired once on the GONE/NEVER_READY transition and no-opped, and DELEGATION_ORPHANED is not in terminal(), so nothing ever retries abandon().

What to do — in this order

  1. Answer the reachability question first. Name the exact sequence of public API calls that puts a task in this state and leaves it there. If no such sequence exists, say so, write no code, and close this with the explanation. A defect on paper is not a defect.
  2. Read the long javadoc above abandon() before you start. It already reasons carefully about which states this method can and cannot see, and it explicitly warns against "fixing" a gap with a test that reaches past the class to build a state production cannot reach. Do not add such a test here. If the only way to trigger this is to call Rendezvous.close yourself, that is the answer to step 1, and the answer is "not reachable".
  3. If — and only if — it is reachable, fix it: widen or drop the question == null condition so a torn-down target's ASKING task is completed as WORKER_FAILED and its reverse-rendezvous ask is closed (rendezvous.closeAsk). Consider whether hasOrphanedDelegation needs the same widening.
  4. Prove it with a test that drives the real path — spawn/send/ask/teardown through the public API — not one that calls abandon() with a hand-built task map. A test that reaches the seam directly would have passed every day this gap existed.

Report which of steps 1–4 you actually completed, and the sequence you found (or failed to find) in step 1. "Not reachable, here is why" is a fully acceptable and useful outcome.

Found by a bug-hunt fan-out. **Unlike #272/#273/#274, I have NOT confirmed this one is reachable** — I confirmed only that the code shape the reporter describes is really there. Reachability is the whole question, so this ticket is an investigation first and a fix only if the answer is yes. ## The claim `MessageService.abandon()` matches tasks with `task.question == null`: ```java for (Task task : tasks.values()) { if (target.equals(task.target) && task.question == null && !task.future.isDone()) { matching.add(task); } } ``` `hasOrphanedDelegation` (line ~369) carries the same `question == null` guard. So the claim is: if a member is torn down (pane crash, or `fleet_stop`) while its async ticket is parked in `fleet_ask` (`Phase.ASKING`), `abandon()` skips that task. `resolveQuestion` has already closed the forward waiter, so no waiter is failed either. The ticket's future is never completed: it stays pending forever, `pruneTerminalTickets` will not remove it because it is not terminal, and `hasOrphanedDelegation` cannot flag it because of the same guard. The reporter adds that the worker's own `fleet_ask` eventually times out (~55s) and clears `question` to `null`, but by then `FleetHealthMonitor.failTerminalTarget` has already fired once on the GONE/NEVER_READY transition and no-opped, and `DELEGATION_ORPHANED` is not in `terminal()`, so nothing ever retries `abandon()`. ## What to do — in this order 1. **Answer the reachability question first.** Name the exact sequence of public API calls that puts a task in this state and leaves it there. If no such sequence exists, say so, write no code, and close this with the explanation. A defect on paper is not a defect. 2. Read the long javadoc above `abandon()` before you start. It already reasons carefully about which states this method can and cannot see, and it explicitly warns against "fixing" a gap with a test that reaches past the class to build a state production cannot reach. **Do not add such a test here.** If the only way to trigger this is to call `Rendezvous.close` yourself, that is the answer to step 1, and the answer is "not reachable". 3. If — and only if — it is reachable, fix it: widen or drop the `question == null` condition so a torn-down target's ASKING task is completed as `WORKER_FAILED` and its reverse-rendezvous ask is closed (`rendezvous.closeAsk`). Consider whether `hasOrphanedDelegation` needs the same widening. 4. Prove it with a test that drives the **real** path — spawn/send/ask/teardown through the public API — not one that calls `abandon()` with a hand-built task map. A test that reaches the seam directly would have passed every day this gap existed. Report which of steps 1–4 you actually completed, and the sequence you found (or failed to find) in step 1. "Not reachable, here is why" is a fully acceptable and useful outcome.
ltms commented 2026-09-04 05:19:29 +02:00
Author
Owner
Copy Link
Copy Source

Merged to main in 66e5247 (PR #279). Reachable — the investigation answered yes, with a full public-API sequence.

The ticket's prescribed fix was wrong, and the worker was right to refuse it

I wrote "widen or drop the question == null condition". The worker found that doing so would break abandonDoesNotFailAnAsyncTicketWaitingForAnAnswer, an existing test that deliberately pins the opposite: an ASKING ticket must survive abandon(), because the primary may be mid-answer() for that very turn. Following my instruction literally would have traded this bug for a worse one — a shaky health reading killing a live conversation.

The fix it built instead splits the callers by what each actually knows:

Caller Knows sweepAsking
sessions.onRelease (fleet_stop, idle reaper) the pane is being stopped right now true — sweep
FleetHealthMonitor a GONE/NEVER_READY classification from the agent list false — unchanged

That is the right distinction, and it is one my brief did not make.

I verified the reachability chain rather than trusting it

Every link checked in the code:

  • FleetHealth.decide tests targetNotFound() → GONE before orphanedDelegation() → DELEGATION_ORPHANED, so once GONE the orphan state can never surface.
  • FleetHealthMonitor.reportTransition opens with if (previous == next) return; — the fire-once-per-transition gate.
  • terminal() is GONE || NEVER_READY only, so DELEGATION_ORPHANED never reaches failTerminalTarget.

So after the single GONE transition fires and no-ops on the ASKING task, nothing calls abandon() on that target again. The ticket sits in tasks forever: never terminal, so pruneTerminalTickets never drops it.

Verification

  • the real merge into current main builds green — 1283 tests, BUILD SUCCESS.
  • abandonDoesNotFailAnAsyncTicketWaitingForAnAnswer still passes, untouched by the diff.
  • my own mutation, run independently: reverting the sweepAsking || widening fails the new test with a released target's open ask can never resume, so it must fail right here ==> expected: <true> but was: <false>.
  • the new tests drive the real sendAsync → ask → abandon sequence, not a hand-built task map.

One deliberate widening beyond the ticket, which I checked and accept: asyncTasksByTurn.remove + rendezvous.closeAsk now run for any matched task carrying a turnId, not only the recovered-reply branch. For a task being failed because its target is gone, tearing down the dead turn is correct, and it stops hasAsyncQuestion reporting a finished turn as open.

Follow-up filed as #280 for the gap the worker flagged honestly rather than quietly leaving.

Merged to `main` in `66e5247` (PR #279). **Reachable — the investigation answered yes, with a full public-API sequence.** ## The ticket's prescribed fix was wrong, and the worker was right to refuse it I wrote "widen or drop the `question == null` condition". The worker found that doing so would break `abandonDoesNotFailAnAsyncTicketWaitingForAnAnswer`, an existing test that deliberately pins the opposite: an ASKING ticket must **survive** `abandon()`, because the primary may be mid-`answer()` for that very turn. Following my instruction literally would have traded this bug for a worse one — a shaky health reading killing a live conversation. The fix it built instead splits the callers by what each actually knows: | Caller | Knows | `sweepAsking` | |---|---|---| | `sessions.onRelease` (`fleet_stop`, idle reaper) | the pane is being stopped right now | `true` — sweep | | `FleetHealthMonitor` | a GONE/NEVER_READY *classification* from the agent list | `false` — unchanged | That is the right distinction, and it is one my brief did not make. ## I verified the reachability chain rather than trusting it Every link checked in the code: * `FleetHealth.decide` tests `targetNotFound()` → `GONE` **before** `orphanedDelegation()` → `DELEGATION_ORPHANED`, so once GONE the orphan state can never surface. * `FleetHealthMonitor.reportTransition` opens with `if (previous == next) return;` — the fire-once-per-transition gate. * `terminal()` is `GONE || NEVER_READY` only, so `DELEGATION_ORPHANED` never reaches `failTerminalTarget`. So after the single GONE transition fires and no-ops on the ASKING task, nothing calls `abandon()` on that target again. The ticket sits in `tasks` forever: never terminal, so `pruneTerminalTickets` never drops it. ## Verification * the real merge into current `main` builds green — **1283 tests**, `BUILD SUCCESS`. * `abandonDoesNotFailAnAsyncTicketWaitingForAnAnswer` still passes, untouched by the diff. * my own mutation, run independently: reverting the `sweepAsking ||` widening fails the new test with `a released target's open ask can never resume, so it must fail right here ==> expected: <true> but was: <false>`. * the new tests drive the real `sendAsync` → `ask` → `abandon` sequence, not a hand-built task map. One deliberate widening beyond the ticket, which I checked and accept: `asyncTasksByTurn.remove` + `rendezvous.closeAsk` now run for any matched task carrying a `turnId`, not only the recovered-reply branch. For a task being failed because its target is gone, tearing down the dead turn is correct, and it stops `hasAsyncQuestion` reporting a finished turn as open. Follow-up filed as #280 for the gap the worker flagged honestly rather than quietly leaving.
ltms closed this issue 2026-09-04 05:19:56 +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#275
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?