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

CompositePeerLauncher's single-daemon stop() shortcut can skip tab teardown, orphaning a worker tab #342

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

Found by a hunt over member/ and proven by the hunter with a throwaway test. Lower priority than
#341 — it needs a specific fleet shape — but it is real and it leaks operator workspace.

What happens

CompositePeerLauncher.stop(id) looks the owning adapter up in spawnedBy. That map is in-memory
only, so it is empty after any daemon restart. On a miss, when herdrDaemonCount() == 1, it takes a
shortcut (CompositePeerLauncher.java:544-546) and uses delegates.getFirst() instead of probing
for the pane's real owner.

Teardown then runs through the wrong adapter's HerdrPeerLauncher. That instance's tab cleanup
is gated on usesTabPlacement() (HerdrPeerLauncher.java:989-991), which reads its own
configured profiles — not the profile that actually spawned the pane. If the wrong adapter has no
tab-placement profiles, usesTabPlacement() returns false and the whole
spaces.locatePane / spaces.closeTab block (:947, :954-977) is skipped.

Direction of harm

The member does stop: agents.close(paneId) is adapter-agnostic and reaches the same herdr
connection either way. What leaks is the now-empty tab that a tab-placement adapter created.
Nothing reaps it — EnvAllowListScrub.reapOrphans only reaps generated ZDOTDIR temp directories,
and there is no orphan-tab sweep anywhere in this package or its callers.

So: workspace clutter that accumulates silently in the operator's herdr session, one tab per
affected teardown, forever. Not data loss, not a wrong answer to a lead — which is why this is
filed below #341.

Trigger — narrow, and worth stating plainly

All four must hold:

  1. two or more adapters sharing one herdr daemon,
  2. their profiles disagreeing on tab vs pane placement,
  3. spawnedBy empty for a still-live pane — a daemon restart is the documented case (CB-185),
  4. a stop() on that surviving pane by its raw id.

Confirmed

The hunter built a CompositePeerLauncher over one fake herdr with a ClaudeCodeLauncher
(placement pane) first and an OpenCodeLauncher (placement tab) second, spawned through the
opencode profile so a dedicated tab was created, then called stop with the raw pane id. Result:
pane.close was called, tab.close was not. Its assertion failed as expected. The test was
deleted afterwards — I checked the worktree and it is clean.

I have re-read CompositePeerLauncher.java:541-567 and HerdrPeerLauncher.java:938-991 and the
routing and the gate are as described. I have not reproduced it against a live herdr.

Goal and invariants

Goal: teardown of a pane closes whatever that pane actually occupies, whichever adapter's
config the teardown happens to route through.

Invariants:

  1. The single-daemon shortcut exists for a reason — a probe costs a round trip on every unowned
    stop. Do not simply delete it without saying what it costs to.
  2. Stopping the pane itself must keep working on every path. That half is correct today.
  3. Teardown must stay tolerant of a pane that is already gone. Do not turn a missing tab into a
    thrown exception on a teardown path — see #335 on what an uncaught throw in a teardown loop does
    to the tasks behind it.

Candidate mechanisms, as candidates only — pick one and justify it:

  1. Make the single-daemon fallback still run probeOwner, so the routing is right rather than
    assumed.
  2. Make the tab-cleanup decision depend on the pane's actual placement — call
    spaces.locatePane unconditionally and tolerate "not found" — rather than on the delegate's
    static config.

Option 2 looks more robust to me because it stops depending on routing being correct at all, but it
adds a lookup to every stop. I have not measured that cost. Decide it yourself.

Why the tests miss it

Every existing CompositePeerLauncher stop-fallback test
(stopAllowsAnUnownedPaneIdWithTwoAdaptersSharingOneDaemon,
stopWithEmptySpawnedByResolvesTheOwnerThroughAProbeAndSkipsTheOtherDaemon, and the rest in
CompositePeerLauncherTest) configures both test adapters with placement: "tab". So
usesTabPlacement() returns true on either delegate and the mis-routing never shows. None of them
mixes placement across adapters.

