Browse Source

Fix background task cancellation lifecycle

Alvin Unreal 2 months ago
parent
commit
fbd952a584

+ 39 - 3
src/multiplexer/session-manager.test.ts

@@ -289,7 +289,7 @@ describe('MultiplexerSessionManager', () => {
       expect(mockFetch).not.toHaveBeenCalled();
     });
 
-    test('closes pane when idle event reaches a different manager instance', async () => {
+    test('does not close another manager instance pane on idle event', async () => {
       const ctx = createMockContext();
       mockMultiplexer.spawnPane.mockResolvedValue({
         success: true,
@@ -315,7 +315,7 @@ describe('MultiplexerSessionManager', () => {
         properties: { sessionID: 'shared-child' },
       });
 
-      expect(mockMultiplexer.closePane).toHaveBeenCalledWith('p-shared-idle');
+      expect(mockMultiplexer.closePane).not.toHaveBeenCalled();
     });
 
     test('respawns resumed known session from a different manager instance', async () => {
@@ -351,7 +351,7 @@ describe('MultiplexerSessionManager', () => {
         },
       });
 
-      await secondManager.onSessionStatus({
+      await firstManager.onSessionStatus({
         type: 'session.idle',
         properties: { sessionID: 'resumed-child' },
       });
@@ -373,6 +373,42 @@ describe('MultiplexerSessionManager', () => {
       );
     });
 
+    test('does not close running background child pane on idle event', async () => {
+      const ctx = createMockContext();
+      const board = new BackgroundJobBoard();
+      board.registerLaunch({
+        taskID: 'running-idle-child',
+        parentSessionID: 'parent-1',
+        agent: 'explorer',
+      });
+      mockMultiplexer.spawnPane.mockResolvedValue({
+        success: true,
+        paneId: 'p-running-idle-child',
+      });
+      const manager = new MultiplexerSessionManager(
+        ctx,
+        defaultMultiplexerConfig,
+        board,
+      );
+
+      await manager.onSessionCreated({
+        type: 'session.created',
+        properties: {
+          info: { id: 'running-idle-child', parentID: 'parent-1' },
+        },
+      });
+
+      await manager.onSessionStatus({
+        type: 'session.status',
+        properties: {
+          sessionID: 'running-idle-child',
+          status: { type: 'idle' },
+        },
+      });
+
+      expect(mockMultiplexer.closePane).not.toHaveBeenCalled();
+    });
+
     test('does not close on transient status absence', async () => {
       const ctx = createMockContext();
       const manager = new MultiplexerSessionManager(

+ 43 - 1
src/multiplexer/session-manager.ts

@@ -15,6 +15,7 @@ interface TrackedSession {
   parentId: string;
   title: string;
   directory: string;
+  ownerInstanceId: string;
   createdAt: number;
   lastSeenAt: number;
   seenInStatus: boolean;
@@ -224,6 +225,7 @@ export class MultiplexerSessionManager {
         parentId,
         title,
         directory,
+        ownerInstanceId: this.instanceId,
         createdAt: now,
         lastSeenAt: now,
         seenInStatus: false,
@@ -253,6 +255,7 @@ export class MultiplexerSessionManager {
         sessionId,
         tracked: this.sessions.has(sessionId),
         known: this.knownSessions.has(sessionId),
+        ownerInstanceId: this.sessions.get(sessionId)?.ownerInstanceId,
         backgroundJobState: this.backgroundJobBoard?.get(sessionId)?.state,
       });
 
@@ -271,6 +274,7 @@ export class MultiplexerSessionManager {
         sessionId,
         tracked: this.sessions.has(sessionId),
         known: this.knownSessions.has(sessionId),
+        ownerInstanceId: this.sessions.get(sessionId)?.ownerInstanceId,
         backgroundJobState: this.backgroundJobBoard?.get(sessionId)?.state,
       });
       await this.closeSession(sessionId, 'idle');
@@ -283,6 +287,7 @@ export class MultiplexerSessionManager {
         sessionId,
         tracked: this.sessions.has(sessionId),
         known: this.knownSessions.has(sessionId),
+        ownerInstanceId: this.sessions.get(sessionId)?.ownerInstanceId,
         backgroundJobState: this.backgroundJobBoard?.get(sessionId)?.state,
       });
       await this.respawnIfKnown(sessionId);
@@ -301,6 +306,7 @@ export class MultiplexerSessionManager {
       sessionId,
       tracked: this.sessions.has(sessionId),
       known: this.knownSessions.has(sessionId),
+      ownerInstanceId: this.sessions.get(sessionId)?.ownerInstanceId,
       backgroundJobState: this.backgroundJobBoard?.get(sessionId)?.state,
     });
 
@@ -343,6 +349,16 @@ export class MultiplexerSessionManager {
         [];
 
       for (const [sessionId, tracked] of this.sessions.entries()) {
+        if (tracked.ownerInstanceId !== this.instanceId) {
+          log('[multiplexer-session-manager] skipping non-owner poll close', {
+            instanceId: this.instanceId,
+            ownerInstanceId: tracked.ownerInstanceId,
+            sessionId,
+            paneId: tracked.paneId,
+          });
+          continue;
+        }
+
         const status = allStatuses[sessionId];
         const isIdle = status?.type === 'idle';
 
@@ -358,7 +374,7 @@ export class MultiplexerSessionManager {
           !!tracked.missingSince &&
           now - tracked.missingSince >= SESSION_MISSING_GRACE_MS;
         const shouldKeepRunningBackgroundJob =
-          missingTooLong && this.isRunningBackgroundJob(sessionId);
+          (isIdle || missingTooLong) && this.isRunningBackgroundJob(sessionId);
         if (isIdle || missingTooLong) {
           if (shouldKeepRunningBackgroundJob) {
             log(
@@ -435,6 +451,31 @@ export class MultiplexerSessionManager {
       return;
     }
 
+    if (tracked.ownerInstanceId !== this.instanceId) {
+      log('[multiplexer-session-manager] close skipped; non-owner instance', {
+        instanceId: this.instanceId,
+        ownerInstanceId: tracked.ownerInstanceId,
+        sessionId,
+        paneId: tracked.paneId,
+        reason,
+      });
+      return;
+    }
+
+    if (reason === 'idle' && this.isRunningBackgroundJob(sessionId)) {
+      log(
+        '[multiplexer-session-manager] close skipped; background job running',
+        {
+          instanceId: this.instanceId,
+          sessionId,
+          paneId: tracked.paneId,
+          reason,
+          backgroundJobState: this.backgroundJobBoard?.get(sessionId)?.state,
+        },
+      );
+      return;
+    }
+
     this.sessions.delete(sessionId);
 
     log('[multiplexer-session-manager] closing session pane', {
@@ -547,6 +588,7 @@ export class MultiplexerSessionManager {
         parentId: known.parentId,
         title: known.title,
         directory: known.directory,
+        ownerInstanceId: this.instanceId,
         createdAt: now,
         lastSeenAt: now,
         seenInStatus: false,

+ 37 - 0
src/tools/cancel-task.test.ts

@@ -90,6 +90,43 @@ describe('cancel_task tool', () => {
     expect(board.get('ses_1')).toMatchObject({ state: 'completed' });
   });
 
+  test('still aborts stale cancelled jobs', async () => {
+    const { board, abort, cancelTask } = createTool();
+    board.registerLaunch({
+      taskID: 'ses_1',
+      parentSessionID: 'parent-1',
+      agent: 'explorer',
+    });
+    board.updateStatus({ taskID: 'ses_1', state: 'cancelled' });
+
+    const output = await cancelTask.execute(
+      { task_id: 'ses_1', reason: 'stop ghost worker' },
+      context,
+    );
+
+    expect(abort).toHaveBeenCalledWith({ path: { id: 'ses_1' } });
+    expect(String(output)).toContain('state: cancelled');
+  });
+
+  test('still aborts reconciled stale cancellations', async () => {
+    const { board, abort, cancelTask } = createTool();
+    board.registerLaunch({
+      taskID: 'ses_1',
+      parentSessionID: 'parent-1',
+      agent: 'explorer',
+    });
+    board.updateStatus({ taskID: 'ses_1', state: 'cancelled' });
+    board.markReconciled('ses_1');
+
+    const output = await cancelTask.execute(
+      { task_id: 'ses_1', reason: 'stop ghost worker' },
+      context,
+    );
+
+    expect(abort).toHaveBeenCalledWith({ path: { id: 'ses_1' } });
+    expect(String(output)).toContain('state: reconciled');
+  });
+
   test('does not mark cancelled when abort fails', async () => {
     const { board, abort, cancelTask } = createTool({
       abort: async () => {

+ 6 - 1
src/tools/cancel-task.ts

@@ -61,7 +61,12 @@ Use only for obsolete, wrong, conflicting, or user-requested cancellation. Accep
         ].join('\n');
       }
 
-      if (job.state !== 'running') {
+      const shouldAbort =
+        job.state === 'running' ||
+        job.state === 'cancelled' ||
+        (job.state === 'reconciled' && job.terminalState === 'cancelled');
+
+      if (!shouldAbort) {
         return [
           `task_id: ${job.taskID}`,
           `state: ${job.state}`,