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

Two spawn-side exits leak a live pane, and the caller cannot clean up because it never learns the pane id #296

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

Found by a read-only hunt over the spawn path. I read both sites myself and confirmed every claim below, including the two that make these worth fixing rather than filing and forgetting.

Same shape as #274, #283 and #293: a cleanup applied on one exit and missing on its sibling. The new part is why nobody downstream saves you — see "The caller cannot compensate".

Exit A — HerdrPeerLauncher.java:1007 — the readiness gate lets a non-*_not_found error escape without teardown

try {
    sample = agents.get(paneId);
} catch (HerdrException e) {
    if (isAlreadyGone(e)) {
        failFastOnGoneBackend(paneId, e, nowMillis.getAsLong() - start);   // calls stop(paneId)
    }
    throw e; // any other herdr failure is not ours to interpret — let it propagate
}

By this point the pane is running and the backend has started. The *_not_found branch tears down. The sibling branch does not.

This is a one-way gate, and the file says so itself. The javadoc directly above (:995-998) describes the harm:

...teardown (stop) entirely, leaking the pane/tab. failFastOnGoneBackend closes that gap: it stops waiting immediately (never burns the rest of the timeout), runs the same teardown the timeout path below runs...

#176 found this exact leak and closed it for the one direction the incident came from. The other direction was left open. The timeout path at :1029 and the gone-backend path at :1050 both call stop(paneId); only this branch does not.

Reachable path: the backend starts fine, then herdr answers agents.get with internal_error, or the unix socket drops, or the codec fails. Any non-*_not_found HerdrException.

Lifetime: the pane has a started named agent, so reapOrphanWorkers (:845-870) does reclaim it — but only at the next daemon startup, and only because a fresh process has a fresh nonce (isForeignWorker skips the current one). Within the running daemon it is never reaped.

Exit B — HerdrPeerLauncher.java:700 — spawnAsPane never closes the pane it just split

String paneId = spaces.splitPane(cwd, workerEnv);
if (paneId == null) { throw new IllegalStateException(...); }
Agent peer = startUniquelyNamed(cfg, argv, paneId).agent();   // can throw — pane is never closed

Its sibling spawnInTab (:648-658) wraps the same startUniquelyNamed call in a try/catch that closes the tab it created, with the comment "The peer never started — don't leave the tab we just created orphaned." Same call, same failure, one path cleans up and the other does not.

Reachable path: startUniquelyNamed (:735-753) rethrows immediately on any code that is not agent_name_taken, and throws last after NAME_RETRIES exhausted retries.

Lifetime: forever. This pane has no started named agent, so reapOrphanWorkers cannot see it — it iterates agents.list(), and this pane is in no such list. No restart reclaims it. It is a bare shell pane in the operator's shared tab that nothing will ever remove.

The caller cannot compensate — this is the part that settles it

SessionManager.acquire's catch (:516-541) cleans up the worktree and the branch, and nothing else:

