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

A released member's presence entry is never cleared: release() does not forget it, and only a later delivery attempt does #736

New Issue
Closed
opened 2026-10-04 19:28:36 +02:00 by ltms · 1 comment
ltms commented 2026-10-04 19:28:36 +02:00
Owner
Copy Link
Copy Source

Found while reviewing #722's PR (#735). Pre-existing and not caused by that change — filing it apart for the same reason #722 was split out of #705.

The enumeration

I enumerated the writers of MemberPresence, not the readers. presence.forget has exactly one wiring in src/main/java:

// FleetdAssembly.java:375
Injector injector = new Injector(router, turnListener, deliverable,
        presence::forget, Fleetd.turnRegistrar(completion));

and the Injector calls it at two places, both on a delivery attempt that finds the worker gone:

Injector.java:676   forget.accept(target);
Injector.java:845   forget.accept(target); // the worker is gone — clear its readiness/presence too (CB-114)

MemberRegistry.released(String) (auth/MemberRegistry.java:405) is about slot bindings and touches no presence.

Neither SessionManager.release nor releaseRemoved clears presence. I grepped both bodies: no presence, no forget. releaseRemoved does call memberLifecycle.released(removed.terminalId()), which is the slot path, not this one.

The consequence

fleet_stop → release → the pane is destroyed, and the terminal stays in MemberPresence's set for the daemon's lifetime. MemberPresence's own javadoc describes the set as "which workers are available", so after a stop it holds a terminal that is not available and not in the roster.

It is self-healing in the common case: nothing addresses a terminal that has left the roster, and the one path that would — a fleet_send{sessionId: "term_old"} — reaches the injector, which finds the pane gone and calls forget itself. So an operator typing a stale session id is cleaned up by the attempt.

Where it stops being harmless

The set is keyed on the terminal id. So the harm needs terminal-id reuse inside one daemon run: a new pane handed a terminal id that a stopped member already marked present. Then

  • the injector's gate (Fleetd.java:240, presence.isPresent(target)) says ready before that member's Claude has booted, which is exactly what MemberPresence exists to prevent — its javadoc: "Delivering into that boot window pastes into a not-yet-ready TUI (the text is lost) and wedges the worker's delivery state";
  • and after #722, reconcilePresence would additionally mark the new session READY at registration, on the strength of the previous member's contact.

Whether a herdr terminal id can recur is not established. It is the same open question #729 was gated on and that #734 records for three other identifiers: fleetd never mints one (AgentControl.resolveTarget only tests the term_ prefix) and herdr's source is not in this repository. Treat this as a defect on paper until someone names a path in. Presence is in-memory, so a daemon restart clears the set and the cross-boot case does not apply — unlike #734's, this needs reuse within one run.

Why #722's fix does not widen it

Worth stating so the two are not confused. The injector's gate is presence alone, so a stale entry already permits a delivery into the boot window today, with or without reconcilePresence. What #722 adds is that the bookkeeping now agrees with the delivery that would have happened anyway, and in that case it is the better outcome: the session holds its seat instead of looking reclaimable while a turn is being delivered into it.

Suggested shape

Clear presence where the session is torn down, so the set's lifetime matches the pane's rather than depending on a later delivery attempt. releaseRemoved is where the existing teardown lives, which is where this belongs.

Two notes for whoever takes it:

  • Do not break the lead and collaborator case. Fleetd.deliverableTo is presence.isPresent(target) || leads… || collaborators…, and FleetDeliverabilityTest already pins that forgetting a torn-down worker must not strip a lead or a collaborator of deliverability (forgetDoesNotDisarmALead). A new forget call must keep those green.
  • The test has to assert the clearing, not just the teardown. A test that stops a member and checks it left the roster passes today. The assertion that matters is presence.isPresent(terminal) being false afterwards, plus a control that it was true before — otherwise it passes because the terminal was never present at all.

Not measured

I did not reproduce any harm. What I measured is the writer enumeration above and the two absent call sites. Nobody has shown a terminal id recurring within one daemon run.

