Archived
fix: preserve errors and reject nonregular sources
This commit is contained in:
1 parent
94effe4f2c
commit
c069ffb807
6 files changed
+124
-8
No files matched your search
@@ -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
|
||||
|
||||
@@ -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."""
|
||||
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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"]
|
||||
@@ -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
|
||||
Reference in new issue
Block a user