From 69f0e2748fc507a3a3caa483bd1acc522c54e498 Mon Sep 17 00:00:00 2001 From: GitHub Copilot Date: Wed, 28 Jan 2026 13:30:21 +0000 Subject: [PATCH] Bump version and prevent screenshot deadlock --- pyproject.toml | 2 +- src/textual_webterm/local_server.py | 32 +++-- src/textual_webterm/terminal_session.py | 41 +++--- tests/test_docker_watcher.py | 119 ++++++++++++------ ...test_local_server_websocket_integration.py | 83 ++++++++++++ 5 files changed, 203 insertions(+), 74 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 739faa2..58c22d1 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [tool.poetry] name = "textual-webterm" -version = "1.0.0" +version = "1.0.1" description = "Serve terminal sessions over the web" authors = ["Will McGugan "] license = "MIT" diff --git a/src/textual_webterm/local_server.py b/src/textual_webterm/local_server.py index 335fb37..5b87995 100644 --- a/src/textual_webterm/local_server.py +++ b/src/textual_webterm/local_server.py @@ -536,24 +536,7 @@ class LocalServer: raise web.HTTPNotFound(text="Session not found") # Get the actual screen state from the terminal session's pyte screen - # Use a lightweight dirty check first to avoid clearing dirty flags unnecessarily. - has_changes = await session_process.get_screen_has_changes() # type: ignore[union-attr] cached = self._screenshot_cache.get(route_key) - if not has_changes and cached is not None: - cached_response = self._get_cached_screenshot_response(request, route_key) - if cached_response is not None: - return cached_response - - screen_width, screen_height, screen_buffer, _ = await session_process.get_screen_state() # type: ignore[union-attr] - - now = asyncio.get_event_loop().time() - ttl = self._get_screenshot_cache_ttl(route_key, now) - - # Also check time-based cache for recently rendered screenshots - if cached is not None and (now - cached[0]) < ttl: - cached_response = self._get_cached_screenshot_response(request, route_key) - if cached_response is not None: - return cached_response lock = self._screenshot_locks.get(route_key) if lock is None: @@ -561,7 +544,7 @@ class LocalServer: self._screenshot_locks[route_key] = lock async with lock: - # Another request may have refreshed the cache while we waited. + now = asyncio.get_event_loop().time() ttl = self._get_screenshot_cache_ttl(route_key, now) cached = self._screenshot_cache.get(route_key) if cached is not None and (now - cached[0]) < ttl: @@ -569,6 +552,19 @@ class LocalServer: if cached_response is not None: return cached_response + has_changes = await session_process.get_screen_has_changes() # type: ignore[union-attr] + if not has_changes and cached is not None: + cached_response = self._get_cached_screenshot_response(request, route_key) + if cached_response is not None: + return cached_response + + ( + screen_width, + screen_height, + screen_buffer, + _, + ) = await session_process.get_screen_state() # type: ignore[union-attr] + def _render_svg() -> str: # Use custom SVG exporter - simpler and more reliable than Rich return render_terminal_svg( diff --git a/src/textual_webterm/terminal_session.py b/src/textual_webterm/terminal_session.py index 3d08abf..dbe98bc 100644 --- a/src/textual_webterm/terminal_session.py +++ b/src/textual_webterm/terminal_session.py @@ -206,25 +206,28 @@ class TerminalSession(Session): has_changes = len(self._screen.dirty) > 0 # Clear dirty set after checking self._screen.dirty.clear() - # Copy the buffer data to avoid holding the lock - buffer = [] - for row in range(height): - row_data = [] - for col in range(width): - char = self._screen.buffer[row][col] - row_data.append( - { - "data": char.data if char.data else " ", - "fg": char.fg, - "bg": char.bg, - "bold": char.bold, - "italics": char.italics, - "underscore": char.underscore, - "reverse": char.reverse, - } - ) - buffer.append(row_data) - return (width, height, buffer, has_changes) + # Snapshot buffer cells quickly to minimize lock hold time + snapshot = [ + [self._screen.buffer[row][col] for col in range(width)] for row in range(height) + ] + + buffer = [] + for row_data in snapshot: + row_chars = [] + for char in row_data: + row_chars.append( + { + "data": char.data if char.data else " ", + "fg": char.fg, + "bg": char.bg, + "bold": char.bold, + "italics": char.italics, + "underscore": char.underscore, + "reverse": char.reverse, + } + ) + buffer.append(row_chars) + return (width, height, buffer, has_changes) def update_connector(self, connector: SessionConnector) -> None: """Update the connector for reconnection without restarting the session.""" diff --git a/tests/test_docker_watcher.py b/tests/test_docker_watcher.py index d5f2127..bfe91d8 100644 --- a/tests/test_docker_watcher.py +++ b/tests/test_docker_watcher.py @@ -9,62 +9,67 @@ import pytest from textual_webterm.docker_watcher import DEFAULT_COMMAND, LABEL_NAME, DockerWatcher +@pytest.fixture +def session_manager(): + manager = MagicMock() + manager.apps_by_slug = {} + manager.apps = [] + manager.get_session_by_route_key.return_value = None + return manager + + +@pytest.fixture +def docker_watcher(session_manager): + return DockerWatcher(session_manager) + + class TestDockerWatcher: """Tests for DockerWatcher class.""" - def test_container_to_slug(self): + def test_container_to_slug(self, docker_watcher): """Test slug generation from container names.""" - watcher = DockerWatcher(MagicMock()) - # Test basic name container = {"Names": ["/my-container"]} - assert watcher._container_to_slug(container) == "my-container" + assert docker_watcher._container_to_slug(container) == "my-container" # Test with underscores container = {"Names": ["/my_container_name"]} - assert watcher._container_to_slug(container) == "my-container-name" + assert docker_watcher._container_to_slug(container) == "my-container-name" # Test with dots container = {"Names": ["/service.name"]} - assert watcher._container_to_slug(container) == "service-name" + assert docker_watcher._container_to_slug(container) == "service-name" # Test fallback to ID container = {"Id": "abc123def456"} - assert watcher._container_to_slug(container) == "abc123def456" + assert docker_watcher._container_to_slug(container) == "abc123def456" - def test_get_container_name(self): + def test_get_container_name(self, docker_watcher): """Test extracting container name.""" - watcher = DockerWatcher(MagicMock()) - container = {"Names": ["/my-container"]} - assert watcher._get_container_name(container) == "my-container" + assert docker_watcher._get_container_name(container) == "my-container" container = {"Names": []} container["Id"] = "abc123def456789" - assert watcher._get_container_name(container) == "abc123def456" + assert docker_watcher._get_container_name(container) == "abc123def456" - def test_get_container_command_auto(self): + def test_get_container_command_auto(self, docker_watcher): """Test command generation when label is 'auto'.""" - watcher = DockerWatcher(MagicMock()) - container = {"Names": ["/my-container"], "Labels": {LABEL_NAME: "auto"}} expected = f"docker exec -it my-container {DEFAULT_COMMAND}" - assert watcher._get_container_command(container) == expected + assert docker_watcher._get_container_command(container) == expected - def test_get_container_command_custom(self): + def test_get_container_command_custom(self, docker_watcher): """Test command when label has custom value.""" - watcher = DockerWatcher(MagicMock()) - container = { "Names": ["/my-container"], "Labels": {LABEL_NAME: "docker logs -f my-container"}, } - assert watcher._get_container_command(container) == "docker logs -f my-container" + assert docker_watcher._get_container_command(container) == "docker logs -f my-container" @pytest.mark.asyncio - async def test_add_container(self): + async def test_add_container(self, session_manager): """Test adding a container.""" - session_manager = MagicMock() on_added = MagicMock() watcher = DockerWatcher(session_manager, on_container_added=on_added) @@ -87,9 +92,8 @@ class TestDockerWatcher: assert "test-container" in watcher._managed_containers @pytest.mark.asyncio - async def test_add_container_already_managed(self): + async def test_add_container_already_managed(self, session_manager): """Test adding a container that's already managed.""" - session_manager = MagicMock() watcher = DockerWatcher(session_manager) watcher._managed_containers["test-container"] = "abc123" @@ -101,9 +105,8 @@ class TestDockerWatcher: session_manager.add_app.assert_not_called() @pytest.mark.asyncio - async def test_remove_container(self): + async def test_remove_container(self, session_manager): """Test removing a container.""" - session_manager = MagicMock() session_manager.apps_by_slug = {"test-container": MagicMock()} session_manager.apps = [session_manager.apps_by_slug["test-container"]] session_manager.get_session_by_route_key.return_value = None @@ -121,9 +124,8 @@ class TestDockerWatcher: on_removed.assert_called_once_with("test-container") @pytest.mark.asyncio - async def test_remove_container_not_managed(self): + async def test_remove_container_not_managed(self, session_manager): """Test removing a container that's not managed.""" - session_manager = MagicMock() on_removed = MagicMock() watcher = DockerWatcher(session_manager, on_container_removed=on_removed) @@ -133,9 +135,8 @@ class TestDockerWatcher: on_removed.assert_not_called() @pytest.mark.asyncio - async def test_start_stop(self): + async def test_start_stop(self, session_manager): """Test starting and stopping the watcher.""" - session_manager = MagicMock() watcher = DockerWatcher(session_manager, socket_path="/nonexistent.sock") # Mock the methods that would fail without Docker @@ -153,9 +154,8 @@ class TestDockerWatcherIntegration: """Integration-style tests for Docker watcher.""" @pytest.mark.asyncio - async def test_handle_start_event(self): + async def test_handle_start_event(self, session_manager): """Test handling a container start event.""" - session_manager = MagicMock() watcher = DockerWatcher(session_manager) # Mock the docker request to return container info @@ -180,9 +180,8 @@ class TestDockerWatcherIntegration: session_manager.add_app.assert_called_once() @pytest.mark.asyncio - async def test_handle_die_event(self): + async def test_handle_die_event(self, session_manager): """Test handling a container die event.""" - session_manager = MagicMock() session_manager.apps_by_slug = {} session_manager.apps = [] session_manager.get_session_by_route_key.return_value = None @@ -201,9 +200,8 @@ class TestDockerWatcherIntegration: assert "test-service" not in watcher._managed_containers @pytest.mark.asyncio - async def test_handle_event_without_label(self): + async def test_handle_event_without_label(self, session_manager): """Test that events without our label are ignored.""" - session_manager = MagicMock() watcher = DockerWatcher(session_manager) event = { @@ -218,3 +216,52 @@ class TestDockerWatcherIntegration: # Should not add container session_manager.add_app.assert_not_called() + + +@pytest.mark.parametrize( + ("labels", "expected"), + [ + ({"webterm-command": "echo hi"}, "echo hi"), + ({"webterm-command": "auto"}, f"docker exec -it my-container {DEFAULT_COMMAND}"), + ({"other": "value"}, f"docker exec -it my-container {DEFAULT_COMMAND}"), + ], +) +def test_get_container_command_variants(docker_watcher, labels, expected): + container = {"Names": ["/my-container"], "Labels": labels} + assert docker_watcher._get_container_command(container) == expected + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + ("status", "body", "expected"), + [ + (200, '[{"Id":"abc","Names":["/c1"],"Labels":{"webterm-command":"auto"}}]', 1), + (200, "[]", 0), + (500, "error", 0), + ], +) +async def test_get_labeled_containers_handles_status( + docker_watcher, status, body, expected, monkeypatch +): + async def fake_request(method: str, path: str): + return status, body + + monkeypatch.setattr(docker_watcher, "_docker_request", fake_request) + result = await docker_watcher._get_labeled_containers() + assert len(result) == expected + + +@pytest.mark.asyncio +async def test_watch_events_recovers_from_errors(docker_watcher, monkeypatch): + docker_watcher._running = True + + async def fail_once(*_args, **_kwargs): + docker_watcher._running = False + raise OSError("boom") + + async def fake_sleep(_seconds): + return None + + monkeypatch.setattr("textual_webterm.docker_watcher.asyncio.open_unix_connection", fail_once) + monkeypatch.setattr("textual_webterm.docker_watcher.asyncio.sleep", fake_sleep) + await docker_watcher._watch_events() diff --git a/tests/test_local_server_websocket_integration.py b/tests/test_local_server_websocket_integration.py index 7240cda..54ea807 100644 --- a/tests/test_local_server_websocket_integration.py +++ b/tests/test_local_server_websocket_integration.py @@ -1,6 +1,8 @@ from __future__ import annotations import json +from contextlib import asynccontextmanager +from typing import TYPE_CHECKING from unittest.mock import AsyncMock import pytest @@ -9,7 +11,10 @@ from aiohttp.test_utils import TestClient, TestServer from textual_webterm.config import App, Config from textual_webterm.local_server import LocalServer +from textual_webterm.types import RouteKey, SessionID +if TYPE_CHECKING: + from collections.abc import AsyncIterator async def _make_client(server: LocalServer) -> TestClient: app = web.Application() @@ -20,6 +25,36 @@ async def _make_client(server: LocalServer) -> TestClient: return client +@pytest.fixture +def server_factory(tmp_path): + counter = {"i": 0} + + def _make(apps: list[App] | None = None) -> LocalServer: + counter["i"] += 1 + config = Config( + apps=apps + or [App(name="Test", slug="test", path=".", command="echo test", terminal=True)] + ) + config_file = tmp_path / f"config-{counter['i']}.toml" + config_file.write_text("") + return LocalServer(config_path=str(config_file), config=config) + + return _make + + +@pytest.fixture +def client_factory(): + @asynccontextmanager + async def _factory(server: LocalServer) -> AsyncIterator[TestClient]: + client = await _make_client(server) + try: + yield client + finally: + await client.close() + + return _factory + + @pytest.mark.asyncio async def test_websocket_creates_session_on_resize(tmp_path): config = Config( @@ -121,3 +156,51 @@ async def test_websocket_ignores_invalid_envelopes(tmp_path): await ws.close() finally: await client.close() + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + ("payload", "is_binary"), + [ + ("not json", False), + (json.dumps({"not": "a list"}), False), + (json.dumps([]), False), + (b"\x00\x01\x02", True), + ], +) +async def test_websocket_invalid_payloads_keep_connection( + server_factory, client_factory, payload, is_binary +): + server = server_factory() + async with client_factory(server) as client: + ws = await client.ws_connect("/ws/test") + if is_binary: + await ws.send_bytes(payload) + else: + await ws.send_str(payload) + await ws.send_str(json.dumps(["ping", "ok"])) + + msg = await ws.receive(timeout=1) + assert msg.type == WSMsgType.TEXT + assert json.loads(msg.data) == ["pong", "ok"] + await ws.close() + + +@pytest.mark.asyncio +async def test_websocket_clears_stale_session(server_factory, client_factory): + server = server_factory() + + class DummySession: + def is_running(self): + return False + + session_id = SessionID("sid") + route_key = RouteKey("test") + server.session_manager.routes[route_key] = session_id + server.session_manager.sessions[session_id] = DummySession() + + async with client_factory(server) as client: + ws = await client.ws_connect("/ws/test") + assert server.session_manager.get_session(session_id) is None + assert server.session_manager.routes.get(route_key) is None + await ws.close()