fix(serve): close session-binding races in scheduled-task create/reuse

R4-1: re-validate a caller-provided session under the cron write lock;
archive/delete tears the session out of the live map before its cron
hook runs, so a session that left the map between validation and commit
is now rejected with 409 session_not_live instead of binding a
201-returned task to an archived/deleted session.

R4-2: the in-lock duplicate-binding check now covers just-minted
sessions too (boundSessionId, not only providedSessionId) and runs
before the cap check; the alreadyBound branch no longer rolls the
session back, since a committed owner task means a concurrent
reuse-create won the race and owns the session.

R4-3 (narrowed, not closed): DELETE re-reads the cron file right before
closeSession and skips teardown when a surviving task references the
session; the residual re-read-to-close window needs session-scoped
serialization shared with the bind path (follow-up).

R4-4: keepalive bind writes also bail when any committed task already
references the just-minted session, mirroring the route's in-lock
check.

R4-5/R4-6: add the missing discriminating tests (mint-site naming,
sessionOwnedByTask validation); both mutation-verified.

Author: yiliang114

packages/cli/src/serve/routes/scheduled-tasks.test.ts

@@ -861,6 +861,83 @@ describe('scheduled-tasks routes', () => {
     expect(h.bridge.closed).toEqual([]);
   });
 
