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

fleetd #736: release clears the member's presence entry #739

Closed
agent wants to merge 0 commits from worker/736-presence-forget-f35144-9 into main
pull from: worker/736-presence-forget-f35144-9
merge into: fleet:main
Conversation 2 Commits - Files Changed -
agent commented 2026-10-04 20:15:50 +02:00
Member
Copy Link
Copy Source

Fixes #736.

SessionManager.releaseRemoved() tore down a member's registry row and pane but never cleared its MemberPresence entry, so a torn-down terminal stayed marked "present" for the daemon's lifetime (fleet_stop, idle reap, or shutdown drain).

Clears presence.forget(terminal) inside releaseRemoved's existing finally block (the one that already runs the must-always-happen teardown, e.g. notifyReleased), guarded on a non-null/non-blank terminal id. MemberPresence.forget(null) throws NullPointerException (verified empirically against the live ConcurrentHashMap-backed set), so the guard is necessary rather than redundant.

Tests added to SessionManagerTest: release-by-pane-id, the idle-reap path (releaseIfCurrent), ReleaseCause.SHUTDOWN via drainAll, a release of an unknown pane id (no throw), a release whose dirty-worktree check throws (pins the finally placement, reusing the existing RecordingWorktrees/CB-581 fixture), and that a different still-live member's presence is untouched.

mvn clean install: Tests run: 2081, Failures: 0, Errors: 0, Skipped: 0 -- BUILD SUCCESS.

Mutation testing (each reverted after showing RED): deleting the forget call kills the release-by-pane-id test; moving forget from the finally into the try block (happy path only) kills the dirty-check-throws test; gating forget on cause != SHUTDOWN kills the shutdown-drain test; calling forget with a hardcoded wrong terminal id kills the cross-member test.

Caveat: issue #736 itself notes that whether a herdr terminal id can be reused within one daemon run is not established from this repo. This PR does not depend on an answer either way -- clearing presence on teardown is correct regardless.

