Merge CB-565: remove the recycle trap (PR #35)
CI / contract (push) Successful in 58s
CI / build (push) Successful in 1m27s

SessionManager.recycle called the 4-argument acquire overload, which defaults
the role to DEV and requests no worktree. So it carried profile, cwd and owner
across and silently dropped two things: the member's role, and its worktree.

A recycled architect would have come back a plain worker, never rebound to its
slot, with nothing logged. Worse, a recycled member would have come back with
no worktree at all — and a member's uncommitted work exists in exactly one
place. Role loss is recoverable; that is not.

Nothing called it. The only reference outside its own javadoc was one test, and
the context-cap path calls release, not recycle. So this was a trap waiting for
its first caller, and that caller would not have noticed either loss.

Deleted rather than repaired, on the CB-561 precedent: an API that looks correct
and silently drops a property is worse than no API. The no-reuse invariant it
documented is still true and is now stated directly.

Found by a reviewer asked to hunt the rest of the CB-548 fallout. The worktree
half was found while verifying the report.
This commit is contained in:
Dai Ha
2026-08-15 04:31:46 +02:00
3 changed files with 5 additions and 55 deletions
@@ -29,8 +29,7 @@ import java.util.function.LongSupplier;
* teardown on top.
*
* <p>The state machine is intentionally one-shot / no-reuse: every acquired worker is fresh,
* and a finished or released worker is torn down, never pooled. {@link #recycle} is a convenience
* for {@code release + acquire} with a new distinct pane id.
* and a finished or released worker is torn down, never pooled or reused.
*
* <p>The manager implements {@link TurnListener} so the injector's turn boundaries drive
* {@code READY → BUSY → DONE} (or {@code FAILED}). It exposes a {@link MemberPresence} view via
@@ -180,7 +179,7 @@ public final class SessionManager implements TurnListener {
* <p>CB-544: these are two concerns that used to be fused. Stopping the pane is correct on every
* teardown — the worker process must end. Removing the worktree is a destructive act that is only
* correct for a deliberately-finished teardown (an explicit stop of a completed session, the
* reaper releasing a genuinely idle one, a context-capped or recycled session). A shutdown drain
* reaper releasing a genuinely idle one, or a context-capped session). A shutdown drain
* must stop panes but preserve worktrees: a worker's uncommitted work exists in exactly one
* place — its worktree — so deleting it while the daemon simply goes down is silent data loss,
* with no copy and no error. Do NOT fuse these back together; the cost of an orphaned worktree
@@ -248,7 +247,7 @@ public final class SessionManager implements TurnListener {
/**
* Register a callback invoked with a session's {@code terminalId} whenever it is released
* (CB-516). Every teardown path funnels through {@link #release}, so one hook covers the REST
* and MCP stop tools, the idle-TTL reaper, {@code recycle}, and shutdown drain alike.
* and MCP stop tools, the idle-TTL reaper, and shutdown drain alike.
*
* <p>Added rather than injected because {@code MessageService} — one intended listener — is
* constructed after this manager (it needs the injector and rendezvous, which need the session
@@ -370,19 +369,6 @@ public final class SessionManager implements TurnListener {
return launcher.defaultProfile();
}
/**
* Release the old session and acquire a fresh one with the same profile and working directory.
* The new session is guaranteed to have a pane id distinct from the old one (no-reuse invariant).
*/
public MemberSession recycle(String paneId) {
MemberSession old = registry.get(paneId);
if (old == null) {
throw new IllegalArgumentException("no session for paneId " + paneId);
}
release(paneId);
return acquire(old.profile(), old.cwd(), old.cwd(), old.ownerTerminal());
}
/** The session for {@code paneId}, if it is still registered and not released. */
public Optional<MemberSession> get(String paneId) {
return Optional.ofNullable(registry.get(paneId));
@@ -157,35 +157,6 @@ class SessionManagerTest {
assertTrue(sessions.roster().contains(updated), "FAILED is still in acquired-minus-released roster");
}
@Test
void recycleProducesNewPaneIdAndOldOneIsGone() {
FakeHerdr herdr = new FakeHerdr();
SessionManager sessions = sessionManager(herdr);
MemberSession oldSession = sessions.acquire("ltms-local", null, "/caller", "term_primary");
String oldPane = oldSession.paneId();
String oldTerminal = oldSession.terminalId();
MemberSession fresh = sessions.recycle(oldPane);
assertNotEquals(oldPane, fresh.paneId(), "recycle yields a new pane id");
assertNotEquals(oldTerminal, fresh.terminalId(), "recycle yields a new terminal id");
assertEquals(oldSession.profile(), fresh.profile(), "profile is preserved");
assertEquals(oldSession.cwd(), fresh.cwd(), "cwd is preserved");
assertEquals(oldSession.ownerTerminal(), fresh.ownerTerminal(), "owner is preserved");
assertTrue(sessions.get(oldPane).isEmpty(), "old pane is deregistered");
assertEquals(1, sessions.roster().size(), "only the fresh session remains");
assertEquals(fresh.paneId(), sessions.roster().getFirst().paneId());
// The old session was the first spawn → pane w9:pRoot_1 (CB-519: the registry key is the
// uuid id, so teardown is asserted on the real pane coordinate).
long paneCloseCount = herdr.calls.stream()
.filter(c -> "pane.close".equals(c.method()))
.filter(c -> "w9:pRoot_1".equals(((Map<?, ?>) c.params()).get("pane_id")))
.count();
assertEquals(1, paneCloseCount, "the old worker was torn down");
}
@Test
void rosterReflectsAcquiredMinusReleased() {
FakeHerdr herdr = new FakeHerdr();
+2 -9
View File
@@ -35,7 +35,7 @@ build on.
- **No checkpoint content.** Writing `STATE.md` + commit on teardown is CB-302; CB-301 only exposes
the release hook it will attach to.
"Recycle" under no-reuse is simply **release + fresh acquire** — a helper, not a pool operation.
Under no-reuse, a released session is terminal. A new `acquire` always creates a fresh session.
## Design
@@ -46,11 +46,6 @@ ownership on top.
**Package:** new `dev.ltms.bridged.session` — keeps the registry/lifecycle concern separate from
the `worker` spawn mechanics. Holds `SessionManager` + `WorkerSession`.
**`recycle` is IN SCOPE for CB-301** (decided): implement `recycle(paneId, …)` = `release` the old
session then `acquire` a fresh one, asserting a new distinct paneId (the no-reuse invariant). It is
a thin convenience over the two primitives, shipped now so the no-reuse teardown+respawn path is
covered by a test from day one.
### `WorkerSession` (record or small mutable holder)
| Field | Source | Notes |
@@ -88,7 +83,6 @@ SPAWNING|READY|BUSY|DONE --vanished/drop--> FAILED
final class SessionManager {
WorkerSession acquire(String profile, String requestedCwd, String callerCwd, String ownerTerminal);
void release(String paneId); // deterministic teardown + deregister
WorkerSession recycle(String paneId, ...); // release + acquire (no-reuse convenience)
Optional<WorkerSession> get(String paneId);
List<WorkerSession> roster(); // bridge-owned view (CB-304 consumes this)
// lifecycle hooks (package-private): onReady/onDelivered/onComplete/onFailed(target)
@@ -120,8 +114,7 @@ final class SessionManager {
3. `release` tears the worker down via `WorkerService.stop` and removes it from `roster()`;
a second `release` on the same paneId is a harmless no-op.
4. `onTurnFailed` / drop moves the session to `FAILED` and it is absent from the live roster.
5. `recycle` produces a new paneId and the old one is gone (no-reuse invariant).
6. `roster()` reflects exactly the sessions acquired-minus-released, joined with live status.
5. `roster()` reflects exactly the sessions acquired-minus-released, joined with live status.
## Seams left open (deliberately)