+  it('rejects a reuse create whose session is archived between validation and commit', async () => {
+    // The session passes the pre-lock validation (live, idle) but a
+    // concurrent archive/delete removes it from the live map before the cron
+    // write commits. The archive hook (disableTasksForSessions) only sees
+    // tasks already on disk, so it no-ops for this task — the under-write-lock
+    // re-validation is the only guard. Deleting it turns the 409 into a 201
+    // bound to an archived session.
+    h.bridge.liveSessions.set(CALLER_SESSION_ID, {
+      sessionId: CALLER_SESSION_ID,
+      workspaceCwd: h.workspace,
+      hasActivePrompt: false,
+    });
+    let summaryCalls = 0;
+    const originalGetSessionSummary = h.bridge.getSessionSummary.bind(h.bridge);
+    h.bridge.getSessionSummary = (sessionId: string) => {
+      summaryCalls += 1;
+      if (summaryCalls > 1) {
+        // Simulate the archive/delete landing after the first (pre-lock)
+        // validation: archiving removes a session from the live map first.
+        throw new SessionNotFoundError(sessionId);
+      }
+      return originalGetSessionSummary(sessionId);
+    };
+    const res = await create({
+      cron: '0 9 * * *',
+      prompt: 'p',
+      sessionId: CALLER_SESSION_ID,
+    });
+    expect(res.status).toBe(409);
+    expect(res.body.code).toBe('session_not_live');
+    expect(await readCronTasks(h.workspace)).toEqual([]);
+    // Caller-provided session — never torn down by this route.
+    expect(h.bridge.closed).toEqual([]);
+    expect(h.bridge.named).toEqual([]);
+  });
+
+  it('does not double-bind a just-minted session a concurrent reuse-create committed', async () => {
+    // Mint-vs-reuse race: a mint registers its session in the live map
+    // (doSpawn) BEFORE its cron write commits, so a concurrent reuse-create
+    // for that session passes every validation and commits first. The in-lock
+    // duplicate check must cover minted sessions too — deleting the check
+    // turns this create into a second 201 bound to the same session — and the
+    // rejected create must NOT tear down the session the winner now owns.
+    h.bridge.spawnOrAttach = async () => {
+      const sessionId = 'sess-contested-mint';
+      h.bridge.spawned.push(sessionId);
+      // Simulate the concurrent reuse-create committing while this mint's
+      // write is still pending.
+      await updateCronTasks(h.workspace, (tasks) => [
+        ...tasks,
+        {
+          id: 'reuse-task',
+          cron: '0 10 * * *',
+          prompt: 'q',
+          recurring: true,
+          createdAt: 1_700_000_000_000,
+          lastFiredAt: 1_700_000_000_000,
+          enabled: true,
+          sessionId,
+          sessionOwnedByTask: false,
+        },
+      ]);
+      return { sessionId };
+    };
+    const res = await create({ cron: '0 9 * * *', prompt: 'p' });
+    expect(res.status).toBe(409);
+    expect(res.body.code).toBe('session_already_bound');
+    // Exactly one task on disk — the reuse winner — still bound.
+    const tasks = await readCronTasks(h.workspace);
+    expect(tasks).toHaveLength(1);
+    expect(tasks[0]?.id).toBe('reuse-task');
+    expect(tasks[0]?.sessionId).toBe('sess-contested-mint');
+    // The loser must not kill the session the winner committed to.
+    expect(h.bridge.closed).toEqual([]);
+    expect(h.cleanupSession).not.toHaveBeenCalled();
+  });
+
   it('rejects an invalid sessionId field with 400 invalid_session_id', async () => {
     for (const bad of [
       123,
@@ -1322,6 +1399,49 @@ describe('scheduled-tasks routes', () => {
     expect(h.bridge.closed).toEqual(['sess-legacy']);
   });
 
+  it("does not close a deleted task's session when another committed task references it", async () => {
+    // DELETE captures the bound session under the lock, but a concurrent
+    // reuse-create can commit a binding to it right after the removal lands
+    // (its in-lock duplicate check legitimately passes once the old task is
+    // gone). The pre-close re-read must notice the surviving reference and
+    // skip the teardown — closing on the stale capture would kill the
+    // surviving task's live session. Deleting the recheck puts 'sess-shared'
+    // back in `closed`.
+    await updateCronTasks(h.workspace, (tasks) => [
+      ...tasks,
+      {
+        id: 'deleted-task',
+        cron: '0 9 * * *',
+        prompt: 'p',
+        recurring: true,
+        createdAt: 1_700_000_000_000,
+        lastFiredAt: 1_700_000_000_000,
+        enabled: true,
+        sessionId: 'sess-shared',
+        sessionOwnedByTask: true,
+      },
+      {
+        // The race winner: committed while DELETE's close was still pending.
+        id: 'surviving-task',
+        cron: '0 10 * * *',
+        prompt: 'q',
+        recurring: true,
+        createdAt: 1_700_000_000_000,
+        lastFiredAt: 1_700_000_000_000,
+        enabled: true,
+        sessionId: 'sess-shared',
+        sessionOwnedByTask: false,
+      },
+    ]);
+    const del = await request(h.app).delete('/scheduled-tasks/deleted-task');
+    expect(del.status).toBe(200);
+    expect(del.body).toEqual({ deleted: true, id: 'deleted-task' });
+    expect(h.bridge.closed).toEqual([]); // surviving task keeps its session
+    const tasks = await readCronTasks(h.workspace);
+    expect(tasks).toHaveLength(1);
+    expect(tasks[0]?.id).toBe('surviving-task');
+  });
+
   it('returns 500 (not 404) when the persisted-session probe hits a filesystem failure', async () => {
     // The probe helpers rethrow non-ENOENT errors (EACCES/EIO/ESTALE). Such a
     // failure is transient I/O, not "genuinely gone" — it must surface as a

packages/cli/src/serve/routes/scheduled-tasks.ts

@@ -875,30 +875,59 @@ function registerScheduledTaskCrudRoutes(
 
       let overCap = false;
       let alreadyBound = false;
+      let sessionGoneUnderLock = false;
       let rollbackBefore: DurableCronTask[] | undefined;
       let rollbackAfter: DurableCronTask[] | undefined;
       try {
         await runWithScheduledTaskTarget(target, () =>
           updateCronTasks(
             workspaceCwd,
             (tasks) => {
+              // Same-lock duplicate-binding check in BOTH binding modes: the
+              // pre-check read above is best-effort, and a concurrent create
+              // may have bound the same session since. For a caller-provided
+              // session that's another reuse-create; for a just-minted one
+              // it's a reuse-create that committed while this request's mint
+              // was still in flight (the mint registers the session in the
+              // live map before THIS write commits, so the reuse path's
+              // validation can pass against it). Runs before the cap check so
+              // an over-cap loser never tears down a session another
+              // committed task already references.
+              if (
+                boundSessionId !== undefined &&
+                tasks.some((t) => t.sessionId === boundSessionId)
+              ) {
+                alreadyBound = true;
+                return tasks;
+              }
+              // Re-validate a caller-provided session UNDER the write lock:
+              // archiving/deleting tears the session out of the live map
+              // BEFORE its cron hook (disable/removeTasksForSessions) runs,
+              // and that hook only sees tasks already on disk — so a session
+              // that left the live map between the pre-lock validation and
+              // this cycle is being archived/deleted and its hook skipped
+              // this (not yet written) task. Committing anyway would bind a
+              // 201-returned task to an archived or gone session. Cron write
+              // cycles are serialized, so a hook that runs after THIS cycle
+              // sees the new task and disables/removes it correctly.
+              if (providedSessionId !== undefined && bridge) {
+                try {
+                  bridge.getSessionSummary(providedSessionId);
+                } catch (err) {
+                  if (err instanceof SessionNotFoundError) {
+                    sessionGoneUnderLock = true;
+                    return tasks; // no write
+                  }
+                  throw err;
+                }
+              }
               // Cap check under the write lock so two concurrent creates can't both
               // slip past a stale count. Returning the input unchanged is a no-op
               // (no write), which the flag below turns into a 409.
               if (tasks.length >= MAX_SCHEDULED_TASKS) {
                 overCap = true;
                 return tasks;
               }
-              // Same-lock duplicate-binding check for a caller-provided
-              // session: the pre-check read above is best-effort, and a
-              // concurrent create may have bound the same session since.
-              if (
-                providedSessionId !== undefined &&
-                tasks.some((t) => t.sessionId === providedSessionId)
-              ) {
-                alreadyBound = true;
-                return tasks;
-              }
               rollbackBefore = tasks;
               rollbackAfter = [...tasks, task];
               return rollbackAfter;
@@ -942,8 +971,23 @@ function registerScheduledTaskCrudRoutes(
         });
         return;
       }
+      if (sessionGoneUnderLock) {
+        // Reuse mode only — a caller-provided session is never torn down
+        // here, so there is nothing to roll back. Retryable: the session's
+        // archive/delete completed between validation and commit.
+        res.status(409).json({
+          error:
+            'The requested session was archived or deleted while the task was being created; retry with a live session',
+          code: 'session_not_live',
+        });
+        return;
+      }
       if (alreadyBound) {
-        await rollbackSession();
+        // NO rollbackSession here: the in-lock check fires only when a
+        // COMMITTED task already references the bound session. For a
+        // just-minted session that means a concurrent reuse-create won the
+        // race and owns it — tearing it down would kill that task's session.
+        // (For a caller-provided session rollbackSession is a no-op anyway.)
         res.status(409).json({
           error:
             'The requested session is already bound to another scheduled task',
@@ -1352,12 +1396,35 @@ function registerScheduledTaskCrudRoutes(
       // task, may be the user's live working session, and must survive the
       // task's deletion (same invariant the create path's rollback honors).
       if (boundSessionId && sessionOwnedByTask && bridge) {
+        // Re-read just before teardown: between the removal commit above and
+        // this close, a concurrent reuse-create can bind THIS session (its
+        // in-lock duplicate check legitimately passes once the old task is
+        // gone) — from that task's perspective the session IS caller-provided
+        // and must survive. Closing on the stale capture would tear down the
+        // surviving task's live session mid-use. Best-effort: a rebind that
+        // commits between this re-read and the close still slips through;
+        // fully closing that window needs session-scoped serialization shared
+        // with the bind path (tracked as follow-up). A read failure falls
+        // back to the pre-recheck behavior (close).
+        let claimedBySurvivingTask = false;
         try {
-          await runWithScheduledTaskTarget(target, () =>
-            bridge.closeSession(boundSessionId!),
+          const currentTasks = await runWithScheduledTaskTarget(target, () =>
+            readCronTasks(workspaceCwd),
           );
-        } catch (error) {
-          if (sendActivityGateError(res, error)) return;
+          claimedBySurvivingTask = currentTasks.some(
+            (t) => t.sessionId === boundSessionId,
+          );
+        } catch {
+          // Read failure → keep the historical behavior (close the session).
+        }
+        if (!claimedBySurvivingTask) {
+          try {
+            await runWithScheduledTaskTarget(target, () =>
+              bridge.closeSession(boundSessionId!),
+            );
+          } catch (error) {
+            if (sendActivityGateError(res, error)) return;
+          }
         }
       }
       if (boundSessionId) {

packages/cli/src/serve/scheduled-task-keepalive.test.ts

@@ -661,8 +661,12 @@ describe('scheduled-task keepalive', () => {
   });
 
   it('binds an unbound task to a dedicated session and writes sessionId to disk', async () => {
+    // Named fixture + exact-name assertion discriminate the MINT-site naming
+    // payload (`task.name ?? task.prompt`): mutating it to `task.prompt`
+    // yields '⏰ check build' and fails this test. The already-bound rename
+    // branch is covered separately below.
     await updateCronTasks(workspace, () => [
-      task({ id: 'unbound-1', prompt: 'check build' }),
+      task({ id: 'unbound-1', prompt: 'check build', name: 'Digest' }),
     ]);
     const spawns: unknown[] = [];
     const names: Array<[string, { displayName?: string }]> = [];
@@ -693,7 +697,7 @@ describe('scheduled-task keepalive', () => {
     });
     expect(names).toHaveLength(1);
     expect(names[0]![0]).toBe('new-sess-1');
-    expect(names[0]![1].displayName).toContain('⏰');
+    expect(names[0]![1].displayName).toBe('⏰ Digest');
     const tasks = await readCronTasks(workspace);
     expect(tasks[0]!.sessionId).toBe('new-sess-1');
     // The keepalive minted this session, so the task records ownership —
@@ -883,6 +887,59 @@ describe('scheduled-task keepalive', () => {
     removeSpy.mockRestore();
   });
 
+  it('rolls back when the just-minted session is already committed to another task', async () => {
+    // The scheduled-tasks reuse path can bind a session as soon as the spawn
+    // registers it in the live map — BEFORE this bind write commits. The
+    // in-lock check must notice the committed reference and leave the task
+    // unbound (the orphan spawn rolls back), not double-bind the session.
+    const closed: string[] = [];
+    const removeSpy = vi
+      .spyOn(SessionService.prototype, 'removeSession')
+      .mockResolvedValue(true);
+    const raceBridge = {
+      ...bridge,
+      spawnOrAttach: async () => {
+        // Simulate a concurrent caller-provided binding committing while our
+        // spawn is in flight.
+        await updateCronTasks(workspace, (list) => [
+          ...list,
+          task({
+            id: 'caller-task',
+            sessionId: 'contested-sess',
+            sessionOwnedByTask: false,
+          }),
+        ]);
+        return { sessionId: 'contested-sess' };
+      },
+      closeSession: async (id: string) => {
+        closed.push(id);
+      },
+      markSessionCatalogChanged: vi.fn(),
+      updateSessionMetadata: () => {},
+    };
+    await updateCronTasks(workspace, () => [
+      task({ id: 'tool-task', prompt: 'contested' }),
+    ]);
+    const ka = startScheduledTaskKeepalive({
+      bridge: raceBridge,
+      boundWorkspace: workspace,
+      intervalMs: 60_000,
+    });
+    await ka.tick();
+    ka.stop();
+    // The orphaned mint is rolled back...
+    expect(closed).toContain('contested-sess');
+    expect(raceBridge.markSessionCatalogChanged).toHaveBeenCalledTimes(1);
+    // ...and the session stays bound to exactly ONE task: the caller's.
+    const tasks = await readCronTasks(workspace);
+    expect(tasks).toHaveLength(2);
+    const toolTask = tasks.find((t) => t.id === 'tool-task');
+    const callerTask = tasks.find((t) => t.id === 'caller-task');
+    expect(toolTask?.sessionId).toBeUndefined(); // still unbound
+    expect(callerTask?.sessionId).toBe('contested-sess');
+    removeSpy.mockRestore();
+  });
+
   it('a hung spawnOrAttach does not stall subsequent ticks', async () => {
     // spawnOrAttach is not abortable — if it hangs, the keepalive must time
     // out and move on so later ticks can still heartbeat/revive other

packages/cli/src/serve/scheduled-task-keepalive.ts

@@ -205,10 +205,20 @@ async function bindAndNameSessions(
         // read and this write-lock acquisition — only attach when the task is
         // still unbound and enabled. Otherwise return unchanged so the
         // orphan spawn is rolled back below.
+        //
+        // Also bail when ANY committed task already references the
+        // just-minted session: the scheduled-tasks reuse path can bind a
+        // session the moment the spawn above registers it in the live map,
+        // BEFORE this write commits — without this check the session would be
+        // bound to two tasks (same transcript, conflicting ⏰ renames), and a
+        // later delete of THIS task would close the session out from under
+        // the surviving one. The orphan rollback below then tears the
+        // unclaimed session back down.
         if (
           !list.some(
             (t) => t.id === task.id && !t.sessionId && t.enabled !== false,
-          )
+          ) ||
+          list.some((t) => t.sessionId === sessionId)
         ) {
           return list;
         }