Skip to content

Fix three critical bugs causing bash/tmux session leaks #2804

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
rbren wants to merge 4 commits into from
Closed

Fix three critical bugs causing bash/tmux session leaks #2804

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0fd0ea6
Enable delegation by default in agent-server and add stress tests
rbren Apr 3, 2026
8fb7862
Merge branch 'main' of https://github.com/openhands/software-agent-sdk
rbren Apr 4, 2026
e63469d
Merge branch 'main' of https://github.com/openhands/software-agent-sdk
rbren Apr 11, 2026
d0289a6
Fix three critical bugs causing bash/tmux session leaks
rbren Apr 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions openhands-agent-server/openhands/agent_server/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,50 @@
logger = get_logger(__name__)


async def cleanup_stale_tmux_sessions() -> None:
"""Clean up any stale tmux sessions on server startup.

Tmux sessions live in a separate process that survives agent-server restarts.
This function kills all existing sessions on the openhands socket to prevent
accumulation of orphaned sessions. Reconnecting conversations will create
fresh sessions as needed.
"""
try:
import libtmux

# Connect to the dedicated OpenHands tmux server
server = libtmux.Server(socket_name="openhands")

# Get all sessions on this server
sessions = server.sessions
if not sessions:
logger.debug("No tmux sessions found on openhands socket")
return

logger.info("Cleaning up %d stale tmux session(s) on startup", len(sessions))

# Kill all sessions - they're all stale since we're starting up
for session in sessions:
try:
logger.debug("Killing tmux session: %s", session.name)
session.kill()
except Exception as e:
logger.warning("Failed to kill tmux session %s: %s", session.name, e)

logger.info("Tmux cleanup completed")

except ImportError:
logger.debug("libtmux not available, skipping tmux cleanup")
except Exception as e:
# Don't let tmux cleanup failures prevent server startup
logger.warning("Failed to cleanup tmux sessions: %s", e)
Comment on lines +52 to +88
Copy link
Copy Markdown
Collaborator

@all-hands-bot all-hands-bot Apr 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Important: This cleanup logic is overly aggressive - it kills ALL sessions on the "openhands" socket without checking if they're actually stale.

Problem: What if multiple agent-server instances share the same tmux socket? What if there are legitimate long-running sessions that should persist across server restarts?

Better approach: Track session ownership (e.g., PID file, session metadata) and only kill sessions that belong to this server instance or are truly orphaned.



@asynccontextmanager
async def api_lifespan(api: FastAPI) -> AsyncIterator[None]:
# Clean up stale tmux sessions from previous server runs
await cleanup_stale_tmux_sessions()

service = get_default_conversation_service()
vscode_service = get_vscode_service()
desktop_service = get_desktop_service()
Expand Down
1 change: 1 addition & 0 deletions openhands-agent-server/openhands/agent_server/tool_router.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
register_builtins_agents(enable_browser=True)
register_gemini_tools(enable_browser=True)
register_planning_tools()
register_builtins_agents()
Copy link
Copy Markdown
Collaborator

@all-hands-bot all-hands-bot Apr 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Critical: Duplicate registration call. register_builtins_agents(enable_browser=True) is already called on line 16. This second call with different parameters will either be redundant or cause double-registration issues.

Remove this duplicate line.

Copy link
Copy Markdown
Collaborator

@all-hands-bot all-hands-bot Apr 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Important: Duplicate function call.

This line calls register_builtins_agents() again, but line 16 already calls register_builtins_agents(enable_browser=True). This is a merge conflict artifact - commit 0fd0ea6 added this call, then a merge from main added the call on line 16, but this one wasn't removed.

While harmless (the function is idempotent via register_agent_if_absent), it's wasteful to register agents twice on every server startup.

Recommendation: Remove this line, keep only line 16.



# Tool listing
Expand Down
4 changes: 4 additions & 0 deletions openhands-tools/openhands/tools/preset/default.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,15 @@
def register_default_tools(enable_browser: bool = True) -> None:
"""Register the default set of tools."""
# Tools are now automatically registered when imported
from openhands.tools.delegate import DelegateTool
from openhands.tools.file_editor import FileEditorTool
from openhands.tools.task_tracker import TaskTrackerTool
from openhands.tools.terminal import TerminalTool

logger.debug(f"Tool: {TerminalTool.name} registered.")
logger.debug(f"Tool: {FileEditorTool.name} registered.")
logger.debug(f"Tool: {TaskTrackerTool.name} registered.")
Comment on lines +22 to 29
Copy link
Copy Markdown
Collaborator

@all-hands-bot all-hands-bot Apr 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Critical - Scope Creep: These DelegateTool changes are completely unrelated to "fixing tmux session leaks" as described in the PR title and description.

This PR mixes unrelated changes:

  • Tmux cleanup (described)
  • Task manager delete_on_close fix (described)
  • DelegateTool registration (NOT described)
  • 328-line delegation stress test (NOT described)

Split the DelegateTool changes into a separate PR with proper description and justification.

logger.debug(f"Tool: {DelegateTool.name} registered.")

if enable_browser:
from openhands.tools.browser_use import BrowserToolSet
Expand All @@ -44,6 +46,7 @@ def get_default_tools(
register_default_tools(enable_browser=enable_browser)

# Import tools to access their name attributes
from openhands.tools.delegate import DelegateTool
from openhands.tools.file_editor import FileEditorTool
from openhands.tools.task_tracker import TaskTrackerTool
from openhands.tools.terminal import TerminalTool
Expand All @@ -52,6 +55,7 @@ def get_default_tools(
Tool(name=TerminalTool.name),
Tool(name=FileEditorTool.name),
Tool(name=TaskTrackerTool.name),
Tool(name=DelegateTool.name),
]
if enable_browser:
from openhands.tools.browser_use import BrowserToolSet
Expand Down
4 changes: 2 additions & 2 deletions openhands-tools/openhands/tools/task/manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ def _resume_task(self, resume: str, subagent_type: str) -> Task:
persistence_dir=self._persistence_dir,
conversation_id=conversation_id,
hook_config=factory.definition.hooks,
delete_on_close=False,
delete_on_close=True,
)

self._set_confirmation_policy(
Expand Down Expand Up @@ -285,7 +285,7 @@ def _get_conversation(
conversation_id=conversation_id,
max_iteration_per_run=max_iteration_per_run,
hook_config=hook_config,
delete_on_close=False,
delete_on_close=True,
)

def _get_sub_agent(self, subagent_type: str) -> Agent:
Expand Down
Loading
Loading