diff --git a/strix/config/subscription_store.py b/strix/config/subscription_store.py index 3e68d205..148e8871 100644 --- a/strix/config/subscription_store.py +++ b/strix/config/subscription_store.py @@ -28,6 +28,14 @@ if TYPE_CHECKING: from pathlib import Path +class StoreLockError(RuntimeError): + """The cross-process store lock could not be acquired. + + Raised instead of silently proceeding, so a read-modify-write never runs + unlocked (which would let concurrent provider logins/refreshes/logouts race). + """ + + def read(path: Path) -> dict[str, Any]: """The store's contents, or an empty dict when absent/unreadable.""" try: @@ -79,6 +87,7 @@ class _StoreLock: def _release_flock(self) -> None: handle = self._flock_handle + self._flock_handle = None if handle is None: return try: @@ -90,7 +99,6 @@ class _StoreLock: pass finally: handle.close() - self._flock_handle = None _store_lock = _StoreLock() @@ -101,17 +109,30 @@ def guard(path: Path) -> contextlib.AbstractContextManager[None]: return _store_lock.hold(path) -def _acquire_flock(path: Path) -> TextIOWrapper | None: +def _acquire_flock(path: Path) -> TextIOWrapper: + """Hold an exclusive cross-process lock on the store, or raise. + + Never returns without the lock held: a missing ``fcntl`` or a failed + ``flock`` raises :class:`StoreLockError` so the caller aborts rather than + mutating the store unlocked. + """ try: import fcntl - except ImportError: - return None + except ImportError as exc: # pragma: no cover - non-POSIX + msg = "cross-process credential locking requires fcntl (a POSIX platform)" + raise StoreLockError(msg) from exc lock_path = path.with_suffix(".lock") + lock_path.parent.mkdir(parents=True, exist_ok=True) + handle = lock_path.open("w") try: - lock_path.parent.mkdir(parents=True, exist_ok=True) - handle = lock_path.open("w") - except OSError: - return None - with contextlib.suppress(OSError): - fcntl.flock(handle.fileno(), fcntl.LOCK_EX) + while True: + try: + fcntl.flock(handle.fileno(), fcntl.LOCK_EX) + break + except InterruptedError: # EINTR — retry the blocking acquire + continue + except OSError as exc: + handle.close() + msg = f"could not lock {lock_path}: {exc}" + raise StoreLockError(msg) from exc return handle diff --git a/tests/test_subscription_store.py b/tests/test_subscription_store.py index 522c8fcb..b6b83b1f 100644 --- a/tests/test_subscription_store.py +++ b/tests/test_subscription_store.py @@ -2,17 +2,18 @@ from __future__ import annotations +import fcntl import stat from typing import TYPE_CHECKING +import pytest + from strix.config import codex, grok, subscription_store if TYPE_CHECKING: from pathlib import Path - import pytest - def test_write_creates_owner_only_file(tmp_path: Path) -> None: path = tmp_path / ".strix" / "subscription-auth.json" @@ -53,3 +54,21 @@ def test_guard_is_reentrant(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> record = grok.read_record() assert record is not None assert record["access"] == "g" + + +def test_mutation_aborts_when_lock_cannot_be_acquired( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + store = tmp_path / ".strix" / "subscription-auth.json" + monkeypatch.setattr(grok, "AUTH_PATH", store) + + def _no_lock(*_args: object, **_kwargs: object) -> None: + raise OSError("no locks available") + + monkeypatch.setattr(fcntl, "flock", _no_lock) + + # Rather than silently doing an unlocked read-modify-write, the store raises + # and writes nothing. + with pytest.raises(subscription_store.StoreLockError): + grok.save_record({"type": "oauth", "access": "g", "refresh": "r"}) + assert not store.exists()