diff --git a/docs/superpowers/plans/2026-05-19-workflow-runner-refactor.md b/docs/superpowers/plans/2026-05-19-workflow-runner-refactor.md index 16eca2d78..008c172bf 100644 --- a/docs/superpowers/plans/2026-05-19-workflow-runner-refactor.md +++ b/docs/superpowers/plans/2026-05-19-workflow-runner-refactor.md @@ -2,6 +2,14 @@ > **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. +> **Updated 2026-09-24:** line references below are refreshed against current +> `dev`. More importantly: the motivating bugs (#12386 and the tag/m2m race) +> are **already fixed** by #12389 and #13178, independently of this plan — see +> "Status update (2026-09-24)" in the linked spec. This is now a structural +> cleanup (kill `use_overrides` dual-mode branching and the `original_file` +> parameter plumbing), not an active-bug fix. Task 7 is rescoped accordingly +> (the regression test it originally asked for already exists). + **Goal:** Replace the `use_overrides` dual-mode branching in `run_workflows` with a polymorphic `WorkflowRunContext`, and make the workflow-execution → file-rename sequence deterministic via a `ContextVar` guard. **Architecture:** A new `documents/workflows/context.py` module defines a `WorkflowRunContext` `Protocol` and two implementations — `ConsumptionContext` (wraps `ConsumableDocument` + `DocumentMetadataOverrides`) and `PersistedContext` (wraps a real `Document`). `run_workflows` becomes a flat match-and-dispatch loop with no mode flag. A module-level `ContextVar` guard suppresses `update_filename_and_move_files` for the duration of a workflow run; the file rename is invoked once, explicitly, after the run. @@ -144,7 +152,7 @@ git commit -m "Add ContextVar guard for workflow runner" - Test: `src/documents/tests/test_workflow_context.py` The `ConsumptionContext` absorbs the `use_overrides=True` branches currently in -`run_workflows` (`handlers.py:854-1010`) and `build_workflow_action_context` +`run_workflows` (`handlers.py:862-1051`) and `build_workflow_action_context` (`actions.py:33,53-83`). - [ ] **Step 1: Write the failing test** @@ -446,7 +454,7 @@ git commit -m "Add WorkflowRunContext protocol and ConsumptionContext" - Test: `src/documents/tests/test_workflow_context.py` `PersistedContext` absorbs the `use_overrides=False` branches of `run_workflows` -(`handlers.py:896-1005`) and `build_workflow_action_context` (`actions.py:35-51`). +(`handlers.py:904-1046`) and `build_workflow_action_context` (`actions.py:35-51`). - [ ] **Step 1: Write the failing test** @@ -702,7 +710,7 @@ def build_workflow_context( > Note: `build_placeholder_context` here is moved verbatim from the > non-`use_overrides` branch of `build_workflow_action_context` > (`actions.py:35-51`). `_WORKFLOW_SAVE_FIELDS` and `persist()` reproduce the -> save at `handlers.py:987-996` — keep the field list and the comment. +> save at `handlers.py:1028-1037` — keep the field list and the comment. - [ ] **Step 4: Run test to verify it passes** @@ -724,11 +732,18 @@ git commit -m "Add PersistedContext and build_workflow_context factory" **Files:** -- Modify: `src/documents/signals/handlers.py:431-667` +- Modify: `src/documents/signals/handlers.py:437-673` This task is a pure extraction — no behavior change yet. The guard check is added in Task 6. +> **Note:** re-verify these line numbers against current `handlers.py` before +> starting — this file also received a checksum-based "already moved" recovery +> fix (#12389, `_path_matches_checksum` at `handlers.py:416-420` plus changes +> inside `validate_move`) since this plan was written. That logic is unrelated +> to this extraction and must be carried over unchanged inside the extracted +> body — do not simplify or remove it. + - [ ] **Step 1: Confirm the regression baseline is green** Run: `uv run pytest documents/tests/test_file_handling.py -q` @@ -737,8 +752,8 @@ Expected: PASS (records the pre-change baseline for the rename logic) - [ ] **Step 2: Split the function** In `src/documents/signals/handlers.py`, replace the `update_filename_and_move_files` -definition (currently `def update_filename_and_move_files(...)` at line 434, -through the end of its body at line 667) with two definitions: +definition (currently `def update_filename_and_move_files(...)` at line 440, +through the end of its body at line 673) with two definitions: ```python @receiver(models.signals.post_save, sender=CustomFieldInstance, weak=False) @@ -758,13 +773,17 @@ def update_filename_and_move_files( def move_files_for_document(instance: Document) -> None: - def validate_move(instance, old_path: Path, new_path: Path, root: Path) -> None: - ... # body unchanged from current lines 444-463 + def validate_move( + instance, old_path: Path, new_path: Path, root: Path + ) -> None: ... # body unchanged from current lines 444-463 ``` -Move the entire body currently between line 444 (`def validate_move`) and line -667 into `move_files_for_document`, **unchanged**, with one edit: the recursive -call at the end (currently `handlers.py:660-667`) must call the new function: +Move the entire body currently between line 450 (`def validate_move`) and line +673 into `move_files_for_document`, **unchanged**, with one edit: the recursive +call at the end (currently `handlers.py:665-673`, which today calls the whole +receiver directly — `update_filename_and_move_files(Document, version_doc)`, +passing a synthetic `Document` `sender` to satisfy the receiver's signature) +must instead call the new function directly: ```python # Keep version files in sync with root @@ -843,7 +862,7 @@ from documents.workflows.context import build_workflow_context and remove the now-unused `build_workflow_action_context` import. -Replace the entire `run_workflows` function (`handlers.py:854-1010`) with: +Replace the entire `run_workflows` function (`handlers.py:862-1051`) with: ```python def run_workflows( @@ -949,7 +968,7 @@ staged_file` is exactly the constructor `staged_file` arg, which is - [ ] **Step 6: Update `run_workflows_added`** -`run_workflows_added` (`handlers.py:803-816`) already forwards `original_file` +`run_workflows_added` (`handlers.py:811-824`) already forwards `original_file` to `run_workflows`. Leave it as-is — it still receives `original_file` from the `document_consumption_finished` signal and passes it through; `run_workflows` now routes it into `PersistedContext` construction. @@ -985,11 +1004,14 @@ git commit -m "Refactor run_workflows around WorkflowRunContext, drop use_overri - [ ] **Step 1: Write the failing test** -Append a new test class to `src/documents/tests/test_workflows.py`. Use the -existing helpers/fixtures already in that file for creating a `Document`, a -`Workflow` with a `DOCUMENT_UPDATED` trigger, and an ASSIGNMENT action that -assigns a correspondent. Model it on the existing `DOCUMENT_UPDATED` tests in -that file (they call `run_workflows(...)` directly). The new test: +Append a new test class to `src/documents/tests/test_workflows.py`. **The +action must assign both a tag and a correspondent/storage-path** (not +correspondent alone) — a correspondent-only assignment never fires +`m2m_changed` mid-run, so it would call `move_files_for_document` exactly once +even without the guard, and the test would pass vacuously. Use the same shape +as the existing `test_document_updated_workflow_assignment_storage_path_persists_with_tag_assignment` +(`test_workflows.py:3030`) for the Document/StoragePath/Workflow/ASSIGNMENT +setup, and call `run_workflows(...)` directly as that test does. The new test: ```python class TestWorkflowRenameSequencing(TestCase): @@ -1003,9 +1025,10 @@ class TestWorkflowRenameSequencing(TestCase): from documents.signals import handlers - # ... set up a Document, a storage path whose template depends on - # correspondent, and a DOCUMENT_UPDATED workflow with an ASSIGNMENT - # action assigning that correspondent (reuse this file's helpers) ... + # ... set up a Document, a StoragePath whose template depends on + # correspondent, and a DOCUMENT_UPDATED workflow with one ASSIGNMENT + # action assigning BOTH a tag and that correspondent (reuse the setup + # from test_document_updated_workflow_assignment_storage_path_persists_with_tag_assignment) ... with mock.patch.object( handlers, @@ -1025,7 +1048,14 @@ class TestWorkflowRenameSequencing(TestCase): > The implementer should flesh out the document/workflow setup using the > patterns already present in `test_workflows.py`. The assertion that matters: > `move_files_for_document` is called exactly once, via the explicit -> `finalize_file_location()`, not once-per-`save()` from signals. +> `finalize_file_location()`, not once-per-`save()` from signals. Pre-guard, +> current code (post-#13178) calls it **twice** for this setup: once from the +> `m2m_changed` receiver when `apply_assignment_to_document` fetches a fresh +> instance to add the tag (a self-consistent no-op move against still-current +> DB state), and once from the final `document.save()`'s `post_save`. So this +> test is a genuine (if narrower) regression check, not a vacuous one — it's +> just checking call count / ordering discipline rather than a wrong final +> path, since #13178 already ensures the final path is correct either way. - [ ] **Step 2: Run test to verify it fails** @@ -1112,60 +1142,49 @@ git commit -m "Defer workflow file rename via ContextVar guard" --- -## Task 7: Regression test for the metadata-vs-rename race (#12386) +## Task 7: Confirm pre-existing metadata-vs-rename regression coverage still holds -**Files:** +**Files:** none (verification only — no new test) -- Test: `src/documents/tests/test_workflows.py` +> **Rescoped 2026-09-24:** this task originally asked for a new regression +> test for the #12386 metadata-vs-rename race. That race was independently +> fixed by #13178 (merged 2026-07-20, after this plan was written), whose only +> added test is +> `test_document_updated_workflow_assignment_storage_path_persists_with_tag_assignment` +> in `test_workflows.py`. Writing another test asserting the same final-state +> outcome would duplicate that coverage. Task 6's +> `TestWorkflowRenameSequencing` already adds the piece that test doesn't +> cover — call-count/ordering discipline on `move_files_for_document` — so +> this task is now pure verification that nothing regressed. -- [ ] **Step 1: Write the regression test** +- [ ] **Step 1: Run the pre-existing regression tests by name** -Append to `src/documents/tests/test_workflows.py` a test that reproduces the -original bug: a workflow that assigns **both** tags (firing `m2m_changed`) and a -correspondent, where the storage path template depends on the correspondent. -Before the fix the `m2m_changed`-triggered rename ran with a stale (empty) -correspondent and moved the file to the wrong path. - -```python -class TestWorkflowMetadataRenameRace(TestCase): - @pytest.mark.django_db - def test_tag_and_correspondent_assignment_lands_file_at_final_path( - self, - ) -> None: - """ - Regression for #12386: assigning tags (m2m_changed) plus a correspondent - used by the storage-path template must not move the file using stale - metadata. After the run the DB filename and the on-disk file agree. - """ - # ... reuse this file's helpers to: - # - create a StoragePath whose path template references {correspondent} - # - create a Document assigned to that storage path with a real file - # on disk at document.source_path - # - create a DOCUMENT_UPDATED Workflow with one ASSIGNMENT action that - # assigns BOTH a tag and a correspondent - # - call run_workflows(DOCUMENT_UPDATED, document=document) - # Assert: - document.refresh_from_db() - assert document.correspondent is not None - assert Path(document.source_path).is_file() - # the path reflects the assigned correspondent, not an empty value - assert document.correspondent.name in str(document.filename) -``` - -- [ ] **Step 2: Run the test** - -Run: `uv run pytest documents/tests/test_workflows.py::TestWorkflowMetadataRenameRace -v` -Expected: PASS (the guard from Task 6 fixes the race; this test locks it in) - -- [ ] **Step 3: Lint and commit** +Run (all three are in the `TestWorkflows` class, `test_workflows.py:78-5123`): ```bash -ruff check --fix src/documents/tests/test_workflows.py -ruff format src/documents/tests/test_workflows.py -git add src/documents/tests/test_workflows.py -git commit -m "Add regression test for workflow metadata vs rename race" +uv run pytest \ + "documents/tests/test_workflows.py::TestWorkflows::test_document_updated_workflow_assignment_storage_path_persists_with_tag_assignment" \ + "documents/tests/test_workflows.py::TestWorkflows::test_document_updated_workflow_assignment_persists_when_removing_trigger_tag" \ + "documents/tests/test_workflows.py::TestWorkflows::test_workflow_document_updated_does_not_overwrite_filename" \ + -v ``` +(Confirm the exact enclosing class name for each with +`grep -n "class Test\|def test_" src/documents/tests/test_workflows.py` before +running — class boundaries may have shifted since this plan was written.) + +Expected: PASS, **unchanged** — same assertions, no edits needed. If any of +these needs modification to pass after the refactor, that is a behavior +regression introduced by this plan, not an acceptable side effect — stop and +investigate before continuing. + +- [ ] **Step 2: No commit** + +This task makes no code changes. If Step 1 required a fix elsewhere, that fix +belongs to whichever earlier task's commit introduced the regression — amend +that task's changes (as a new commit, not `--amend`) rather than committing +here. + --- ## Task 8: Full verification @@ -1207,7 +1226,7 @@ git commit -m "Clean up after workflow runner refactor" - **Spec coverage:** §1 Protocol → Tasks 2-3; §2 branch-free `run_workflows` → Task 5; §3 `source_file` / staged-path relocation → Tasks 3, 5; §4 guard + sequencing → Tasks 4, 6; deferred password-removal hook left as-is (Task 5 - note); testing → Tasks 1-3, 6-8. + note); testing → Tasks 1-3, 6, 7 (rescoped), 8. - **`update_fields` exclusion kept** (`_WORKFLOW_SAVE_FIELDS` in Task 3) — per the design decision that it guards a cross-process hazard the guard does not cover. No task removes it. @@ -1216,3 +1235,37 @@ git commit -m "Clean up after workflow runner refactor" `apply_removal`, `log_action`, `persist`, `record_run`, `finalize_file_location`) is identical across the Protocol, both implementations, and all call sites in the rewritten `run_workflows`. + +## 2026-09-24 status update + +- All line-number references throughout this plan were re-verified against + current `dev` and corrected (`handlers.py` shifted by ~6-8 lines throughout + due to unrelated intervening changes, chiefly #12389's + `_path_matches_checksum` addition). +- The two concrete bugs motivating this refactor are already fixed + independently, by different mechanisms, without this plan: #12389 + (cross-process already-moved-file race, checksum-based recovery in + `validate_move`) and #13178 (intra-workflow tag/`m2m_changed` clobbering + unsaved fields, fixed by fetching a fresh `Document` instance for tag + mutation — see `mutations.py:26-31`). Neither fix uses `WorkflowRunContext` + or the `ContextVar` guard. +- Task 4 now calls out explicitly that the checksum-recovery logic inside + `validate_move` must be carried over unchanged during extraction, and that + the version-document recursive call site (`handlers.py:670-673`) needs + updating to call the extracted `move_files_for_document` directly, which the + original plan did not mention. +- Task 6's test setup was corrected — a correspondent-only assignment doesn't + exercise the guard (no `m2m_changed` fires), so the test as originally + written would have passed vacuously, guard or no guard. It now requires a + combined tag + correspondent/storage-path assignment, matching the shape of + #13178's own regression test. +- Task 7 was rescoped from "write a new regression test for #12386" to + "confirm #13178's existing regression tests still pass unchanged" — writing + a new test asserting the same final-state outcome would have duplicated + coverage that already exists and predates this plan's completion. +- **Net effect on scope:** this plan still fully replaces `use_overrides` + dual-mode branching and the `original_file` parameter plumbing (real, + unfixed structural debt) with a cleaner `WorkflowRunContext` abstraction. + The `ContextVar` guard piece is now justified as consolidating two + independent point-fixes into one general mechanism, not as fixing an active + bug — worth doing, but not urgent. diff --git a/docs/superpowers/plans/2026-08-18-pdf-thumbnail-poppler-migration.md b/docs/superpowers/plans/2026-08-18-pdf-thumbnail-poppler-migration.md index 6bb49b860..1b831a56c 100644 --- a/docs/superpowers/plans/2026-08-18-pdf-thumbnail-poppler-migration.md +++ b/docs/superpowers/plans/2026-08-18-pdf-thumbnail-poppler-migration.md @@ -58,21 +58,30 @@ def get_pdf_first_page_size_points( ) -> tuple[float, float] | None: """Return the first page's (width, height) in PDF points, post-rotation. - Uses ``page.cropbox`` (pikepdf 10.2.0, pinned in uv.lock) — this is a - read-only property, not a raw dict lookup, and already implements the + Uses ``page.cropbox`` (pikepdf, pinned at 10.13.0.post1 in uv.lock) — this + is a read-only property, not a raw dict lookup, and already implements the PDF-spec-correct fallback/inheritance to MediaBox when a page has no - ``/CropBox`` of its own (confirmed against pikepdf's own - ``_get_cropbox(True, False)`` implementation and by testing against - `src/documents/tests/samples/simple.pdf`, which has no ``/CropBox`` key - at all). This must match whatever box the renderer is told to use - (pdftoppm's ``-cropbox`` flag), or the computed DPI will target the - wrong box's dimensions. Swaps width/height when ``/Rotate`` is 90 or - 270, since that's the orientation the page will actually be rendered - in — note ``page.rotate`` is a *mutator* method - (``rotate(angle, relative) -> None``) in this pikepdf version, not a - getter; the current rotation must be read via - ``page.obj.get("/Rotate", 0)`` instead (confirmed: returns ``None`` - cleanly, not an exception, when the key is absent). + ``/CropBox`` of its own. Per pikepdf's release notes, box properties + (``mediabox``/``cropbox``/etc.) were reimplemented in C++ in 10.8.0 for + performance, with behavior explicitly unchanged — confirmed stable across + the range this project has used. This must match whatever box the + renderer is told to use (pdftoppm's ``-cropbox`` flag), or the computed + DPI will target the wrong box's dimensions. + + Swaps width/height when the page's effective rotation is 90 or 270, since + that's the orientation the page will actually be rendered in. Uses + ``page.rotation`` (added in pikepdf 10.9.0, available at the pinned + 10.13.0.post1) rather than a raw ``page.obj.get("/Rotate", 0)`` dict + lookup: ``/Rotate`` is one of the PDF spec's *inheritable* page + attributes — a page can take its rotation from an ancestor ``/Pages`` + node rather than setting ``/Rotate`` on its own object dict — and + ``page.rotation`` is documented to resolve that inheritance and normalize + the result to ``[0, 360)``, defaulting to ``0`` when nothing is set + anywhere in the chain. A raw ``page.obj.get("/Rotate", 0)`` lookup would + silently miss an inherited rotation and compute DPI for the wrong + orientation on such a PDF. (Do not use ``page.rotate(...)`` — as of + 10.9.0 that's a *mutator* method, and even before that it was never a + getter.) Parameters ---------- @@ -100,7 +109,7 @@ def get_pdf_first_page_size_points( height = abs(ury - lly) if width <= 0 or height <= 0: return None - rotate = int(page.obj.get("/Rotate", 0)) % 360 + rotate = page.rotation % 360 if rotate in (90, 270): width, height = height, width return width, height @@ -113,7 +122,15 @@ def get_pdf_first_page_size_points( return None ``` -`page.cropbox`/`page.mediabox` returning `pikepdf.Array` (indexable, four numeric elements) and `page.obj.get("/Rotate", 0)` behavior were both confirmed directly against the pinned version rather than assumed — no further verification needed during implementation. +> **Verify before implementing:** confirm `page.rotation` behaves as pikepdf's +> release notes describe against the pinned 10.13.0.post1 — test with a PDF +> that has no `/Rotate` at all (expect `0`, not an exception) and, if a +> sample with inherited (ancestor-node) rotation is available, confirm it +> resolves correctly. The VM (`paperless-vm`) is the only place pikepdf is +> installed per this project's Windows/Linux split — do this check there +> before relying on it. + +`page.cropbox`/`page.mediabox` returning `pikepdf.Array` (indexable, four numeric elements) is confirmed stable per pikepdf's own release notes (box properties moved to a C++ implementation in 10.8.0 with behavior explicitly unchanged). `page.rotation` (used above in place of the earlier draft's raw `page.obj.get("/Rotate", 0)`) was added in 10.9.0 and is documented to resolve inherited `/Rotate` values — **this one is worth a quick empirical check against the pinned 10.13.0.post1 on the VM before implementation**, since it wasn't available to test directly when this plan was last touched. - [ ] **Step 2: Add `rasterize_pdf_page_to_png()` to `documents/parsers.py`** @@ -135,8 +152,12 @@ def rasterize_pdf_page_to_png( # -singlefile suppresses that so out_path is written exactly as given. args = [ "pdftoppm", - "-f", "1", "-l", "1", - "-r", str(dpi), + "-f", + "1", + "-l", + "1", + "-r", + str(dpi), "-png", "-singlefile", ] @@ -207,7 +228,9 @@ def make_thumbnail_from_pdf(in_path: Path, temp_dir: Path, logging_group=None) - encode_thumbnail_webp(png_path, out_path) except ParseError as e: logger.error(f"Unable to make thumbnail with pdftoppm: {e}") - out_path = make_thumbnail_from_pdf_qpdf_fallback(in_path, temp_dir, logging_group) + out_path = make_thumbnail_from_pdf_qpdf_fallback( + in_path, temp_dir, logging_group + ) return out_path ``` @@ -244,7 +267,7 @@ def _compute_thumbnail_dpi(in_path: Path, logging_group=None) -> int: - [ ] **Step 5: Add tests for `get_pdf_first_page_size_points()`** -Cover: a normal PDF (returns expected width/height for a known sample), a PDF with `/Rotate 90` (width/height swapped vs. the unrotated equivalent), and a nonexistent/corrupt path (returns `None`, doesn't raise). Use existing sample PDFs under `src/paperless/tests/parsers/samples/` or `src/documents/tests/samples/` where possible rather than adding new binary fixtures. +Cover: a normal PDF (returns expected width/height for a known sample), a PDF with `/Rotate 90` set directly on the page (width/height swapped vs. the unrotated equivalent), and a nonexistent/corrupt path (returns `None`, doesn't raise). If a sample PDF with _inherited_ rotation (set on an ancestor `/Pages` node rather than the page itself) is easy to construct or already exists, add that case too — it's the specific gap `page.rotation` closes over a raw `/Rotate` dict lookup; if constructing one isn't cheap, skip it rather than manufacturing a fixture just for this, but don't skip verifying `page.rotation`'s basic no-rotation-set default (`0`) against the pinned pikepdf version. Use existing sample PDFs under `src/paperless/tests/parsers/samples/` or `src/documents/tests/samples/` where possible rather than adding new binary fixtures. - [ ] **Step 6: Add a dimension/format assertion to `TestGetThumbnail`** @@ -303,14 +326,15 @@ git commit -m "refactor: rasterize PDF thumbnails with pdftoppm+pikepdf+Pillow i Replace `make_thumbnail_from_pdf_gs_fallback()` (`parsers.py:130-171`) with: ```python -def make_thumbnail_from_pdf_qpdf_fallback(in_path, temp_dir, logging_group=None) -> Path: +def make_thumbnail_from_pdf_qpdf_fallback( + in_path, temp_dir, logging_group=None +) -> Path: png_path: Path = Path(temp_dir) / "page1_repaired.png" out_path: Path = Path(temp_dir) / "convert_qpdf.webp" repaired_path: Path = Path(temp_dir) / "repaired.pdf" logger.warning( - "Thumbnail generation with pdftoppm failed, attempting qpdf " - "repair and retry.", + "Thumbnail generation with pdftoppm failed, attempting qpdf repair and retry.", extra={"group": logging_group}, ) diff --git a/docs/superpowers/specs/2026-05-19-workflow-runner-refactor-design.md b/docs/superpowers/specs/2026-05-19-workflow-runner-refactor-design.md index 6a561c616..3e5871bfc 100644 --- a/docs/superpowers/specs/2026-05-19-workflow-runner-refactor-design.md +++ b/docs/superpowers/specs/2026-05-19-workflow-runner-refactor-design.md @@ -1,42 +1,110 @@ # Workflow Runner Refactor — Design **Date:** 2026-05-19 +**Updated:** 2026-09-24 — see "Status update (2026-09-24)" below; line references +throughout refreshed against current `dev`. **Branch base:** `dev` -**Status:** Approved design, pending implementation plan +**Status:** Approved design, pending implementation plan. Motivating bugs are now +independently fixed (see status update) — this is a structural cleanup, not an +active-bug fix. + +## Status update (2026-09-24) + +The acute bugs this design was written against have since been closed by two +targeted, already-merged fixes that do **not** use the `WorkflowRunContext` / +`ContextVar` approach below: + +- **#12386** (cross-process "file already moved" race, cited in cause 3) was + fixed by **#12389** ("Fix: avoid moving files if already moved", merged in + v2.20.12): `validate_move` in `update_filename_and_move_files` now detects, + via a checksum comparison (`_path_matches_checksum`), when the target file + already exists because a concurrent save already moved it, and recovers the + DB pointer instead of raising `CannotMoveFilesException`. This is a different + mechanism from anything proposed here and stays as-is; nothing in this + refactor should touch `_path_matches_checksum` or its call sites. +- **The intra-workflow tag/`m2m_changed` race** described in cause 3 and design + §4 (`add_nested_tags` firing `m2m_changed` → `refresh_from_db()` → wiping an + earlier-ordered action's unsaved `correspondent`/`storage_path`) was fixed by + **#13178** ("Fix: prevent tag assignment from reverting other pending + workflow assignments", merged 2026-07-20 — _after_ this design was written). + `apply_assignment_to_document` (`mutations.py`) now applies tag changes to a + **freshly-fetched** `Document` instance rather than the shared in-memory one, + matching the pattern `apply_removal_to_document` already used for tag + removal. Because the mid-run rename this triggers now reads the _old_, + still-current DB state (nothing else has been saved yet), it recomputes the + same path and is a no-op; the real rename happens once, correctly, at the + final `document.save()`. Regression coverage: + `test_document_updated_workflow_assignment_storage_path_persists_with_tag_assignment` + in `test_workflows.py` (the only test #13178 itself added). A related, + earlier fix (#12664) covers a similar but distinct case — tag removal + alongside a title assignment — via + `test_document_updated_workflow_assignment_persists_when_removing_trigger_tag`. +- The `filename`/`archive_filename` exclusion from `update_fields` (cause 3, + design §4 point 2) was already in place before this design was written (added + by #12390-adjacent work) and is unaffected — still correctly attributed here + as a load-bearing cross-process guard, not duct tape. +- **#13178's own PR description is the origin of this idea**: "Once the beta + is out, I do have some thoughts about using ContextVar to delete this class + entirely. We've run into it plenty of times." So the guard concept predates + this design; the acute pain it was meant to address has since been patched + piecemeal instead. + +**What this means for scope:** causes 1 and 2 below (dual-mode branching, +staged-file parameter plumbing) are unchanged and fully present in current code +— this refactor is still worth doing for them. Cause 3's race is no longer an +_active_ bug; the `ContextVar` guard is now defense-in-depth / simplification +(it also collapses two independent workarounds — the fresh-instance tag fetch +and the per-workflow `update_fields` restriction — into one clearer mechanism) +rather than a fix for a reproducible failure. Treat any "fixes a bug" framing +below as historical motivation, not a current defect claim. ## Problem Workflow execution and the Django signal layer have repeatedly produced fragile, hard-to-fix bugs (see the revert/refix history around password removal: #12803, -#12814, #12716, and the filename race #12386). Three structural causes: +#12814, #12716, and the filename race #12386, now closed — see status update +above). Three structural causes: 1. **`run_workflows` is dual-mode.** A single function handles both consumption (mutating a `DocumentMetadataOverrides`) and post-save (mutating a real `Document`), branching on a `use_overrides` flag. The branching is concentrated in two places — the action dispatch inside `run_workflows` - (`handlers.py:931-1001`) and `build_workflow_action_context` - (`actions.py:33-83`), each with two full code paths. The `apply_*` helpers in - `workflows/mutations.py` are _already_ split by target type - (`apply_assignment_to_document` vs `apply_assignment_to_overrides`, etc.); the - refactor unifies their callers, not the helpers themselves. + (`handlers.py:938-1014`, `use_overrides` first set at `handlers.py:881`) and + `build_workflow_action_context` (`actions.py:33-83`), each with two full code + paths. The `apply_*` helpers in `workflows/mutations.py` are _already_ split + by target type (`apply_assignment_to_document` vs + `apply_assignment_to_overrides`, etc.); the refactor unifies their callers, + not the helpers themselves. **Still fully present in current code — unchanged + by any fix since this design was written.** 2. **File location is an implicit, timing-dependent side channel.** The - `DOCUMENT_ADDED` workflow fires from `run_workflows_added`, which runs while - the consumer is still inside its transaction — _before_ the consumed file is - copied to `document.source_path` (`document_consumption_finished` is sent at - `consumer.py:654`; the file copy happens after, at `consumer.py:666+`). The - staged path is therefore threaded through as `original_file` / - `caller_supplied_original_file` parameters. Actions that read the file - (password removal, email attachments) depend on this plumbing being correct. + `DOCUMENT_ADDED` workflow fires from `run_workflows_added` + (`handlers.py:811-824`), which runs while the consumer is still inside its + transaction — _before_ the consumed file is copied to `document.source_path` + (`document_consumption_finished` is sent from `consumer.py`, the file copy + happens after). The staged path is therefore threaded through as + `original_file` / `caller_supplied_original_file` parameters + (`handlers.py:868,890-898,976-979`). Actions that read the file (password + removal, email attachments) depend on this plumbing being correct. **Still + fully present in current code.** -3. **The workflow run races the filename rename.** `update_filename_and_move_files` - is a raw `post_save` receiver on `Document`. When a workflow persists its - changes via `document.save(update_fields=[...])`, that save fires `post_save` - and runs the rename _while the workflow is still executing_. Under concurrent - Celery/UI updates the interleaved `refresh_from_db()` calls corrupt state. The - comment at `handlers.py:980-984` — deliberately excluding `filename` / - `archive_filename` from the workflow save — is a load-bearing workaround for - exactly this. +3. **The workflow run could race the filename rename — now mitigated by two + independent, narrower fixes (see "Status update" above), not eliminated + structurally.** `update_filename_and_move_files` (`handlers.py:437-439` + decorators, body `440-673`) is a raw `post_save`/`m2m_changed` receiver. When + a workflow persists its changes via `document.save(update_fields=[...])`, or + when `apply_assignment_to_document` mutates tags via a freshly-fetched + instance (`mutations.py:26-31`), that write fires the receiver _while the + workflow is still executing_. As of #13178 this no longer corrupts in-memory + state (tags are applied to a separate instance, so the mid-run rename it + triggers reads only already-committed data and is a no-op or self-consistent + move); as of #12389 a genuine cross-process "someone else already moved this + file" race recovers via checksum comparison instead of erroring. The comment + documenting the `filename`/`archive_filename` exclusion from the workflow's + own `update_fields` (`handlers.py:1019-1027`) remains a load-bearing guard + against a _different_, still-real cross-process hazard (an in-memory + `document.filename` going stale while another process moves the file) and is + unaffected by either fix above. Note: `run_workflows_added` / `run_workflows_updated` are connected to the _custom_ signals `document_consumption_finished` / `document_updated`, fired @@ -155,12 +223,23 @@ let one workflow's refresh wipe a prior workflow's in-memory changes. So: 3. The rename is suppressed for the whole run and invoked **exactly once, afterward**, against final committed state. -The actual race being fixed: `apply_assignment_to_document` assigns tags via -`document.add_nested_tags(...)`, which fires `m2m_changed` on -`Document.tags.through` _before_ the workflow's `document.save()`. The -`m2m_changed` receiver `update_filename_and_move_files` then calls -`refresh_from_db()`, wiping the workflow's in-memory correspondent/type, and -moves the file to a path computed from stale metadata. The guard prevents this. +**Historical race, already fixed by other means (see "Status update"):** +`apply_assignment_to_document` used to assign tags via +`document.add_nested_tags(...)` directly on the shared in-memory `document`, +which fired `m2m_changed` on `Document.tags.through` _before_ the workflow's +`document.save()`; the `m2m_changed` receiver `update_filename_and_move_files` +then called `refresh_from_db()` on that shared instance, wiping the workflow's +in-memory correspondent/type, and moved the file to a path computed from stale +metadata. #13178 fixed this by having `apply_assignment_to_document` (and +`apply_removal_to_document`, which already did this) mutate tags on a +**freshly-fetched** `Document.objects.get(pk=document.pk)` instead of the +shared one (`mutations.py:26-31`), so the `refresh_from_db()` triggered by +`m2m_changed` no longer touches the workflow's unsaved in-memory fields. The +guard below is not needed to fix that specific corruption anymore — it instead +gives a single, general mechanism that supersedes the fresh-instance-fetch +workaround (and the equivalent one already in `apply_removal_to_document`), +rather than requiring every future mutation path to remember to fetch a +separate instance. To stop the rename from firing mid-workflow, a **`ContextVar` guard** is introduced (e.g. `documents/workflows/context.py` module-level @@ -173,8 +252,16 @@ reentrancy-safe for nested saves or nested workflow runs. The guard must span the whole execution, not just `persist()`, because `update_filename_and_move_files` is _also_ registered to `m2m_changed` on `Document.tags.through` and to `post_save` on `CustomFieldInstance` -(`handlers.py:431-432`). A workflow action that assigns tags or custom fields -would otherwise trigger a rename mid-workflow through those signals. +(`handlers.py:437-438`). A workflow action that assigns tags or custom fields +would otherwise trigger a rename mid-workflow through those signals. If the +`ContextVar` guard lands, the fresh-instance-fetch workaround in +`apply_assignment_to_document`/`apply_removal_to_document` becomes redundant +but is not itself incorrect — decide during implementation whether to simplify +those two call sites back to mutating `document` directly now that the guard +covers the hazard, or leave them as extra defense-in-depth. Note either way: +`document` passed to `add_nested_tags`/`tags.clear`/`tags.remove` must still be +re-fetched or `refresh_from_db()`'d for the tags relation to reflect the +change on the in-memory instance used later in the same action. After execution completes, `run_workflows` calls `persist()` once and then explicitly invokes the move logic once. The `ContextVar` is set/reset in the @@ -184,26 +271,37 @@ pools are also `contextvars`-aware — non-issues, noted for completeness.) The move body of `update_filename_and_move_files` is extracted into a plain callable that the runner invokes directly. The function is already invoked -directly (as a plain call, bypassing the decorator) for version documents at -`handlers.py:664-667`, so this extraction has precedent. The thin `post_save` -receiver remains as a guard-checking wrapper. +directly today for version documents — currently as +`update_filename_and_move_files(Document, version_doc)` +(`handlers.py:670-673`), i.e. the _whole receiver_ is called with a synthetic +`sender` positional arg, bypassing only the `@receiver` decorator/dispatch, not +the guard-check-then-body split this refactor introduces. Once the move body is +extracted into its own callable (`move_files_for_document(instance)` per the +implementation plan), this recursive call site must be updated to call that +extracted function directly (`move_files_for_document(version_doc)`) rather +than the thin wrapper — otherwise recursing into version documents would +re-enter the guard-check wrapper unnecessarily (harmless, since the guard +should be set during a workflow run and unset otherwise, but pointless +indirection). The thin `post_save`/`m2m_changed` receivers remain as a +guard-checking wrapper around the extracted callable. The two `post_save` receivers on `Document` are `update_filename_and_move_files` -(`handlers.py:433`) and `update_llm_suggestions_cache` (`handlers.py:740`). The -`ContextVar` guard suppresses **only** the former — `update_llm_suggestions_cache` -keeps running normally, as do `document_consumption_finished` receivers such as +(`handlers.py:439`) and `update_llm_suggestions_cache` (`handlers.py:746-747`). +The `ContextVar` guard suppresses **only** the former — +`update_llm_suggestions_cache` keeps running normally, as do +`document_consumption_finished` receivers such as `add_or_update_document_in_llm_index` (which is _not_ a `post_save` receiver). This is why the guard is preferred over persisting with `.update()`, which would silently suppress _all_ `post_save` receivers including `update_llm_suggestions_cache`. `WorkflowRun.objects.create(...)` is created per matching workflow as today -(`handlers.py:998-1002`); it is a separate model and is not deferred. +(`handlers.py:1039-1043`); it is a separate model and is not deferred. -The comment at `handlers.py:980-984` is updated to describe the new flow -(per-workflow save under the guard; single explicit rename afterward) but the -`filename` / `archive_filename` exclusion it documents is kept — see point 2 -above. +The comment at `handlers.py:1019-1027` (added by the pre-existing `update_fields` +fix, predating this design) is updated to describe the new flow (per-workflow +save under the guard; single explicit rename afterward) but the `filename` / +`archive_filename` exclusion it documents is kept — see point 2 above. ## Testing @@ -216,11 +314,16 @@ above. - **ContextVar guard** — assert `update_filename_and_move_files` early-returns while the guard is set, and that the rename runs exactly once after `persist()`. -- **Regression: the racy case** — a workflow that reassigns metadata while the - document is subject to a filename template; assert final DB filename and file - location are consistent (the #12386 scenario). -- **Regression safety net** — the existing `test_workflows.py` suite (~100 - tests; ~19 `document_consumption_finished.send` sites plus many direct +- **Regression: the racy case is already covered, not newly needed.** The + scenario this design originally asked for a new test for — a workflow that + reassigns metadata (tags + correspondent/storage path) while the document is + subject to a filename template, asserting final DB filename and file location + stay consistent — is already exercised by + `test_document_updated_workflow_assignment_storage_path_persists_with_tag_assignment` + in `test_workflows.py` (added by #13178). No new regression test is required + for this; the refactor's job is to keep it passing **unchanged**. +- **Regression safety net** — the existing `test_workflows.py` suite (~100+ + tests; many `document_consumption_finished.send` sites plus many direct `run_workflows(...)` calls for the `DOCUMENT_UPDATED` path) must stay green **unchanged**. A test that needs editing signals a behavior change to flag explicitly, not a silent refactor outcome. @@ -238,16 +341,24 @@ Each step is independently reviewable and keeps the test suite green: `run_workflows_added`); delete the `original_file` / `caller_supplied_original_file` parameter plumbing through `run_workflows` and the `execute_*` helpers. -3. Extract the move body from `update_filename_and_move_files` into a callable; - add the `ContextVar` guard; `run_workflows` invokes the move once after the - run completes. The `filename` / `archive_filename` exclusion in the - per-workflow save is kept; only the comment at `handlers.py:980-984` is - updated to describe the new flow. +3. Extract the move body from `update_filename_and_move_files` into a callable + (updating the version-document recursive call site at `handlers.py:670-673` + to call it directly); add the `ContextVar` guard; `run_workflows` invokes the + move once after the run completes. The `filename` / `archive_filename` + exclusion in the per-workflow save is kept; only the comment at + `handlers.py:1019-1027` is updated to describe the new flow. ## Pain points addressed - **Dual-mode** → eliminated by the `Protocol` + two contexts; no `use_overrides`. -- **File staging** → `source_file` is a context property; side-channel args deleted. + Still an open, unfixed problem in current code — this is the refactor's main + remaining justification. +- **File staging** → `source_file` is a context property; side-channel args + deleted. Still an open, unfixed problem in current code. - **Rename race** → per-workflow save under a `ContextVar` guard that suppresses the mid-workflow rename; a single explicit rename runs once at the end against - final state. + final state. **No longer an active bug** — #12389 and #13178 independently + closed the two concrete failure modes (cross-process already-moved file; + intra-workflow tag/m2m clobbering unsaved fields) by narrower means. The + guard is now valuable as a single general mechanism replacing two + independent point-fixes, not as a bug fix.