fix(auth): serialize keyring migration under a reentrant credential lock

load_tokens()'s keyring read-copy-delete migration ran outside
_credential_transaction_locks, so it could race persist_login/persist_logout
and overwrite a newer file token with the older keyring value. Serialize it
under the credential lock. The lock is now reentrant per thread (fcntl.flock /
msvcrt.locking deny a second same-file acquisition within one process), so
callers already inside a credential transaction — e.g. persist_login loading
the previous token at the same key (oauth.py:804) — re-enter instead of
self-deadlocking. Adds reentrancy and migration-under-held-lock regression tests.

Author: elkaix

src/pythinker_code/auth/oauth.py

@@ -641,7 +641,13 @@ def _migrate_keyring_token(ref: OAuthRef) -> OAuthToken | None:
 
 def load_tokens(ref: OAuthRef) -> OAuthToken | None:
     if ref.storage == "keyring":
-        return _migrate_keyring_token(ref)
+        # Serialize the read-copy-delete keyring migration with credential writes
+        # so it cannot race persist_login/persist_logout and overwrite a newer
+        # file token with the older keyring value. The lock is reentrant per
+        # thread, so callers already inside a credential transaction (e.g.
+        # persist_login loading the previous token) do not deadlock.
+        with _credential_transaction_locks([ref.key]):
+            return _migrate_keyring_token(ref)
     return _load_from_file(ref.key)
 
 
@@ -719,11 +725,36 @@ def _config_transaction_lock() -> Generator[None]:
         ) from exc
 
 
+# Per-thread reentrancy guard for the credential lock. fcntl.flock (Unix) and
+# msvcrt.locking (Windows) deny a second acquisition of the same lock file within
+# one process, so a thread that already holds a key's lock must reference that
+# hold instead of re-acquiring it — otherwise nested acquisitions (e.g.
+# load_tokens() migrating a keyring credential while inside a persist transaction)
+# would self-deadlock.
+_HELD_CREDENTIAL_KEYS = threading.local()
+
+
+def _held_credential_key_counts() -> dict[str, int]:
+    counts: dict[str, int] | None = getattr(_HELD_CREDENTIAL_KEYS, "counts", None)
+    if counts is None:
+        counts = {}
+        _HELD_CREDENTIAL_KEYS.counts = counts
+    return counts
+
+
 @contextmanager
 def _credential_transaction_locks(keys: list[str]) -> Generator[None]:
+    held = _held_credential_key_counts()
+    bumped: list[str] = []
     locks: list[_CrossProcessLock] = []
     try:
         for key in sorted(set(keys)):
+            if held.get(key, 0) > 0:
+                # Reentrant: this thread already holds `key`'s cross-process lock;
+                # re-acquiring the same file lock would be denied by the OS.
+                held[key] += 1
+                bumped.append(key)
+                continue
             try:
                 lock = _CrossProcessLock(key)
                 acquired = lock.acquire_with_retry_sync()
@@ -736,8 +767,18 @@ def _credential_transaction_locks(keys: list[str]) -> Generator[None]:
                     "Could not acquire the OAuth credential lock; no changes were made."
                 )
             locks.append(lock)
+            held[key] = 1
+            bumped.append(key)
         yield
     finally:
+        # Decrement only what this invocation bumped (correct even on a partial
+        # acquisition), and release only the OS locks it actually acquired.
+        for key in bumped:
+            count = held.get(key, 0)
+            if count <= 1:
+                held.pop(key, None)
+            else:
+                held[key] = count - 1
         for lock in reversed(locks):
             lock.release()
 

tests/auth/test_oauth_persist.py

@@ -511,6 +511,49 @@ async def test_replacement_with_same_credential_key_keeps_new_token(
     assert caller == committed
 
 
+def test_credential_transaction_locks_are_reentrant_same_thread(tmp_path, monkeypatch) -> None:
+    """A thread already holding a credential key's lock can re-enter it without
+    deadlocking — fcntl.flock / msvcrt.locking deny a second same-file
+    acquisition within one process, so reentrancy is tracked in Python. Guards
+    the keyring-migration self-deadlock (load_tokens inside a persist transaction).
+    """
+    monkeypatch.setenv("PYTHINKER_SHARE_DIR", str(tmp_path))
+    key = "oauth/reentrant-provider"
+    counts = oauth._held_credential_key_counts()
+    assert counts.get(key, 0) == 0
+    with oauth._credential_transaction_locks([key]):
+        assert counts.get(key, 0) == 1
+        with oauth._credential_transaction_locks([key]):
+            # Reentrant no-op acquisition; must not block on the same file lock.
+            assert counts.get(key, 0) == 2
+        # Inner exit decrements but the outer hold (and OS lock) remains.
+        assert counts.get(key, 0) == 1
+    assert counts.get(key, 0) == 0
+
+
+def test_load_tokens_keyring_migration_reentrant_under_held_lock(tmp_path, monkeypatch) -> None:
+    """The keyring read-copy-delete migration in load_tokens() is serialized under
+    the credential lock (so it cannot race persist), and re-enters safely when the
+    caller already holds that key's lock (the persist_login -> load_tokens path).
+    Pre-fix this path self-deadlocked on the non-reentrant cross-process lock.
+    """
+    monkeypatch.setenv("PYTHINKER_SHARE_DIR", str(tmp_path))
+    key = "oauth/keyring-migrate"
+    migrated_token = _token("keyring-access", "keyring-refresh")
+    monkeypatch.setattr(oauth, "_load_from_keyring", lambda _k: migrated_token)
+    deleted: list[str] = []
+    monkeypatch.setattr(oauth, "_delete_from_keyring", deleted.append)
+    ref = OAuthRef(storage="keyring", key=key)
+
+    with oauth._credential_transaction_locks([key]):
+        result = oauth.load_tokens(ref)  # would self-deadlock without reentrancy
+
+    assert result == migrated_token
+    assert deleted == [key]  # keyring entry removed as part of the migration
+    # Token now lives on the authoritative file copy.
+    assert oauth.load_tokens(OAuthRef(storage="file", key=key)) == migrated_token
+
+
 @pytest.mark.asyncio
 async def test_old_token_rollback_failure_preserves_committed_new_pair(
     monkeypatch: pytest.MonkeyPatch, tmp_path