Merge pull request #557 from dhaern/fix/smartfetch-session-cleanup

fix(smartfetch): retry session cleanup to prevent orphaned conversations

src/tools/smartfetch/secondary-model.test.ts

@@ -1,5 +1,5 @@
 import { afterEach, describe, expect, mock, test } from 'bun:test';
-import { runSecondaryModelWithFallback } from './secondary-model';
+import { runSecondaryModelWithFallback, _testConfig } from './secondary-model';
 import type { SecondaryModel } from './types';
 
 type PromptStep = {
@@ -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,13 @@ function createMockClient(steps: PromptStep[]) {
           },
         };
       }),
-      delete: mock(async () => ({})),
+      delete: mock(async () => {
+        deleteCallCount++;
+        if (deleteCallCount <= failTimes) {
+          throw new Error('delete failed');
+        }
+        return {};
+      }),
     },
     tool: {
       ids: mock(async () => ({ data: ['read', 'bash'] })),
@@ -82,4 +92,98 @@ 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);
+    const originalDelay = _testConfig.deleteRetryDelayMs;
+    _testConfig.deleteRetryDelayMs = 0;
+    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;
+      _testConfig.deleteRetryDelayMs = originalDelay;
+    }
+  });
+
+  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);
+    const originalDelay = _testConfig.deleteRetryDelayMs;
+    _testConfig.deleteRetryDelayMs = 0;
+    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;
+      _testConfig.deleteRetryDelayMs = originalDelay;
+    }
+  });
+
+  test('falls back to next model when prompt times out', async () => {
+    const client = {
+      session: {
+        create: mock(async () => ({ id: 'session-timeout' })),
+        prompt: mock(async (opts: any) => {
+          const model = opts.body.model;
+          if (model.modelID === 'small') {
+            throw new Error('Secondary model timed out');
+          }
+          return {
+            data: {
+              parts: [{ type: 'text', text: 'Fallback answer' }],
+            },
+          };
+        }),
+        delete: mock(async () => ({})),
+      },
+      tool: {
+        ids: mock(async () => ({ data: ['read'] })),
+      },
+    } as any;
+
+    const result = await runSecondaryModelWithFallback(
+      client,
+      '/tmp/project',
+      models,
+      'Summarize',
+      'This is enough fetched content to clear the short-content guard.',
+    );
+
+    expect(result.text).toBe('Fallback answer');
+    expect(result.model).toEqual(models[1]);
+  });
 });

src/tools/smartfetch/secondary-model.ts

@@ -156,6 +156,55 @@ function isUsableSecondaryText(text: string) {
   return true;
 }
 
+const SESSION_DELETE_RETRIES = 3;
+const SESSION_DELETE_RETRY_DELAY_MS = 500;
+const SECONDARY_MODEL_TIMEOUT_MS = 30_000;
+
+/**
+ * Exposed for tests so they can avoid real wall-clock sleeps.
+ * Not part of the public API.
+ */
+export const _testConfig = {
+  deleteRetryDelayMs: SESSION_DELETE_RETRY_DELAY_MS,
+};
+
+/**
+ * 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, _testConfig.deleteRetryDelayMs),
+      );
+    }
+  }
+}
+
 async function runSecondaryModel(
   client: OpenCodeClient,
   directory: string,
@@ -199,24 +248,32 @@ async function runSecondaryModel(
       (toolIDs || []).map((id: string) => [id, false]),
     );
 
-    const result = await client.session.prompt({
-      responseStyle: 'data',
-      throwOnError: true,
-      path: { id: sessionId },
-      query: { directory },
-      body: {
-        model,
-        system:
-          'Answer only from the supplied content. Do not use tools or outside knowledge.',
-        tools: disabledTools,
-        parts: [
-          {
-            type: 'text',
-            text: buildPrompt(truncatedContent, effectivePrompt),
-          },
-        ],
-      },
-    });
+    const result = await Promise.race([
+      client.session.prompt({
+        responseStyle: 'data',
+        throwOnError: true,
+        path: { id: sessionId },
+        query: { directory },
+        body: {
+          model,
+          system:
+            'Answer only from the supplied content. Do not use tools or outside knowledge.',
+          tools: disabledTools,
+          parts: [
+            {
+              type: 'text',
+              text: buildPrompt(truncatedContent, effectivePrompt),
+            },
+          ],
+        },
+      }),
+      new Promise<never>((_, reject) =>
+        setTimeout(
+          () => reject(new Error('Secondary model timed out')),
+          SECONDARY_MODEL_TIMEOUT_MS,
+        ),
+      ),
+    ]);
 
     const parts =
       (result as { data?: { parts?: Array<{ type?: string; text?: string }> } })
@@ -235,12 +292,7 @@ async function runSecondaryModel(
       sourceChars,
     };
   } finally {
-    await client.session
-      .delete({
-        path: { id: sessionId },
-        query: { directory },
-      })
-      .catch(() => undefined);
+    await deleteSessionSafely(client, sessionId, directory);
   }
 }