Skip to content

fix(main): stop hidden conversation sessions#2198

Open
janburzinski wants to merge 4 commits into
mainfrom
emdash/kill-terminal-grace-gth2i
Open

fix(main): stop hidden conversation sessions#2198
janburzinski wants to merge 4 commits into
mainfrom
emdash/kill-terminal-grace-gth2i

Conversation

@janburzinski
Copy link
Copy Markdown
Collaborator

summary

  • hidden conversations now get stopped after grac eperiod (30s)
  • track visible conversations
  • prevents runaway background agents

@janburzinski janburzinski requested a review from jschwxrz May 22, 2026 20:05
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 22, 2026

Greptile Summary

This PR introduces a ConversationSessionVisibilityService that tracks which conversation PTY sessions are visible in the renderer and stops hidden ones after a 30 s grace period, preventing runaway background agents.

  • New service (conversation-session-visibility.ts): maintains a trackedTasks set and a visibleConversationsByTask map, schedules kill timers for hidden sessions by capturing stopSession at schedule time (not at fire time), and cancels timers when a conversation becomes visible again.
  • Renderer integration (workspace-view-model.tsx): adds a MobX reaction with comparer.structural equality that reports the set of active conversation IDs via IPC on every change, plus an explicit [] call on dispose() to immediately signal session teardown.
  • Provider hooks (local-conversation.ts, ssh-conversation.ts): call onConversationSessionStarted immediately after PTY registration so sessions that start while the task is already hidden are picked up.

Confidence Score: 5/5

Safe to merge; the grace-period logic is sound and the regression risks are well-covered by the new test suite.

All five scenarios flagged in the prior review thread are addressed: visibleConversationsByTask is cleaned up correctly, MobX structural equality prevents spurious IPC calls, stopSession is captured at schedule time so task deletion after scheduling cannot leak sessions, test timers are cleared in afterEach, and trackedTasks prevents onConversationSessionStarted from silently skipping tasks whose visible-set was last reported as empty. The one remaining gap — trackedTasks itself is never pruned — can cause a stale entry to schedule a brief, self-cancelling kill timer on workspace re-open, but the 30 s grace period means the IPC round-trip always wins in practice.

src/main/core/conversations/conversation-session-visibility.ts — the trackedTasks set has no corresponding cleanup path

Important Files Changed

Filename Overview
src/main/core/conversations/conversation-session-visibility.ts New service tracking visible conversations and scheduling kill timers for hidden PTY sessions; introduces trackedTasks set that is never cleaned up
src/main/core/conversations/conversation-session-visibility.test.ts New test file covering the five main scenarios including grace period, cancellation, deferred starts on empty/non-empty visibility sets, and task removal before timer fire
src/renderer/features/tasks/stores/workspace-view-model.tsx Adds MobX reaction to track visible conversation IDs and report them via IPC; uses comparer.structural to avoid spurious calls; adds explicit empty-list call on dispose
src/main/core/conversations/controller.ts Adds updateVisibleConversations RPC endpoint that delegates to the new visibility service
src/main/core/conversations/impl/local-conversation.ts Calls onConversationSessionStarted immediately after PTY session is registered, correctly ordering the registry write before the visibility check
src/main/core/conversations/impl/ssh-conversation.ts Mirror of local-conversation.ts change: hooks SSH sessions into the visibility service after PTY registration
src/renderer/features/tasks/stores/workspace-view-model.test.ts Adds updateVisibleConversations stub to the IPC mock so existing tests continue to compile and pass

Sequence Diagram

sequenceDiagram
    participant R as Renderer (WorkspaceViewModel)
    participant IPC as IPC / Controller
    participant VS as ConversationSessionVisibilityService
    participant PR as PtySessionRegistry
    participant Task as Task.conversations

    Note over R: Constructor — fireImmediately reaction
    R->>IPC: updateVisibleConversations(projectId, taskId, [conv-A])
    IPC->>VS: updateVisibleConversations(...)
    VS->>VS: trackedTasks.add(key)
    VS->>VS: "visibleConversationsByTask.set(key, {conv-A})"
    VS->>VS: cancel timer for conv-A

    Note over R: User navigates away — isActive = false
    R->>IPC: updateVisibleConversations(projectId, taskId, [])
    IPC->>VS: updateVisibleConversations(..., [])
    VS->>VS: trackedTasks.add(key)
    VS->>VS: visibleConversationsByTask.delete(key)
    VS->>PR: listActiveSessions()
    PR-->>VS: [conv-A session]
    VS->>Task: resolveTask — capture stopSession ref
    VS->>VS: "schedule(conv-A, timer=30s)"

    Note over VS: 29999 ms pass

    Note over R: User navigates back — isActive = true
    R->>IPC: updateVisibleConversations(..., [conv-A])
    IPC->>VS: updateVisibleConversations(..., [conv-A])
    VS->>VS: cancel timer for conv-A

    Note over VS: alternatively, timer fires at 30s
    VS->>PR: get(sessionId) — still active?
    PR-->>VS: PTY instance
    VS->>Task: stopSession(conv-A)
Loading
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
src/main/core/conversations/conversation-session-visibility.ts:9-13
`trackedTasks` is never pruned. `updateVisibleConversations` always adds the task key but nothing ever removes it, so the set grows by one entry per task ever opened for the lifetime of the process. The prior review thread called out the same pattern for `visibleConversationsByTask` (now fixed). A more consequential effect: if a workspace is disposed and immediately recreated for the same task ID — which can happen during task re-provisioning — `onConversationSessionStarted` will see `trackedTasks.has(key) === true` but `visibleConversationsByTask.get(key) === undefined`, so it will schedule a 30 s kill timer for every new session before the new workspace's `fireImmediately` reaction can cancel it. The timer races the IPC round-trip; in practice the reaction wins, but the intermediate state is incorrect. Adding a `removeTask` method (called from `dispose()` alongside the explicit `[]` IPC) would clean up both sets.

```suggestion
  private readonly killTimers = new Map<string, ReturnType<typeof setTimeout>>();
  private readonly visibleConversationsByTask = new Map<string, Set<string>>();
  private readonly trackedTasks = new Set<string>();

  removeTask(projectId: string, taskId: string) {
    const taskKey = this.taskKey(projectId, taskId);
    this.trackedTasks.delete(taskKey);
    this.visibleConversationsByTask.delete(taskKey);
  }

  updateVisibleConversations(projectId: string, taskId: string, visibleConversationIds: string[]) {
```

Reviews (3): Last reviewed commit: "fix: track empty conversation visibility" | Re-trigger Greptile

Comment thread src/main/core/conversations/conversation-session-visibility.ts Outdated
Comment thread src/renderer/features/tasks/stores/workspace-view-model.tsx Outdated
Comment thread src/main/core/conversations/conversation-session-visibility.ts
Comment thread src/main/core/conversations/conversation-session-visibility.test.ts
@janburzinski
Copy link
Copy Markdown
Collaborator Author

@greptileai

Comment thread src/main/core/conversations/conversation-session-visibility.ts
@janburzinski
Copy link
Copy Markdown
Collaborator Author

@greptileai

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant