fix: release foreground bash shells after completion#135
fix: release foreground bash shells after completion#135frostming merged 2 commits intobubbuild:mainfrom
Conversation
| 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) |
There was a problem hiding this comment.
Seems it can be moved into wait_closed to make it cleaner, since wait_closed will be called on all paths
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Aha, yes, then move to _finalize_shell
There was a problem hiding this comment.
Hi, @Andy963 would you like to update the PR? Does my last comment make sense to you?
There was a problem hiding this comment.
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.
2 participants
Summary
Foreground
bashcommands are now released fromShellManagerafter completion, preventing finished shells (and their buffered stdout/stderr) from being retained in memory in long-running Bub processes (e.g.chat/gateway).Changes
ShellManager.release(shell_id)to remove a shell from the internal registry.bashalways callsrelease()in afinallyblock (success, non-zero exit, timeout)._shellsafter execution (including failure).Notes
bash.output/bash.kill).Verification
uv run ruff check .uv run mypy srcuv run pytest -q