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

fleetd #342: base tab-cleanup teardown on the pane's real placement #350

Closed
agent wants to merge 0 commits from worker/fd342-174a17-2 into main
pull from: worker/fd342-174a17-2
merge into: fleet:main
Conversation 0 Commits - Files Changed -
agent commented 2026-09-04 11:36:47 +02:00
Member
Copy Link
Copy Source

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

The bug

CompositePeerLauncher.stop(id) looks the owning adapter up in the in-memory spawnedBy map. That map is empty after any daemon restart. On a miss, when herdrDaemonCount() == 1, it takes a shortcut and uses delegates.getFirst() instead of probing for the pane's real owner. Teardown then runs through the WRONG adapter's HerdrPeerLauncher. Its tab cleanup was gated on usesTabPlacement(), which read THAT delegate's own configured profiles, not the profile that actually spawned the pane. If the wrong adapter had no tab-placement profiles, spaces.locatePane / spaces.closeTab was skipped entirely. The pane still stops (agents.close is adapter-agnostic), but the now-empty tab leaks with nothing to reap it.

Why Option 1 (probe even in the single-daemon fallback) does not fix it

I considered making the single-daemon shortcut still call probeOwner. But probeOwner groups by daemon identity (HerdrClient), not by adapter: when two delegates share one herdr daemon (the exact repro shape — a pane-placement ClaudeCodeLauncher and a tab-placement OpenCodeLauncher on one fake herdr), byDaemon collapses to a single entry mapped to whichever delegate was inserted first — i.e. delegates.getFirst() again. So probing would not have changed which delegate's config governed the tab-cleanup decision in the confirmed repro; it only helps when there are genuinely multiple daemons, which is not this bug.

The fix (Option 2)

HerdrPeerLauncher.stop() now resolves the pane's real tab unconditionally (spaces.locatePane(paneId)) instead of gating that call on usesTabPlacement() (a static per-delegate config read). WorkspaceControl#locatePane already tolerates a missing pane, returning null rather than throwing, so this stays tolerant of a pane/tab that is already gone (invariant 3). The existing single-occupant check (loc.tabPaneCount() == 1) is unchanged and is what actually protects a peer sitting in one of the user's shared tabs, regardless of how it was placed or which delegate's config the call happened to route through — the same design the code's own comments already described for the pane-placement case. Removed the now-dead usesTabPlacement() method.

Cost accepted: every stop() now does one extra pane.get (+ tab.list if the pane is still there) round trip, even for a purely pane-placement fleet that previously skipped it entirely. This is deliberate — the alternative (Option 1) does not fix the reported defect in the confirmed repro shape, as shown above.

