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
This commit is contained in:
@@ -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)
|
||||
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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):
|
||||
|
||||
Reference in New Issue
Block a user