[LEN5010]

Issue

Fixes #14772

Follows up on the restart cleanup finding in #12812.

Context

A restarted PTY can finish creating after its terminal closes or its panel reloads. The restart then updates a detached entry, leaving the replacement outside cleanup bookkeeping. Setup failures can leave the same gap.

Implementation

Keep PTY ownership until removal succeeds, and invalidate the entry revision when teardown begins. Check identity and revision after asynchronous setup and cleanup so an obsolete restart cannot return a stale URL or replace a newer entry.

Close/dispose clean up known PTYs without waiting for pending create/resize requests. Late allocations are removed when their requests settle. The router retains retired managers until their PTYs and pending requests are settled, so later disposal and directory cleanup can retry failed removals after a panel reload. Directory cleanup includes detached ownership and stays scoped to the target directory. Only a matching typed PtyNotFoundError counts as already removed. Ordinary restart and concurrent-restart deduplication are preserved.

Screenshots / Video

N/A: no visual changes.

How to Test

Manual/local verification

Agent-executed with Bun 1.3.14:

• 157 passed, 0 failed, 645 assertions across the manager, routing, terminal architecture, ScriptTerminalManager, PTY cleanup and Agent Manager architecture suites.
• Host/webview typecheck, extension lint, Knip, formatting, git diff --check and bun run check-kilocode-change passed.
• Four new router regressions failed before the lifecycle fix and passed afterwards. Six added cases cover late failed cleanup through repeated disposal and directory cleanup, retirement release, initial creation awaiting connection, and terminal-ID reuse across directories.
• Original five-case reproduction: 3 passed / 2 failed before; 5 passed after.
• git diff --check and bun run check-kilocode-change passed.

Reviewer test steps

From packages/kilo-vscode:

bun test tests/unit/agent-manager-terminal-manager.test.ts tests/unit/agent-manager-terminal-routing.test.ts tests/unit/terminal-architecture.test.ts tests/unit/script-terminal-manager.test.ts tests/unit/pty-cleanup.test.ts tests/unit/agent-manager-arch.test.ts

The regression suite holds create/resize responses until after close/dispose completes, then checks late cleanup. It also covers setup failure, repeated/deduplicated restart, terminal ID reuse, typed not-found responses and partial cleanup recovery.

Blocked checks and substitute verification

Dependencies were installed with the frozen lockfile. Typecheck, lint, format, Knip and the focused architecture/PTY suites all ran successfully. Live VS Code extension/backend verification remains pending.

Checklist

• Issue linked above, or exception explained
• Tests/verification described
• Screenshots/video included for visual changes, or marked N/A
• Changeset considered for user-facing changes
• I personally reviewed the diff and can explain the changes, including any AI-assisted work.

[kilo-code-bot]

Code Review Summary

Status: No Issues Found | Recommendation: Merge

The follow-up commit (fc6ad128, "retain terminal cleanup across router disposal") closes the panel-reload gap left by the prior ownership rework. I traced the new lifecycle: TerminalRouter.dispose() retires the old manager before replacing it, prune() releases a retired manager only when both the router's create counter and hasPendingCleanup() (owned / creates / restarts) are clear, and every path that can drain those sets — initial create finally, restart .finally, dispose() .then, and closeDirectory() finally — triggers prune(), so a retired manager is not dropped while a late allocation or failed removal is still outstanding. TerminalManager.closeDirectory now iterates owned instead of entries, keeping detached ownership reachable while scoping by resolved cwd, and blockDirectory/closeDirectory correctly fan out across current + retired managers. The new router tests exercise the real manager against a resource-tracking SDK fixture (failed first removal, retry via dispose and via closeDirectory, initial create awaiting connection), and the assertion that directory cleanup ignores other directories matches the changed ownership filter. No memory leak was found: a retired manager is retained only while a PTY, create, or restart is genuinely pending, and that retention is bounded by cleanup state rather than per-reload.