Tests

  • New: CompositePeerLauncherTest.stopThroughTheSingleDaemonShortcutStillClosesTheTabWhenTheFallbackDelegateUsesPanePlacement — mixes placements across the two adapters (claude=pane, opencode=tab) sharing one daemon, calls stop() on an id spawnedBy never recorded (simulating a post-restart cache miss), and asserts tab.close fires. Every existing stop-fallback test in this class configured BOTH adapters as tab placement, so usesTabPlacement() was true either way and the mis-routing never showed.
  • Updated: FleetAppTest.stopWorkerInPanePlacementClosesOnlyThePane — its old assertion (assertFalse(herdr.called("pane.get"))) documented exactly the skip this fix removes. Rewrote it to model pane placement realistically (the peer's pane shares its tab with another occupant, withWorkerTabPaneCount(2)) and assert the tab is still never closed, now via the single-occupant check rather than via skipped resolution.

Mutation proof

Reverted the fix locally (restored the usesTabPlacement() gate) and ran the full suite: exactly 2 failures, both expected —

  • CompositePeerLauncherTest.stopThroughTheSingleDaemonShortcutStillClosesTheTabWhenTheFallbackDelegateUsesPanePlacement (tab.close never fired)
  • FleetAppTest.stopWorkerInPanePlacementClosesOnlyThePane (pane.get never fired)

Restored the fix afterward; full build is green again.

Build

mvn clean install, run unpiped: Tests run: 1364, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESS.

Shape note (not fixed, per ticket scope)

Swept CompositePeerLauncher.java and HerdrPeerLauncher.java for other places deciding teardown behaviour from a delegate's static config rather than the pane's real state. CompositePeerLauncher.clearContext(id) has a related but distinct issue: on a spawnedBy cache miss it just no-ops (return false) rather than falling back to a probe or a shortcut, so it silently stops resetting context for any peer whose owner record was lost to a restart — same root cause (in-memory spawnedBy), different failure mode (silent no-op, not mis-routed teardown). Not fixed here, out of scope.

Fixes fleetd #342 — CompositePeerLauncher's single-daemon stop() shortcut can skip tab teardown, orphaning a worker tab. ## The bug `CompositePeerLauncher.stop(id)` looks the owning adapter up in the in-memory `spawnedBy` map. That map is empty after any daemon restart. On a miss, when `herdrDaemonCount() == 1`, it takes a shortcut and uses `delegates.getFirst()` instead of probing for the pane's real owner. Teardown then runs through the WRONG adapter's `HerdrPeerLauncher`. Its tab cleanup was gated on `usesTabPlacement()`, which read THAT delegate's own configured profiles, not the profile that actually spawned the pane. If the wrong adapter had no tab-placement profiles, `spaces.locatePane` / `spaces.closeTab` was skipped entirely. The pane still stops (`agents.close` is adapter-agnostic), but the now-empty tab leaks with nothing to reap it. ## Why Option 1 (probe even in the single-daemon fallback) does not fix it I considered making the single-daemon shortcut still call `probeOwner`. But `probeOwner` groups by **daemon identity** (`HerdrClient`), not by adapter: when two delegates share one herdr daemon (the exact repro shape — a `pane`-placement `ClaudeCodeLauncher` and a `tab`-placement `OpenCodeLauncher` on one fake herdr), `byDaemon` collapses to a single entry mapped to whichever delegate was inserted first — i.e. `delegates.getFirst()` again. So probing would not have changed which delegate's config governed the tab-cleanup decision in the confirmed repro; it only helps when there are genuinely multiple *daemons*, which is not this bug. ## The fix (Option 2) `HerdrPeerLauncher.stop()` now resolves the pane's real tab **unconditionally** (`spaces.locatePane(paneId)`) instead of gating that call on `usesTabPlacement()` (a static per-delegate config read). `WorkspaceControl#locatePane` already tolerates a missing pane, returning `null` rather than throwing, so this stays tolerant of a pane/tab that is already gone (invariant 3). The existing single-occupant check (`loc.tabPaneCount() == 1`) is unchanged and is what actually protects a peer sitting in one of the user's shared tabs, regardless of how it was placed or which delegate's config the call happened to route through — the same design the code's own comments already described for the pane-placement case. Removed the now-dead `usesTabPlacement()` method. **Cost accepted:** every `stop()` now does one extra `pane.get` (+ `tab.list` if the pane is still there) round trip, even for a purely pane-placement fleet that previously skipped it entirely. This is deliberate — the alternative (Option 1) does not fix the reported defect in the confirmed repro shape, as shown above. ## Tests - **New:** `CompositePeerLauncherTest.stopThroughTheSingleDaemonShortcutStillClosesTheTabWhenTheFallbackDelegateUsesPanePlacement` — mixes placements across the two adapters (claude=`pane`, opencode=`tab`) sharing one daemon, calls `stop()` on an id `spawnedBy` never recorded (simulating a post-restart cache miss), and asserts `tab.close` fires. Every existing stop-fallback test in this class configured BOTH adapters as `tab` placement, so `usesTabPlacement()` was true either way and the mis-routing never showed. - **Updated:** `FleetAppTest.stopWorkerInPanePlacementClosesOnlyThePane` — its old assertion (`assertFalse(herdr.called("pane.get"))`) documented exactly the skip this fix removes. Rewrote it to model pane placement realistically (the peer's pane shares its tab with another occupant, `withWorkerTabPaneCount(2)`) and assert the tab is still never closed, now via the single-occupant check rather than via skipped resolution. ## Mutation proof Reverted the fix locally (restored the `usesTabPlacement()` gate) and ran the full suite: exactly 2 failures, both expected — - `CompositePeerLauncherTest.stopThroughTheSingleDaemonShortcutStillClosesTheTabWhenTheFallbackDelegateUsesPanePlacement` (`tab.close` never fired) - `FleetAppTest.stopWorkerInPanePlacementClosesOnlyThePane` (`pane.get` never fired) Restored the fix afterward; full build is green again. ## Build `mvn clean install`, run unpiped: `Tests run: 1364, Failures: 0, Errors: 0, Skipped: 0` — `BUILD SUCCESS`. ## Shape note (not fixed, per ticket scope) Swept `CompositePeerLauncher.java` and `HerdrPeerLauncher.java` for other places deciding teardown behaviour from a delegate's static config rather than the pane's real state. `CompositePeerLauncher.clearContext(id)` has a related but distinct issue: on a `spawnedBy` cache miss it just no-ops (`return false`) rather than falling back to a probe or a shortcut, so it silently stops resetting context for any peer whose owner record was lost to a restart — same root cause (in-memory `spawnedBy`), different failure mode (silent no-op, not mis-routed teardown). Not fixed here, out of scope.
agent added 1 commit 2026-09-04 11:36:47 +02:00
fleetd #342: base tab-cleanup teardown on the pane's real placement, not a delegate's static config
CI / contract (pull_request) Successful in 1m22s
Details
CI / build (pull_request) Failing after 1m38s
Details
3fd23ecafa 
HerdrPeerLauncher.stop() used to gate spaces.locatePane() on usesTabPlacement(),
which reads the delegate's OWN configured profiles. When CompositePeerLauncher's
single-daemon stop() shortcut hands a pane to a delegate that never spawned it
(spawnedBy empty after a daemon restart, herdrDaemonCount()==1), that delegate's
placement config says nothing true about how the pane was actually placed, and a
dedicated tab could be skipped and leaked.

Resolve the tab unconditionally instead — WorkspaceControl#locatePane already
tolerates a missing pane by returning null — and let the existing single-occupant
check (tabPaneCount()==1) be the only thing that decides whether to close it, same
as it already protects a shared tab regardless of declared placement.

Adds a mixed-placement CompositePeerLauncherTest (every existing stop-fallback test
configured both adapters as tab placement, so the mis-routing never showed) and
updates FleetAppTest#stopWorkerInPanePlacementClosesOnlyThePane, whose old
assertion (no pane.get on pane placement) documented exactly the skip this fix
removes.
ltms closed this pull request 2026-09-04 11:41:09 +02:00
Some checks are pending
CI / contract (pull_request) Successful in 1m22s
Details
CI / build (pull_request) Failing after 1m38s
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
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#350
Delete Branch "worker/fd342-174a17-2"

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?