Found while reviewing #722's PR (#735). **Pre-existing and not caused by that change** — filing it apart for the same reason #722 was split out of #705. ## The enumeration I enumerated the **writers** of `MemberPresence`, not the readers. `presence.forget` has exactly one wiring in `src/main/java`: ```java // FleetdAssembly.java:375 Injector injector = new Injector(router, turnListener, deliverable, presence::forget, Fleetd.turnRegistrar(completion)); ``` and the `Injector` calls it at two places, both on a delivery attempt that finds the worker gone: ``` Injector.java:676 forget.accept(target); Injector.java:845 forget.accept(target); // the worker is gone — clear its readiness/presence too (CB-114) ``` `MemberRegistry.released(String)` (`auth/MemberRegistry.java:405`) is about slot bindings and touches no presence. **Neither `SessionManager.release` nor `releaseRemoved` clears presence.** I grepped both bodies: no `presence`, no `forget`. `releaseRemoved` does call `memberLifecycle.released(removed.terminalId())`, which is the slot path, not this one. ## The consequence `fleet_stop` → `release` → the pane is destroyed, and the terminal stays in `MemberPresence`'s set for the daemon's lifetime. `MemberPresence`'s own javadoc describes the set as "which workers are *available*", so after a stop it holds a terminal that is not available and not in the roster. It is **self-healing in the common case**: nothing addresses a terminal that has left the roster, and the one path that would — a `fleet_send{sessionId: "term_old"}` — reaches the injector, which finds the pane gone and calls `forget` itself. So an operator typing a stale session id is cleaned up by the attempt. ## Where it stops being harmless The set is keyed on the terminal id. So the harm needs **terminal-id reuse inside one daemon run**: a new pane handed a terminal id that a stopped member already marked present. Then - the injector's gate (`Fleetd.java:240`, `presence.isPresent(target)`) says ready before that member's Claude has booted, which is exactly what `MemberPresence` exists to prevent — its javadoc: "Delivering into that boot window pastes into a not-yet-ready TUI (the text is lost) and wedges the worker's delivery state"; - and after #722, `reconcilePresence` would additionally mark the new session `READY` at registration, on the strength of the *previous* member's contact. **Whether a herdr terminal id can recur is not established.** It is the same open question #729 was gated on and that #734 records for three other identifiers: fleetd never mints one (`AgentControl.resolveTarget` only tests the `term_` prefix) and herdr's source is not in this repository. Treat this as a defect on paper until someone names a path in. Presence is in-memory, so a daemon restart clears the set and the cross-boot case does not apply — unlike #734's, this needs reuse **within** one run. ## Why #722's fix does not widen it Worth stating so the two are not confused. The injector's gate is presence alone, so a stale entry already permits a delivery into the boot window today, with or without `reconcilePresence`. What #722 adds is that the bookkeeping now agrees with the delivery that would have happened anyway, and in that case it is the *better* outcome: the session holds its seat instead of looking reclaimable while a turn is being delivered into it. ## Suggested shape Clear presence where the session is torn down, so the set's lifetime matches the pane's rather than depending on a later delivery attempt. `releaseRemoved` is where the existing teardown lives, which is where this belongs. Two notes for whoever takes it: - **Do not break the lead and collaborator case.** `Fleetd.deliverableTo` is `presence.isPresent(target) || leads… || collaborators…`, and `FleetDeliverabilityTest` already pins that forgetting a torn-down worker must not strip a lead or a collaborator of deliverability (`forgetDoesNotDisarmALead`). A new forget call must keep those green. - **The test has to assert the clearing, not just the teardown.** A test that stops a member and checks it left the roster passes today. The assertion that matters is `presence.isPresent(terminal)` being false afterwards, plus a control that it was true before — otherwise it passes because the terminal was never present at all. ## Not measured I did not reproduce any harm. What I measured is the writer enumeration above and the two absent call sites. Nobody has shown a terminal id recurring within one daemon run.
ltms commented 2026-10-04 20:21:00 +02:00
Author
Owner
Copy Link
Copy Source

Fixed and merged to main as aabecce. Closing.

SessionManager.releaseRemoved now calls presence.forget(terminal) inside the finally that already carries the must-always-run teardown, next to notifyReleased. PR #739, merged locally and closed there.

Verified by building the merge, not the branch: 2089 tests, 0 failures, BUILD SUCCESS, and the tested tree and the merged tree are the same git object (48066ca8…). Details on the PR.

Two things this ticket raised that are now settled