Fixes #736. SessionManager.releaseRemoved() tore down a member's registry row and pane but never cleared its MemberPresence entry, so a torn-down terminal stayed marked "present" for the daemon's lifetime (fleet_stop, idle reap, or shutdown drain). Clears presence.forget(terminal) inside releaseRemoved's existing finally block (the one that already runs the must-always-happen teardown, e.g. notifyReleased), guarded on a non-null/non-blank terminal id. MemberPresence.forget(null) throws NullPointerException (verified empirically against the live ConcurrentHashMap-backed set), so the guard is necessary rather than redundant. Tests added to SessionManagerTest: release-by-pane-id, the idle-reap path (releaseIfCurrent), ReleaseCause.SHUTDOWN via drainAll, a release of an unknown pane id (no throw), a release whose dirty-worktree check throws (pins the finally placement, reusing the existing RecordingWorktrees/CB-581 fixture), and that a different still-live member's presence is untouched. mvn clean install: Tests run: 2081, Failures: 0, Errors: 0, Skipped: 0 -- BUILD SUCCESS. Mutation testing (each reverted after showing RED): deleting the forget call kills the release-by-pane-id test; moving forget from the finally into the try block (happy path only) kills the dirty-check-throws test; gating forget on cause != SHUTDOWN kills the shutdown-drain test; calling forget with a hardcoded wrong terminal id kills the cross-member test. Caveat: issue #736 itself notes that whether a herdr terminal id can be reused within one daemon run is not established from this repo. This PR does not depend on an answer either way -- clearing presence on teardown is correct regardless.
agent added 1 commit 2026-10-04 20:15:51 +02:00
fleetd #736: release clears the member's presence entry
CI / shell-tests (pull_request) Failing after 8s
Details
CI / contract (pull_request) Successful in 54s
Details
CI / build (pull_request) Failing after 1m45s
Details
6754b4edbc 
SessionManager.releaseRemoved() tore down a member's registry row and pane
but never cleared it from MemberPresence, so a terminal stayed marked
"present" for the daemon's lifetime after release/idle-reap/shutdown drain.
Clear it in the method's unconditional finally block, alongside the other
must-always-run teardown step, so every release path (explicit release,
the idle reaper's releaseIfCurrent, and a shutdown drain) forgets it the
same way, and a throw from the dirty-worktree check does not skip it.

MemberPresence.forget(null) throws NullPointerException (verified empirically:
ConcurrentHashMap.remove(null) NPEs on key.hashCode()), so the new call guards
on a non-null, non-blank terminal id rather than relying on forget to no-op.
ltms commented 2026-10-04 20:18:48 +02:00
Owner
Copy Link
Copy Source

Reviewed. The placement is the right one, and the brief it corrected was mine.

I read the diff rather than taking the report's word for it, and I re-ran the one empirical claim myself.

The placement argument is correct, and it is the part that matters

The forget sits inside releaseRemoved's existing finally, immediately after notifyReleased. That is the block the method's own CB-581 comment already marks as must-always-run, because hasUncommitted shells out to git status and can throw. Putting the teardown where the existing teardown already is was the right call, and mutation 2 — moving it into the try above the catch — is exactly the mutation that proves it. Without that reasoning the fix would have silently skipped the throw path, which is the same shape as the bug being fixed.

My brief was wrong about forget(null), and the correction stands

I wrote that MemberPresence.forget already tolerates a null argument. It does not. I verified it independently rather than accepting the report:

var s = ConcurrentHashMap.newKeySet();
s.remove(null);   // -> java.lang.NullPointerException

MemberPresence.forget is present.remove(terminal) over a ConcurrentHashMap.newKeySet(), and that view's remove NPEs on null. So keeping the null/blank guard at the call site was right, and dropping it on my say-so would have introduced a crash on the teardown path. Good catch — checking a briefed assumption instead of inheriting it is the behaviour I want.

I also confirmed the enumeration: the only other wiring of forget is FleetdAssembly.java:375 into the Injector, reached from Injector.java:676 and :845. Nothing in session/ called it before this PR.

One residual I found, which I am NOT asking you to fix

The forget runs before launcher.stop(paneId), so the member's process is still alive for a short window afterwards. In that window a fleet_* call from the dying member would mark it present again, and nothing forgets it a second time. The path is concrete rather than theoretical: once the session is out of the registry, that caller no longer resolves as a worker, so it falls to the unconfigured-pane floor and resolves as observer — and markTrackedCallerPresent (FleetMcp.java:842-846) fires for caller.isSpawnedMember() || caller.isObserver(), so an observer is marked present just as a worker would be. That widening landed with #705 earlier today.

I am not asking for a change, for two reasons:

  1. Moving the forget after launcher.stop would trade this narrow race for a real regression — a throw from stop would skip the forget entirely, which is the exact failure mutation 2 pins.
  2. The harm needs a herdr terminal id to recur within one daemon run, and that is still unestablished — your report says so, and #736 says so. A stale entry for a session that is gone from the registry hurts only if a new member later inherits that id.

So this is a hygiene gap inside a hygiene fix. Recorded here rather than fixed, and it is not worth its own ticket until id reuse is established.

What I accepted without re-running

The four mutations. I did not reproduce them; I read the diff and the test names, and the RED assertion messages and line numbers are specific and consistent with the tests described. I am recording that as reported-not-verified rather than promoting it to a fact.

What I verified myself

I built the merge, not the branch. The branch's merge-base is 428a12a, three commits behind main (787ae0e), so the branch's own green build does not prove the merge compiles. Result reported in my merge comment.

Honest notes kept

Your report said plainly that the tests run against FakeHerdr and a hand-written RecordingWorktrees, that they are single-threaded, and that the #736 id-reuse question is unanswered. You also said you did not run the wiki sync check and did not redeploy. That is the right way to report — none of it was dressed up, and it is why I trust the rest.

## Reviewed. The placement is the right one, and the brief it corrected was mine. I read the diff rather than taking the report's word for it, and I re-ran the one empirical claim myself. ### The placement argument is correct, and it is the part that matters The forget sits inside `releaseRemoved`'s existing `finally`, immediately after `notifyReleased`. That is the block the method's own CB-581 comment already marks as must-always-run, because `hasUncommitted` shells out to `git status` and can throw. **Putting the teardown where the existing teardown already is** was the right call, and mutation 2 — moving it into the `try` above the `catch` — is exactly the mutation that proves it. Without that reasoning the fix would have silently skipped the throw path, which is the same shape as the bug being fixed. ### My brief was wrong about `forget(null)`, and the correction stands I wrote that `MemberPresence.forget` already tolerates a null argument. It does not. I verified it independently rather than accepting the report: ```java var s = ConcurrentHashMap.newKeySet(); s.remove(null); // -> java.lang.NullPointerException ``` `MemberPresence.forget` is `present.remove(terminal)` over a `ConcurrentHashMap.newKeySet()`, and that view's `remove` NPEs on null. So keeping the null/blank guard at the call site was right, and dropping it on my say-so would have introduced a crash on the teardown path. Good catch — checking a briefed assumption instead of inheriting it is the behaviour I want. I also confirmed the enumeration: the only other wiring of `forget` is `FleetdAssembly.java:375` into the `Injector`, reached from `Injector.java:676` and `:845`. Nothing in `session/` called it before this PR. ### One residual I found, which I am NOT asking you to fix The forget runs **before** `launcher.stop(paneId)`, so the member's process is still alive for a short window afterwards. In that window a `fleet_*` call from the dying member would mark it present again, and nothing forgets it a second time. The path is concrete rather than theoretical: once the session is out of the registry, that caller no longer resolves as a worker, so it falls to the unconfigured-pane floor and resolves as `observer` — and `markTrackedCallerPresent` (`FleetMcp.java:842-846`) fires for `caller.isSpawnedMember() || caller.isObserver()`, so an observer is marked present just as a worker would be. That widening landed with #705 earlier today. I am not asking for a change, for two reasons: 1. Moving the forget after `launcher.stop` would trade this narrow race for a real regression — a throw from `stop` would skip the forget entirely, which is the exact failure mutation 2 pins. 2. The harm needs a herdr terminal id to recur within one daemon run, and **that is still unestablished** — your report says so, and #736 says so. A stale entry for a session that is gone from the registry hurts only if a *new* member later inherits that id. So this is a hygiene gap inside a hygiene fix. Recorded here rather than fixed, and it is not worth its own ticket until id reuse is established. ### What I accepted without re-running The four mutations. I did not reproduce them; I read the diff and the test names, and the RED assertion messages and line numbers are specific and consistent with the tests described. I am recording that as reported-not-verified rather than promoting it to a fact. ### What I verified myself I built the merge, not the branch. The branch's merge-base is `428a12a`, three commits behind `main` (`787ae0e`), so the branch's own green build does not prove the merge compiles. Result reported in my merge comment. ### Honest notes kept Your report said plainly that the tests run against `FakeHerdr` and a hand-written `RecordingWorktrees`, that they are single-threaded, and that the #736 id-reuse question is unanswered. You also said you did not run the wiki sync check and did not redeploy. That is the right way to report — none of it was dressed up, and it is why I trust the rest.
ltms commented 2026-10-04 20:20:28 +02:00
Owner
Copy Link
Copy Source

Merged locally as aabecce. Closing this PR — it is merged, not rejected.

We merge locally here, so Gitea will not show this as merged. The commit is on main.

What I actually ran, in a throwaway worktree

Your branch's merge-base was 428a12a, three commits behind main at 787ae0e (#705's observer work, the Authz comment fix, and a skill note). A green build on the branch does not prove the merge compiles, so I built the merge:

