Просмотр исходного кода

refactor: simplify multiplexer lifecycle guards

dhaern 3 месяцев назад
Родитель
Сommit
01a7d7f52b
2 измененных файлов с 49 добавлено и 198 удалено
  1. 5 154
      src/multiplexer/session-manager.test.ts
  2. 44 44
      src/multiplexer/session-manager.ts

+ 5 - 154
src/multiplexer/session-manager.test.ts

@@ -365,8 +365,11 @@ describe('MultiplexerSessionManager', () => {
 
       expect(mockMultiplexer.closePane).toHaveBeenCalledTimes(1);
       expect(mockMultiplexer.spawnPane).toHaveBeenCalledTimes(2);
-      expect((manager as any).sessions.get('child-close-race')?.paneId).toBe(
-        'p-close-race-resumed',
+      expect(mockMultiplexer.spawnPane).toHaveBeenLastCalledWith(
+        'child-close-race',
+        'Worker',
+        `http://localhost:${process.env.OPENCODE_PORT ?? '4096'}/`,
+        '/test/directory',
       );
     });
 
@@ -434,58 +437,6 @@ describe('MultiplexerSessionManager', () => {
       expect(mockMultiplexer.spawnPane).toHaveBeenCalledTimes(1);
     });
 
-    test('deduplicates concurrent close requests for the same pane', async () => {
-      const ctx = createMockContext();
-      const manager = new MultiplexerSessionManager(
-        ctx,
-        defaultMultiplexerConfig,
-      );
-      const closeDeferred = createDeferred<boolean>();
-
-      mockMultiplexer.spawnPane.mockResolvedValueOnce({
-        success: true,
-        paneId: 'p-dedupe',
-      });
-      mockMultiplexer.closePane.mockImplementationOnce(
-        () => closeDeferred.promise,
-      );
-
-      await manager.onSessionCreated({
-        type: 'session.created',
-        properties: {
-          info: {
-            id: 'child-dedupe',
-            parentID: 'parent-dedupe',
-          },
-        },
-      });
-
-      const idleClose = manager.onSessionStatus({
-        type: 'session.status',
-        properties: {
-          sessionID: 'child-dedupe',
-          status: { type: 'idle' },
-        },
-      });
-
-      const deletedClose = manager.onSessionDeleted({
-        type: 'session.deleted',
-        properties: {
-          sessionID: 'child-dedupe',
-        },
-      });
-
-      await Promise.resolve();
-
-      expect(mockMultiplexer.closePane).toHaveBeenCalledTimes(1);
-
-      closeDeferred.resolve(true);
-      await Promise.all([idleClose, deletedClose]);
-
-      expect(mockMultiplexer.closePane).toHaveBeenCalledWith('p-dedupe');
-      expect(mockMultiplexer.closePane).toHaveBeenCalledTimes(1);
-    });
-
     test('closes pane on session.deleted using info.id', async () => {
       const ctx = createMockContext();
       const manager = new MultiplexerSessionManager(
@@ -563,55 +514,6 @@ describe('MultiplexerSessionManager', () => {
       await createPromise;
 
       expect(mockMultiplexer.closePane).toHaveBeenCalledWith('p-stale-spawn');
-      expect((manager as any).sessions.has('child-stale-spawn')).toBe(false);
-    });
-
-    test('does not let duplicate created event reopen a deleted pending spawn', async () => {
-      const ctx = createMockContext();
-      const manager = new MultiplexerSessionManager(
-        ctx,
-        defaultMultiplexerConfig,
-      );
-      const spawnDeferred = createDeferred<{ success: true; paneId: string }>();
-
-      mockMultiplexer.spawnPane.mockImplementationOnce(
-        () => spawnDeferred.promise,
-      );
-
-      const createEvent = {
-        type: 'session.created',
-        properties: {
-          info: {
-            id: 'child-duplicate-stale',
-            parentID: 'parent-duplicate-stale',
-          },
-        },
-      };
-
-      const firstCreate = manager.onSessionCreated(createEvent);
-
-      await Promise.resolve();
-
-      await manager.onSessionDeleted({
-        type: 'session.deleted',
-        properties: {
-          info: { id: 'child-duplicate-stale' },
-        },
-      });
-
-      await manager.onSessionCreated(createEvent);
-
-      expect(mockMultiplexer.spawnPane).toHaveBeenCalledTimes(1);
-
-      spawnDeferred.resolve({ success: true, paneId: 'p-duplicate-stale' });
-      await firstCreate;
-
-      expect(mockMultiplexer.closePane).toHaveBeenCalledWith(
-        'p-duplicate-stale',
-      );
-      expect((manager as any).sessions.has('child-duplicate-stale')).toBe(
-        false,
-      );
     });
 
     test('does nothing on busy for unknown session', async () => {
@@ -632,57 +534,6 @@ describe('MultiplexerSessionManager', () => {
       expect(mockMultiplexer.spawnPane).not.toHaveBeenCalled();
     });
 
-    test('re-checks tracked sessions after async respawn guard', async () => {
-      const ctx = createMockContext();
-      const manager = new MultiplexerSessionManager(
-        ctx,
-        defaultMultiplexerConfig,
-      );
-
-      mockMultiplexer.spawnPane
-        .mockResolvedValueOnce({ success: true, paneId: 'p-1' })
-        .mockResolvedValueOnce({
-          success: true,
-          paneId: 'p-should-not-happen',
-        });
-
-      await manager.onSessionCreated({
-        type: 'session.created',
-        properties: {
-          info: {
-            id: 'child-999',
-            parentID: 'parent-999',
-            title: 'Worker',
-            directory: '/task/dir',
-          },
-        },
-      });
-
-      ctx.client.session.status.mockResolvedValue({
-        data: { 'child-999': { type: 'idle' } },
-      });
-      await (manager as any).pollSessions();
-
-      const respawnPromise = (manager as any).respawnIfKnown('child-999');
-
-      (manager as any).sessions.set('child-999', {
-        sessionId: 'child-999',
-        paneId: 'p-existing',
-        parentId: 'parent-999',
-        title: 'Worker',
-        directory: '/task/dir',
-        createdAt: Date.now(),
-        lastSeenAt: Date.now(),
-      });
-
-      await respawnPromise;
-
-      expect(mockMultiplexer.spawnPane).toHaveBeenCalledTimes(1);
-      expect((manager as any).sessions.get('child-999')?.paneId).toBe(
-        'p-existing',
-      );
-    });
-
     test('does not respawn while initial pane spawn is still in progress', async () => {
       const ctx = createMockContext();
       const manager = new MultiplexerSessionManager(

+ 44 - 44
src/multiplexer/session-manager.ts

@@ -61,7 +61,6 @@ export class MultiplexerSessionManager {
   private knownSessions = new Map<string, KnownSession>();
   private spawningSessions = new Set<string>();
   private closingSessions = new Map<string, Promise<void>>();
-  private deletedSessions = new Set<string>();
   private pollInterval?: ReturnType<typeof setInterval>;
   private enabled = false;
 
@@ -99,23 +98,24 @@ export class MultiplexerSessionManager {
     const title = info.title ?? 'Subagent';
     const directory = info.directory ?? this.directory;
 
-    if (this.deletedSessions.has(sessionId)) {
+    if (this.isTrackedOrSpawning(sessionId)) {
+      log('[multiplexer-session-manager] session already tracked or spawning', {
+        sessionId,
+      });
       return;
     }
 
+    const closing = this.closingSessions.get(sessionId);
+    if (closing) await closing;
+
+    if (this.isTrackedOrSpawning(sessionId)) return;
+
     this.knownSessions.set(sessionId, {
       parentId,
       title,
       directory,
     });
 
-    if (this.isTrackedOrSpawning(sessionId)) {
-      log('[multiplexer-session-manager] session already tracked or spawning', {
-        sessionId,
-      });
-      return;
-    }
-
     this.spawningSessions.add(sessionId);
 
     try {
@@ -127,7 +127,7 @@ export class MultiplexerSessionManager {
         return;
       }
 
-      if (this.isDeletedOrClosing(sessionId) || this.sessions.has(sessionId)) {
+      if (this.closingSessions.has(sessionId) || this.sessions.has(sessionId)) {
         return;
       }
 
@@ -151,13 +151,19 @@ export class MultiplexerSessionManager {
 
       if (!paneResult.success || !paneResult.paneId) return;
 
-      if (this.isDeletedOrClosing(sessionId)) {
+      if (
+        !this.knownSessions.has(sessionId) ||
+        this.closingSessions.has(sessionId)
+      ) {
         await this.multiplexer.closePane(paneResult.paneId).catch((err) =>
-          log('[multiplexer-session-manager] closing stale spawned pane failed', {
-            sessionId,
-            paneId: paneResult.paneId,
-            error: String(err),
-          }),
+          log(
+            '[multiplexer-session-manager] closing stale spawned pane failed',
+            {
+              sessionId,
+              paneId: paneResult.paneId,
+              error: String(err),
+            },
+          ),
         );
         return;
       }
@@ -287,7 +293,7 @@ export class MultiplexerSessionManager {
     reason: CloseReason,
   ): Promise<void> {
     if (reason === 'deleted') {
-      this.markSessionDeleted(sessionId);
+      this.knownSessions.delete(sessionId);
     }
 
     const existingClose = this.closingSessions.get(sessionId);
@@ -317,30 +323,17 @@ export class MultiplexerSessionManager {
       )
       .finally(() => {
         this.closingSessions.delete(sessionId);
-
-        if (this.sessions.size === 0) {
-          this.stopPolling();
-        }
+        this.updatePolling();
       });
 
     this.closingSessions.set(sessionId, closePromise);
     await closePromise;
   }
 
-  private markSessionDeleted(sessionId: string): void {
-    this.deletedSessions.add(sessionId);
-    this.knownSessions.delete(sessionId);
-  }
-
   private async respawnIfKnown(sessionId: string): Promise<void> {
     if (!this.enabled || !this.multiplexer) return;
-    if (this.deletedSessions.has(sessionId)) return;
-
     const closing = this.closingSessions.get(sessionId);
-    if (closing) {
-      await closing;
-      if (this.deletedSessions.has(sessionId)) return;
-    }
+    if (closing) await closing;
 
     if (this.isTrackedOrSpawning(sessionId)) {
       return;
@@ -364,7 +357,7 @@ export class MultiplexerSessionManager {
         return;
       }
 
-      if (this.sessions.has(sessionId) || this.isDeletedOrClosing(sessionId)) {
+      if (this.sessions.has(sessionId) || this.closingSessions.has(sessionId)) {
         return;
       }
 
@@ -388,13 +381,19 @@ export class MultiplexerSessionManager {
 
       if (!paneResult.success || !paneResult.paneId) return;
 
-      if (this.isDeletedOrClosing(sessionId)) {
+      if (
+        !this.knownSessions.has(sessionId) ||
+        this.closingSessions.has(sessionId)
+      ) {
         await this.multiplexer.closePane(paneResult.paneId).catch((err) =>
-          log('[multiplexer-session-manager] closing stale respawned pane failed', {
-            sessionId,
-            paneId: paneResult.paneId,
-            error: String(err),
-          }),
+          log(
+            '[multiplexer-session-manager] closing stale respawned pane failed',
+            {
+              sessionId,
+              paneId: paneResult.paneId,
+              error: String(err),
+            },
+          ),
         );
         return;
       }
@@ -425,10 +424,12 @@ export class MultiplexerSessionManager {
     return this.sessions.has(sessionId) || this.spawningSessions.has(sessionId);
   }
 
-  private isDeletedOrClosing(sessionId: string): boolean {
-    return (
-      this.deletedSessions.has(sessionId) || this.closingSessions.has(sessionId)
-    );
+  private updatePolling(): void {
+    if (this.sessions.size > 0 || this.closingSessions.size > 0) {
+      this.startPolling();
+    } else {
+      this.stopPolling();
+    }
   }
 
   private getSessionId(event: SessionEvent): string | undefined {
@@ -461,7 +462,6 @@ export class MultiplexerSessionManager {
 
     this.knownSessions.clear();
     this.spawningSessions.clear();
-    this.deletedSessions.clear();
     this.closingSessions.clear();
 
     log('[multiplexer-session-manager] cleanup complete');