Files Reviewed (4 files)

• packages/kilo-vscode/src/agent-manager/terminal-manager.ts
• packages/kilo-vscode/src/agent-manager/terminal-routing.ts
• packages/kilo-vscode/tests/unit/agent-manager-terminal-manager.test.ts
• packages/kilo-vscode/tests/unit/agent-manager-terminal-routing.test.ts

Previous Review Summary (commit 0913273)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit 0913273)

Status: No Issues Found | Recommendation: Merge

The retry/ownership rework in terminal-manager.ts is internally consistent. I traced the create/restart/close/dispose interleavings (close and dispose during a pending create/resize, failed replacement cleanup, terminal-ID reuse, lost removal responses, and partial close failures) and each path either re-derives ownership from owned/ptys or lets restartEntry's finally reap a late allocation, so no PTY is dropped from bookkeeping while still live. entries ⊆ owned holds, release guards on ptys.size/pending restarts, and the typed PtyNotFoundError match uses the v2 SDK's top-level _tag/ptyID body shape (confirmed against packages/protocol/src/errors.ts and httpapi-v2-pty.test.ts). The new tests exercise the real manager against a resource-tracking SDK fixture rather than asserting on the mock, and the patch changeset is present and user-facing.

Files Reviewed (3 files)

• .changeset/tidy-terminal-restarts.md
• packages/kilo-vscode/src/agent-manager/terminal-manager.ts
• packages/kilo-vscode/tests/unit/agent-manager-terminal-manager.test.ts

---

_{Reviewed by deepseek-v4.1-flash · Input: 80.4K · Output: 25.9K · Cached: 690.8K}

_{Review guidance: REVIEW.md from base branch main}

[marius-kilocode]

The core revision and identity fix is correct, but cleanup ownership is still lost across the production router lifecycle. Please address these two cases before merging.

1. Keep late-cleanup ownership reachable after router disposal.

TerminalRouter.dispose() replaces manager M1 with M2 immediately. M1.dispose() intentionally returns without waiting for a pending replacement create. If pty-2 then arrives and its removal fails, M1 retains it in owned, but nothing references M1 after the restart promise settles. Later disposal calls only M2, so pty-2 cannot be retried and remains alive until kilo serve exits. This contradicts the retryable-cleanup goal.

Please either retain retired managers in TerminalRouter until they have no owned PTYs or pending restarts, and retry them during later cleanup, or use an equivalent retry mechanism that survives manager replacement. Do not make disposal wait for a stalled create request.

2. Include detached ownership in directory cleanup.

TerminalManager.closeDirectory() scans entries, while close() correctly scans owned. After a late replacement cleanup fails, the entry is detached from entries but remains in owned, so worktree cleanup can report success without retrying that PTY. Directory cleanup should operate on owned entries for the target directory.

Please add regression coverage at the router boundary:

1. Hold a replacement create, dispose the router, release the replacement, fail its first removal, trigger cleanup again, and assert that the replacement is removed and the retired manager can be released.
2. Leave a failed late replacement only in owned, call closeDirectory(), allow removal to succeed, and assert that no PTY remains.

I reproduced the first case through TerminalRouter: after the failed late removal and a second dispose(), the live set is still ["pty-2"]. The existing manager-level retry test reuses the same manager, so it does not cover this production lifecycle.

[LEN5010]

@marius-kilocode both points should be fixed in fc6ad12: retired managers get retried on later dispose()/closeDirectory(), and closeDirectory() now walks owned.
Added the two router tests you described. Mind taking another look? Thanks!

[marius-kilocode]

Both cleanup issues from my previous review are fixed. Retired managers remain available for cleanup retries, and directory cleanup includes detached ownership. All 157 focused tests, compilation, lint, and typechecks pass. I also tested terminal restart and terminal/panel teardown in isolated VS Code and confirmed the shell processes were removed. Thanks for addressing this and adding the router regressions!