Skip to content

ADE-91: Lane lifecycle operations crash active CLI sessions and chats#505

Merged
arul28 merged 2 commits into
mainfrom
ade-91-lane-lifecycle-operations-crash-active-cli-sessions-and-chats
Jun 2, 2026
Merged

ADE-91: Lane lifecycle operations crash active CLI sessions and chats#505
arul28 merged 2 commits into
mainfrom
ade-91-lane-lifecycle-operations-crash-active-cli-sessions-and-chats

Conversation

@arul28
Copy link
Copy Markdown
Owner

@arul28 arul28 commented Jun 1, 2026

Fixes ADE-91

Summary

Describe the change.

What Changed

Key files and behaviors.

Validation

How you tested.

Risks

Anything to watch.

Linked Linear issues

ADE   Open in ADE  ·  ade-91-lane-lifecycle-operations-crash-active-cli-sessions-and-chats branch  ·  PR #505

Summary by CodeRabbit

  • New Features

    • Lane deletion process now identifies and warns users about active chat sessions that will be lost
    • Chat sessions are automatically closed during lane deletion cleanup
  • Bug Fixes

    • Prevented duplicate background auto-create submit actions for chat drafts while lane naming is in progress

Greptile Summary

This PR closes ADE-91 by adding chat session lifecycle management to the lane deletion pipeline. When a lane is deleted, active chat sessions belonging to that lane (or routing execution through it) are now identified, warned about in the preflight UI, and closed during teardown.

  • Lane teardown extended: A new stop_chats step (with { fatal: false }) is injected between stop_processes and stop_ptys; claude_sessions and their associated session_linear_issues rows are also cleaned up in the DB teardown.
  • UI preflight updated: LaneDeleteRisk gains activeChatCount, surfaced with a ChatCircle icon in ManageLaneDialog and in the CLI formatLaneDeleteRisk formatter.
  • Duplicate submit guard: AgentChatPane gains a draftLaunchInFlightKeysRef set to suppress duplicate auto-create background submits while a lane-naming round-trip is still in flight.

Confidence Score: 4/5

Safe to merge with one fix: the error-accounting bug in disposeForLane causes misleading warning messages when a session is successfully force-closed after a failed graceful close.

The teardown pipeline and DB cleanup are well-structured and the non-fatal guard ensures deletion always completes. The one defect is in agentChatService.disposeForLane: when dispose() fails but forceDisposeManagedSession() succeeds, the original error is still pushed to errors and the function throws — reporting a failure for sessions that were actually cleaned up. When both paths fail, two strings are pushed for one session, so the count in the thrown message is off. Everything else in the PR — the stop_chats step ordering, the DB delete ordering, the duplicate-submit guard, and the preflight UI — looks correct.

apps/desktop/src/main/services/chat/agentChatService.ts — specifically the error-accumulation logic in disposeForLane

Important Files Changed

Filename Overview
apps/desktop/src/main/services/chat/agentChatService.ts Adds countActiveForLane, disposeForLane, managedSessionBelongsToLane, and forceDisposeManagedSession. The error-accounting in disposeForLane has a defect: the original dispose error is pushed to errors even when force-close succeeds, so the function throws and reports failure for sessions that were actually cleaned up.
apps/desktop/src/main/services/lanes/laneService.ts Adds stop_chats teardown step (correctly marked non-fatal), agentChatService to LaneDeleteTeardownDeps, activeChatCount to risk reporting, and DB cleanup for claude_sessions and session_linear_issues. Delete order is correct.
apps/desktop/src/renderer/components/chat/AgentChatPane.tsx Adds draftLaunchInFlightKeysRef dedup guard using a JSON-serialized snapshot key to prevent duplicate background auto-create submits; key is deleted in finally to allow retries after completion.
apps/ade-cli/src/bootstrap.ts Wires agentChatService into laneTeardownDeps using the established lazy-population pattern; the stop_chats step gracefully skips when the service is not yet populated.
apps/desktop/src/main/main.ts Desktop entry point registers agentChatService into laneTeardownDeps after service construction, mirroring the CLI bootstrap pattern.
apps/desktop/src/shared/types/lanes.ts Adds stop_chats to LaneDeleteStepName union and activeChatCount: number to LaneDeleteRisk; straightforward type extension with no issues.
apps/desktop/src/main/services/lanes/laneService.test.ts Adds agentChatService mock to teardown fixture, extends the happy-path ordering test, adds a non-fatal-failure test, and covers getDeleteRisk reporting activeChatCount.
apps/desktop/src/renderer/components/lanes/ManageLaneDialog.tsx Extends preflight panel to display active chat count with a ChatCircle icon; label copy and icon choice are consistent with existing process/PTY rows.

