Skip to content

fix: release foreground bash shells after completion #135

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

Merged
frostming merged 2 commits into from
Mar 27, 2026
Merged

fix: release foreground bash shells after completion #135

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0f6e40d
fix: release foreground bash shells after completion
Andy963 Mar 24, 2026
e213df4
fix: finalize shell release in manager
Andy963 Mar 27, 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
4 changes: 4 additions & 0 deletions src/bub/builtin/shell_manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,9 @@ def get(self, shell_id: str) -> ManagedShell:
except KeyError as exc:
raise KeyError(f"unknown shell id: {shell_id}") from exc

def release(self, shell_id: str) -> ManagedShell | None:
return self._shells.pop(shell_id, None)
Copy link
Copy Markdown
Collaborator

@frostming frostming Mar 25, 2026

Choose a reason for hiding this comment

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

Seems it can be moved into wait_closed to make it cleaner, since wait_closed will be called on all paths

Copy link
Copy Markdown
Contributor Author

@Andy963 Andy963 Mar 25, 2026

Choose a reason for hiding this comment

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

Not exactly. The timeout path goes through terminate(), not wait_closed(), so wait_closed() is not called on all paths. This change is intentionally scoped to fixing foreground shell cleanup. Background shell cleanup is a separate concern, and I'm not sure yet where that cleanup should happen, since we haven't defined the lifecycle for completed background shells.

Copy link
Copy Markdown
Collaborator

@frostming frostming Mar 25, 2026

Choose a reason for hiding this comment

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

Aha, yes, then move to _finalize_shell

Copy link
Copy Markdown
Collaborator

@frostming frostming Mar 27, 2026

Choose a reason for hiding this comment

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

Hi, @Andy963 would you like to update the PR? Does my last comment make sense to you?

Copy link
Copy Markdown
Contributor Author

@Andy963 Andy963 Mar 27, 2026

Choose a reason for hiding this comment

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

Yes. But moving release() into _finalize_shell() centralizes cleanup, but it still does not fully address background shells: a background shell that exits
naturally and is never touched again will still remain registered.


async def terminate(self, shell_id: str) -> ManagedShell:
shell = self.get(shell_id)
if shell.returncode is not None:
Expand Down Expand Up @@ -80,6 +83,7 @@ async def _finalize_shell(self, shell: ManagedShell) -> None:
for task in shell.read_tasks:
with contextlib.suppress(asyncio.CancelledError):
await task
self._shells.pop(shell.shell_id, None)

async def _drain_stream(
self,
Expand Down
30 changes: 27 additions & 3 deletions tests/test_builtin_tools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
from republic.core.errors import ErrorKind
from republic.tools.executor import ToolExecutor

import bub.builtin.tools as builtin_tools
from bub.builtin.shell_manager import ShellManager
from bub.builtin.tools import bash, bash_output, kill_bash


Expand All @@ -27,6 +29,28 @@ async def test_bash_returns_stdout_for_foreground_command(tmp_path) -> None:
assert result == "hello"


@pytest.mark.asyncio
async def test_foreground_bash_releases_shell_from_shell_manager(tmp_path, monkeypatch) -> None:
manager = ShellManager()
monkeypatch.setattr(builtin_tools, "shell_manager", manager)

result = await bash.run(cmd=_python_shell("print('hello')"), context=_tool_context(tmp_path))

assert result == "hello"
assert manager._shells == {}


@pytest.mark.asyncio
async def test_foreground_bash_releases_shell_when_command_fails(tmp_path, monkeypatch) -> None:
manager = ShellManager()
monkeypatch.setattr(builtin_tools, "shell_manager", manager)

with pytest.raises(RuntimeError, match="command exited with code"):
await bash.run(cmd=_python_shell("import sys; sys.exit(2)"), context=_tool_context(tmp_path))

assert manager._shells == {}


@pytest.mark.asyncio
async def test_bash_non_zero_exit_is_returned_as_tool_error(tmp_path) -> None:
command = _python_shell("import sys; print('boom'); sys.exit(7)")
Expand Down Expand Up @@ -71,7 +95,7 @@ async def test_background_bash_exposes_output_via_bash_output(tmp_path) -> None:


@pytest.mark.asyncio
async def test_kill_bash_terminates_background_process(tmp_path) -> None:
async def test_kill_bash_terminates_background_process_and_releases_shell(tmp_path) -> None:
started = await bash.run(
cmd=_python_shell("import time; time.sleep(10)"),
background=True,
Expand All @@ -80,11 +104,11 @@ async def test_kill_bash_terminates_background_process(tmp_path) -> None:
shell_id = started.removeprefix("started: ").strip()

killed = await kill_bash.run(shell_id=shell_id)
output = await bash_output.run(shell_id=shell_id)

assert killed.startswith(f"id: {shell_id}\nstatus: exited\nexit_code: ")
assert "exit_code: null" not in killed
assert output.startswith(f"id: {shell_id}\nstatus: exited\n")
with pytest.raises(KeyError, match="unknown shell id"):
await bash_output.run(shell_id=shell_id)


@pytest.mark.asyncio
Expand Down
Loading