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

Restore a test for reapIdle's per-session guard, which #283 left uncovered #290

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

Flagged by the #283 worker in its own report rather than left quiet. Filing it so the coverage loss is on the record.

What happened

reapIdle wraps each release(...) in its own try/catch (CB-581), so one session that fails to release does not abort the whole reaping pass. The only test of that guard, SessionManagerTest.reapIdleSurvivesOneSessionThatFailsToRelease, triggered it by making the worktree removal throw.

#283 fixed release() so a failing worktree removal is now logged and swallowed instead of escaping. That is the right fix — but it also means the old test's trigger no longer reaches reapIdle's guard. The worker renamed it to reapIdleCountsAllThreeSessionsWhenOnlyItsWorktreeRemovalFails and corrected the count from 2 to 3, which is honest and correct for what the test now proves.

The result is that reapIdle's per-session try/catch is still in production, still correct, and no longer has a test.

The trigger to use

The removal path is closed, so the test needs a different way to make release() throw. Reading release() on main, the remaining ones are:

  • launcher.stop(paneId) at SessionManager.java:335 — the obvious one, and the failure CB-581 was really written for.
  • resolveAgentSessionId(removed, removedHandle) at :327, if the handle throws.

Do not try the release listener: notifyReleased (:461-470) already catches and logs a listener's RuntimeException, so it can never be the trigger.

The worker's stated blocker was that FakeHerdr's failure injection is global, not per-pane, so it cannot make exactly one session's stop fail. That is the work: give the fake per-pane failure injection, then assert that reaping three sessions where the middle one's launcher.stop throws still reaps the other two and logs the warning.

Scope

Test and test-infrastructure only. Do not change SessionManager. The guard is correct; it is the coverage that is missing.

Keep reapIdleCountsAllThreeSessionsWhenOnlyItsWorktreeRemovalFails as it is — it now proves something real and different (a swallowed removal failure does not reduce the reaped count). This ticket adds a second test beside it, it does not replace it.

Why bother

A guard with no test is the shape that has bitten this repo repeatedly: the code looks right, nothing exercises it, and it rots or gets refactored away silently. The per-pane injection is also reusable — several other one-session-fails cases in this suite would be easier to write once it exists.

Flagged by the #283 worker in its own report rather than left quiet. Filing it so the coverage loss is on the record. ## What happened `reapIdle` wraps each `release(...)` in its own try/catch (CB-581), so one session that fails to release does not abort the whole reaping pass. The only test of that guard, `SessionManagerTest.reapIdleSurvivesOneSessionThatFailsToRelease`, triggered it by making the worktree removal throw. #283 fixed `release()` so a failing worktree removal is now logged and swallowed instead of escaping. That is the right fix — but it also means the old test's trigger no longer reaches `reapIdle`'s guard. The worker renamed it to `reapIdleCountsAllThreeSessionsWhenOnlyItsWorktreeRemovalFails` and corrected the count from 2 to 3, which is honest and correct for what the test now proves. The result is that **`reapIdle`'s per-session try/catch is still in production, still correct, and no longer has a test.** ## The trigger to use The removal path is closed, so the test needs a different way to make `release()` throw. Reading `release()` on `main`, the remaining ones are: - **`launcher.stop(paneId)` at `SessionManager.java:335`** — the obvious one, and the failure CB-581 was really written for. - `resolveAgentSessionId(removed, removedHandle)` at `:327`, if the handle throws. Do **not** try the release listener: `notifyReleased` (`:461-470`) already catches and logs a listener's `RuntimeException`, so it can never be the trigger. The worker's stated blocker was that `FakeHerdr`'s failure injection is global, not per-pane, so it cannot make exactly one session's stop fail. That is the work: give the fake per-pane failure injection, then assert that reaping three sessions where the middle one's `launcher.stop` throws still reaps the other two and logs the warning. ## Scope Test and test-infrastructure only. **Do not change `SessionManager`.** The guard is correct; it is the coverage that is missing. Keep `reapIdleCountsAllThreeSessionsWhenOnlyItsWorktreeRemovalFails` as it is — it now proves something real and different (a swallowed removal failure does not reduce the reaped count). This ticket adds a second test beside it, it does not replace it. ## Why bother A guard with no test is the shape that has bitten this repo repeatedly: the code looks right, nothing exercises it, and it rots or gets refactored away silently. The per-pane injection is also reusable — several other one-session-fails cases in this suite would be easier to write once it exists.
ltms commented 2026-09-04 06:28:17 +02:00
Author
Owner
Copy Link
Copy Source

Merged as 61097e5, tidied in d5128a1. Real merge built green at 1297 tests.

The scope rule held: git diff origin/main...<branch> touches only FakeHerdr.java and SessionManagerTest.java. No main source, and reapIdleCountsAllThreeSessionsWhenOnlyItsWorktreeRemovalFails is byte-identical — this ticket added a second test beside it, as asked.

My mutation — a different one from the worker's

The worker deleted the try/catch entirely and quoted the resulting stack trace. That proves the test fails when the guard is absent. It does not prove the test checks the guard's actual job, which is to keep reaping the rest of the pass. So I mutated the other way: kept the catch and the WARN, and added a break.

SessionManagerTest.reapIdleSurvivesOneSessionWhoseLauncherStopFails:1126
  the middle session's launcher.stop failure is not counted as reaped, but must not abort
  reaping the other two ==> expected: <2> but was: <1>