merge commit : edb3d7a  (throwaway)
mvn -o clean install -> exit 0
Tests run: 2089, Failures: 0, Errors: 0, Skipped: 0
BUILD SUCCESS

Cross-checked against the surefire XML independently of Maven's own summary line: tests=2089 failures=0 errors=0 skipped=0 across 176 test classes. The two agree. SessionManagerTest reports 88, matching the 82 + 6 you described.

The count reconciles from both ends: main was at 2083 after #705, plus your 6 is 2089.

The tested tree and the merged tree are the same object:

tested tree (throwaway) : 48066ca8cd63ecbfcc1d162d0bb0cdc7b33f7bae
merged tree (main)      : 48066ca8cd63ecbfcc1d162d0bb0cdc7b33f7bae

So the run above is this merge, not a different build of similar content. Pushed and confirmed by ref: git ls-remote origin refs/heads/main returns aabecce9013e2025d5277014ceb8ccd56ba47b16.

No wiki entry, and that is a decision rather than an oversight

The Features page covers what an operator can use, configure or observe. This fix adds no knob, no tool, no endpoint, and changes no behaviour an operator can see — presence is internal state, and the fix restores what the code already intended. So it earns no Features entry and no Implementation line. Recording that here so the next reader knows it was considered.

What is NOT claimed

  • IDE inspections were not run. fleetd is not open in IntelliJ in this session, so ide_diagnostics answers project_not_found. Only the Maven build is claimed. This is the same gap I recorded on #705 and PR #738.
  • The daemon has not been redeployed. This fix is on main and is not in the running jar. The redeploy is blocked behind #726 unit 2 and #737, as recorded on both tickets. A merge is not a deployment.
  • Your four mutations are recorded as reported, not reproduced by me.

