From b87a69843800e3e075b4b0e5ea8cb1732bd863a5 Mon Sep 17 00:00:00 2001 From: GitHub Copilot Date: Fri, 6 Feb 2026 17:27:49 +0000 Subject: [PATCH] fix: properly clean up sessions when Docker containers are destroyed - close_session() now calls on_session_end() to remove the session from sessions dict and routes, preventing zombie entries that persist after the container is gone - _remove_container() now closes the active session before removing the app from apps_by_slug/apps, so session cleanup can still reference the app during teardown - Updated and added tests to verify session tracking cleanup --- src/webterm/docker_watcher.py | 17 +++++++++-------- src/webterm/session_manager.py | 3 ++- tests/test_docker_watcher.py | 28 +++++++++++++++++++++++++++- tests/test_session_manager.py | 6 +++++- 4 files changed, 43 insertions(+), 11 deletions(-) diff --git a/src/webterm/docker_watcher.py b/src/webterm/docker_watcher.py index 03be044..00af317 100644 --- a/src/webterm/docker_watcher.py +++ b/src/webterm/docker_watcher.py @@ -211,20 +211,21 @@ class DockerWatcher: log.info("Removing container: %s", slug) del self._managed_containers[slug] - # Remove from session manager's apps - if slug in self._session_manager.apps_by_slug: - app = self._session_manager.apps_by_slug.pop(slug) - if app in self._session_manager.apps: - self._session_manager.apps.remove(app) - - # Close any active session for this slug - route_key = slug # In our case, slug is used as route_key + # Close any active session for this slug before removing the app, + # so session cleanup can still look up the app if needed. + route_key = slug session = self._session_manager.get_session_by_route_key(route_key) if session: session_id = self._session_manager.routes.get(route_key) if session_id: await self._session_manager.close_session(session_id) + # Remove from session manager's apps + if slug in self._session_manager.apps_by_slug: + app = self._session_manager.apps_by_slug.pop(slug) + if app in self._session_manager.apps: + self._session_manager.apps.remove(app) + if self._on_container_removed: self._on_container_removed(slug) diff --git a/src/webterm/session_manager.py b/src/webterm/session_manager.py index 093eb51..bb84130 100644 --- a/src/webterm/session_manager.py +++ b/src/webterm/session_manager.py @@ -168,7 +168,7 @@ class SessionManager: return session_process async def close_session(self, session_id: SessionID) -> None: - """Close a session. + """Close a session and remove it from tracking. Args: session_id: Session identity. @@ -177,6 +177,7 @@ class SessionManager: if session_process is None: return await session_process.close() + self.on_session_end(session_id) def get_session(self, session_id: SessionID) -> Session | None: """Get a session from a session ID. diff --git a/tests/test_docker_watcher.py b/tests/test_docker_watcher.py index 4f96d19..9d6a8de 100644 --- a/tests/test_docker_watcher.py +++ b/tests/test_docker_watcher.py @@ -142,7 +142,33 @@ class TestDockerWatcher: on_removed.assert_called_once_with("test-container") @pytest.mark.asyncio - async def test_remove_container_not_managed(self, session_manager): + async def test_remove_container_with_active_session(self, session_manager): + """Test removing a container that has an active session cleans up session.""" + mock_session = MagicMock() + mock_app = MagicMock() + session_manager.apps_by_slug = {"test-container": mock_app} + session_manager.apps = [mock_app] + session_manager.get_session_by_route_key.return_value = mock_session + session_manager.routes = MagicMock() + session_manager.routes.get.return_value = "session-123" + session_manager.close_session = AsyncMock() + + on_removed = MagicMock() + watcher = DockerWatcher(session_manager, on_container_removed=on_removed) + watcher._managed_containers["test-container"] = "abc123" + + await watcher._remove_container("abc123") + + # Session should be closed + session_manager.close_session.assert_called_once_with("session-123") + + # App should be removed after session cleanup + assert "test-container" not in session_manager.apps_by_slug + assert mock_app not in session_manager.apps + + # Container should be untracked + assert "test-container" not in watcher._managed_containers + on_removed.assert_called_once_with("test-container") """Test removing a container that's not managed.""" on_removed = MagicMock() watcher = DockerWatcher(session_manager, on_container_removed=on_removed) diff --git a/tests/test_session_manager.py b/tests/test_session_manager.py index a9ab00f..d403984 100644 --- a/tests/test_session_manager.py +++ b/tests/test_session_manager.py @@ -137,17 +137,21 @@ class TestSessionManager: @pytest.mark.asyncio async def test_close_session(self, mock_poller, mock_path, sample_apps): - """Test closing a specific session.""" + """Test closing a specific session removes it from tracking.""" manager = SessionManager(mock_poller, mock_path, sample_apps) mock_session = MagicMock() mock_session.close = AsyncMock() session_id = SessionID("test-session") + route_key = RouteKey("test-route") manager.sessions[session_id] = mock_session + manager.routes[route_key] = session_id await manager.close_session(session_id) mock_session.close.assert_called_once() + assert session_id not in manager.sessions + assert route_key not in manager.routes @pytest.mark.asyncio async def test_close_session_nonexistent(self, mock_poller, mock_path, sample_apps):