Kill sessions by alvinunreal · Pull Request #54 · alvinunreal/oh-my-opencode-slim

alvinunreal · 2026-01-20T19:47:25Z

Related #50

POC implementation, need to be redone; has bugs, kills sometimes own session; tmux broke?

greptile-apps · 2026-01-20T19:50:12Z

Greptile Summary

Implemented automatic cleanup of idle child sessions to prevent history pollution. Added a global "reaper" that monitors session.idle events and automatically deletes finished child sessions after a 2-second delay, giving parent tools time to read results first.

Key changes:

Created shared deleteSession utility with multiple fallback strategies (SDK v1, SDK v2, raw fetch)
Extended TmuxSessionManager to handle both session.created and session.deleted events for proper tmux pane lifecycle management
Added background_cleanup tool for manual session cleanup
Sync tasks now automatically delete their sessions after completion
Global reaper in event handler auto-cleans idle child sessions with toast notifications

The implementation handles empty responses gracefully and ensures tmux panes are properly closed when sessions are deleted.

Confidence Score: 3/5

Functional but has a potential issue with async cleanup timing
The session cleanup logic is sound and well-structured with multiple fallback strategies. However, the setTimeout in the global reaper creates a fire-and-forget async operation that may not complete if the event handler exits. The selectSession type cast to any also needs verification.
Pay close attention to src/index.ts - the async cleanup in the event handler needs proper error handling

Important Files Changed

Filename	Overview
src/utils/session-utils.ts	New shared utility for deleting sessions with multiple fallback strategies
src/tools/background.ts	Added `background_cleanup` tool and automatic session deletion after sync task completion
src/index.ts	Added global reaper for cleaning up idle child sessions with 2-second delay

Sequence Diagram

sequenceDiagram
    participant User
    participant Plugin
    participant BackgroundManager
    participant TmuxSessionManager
    participant GlobalReaper
    participant SessionAPI
    participant TUI

    User->>Plugin: Launch background task
    Plugin->>SessionAPI: Create child session
    SessionAPI-->>Plugin: session.created event
    Plugin->>TmuxSessionManager: onSessionEvent(session.created)
    TmuxSessionManager->>TmuxSessionManager: Spawn tmux pane
    Plugin->>BackgroundManager: Launch task in session
    BackgroundManager->>SessionAPI: Send prompt to session
    
    Note over BackgroundManager: Task executes...
    
    BackgroundManager->>BackgroundManager: Poll for completion
    BackgroundManager->>SessionAPI: Check session status
    SessionAPI-->>BackgroundManager: Status: idle
    BackgroundManager->>BackgroundManager: Mark task completed
    
    SessionAPI-->>Plugin: session.idle event
    Plugin->>GlobalReaper: Detect idle child session
    GlobalReaper->>GlobalReaper: Wait 2 seconds
    GlobalReaper->>SessionAPI: Get session details
    SessionAPI-->>GlobalReaper: Session has parentID
    GlobalReaper->>SessionAPI: Delete session (via deleteSession utility)
    GlobalReaper->>TUI: Show toast notification
    GlobalReaper->>TUI: Re-select parent session
    
    SessionAPI-->>Plugin: session.deleted event
    Plugin->>TmuxSessionManager: onSessionEvent(session.deleted)
    TmuxSessionManager->>TmuxSessionManager: Close tmux pane

greptile-apps

_{7 files reviewed, 3 comments}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2026-01-20T19:50:10Z

+        const sessionID = event.properties.sessionID;
+
+        // Wait a bit to ensure the parent tool has time to read the result
+        setTimeout(async () => {


logic: setTimeout creates async cleanup that won't be awaited. If the event handler completes before the timeout fires, there's no guarantee the cleanup will execute.

Suggested change

setTimeout(async () => {

// Use async function but don't await (let it run in background)

(async () => {

await new Promise(resolve => setTimeout(resolve, 2000));

Prompt To Fix With AI

This is a comment left during a code review. Path: src/index.ts Line: 126:126 Comment: **logic:** `setTimeout` creates async cleanup that won't be awaited. If the event handler completes before the timeout fires, there's no guarantee the cleanup will execute. ```suggestion // Use async function but don't await (let it run in background) (async () => { await new Promise(resolve => setTimeout(resolve, 2000)); ``` How can I resolve this? If you propose a fix, please make it concise.

greptile-apps · 2026-01-20T19:50:11Z

+            // Session might already be gone or other error
+            log(`[reaper] error during cleanup`, { sessionID, error: String(err) });
+          }
+        }, 2000); 


syntax: The closing })(); for the async IIFE is missing.

Suggested change

}, 2000);

}, 2000);

})();