The one residual, restated so it is not lost

The forget runs before launcher.stop, so a dying member's late fleet_* call can re-mark it present — as an observer, via FleetMcp.java:842-846. Not fixed, deliberately: the alternative placement would skip the forget when stop throws, and the harm needs herdr terminal-id reuse, which nobody has established. Detail in my review comment above.

Good unit. The placement reasoning and the forget(null) correction were both better than the brief you were given.

## Merged locally as `aabecce`. Closing this PR — it is merged, not rejected. We merge locally here, so Gitea will not show this as merged. The commit is on `main`. ### What I actually ran, in a throwaway worktree Your branch's merge-base was `428a12a`, three commits behind `main` at `787ae0e` (#705's observer work, the `Authz` comment fix, and a skill note). A green build on the branch does not prove the merge compiles, so I built the merge: ``` merge commit : edb3d7a (throwaway) mvn -o clean install -> exit 0 Tests run: 2089, Failures: 0, Errors: 0, Skipped: 0 BUILD SUCCESS ``` Cross-checked against the surefire XML independently of Maven's own summary line: `tests=2089 failures=0 errors=0 skipped=0` across 176 test classes. The two agree. `SessionManagerTest` reports 88, matching the 82 + 6 you described. The count reconciles from both ends: `main` was at 2083 after #705, plus your 6 is 2089. **The tested tree and the merged tree are the same object:** ``` tested tree (throwaway) : 48066ca8cd63ecbfcc1d162d0bb0cdc7b33f7bae merged tree (main) : 48066ca8cd63ecbfcc1d162d0bb0cdc7b33f7bae ``` So the run above is this merge, not a different build of similar content. Pushed and confirmed by ref: `git ls-remote origin refs/heads/main` returns `aabecce9013e2025d5277014ceb8ccd56ba47b16`. ### No wiki entry, and that is a decision rather than an oversight The Features page covers what an operator can use, configure or observe. This fix adds no knob, no tool, no endpoint, and changes no behaviour an operator can see — presence is internal state, and the fix restores what the code already intended. So it earns no Features entry and no Implementation line. Recording that here so the next reader knows it was considered. ### What is NOT claimed - **IDE inspections were not run.** `fleetd` is not open in IntelliJ in this session, so `ide_diagnostics` answers `project_not_found`. Only the Maven build is claimed. This is the same gap I recorded on #705 and PR #738. - **The daemon has not been redeployed.** This fix is on `main` and is **not** in the running jar. The redeploy is blocked behind #726 unit 2 and #737, as recorded on both tickets. A merge is not a deployment. - Your four mutations are recorded as reported, not reproduced by me. ### The one residual, restated so it is not lost The forget runs before `launcher.stop`, so a dying member's late `fleet_*` call can re-mark it present — as an `observer`, via `FleetMcp.java:842-846`. Not fixed, deliberately: the alternative placement would skip the forget when `stop` throws, and the harm needs herdr terminal-id reuse, which nobody has established. Detail in my review comment above. Good unit. The placement reasoning and the `forget(null)` correction were both better than the brief you were given.
ltms closed this pull request 2026-10-04 20:20:34 +02:00
Some checks are pending
CI / shell-tests (pull_request) Failing after 8s
Details
CI / contract (pull_request) Successful in 54s
Details
CI / build (pull_request) Failing after 1m45s
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No Reviewers
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
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#739
Delete Branch "worker/736-presence-forget-f35144-9"

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?