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

A failing tab.close during teardown leaks both the ZDOTDIR and the worktree #293

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

Spotted by the #290 worker in its out-of-scope note. I read the code myself and traced the consequences, which are worse than the note said, so I am filing it rather than dropping it.

Same shape as #283 and #274: cleanup wrapped on one branch, bare on its sibling.

The bare call

HerdrPeerLauncher.stop() (:920-950) runs three teardown steps. Two are guarded, the middle one is not:

try {
    agents.close(paneId);                       // guarded: tolerates *_not_found
} catch (HerdrException e) {
    if (!isAlreadyGone(e)) throw e;
    log.debug(...);
}
if (loc != null && loc.tabPaneCount() == 1) {
    spaces.closeTab(loc.tabId());               // :937 — BARE
} else if (loc != null) { ... }
// CB-633: ... Last in, best-effort — a failure here must not mask a real teardown failure above.
try {
    releaseZdotdir(paneId);                     // guarded
} catch (RuntimeException e) {
    log.warn(...);
}

WorkspaceControl.closeTab (:122-129) swallows *_not_found only. Every other herdr error propagates out of stop().

I checked the two neighbouring suspicions and both are fine — do not chase them:

  • spaces.locatePane(paneId) (WorkspaceControl:143-154) catches every HerdrException and returns null, so it never propagates.
  • tabPaneCount (:157-167) catches too, returning -1.

So closeTab is the only bare call on this path.

Why it matters — two leaks, not one

1. The ZDOTDIR leaks. releaseZdotdir never runs. That directory holds the generated memberCredentials allow-list scrub files, and its allowed N of M report is never read back or logged — so the operator loses the one line that says how many variables the scrub actually blanked. Self-healing is slow but real: EnvAllowListScrub.reapOrphans removes directories older than 24h on the next generate, and deleteOnExit catches the rest at daemon restart.

2. The worktree leaks, and this one does not self-heal. The exception propagates into SessionManager.release() at :335, which is before the worktree removal at the end of that method. #283 wrapped that removal — but this throw happens earlier, so the removal is never reached at all. By then registry.remove(paneId) has already run, so the session is deregistered and a second stop is a no-op: there is no retry path, and the directory is invisible to fleet_list. That is exactly the harm #283 exists to prevent, arriving through a different door.

The new #290 test already documents half of this without meaning to:

assertEquals(List.of(a.worktree(), c.worktree()), worktrees.removeCalls()...,
        "the middle session's worktree removal never runs — release() throws before reaching it");

That assertion is correct about today's behaviour. This ticket is about whether today's behaviour is right.

What to decide

The comment above releaseZdotdir states the rule this file already believes: "a failure here must not mask a real teardown failure above". A failing closeTab is not a real teardown failure above — the pane is already closed by then. It is cosmetic workspace tidying, and it is masking two real cleanups below it.

So the obvious fix is to wrap closeTab in a try/catch with a WARN naming the tab, and carry on. But decide it, do not just apply it, and put the reasoning in your report:

  • Is there any caller that genuinely needs a closeTab failure to surface as a failed stop? Check SessionManager.release, FleetMcp.stop (which catches only HerdrException) and FleetApp.stopMember (which catches nothing).
  • If you wrap it, the WARN must name the tab id and say the tab may need manual cleanup — a silently swallowed failure with no message is not an improvement.

Rules

  • Prove the fix with a test that fails without it: make tab.close fail with a non-not_found code and assert that releaseZdotdir still runs and that release() still removes the worktree. FakeHerdr gained per-pane pane.close failure injection in #290 — you may need the equivalent for tab.close; add it in the same style.
  • Do not widen isAlreadyGone. Treating an internal_error as "already gone" would hide a real failure. The fix is to continue despite the failure while still logging it, not to reclassify it.
  • Check whether drainAll and the spawn-readiness gate's self-reap path reach stop() too, and say what a throw does there. One line each is enough.

Shape check

This is the third instance of "a guard on one branch, bare on its sibling" in this file's neighbourhood (#274, #283, now this). When you are done, look for the same shape elsewhere in HerdrPeerLauncher and WorkspaceControl only — report what you find in one line each, and do not fix any of it.