MemberPresence.forget(null) throws. My brief asserted it was null-tolerant. It is not — present is a ConcurrentHashMap.newKeySet(), and that view's remove(null) throws NullPointerException. The implementer checked instead of believing me, and I reproduced it independently. The null/blank guard at the call site is load-bearing, not defensive clutter.

The forget enumeration was complete. The only other wiring is FleetdAssembly.java:375 into the Injector, reached from Injector.java:676 and :845. Nothing in session/ called it before this fix.

Two things this ticket raised that are NOT settled

Terminal-id reuse is still unestablished. Whether herdr can mint the same terminal id twice within one daemon run is not answered by this repo, and nothing in the fix or its tests answers it. The fix is correct either way — clearing state on teardown needs no justification from a reuse scenario — but the severity of the original bug remains unknown. Nobody should write down that this fixed a live incident, because no incident was observed.

A residual race, recorded on PR #739 rather than fixed. The forget runs before launcher.stop(paneId), so for a short window the member's process is still alive. A fleet_* call from it in that window re-marks it present: once the session is out of the registry the caller falls to the unconfigured-pane floor and resolves as observer, and markTrackedCallerPresent (FleetMcp.java:842-846) marks observers present just as it does workers — a widening that landed with #705 today.

I left it alone on purpose. Moving the forget after launcher.stop would trade a narrow race for a real regression, because a throw from stop would then skip the forget entirely — which is the exact failure the implementer's second mutation pins. And the harm needs the same unestablished id reuse. If id reuse is ever established, re-open this window as its own ticket; until then it is a hygiene gap inside a hygiene fix, and not worth a ticket of its own.

Deployment

Not deployed. This is on main and not in the running jar. The redeploy is held until #726 unit 2 and #737 are also in the tree, for the reason recorded on both: #737's fix must be in the jar before any handover runs the new restart path.

## Fixed and merged to `main` as `aabecce`. Closing. `SessionManager.releaseRemoved` now calls `presence.forget(terminal)` inside the `finally` that already carries the must-always-run teardown, next to `notifyReleased`. PR #739, merged locally and closed there. Verified by building the merge, not the branch: 2089 tests, 0 failures, `BUILD SUCCESS`, and the tested tree and the merged tree are the same git object (`48066ca8…`). Details on the PR. ### Two things this ticket raised that are now settled **`MemberPresence.forget(null)` throws.** My brief asserted it was null-tolerant. It is not — `present` is a `ConcurrentHashMap.newKeySet()`, and that view's `remove(null)` throws `NullPointerException`. The implementer checked instead of believing me, and I reproduced it independently. The null/blank guard at the call site is load-bearing, not defensive clutter. **The `forget` enumeration was complete.** The only other wiring is `FleetdAssembly.java:375` into the `Injector`, reached from `Injector.java:676` and `:845`. Nothing in `session/` called it before this fix. ### Two things this ticket raised that are NOT settled **Terminal-id reuse is still unestablished.** Whether herdr can mint the same terminal id twice within one daemon run is not answered by this repo, and nothing in the fix or its tests answers it. The fix is correct either way — clearing state on teardown needs no justification from a reuse scenario — but the *severity* of the original bug remains unknown. Nobody should write down that this fixed a live incident, because no incident was observed. **A residual race, recorded on PR #739 rather than fixed.** The forget runs before `launcher.stop(paneId)`, so for a short window the member's process is still alive. A `fleet_*` call from it in that window re-marks it present: once the session is out of the registry the caller falls to the unconfigured-pane floor and resolves as `observer`, and `markTrackedCallerPresent` (`FleetMcp.java:842-846`) marks observers present just as it does workers — a widening that landed with #705 today. I left it alone on purpose. Moving the forget after `launcher.stop` would trade a narrow race for a real regression, because a throw from `stop` would then skip the forget entirely — which is the exact failure the implementer's second mutation pins. And the harm needs the same unestablished id reuse. **If id reuse is ever established, re-open this window as its own ticket**; until then it is a hygiene gap inside a hygiene fix, and not worth a ticket of its own. ### Deployment **Not deployed.** This is on `main` and not in the running jar. The redeploy is held until #726 unit 2 and #737 are also in the tree, for the reason recorded on both: #737's fix must be in the jar before any handover runs the new restart path.
ltms closed this issue 2026-10-04 20:21:04 +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#736
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?