docs(07-04): add plan summary — Celery retry harness + POST /classify re-queue complete

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-06-04 23:12:11 +02:00
parent 63cd707d52
commit 0c1ae5284f
1 changed files with 136 additions and 0 deletions
@@ -0,0 +1,136 @@
+---
+phase: 07-redo-and-optimize-llm-integration
+plan: 04
+subsystem: backend/tasks + backend/api
+tags: [celery, retry, classification, async, tdd]
+wave: 4
+
+dependency_graph:
+  requires:
+    - 07-03  # load_provider_config + provider singletons
+  provides:
+    - D-09   # Celery exponential backoff (30/90/270s)
+    - D-10   # Final classification_failed status writeback
+    - D-11   # backend half: POST /classify re-queues Celery
+  affects:
+    - backend/tasks/document_tasks.py
+    - backend/api/documents.py
+    - backend/tests/test_document_tasks.py
+    - backend/tests/test_documents.py
+
+tech_stack:
+  added: []
+  patterns:
+    - "_ClassificationError sentinel escapes asyncio.run() to trigger Celery retry in sync layer (Pitfall 3)"
+    - "MaxRetriesExceededError caught in nested try/except inside except _ClassificationError block"
+    - "push_request(retries=N) + patch.object(task, retry) pattern for testing bound Celery tasks"
+    - "assert_called_once_with (not assert_awaited_once_with) for asyncio.run(AsyncMock(...)) calling convention"
+
+key_files:
+  modified:
+    - backend/tasks/document_tasks.py
+    - backend/api/documents.py
+    - backend/tests/test_document_tasks.py
+    - backend/tests/test_documents.py
+
+decisions:
+  - "MaxRetriesExceededError caught in nested try/except inside except _ClassificationError — not as sibling except — because raise self.retry() raises MaxRetriesExceededError during exception handling, which would propagate past a sibling except block"
+  - "module-level extract_and_classify import retained in documents.py (already existed, already used in confirm and upload handlers) — no deferred import needed"
+  - "test_reclassify_cross_user_returns_404 added as new IDOR test for classify endpoint (plan said 'regression of existing IDOR test' but none existed for classify; added new test per security mandate)"
+
+metrics:
+  completed_date: "2026-06-04"
+  tasks_completed: 2
+  tasks_total: 2
+  files_modified: 4
+---
+
+# Phase 07 Plan 04: Celery Retry Harness + POST /classify Re-queue Summary
+
+## One-liner
+
+Celery exponential-backoff retry harness (30s/90s/270s) via `_ClassificationError` sentinel + MaxRetriesExceededError nested catch, and `POST /classify` converted from synchronous classification to Celery re-queue.
+
+## Tasks Completed
+
+| Task | Name | Commit | Files |
+|------|------|--------|-------|
+| 1 | Celery retry harness + _ClassificationError + classification_failed writeback | e9ee5d4 | document_tasks.py, test_document_tasks.py |
+| 2 | POST /api/documents/{id}/classify → re-queue Celery | 63cd707 | documents.py, test_documents.py |
+
+## What Was Built
+
+### Task 1: Celery retry harness (D-09 / D-10)
+
+`backend/tasks/document_tasks.py` was refactored to implement the self-healing retry loop:
+
+- `class _ClassificationError(Exception)` — sentinel raised by `_run()` when `classifier.classify_document()` fails; escapes `asyncio.run()` and is caught by the outer sync task
+- `async def _mark_classification_failed(document_id)` — writes `doc.status = "classification_failed"` after all retries exhausted; called via `asyncio.run(...)` from the `MaxRetriesExceededError` handler
+- Task decorator changed to `@celery_app.task(name=..., bind=True, max_retries=3)`
+- Retry loop: `except _ClassificationError as exc` → `countdowns = [30, 90, 270]` → `raise self.retry(exc=exc, countdown=countdown)`
+- `MaxRetriesExceededError` caught in a **nested** `try/except` inside the `_ClassificationError` handler (not as a sibling `except`) because `raise self.retry()` raises `MaxRetriesExceededError` during exception handling context
+
+**Critical architectural decision:** The nested catch was required. If `MaxRetriesExceededError` were a sibling `except` block after `except _ClassificationError`, Celery's `self.retry()` call raising `MaxRetriesExceededError` inside the `_ClassificationError` handler would propagate past it without being caught.
+
+### Task 2: POST /classify re-queue (D-11)
+
+`backend/api/documents.py` classify endpoint was refactored:
+
+- Removed: `await classifier.classify_document(session, doc_id, topic_names)` and the topics response
+- Added: `doc.status = "processing"`, `await session.commit()`, `extract_and_classify.delay(str(doc.id))`
+- Returns: `{"document_id": str(doc.id), "status": "processing"}` with HTTP 200
+- Ownership check retained inline (no `_get_owned_doc` helper existed; plan said to replicate if absent)
+- `body: dict = {}` parameter removed (no longer needed since topics are written by the Celery task)
+
+## Test Coverage
+
+### Promoted from xfail:
+- `test_document_tasks.py::test_retry_backoff` — verifies countdowns [30, 90, 270] for retries 0/1/2
+- `test_document_tasks.py::test_exhaustion_sets_failed_status` — verifies `_mark_classification_failed` called and return dict correct
+- `test_documents.py::test_reclassify_requeues_celery` — verifies Celery delay called, doc.status=processing, response correct
+
+### New tests added:
+- `test_documents.py::test_reclassify_cross_user_returns_404` — IDOR test for classify endpoint (T-07-10)
+
+### Test suite result:
+- Before Plan 04: 366 passed, 12 xfailed, 1 pre-existing failure
+- After Plan 04: 370 passed, 9 xfailed, 1 pre-existing failure (test_extract_docx missing module — unrelated)
+- Net: +4 promoted tests, +1 new IDOR test
+
+## Deviations from Plan
+
+### Auto-fixed Issues
+
+**1. [Rule 1 - Bug] MaxRetriesExceededError requires nested try/except, not sibling**
+- **Found during:** Task 1 implementation — tests confirmed the structure
+- **Issue:** The plan described `except MaxRetriesExceededError` as a sibling handler after `except _ClassificationError`. However, `raise self.retry(exc=exc, countdown=countdown)` is called inside the `except _ClassificationError` block. When `self.retry()` raises `MaxRetriesExceededError`, Python is already unwinding the `_ClassificationError` handler — a sibling `except MaxRetriesExceededError` would not catch it.
+- **Fix:** Used nested `try/except MaxRetriesExceededError` inside the `except _ClassificationError` block
+- **Files modified:** `backend/tasks/document_tasks.py`
+- **Commit:** e9ee5d4
+
+**2. [Rule 2 - Missing Critical Functionality] Added IDOR test for classify endpoint**
+- **Found during:** Task 2 — plan referenced "regression of existing IDOR test" but no such test existed for POST /classify
+- **Issue:** T-07-10 (cross-user reclassify → 404) had no test coverage
+- **Fix:** Added `test_reclassify_cross_user_returns_404`
+- **Files modified:** `backend/tests/test_documents.py`
+- **Commit:** 63cd707
+
+### No architectural changes required.
+
+## Threat Surface Scan
+
+No new network endpoints or trust boundaries introduced. The `POST /classify` endpoint already existed; this plan changed its implementation from synchronous to async-queued. The IDOR invariant (T-07-10) is enforced by the ownership check `doc.user_id != current_user.id → 404` — same pattern as all other document endpoints.
+
+No new entries needed in threat model.
+
+## Known Stubs
+
+None — all classification path changes write real state to the DB via the Celery task.
+
+## Self-Check: PASSED
+
+- `backend/tasks/document_tasks.py` contains `class _ClassificationError`, `bind=True`, `max_retries=3`, `async def _mark_classification_failed`, `countdowns = [30, 90, 270]`, `raise self.retry(exc=`
+- `backend/api/documents.py` POST classify contains `extract_and_classify.delay(` and `doc.status = "processing"`, does NOT contain `await classifier.classify_document(`
+- Commits e9ee5d4 and 63cd707 exist in git log
+- `pytest tests/test_document_tasks.py tests/test_documents.py::test_reclassify_requeues_celery` exits 0
+- Full suite: 370 passed, 1 pre-existing failure (test_extract_docx — unrelated)