Spotted by the #290 worker in its out-of-scope note. **I read the code myself and traced the consequences**, which are worse than the note said, so I am filing it rather than dropping it. Same shape as #283 and #274: cleanup wrapped on one branch, bare on its sibling. ## The bare call `HerdrPeerLauncher.stop()` (`:920-950`) runs three teardown steps. Two are guarded, the middle one is not: ```java try { agents.close(paneId); // guarded: tolerates *_not_found } catch (HerdrException e) { if (!isAlreadyGone(e)) throw e; log.debug(...); } if (loc != null && loc.tabPaneCount() == 1) { spaces.closeTab(loc.tabId()); // :937 — BARE } else if (loc != null) { ... } // CB-633: ... Last in, best-effort — a failure here must not mask a real teardown failure above. try { releaseZdotdir(paneId); // guarded } catch (RuntimeException e) { log.warn(...); } ``` `WorkspaceControl.closeTab` (`:122-129`) swallows `*_not_found` only. Every other herdr error propagates out of `stop()`. I checked the two neighbouring suspicions and **both are fine** — do not chase them: - `spaces.locatePane(paneId)` (`WorkspaceControl:143-154`) catches *every* `HerdrException` and returns `null`, so it never propagates. - `tabPaneCount` (`:157-167`) catches too, returning `-1`. So `closeTab` is the only bare call on this path. ## Why it matters — two leaks, not one **1. The ZDOTDIR leaks.** `releaseZdotdir` never runs. That directory holds the generated `memberCredentials` allow-list scrub files, and its `allowed N of M` report is never read back or logged — so the operator loses the one line that says how many variables the scrub actually blanked. Self-healing is slow but real: `EnvAllowListScrub.reapOrphans` removes directories older than 24h on the *next* `generate`, and `deleteOnExit` catches the rest at daemon restart. **2. The worktree leaks, and this one does not self-heal.** The exception propagates into `SessionManager.release()` at `:335`, which is *before* the worktree removal at the end of that method. #283 wrapped that removal — but this throw happens earlier, so the removal is never reached at all. By then `registry.remove(paneId)` has already run, so the session is deregistered and **a second stop is a no-op**: there is no retry path, and the directory is invisible to `fleet_list`. That is exactly the harm #283 exists to prevent, arriving through a different door. The new #290 test already documents half of this without meaning to: ```java assertEquals(List.of(a.worktree(), c.worktree()), worktrees.removeCalls()..., "the middle session's worktree removal never runs — release() throws before reaching it"); ``` That assertion is correct about today's behaviour. This ticket is about whether today's behaviour is right. ## What to decide The comment above `releaseZdotdir` states the rule this file already believes: *"a failure here must not mask a real teardown failure above"*. A failing `closeTab` is **not** a real teardown failure above — the pane is already closed by then. It is cosmetic workspace tidying, and it is masking two real cleanups below it. So the obvious fix is to wrap `closeTab` in a try/catch with a WARN naming the tab, and carry on. But **decide it, do not just apply it**, and put the reasoning in your report: - Is there any caller that genuinely needs a `closeTab` failure to surface as a failed stop? Check `SessionManager.release`, `FleetMcp.stop` (which catches only `HerdrException`) and `FleetApp.stopMember` (which catches nothing). - If you wrap it, the WARN must name the tab id and say the tab may need manual cleanup — a silently swallowed failure with no message is not an improvement. ## Rules - Prove the fix with a test that fails without it: make `tab.close` fail with a **non**-`not_found` code and assert that `releaseZdotdir` still runs and that `release()` still removes the worktree. `FakeHerdr` gained per-pane `pane.close` failure injection in #290 — you may need the equivalent for `tab.close`; add it in the same style. - **Do not widen `isAlreadyGone`.** Treating an `internal_error` as "already gone" would hide a real failure. The fix is to continue *despite* the failure while still logging it, not to reclassify it. - Check whether `drainAll` and the spawn-readiness gate's self-reap path reach `stop()` too, and say what a throw does there. One line each is enough. ## Shape check This is the third instance of "a guard on one branch, bare on its sibling" in this file's neighbourhood (#274, #283, now this). When you are done, look for the same shape **elsewhere in `HerdrPeerLauncher` and `WorkspaceControl` only** — report what you find in one line each, and **do not fix any of it**.
ltms commented 2026-09-04 06:48:11 +02:00
Author
Owner
Copy Link
Copy Source

Merged as 086c598, extended in ba51e0c. Real merge built green at 1303 tests.

The worker read every caller I named before deciding, rather than applying the obvious fix and moving on. Its findings match mine, and it added one I had not traced: the spawn-readiness gate (waitUntilInjectableOrThrow / failFastOnGoneBackend) calls stop(paneId) bare before throwing its own PeerUnreachableException, so a closeTab failure there threw the wrong exception and masked the real cause of the spawn failure. That is a fourth consequence the ticket did not list.

My mutation — the over-application check

The worker reverted the try/catch to bare and quoted both tests failing. That proves the fix does something. The real risk in a change like this is the opposite: applying it too widely, so a genuine teardown failure gets swallowed too. So I mutated the step above — widened agents.close's guard to swallow every failure rather than only *_not_found:

SessionManagerTest.reapIdleSurvivesOneSessionWhoseLauncherStopFails:1121
  the WARN names the failed session's terminal: no reap-failed WARN logged for the failing
  session ==> expected: <true> but was: <false>

Caught. The two steps are still treated differently, and #290's test — which injects a pane.close failure, not a tab.close one — is what pins the difference. That asymmetry is deliberate and is now load-bearing in both directions: the pane close is the one step whose failure means the teardown may genuinely not have happened, so it must still propagate. The tab close is not.

isAlreadyGone was not widened, as the ticket required.

My extension — the same asymmetry, one level down

The new guard caught HerdrException. Its sibling five lines below (releaseZdotdir) catches RuntimeException.

I checked whether that gap can actually fire today, rather than assuming: HerdrCodec wraps every encode/decode failure in a HerdrException (:36-37, :50-51, :57, :61), and UnixSocketHerdrClient.call wraps every IOException (:75-77). So HerdrException really is all closeTab can throw right now. The narrow catch was not a live bug, and I want that stated plainly rather than dressed up.

