Skip to content

fix: clean up orphaned tmux windows on worktree list poll - #60

Merged
centdix merged 3 commits into
mainfrom
cleanup-tmux-windows
Mar 3, 2026
Merged

fix: clean up orphaned tmux windows on worktree list poll#60
centdix merged 3 commits into
mainfrom
cleanup-tmux-windows

Conversation

@centdix

@centdix centdix commented Mar 3, 2026

Copy link
Copy Markdown
Contributor

Summary

Orphaned tmux windows (e.g. wm-favicon-ideas, wm-ai-logic-share) accumulate when worktrees are removed outside of workmux rm (manual git commands, crashes, Ctrl-C). This adds automatic cleanup that runs on every dashboard poll, killing any wm-* tmux window that has no matching active worktree.

Changes

  • Add cleanupStaleWindows() in workmux.ts — compares all wm-* tmux windows against a set of active branch names and kills orphans
  • Call it from apiGetWorktrees() in server.ts after fetching worktree and pane data, so it piggybacks on the existing polling cadence with no new timers

Test plan

  • Create a worktree with workmux add, then remove it with git worktree remove (bypassing workmux) — verify the tmux window gets cleaned up on next dashboard poll
  • Verify active worktree windows (wm-cleanup-tmux-windows, wm-startup-envs, etc.) are never killed
  • Check backend logs show [cleanup] killing stale tmux window "wm-..." for each orphan

Generated with Claude Code

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@claude

claude Bot commented Mar 3, 2026

Copy link
Copy Markdown

Claude finished @centdix's task in 1m 9s —— View job


Code Review: Clean up orphaned tmux windows

  • Read changed files and understand the implementation
  • Review cleanupStaleWindows() in workmux.ts
  • Review integration in server.ts
  • Post review feedback

Clean, well-scoped PR. The approach of piggybacking on the existing poll cadence rather than adding a new timer is the right call. A few observations:

Looks good

  • Fire-and-forget with proper error handling — try/catch around the whole thing with log.warn on failure is exactly right for cleanup code that shouldn't affect the main response path.
  • Deduplication via killed set — handles the case where the same wm-* window appears across multiple tmux sessions.
  • No new dependencies or timers — minimal blast radius.

Minor concerns

1. Synchronous blocking on every poll (workmux.ts:537-540)

Bun.spawnSync blocks the event loop. On a typical poll cycle this adds a few milliseconds for list-windows, but if there are many orphans to kill, it's one spawnSync per orphan. In practice this is fine since orphans are rare, but worth being aware that this runs in the hot path of GET /api/worktrees (polled every ~2s by the dashboard).

If this ever becomes a concern, switching to async Bun.spawn + proc.exited would let it truly run fire-and-forget without blocking the response. Not needed now, just noting it.

2. Hardcoded "main" branch (server.ts:231)

activeBranches.add("main");

This protects wm-main from being killed, but if the repo's default branch is master or something else, it wouldn't be included. That said, the main worktree typically doesn't have a wm-* tmux window at all, so this is more of a safety belt. Fine as-is, but a comment explaining why "main" specifically might help future readers (the existing comment is good but could mention it's the default branch assumption).

3. Potential code reuse with findWorktreeSession (workmux.ts:482-498)

Both findWorktreeSession and cleanupStaleWindows call tmux list-windows -a -F "#{session_name}:#{window_name}" and parse the output identically. Not a blocker since one is sync and one is async, and extracting a shared parser would be over-engineering for two call sites. Just noting the pattern overlap.

Verdict

LGTM — this is a solid, minimal fix for a real problem. No blocking issues.

centdix and others added 2 commits March 3, 2026 13:52
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@centdix
centdix marked this pull request as ready for review March 3, 2026 12:57
@centdix
centdix merged commit c399d02 into main Mar 3, 2026
1 check passed
@centdix
centdix deleted the cleanup-tmux-windows branch March 3, 2026 12:58
@claude

claude Bot commented Mar 3, 2026

Copy link
Copy Markdown

Claude finished @centdix's task in 1m 11s —— View job


Code Review: Clean up orphaned tmux windows on worktree list poll

  • Read changed files and understand the implementation
  • Review cleanupStaleWindows() in workmux.ts
  • Review integration in server.ts
  • Post review feedback

Clean, well-scoped PR. The approach of piggybacking on the existing poll cadence rather than adding a new timer is the right call. A few observations:

Looks good

  • Fire-and-forget with proper error handling — try/catch around the whole thing with log.warn on failure is exactly right for cleanup code that shouldn't affect the main response path.
  • Project scoping via panePath.startsWith(worktreeBaseDir) — this is the key safety mechanism that prevents killing windows from other projects. Well thought out.
  • Deduplication via seen set — handles multi-pane windows correctly (multiple panes → multiple lines from list-panes -a), only killing each window once.
  • Async Bun.spawn + Promise.all — non-blocking kills in parallel. Good improvement over the sync approach used elsewhere in the codebase.
  • No new dependencies or timers — minimal blast radius.

Minor concerns

1. cleanupStaleWindows is not awaited (server.ts:232)

cleanupStaleWindows(activeBranches, `${PROJECT_DIR}__worktrees/`);

The call is intentionally fire-and-forget, which is fine for cleanup. However, since it's an async function returning a Promise that's not awaited, any rejection would become an unhandled promise rejection (the internal try/catch should prevent this, but if something unexpected throws before the try block runs — e.g., argument validation — it could surface). Currently the function body is entirely wrapped in try/catch, so this is safe in practice.

2. Hardcoded "main" branch (server.ts:231)

activeBranches.add("main");

This protects wm-main from cleanup, but if the repo's default branch is master or something custom, it wouldn't be included. In practice the main worktree likely doesn't have a wm-* tmux window, and even if it did, the panePath.startsWith(worktreeBaseDir) check would probably save it (main worktree lives outside __worktrees/). So this is a reasonable safety belt.

3. Race between pane path check and worktree removal

There's a narrow window where a worktree is being actively removed while cleanupStaleWindows runs — the pane could still show the old cwd. This is benign since killing an already-closing window is harmless, but worth noting.

4. list-panes -a output for windows with multiple panes

The seen set deduplicates by windowName, so only the first pane's path is checked against worktreeBaseDir. If a window's first pane has navigated to a directory outside the worktree base (e.g., user cd'd to /tmp), the window would survive cleanup even if orphaned. Conversely, if pane 0 happens to be in the worktree dir, it works correctly. This is a very unlikely edge case and probably not worth over-engineering for.

Verdict

LGTM — this is a solid, minimal fix for a real problem. The project-scoping via pane path is particularly well done — it avoids the cross-project footgun that a naive wm-* glob would introduce. No blocking issues.


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