fix: make _clear_cookies safe against concurrent jar mutation

- skip None entries yielded by py<=3.10 http.cookiejar.deepvalues
- tolerate KeyError when a peer removed the cookie before clear()
- retarget the concurrency test at _clear_cookies, not stdlib jar reads
- add deterministic tests covering both guards

Author: LukasGold

src/osw/wtsite.py

@@ -498,10 +498,21 @@ def _clear_cookies(self):
         # hold the session lock so a peer thread cannot clear/re-login concurrently.
         with self._get_session_lock():
             for cookie in list(self._site.connection.cookies):
+                # The session lock does not cover requests' own jar writes:
+                # extract_cookies() drops any cookie whose Set-Cookie expiry is in
+                # the past, on every response. On py<=3.10 http.cookiejar iterates
+                # via a lazy map(dict.get, sorted(keys)), so such a delete makes the
+                # snapshot yield None instead of a cookie.
+                if cookie is None:
+                    continue
                 if "PostEditRevision" in cookie.name:
-                    self._site.connection.cookies.clear(
-                        cookie.domain, cookie.path, cookie.name
-                    )
+                    try:
+                        self._site.connection.cookies.clear(
+                            cookie.domain, cookie.path, cookie.name
+                        )
+                    except KeyError:
+                        # already removed between snapshot and delete
+                        pass
 
     class SearchParam(wt.SearchParam):
         pass

tests/test_wtsite_session_lock.py

@@ -14,14 +14,19 @@
 from osw.wtsite import WtPage, WtSite
 
 
-def _make_wtsite_with_jar():
-    """Build a minimal WtSite carrying a real cookie jar, bypassing __init__."""
-    jar = rc.RequestsCookieJar()
+def _make_wtsite_for(jar):
+    """Wrap an arbitrary cookie jar in a minimal WtSite, bypassing __init__."""
     fake_site = types.SimpleNamespace(connection=types.SimpleNamespace(cookies=jar))
     ws = WtSite.__new__(WtSite)
     ws._session_lock = threading.RLock()
     ws._site = fake_site
-    return ws, jar
+    return ws
+
+
+def _make_wtsite_with_jar():
+    """Build a minimal WtSite carrying a real cookie jar, bypassing __init__."""
+    jar = rc.RequestsCookieJar()
+    return _make_wtsite_for(jar), jar
 
 
 def _populate(jar, n=25):
@@ -40,51 +45,123 @@ def test_clear_cookies_removes_only_post_edit_revision():
     assert {c.name for c in jar} == {"sessionToken"}
 
 
+def _fake_cookie(name):
+    return types.SimpleNamespace(name=name, domain="example.org", path="/")
+
+
+class _FakeJar:
+    """Stands in for a jar being mutated by requests while we iterate it."""
+
+    def __init__(self, entries):
+        self._entries = entries
+        self.cleared = []
+
+    def __iter__(self):
+        return iter(self._entries)
+
+    def clear(self, domain, path, name):
+        self.cleared.append((domain, path, name))
+
+
+def test_clear_cookies_skips_none_entries_from_the_jar():
+    """py<=3.10 http.cookiejar yields None for a cookie deleted mid-iteration.
+
+    Its deepvalues() walks a lazy map(dict.get, sorted(keys)), so a concurrent
+    delete turns into a None rather than a missing entry. Without the guard this
+    raises AttributeError: 'NoneType' object has no attribute 'name'.
+    """
+    jar = _FakeJar([None, _fake_cookie("PostEditRevision1"), None])
+    ws = _make_wtsite_for(jar)
+
+    ws._clear_cookies()
+
+    assert jar.cleared == [("example.org", "/", "PostEditRevision1")]
+
+
+def test_clear_cookies_tolerates_a_cookie_already_removed():
+    """CookieJar.clear raises KeyError if a peer deleted the cookie first.
+
+    _clear_cookies works off a snapshot, so any entry may be gone by the time it
+    calls clear(). It must swallow that and keep going, not abort the sweep.
+    """
+
+    class _RacingJar(_FakeJar):
+        def clear(self, domain, path, name):
+            super().clear(domain, path, name)
+            raise KeyError(name)
+
+    jar = _RacingJar([
+        _fake_cookie("PostEditRevision1"),
+        _fake_cookie("PostEditRevision2"),
+    ])
+    ws = _make_wtsite_for(jar)
+
+    ws._clear_cookies()
+
+    # the KeyError on the first cookie must not stop the second from being tried
+    assert len(jar.cleared) == 2
+
+
 def test_clear_cookies_under_concurrency_is_safe():
-    """Concurrent clears + reads must not raise and must preserve unrelated cookies."""
+    """_clear_cookies must survive jar writes the session lock does not cover.
+
+    requests' own extract_cookies() runs on every response and drops any cookie
+    whose Set-Cookie expiry is in the past, taking only the jar's internal lock.
+    So it races _clear_cookies two ways: the delete can land between the
+    snapshot and the follow-up clear() (KeyError, any version), and on py<=3.10
+    http.cookiejar iterates via a lazy map(dict.get, sorted(keys)), so the
+    snapshot yields None instead of the deleted cookie.
+
+    Only _clear_cookies is asserted on. Unsynchronized iteration of the jar is
+    not something WtSite can make safe, since requests never takes the session
+    lock, so the reader here is a contention generator and nothing more.
+    """
     ws, jar = _make_wtsite_with_jar()
     _populate(jar)
 
-    errors = []
+    clear_errors = []
     stop = threading.Event()
 
+    def peer_writer():
+        """Stand in for requests.extract_cookies: set + delete, no session lock."""
+        while not stop.is_set():
+            for i in range(25):
+                jar.set(f"PostEditRevision{i}", f"v{i}", domain="example.org", path="/")
+            for i in range(25):
+                try:
+                    jar.clear("example.org", "/", f"PostEditRevision{i}")
+                except KeyError:
+                    pass
+
     def clearer():
         try:
             for _ in range(200):
-                # re-add under the lock so there is always something to clear,
-                # creating real contention with the concurrent _clear_cookies calls
-                with ws._session_lock:
-                    for i in range(25):
-                        jar.set(
-                            f"PostEditRevision{i}",
-                            f"v{i}",
-                            domain="example.org",
-                            path="/",
-                        )
                 ws._clear_cookies()
         except Exception as exc:
-            errors.append(exc)
+            clear_errors.append(exc)
 
     def reader():
-        try:
-            while not stop.is_set():
-                _ = [c.name for c in list(jar)]
-        except Exception as exc:
-            errors.append(exc)
-
+        while not stop.is_set():
+            try:
+                _ = [c.name for c in list(jar) if c is not None]
+            except RuntimeError:
+                # dict resized mid-iteration; the stdlib jar is not thread-safe
+                pass
+
+    peers = [threading.Thread(target=peer_writer, daemon=True) for _ in range(2)]
+    readers = [threading.Thread(target=reader, daemon=True) for _ in range(2)]
     clearers = [threading.Thread(target=clearer) for _ in range(4)]
-    readers = [threading.Thread(target=reader) for _ in range(2)]
-    for t in readers:
+    for t in peers + readers:
         t.start()
     for t in clearers:
         t.start()
     for t in clearers:
         t.join()
     stop.set()
-    for t in readers:
+    for t in peers + readers:
         t.join()
 
-    assert errors == []
+    assert clear_errors == []
     # the non-PostEditRevision cookie must survive every clear
     assert "sessionToken" in {c.name for c in jar}