handle = launcher.spawn(new SpawnRequest(...));
} catch (RuntimeException e) {
    ... worktrees.remove(repoRoot, path);  worktrees.deleteBranch(repoRoot, branch);

handle is never assigned when spawn throws, so the caller never learns the pane id. It cannot call launcher.stop(paneId) even if it wanted to. The pane id exists only inside the launcher's own stack frame.

That is what makes exit A's stated rationale wrong (see below), and it makes the consequence worse than a leak: the caller removes the worktree while the pane is still running with that worktree as its cwd. The result is a live backend process, with a deleted working directory, invisible to fleet_list, holding a real backend seat that costs money and that nothing decrements.

What to decide, and the one thing you must argue rather than assume

Exit A's behaviour is pinned by a test with a stated rationale, ClaudeCodeLauncherTest.spawnLetsAnUnrelatedHerdrErrorPropagateUnchanged (:1165-1187):

assertEquals(0, paneCloseCount(herdr, "w9:pRoot_1"),
        "an error this gate does not recognize is not this gate's teardown to run");

Do not just flip that assertion. Someone decided this on purpose, and you have to answer their reasoning before you change it. My reading, which you should check rather than accept:

  • "Not this gate's teardown to run" would be correct if some other layer could run it. No layer can — the pane id dies with the stack frame. A rule that hands responsibility to nobody is not a separation of concerns; it is a leak with a comment on it.
  • The distinction worth preserving is between the exception and the cleanup. The gate genuinely should not interpret or convert an error it does not recognise — that part of the rationale is right and must survive. It should still close the pane it opened. Propagate e unchanged, and stop the pane on the way out.

So: keep the exception exactly as it is, add the teardown. If you conclude I am wrong, say so with your reasoning and change nothing — that is a valid outcome and I will read the argument.

Exit B needs no such argument. Its sibling five hundred lines up already does the right thing for the identical failure.

Rules

  • Both cleanups are best-effort and must never mask the original exception. Copy the shape spawnInTab already uses at :651-657: inner try/catch, log a WARN naming the resource, rethrow the original. #293 has just been through this exact argument on the teardown side — read stop() at :930-950 for the house style, and catch RuntimeException, not HerdrException, for the same reason recorded there.
  • Prove each fix with a test that fails without it. FakeHerdr already has the injection points: agentGetFailsWithAfter(n, code) for exit A, and it can fail agent.start. For exit B assert paneCloseCount for the split pane.
  • Update spawnLetsAnUnrelatedHerdrErrorPropagateUnchanged rather than deleting it. Its real subject — the exception propagates unchanged, with its original code — must still be asserted. Only the pane-close expectation changes, and its message must say why.
  • Do not widen isAlreadyGone. Exit A must still not treat an internal_error as "already gone".
  • Do not touch stop(), the timeout path, or failFastOnGoneBackend. They are correct.

Shape check

When you are done, look for the same shape — a resource created, then an exit that does not release it — in HerdrPeerLauncher only, and only on the spawn side (stop and teardown were just fixed under #293, leave them alone). Report anything you find in one line each and do not fix it. Bare is not automatically the defect: bare next to a guarded sibling handling the same failure is.

Found by a read-only hunt over the spawn path. **I read both sites myself and confirmed every claim below**, including the two that make these worth fixing rather than filing and forgetting. Same shape as #274, #283 and #293: a cleanup applied on one exit and missing on its sibling. The new part is *why nobody downstream saves you* — see "The caller cannot compensate". ## Exit A — `HerdrPeerLauncher.java:1007` — the readiness gate lets a non-`*_not_found` error escape without teardown ```java try { sample = agents.get(paneId); } catch (HerdrException e) { if (isAlreadyGone(e)) { failFastOnGoneBackend(paneId, e, nowMillis.getAsLong() - start); // calls stop(paneId) } throw e; // any other herdr failure is not ours to interpret — let it propagate } ``` By this point the pane is running and the backend has started. The `*_not_found` branch tears down. The sibling branch does not. **This is a one-way gate, and the file says so itself.** The javadoc directly above (`:995-998`) describes the harm: > ...teardown (`stop`) entirely, leaking the pane/tab. `failFastOnGoneBackend` closes that gap: it stops waiting immediately (never burns the rest of the timeout), runs the same teardown the timeout path below runs... #176 found this exact leak and closed it for the one direction the incident came from. The other direction was left open. The timeout path at `:1029` and the gone-backend path at `:1050` both call `stop(paneId)`; only this branch does not. **Reachable path:** the backend starts fine, then herdr answers `agents.get` with `internal_error`, or the unix socket drops, or the codec fails. Any non-`*_not_found` `HerdrException`. **Lifetime:** the pane has a started named agent, so `reapOrphanWorkers` (`:845-870`) does reclaim it — but only at the **next daemon startup**, and only because a fresh process has a fresh nonce (`isForeignWorker` skips the current one). Within the running daemon it is never reaped. ## Exit B — `HerdrPeerLauncher.java:700` — `spawnAsPane` never closes the pane it just split ```java String paneId = spaces.splitPane(cwd, workerEnv); if (paneId == null) { throw new IllegalStateException(...); } Agent peer = startUniquelyNamed(cfg, argv, paneId).agent(); // can throw — pane is never closed ``` Its sibling `spawnInTab` (`:648-658`) wraps the **same** `startUniquelyNamed` call in a try/catch that closes the tab it created, with the comment *"The peer never started — don't leave the tab we just created orphaned."* Same call, same failure, one path cleans up and the other does not. **Reachable path:** `startUniquelyNamed` (`:735-753`) rethrows immediately on any code that is not `agent_name_taken`, and throws `last` after `NAME_RETRIES` exhausted retries. **Lifetime: forever.** This pane has **no started named agent**, so `reapOrphanWorkers` cannot see it — it iterates `agents.list()`, and this pane is in no such list. No restart reclaims it. It is a bare shell pane in the operator's shared tab that nothing will ever remove. ## The caller cannot compensate — this is the part that settles it `SessionManager.acquire`'s catch (`:516-541`) cleans up the worktree and the branch, and nothing else: ```java handle = launcher.spawn(new SpawnRequest(...)); } catch (RuntimeException e) { ... worktrees.remove(repoRoot, path); worktrees.deleteBranch(repoRoot, branch); ``` `handle` is never assigned when `spawn` throws, so **the caller never learns the pane id**. It cannot call `launcher.stop(paneId)` even if it wanted to. The pane id exists only inside the launcher's own stack frame. That is what makes exit A's stated rationale wrong (see below), and it makes the consequence worse than a leak: the caller **removes the worktree** while the pane is still running with that worktree as its cwd. The result is a live backend process, with a deleted working directory, invisible to `fleet_list`, holding a real backend seat that costs money and that nothing decrements. ## What to decide, and the one thing you must argue rather than assume Exit A's behaviour is **pinned by a test with a stated rationale**, `ClaudeCodeLauncherTest.spawnLetsAnUnrelatedHerdrErrorPropagateUnchanged` (`:1165-1187`): ```java assertEquals(0, paneCloseCount(herdr, "w9:pRoot_1"), "an error this gate does not recognize is not this gate's teardown to run"); ``` **Do not just flip that assertion.** Someone decided this on purpose, and you have to answer their reasoning before you change it. My reading, which you should check rather than accept: - "Not this gate's teardown to run" would be correct if some other layer could run it. No layer can — the pane id dies with the stack frame. A rule that hands responsibility to nobody is not a separation of concerns; it is a leak with a comment on it. - The distinction worth preserving is between **the exception** and **the cleanup**. The gate genuinely should not interpret or convert an error it does not recognise — that part of the rationale is right and must survive. It should still close the pane it opened. Propagate `e` unchanged, and stop the pane on the way out. So: keep the exception exactly as it is, add the teardown. If you conclude I am wrong, say so with your reasoning and change nothing — that is a valid outcome and I will read the argument. Exit B needs no such argument. Its sibling five hundred lines up already does the right thing for the identical failure. ## Rules - **Both cleanups are best-effort and must never mask the original exception.** Copy the shape `spawnInTab` already uses at `:651-657`: inner try/catch, log a WARN naming the resource, rethrow the original. #293 has just been through this exact argument on the teardown side — read `stop()` at `:930-950` for the house style, and catch `RuntimeException`, not `HerdrException`, for the same reason recorded there. - **Prove each fix with a test that fails without it.** `FakeHerdr` already has the injection points: `agentGetFailsWithAfter(n, code)` for exit A, and it can fail `agent.start`. For exit B assert `paneCloseCount` for the split pane. - **Update `spawnLetsAnUnrelatedHerdrErrorPropagateUnchanged` rather than deleting it.** Its real subject — the exception propagates unchanged, with its original code — must still be asserted. Only the pane-close expectation changes, and its message must say why. - Do not widen `isAlreadyGone`. Exit A must still not treat an `internal_error` as "already gone". - Do not touch `stop()`, the timeout path, or `failFastOnGoneBackend`. They are correct. ## Shape check When you are done, look for the same shape — **a resource created, then an exit that does not release it** — in `HerdrPeerLauncher` **only**, and only on the spawn side (`stop` and teardown were just fixed under #293, leave them alone). Report anything you find in one line each and **do not fix it**. Bare is not automatically the defect: bare next to a guarded sibling handling the same failure is.
ltms commented 2026-09-04 07:10:09 +02:00
Author
Owner
Copy Link
Copy Source

Merged as aef14ff. Real merge built green at 1304 tests.

Both exits now close the pane they opened, best-effort, catching RuntimeException, logging a WARN that names the pane, and rethrowing the original exception untouched. The javadoc on the readiness gate was updated to record the distinction that matters:

Any other HerdrException still propagates unchanged — this gate does not interpret or recover from it, but it still closes the pane it opened before handing the exception to its caller.

That is the right split. The worker argued the point rather than flipping the assertion, which is what the ticket asked for.

The choice the ticket did not prescribe, and why it is correct

Exit B calls stop(paneId) on a pane whose agent never started. I checked whether that actually closes anything, since it is not obvious:

  • AgentControl.close sends pane.close, not an agent operation — "there is no agent.stop — close its pane" (AgentControl.java:160-162). So it does close a bare split pane.
  • stop() only touches a tab when usesTabPlacement() and loc.tabPaneCount() == 1. A split pane lives in the operator's shared tab, where the count is greater than one, so the operator's tab is never closed. The new test uses "pane" placement, so usesTabPlacement() is false and the tab path is not even reached.

Reusing stop rather than writing a second teardown was the better call: it inherits #293's guards instead of copying them.

My mutation — a different one from the worker's

The worker reverted each cleanup and quoted both failures. That proves the cleanups run. It does not test the risk specific to this change: the worker had to edit spawnLetsAnUnrelatedHerdrErrorPropagateUnchanged, the one test guarding "the exception propagates unchanged". A change that edits its own guard needs checking that the guard still guards.

So I mutated the propagation instead of the cleanup — Exit A now converts the error rather than rethrowing it:

ClaudeCodeLauncherTest.spawnLetsAnUnrelatedHerdrErrorPropagateUnchanged:1181
  Unexpected exception type thrown,
  expected: <dev.ltms.fleet.herdr.HerdrException>
  but was:  <dev.ltms.fleet.peer.PeerUnreachableException>

Caught. The test's original subject survived the edit intact; only the pane-close expectation changed, and its new message says why. Both halves are now load-bearing.

Also landed

e97502d corrects the drainAll javadoc, from a separate hunt over the reapers. The wording promised a per-session grace period ("for each session that is BUSY, poll up to timeoutNanos") when the deadline is taken once before the loop, so the first BUSY session can spend all of it.

That is deliberate and it is the safer design, so the code is unchanged: the drain is one phase of a shutdown sequence that must finish inside launchd's exit window, and a per-session grace would overrun it and get the daemon SIGKILLed part-way through — leaving every session not yet reached with no clean release, no preserved-worktree log and no snapshot. Cutting one turn short is the cheaper failure. The comment now says so, and points at logPreservedForShutdown, which already logs the abandoned session at WARN.

Shape check came back clean for the rest of the spawn side.

Merged as `aef14ff`. Real merge built green at **1304 tests**. Both exits now close the pane they opened, best-effort, catching `RuntimeException`, logging a WARN that names the pane, and rethrowing the original exception untouched. The javadoc on the readiness gate was updated to record the distinction that matters: > Any other `HerdrException` still propagates unchanged — this gate does not interpret or recover from it, **but it still closes the pane it opened** before handing the exception to its caller. That is the right split. The worker argued the point rather than flipping the assertion, which is what the ticket asked for. ## The choice the ticket did not prescribe, and why it is correct Exit B calls `stop(paneId)` on a pane whose agent **never started**. I checked whether that actually closes anything, since it is not obvious: - `AgentControl.close` sends `pane.close`, not an agent operation — *"there is no agent.stop — close its pane"* (`AgentControl.java:160-162`). So it does close a bare split pane. - `stop()` only touches a tab when `usesTabPlacement()` **and** `loc.tabPaneCount() == 1`. A split pane lives in the operator's shared tab, where the count is greater than one, so the operator's tab is never closed. The new test uses `"pane"` placement, so `usesTabPlacement()` is false and the tab path is not even reached. Reusing `stop` rather than writing a second teardown was the better call: it inherits #293's guards instead of copying them. ## My mutation — a different one from the worker's The worker reverted each cleanup and quoted both failures. That proves the cleanups run. It does not test the risk specific to *this* change: the worker had to edit `spawnLetsAnUnrelatedHerdrErrorPropagateUnchanged`, the one test guarding "the exception propagates unchanged". A change that edits its own guard needs checking that the guard still guards. So I mutated the propagation instead of the cleanup — Exit A now converts the error rather than rethrowing it: ``` ClaudeCodeLauncherTest.spawnLetsAnUnrelatedHerdrErrorPropagateUnchanged:1181 Unexpected exception type thrown, expected: <dev.ltms.fleet.herdr.HerdrException> but was: <dev.ltms.fleet.peer.PeerUnreachableException> ``` Caught. The test's original subject survived the edit intact; only the pane-close expectation changed, and its new message says why. Both halves are now load-bearing. ## Also landed `e97502d` corrects the `drainAll` javadoc, from a separate hunt over the reapers. The wording promised a per-session grace period (*"for each session that is BUSY, poll up to timeoutNanos"*) when the deadline is taken once before the loop, so the first BUSY session can spend all of it. That is deliberate and it is the safer design, so the code is unchanged: the drain is one phase of a shutdown sequence that must finish inside launchd's exit window, and a per-session grace would overrun it and get the daemon SIGKILLed part-way through — leaving every session not yet reached with no clean release, no preserved-worktree log and no snapshot. Cutting one turn short is the cheaper failure. The comment now says so, and points at `logPreservedForShutdown`, which already logs the abandoned session at WARN. Shape check came back clean for the rest of the spawn side.
ltms closed this issue 2026-09-04 07:10:16 +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#296
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?