I widened it to RuntimeException anyway. The whole claim of this fix is "nothing here may mask the cleanups below it". A step guarded against the expected exception type and bare against every other one is the same shape this ticket exists to remove, one level down — and a later change inside WorkspaceControl.closeTab (a null check, a validation) would reopen it silently. The cost is zero and the comment records that it changes no behaviour today.

Shape check — accepted, and I spot-checked it

The worker read HerdrPeerLauncher (1837 lines) and WorkspaceControl (174 lines) end to end and reported no further instance. I verified its three "already guarded" claims and they hold: spawnInTab's orphan-tab cleanup, tidy(), and reapOrphanWorkers' per-orphan try/catch are all wrapped.

Its reasoning on WorkspaceControl's other bare calls is right and worth keeping: ensureWorkspace, createTab, splitPane, renameTab, listWorkspaces and listTabs are all bare, but none sits beside a guarded sibling in a teardown sequence. They are spawn-side single calls. Bare is not the defect — bare next to guarded, in a cleanup chain, is. A sweep that flagged every bare call would have returned six false positives here.

One note on process

The worker mentioned starting a background fork for the shape check and then completing the read itself, saying it did not rely on the fork's result. I checked fleet_list: no stray members, so nothing leaked into the fleet. Reporting it rather than quietly dropping it was the right call.

Wiki entry added as a third paragraph under A member's teardown no longer leaks a worktree or a branch — same leak, third door.

Merged as `086c598`, extended in `ba51e0c`. Real merge built green at **1303 tests**. The worker read every caller I named before deciding, rather than applying the obvious fix and moving on. Its findings match mine, and it added one I had not traced: the spawn-readiness gate (`waitUntilInjectableOrThrow` / `failFastOnGoneBackend`) calls `stop(paneId)` bare before throwing its own `PeerUnreachableException`, so a `closeTab` failure there threw the **wrong exception** and masked the real cause of the spawn failure. That is a fourth consequence the ticket did not list. ## My mutation — the over-application check The worker reverted the try/catch to bare and quoted both tests failing. That proves the fix does something. The real risk in a change like this is the opposite: applying it too widely, so a genuine teardown failure gets swallowed too. So I mutated the step **above** — widened `agents.close`'s guard to swallow every failure rather than only `*_not_found`: ``` SessionManagerTest.reapIdleSurvivesOneSessionWhoseLauncherStopFails:1121 the WARN names the failed session's terminal: no reap-failed WARN logged for the failing session ==> expected: <true> but was: <false> ``` Caught. The two steps are still treated differently, and #290's test — which injects a `pane.close` failure, not a `tab.close` one — is what pins the difference. That asymmetry is deliberate and is now load-bearing in both directions: the pane close is the one step whose failure means the teardown may genuinely not have happened, so it must still propagate. The tab close is not. `isAlreadyGone` was not widened, as the ticket required. ## My extension — the same asymmetry, one level down The new guard caught `HerdrException`. Its sibling five lines below (`releaseZdotdir`) catches `RuntimeException`. I checked whether that gap can actually fire today, rather than assuming: `HerdrCodec` wraps every encode/decode failure in a `HerdrException` (`:36-37`, `:50-51`, `:57`, `:61`), and `UnixSocketHerdrClient.call` wraps every `IOException` (`:75-77`). So `HerdrException` really is all `closeTab` can throw right now. **The narrow catch was not a live bug, and I want that stated plainly rather than dressed up.** I widened it to `RuntimeException` anyway. The whole claim of this fix is "nothing here may mask the cleanups below it". A step guarded against the expected exception type and bare against every other one is the same shape this ticket exists to remove, one level down — and a later change inside `WorkspaceControl.closeTab` (a null check, a validation) would reopen it silently. The cost is zero and the comment records that it changes no behaviour today. ## Shape check — accepted, and I spot-checked it The worker read `HerdrPeerLauncher` (1837 lines) and `WorkspaceControl` (174 lines) end to end and reported no further instance. I verified its three "already guarded" claims and they hold: `spawnInTab`'s orphan-tab cleanup, `tidy()`, and `reapOrphanWorkers`' per-orphan try/catch are all wrapped. Its reasoning on `WorkspaceControl`'s other bare calls is right and worth keeping: `ensureWorkspace`, `createTab`, `splitPane`, `renameTab`, `listWorkspaces` and `listTabs` are all bare, but none sits beside a guarded sibling in a teardown sequence. They are spawn-side single calls. Bare is not the defect — **bare next to guarded, in a cleanup chain, is.** A sweep that flagged every bare call would have returned six false positives here. ## One note on process The worker mentioned starting a background fork for the shape check and then completing the read itself, saying it did not rely on the fork's result. I checked `fleet_list`: no stray members, so nothing leaked into the fleet. Reporting it rather than quietly dropping it was the right call. Wiki entry added as a third paragraph under *A member's teardown no longer leaks a worktree or a branch* — same leak, third door.
ltms closed this issue 2026-09-04 06:50:45 +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#293
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?