Sequence Diagram

sequenceDiagram
    participant UI as UI / CLI
    participant LS as laneService.delete()
    participant ACS as agentChatService
    participant DB as SQLite

    UI->>LS: getDeleteRisk(laneId)
    LS->>ACS: countActiveForLane(laneId)
    ACS-->>LS: activeChatCount
    LS-->>UI: "LaneDeleteRisk { activeChatCount, ... }"

    UI->>LS: delete(laneId)
    LS->>LS: runStep(cancel_auto_rebase)
    LS->>LS: runStep(stop_processes)
    LS->>ACS: "runStep(stop_chats) [fatal=false]"
    ACS->>ACS: disposeForLane(laneId)
    alt dispose succeeds
        ACS-->>LS: disposed count
    else dispose fails then forceDispose
        ACS->>ACS: forceDisposeManagedSession()
        ACS-->>LS: warning (non-fatal)
    end
    LS->>LS: runStep(stop_ptys)
    LS->>LS: runStep(stop_watchers)
    LS->>LS: runStep(git_worktree_remove)
    LS->>DB: DELETE claude_sessions WHERE lane_id
    LS->>DB: DELETE session_linear_issues (lane + sessions)
    LS->>DB: DELETE terminal_sessions WHERE lane_id
    LS->>DB: DELETE lanes WHERE id
    LS-->>UI: completed / completed_with_warnings
Loading

Fix All in Claude Code

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
apps/desktop/src/main/services/chat/agentChatService.ts:21882-21896
The original `dispose` error is unconditionally pushed to `errors` even when `forceDisposeManagedSession` succeeds. This means a session that was actually cleaned up via force-close is counted as a failure, so `disposeForLane` throws "Failed to close 1 chat session: …" for sessions that were in fact disposed. A second issue: when both `dispose` and `forceDispose` both fail, two error strings are pushed for a single session, causing the message to say "Failed to close 2 chat sessions" for one session. Since `stop_chats` runs with `{ fatal: false }`, the warning text surfaced to the user is the only observable output — getting it wrong undermines the diagnostic value.

```suggestion
      try {
        await dispose({ sessionId });
        disposed += 1;
      } catch (error) {
        const managed = managedSessions.get(sessionId);
        let forceClosed = false;
        if (managed) {
          try {
            forceDisposeManagedSession(managed, "Session force-closed during lane deletion.");
            disposed += 1;
            forceClosed = true;
          } catch (forceError) {
            errors.push(`${sessionId}: force close failed: ${forceError instanceof Error ? forceError.message : String(forceError)}`);
          }
        }
        if (!forceClosed) {
          errors.push(`${sessionId}: ${error instanceof Error ? error.message : String(error)}`);
        }
      }
```

Reviews (2): Last reviewed commit: "Address lane delete chat teardown review" | Re-trigger Greptile

@vercel
Copy link
Copy Markdown

vercel Bot commented Jun 1, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
ade Ignored Ignored Preview Jun 2, 2026 1:20am

@linear-code
Copy link
Copy Markdown

linear-code Bot commented Jun 1, 2026

ADE-91

@arul28
Copy link
Copy Markdown
Owner Author

arul28 commented Jun 1, 2026

@copilot review but do not make fixes

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 1, 2026

Review Change Stack

📝 Walkthrough

Walkthrough

This PR adds chat session teardown during lane deletion by introducing service-level counting and disposal helpers, integrating them into the lane-deletion workflow with a new progress step, extending UI components to report active chats, and preventing draft-launch duplicates. Changes span shared types, service implementations, dependency wiring, UI components, CLI display, and comprehensive test coverage.

Changes

Chat session teardown and deletion flow

