Browse Source

Fix: Tmux Session Leak - Complete Session Lifecycle Management (#127)

* Fix: Tmux Session Leak - Complete Session Lifecycle Management

## Problem
When background tasks complete, tmux panes remained open and opencode attach
processes became orphaned, accumulating over time.

## Root Cause
Missing session lifecycle management:
1. No session.abort() on task completion
2. No session.abort() on cancellation
3. No session.deleted event handler
4. No graceful shutdown (Ctrl+C before kill)

## Solution
- Add session.abort() in completeTask() (single point of responsibility)
- Add handleSessionDeleted() for cleanup on session deletion
- Add onSessionDeleted() in TmuxSessionManager for pane cleanup
- Implement graceful shutdown: Ctrl+C before kill-pane
- Wire up event handlers in main dispatcher

## Code Quality
- Removed duplicate code in completeTask()
- Eliminated redundant session.abort() calls
- All 43 tests pass

## Documentation
- Updated AGENTS.md with session lifecycle section
- Updated docs/tmux-integration.md with troubleshooting

## Testing
- @explorer and @librarian tasks complete and close panes automatically
- Zero orphaned processes after task completion

Inspired by oh-my-opencode session management implementation.

* fix: Address Greptile review feedback

- Remove redundant resolver deletion in handleSessionDeleted()
- Add session tracking cleanup in completeTask() as fallback
- Prevents memory leak if session.deleted event doesn't fire

Fixes issues identified in latest Greptile review.
Riccardo Sallusti 2 months ago
parent
commit
27a11bd087

+ 140 - 0
AGENTS.md

@@ -87,9 +87,149 @@ oh-my-opencode-slim/
 4. Run `bun test` to verify tests pass
 5. Commit changes
 
+## Tmux Session Lifecycle Management
+
+When working with tmux integration, understanding the session lifecycle is crucial for preventing orphaned processes and ghost panes.
+
+### Session Lifecycle Flow
+
+```
+Task Launch:
+  session.create() → tmux pane spawned → task runs
+
+Task Completes Normally:
+  session.status (idle) → extract results → session.abort()
+  → session.deleted event → tmux pane closed
+
+Task Cancelled:
+  cancel() → session.abort() → session.deleted event
+  → tmux pane closed
+
+Session Deleted Externally:
+  session.deleted event → task cleanup → tmux pane closed
+```
+
+### Key Implementation Details
+
+**1. Graceful Shutdown (src/utils/tmux.ts)**
+```typescript
+// Always send Ctrl+C before killing pane
+spawn([tmux, "send-keys", "-t", paneId, "C-c"])
+await delay(250)
+spawn([tmux, "kill-pane", "-t", paneId])
+```
+
+**2. Session Abort Timing (src/background/background-manager.ts)**
+- Call `session.abort()` AFTER extracting task results
+- This ensures content is preserved before session termination
+- Triggers `session.deleted` event for cleanup
+
+**3. Event Handlers (src/index.ts)**
+Both handlers must be wired up:
+- `backgroundManager.handleSessionDeleted()` - cleans up task state
+- `tmuxSessionManager.onSessionDeleted()` - closes tmux pane
+
+### Testing Tmux Integration
+
+After making changes to session management:
+
+```bash
+# 1. Build the plugin
+bun run build
+
+# 2. Run from local fork (in ~/.config/opencode/opencode.jsonc):
+# "plugin": ["file:///path/to/oh-my-opencode-slim"]
+
+# 3. Launch test tasks
+@explorer count files in src/
+@librarian search for Bun documentation
+
+# 4. Verify no orphans
+ps aux | grep "opencode attach" | grep -v grep
+# Should return 0 processes after tasks complete
+```
+
+### Common Issues
+
+**Ghost panes remaining open:**
+- Check that `session.abort()` is called after result extraction
+- Verify `session.deleted` handler is wired in src/index.ts
+
+**Orphaned opencode attach processes:**
+- Ensure graceful shutdown sends Ctrl+C before kill-pane
+- Check that tmux pane closes before process termination
+
+## Pre-Push Code Review
+
+Before pushing changes to the repository, always run a code review to catch issues like:
+- Duplicate code
+- Redundant function calls
+- Race conditions
+- Logic errors
+
+### Using `/review` Command (Recommended)
+
+OpenCode has a built-in `/review` command that automatically performs comprehensive code reviews:
+
+```bash
+# Review uncommitted changes (default)
+/review
+
+# Review specific commit
+/review <commit-hash>
+
+# Review branch comparison
+/review <branch-name>
+
+# Review PR
+/review <pr-url-or-number>
+```
+
+**Why use `/review` instead of asking @oracle manually?**
+- Standardized review process with consistent focus areas (bugs, structure, performance)
+- Automatically handles git operations (diff, status, etc.)
+- Context-aware: reads full files and convention files (AGENTS.md, etc.)
+- Delegates to specialized @build subagent with proper permissions
+- Provides actionable, matter-of-fact feedback
+
+### Workflow Before Pushing
+
+1. **Make your changes**
+   ```bash
+   # ... edit files ...
+   ```
+
+2. **Stage changes**
+   ```bash
+   git add .
+   ```
+
+3. **Run code review**
+   ```
+   /review
+   ```
+
+4. **Address any issues found**
+
+5. **Run checks**
+   ```bash
+   bun run check:ci
+   bun test
+   ```
+
+6. **Commit and push**
+   ```bash
+   git commit -m "..."
+   git push origin <branch>
+   ```
+
+**Note:** The `/review` command found issues in our PR #127 (duplicate code, redundant abort calls) that neither linter nor tests caught. Always use it before pushing!
+
 ## Common Patterns
 
 - This is an OpenCode plugin - most functionality lives in `src/`
 - The CLI entry point is `src/cli/index.ts`
 - The main plugin export is `src/index.ts`
 - Skills are located in `src/skills/` (included in package publish)
+- Background task management is in `src/background/`
+- Tmux utilities are in `src/utils/tmux.ts`

+ 40 - 0
docs/tmux-integration.md

@@ -216,6 +216,46 @@ tmux switch -t project2
    tail -f ~/.config/opencode/logs/opencode.log
    ```
 
+### Ghost Panes and Orphaned Processes
+
+**Problem:** Tmux panes remain open after tasks complete, or `opencode attach` processes accumulate
+
+**This issue is fixed in the latest version.** The session lifecycle now properly closes panes and terminates processes.
+
+**To verify the fix is working:**
+```bash
+# After running some background tasks, check for orphans
+ps aux | grep "opencode attach" | grep -v grep
+# Should return no results
+
+# Check active tmux panes
+tmux list-panes
+# Should only show your main session pane(s)
+```
+
+**If you still see orphaned processes:**
+1. **Kill all orphaned processes:**
+   ```bash
+   pkill -f "opencode attach"
+   ```
+
+2. **Close all ghost panes:**
+   ```bash
+   # In tmux, close panes manually
+   tmux kill-pane -t <pane-id>
+   ```
+
+3. **Restart OpenCode** with the updated plugin
+
+**Technical Details:**
+The fix implements proper session lifecycle management:
+- `session.abort()` is called after task completion
+- Graceful shutdown with Ctrl+C before killing panes
+- Event handlers for `session.deleted` events
+- Automatic cleanup of tmux panes and processes
+
+See [AGENTS.md](../AGENTS.md) for implementation details.
+
 ### Port Conflicts
 
 **Problem:** "Port already in use" or agents not connecting

+ 1 - 0
src/background/background-manager.test.ts

@@ -37,6 +37,7 @@ function createMockContext(overrides?: {
           }
           return {};
         }),
+        abort: mock(async () => ({})),
       },
     },
     directory: '/test/directory',

+ 56 - 1
src/background/background-manager.ts

@@ -430,6 +430,51 @@ export class BackgroundTaskManager {
   }
 
   /**
+   * Handle session.deleted events for cleanup.
+   * When a session is deleted, cancel associated tasks and clean up.
+   */
+  async handleSessionDeleted(event: {
+    type: string;
+    properties?: { info?: { id?: string }; sessionID?: string };
+  }): Promise<void> {
+    if (event.type !== 'session.deleted') return;
+
+    const sessionId = event.properties?.info?.id ?? event.properties?.sessionID;
+    if (!sessionId) return;
+
+    const taskId = this.tasksBySessionId.get(sessionId);
+    if (!taskId) return;
+
+    const task = this.tasks.get(taskId);
+    if (!task) return;
+
+    // Only handle if task is still active
+    if (task.status === 'running' || task.status === 'pending') {
+      log(`[background-manager] Session deleted, cancelling task: ${task.id}`);
+
+      // Mark as cancelled
+      (task as BackgroundTask & { status: string }).status = 'cancelled';
+      task.completedAt = new Date();
+      task.error = 'Session deleted';
+
+      // Clean up session tracking
+      this.tasksBySessionId.delete(sessionId);
+      this.agentBySessionId.delete(sessionId);
+
+      // Resolve any waiting callers
+      const resolver = this.completionResolvers.get(taskId);
+      if (resolver) {
+        resolver(task);
+        this.completionResolvers.delete(taskId);
+      }
+
+      log(
+        `[background-manager] Task cancelled due to session deletion: ${task.id}`,
+      );
+    }
+  }
+
+  /**
    * Extract task result and mark complete.
    */
   private async extractAndCompleteTask(task: BackgroundTask): Promise<void> {
@@ -499,12 +544,22 @@ export class BackgroundTaskManager {
       task.error = resultOrError;
     }
 
-    // Clean up session tracking maps to prevent memory leak
+    // Clean up session tracking maps as fallback
+    // (handleSessionDeleted also does this when session.deleted event fires)
     if (task.sessionId) {
       this.tasksBySessionId.delete(task.sessionId);
       this.agentBySessionId.delete(task.sessionId);
     }
 
+    // Abort session to trigger pane cleanup and free resources
+    if (task.sessionId) {
+      this.client.session
+        .abort({
+          path: { id: task.sessionId },
+        })
+        .catch(() => {});
+    }
+
     // Send notification to parent session
     if (task.parentSessionId) {
       this.sendCompletionNotification(task).catch((err) => {

+ 18 - 0
src/background/tmux-session-manager.ts

@@ -141,6 +141,24 @@ export class TmuxSessionManager {
     }
   }
 
+  /**
+   * Handle session.deleted events.
+   * When a session is deleted, close its tmux pane immediately.
+   */
+  async onSessionDeleted(event: SessionEvent): Promise<void> {
+    if (!this.enabled) return;
+    if (event.type !== 'session.deleted') return;
+
+    const sessionId = event.properties?.sessionID;
+    if (!sessionId) return;
+
+    log('[tmux-session-manager] session deleted, closing pane', {
+      sessionId,
+    });
+
+    await this.closeSession(sessionId);
+  }
+
   private startPolling(): void {
     if (this.pollInterval) return;
 

+ 16 - 0
src/index.ts

@@ -179,6 +179,22 @@ const OhMyOpenCodeLite: Plugin = async (ctx) => {
           properties?: { sessionID?: string; status?: { type: string } };
         },
       );
+
+      // Handle session.deleted events for:
+      // 1. BackgroundTaskManager: task cleanup
+      // 2. TmuxSessionManager: pane cleanup
+      await backgroundManager.handleSessionDeleted(
+        input.event as {
+          type: string;
+          properties?: { info?: { id?: string }; sessionID?: string };
+        },
+      );
+      await tmuxSessionManager.onSessionDeleted(
+        input.event as {
+          type: string;
+          properties?: { sessionID?: string };
+        },
+      );
     },
 
     // Inject phase reminder before sending to API (doesn't show in UI)

+ 14 - 0
src/utils/tmux.ts

@@ -309,6 +309,20 @@ export async function closeTmuxPane(paneId: string): Promise<boolean> {
   }
 
   try {
+    // Send Ctrl+C for graceful shutdown first
+    log('[tmux] closeTmuxPane: sending Ctrl+C for graceful shutdown', {
+      paneId,
+    });
+    const ctrlCProc = spawn([tmux, 'send-keys', '-t', paneId, 'C-c'], {
+      stdout: 'pipe',
+      stderr: 'pipe',
+    });
+    await ctrlCProc.exited;
+
+    // Wait for graceful shutdown
+    await new Promise((r) => setTimeout(r, 250));
+
+    log('[tmux] closeTmuxPane: killing pane', { paneId });
     const proc = spawn([tmux, 'kill-pane', '-t', paneId], {
       stdout: 'pipe',
       stderr: 'pipe',