#293: wrap the bare tab.close in HerdrPeerLauncher.stop()
The pane is already closed by the time spaces.closeTab runs, so a failing tab.close is cosmetic workspace tidying, not a real teardown failure. Left bare, it propagated out of stop() and masked releaseZdotdir (ZDOTDIR leak) and, worse, SessionManager.release()'s worktree removal (no self-heal, no retry — the registry entry is already gone by then). Wrap it in a try/catch that logs a WARN naming the tab id, matching the "must not mask a real teardown failure above" comment already on releaseZdotdir. isAlreadyGone is untouched — this continues past *any* tab.close failure, not just *_not_found, since the failure is cosmetic regardless of its cause. Adds FakeHerdr#tabCloseFailsWith/tabCloseFailsForTab (the tab.close counterpart to #290's paneCloseFailsForPane) plus two tests: one proving releaseZdotdir still runs (the generated ZDOTDIR is deleted) and one proving SessionManager.release() still removes the worktree, both with a non-not_found tab.close failure.
This commit is contained in:
@@ -913,9 +913,14 @@ public abstract class HerdrPeerLauncher implements PeerLauncher {
|
||||
* {@link #reapOrphanWorkers() orphan-reap} and spawn-gate-timeout paths, plus any caller that
|
||||
* passes a pane directly, keep working without an owning id.
|
||||
*
|
||||
* <p>Resolves the tab from the pane <em>before</em> closing it. An already-gone pane/tab
|
||||
* (repeated DELETE, crashed peer) is treated as success; any other failure propagates so a
|
||||
* genuinely failed teardown is not reported as done.
|
||||
* <p>Resolves the tab from the pane <em>before</em> closing it. {@code agents.close} (the pane)
|
||||
* is the one step whose failure means the teardown itself may not have happened: an already-gone
|
||||
* pane (repeated DELETE, crashed peer) is treated as success, but any other failure propagates so
|
||||
* a genuinely failed teardown is not reported as done. {@code spaces.closeTab} (fleetd #293) is
|
||||
* different — by the time it runs the pane is already closed, so it is cosmetic workspace tidying
|
||||
* rather than a real teardown failure, and a failure there is logged and never propagates, so it
|
||||
* cannot mask the two cleanups below it ({@link #releaseZdotdir}, and the caller's worktree
|
||||
* removal in {@code SessionManager.release}).
|
||||
*/
|
||||
@Override
|
||||
public void stop(String idOrPane) {
|
||||
@@ -934,7 +939,18 @@ public abstract class HerdrPeerLauncher implements PeerLauncher {
|
||||
log.debug("pane.close({}) ignored — already gone: {}", paneId, e.getMessage());
|
||||
}
|
||||
if (loc != null && loc.tabPaneCount() == 1) {
|
||||
spaces.closeTab(loc.tabId());
|
||||
// fleetd #293: the pane above is already closed by this point, so a failing tab.close is
|
||||
// cosmetic workspace tidying, not a real teardown failure — it must not mask the two
|
||||
// cleanups below it (releaseZdotdir, and the caller's worktree removal). Unlike
|
||||
// agents.close above, this is not narrowed to "already gone": any failure here, whatever
|
||||
// its cause, is one we continue past, so we log it at WARN (not debug) with the tab id a
|
||||
// person can go close by hand.
|
||||
try {
|
||||
spaces.closeTab(loc.tabId());
|
||||
} catch (HerdrException e) {
|
||||
log.warn("tab.close({}) failed — the pane is already torn down, so continuing; the "
|
||||
+ "tab may need manual cleanup: {}", loc.tabId(), e.getMessage());
|
||||
}
|
||||
} else if (loc != null) {
|
||||
log.debug("not closing tab {} — it holds {} panes (not a dedicated peer tab)",
|
||||
loc.tabId(), loc.tabPaneCount());
|
||||
|
||||
@@ -43,6 +43,8 @@ public final class FakeHerdr implements HerdrClient {
|
||||
private int workerTabPaneCount = 1;
|
||||
private String paneCloseErrorCode = null;
|
||||
private final Map<String, String> paneCloseErrorCodeFor = new ConcurrentHashMap<>();
|
||||
private String tabCloseErrorCode = null;
|
||||
private final Map<String, String> tabCloseErrorCodeFor = new ConcurrentHashMap<>();
|
||||
private String agentSendErrorCode = null;
|
||||
private boolean noPanes = false;
|
||||
private volatile String agentStatus = "idle"; // steady-state agent.get status
|
||||
@@ -106,6 +108,26 @@ public final class FakeHerdr implements HerdrClient {
|
||||
return this;
|
||||
}
|
||||
|
||||
/** Make {@code tab.close} fail with this herdr error code, for every tab. */
|
||||
public FakeHerdr tabCloseFailsWith(String code) {
|
||||
this.tabCloseErrorCode = code;
|
||||
return this;
|
||||
}
|
||||
|
||||
/**
|
||||
* Make {@code tab.close} fail with this herdr error code, but only for the given {@code
|
||||
* tab_id} — every other tab's {@code tab.close} still succeeds. The {@code tab.close}
|
||||
* counterpart to {@link #paneCloseFailsForPane} (fleetd #290): lets a test make exactly one
|
||||
* session's tab teardown fail while proving the rest of {@code stop()} — {@code
|
||||
* releaseZdotdir}, and the caller's worktree removal — still runs (fleetd #293). Named "ForTab"
|
||||
* rather than "ForPane" (unlike its sibling) because {@code tab.close} keys on {@code tab_id},
|
||||
* not a pane id.
|
||||
*/
|
||||
public FakeHerdr tabCloseFailsForTab(String tabId, String code) {
|
||||
this.tabCloseErrorCodeFor.put(tabId, code);
|
||||
return this;
|
||||
}
|
||||
|
||||
/**
|
||||
* Make {@code pane.list} report no panes at all — models a second herdr daemon (CB-185) that
|
||||
* simply does not host the pane a {@link PaneLocator} is searching for.
|
||||
@@ -352,7 +374,17 @@ public final class FakeHerdr implements HerdrClient {
|
||||
.formatted(workerTabPaneCount,
|
||||
seeded.isEmpty() ? "" : "," + String.join(",", seeded)));
|
||||
}
|
||||
case "tab.close" -> mapper.readTree("{\"type\":\"ok\"}");
|
||||
case "tab.close" -> {
|
||||
Object tabIdParam = params instanceof Map<?, ?> m ? m.get("tab_id") : null;
|
||||
String perTabCode = tabIdParam == null ? null
|
||||
: tabCloseErrorCodeFor.get(String.valueOf(tabIdParam));
|
||||
String code = perTabCode != null ? perTabCode : tabCloseErrorCode;
|
||||
if (code != null) {
|
||||
throw new HerdrException("herdr error [" + code + "]: tab.close failed",
|
||||
code, null);
|
||||
}
|
||||
yield mapper.readTree("{\"type\":\"ok\"}");
|
||||
}
|
||||
case "pane.get" -> mapper.readTree("""
|
||||
{"type":"pane_info","pane":{"pane_id":"w9:pW","workspace_id":"w9",
|
||||
"tab_id":"w9:t2","agent_status":"idle"}}""");
|
||||
|
||||
@@ -925,6 +925,71 @@ class ClaudeCodeLauncherTest {
|
||||
assertTrue(herdr.called("pane.close"), "stop via handle.id() must close the pane");
|
||||
}
|
||||
|
||||
/** A tab-placement launcher with {@code memberCredentials policy=allow-list} under a zsh shell — the
|
||||
* combination that makes {@link HerdrPeerLauncher#spawn} generate a real ZDOTDIR, so {@code
|
||||
* releaseZdotdir}'s effect (the directory's deletion) is observable from a test. */
|
||||
private ClaudeCodeLauncher serviceWithAllowList(FakeHerdr herdr) {
|
||||
FleetConfig.Profile cfg = new FleetConfig.Profile(
|
||||
"ltms-local", "http://gx00.gw:8000", "coder", null, "FLEETD_WORKER_TOKEN",
|
||||
List.of("ccs", "ltms-local"), "tab", "fleetd-workers",
|
||||
"worker: {profile} #{n}", null, null, null);
|
||||
Supplier<FleetConfig.MemberCredentials> creds = () -> new FleetConfig.MemberCredentials(
|
||||
FleetConfig.MemberCredentials.POLICY_ALLOW_LIST, List.of(), List.of(), null);
|
||||
Function<String, String> env = name -> "SHELL".equals(name) ? "/bin/zsh" : null;
|
||||
return new ClaudeCodeLauncher(new AgentControl(herdr), new WorkspaceControl(herdr),
|
||||
new SubscriptionGuard(Set.of("gx00.gw")), Map.of(cfg.profile(), cfg), cfg.profile(),
|
||||
env, 0, System::currentTimeMillis, () -> { }, null, creds);
|
||||
}
|
||||
|
||||
/**
|
||||
* fleetd #293: {@code stop()} used to run {@code spaces.closeTab} bare — any non-{@code
|
||||
* *_not_found} herdr error propagated straight out of {@code stop()}, skipping {@code
|
||||
* releaseZdotdir} entirely (the pane was already closed by that point, so the tab-close failure
|
||||
* is cosmetic, not a real teardown failure). Proves both halves of the fix: {@code stop()} no
|
||||
* longer throws for this failure, and {@code releaseZdotdir} still runs — observed here by the
|
||||
* generated ZDOTDIR actually being deleted, since {@code releaseZdotdir}'s last line is {@code
|
||||
* EnvAllowListScrub.deleteRecursively(dir)}.
|
||||
*/
|
||||
@Test
|
||||
@SuppressWarnings("unchecked")
|
||||
void stopStillReleasesZdotdirWhenCloseTabFailsWithANonNotFoundCode() {
|
||||
FakeHerdr herdr = new FakeHerdr();
|
||||
ClaudeCodeLauncher svc = serviceWithAllowList(herdr);
|
||||
PeerHandle handle = svc.spawn(new SpawnRequest(null, null, null));
|
||||
Map<String, Object> tabCreateParams = (Map<String, Object>) herdr.lastCall("tab.create").params();
|
||||
Map<String, String> tabEnv = (Map<String, String>) tabCreateParams.get("env");
|
||||
String zdotdir = tabEnv.get("ZDOTDIR");
|
||||
assertNotNull(zdotdir, "policy=allow-list under a zsh shell must have generated a ZDOTDIR: " + tabEnv);
|
||||
Path dir = Path.of(zdotdir);
|
||||
assertTrue(Files.isDirectory(dir), "the generated ZDOTDIR must exist before stop(): " + dir);
|
||||
herdr.tabCloseFailsForTab("w9:t2", "internal_error");
|
||||
|
||||
Logger logger = (Logger) LoggerFactory.getLogger(HerdrPeerLauncher.class);
|
||||
ListAppender<ILoggingEvent> appender = new ListAppender<>();
|
||||
appender.start();
|
||||
logger.addAppender(appender);
|
||||
try {
|
||||
assertDoesNotThrow(() -> svc.stop(handle.id()),
|
||||
"fleetd #293: a failing tab.close is cosmetic — it must not propagate out of stop()");
|
||||
} finally {
|
||||
logger.detachAppender(appender);
|
||||
}
|
||||
|
||||
assertTrue(herdr.called("tab.close"), "tab.close was still attempted");
|
||||
assertFalse(Files.exists(dir),
|
||||
"releaseZdotdir must still run and delete the generated ZDOTDIR despite the tab.close "
|
||||
+ "failure: " + dir);
|
||||
String warn = appender.list.stream()
|
||||
.filter(e -> e.getLevel().equals(Level.WARN))
|
||||
.map(ILoggingEvent::getFormattedMessage)
|
||||
.filter(m -> m.contains("tab.close") && m.contains("w9:t2"))
|
||||
.findFirst()
|
||||
.orElse(null);
|
||||
assertNotNull(warn, "the failing tab.close must be logged at WARN naming the tab id — a "
|
||||
+ "silently swallowed failure with no message is not an improvement. Log lines: "
|
||||
+ appender.list.stream().map(ILoggingEvent::getFormattedMessage).toList());
|
||||
}
|
||||
|
||||
// --- CB-519: host-unique id, decoupled from the pane coordinate ------------------------------
|
||||
|
||||
@Test
|
||||
|
||||
@@ -1156,6 +1156,41 @@ class SessionManagerTest {
|
||||
"COMPLETED release of a clean worktree still removes it");
|
||||
}
|
||||
|
||||
/**
|
||||
* fleetd #293: {@code HerdrPeerLauncher.stop()} used to run {@code spaces.closeTab} bare — a
|
||||
* failing {@code tab.close} (any code other than {@code *_not_found}) propagated straight out
|
||||
* of {@code stop()}. {@code SessionManager.release} calls {@code launcher.stop(paneId)} with
|
||||
* no try/catch (fleetd #283 wrapped the WORKTREE-removal step further down, not this one), so
|
||||
* the throw happened <em>before</em> that worktree-removal step ever ran — and by then {@code
|
||||
* registry.remove(paneId)} had already run, so a second {@code stop} is a no-op: the worktree
|
||||
* leaked with no retry path. The pane itself is already closed by the time {@code tab.close}
|
||||
* runs, so its failure is cosmetic workspace tidying, not a real teardown failure. The fix
|
||||
* wraps {@code closeTab} inside {@code stop()} so it no longer throws for this reason; this
|
||||
* test proves both halves at once: {@code release()} does not throw, and it still removes the
|
||||
* worktree.
|
||||
*/
|
||||
@Test
|
||||
void releaseStillRemovesTheWorktreeWhenCloseTabFails() {
|
||||
FakeHerdr herdr = new FakeHerdr();
|
||||
RecordingWorktrees worktrees = new RecordingWorktrees();
|
||||
SessionManager sessions = sessionManager(herdr, worktrees);
|
||||
MemberSession s = sessions.acquire("ltms-local", null, "/caller/proj", null,
|
||||
new WorktreeRequest("cb-293a", null));
|
||||
// FakeHerdr's pane.get always answers with tab_id "w9:t2" for a tab-placement spawn.
|
||||
herdr.tabCloseFailsForTab("w9:t2", "internal_error");
|
||||
|
||||
assertDoesNotThrow(() -> sessions.release(s.paneId()),
|
||||
"a failing tab.close is cosmetic (the pane is already closed by then) — it must not "
|
||||
+ "propagate out of release()");
|
||||
|
||||
assertTrue(herdr.called("tab.close"), "tab.close was still attempted");
|
||||
assertEquals(List.of(s.worktree()), worktrees.removeCalls(),
|
||||
"release() must still remove the worktree even though tab.close failed — this is "
|
||||
+ "the leak fleetd #293 reports: before the fix, release() never reached this "
|
||||
+ "step at all");
|
||||
assertTrue(sessions.get(s.paneId()).isEmpty(), "the session is still deregistered");
|
||||
}
|
||||
|
||||
@Test
|
||||
void unchangedRegressionDirtyCompletedReleaseStillPreservesTheWorktree() {
|
||||
FakeHerdr herdr = new FakeHerdr();
|
||||
|
||||
Reference in New Issue
Block a user