From c069ffb8072d4c181c7900bf5d8d72d3ef13fe62 Mon Sep 17 00:00:00 2001 From: Lucas Winther Date: Mon, 28 Sep 2026 02:45:59 +0200 Subject: [PATCH] fix: preserve errors and reject nonregular sources --- docs/DECISIONS.md | 27 +++++++++ src/recoll_modern/domain/errors.py | 2 +- src/recoll_modern/indexing/discovery.py | 10 +++- src/recoll_modern/indexing/paths.py | 2 +- tests/indexing/test_discovery.py | 75 +++++++++++++++++++++++-- tests/storage/test_database.py | 16 ++++++ 6 files changed, 124 insertions(+), 8 deletions(-) diff --git a/docs/DECISIONS.md b/docs/DECISIONS.md index 047d0fa..1c6ddb7 100644 --- a/docs/DECISIONS.md +++ b/docs/DECISIONS.md @@ -3,6 +3,33 @@ Append-only, newest first. Never edit or delete an entry — supersede it with a new one that links back. One entry per decision a future maintainer would otherwise have to reverse-engineer. +## 2026-09-28 — Reject non-regular explicit sources before opening +**Status:** accepted +**Context:** Directory discovery filters non-regular entries, but an explicit file source could name +a FIFO. Opening that path for hashing blocks while waiting for a writer. +**Decision:** open explicit sources with `O_NONBLOCK` and `O_NOFOLLOW`, inspect the opened descriptor's +file mode, and return `file_hash_failed` for non-regular sources. Retain the existing before/after +metadata checks for content changes. +**Alternatives:** rely on directory discovery alone (explicit sources bypass it); open every path first +(can block on a FIFO). +**Consequences:** an explicit FIFO, including one swapped into place immediately before opening, +is reported as a file error without blocking the worker. + +--- + +## 2026-09-28 — Keep exception dataclasses mutable for transaction propagation +**Status:** accepted +**Context:** Python's generator-based context managers assign `__traceback__` while re-raising an +exception. Frozen `AppError` and `PathPolicyError` dataclasses rejected that assignment, replacing an +expected application error with `TypeError` when it crossed `Database.transaction()`. +**Decision:** retain `slots=True` but remove `frozen=True` from both exception classes. +**Alternatives:** move all error checks outside transactions (introduces races); wrap every exception +at each transaction boundary (duplicates handling and can change error identity). +**Consequences:** the original application or path error survives rollback and reaches its caller. +The transaction regression test covers both exception types. + +--- + ## 2026-09-28 — Guard generated API documentation with the service session **Status:** accepted **Context:** FastAPI exposes generated OpenAPI and documentation routes outside ordinary route diff --git a/src/recoll_modern/domain/errors.py b/src/recoll_modern/domain/errors.py index 75a5fda..b4aa405 100644 --- a/src/recoll_modern/domain/errors.py +++ b/src/recoll_modern/domain/errors.py @@ -4,7 +4,7 @@ from collections.abc import Mapping from dataclasses import dataclass, field -@dataclass(frozen=True, slots=True) +@dataclass(slots=True) class AppError(Exception): """A stable, safe error contract for expected application failures.""" diff --git a/src/recoll_modern/indexing/discovery.py b/src/recoll_modern/indexing/discovery.py index ae5d4e9..5c05268 100644 --- a/src/recoll_modern/indexing/discovery.py +++ b/src/recoll_modern/indexing/discovery.py @@ -2,6 +2,7 @@ import hashlib import os +import stat from dataclasses import dataclass from os import stat_result from pathlib import Path @@ -64,8 +65,15 @@ class DiscoveryReport: def hash_file_stable(path: Path) -> HashResult: """Hash a file and reject a concurrent metadata change.""" try: - with path.open("rb") as stream: + with open( + path, "rb", + opener=lambda name, flags: os.open( + name, flags | os.O_NONBLOCK | getattr(os, "O_NOFOLLOW", 0) + ), + ) as stream: before = os.fstat(stream.fileno()) + if not stat.S_ISREG(before.st_mode): + return HashResult(code="file_hash_failed") digest = hashlib.sha256() while chunk := stream.read(1024 * 1024): digest.update(chunk) diff --git a/src/recoll_modern/indexing/paths.py b/src/recoll_modern/indexing/paths.py index 6d0e5eb..44950e4 100644 --- a/src/recoll_modern/indexing/paths.py +++ b/src/recoll_modern/indexing/paths.py @@ -5,7 +5,7 @@ from pathlib import Path, PurePosixPath from unicodedata import normalize -@dataclass(frozen=True, slots=True) +@dataclass(slots=True) class PathPolicyError(Exception): code: str diff --git a/tests/indexing/test_discovery.py b/tests/indexing/test_discovery.py index 0186fd9..32a60b1 100644 --- a/tests/indexing/test_discovery.py +++ b/tests/indexing/test_discovery.py @@ -2,6 +2,8 @@ import hashlib import os from pathlib import Path +import pytest + from recoll_modern.indexing.discovery import Source, discover_sources, hash_file_stable from recoll_modern.indexing.paths import PathPolicy @@ -63,14 +65,14 @@ def test_unreadable_file_reports_error(tmp_path: Path, monkeypatch) -> None: # root = tmp_path / "docs" root.mkdir() (root / "a.txt").write_text("a") - real_open = Path.open + real_open = os.open - def denied(path: Path, *args: object, **kwargs: object): # type: ignore[no-untyped-def] - if path.name == "a.txt": + def denied(path: str, flags: int, mode: int = 0o777, *, dir_fd: int | None = None) -> int: + if Path(path).name == "a.txt": raise PermissionError("denied") - return real_open(path, *args, **kwargs) # type: ignore[call-overload] + return real_open(path, flags, mode, dir_fd=dir_fd) - monkeypatch.setattr(Path, "open", denied) + monkeypatch.setattr(os, "open", denied) report = discover_sources(PathPolicy(root), [Source.file("a.txt")]) assert [error.code for error in report.errors] == ["file_unreadable"] @@ -99,3 +101,66 @@ def test_discovery_hashes_file_bytes(tmp_path: Path) -> None: report = discover_sources(PathPolicy(tmp_path), [Source.file("a.txt")]) assert report.files[0].content_hash == hashlib.sha256(b"hello").hexdigest() assert report.files[0].byte_size == 5 + + +def test_explicit_fifo_is_rejected_before_opening( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + fifo = tmp_path / "pipe" + os.mkfifo(fifo) + real_open = Path.open + + def guarded_open(path: Path, *args: object, **kwargs: object): # type: ignore[no-untyped-def] + if path == fifo: + raise AssertionError("FIFO must not be opened") + return real_open(path, *args, **kwargs) # type: ignore[call-overload] + + monkeypatch.setattr(Path, "open", guarded_open) + report = discover_sources(PathPolicy(tmp_path), [Source.file("pipe")]) + assert report.files == () + assert [error.code for error in report.errors] == ["file_hash_failed"] + + +def test_file_swapped_for_fifo_during_open_cannot_block( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + path = tmp_path / "source.txt" + path.write_text("regular") + real_stat = Path.stat + real_os_open = os.open + real_path_open = Path.open + swapped = False + + def swap() -> None: + nonlocal swapped + if not swapped: + path.unlink() + os.mkfifo(path) + swapped = True + + def stat_then_swap(target: Path, *args: object, **kwargs: object): # type: ignore[no-untyped-def] + result = real_stat(target, *args, **kwargs) # type: ignore[arg-type] + if target == path: + swap() + return result + + def guarded_path_open(target: Path, *args: object, **kwargs: object): # type: ignore[no-untyped-def] + if target == path: + raise AssertionError("blocking Path.open was used") + return real_path_open(target, *args, **kwargs) # type: ignore[call-overload] + + def swap_before_fd_open( + name: str, flags: int, mode: int = 0o777, *, dir_fd: int | None = None + ) -> int: + if str(name) == str(path): + swap() + assert flags & os.O_NONBLOCK + return real_os_open(name, flags, mode, dir_fd=dir_fd) + + monkeypatch.setattr(Path, "stat", stat_then_swap) + monkeypatch.setattr(Path, "open", guarded_path_open) + monkeypatch.setattr(os, "open", swap_before_fd_open) + report = discover_sources(PathPolicy(tmp_path), [Source.file("source.txt")]) + assert swapped + assert report.files == () + assert [error.code for error in report.errors] == ["file_hash_failed"] diff --git a/tests/storage/test_database.py b/tests/storage/test_database.py index 5badc94..06b40bd 100644 --- a/tests/storage/test_database.py +++ b/tests/storage/test_database.py @@ -5,6 +5,8 @@ from pathlib import Path import pytest +from recoll_modern.domain.errors import AppError +from recoll_modern.indexing.paths import PathPolicyError from recoll_modern.storage.database import Database @@ -69,3 +71,17 @@ def test_transaction_rolls_back_and_closes_connection(database: Database) -> Non assert transaction_connection is not None with pytest.raises(sqlite3.ProgrammingError): transaction_connection.execute("SELECT 1") + + +@pytest.mark.parametrize( + "error", + [AppError("example", "Expected error", 422), PathPolicyError("invalid_source")], +) +def test_transaction_preserves_application_exceptions( + database: Database, error: Exception +) -> None: + database.migrate() + with pytest.raises(type(error)) as caught: + with database.transaction(): + raise error + assert caught.value is error