The fix needs a test that does mix them — that is the whole bug.

Found by a hunt over `member/` and proven by the hunter with a throwaway test. Lower priority than #341 — it needs a specific fleet shape — but it is real and it leaks operator workspace. ## What happens `CompositePeerLauncher.stop(id)` looks the owning adapter up in `spawnedBy`. That map is in-memory only, so it is empty after any daemon restart. On a miss, when `herdrDaemonCount() == 1`, it takes a shortcut (`CompositePeerLauncher.java:544-546`) and uses `delegates.getFirst()` instead of probing for the pane's real owner. Teardown then runs through the **wrong adapter's** `HerdrPeerLauncher`. That instance's tab cleanup is gated on `usesTabPlacement()` (`HerdrPeerLauncher.java:989-991`), which reads *its own* configured profiles — not the profile that actually spawned the pane. If the wrong adapter has no tab-placement profiles, `usesTabPlacement()` returns `false` and the whole `spaces.locatePane` / `spaces.closeTab` block (`:947`, `:954-977`) is skipped. ## Direction of harm The member does stop: `agents.close(paneId)` is adapter-agnostic and reaches the same herdr connection either way. What leaks is the now-empty **tab** that a tab-placement adapter created. Nothing reaps it — `EnvAllowListScrub.reapOrphans` only reaps generated ZDOTDIR temp directories, and there is no orphan-tab sweep anywhere in this package or its callers. So: workspace clutter that accumulates silently in the operator's herdr session, one tab per affected teardown, forever. Not data loss, not a wrong answer to a lead — which is why this is filed below #341. ## Trigger — narrow, and worth stating plainly All four must hold: 1. two or more adapters sharing **one** herdr daemon, 2. their profiles disagreeing on `tab` vs `pane` placement, 3. `spawnedBy` empty for a still-live pane — a daemon restart is the documented case (CB-185), 4. a `stop()` on that surviving pane by its raw id. ## Confirmed The hunter built a `CompositePeerLauncher` over one fake herdr with a `ClaudeCodeLauncher` (placement `pane`) first and an `OpenCodeLauncher` (placement `tab`) second, spawned through the opencode profile so a dedicated tab was created, then called `stop` with the raw pane id. Result: `pane.close` was called, `tab.close` was **not**. Its assertion failed as expected. The test was deleted afterwards — I checked the worktree and it is clean. I have re-read `CompositePeerLauncher.java:541-567` and `HerdrPeerLauncher.java:938-991` and the routing and the gate are as described. I have **not** reproduced it against a live herdr. ## Goal and invariants **Goal:** teardown of a pane closes whatever that pane actually occupies, whichever adapter's config the teardown happens to route through. **Invariants:** 1. The single-daemon shortcut exists for a reason — a probe costs a round trip on every unowned stop. Do not simply delete it without saying what it costs to. 2. Stopping the pane itself must keep working on every path. That half is correct today. 3. Teardown must stay tolerant of a pane that is already gone. Do not turn a missing tab into a thrown exception on a teardown path — see #335 on what an uncaught throw in a teardown loop does to the tasks behind it. **Candidate mechanisms, as candidates only — pick one and justify it:** 1. Make the single-daemon fallback still run `probeOwner`, so the routing is right rather than assumed. 2. Make the tab-cleanup decision depend on the **pane's actual placement** — call `spaces.locatePane` unconditionally and tolerate "not found" — rather than on the delegate's static config. Option 2 looks more robust to me because it stops depending on routing being correct at all, but it adds a lookup to every stop. I have not measured that cost. Decide it yourself. ## Why the tests miss it Every existing `CompositePeerLauncher` stop-fallback test (`stopAllowsAnUnownedPaneIdWithTwoAdaptersSharingOneDaemon`, `stopWithEmptySpawnedByResolvesTheOwnerThroughAProbeAndSkipsTheOtherDaemon`, and the rest in `CompositePeerLauncherTest`) configures **both** test adapters with `placement: "tab"`. So `usesTabPlacement()` returns `true` on either delegate and the mis-routing never shows. None of them mixes placement across adapters. The fix needs a test that does mix them — that is the whole bug.
ltms commented 2026-09-04 11:41:00 +02:00
Author
Owner
Copy Link
Copy Source

