fix(server): make Stop work while a chat runs background work (#528)

.changeset/stop-during-background-phase.md

@@ -0,0 +1,45 @@
+---
+"@paddock/server": patch
+---
+
+Fix Stop being a permanent no-op while a chat runs background work (#528)
+
+A chat could sit with the spinner and the **Stop** button showing forever. Stop
+did nothing — no error, no frame, no log line. The composer silently queued
+anything typed instead of sending it, and reloading didn't help because the state
+is server-authoritative and replays as running. Only restarting the server
+cleared it.
+
+Two independent things had to be wrong at once, and both were.
+
+**The turn had no cancellable identity.** Once a session-mode turn's primary
+`result` lands, the session can stay open — the reaper holds it while the turn's
+background work runs — and autonomous re-invocation turns keep arriving on the
+same stream. Paddock renders that stretch through `makeBackgroundTurnSink`, which
+opened its hub turn and never called `setJobId`. `setJobId` was being called at
+only two of the five turn-start sites, and this was one of the three that missed,
+so every frame and every `chat:active` carried `jobId: null`. The client's
+deferred-cancel (#196) waits for a jobId that never arrives, so clicking Stop put
+**nothing on the wire at all** — which is why it failed silently rather than
+erroring. The sink now mints a synthetic job id and publishes it the moment the
+turn opens, exactly as the foreground path does via `onJobCreated`.
+
+**Nothing it could route to.** `HerdctlService.cancel` knew two kinds of id: a
+live turn in `liveSessions` (→ `session.interrupt()`) and a batch job (→
+`cancelJob`). The primary turn's `liveSessions` entry is deleted the moment it
+returns, so a background-phase id matched neither and fell through to
+`cancelJob(<synthetic uuid>)` → `JobNotFoundError` → `false`, discarded by the WS
+layer. `interrupt()` would have been wrong anyway: it targets an in-flight model
+turn, and this session is idle, holding background work. Cancel now routes these
+to `fleet.reapChatSession()` (new in `@herdctl/core` 5.31.0) — end the session,
+let the stream end, and let the existing unwind emit `chat:complete` and unlock
+the UI.
+
+The wedge is easiest to hit on a **subscription usage limit**: sub-agents die, the
+parent's re-invocation turn dies without a Stop hook, and the reaper's
+`awaitingTasks` state (cleared by that turn's `activity`) means no later signal
+can ever reap the session. It also covers the originally reported trigger — a
+model-authored `until` loop whose sentinel never arrives, so the background task
+set never drains.
+
+Requires `@herdctl/core` ≥ 5.31.0.

docs/INTEGRATION.md

@@ -259,6 +259,24 @@ survive a turn boundary: herdctl's reaper keeps the session alive while it holds
 live background work. The caller must treat **the message stream ending as a reap**
 and re-open (resume) later to keep driving the conversation.
 
+The keep-alive rule has **no backstop** — no idle timer, no max lifetime. So a
+session whose background work never finishes is never reaped on its own and its
+message stream never ends, which (since we derive "is this chat running?" from
+that stream) leaves the chat wedged as running forever. **`fleet.reapChatSession(sessionId)`**
+(core ≥ 5.31.0) is the way out: it closes such a session on demand, the stream
+ends, and our ordinary unwind emits `chat:complete`. `HerdctlService.cancel`
+routes Stop here for background-phase turns (paddock#528).
+
+Do **not** reach for `session.close()` instead. It closes the query behind the
+reaper's bookkeeping, leaving the id registered live — a later resume of that
+chat then stalls until its 5-minute ceiling, and the session's wakes are skipped
+indefinitely.
+
+`session.interrupt()` is the other half of the pair, and the two are not
+interchangeable: `interrupt()` ends an **in-flight model turn** and keeps the
+session usable, so it's what Stop means during a live turn. A session held open
+purely for background work has no model turn to interrupt.
+
 `listAgentCommands(agentName, options)` is the one-shot convenience wrapper —
 opens a session, reads the command list, always closes.
 
@@ -345,6 +363,7 @@ fleet.getAgentWorkingDirectory(name);                  // string | undefined
 await fleet.deleteSession(name, sessionId);            // removes the transcript
 await fleet.setSessionName(name, sessionId, custom);   // custom display name
 fleet.invalidateSessions(name);                        // force a fresh listing
+fleet.reapChatSession(sessionId);                      // close a managed session now (§c.1)
 ```
 
 Paddock uses this layer exclusively — there is no `new SessionDiscoveryService(…)`

packages/server/package.json

@@ -26,7 +26,7 @@
     "@fastify/swagger-ui": "^4.2.0",
     "@fastify/websocket": "10.0.1",
     "@herdctl/chat": "^0.8.0",
-    "@herdctl/core": "^5.29.1",
+    "@herdctl/core": "^5.31.0",
     "@modelcontextprotocol/sdk": "^1.29.0",
     "fastify": "4.28.1",
     "jose": "^6.2.3",

packages/server/src/herdctl.ts

@@ -305,6 +305,25 @@ export class HerdctlService {
    */
   private liveSessions = new Map<string, RuntimeSession>();
 
+  /**
+   * Background-phase turns keyed by the synthetic job id the sink minted for
+   * them, valued by the chat session they belong to (paddock#528).
+   *
+   * After a turn's primary `result` lands, {@link chatSession} returns and drops
+   * its {@link liveSessions} entry — but the session itself may live on, held
+   * open by the reaper because the turn launched background work, streaming
+   * autonomous re-invocation turns. Those turns are real, running, and shown as
+   * such; until now they were also uncancellable, because nothing registered
+   * them anywhere Stop could reach.
+   *
+   * `interrupt()` would be the wrong tool even if it could: it targets an
+   * in-flight model turn, and the wedge case is a session sitting idle holding
+   * background work that will never finish. So these route to a force-reap
+   * instead — end the session, let the stream end, and let the ordinary unwind
+   * emit `chat:complete`. Entries are removed when the background stream ends.
+   */
+  private backgroundTurns = new Map<string, string>();
+
   /**
    * Incremental index of `<stateDir>/jobs`, backing the unread-badge reads
    * (`lastTurnCompletedAt*`) — one per service, reused across requests so a warm
@@ -1039,11 +1058,30 @@ export class HerdctlService {
     return { sessionId };
   }
 
+  /**
+   * Register a background-phase turn so Stop can reach it (paddock#528). Called
+   * by the background turn sink once it knows both the synthetic job id it
+   * published and the session that stream belongs to. Re-registering the same
+   * job id with a later session id is fine — the sink refreshes the mapping if
+   * the stream resolves a different session.
+   */
+  registerBackgroundTurn(jobId: string, sessionId: string): void {
+    this.backgroundTurns.set(jobId, sessionId);
+  }
+
+  /** Forget a background-phase turn once its stream has ended. */
+  unregisterBackgroundTurn(jobId: string): void {
+    this.backgroundTurns.delete(jobId);
+  }
+
   /**
    * Cancel a running turn (Stop button → WS chat:cancel). Handles BOTH drive
    * modes off the single id the client holds as `jobId`:
    *  - **session mode** — the id is a synthetic turn id in {@link liveSessions};
    *    interrupt the live `RuntimeSession` (there is no herdctl job to cancel).
+   *  - **background phase** — the id is a synthetic id in
+   *    {@link backgroundTurns}; there is no model turn to interrupt, so reap the
+   *    session (paddock#528).
    *  - **batch mode** — the id is a real herdctl job id; abort it via `cancelJob`
    *    (which kills the CLI subprocess / aborts the SDK query).
    *
@@ -1062,6 +1100,21 @@ export class HerdctlService {
         return false;
       }
     }
+    // Background phase: the primary turn is over and its liveSessions entry is
+    // gone, but the session is still open streaming autonomous re-invocation
+    // turns. Stop here means "end this session" — reaping reaches even a session
+    // the reap policy would hold open forever, and ending the stream is what
+    // drives the sink's onDone → chat:complete → the UI unlocking.
+    const backgroundSession = this.backgroundTurns.get(jobId);
+    if (backgroundSession !== undefined) {
+      const reaped = this.manager.reapChatSession(backgroundSession);
+      if (!reaped) {
+        console.warn(
+          `[herdctl] no live managed session ${backgroundSession} to reap for background turn ${jobId}`,
+        );
+      }
+      return reaped;
+    }
     try {
       await this.manager.cancelJob(jobId);
       return true;

packages/server/src/ws-turn.ts

@@ -11,6 +11,7 @@
  * as a group keeps that intact with no late-binding. ws.ts's socket layer consumes
  * the returned surface.
  */
+import { randomUUID } from "node:crypto";
 import type {
   SDKMessage,
   SessionWakeEntry,
@@ -260,6 +261,13 @@ const makeBackgroundTurnSink = (
   let producedReply = false;
   let noticeEmitted = false;
   let sawError = false;
+  // #528: the synthetic job id published for this background stretch, and the
+  // session id currently registered against it. The stretch is a real running
+  // turn as far as the UI is concerned, so it needs a cancellable identity —
+  // without one `chat:active` carries `jobId: null`, the client's deferred
+  // cancel never fires, and Stop puts nothing on the wire at all.
+  let bgJobId: string | null = null;
+  let registeredSession: string | null = null;
   // #429: sub-agent launches recovered live from the tool_use input, keyed by
   // toolUseId, so the enriched card renders without a refresh (see subagentLaunchFields).
   const launches = new Map<string, SubagentLaunch>();
@@ -268,6 +276,18 @@ const makeBackgroundTurnSink = (
     sessionId: resolvedSession,
     jobId: turn?.jobId ?? null,
   });
+  /**
+   * Point the published job id at whatever session this stream has resolved.
+   * Deferred rather than done once at mint time because the id is published when
+   * the turn opens, which can precede the first message that names a session;
+   * re-run on every message so a stream that resolves (or moves to) a different
+   * session stays cancellable.
+   */
+  const syncBackgroundRegistration = (): void => {
+    if (!bgJobId || !resolvedSession || registeredSession === resolvedSession) return;
+    deps.herdctl.registerBackgroundTurn(bgJobId, resolvedSession);
+    registeredSession = resolvedSession;
+  };
   const emitNotice = (notice: TurnNotice): void => {
     if (!turn || noticeEmitted) return;
     if (suppressNoticeAfterReply(notice, producedReply)) return;
@@ -281,6 +301,13 @@ const makeBackgroundTurnSink = (
     if (turn) return;
     resolvedSession = sid;
     turn = hub.startTurn(projectSlug, null, sid);
+    // #528: give the stretch a cancellable identity the moment it opens, exactly
+    // as the foreground path does via onJobCreated. Stop then routes to a
+    // force-reap of this session rather than an interrupt — there is no model
+    // turn in flight here to interrupt.
+    bgJobId = randomUUID();
+    turn.setJobId(bgJobId);
+    syncBackgroundRegistration();
     const t = turn;
     translate = createSDKMessageHandler({
       onText: (chunk) => {
@@ -332,6 +359,7 @@ const makeBackgroundTurnSink = (
     // across re-invocation boundaries.
     ensureTurn(m.session_id ?? resolvedSession);
     if (m.session_id) turn!.setSession(m.session_id);
+    syncBackgroundRegistration();
     if (messageProducedReply(m as Parameters<typeof messageProducedReply>[0]))
       producedReply = true;
     const notice = noticeFromMessage(m as Parameters<typeof noticeFromMessage>[0]);
@@ -347,6 +375,13 @@ const makeBackgroundTurnSink = (
   // Finalize the single turn once the background stream ends (reaper reap). No-op
   // if nothing was ever rendered (a sidechain-only stretch opened no turn).
   const onDone = (): void => {
+    // Drop the cancellable identity first, and unconditionally: the stream is
+    // over, so a later Stop must fall through to the "already finished" path
+    // rather than reaping whatever session has since taken this id. Runs even
+    // for a sidechain-only stretch that opened no turn, and on a second call.
+    if (bgJobId) deps.herdctl.unregisterBackgroundTurn(bgJobId);
+    bgJobId = null;
+    registeredSession = null;
     if (!turn) return;
     turn.emit({
       type: "chat:complete",