diff --git a/bookmark/api/routes.py b/bookmark/api/routes.py index 2ccac81..a758b59 100644 --- a/bookmark/api/routes.py +++ b/bookmark/api/routes.py @@ -21,7 +21,10 @@ def observe(request: Request): return JSONResponse(status_code=503, content={"error": "Camera unavailable", "detail": str(exc)}) request.app.state.latest_frame = frame fired = session.observe_frame(frame) - return {"fired": fired, "state": session.state.value, "book_id": session.current_book_id} + response = {"fired": fired, "state": session.state.value, "book_id": session.current_book_id} + if session.last_error: + response["error"] = session.last_error + return response @router.post("/api/manual-trigger") diff --git a/bookmark/session.py b/bookmark/session.py index 4c561c7..6a3fe29 100644 --- a/bookmark/session.py +++ b/bookmark/session.py @@ -1,3 +1,4 @@ +import threading from dataclasses import dataclass, field from datetime import datetime, timezone from enum import Enum @@ -28,14 +29,38 @@ class IntakeSession: state: IntakeState = IntakeState.WAITING_FRONT current_book_id: str | None = None _size_bucket: str | None = field(default=None, repr=False) + last_error: str | None = field(default=None, repr=False) + # Guards the capture-handling critical section. /api/observe is polled + # every ~500ms from the frontend without waiting for the previous call + # to resolve, and FastAPI's sync `def` routes run concurrently in + # Starlette's threadpool. Catalogue.update_row() does a non-atomic + # read-all-then-rewrite-whole-file, so two overlapping observe_frame() + # calls could interleave and corrupt catalogue.csv (the nonprofit's + # source of truth for tax reporting). Owning the lock on the session + # keeps a single capture in flight at a time regardless of caller. + _lock: threading.Lock = field(default_factory=threading.Lock, repr=False) def observe_frame(self, frame: np.ndarray) -> bool: - """Feed a frame to the active capture strategy. Returns True if a capture just fired.""" - fired = self.strategy.observe(frame) - if fired: - self._handle_capture(frame) - self.strategy.reset() - return fired + """Feed a frame to the active capture strategy. Returns True if a capture just fired + and produced a state transition. Sets `last_error` (and returns False) if a capture + fired but failed (e.g. no book detected on the platform).""" + with self._lock: + self.last_error = None + fired = self.strategy.observe(frame) + if not fired: + return False + try: + capturing_state = self.state + if capturing_state not in (IntakeState.WAITING_FRONT, IntakeState.WAITING_BACK): + # Nothing to do (e.g. BOOK_COMPLETE) — not a real capture event. + return False + self._handle_capture(frame) + except ValueError as exc: + self.last_error = str(exc) + return False + finally: + self.strategy.reset() + return True def _handle_capture(self, frame: np.ndarray) -> None: if self.state == IntakeState.WAITING_FRONT: @@ -44,11 +69,22 @@ class IntakeSession: self._capture_back(frame) def _capture_front(self, frame: np.ndarray) -> None: - self.current_book_id = self.catalogue.new_book_id() - self._size_bucket = determine_size_bucket(frame, self.reference_frame, self.markers) - wide_path = self.captures_dir / self.current_book_id / "front_wide.jpg" - cropped_path = self.captures_dir / self.current_book_id / "front_cropped.jpg" - self._save_pair(frame, wide_path, cropped_path) + book_id = self.catalogue.new_book_id() + size_bucket = determine_size_bucket(frame, self.reference_frame, self.markers) + wide_path = self.captures_dir / book_id / "front_wide.jpg" + cropped_path = self.captures_dir / book_id / "front_cropped.jpg" + try: + self._save_pair(frame, wide_path, cropped_path) + except ValueError: + # crop_to_book found no book (empty platform) — nothing was + # ever catalogued, so don't leave an orphan wide-image file + # (or the book_id directory) behind, and don't consume this + # book_id or advance session state. + self._cleanup_partial_capture(wide_path, cropped_path) + raise + + self.current_book_id = book_id + self._size_bucket = size_bucket self.catalogue.append( BookRecord( book_id=self.current_book_id, @@ -63,7 +99,14 @@ class IntakeSession: def _capture_back(self, frame: np.ndarray) -> None: wide_path = self.captures_dir / self.current_book_id / "back_wide.jpg" cropped_path = self.captures_dir / self.current_book_id / "back_cropped.jpg" - self._save_pair(frame, wide_path, cropped_path) + try: + self._save_pair(frame, wide_path, cropped_path) + except ValueError: + # No book detected on the platform for the back capture — the + # front row already exists, so leave it alone, just don't + # leave an orphan back_wide.jpg or advance past WAITING_BACK. + self._cleanup_partial_capture(wide_path, cropped_path) + raise self.catalogue.update_row( self.current_book_id, back_wide_path=str(wide_path), @@ -77,6 +120,19 @@ class IntakeSession: cropped = crop_to_book(frame, self.reference_frame) cv2.imwrite(str(cropped_path), cropped) + def _cleanup_partial_capture(self, wide_path: Path, cropped_path: Path) -> None: + """Remove any files written before a ValueError aborted a capture, and the + book_id directory too if it's now empty (front-capture failure case).""" + for path in (cropped_path, wide_path): + try: + path.unlink() + except FileNotFoundError: + pass + try: + wide_path.parent.rmdir() + except OSError: + pass # not empty (e.g. back-capture failure — front images remain) + def start_next_book(self) -> None: if self.state != IntakeState.BOOK_COMPLETE: raise RuntimeError("Cannot start next book before current book is complete") diff --git a/tests/test_api.py b/tests/test_api.py index 383ec6c..9415f2e 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -91,6 +91,20 @@ def test_preview_returns_jpeg_after_observe(tmp_path): assert response.headers["content-type"] == "image/jpeg" +def test_observe_on_empty_platform_does_not_500_and_reports_error(tmp_path): + client, session, camera = make_client(tmp_path) + # camera.frame is already the reference frame (empty platform) by default. + client.post("/api/manual-trigger") + response = client.post("/api/observe") + + assert response.status_code == 200 + body = response.json() + assert body["fired"] is False + assert body["state"] == "waiting_front" + assert "error" in body + assert session.catalogue.read_all() == [] + + def test_observe_returns_503_on_camera_failure(tmp_path): catalogue = Catalogue(tmp_path / "catalogue.csv") strategy = ManualButtonStrategy() diff --git a/tests/test_session.py b/tests/test_session.py index 29c6b82..c841eb3 100644 --- a/tests/test_session.py +++ b/tests/test_session.py @@ -94,3 +94,61 @@ def test_start_next_book_resets_state_and_assigns_new_id(session): def test_start_next_book_raises_if_book_not_complete(session): with pytest.raises(RuntimeError): session.start_next_book() + + +def test_empty_platform_capture_does_not_corrupt_state_or_files(session): + """Feeding the unchanged reference frame (nothing on the platform) should not + crash, should not create a catalogue row, should not leave an orphan file on + disk, should leave the strategy un-armed, and should not advance state.""" + session.strategy.trigger() + + fired = session.observe_frame(session.reference_frame) + + assert fired is False + assert session.last_error is not None + assert session.state == IntakeState.WAITING_FRONT + assert session.current_book_id is None + assert session.catalogue.read_all() == [] + assert session.strategy.observe(make_book_frame()) is False # not left armed + assert not (session.captures_dir).exists() or not any(session.captures_dir.rglob("*.jpg")) + + +def test_empty_platform_capture_on_back_step_does_not_advance_or_corrupt(session): + """Same failure mode, but triggered on the back-capture step: the front row + must survive untouched, and state must stay at WAITING_BACK.""" + session.strategy.trigger() + session.observe_frame(make_book_frame()) + book_id = session.current_book_id + assert session.state == IntakeState.WAITING_BACK + + session.strategy.trigger() + fired = session.observe_frame(session.reference_frame) + + assert fired is False + assert session.last_error is not None + assert session.state == IntakeState.WAITING_BACK + assert session.current_book_id == book_id + rows = session.catalogue.read_all() + assert len(rows) == 1 + assert rows[0]["back_wide_path"] == "" + assert rows[0]["back_cropped_path"] == "" + back_files = list((session.captures_dir / book_id).glob("back_*.jpg")) + assert back_files == [] + + +def test_book_complete_no_op_fire_does_not_return_true(session): + """A strategy re-firing while the session is already BOOK_COMPLETE (e.g. an + auto-strategy re-satisfying stability while a finished book sits on the + platform) must not report a capture and must not stay armed.""" + session.strategy.trigger() + session.observe_frame(make_book_frame()) + session.strategy.trigger() + session.observe_frame(make_book_frame()) + assert session.state == IntakeState.BOOK_COMPLETE + + session.strategy.trigger() + fired = session.observe_frame(make_book_frame()) + + assert fired is False + assert session.state == IntakeState.BOOK_COMPLETE + assert session.strategy.observe(make_book_frame()) is False # reset, not left armed