diff --git a/nanobot/channels/base.py b/nanobot/channels/base.py index aed1407ec..1784d6671 100644 --- a/nanobot/channels/base.py +++ b/nanobot/channels/base.py @@ -248,7 +248,15 @@ class BaseChannel(ABC): permission_id = authorization_id if authorization_id is not None else sender_id if not self.is_allowed(permission_id): if is_dm: - code = generate_code(self.name, str(sender_id)) + try: + code = generate_code(self.name, str(sender_id)) + except OSError: + # Transient pairing-store I/O failure: skip the pairing + # reply for this message rather than crash the handler. + self.logger.warning( + "Pairing store unavailable; dropping DM from {}", sender_id + ) + return await self.send( OutboundMessage( channel=self.name, diff --git a/nanobot/pairing/store.py b/nanobot/pairing/store.py index 38253368a..11d5ca787 100644 --- a/nanobot/pairing/store.py +++ b/nanobot/pairing/store.py @@ -40,9 +40,15 @@ def _load() -> dict[str, Any]: data = json.load(f) except FileNotFoundError: return {"approved": {}, "pending": {}} - except (json.JSONDecodeError, OSError): + except json.JSONDecodeError: logger.warning("Corrupted pairing store, resetting") return {"approved": {}, "pending": {}} + except OSError: + # A transiently locked or busy file is not corruption. Propagate so + # mutating callers fail loudly instead of persisting an empty view + # that would erase every approved sender. + logger.warning("Pairing store temporarily unreadable: {}", path) + raise if not isinstance(data, dict): logger.warning("Corrupted pairing store, resetting") return {"approved": {}, "pending": {}} @@ -171,7 +177,11 @@ def deny_code(code: str) -> bool: def is_approved(channel: str, sender_id: str) -> bool: """Check whether *sender_id* has been approved on *channel*.""" with _LOCK: - data = _load() + try: + data = _load() + except OSError: + # Fail closed for this check; the store itself stays untouched. + return False approved: dict[str, set[str]] = data.get("approved", {}) return str(sender_id) in approved.get(channel, set()) @@ -179,7 +189,10 @@ def is_approved(channel: str, sender_id: str) -> bool: def list_pending() -> list[dict[str, Any]]: """Return all non-expired pending pairing requests.""" with _LOCK: - data = _load() + try: + data = _load() + except OSError: + return [] _gc_pending(data) return [ {"code": code, **info} @@ -257,7 +270,10 @@ def clear_channel(channel: str) -> dict[str, int]: def get_approved(channel: str) -> list[str]: """Return all approved sender IDs for *channel*.""" with _LOCK: - data = _load() + try: + data = _load() + except OSError: + return [] return sorted(data.get("approved", {}).get(channel, set())) @@ -283,6 +299,15 @@ def handle_pairing_command(channel: str, subcommand_text: str) -> str: This is a pure function (no side effects other than store mutations) so it can be used from both the CLI and the agent CommandRouter. """ + try: + return _handle_pairing_subcommand(channel, subcommand_text) + except OSError: + # Mutations fail loudly on a transient I/O error instead of lying + # ("invalid code") or silently rewriting the store from an empty view. + return "The pairing store is temporarily unavailable. Please try again." + + +def _handle_pairing_subcommand(channel: str, subcommand_text: str) -> str: parts = subcommand_text.split() sub = parts[0] if parts else "list" arg = parts[1] if len(parts) > 1 else None diff --git a/tests/channels/test_base_channel.py b/tests/channels/test_base_channel.py index 177371a8b..7605c6aeb 100644 --- a/tests/channels/test_base_channel.py +++ b/tests/channels/test_base_channel.py @@ -83,6 +83,49 @@ async def test_handle_message_dm_sends_pairing_code(monkeypatch) -> None: assert msg.metadata.get("_pairing_code") == "ABCD-EFGH" +@pytest.mark.asyncio +async def test_dm_during_transient_store_failure_keeps_approvals( + tmp_path, monkeypatch +) -> None: + """An unapproved DM while pairing.json is unreadable must not wipe approvals. + + The pairing store treated a transient OSError like corruption and returned + an empty store; the DM pairing path then persisted that empty view, + erasing every approved sender. + """ + import builtins + from pathlib import Path + + from nanobot.pairing import store + + path = tmp_path / "pairing.json" + monkeypatch.setattr(store, "_store_path", lambda: path) + code = store.generate_code("dummy", "friend") + store.approve_code(code) + + channel = _DummyChannel({"allowFrom": []}, MessageBus()) + + real_open = builtins.open + + def flaky_open(file, mode="r", *args, **kwargs): + try: + same = Path(file) == path + except TypeError: + same = False + if same and "r" in mode and "+" not in mode: + raise PermissionError(13, "temporarily locked", str(path)) + return real_open(file, mode, *args, **kwargs) + + with monkeypatch.context() as m: + m.setattr(builtins, "open", flaky_open) + await channel._handle_message( + sender_id="stranger", chat_id="chat1", content="hello", is_dm=True + ) + + assert channel._sent == [] + assert store.is_approved("dummy", "friend") is True + + @pytest.mark.asyncio async def test_handle_message_group_ignores_unknown() -> None: channel = _DummyChannel({"allowFrom": []}, MessageBus()) diff --git a/tests/pairing/test_store.py b/tests/pairing/test_store.py index d16f0705c..928782c1e 100644 --- a/tests/pairing/test_store.py +++ b/tests/pairing/test_store.py @@ -323,3 +323,66 @@ def test_pending_gc_drops_malformed_entries(tmp_path, monkeypatch): ) monkeypatch.setattr(store, "_store_path", lambda: path) assert store.list_pending() == [] + + +def _fail_reads_of(monkeypatch, path): + """Make reads of *path* raise like a transiently locked/busy file.""" + import builtins + from pathlib import Path + + real_open = builtins.open + + def flaky_open(file, mode="r", *args, **kwargs): + try: + same = Path(file) == path + except TypeError: + same = False + if same and "r" in mode and "+" not in mode: + raise PermissionError(13, "temporarily locked", str(path)) + return real_open(file, mode, *args, **kwargs) + + monkeypatch.setattr(builtins, "open", flaky_open) + + +class TestTransientReadFailure: + """A transient I/O failure is not corruption and must never wipe the store.""" + + def test_generate_code_does_not_wipe_approvals(self, tmp_path, monkeypatch): + """An unapproved DM during a read blip previously erased every approval. + + _load treated OSError like corruption and returned an empty store; + generate_code then unconditionally saved it, overwriting pairing.json + with no approved senders. + """ + code = store.generate_code("telegram", "123") + store.approve_code(code) + + with monkeypatch.context() as m: + _fail_reads_of(m, store._store_path()) + with pytest.raises(OSError): + store.generate_code("telegram", "stranger") + + assert store.is_approved("telegram", "123") is True + + def test_reads_fail_closed_without_crashing(self, tmp_path, monkeypatch): + code = store.generate_code("telegram", "123") + store.approve_code(code) + + with monkeypatch.context() as m: + _fail_reads_of(m, store._store_path()) + assert store.is_approved("telegram", "123") is False + assert store.list_pending() == [] + assert store.get_approved("telegram") == [] + + assert store.is_approved("telegram", "123") is True + + def test_approve_command_reports_store_unavailable(self, tmp_path, monkeypatch): + """/pairing approve must fail loudly instead of claiming the code is invalid.""" + code = store.generate_code("telegram", "123") + + with monkeypatch.context() as m: + _fail_reads_of(m, store._store_path()) + reply = store.handle_pairing_command("telegram", f"approve {code}") + + assert "unavailable" in reply.lower() + assert store.approve_code(code) == ("telegram", "123")