[Audit v2 #350] Drop SessionRegistry threading.RLock — single asyncio loop, YAGNI (#370)

* Drop SessionRegistry threading.RLock — single asyncio loop, YAGNI

Closes #350 (Audit v2 finding #6).

The RLock provided no asyncio-level mutual exclusion (it wasn't awaited)
and the only callers run inside the single asyncio WS handler — single
event loop, no thread crossings. It misled readers into assuming
thread-safety the registry doesn't deliver. If a future caller ever runs
from another thread, asyncio.Lock is the right answer for this
async-first code, not threading.RLock — so the existing lock wasn't even
the future-proof choice.

Removed:
- `from threading import RLock`
- `self._lock = RLock()`
- every `with self._lock:` block (10 sites)

Renamed `_find_matching_session_locked` → `_find_matching_session` since
the "_locked" suffix is now misleading. Updated the wait_for_session
docstring's "registry lock" reference.

Added a class docstring on SessionRegistry making the asyncio-only
invariant explicit and naming asyncio.Lock as the future-proof choice
should a thread crossing ever appear.

Tests: TestSessionRegistryNoThreadingLock pins the deletion against
accidental re-introduction (no `_lock` attribute, no `threading` /
`RLock` imported in the module). All 28 test_session_registry.py tests
plus the full 769-test pytest suite pass. Live smoke: GDScript
test_run via MCP against a real Godot 4.6.2 editor — 1157 passed, 0
failed (registry exercised end-to-end via session_manage list,
editor_state, and every test that round-trips through the WS handler).

Refs umbrella #343.

https://claude.ai/code/session_optimistic-tesla-84obZ

* Trim docstrings + drop narrating comment per /simplify

- SessionRegistry class docstring: trim 5 lines re-litigating the removed
  RLock down to 3 lines stating the asyncio-only invariant and the
  forward-looking choice (asyncio.Lock).
- TestSessionRegistryNoThreadingLock: collapse the 7-line docstring into
  one line — test names already convey the invariant.
- Drop "# Notify any waiters blocked on wait_for_session()" — the loop
  body, the to_notify name, and future.set_result(session) already say
  this. Per CLAUDE.md, avoid comments narrating WHAT.

https://claude.ai/code/session_optimistic-tesla-84obZ

* Drop prescriptive cross-thread advice from docstrings (Copilot review)

asyncio.Lock is a cooperative-scheduling primitive within a single
event loop — it does NOT make cross-thread access safe. Recommending
it as the future-proof choice for a hypothetical thread crossing was
misleading and would steer the next maintainer toward the wrong fix.

Drop the "use asyncio.Lock" prescription entirely. Class docstring now
states only the current invariant (single asyncio loop, no locking).
Test docstring trimmed to neutral framing.

If a thread crossing is ever genuinely introduced, the right answer is
to marshal through the event loop (asyncio.run_coroutine_threadsafe /
loop.call_soon_threadsafe), not to add a lock — but that decision
belongs to whoever has the actual cross-thread requirement, not to
this docstring.

https://claude.ai/code/session_optimistic-tesla-84obZ

* Cover the done-waiter skip branch in register() (Codecov patch)

The if future.done(): continue branch in register() was uncovered;
Codecov flagged it as the one missing line in the patch. Added a
regression test that pre-completes a future, sticks it in
_session_waiters, and verifies the next register() doesn't re-resolve
it. This pins the "don't double-resolve out-of-band-completed waiters"
invariant that was previously untested.

https://claude.ai/code/session_optimistic-tesla-84obZ

---------

Co-authored-by: Claude <noreply@anthropic.com>

Author: dsarno

src/godot_ai/sessions/registry.py

@@ -7,7 +7,6 @@
 from dataclasses import dataclass, field
 from datetime import datetime, timezone
 from functools import cached_property
-from threading import RLock
 
 from godot_ai import __version__ as _SERVER_VERSION
 
@@ -75,37 +74,38 @@ def to_dict(self) -> dict:
 
 
 class SessionRegistry:
-    """Tracks all connected Godot editor sessions."""
+    """Tracks all connected Godot editor sessions.
+
+    Callers run on the single asyncio event loop driving the WS transport,
+    so state is mutated without locking.
+    """
 
     def __init__(self):
         self._sessions: dict[str, Session] = {}
         self._active_session_id: str | None = None
         self._session_waiters: list[
             tuple[asyncio.Future[Session], str | None, frozenset[str], str | None]
         ] = []
-        self._lock = RLock()
 
     def register(self, session: Session) -> None:
         to_notify: list[asyncio.Future[Session]] = []
-        with self._lock:
-            self._sessions[session.session_id] = session
-            if self._active_session_id is None:
-                self._active_session_id = session.session_id
-            # Notify any waiters blocked on wait_for_session()
-            remaining = []
-            for future, exclude_id, known_ids, project_path in self._session_waiters:
-                if future.done():
-                    continue
-                if not self._matches_wait_criteria(
-                    session,
-                    exclude_id=exclude_id,
-                    known_ids=known_ids,
-                    project_path=project_path,
-                ):
-                    remaining.append((future, exclude_id, known_ids, project_path))
-                    continue
-                to_notify.append(future)
-            self._session_waiters = remaining
+        self._sessions[session.session_id] = session
+        if self._active_session_id is None:
+            self._active_session_id = session.session_id
+        remaining = []
+        for future, exclude_id, known_ids, project_path in self._session_waiters:
+            if future.done():
+                continue
+            if not self._matches_wait_criteria(
+                session,
+                exclude_id=exclude_id,
+                known_ids=known_ids,
+                project_path=project_path,
+            ):
+                remaining.append((future, exclude_id, known_ids, project_path))
+                continue
+            to_notify.append(future)
+        self._session_waiters = remaining
 
         for future in to_notify:
             if not future.done():
@@ -119,41 +119,35 @@ def unregister(self, session_id: str) -> None:
         ## "routing by registration order" bug. Clear active instead; the
         ## next register() or an explicit session_activate will set it.
         should_log = False
-        with self._lock:
-            self._sessions.pop(session_id, None)
-            if self._active_session_id == session_id:
-                self._active_session_id = None
-                should_log = True
+        self._sessions.pop(session_id, None)
+        if self._active_session_id == session_id:
+            self._active_session_id = None
+            should_log = True
         if should_log:
             logger.info(
                 "Active session %s disconnected; no active session until next register/activate",
                 session_id[:8],
             )
 
     def get(self, session_id: str) -> Session | None:
-        with self._lock:
-            return self._sessions.get(session_id)
+        return self._sessions.get(session_id)
 
     def get_active(self) -> Session | None:
-        with self._lock:
-            if self._active_session_id:
-                return self._sessions.get(self._active_session_id)
-            return None
+        if self._active_session_id:
+            return self._sessions.get(self._active_session_id)
+        return None
 
     def set_active(self, session_id: str) -> None:
-        with self._lock:
-            if session_id not in self._sessions:
-                raise KeyError(f"Session {session_id} not found")
-            self._active_session_id = session_id
+        if session_id not in self._sessions:
+            raise KeyError(f"Session {session_id} not found")
+        self._active_session_id = session_id
 
     def list_all(self) -> list[Session]:
-        with self._lock:
-            return list(self._sessions.values())
+        return list(self._sessions.values())
 
     @property
     def active_session_id(self) -> str | None:
-        with self._lock:
-            return self._active_session_id
+        return self._active_session_id
 
     async def wait_for_session(
         self,
@@ -166,35 +160,31 @@ async def wait_for_session(
         """Block until a new session registers (optionally excluding one ID).
 
         If ``known_ids`` is provided, sessions registered after that snapshot but
-        before this waiter is installed are returned under the same registry lock.
+        before this waiter is installed are returned synchronously without yielding.
         Raises TimeoutError if no matching session appears within timeout.
         """
         loop = asyncio.get_running_loop()
-        with self._lock:
-            known_ids_frozen = (
-                frozenset(self._sessions) if known_ids is None else frozenset(known_ids)
-            )
-            existing = self._find_matching_session_locked(
-                exclude_id=exclude_id,
-                known_ids=known_ids_frozen,
-                project_path=project_path,
-            )
-            if existing is not None:
-                return existing
-            future: asyncio.Future[Session] = loop.create_future()
-            entry = (future, exclude_id, known_ids_frozen, project_path)
-            self._session_waiters.append(entry)
+        known_ids_frozen = frozenset(self._sessions) if known_ids is None else frozenset(known_ids)
+        existing = self._find_matching_session(
+            exclude_id=exclude_id,
+            known_ids=known_ids_frozen,
+            project_path=project_path,
+        )
+        if existing is not None:
+            return existing
+        future: asyncio.Future[Session] = loop.create_future()
+        entry = (future, exclude_id, known_ids_frozen, project_path)
+        self._session_waiters.append(entry)
         try:
             return await asyncio.wait_for(future, timeout=timeout)
         except asyncio.TimeoutError:
             raise TimeoutError("Timed out waiting for new session") from None
         finally:
-            with self._lock:
-                self._session_waiters = [w for w in self._session_waiters if w is not entry]
+            self._session_waiters = [w for w in self._session_waiters if w is not entry]
             if not future.done():
                 future.cancel()
 
-    def _find_matching_session_locked(
+    def _find_matching_session(
         self,
         *,
         exclude_id: str | None,
@@ -228,5 +218,4 @@ def _matches_wait_criteria(
         return True
 
     def __len__(self) -> int:
-        with self._lock:
-            return len(self._sessions)
+        return len(self._sessions)

tests/unit/test_session_registry.py

@@ -124,6 +124,20 @@ def test_server_launch_mode_round_trips_through_to_dict(self):
         assert s.to_dict()["server_launch_mode"] == "dev_venv"
 
 
+class TestSessionRegistryNoThreadingLock:
+    """Guard against reintroducing a threading lock on the registry."""
+
+    def test_registry_has_no_lock_attribute(self):
+        reg = SessionRegistry()
+        assert not hasattr(reg, "_lock")
+
+    def test_registry_module_does_not_import_threading(self):
+        import godot_ai.sessions.registry as registry_module
+
+        assert "threading" not in vars(registry_module)
+        assert "RLock" not in vars(registry_module)
+
+
 class TestSessionMetadata:
     def test_name_derived_from_project_path(self):
         s = _make_session(project_path="/Users/me/projects/my_game/")
@@ -221,6 +235,22 @@ async def test_rechecks_registered_replacement_before_installing_waiter(self):
         assert result is replacement
         assert reg._session_waiters == []
 
+    async def test_register_skips_waiters_whose_futures_are_already_done(self):
+        ## A waiter whose future was resolved or cancelled out-of-band must
+        ## not be re-resolved on the next register(); its entry stays in
+        ## _session_waiters until its own finally-block evicts it. The
+        ## register loop's `if future.done(): continue` path covers this.
+        reg = SessionRegistry()
+        loop = asyncio.get_running_loop()
+        cancelled_future: asyncio.Future[Session] = loop.create_future()
+        cancelled_future.cancel()
+        reg._session_waiters.append((cancelled_future, None, frozenset(), None))
+
+        reg.register(_make_session("a"))
+
+        assert cancelled_future.cancelled()
+        assert len(reg) == 1
+
     async def test_concurrent_registers_and_activate_keep_registry_consistent(self):
         reg = SessionRegistry()
         waiter_task = asyncio.create_task(reg.wait_for_session(exclude_id="a", timeout=1.0))