Merged as 73aab3f (--no-ff). The branch was behind main, so the merge was a real one.
main is green at Tests run: 1365, Failures: 0, Errors: 0, Skipped: 0.

Mechanism: option 2, and the worker gave a reason to reject option 1 that I did not have when I
wrote this ticket. I checked it myself in the code rather than taking the report:

// CompositePeerLauncher.probeOwner
Map<HerdrClient, HerdrPeerLauncher> byDaemon = new IdentityHashMap<>();
for (HerdrPeerLauncher delegate : delegates) {
    byDaemon.putIfAbsent(delegate.herdr(), delegate);
}

byDaemon is keyed by the HerdrClient identity. Two delegates sharing one herdr daemon — which is
trigger condition 1 of this very bug — collapse to a single entry holding whichever delegate was
inserted first. So probeOwner would have returned delegates.getFirst(), exactly what the
shortcut already does. Option 1 would have changed nothing in the shape this ticket is about.
My ticket was wrong to offer it as an equal candidate.

What changed. HerdrPeerLauncher.stop now calls spaces.locatePane(paneId) unconditionally,
instead of usesTabPlacement() ? spaces.locatePane(paneId) : null. usesTabPlacement() is deleted;
that line was its only caller. This puts the code back in line with what the method's own javadoc
has claimed all along: "The single-pane check is what makes this safe regardless of how the peer
was placed." The placement gate was a second, weaker guard that only ever removed correct
behaviour.

Invariant 3 holds unchanged — locatePane returns null for a pane that is gone, it does not
throw.

Cost, stated and not measured: every stop now does one extra pane.get (plus tab.list when
the pane still exists), including in a pure pane-placement fleet that used to skip it. One extra
herdr call on a teardown path. Nobody has timed it.

My own mutation on merge — Mutation CC, aimed at the half the worker did not touch. The worker
proved its fix by reverting the fix. That leaves the more interesting question open: the
single-occupant check is now the only protection for a user's shared tab, and the worker also
rewrote the test that used to cover this path. So I weakened that check instead —
loc.tabPaneCount() == 1 → >= 1. Full suite, unpiped:

FleetAppTest.stopNeverClosesATabThatHoldsOtherPanes:739
  must not close a tab that holds the user's other panes ==> expected: <false> but was: <true>
FleetAppTest.stopWorkerInPanePlacementClosesOnlyThePane:728
  pane placement's shared tab must never be closed ==> expected: <false> but was: <true>
Tests run: 1365, Failures: 2, Errors: 0, Skipped: 0
BUILD FAILURE

Two tests, one of them the reworked one. So the protection that now carries the whole weight is
pinned, and the test edit did not quietly remove it. Restored; git diff --stat on src/ empty.

On the collateral test edit. The worker flagged it rather than widening scope silently, which is
the right call. stopWorkerInPanePlacementClosesOnlyThePane used to assert pane.get was never
called — an assertion on the mechanism this fix removes, so it could not survive. It now models pane
placement as a split into a tab that already holds another pane, and asserts tab.close never
fires. That is a stronger test than the one it replaced: it checks the outcome the operator cares
about instead of the call that used to produce it.

One case nobody covers, and I am not asking for it. A pane-placement peer that is the sole
occupant of its tab will now have that tab closed. spawnAsPane uses spaces.splitPane, which
splits an existing tab, so the count is normally at least 2 — it takes the operator closing their
own pane first to reach it, and the tab is empty after ours goes anyway. Noting it because the
javadoc's phrase "a tab we created" is now slightly generous.

Shape reported, not fixed (as briefed): CompositePeerLauncher.clearContext(id) has the same
root cause — an in-memory spawnedBy — with a different failure mode. On a cache miss it returns
false and logs at debug, with no fallback at all, so after a restart it silently stops resetting
context for any surviving peer. I read :637-641 and it is as described. Filing separately.

Closing.