Prompt To Fix With AI

This is a comment left during a code review. Path: src/index.ts Line: 155:155 Comment: **syntax:** The closing `})();` for the async IIFE is missing. ```suggestion }, 2000); })(); ``` How can I resolve this? If you propose a fix, please make it concise.

greptile-apps · 2026-01-20T19:50:11Z

+              }).catch(() => {});
+
+              // 3. Force TUI to refresh its session view/navigation by re-selecting parent
+              await (ctx.client.tui as any).selectSession({ body: { sessionID: parentID } }).catch(() => {});


style: Check that selectSession method exists in the TUI client API to avoid runtime errors.

Prompt To Fix With AI

This is a comment left during a code review. Path: src/index.ts Line: 149:149 Comment: **style:** Check that `selectSession` method exists in the TUI client API to avoid runtime errors. How can I resolve this? If you propose a fix, please make it concise.

alvinreal

Reviewed the diff for PR #54 (Kill sessions).

Scope: This PR modifies background-manager.ts and tmux-session-manager.ts to handle session cleanup. Changes: (1) always mark tasks completed even with empty response, (2) refactors session event handling into unified onSessionEvent handler, (3) fixes URL origin handling.

Logic issues:

Background manager now marks task completed even on empty — this could mark tasks complete prematurely if the agent simply hasn't responded yet. The original guard was intentional to prevent false completions.
The self-kill bug the author mentioned (kills own session) is likely in the unified — there's no longer a check for before marking a session as tracked. If a parent session fires , it could incorrectly try to close its own tmux pane.

Tests: No test files modified or added. CI checks have not run on this branch.

Status: PR is marked CONFLICTING (needs rebase). Author noted POC with bugs. Recommend closing and reopening as a draft when ready.

Decision: Request changes. Please address the self-kill bug and the premature-completion logic before re-requesting review.

alvinreal

Reviewed the diff for PR #54 (Kill sessions).

Scope: This PR modifies background-manager.ts and tmux-session-manager.ts to handle session cleanup. Changes: (1) always mark tasks completed even with empty response, (2) refactors session event handling into unified onSessionEvent handler, (3) fixes URL origin handling.

Logic issues:

Background manager now marks task completed even on empty responseText - this could mark tasks complete prematurely if the agent has not responded yet. The original guard if (responseText) was intentional to prevent false completions.
The self-kill bug the author mentioned (kills own session) is likely in the unified onSessionEvent - there is no longer a check for parentID before marking a session as tracked. If a parent session fires session.deleted, it could incorrectly try to close its own tmux pane.

Tests: No test files modified or added. CI checks have not run on this branch.

Status: PR is marked CONFLICTING (needs rebase). Author noted POC with bugs. Recommend closing and reopening as a draft when ready.

Decision: Request changes. Please address the self-kill bug and the premature-completion logic before re-requesting review.

alvinunreal added 2 commits January 20, 2026 20:16

Kill agent sessions when done

e790d65

review

90475d1

greptile-apps bot reviewed Jan 20, 2026

View reviewed changes

idk

410e1b7

alvinunreal force-pushed the master branch from a3a88a2 to 21738c4 Compare January 24, 2026 19:31

alvinreal requested changes Mar 25, 2026

View reviewed changes

alvinunreal closed this Mar 25, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kill sessions#54

Kill sessions#54
alvinunreal wants to merge 3 commits intomasterfrom
kill-session

alvinunreal commented Jan 20, 2026 •

edited

Loading

Uh oh!

greptile-apps bot commented Jan 20, 2026

Uh oh!

greptile-apps bot left a comment

Uh oh!

greptile-apps bot Jan 20, 2026

Uh oh!

greptile-apps bot Jan 20, 2026

Uh oh!

greptile-apps bot Jan 20, 2026

Uh oh!

alvinreal left a comment

Uh oh!

alvinreal left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

alvinunreal commented Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

greptile-apps bot commented Jan 20, 2026

Greptile Summary

Confidence Score: 3/5

Important Files Changed

Sequence Diagram

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

alvinreal left a comment

Choose a reason for hiding this comment

Uh oh!

alvinreal left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

alvinunreal commented Jan 20, 2026 •

edited

Loading