Layer / File(s) Summary
Shared types for chat cleanup
apps/desktop/src/shared/types/lanes.ts
LaneDeleteStepName includes new "stop_chats" step, LaneDeleteRisk includes activeChatCount: number, and LaneDeleteTeardownDeps includes optional agentChatService with countActiveForLane() and disposeForLane() contract.
AgentChatService lane-scoped helpers
apps/desktop/src/main/services/chat/agentChatService.ts
New countActiveForLane() and disposeForLane() methods inspect and dispose chat sessions for a given lane, with error aggregation on disposal failure.
Lane service chat integration
apps/desktop/src/main/services/lanes/laneService.ts
Lane deletion now includes stop_chats step before PTY/watcher cleanup, extends database cleanup to remove claude_sessions and scoped session_linear_issues, and getDeleteRisk() computes activeChatCount.
Dependency injection wiring
apps/ade-cli/src/bootstrap.ts, apps/desktop/src/main/main.ts
Bootstrap creates laneTeardownDeps and populates it with process/PTY/watcher/chat teardown accessors; desktop main wires the created agentChatService into teardown deps.
Lane deletion UI: preflight and progress labels
apps/desktop/src/renderer/components/lanes/ManageLaneDialog.tsx, apps/desktop/src/renderer/components/lanes/LanesPage.tsx
Dialog preflight displays active chat count in "Will stop" list with ChatCircle icon and plural-aware session label; delete-progress step labels include stop_chats mapping.
CLI/TUI risk summary reporting
apps/ade-cli/src/tuiClient/app.tsx, apps/ade-cli/src/tuiClient/__tests__/appInput.test.ts
CLI formatLaneDeleteRisk displays active chat count in warning with correct singular/plural phrasing.
Lane service test coverage for chat cleanup
apps/desktop/src/main/services/lanes/laneService.test.ts
Tests scaffold fake agentChatService, verify stop_chats ordering before destructive git operations, assert database cleanup, validate failure handling, and check activeChatCount in risk calculation.
Dialog and browser-mock test updates
apps/desktop/src/renderer/browserMock.ts, apps/desktop/src/renderer/components/lanes/ManageLaneDialog.test.tsx
Browser mock includes activeChatCount in getDeleteRisk return shape; dialog test adds async coverage for preflight display of active chat sessions.

Draft launch duplicate prevention

Layer / File(s) Summary
AgentChatPane: in-flight draft-launch deduplication
apps/desktop/src/renderer/components/chat/AgentChatPane.tsx, apps/desktop/src/renderer/components/chat/AgentChatPane.test.tsx
Computes deterministic request key from launch parameters and tracks in-flight keys to suppress duplicate concurrent auto-create submissions, with test ensuring only one lane-name suggestion occurs for back-to-back clicks.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

  • arul28/ADE#501: Directly implements lane-scoped chat session teardown and duplicate draft-launch prevention mentioned in the issue scope.

Possibly related PRs

  • arul28/ADE#384: Both PRs modify lane deletion orchestration in laneService.ts—this PR adds stop_chats and chat teardown wiring, while PR #384 restructures delete control flow and git-worktree-remove wrapping.
  • arul28/ADE#380: Both PRs touch ManageLaneDialog.tsx around lane-delete risk handling—this adds chat-count rendering from getDeleteRisk, while PR #380 changes how risk is fetched/gated.
  • arul28/ADE#290: Both PRs extend lane-deletion machinery; this PR adds stop_chats/chat risk reporting using the same teardown/warning framework as PR #290.

Suggested labels

desktop

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 9.09% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: adding lane lifecycle teardown support for active CLI sessions and chats to prevent crashes during lane operations.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ade-91-lane-lifecycle-operations-crash-active-cli-sessions-and-chats

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@arul28 arul28 force-pushed the ade-91-lane-lifecycle-operations-crash-active-cli-sessions-and-chats branch from 7a7d68d to d30291f Compare June 1, 2026 23:44
Comment thread apps/desktop/src/main/services/lanes/laneService.ts Outdated
Comment thread apps/desktop/src/main/services/chat/agentChatService.ts
@arul28 arul28 merged commit 7ac4353 into main Jun 2, 2026
27 checks passed
@arul28 arul28 deleted the ade-91-lane-lifecycle-operations-crash-active-cli-sessions-and-chats branch June 2, 2026 03:42
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