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

tui: fix memory leaks in session management and improve permission error handling

Dax Raad 1 месяц назад
Родитель
Сommit
dad9c917d2

+ 1 - 1
packages/opencode/src/permission/next.ts

@@ -222,7 +222,7 @@ export namespace PermissionNext {
     const match = merged.findLast(
       (rule) => Wildcard.match(permission, rule.permission) && Wildcard.match(pattern, rule.pattern),
     )
-    return match ?? { action: "allow", permission, pattern: "*" }
+    return match ?? { action: "ask", permission, pattern: "*" }
   }
 
   const EDIT_TOOLS = ["edit", "write", "patch", "multiedit"]

+ 108 - 0
packages/opencode/src/session/MEMORY_LEAK_FIXES.md

@@ -0,0 +1,108 @@
+# Memory Leak Fixes Plan
+
+## Summary
+
+This document outlines the memory leak issues identified in the session module and the proposed fixes.
+
+## Issues Identified
+
+### Issue 1: Instance Dispose Callback Missing Callback Rejection (HIGH)
+
+**File**: `prompt.ts:69-73`
+
+**Problem**: When an instance is disposed, the dispose callback only aborts the AbortControllers but doesn't reject the pending promise callbacks. This leaves hanging promises that never resolve or reject.
+
+**Current Code**:
+```typescript
+async (current) => {
+  for (const item of Object.values(current)) {
+    item.abort.abort()
+  }
+},
+```
+
+**Fix**: Add callback rejection in the dispose handler:
+```typescript
+async (current) => {
+  for (const item of Object.values(current)) {
+    item.abort.abort()
+    for (const callback of item.callbacks) {
+      callback.reject()
+    }
+  }
+},
+```
+
+---
+
+### Issue 2: Abort Listener Not Removed on Timeout (MEDIUM)
+
+**File**: `retry.ts:10-22`
+
+**Problem**: If the timeout resolves before the abort signal fires, the abort event listener remains attached to the signal. While `{ once: true }` ensures it fires only once if aborted, it doesn't remove the listener if the timeout fires first. This causes a minor memory leak for long-lived signals.
+
+**Current Code**:
+```typescript
+export async function sleep(ms: number, signal: AbortSignal): Promise<void> {
+  return new Promise((resolve, reject) => {
+    const timeout = setTimeout(resolve, Math.min(ms, RETRY_MAX_DELAY))
+    signal.addEventListener(
+      "abort",
+      () => {
+        clearTimeout(timeout)
+        reject(new DOMException("Aborted", "AbortError"))
+      },
+      { once: true },
+    )
+  })
+}
+```
+
+**Fix**: Store the abort handler and remove it when timeout resolves:
+```typescript
+export async function sleep(ms: number, signal: AbortSignal): Promise<void> {
+  return new Promise((resolve, reject) => {
+    const abortHandler = () => {
+      clearTimeout(timeout)
+      reject(new DOMException("Aborted", "AbortError"))
+    }
+    const timeout = setTimeout(() => {
+      signal.removeEventListener("abort", abortHandler)
+      resolve()
+    }, Math.min(ms, RETRY_MAX_DELAY))
+    signal.addEventListener("abort", abortHandler, { once: true })
+  })
+}
+```
+
+---
+
+### Issue 3: Orphaned AbortControllers (LOW - Optional)
+
+**Files**: 
+- `summary.ts:102`, `summary.ts:143`
+- `prompt.ts:884-892`, `prompt.ts:945-953`
+
+**Problem**: New `AbortController()` instances are created inline and passed to functions, but the controllers are never stored or explicitly aborted. While this isn't a significant leak (GC handles them when streams complete), it's a code smell.
+
+**Example**:
+```typescript
+abort: new AbortController().signal,
+```
+
+**Recommendation**: Leave as-is. The overhead is minimal and the code is clearer. The streams complete naturally and the objects are garbage collected.
+
+---
+
+## Implementation Checklist
+
+- [ ] Fix Issue 1: Add callback rejection in `prompt.ts` dispose handler
+- [ ] Fix Issue 2: Clean up abort listener in `retry.ts` sleep function
+- [ ] (Optional) Issue 3: No action needed
+
+## Testing Notes
+
+After implementing fixes:
+1. Verify existing tests pass
+2. Manually test session cancellation during active processing
+3. Verify instance disposal properly cleans up all pending sessions

+ 3 - 0
packages/opencode/src/session/prompt.ts

@@ -69,6 +69,9 @@ export namespace SessionPrompt {
     async (current) => {
       for (const item of Object.values(current)) {
         item.abort.abort()
+        for (const callback of item.callbacks) {
+          callback.reject()
+        }
       }
     },
   )

+ 9 - 6
packages/opencode/src/session/retry.ts

@@ -9,15 +9,18 @@ export namespace SessionRetry {
 
   export async function sleep(ms: number, signal: AbortSignal): Promise<void> {
     return new Promise((resolve, reject) => {
-      const timeout = setTimeout(resolve, Math.min(ms, RETRY_MAX_DELAY))
-      signal.addEventListener(
-        "abort",
+      const abortHandler = () => {
+        clearTimeout(timeout)
+        reject(new DOMException("Aborted", "AbortError"))
+      }
+      const timeout = setTimeout(
         () => {
-          clearTimeout(timeout)
-          reject(new DOMException("Aborted", "AbortError"))
+          signal.removeEventListener("abort", abortHandler)
+          resolve()
         },
-        { once: true },
+        Math.min(ms, RETRY_MAX_DELAY),
       )
+      signal.addEventListener("abort", abortHandler, { once: true })
     })
   }
 

+ 2 - 2
packages/opencode/test/permission/next.test.ts

@@ -451,7 +451,7 @@ test("ask - throws RejectedError when action is deny", async () => {
           always: [],
           ruleset: [{ permission: "bash", pattern: "*", action: "deny" }],
         }),
-      ).rejects.toBeInstanceOf(PermissionNext.RejectedError)
+      ).rejects.toBeInstanceOf(PermissionNext.AutoRejectedError)
     },
   })
 })
@@ -628,7 +628,7 @@ test("ask - checks all patterns and stops on first deny", async () => {
             { permission: "bash", pattern: "rm *", action: "deny" },
           ],
         }),
-      ).rejects.toBeInstanceOf(PermissionNext.RejectedError)
+      ).rejects.toBeInstanceOf(PermissionNext.AutoRejectedError)
     },
   })
 })