fix(pairing): keep approvals across transient store read failures

_load() treated any OSError like corruption and returned an empty store. When pairing.json was transiently unreadable, an unapproved DM could deny the sender, generate a pairing code from the empty view, and overwrite the store without its approved senders.

Keep the existing JSONDecodeError reset behavior, but propagate OSError so mutations cannot persist unreadable state. Read-only checks fail closed without writing; mutating /pairing subcommands report temporary unavailability; and the DM pairing path skips one reply instead of crashing the handler.

This mirrors the refuse-to-overwrite strategy used by the cron and trigger stores.
This commit is contained in:
KDB 2026-07-28 14:37:10 +08:00 committed by Xubin Ren
parent e633f867e8
commit 52680dbe19
4 changed files with 144 additions and 5 deletions

View File

@ -248,7 +248,15 @@ class BaseChannel(ABC):
permission_id = authorization_id if authorization_id is not None else sender_id permission_id = authorization_id if authorization_id is not None else sender_id
if not self.is_allowed(permission_id): if not self.is_allowed(permission_id):
if is_dm: 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( await self.send(
OutboundMessage( OutboundMessage(
channel=self.name, channel=self.name,

View File

@ -40,9 +40,15 @@ def _load() -> dict[str, Any]:
data = json.load(f) data = json.load(f)
except FileNotFoundError: except FileNotFoundError:
return {"approved": {}, "pending": {}} return {"approved": {}, "pending": {}}
except (json.JSONDecodeError, OSError): except json.JSONDecodeError:
logger.warning("Corrupted pairing store, resetting") logger.warning("Corrupted pairing store, resetting")
return {"approved": {}, "pending": {}} 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): if not isinstance(data, dict):
logger.warning("Corrupted pairing store, resetting") logger.warning("Corrupted pairing store, resetting")
return {"approved": {}, "pending": {}} return {"approved": {}, "pending": {}}
@ -171,7 +177,11 @@ def deny_code(code: str) -> bool:
def is_approved(channel: str, sender_id: str) -> bool: def is_approved(channel: str, sender_id: str) -> bool:
"""Check whether *sender_id* has been approved on *channel*.""" """Check whether *sender_id* has been approved on *channel*."""
with _LOCK: 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", {}) approved: dict[str, set[str]] = data.get("approved", {})
return str(sender_id) in approved.get(channel, set()) 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]]: def list_pending() -> list[dict[str, Any]]:
"""Return all non-expired pending pairing requests.""" """Return all non-expired pending pairing requests."""
with _LOCK: with _LOCK:
data = _load() try:
data = _load()
except OSError:
return []
_gc_pending(data) _gc_pending(data)
return [ return [
{"code": code, **info} {"code": code, **info}
@ -257,7 +270,10 @@ def clear_channel(channel: str) -> dict[str, int]:
def get_approved(channel: str) -> list[str]: def get_approved(channel: str) -> list[str]:
"""Return all approved sender IDs for *channel*.""" """Return all approved sender IDs for *channel*."""
with _LOCK: with _LOCK:
data = _load() try:
data = _load()
except OSError:
return []
return sorted(data.get("approved", {}).get(channel, set())) 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) 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. 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() parts = subcommand_text.split()
sub = parts[0] if parts else "list" sub = parts[0] if parts else "list"
arg = parts[1] if len(parts) > 1 else None arg = parts[1] if len(parts) > 1 else None

View File

@ -83,6 +83,49 @@ async def test_handle_message_dm_sends_pairing_code(monkeypatch) -> None:
assert msg.metadata.get("_pairing_code") == "ABCD-EFGH" 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 @pytest.mark.asyncio
async def test_handle_message_group_ignores_unknown() -> None: async def test_handle_message_group_ignores_unknown() -> None:
channel = _DummyChannel({"allowFrom": []}, MessageBus()) channel = _DummyChannel({"allowFrom": []}, MessageBus())

View File

@ -323,3 +323,66 @@ def test_pending_gc_drops_malformed_entries(tmp_path, monkeypatch):
) )
monkeypatch.setattr(store, "_store_path", lambda: path) monkeypatch.setattr(store, "_store_path", lambda: path)
assert store.list_pending() == [] 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")