Skip to content

fix(host-service): clean up exited terminals on workspace delete#3856

Merged
Kitenite merged 1 commit into
mainfrom
fix/terminal-workspace-cleanup-status-filter
Apr 29, 2026
Merged

fix(host-service): clean up exited terminals on workspace delete#3856
Kitenite merged 1 commit into
mainfrom
fix/terminal-workspace-cleanup-status-filter

Conversation

@Kitenite
Copy link
Copy Markdown
Collaborator

@Kitenite Kitenite commented Apr 29, 2026

Summary

  • disposeSessionsByWorkspaceId filtered DB rows by status = "active", so any session whose row had drifted to exited (via pty.onExit) was skipped on workspace deletion
  • The in-memory sessions Map entry stayed behind in those cases — slow leak per workspace delete
  • Widened the filter to status != "disposed" so zombie rows are picked up too. disposeSession is already idempotent against a dead PTY (see comment at terminal.ts:271)

Terminals are intentionally persistent across most lifecycle events; this only changes behavior on the explicit workspace-delete path. Pane-deletion path is unaffected (it goes through disposeSession directly).

Test plan

  • Create a workspace with a terminal, kill the PTY out-of-band so its row becomes exited, then delete the workspace and confirm no sessions Map entry remains
  • Normal workspace delete with a live terminal still kills the PTY
  • Deleting a workspace with no terminals is a no-op

Summary by cubic

Clean up exited terminal sessions on workspace delete to prevent leftover in-memory sessions entries. disposeSessionsByWorkspaceId now disposes all rows where status != 'disposed' (using ne from drizzle-orm) instead of only active; pane deletion is unchanged.

Written for commit 9ab7de4. Summary will update on new commits. Review in cubic

Summary by CodeRabbit

  • Bug Fixes
    • Terminal session disposal has been improved to comprehensively clean up all inactive terminal sessions within a workspace. Previously, only sessions marked as active were targeted for disposal; the system now properly disposes all sessions that are not already disposed, preventing orphaned resources and ensuring more efficient workspace management.

`disposeSessionsByWorkspaceId` filtered by `status = "active"`, so any
session whose row had drifted to `exited` (e.g. via `pty.onExit`) was
skipped on workspace deletion — leaving the in-memory `sessions` Map
entry behind. Widen to `status != "disposed"` so zombie rows get
disposed too. `disposeSession` is already idempotent against a dead PTY.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 29, 2026

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 117fa778-3530-4566-bcb8-8378dfebd1c2

📥 Commits

Reviewing files that changed from the base of the PR and between cc5c103 and 9ab7de4.

📒 Files selected for processing (1)
  • packages/host-service/src/terminal/terminal.ts

📝 Walkthrough

Walkthrough

The terminal session disposal query was broadened to target all non-disposed sessions instead of only active ones. The disposeSessionsByWorkspaceId function now uses the ne (not equal) operator from drizzle-orm to filter sessions by status not equal to "disposed" rather than equal to "active".

Changes

Cohort / File(s) Summary
Terminal Session Query Update
packages/host-service/src/terminal/terminal.ts
Modified disposeSessionsByWorkspaceId query to target all non-disposed sessions (status != "disposed") instead of only active sessions. Added ne operator import from drizzle-orm.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~5 minutes

Poem

🐰 A terminal session, once stuck in its ways,
Now disposes with broader, more sensible ways—
No longer just "active," but all that aren't "gone,"
The ne operator hops swiftly along! ✨

✨ 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 fix/terminal-workspace-cleanup-status-filter

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
Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.

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

@Kitenite Kitenite merged commit 533e00f into main Apr 29, 2026
6 of 8 checks passed
@Kitenite Kitenite deleted the fix/terminal-workspace-cleanup-status-filter branch April 29, 2026 17:01
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 29, 2026

🚀 Preview Deployment

🔗 Preview Links

Service Status Link
Neon Database (Neon) View Branch
Vercel API (Vercel) Open Preview
Vercel Web (Vercel) Open Preview
Vercel Marketing (Vercel) Open Preview
Vercel Admin (Vercel) Open Preview
Vercel Docs (Vercel) Open Preview

Preview updates automatically with new commits

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Apr 29, 2026

Greptile Summary

This PR fixes a memory leak in disposeSessionsByWorkspaceId: terminals whose PTY exited out-of-band had their DB row set to "exited" but their in-memory sessions Map entry retained. The old status = "active" filter skipped these rows on workspace deletion; the new status != "disposed" filter captures them, and disposeSession already handles dead PTYs idempotently.

Confidence Score: 5/5

Safe to merge — minimal, targeted fix with no regressions on other code paths.

Single two-line change (import + filter predicate). The fix is logically correct: disposeSession is already idempotent against dead PTYs, and the only new behavior is cleaning up Map entries for exited rows on workspace deletion, which is the intended fix. No other code paths are affected.

No files require special attention.

Important Files Changed

Filename Overview
packages/host-service/src/terminal/terminal.ts Widened disposeSessionsByWorkspaceId filter from status = "active" to status != "disposed", fixing a memory leak where in-memory sessions in the exited state were skipped on workspace deletion.

Reviews (1): Last reviewed commit: "fix(host-service): clean up exited termi..." | Re-trigger Greptile

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