A guard that catches and then stops is caught. Between the two mutations, both halves of CB-581's contract are pinned.

What I checked beyond the tests

paneCloseFailsForPane reads the pane_id out of the call params and falls back to the global paneCloseErrorCode, so the existing paneCloseFailsWith behaviour is unchanged — I confirmed no existing test was rewired.

The trigger choice is right. release() calls launcher.stop(paneId) at :335 with no try/catch of its own, and the worker's own stack trace shows the exception travelling FakeHerdr.call → AgentControl.close → HerdrPeerLauncher.stop → release → reapIdle. That is the real path, not a fake one.

I also confirmed the ticket's warning about the release listener: notifyReleased (:461-470) does swallow a listener's RuntimeException, so it could never have been the trigger.

Tidy-up in d5128a1

The new code wrote java.util.concurrent.ConcurrentHashMap and params instanceof java.util.Map<?, ?> fully qualified, in a file that already imports java.util.Map. Replaced with the plain names plus one import. No behaviour change.

The out-of-scope note was worth more than one line — filed as #293

The worker flagged that HerdrPeerLauncher.stop()'s spaces.closeTab(loc.tabId()) (:937) has no try/catch, unlike releaseZdotdir right below it. I traced it, and the consequence is worse than a leaked ZDOTDIR.

A non-not_found closeTab failure propagates out of stop() into release() at :335 — which is before the worktree removal. #283 wrapped that removal, but this throw never reaches it. The session is already deregistered by then, so a second stop is a no-op and there is no retry: the worktree leaks and is invisible to fleet_list. That is precisely the harm #283 was filed for, entering through a different door — the third instance of this shape in this neighbourhood after #274 and #283.

The new test in this PR already documents half of it without meaning to:

"the middle session's worktree removal never runs — release() throws before reaching it"

Correct about today's behaviour; #293 is about whether today's behaviour is right.

I checked the two neighbouring calls before filing, and both are safe: WorkspaceControl.locatePane catches every HerdrException and returns null, and tabPaneCount catches too. closeTab is the only bare call on that path.

Merged as `61097e5`, tidied in `d5128a1`. Real merge built green at **1297 tests**. The scope rule held: `git diff origin/main...<branch>` touches only `FakeHerdr.java` and `SessionManagerTest.java`. No main source, and `reapIdleCountsAllThreeSessionsWhenOnlyItsWorktreeRemovalFails` is byte-identical — this ticket added a second test beside it, as asked. ## My mutation — a different one from the worker's The worker deleted the try/catch entirely and quoted the resulting stack trace. That proves the test fails when the guard is **absent**. It does not prove the test checks the guard's actual job, which is to *keep reaping the rest of the pass*. So I mutated the other way: kept the catch and the WARN, and added a `break`. ``` SessionManagerTest.reapIdleSurvivesOneSessionWhoseLauncherStopFails:1126 the middle session's launcher.stop failure is not counted as reaped, but must not abort reaping the other two ==> expected: <2> but was: <1> ``` A guard that catches and then stops is caught. Between the two mutations, both halves of CB-581's contract are pinned. ## What I checked beyond the tests `paneCloseFailsForPane` reads the `pane_id` out of the call params and falls back to the global `paneCloseErrorCode`, so the existing `paneCloseFailsWith` behaviour is unchanged — I confirmed no existing test was rewired. The trigger choice is right. `release()` calls `launcher.stop(paneId)` at `:335` with no try/catch of its own, and the worker's own stack trace shows the exception travelling `FakeHerdr.call` → `AgentControl.close` → `HerdrPeerLauncher.stop` → `release` → `reapIdle`. That is the real path, not a fake one. I also confirmed the ticket's warning about the release listener: `notifyReleased` (`:461-470`) does swallow a listener's `RuntimeException`, so it could never have been the trigger. ## Tidy-up in `d5128a1` The new code wrote `java.util.concurrent.ConcurrentHashMap` and `params instanceof java.util.Map<?, ?>` fully qualified, in a file that already imports `java.util.Map`. Replaced with the plain names plus one import. No behaviour change. ## The out-of-scope note was worth more than one line — filed as #293 The worker flagged that `HerdrPeerLauncher.stop()`'s `spaces.closeTab(loc.tabId())` (`:937`) has no try/catch, unlike `releaseZdotdir` right below it. I traced it, and the consequence is worse than a leaked ZDOTDIR. A non-`not_found` `closeTab` failure propagates out of `stop()` into `release()` at `:335` — which is **before** the worktree removal. #283 wrapped that removal, but this throw never reaches it. The session is already deregistered by then, so a second stop is a no-op and there is no retry: the worktree leaks and is invisible to `fleet_list`. That is precisely the harm #283 was filed for, entering through a different door — the third instance of this shape in this neighbourhood after #274 and #283. The new test in this PR already documents half of it without meaning to: ```java "the middle session's worktree removal never runs — release() throws before reaching it" ``` Correct about today's behaviour; #293 is about whether today's behaviour is right. I checked the two neighbouring calls before filing, and both are safe: `WorkspaceControl.locatePane` catches every `HerdrException` and returns `null`, and `tabPaneCount` catches too. `closeTab` is the only bare call on that path.
ltms closed this issue 2026-09-04 06:28:29 +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#290
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?