Bump version and prevent screenshot deadlock

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 <will@textualize.io>"]
 license = "MIT"

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(

src/textual_webterm/terminal_session.py

@@ -206,13 +206,16 @@ 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
+            # 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 in range(height):
+        for row_data in snapshot:
-                row_data = []
+            row_chars = []
-                for col in range(width):
+            for char in row_data:
-                    char = self._screen.buffer[row][col]
+                row_chars.append(
-                    row_data.append(
                     {
                         "data": char.data if char.data else " ",
                         "fg": char.fg,
@@ -223,7 +226,7 @@ class TerminalSession(Session):
                         "reverse": char.reverse,
                     }
                 )
-                buffer.append(row_data)
+            buffer.append(row_chars)
         return (width, height, buffer, has_changes)
 
     def update_connector(self, connector: SessionConnector) -> None:

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()

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()