diff --git a/.planning/phases/07-redo-and-optimize-llm-integration/07-04-SUMMARY.md b/.planning/phases/07-redo-and-optimize-llm-integration/07-04-SUMMARY.md new file mode 100644 index 0000000..2bce352 --- /dev/null +++ b/.planning/phases/07-redo-and-optimize-llm-integration/07-04-SUMMARY.md @@ -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)