Browse Source

fix(smartfetch): retry session cleanup and log persistent failures

The secondary-model session delete used .catch(() => undefined), silently
swallowing all errors. When the delete failed (e.g. during an OpenCode
instance dispose/reload cycle), sessions were left orphaned in the database
and appeared as empty conversations in the UI.

Replace the silent catch with deleteSessionSafely: retries transient
failures (3 attempts, 500ms apart) and logs a warning if all retries fail,
so the issue is visible instead of silently leaking sessions.
dhaern 1 month ago
parent
commit
4e9a5dac26
2 changed files with 108 additions and 8 deletions
  1. 67 2
      src/tools/smartfetch/secondary-model.test.ts
  2. 41 6
      src/tools/smartfetch/secondary-model.ts

+ 67 - 2
src/tools/smartfetch/secondary-model.test.ts

@@ -7,9 +7,13 @@ type PromptStep = {
   error?: Error;
 };
 
-function createMockClient(steps: PromptStep[]) {
+function createMockClient(steps: PromptStep[], deleteBehavior?: {
+  failTimes?: number;
+}) {
   let createCount = 0;
   let promptCount = 0;
+  let deleteCallCount = 0;
+  const failTimes = deleteBehavior?.failTimes ?? 0;
 
   return {
     session: {
@@ -25,7 +29,14 @@ function createMockClient(steps: PromptStep[]) {
           },
         };
       }),
-      delete: mock(async () => ({})),
+      delete: mock(async () => {
+        deleteCallCount++;
+        if (deleteCallCount <= failTimes) {
+          throw new Error('delete failed');
+        }
+        return {};
+      }),
+      _deleteCallCount: () => deleteCallCount,
     },
     tool: {
       ids: mock(async () => ({ data: ['read', 'bash'] })),
@@ -82,4 +93,58 @@ describe('smartfetch/secondary-model', () => {
     expect(client.session.prompt).toHaveBeenCalledTimes(2);
     expect(client.session.delete).toHaveBeenCalledTimes(2);
   });
+
+  test('retries session delete on transient failure', async () => {
+    const originalWarn = console.warn;
+    const warnCalls: unknown[][] = [];
+    console.warn = (...args: unknown[]) => warnCalls.push(args);
+    try {
+      const client = createMockClient(
+        [{ text: 'Answer' }],
+        { failTimes: 1 },
+      );
+
+      const result = await runSecondaryModelWithFallback(
+        client,
+        '/tmp/project',
+        [models[0]],
+        'Summarize',
+        'This is enough fetched content to clear the short-content guard.',
+      );
+
+      expect(result.text).toBe('Answer');
+      // First attempt failed, second succeeded → 2 calls for one session
+      expect(client.session.delete).toHaveBeenCalledTimes(2);
+      expect(warnCalls.length).toBe(0);
+    } finally {
+      console.warn = originalWarn;
+    }
+  });
+
+  test('logs warning when all delete retries fail but does not throw', async () => {
+    const originalWarn = console.warn;
+    const warnCalls: unknown[][] = [];
+    console.warn = (...args: unknown[]) => warnCalls.push(args);
+    try {
+      const client = createMockClient(
+        [{ text: 'Answer' }],
+        { failTimes: 99 },
+      );
+
+      const result = await runSecondaryModelWithFallback(
+        client,
+        '/tmp/project',
+        [models[0]],
+        'Summarize',
+        'This is enough fetched content to clear the short-content guard.',
+      );
+
+      // Secondary model still succeeds despite cleanup failure
+      expect(result.text).toBe('Answer');
+      expect(warnCalls.length).toBe(1);
+      expect(String(warnCalls[0][0])).toContain('smartfetch');
+    } finally {
+      console.warn = originalWarn;
+    }
+  });
 });

+ 41 - 6
src/tools/smartfetch/secondary-model.ts

@@ -156,6 +156,46 @@ function isUsableSecondaryText(text: string) {
   return true;
 }
 
+const SESSION_DELETE_RETRIES = 3;
+const SESSION_DELETE_RETRY_DELAY_MS = 500;
+
+/**
+ * Delete a temporary secondary-model session with retry.
+ *
+ * The previous implementation swallowed all errors silently via
+ * `.catch(() => undefined)`, which left orphaned sessions in the database
+ * whenever the delete failed (e.g. during an OpenCode instance dispose/reload
+ * cycle). This retries transient failures and logs persistent ones so the
+ * issue is visible instead of silently leaking sessions.
+ */
+async function deleteSessionSafely(
+  client: OpenCodeClient,
+  sessionId: string,
+  directory: string,
+): Promise<void> {
+  for (let attempt = 1; attempt <= SESSION_DELETE_RETRIES; attempt++) {
+    try {
+      await client.session.delete({
+        path: { id: sessionId },
+        query: { directory },
+      });
+      return;
+    } catch (error) {
+      if (attempt >= SESSION_DELETE_RETRIES) {
+        console.warn(
+          `[smartfetch] Failed to clean up secondary session ${sessionId} ` +
+            `after ${SESSION_DELETE_RETRIES} attempts: ` +
+            (error instanceof Error ? error.message : String(error)),
+        );
+        return;
+      }
+      await new Promise((resolve) =>
+        setTimeout(resolve, SESSION_DELETE_RETRY_DELAY_MS),
+      );
+    }
+  }
+}
+
 async function runSecondaryModel(
   client: OpenCodeClient,
   directory: string,
@@ -235,12 +275,7 @@ async function runSecondaryModel(
       sourceChars,
     };
   } finally {
-    await client.session
-      .delete({
-        path: { id: sessionId },
-        query: { directory },
-      })
-      .catch(() => undefined);
+    await deleteSessionSafely(client, sessionId, directory);
   }
 }