Merged as `73aab3f` (`--no-ff`). The branch was **behind main**, so the merge was a real one. `main` is green at `Tests run: 1365, Failures: 0, Errors: 0, Skipped: 0`. **Mechanism: option 2**, and the worker gave a reason to reject option 1 that I did not have when I wrote this ticket. I checked it myself in the code rather than taking the report: ```java // CompositePeerLauncher.probeOwner Map<HerdrClient, HerdrPeerLauncher> byDaemon = new IdentityHashMap<>(); for (HerdrPeerLauncher delegate : delegates) { byDaemon.putIfAbsent(delegate.herdr(), delegate); } ``` `byDaemon` is keyed by the `HerdrClient` identity. Two delegates sharing one herdr daemon — which is trigger condition 1 of this very bug — collapse to a single entry holding whichever delegate was inserted first. So `probeOwner` would have returned `delegates.getFirst()`, exactly what the shortcut already does. **Option 1 would have changed nothing in the shape this ticket is about.** My ticket was wrong to offer it as an equal candidate. **What changed.** `HerdrPeerLauncher.stop` now calls `spaces.locatePane(paneId)` unconditionally, instead of `usesTabPlacement() ? spaces.locatePane(paneId) : null`. `usesTabPlacement()` is deleted; that line was its only caller. This puts the code back in line with what the method's own javadoc has claimed all along: *"The single-pane check is what makes this safe regardless of how the peer was placed."* The placement gate was a second, weaker guard that only ever removed correct behaviour. Invariant 3 holds unchanged — `locatePane` returns `null` for a pane that is gone, it does not throw. **Cost, stated and not measured:** every `stop` now does one extra `pane.get` (plus `tab.list` when the pane still exists), including in a pure pane-placement fleet that used to skip it. One extra herdr call on a teardown path. Nobody has timed it. **My own mutation on merge — Mutation CC, aimed at the half the worker did not touch.** The worker proved its fix by reverting the fix. That leaves the more interesting question open: the single-occupant check is now the *only* protection for a user's shared tab, and the worker also rewrote the test that used to cover this path. So I weakened that check instead — `loc.tabPaneCount() == 1` → `>= 1`. Full suite, unpiped: ``` FleetAppTest.stopNeverClosesATabThatHoldsOtherPanes:739 must not close a tab that holds the user's other panes ==> expected: <false> but was: <true> FleetAppTest.stopWorkerInPanePlacementClosesOnlyThePane:728 pane placement's shared tab must never be closed ==> expected: <false> but was: <true> Tests run: 1365, Failures: 2, Errors: 0, Skipped: 0 BUILD FAILURE ``` Two tests, one of them the reworked one. So the protection that now carries the whole weight is pinned, and the test edit did not quietly remove it. Restored; `git diff --stat` on `src/` empty. **On the collateral test edit.** The worker flagged it rather than widening scope silently, which is the right call. `stopWorkerInPanePlacementClosesOnlyThePane` used to assert `pane.get` was never called — an assertion on the mechanism this fix removes, so it could not survive. It now models pane placement as a split into a tab that already holds another pane, and asserts `tab.close` never fires. That is a stronger test than the one it replaced: it checks the outcome the operator cares about instead of the call that used to produce it. **One case nobody covers, and I am not asking for it.** A pane-placement peer that *is* the sole occupant of its tab will now have that tab closed. `spawnAsPane` uses `spaces.splitPane`, which splits an existing tab, so the count is normally at least 2 — it takes the operator closing their own pane first to reach it, and the tab is empty after ours goes anyway. Noting it because the javadoc's phrase "a tab we created" is now slightly generous. **Shape reported, not fixed** (as briefed): `CompositePeerLauncher.clearContext(id)` has the same root cause — an in-memory `spawnedBy` — with a different failure mode. On a cache miss it returns `false` and logs at debug, with no fallback at all, so after a restart it silently stops resetting context for any surviving peer. I read `:637-641` and it is as described. Filing separately. Closing.
ltms closed this issue 2026-09-04 11:41:07 +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#342
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?