refac: atomic merge, lost-update race, RunPurge cost, duplicated walks

features/media_merge_hardening.qa.md

@@ -0,0 +1,80 @@
+# QA procedure: media_merge_hardening
+
+Package: `test` (black-box). New file `test/media_merge_hardening_test.go`. Go only: no SPA, no env var, no live server, no real SQLite file, no real SMTP. Fake type names must be unique in the package.
+
+## Setup shared by every case
+1. Call `validator.InitValidator()` once per test.
+2. Reuse the fakes of the media tests and the purge tests: `mockImageRepo`, `mockArticleRepo`, the site, author and storage fakes, the variant file fake, the audit recorder fake, `mockContentTypeRepo`, `mockEntryRepo`, `mockPurgeSettingsRepo`, and the helpers `seedGroup` and `articleUsing`.
+3. New fake `mockMergeTransactor` (implements `transaction.Transactor`):
+   - It holds the fakes of step 2 and hands them to the function as the `Repositories` struct. In-memory fakes need no real binding to the transaction.
+   - Rollback emulation: before calling the function it snapshots the state of every fake it holds (a deep copy of their maps), and it restores the snapshots when the function returns an error. Counters (`updateCalls`, `getCalls`, `deleteCalls`) are NOT restored, so tests can still assert that calls happened.
+   - Switches: `failBegin`, `failCommit`. `failCommit` restores the snapshots and returns the error after the function returned nil.
+   - Fields: `runCalls`, and a `committed bool` that is true only between the commit and the next Run.
+   - Each fake gains `snapshot()` and `restore()` helpers in the test package only; no production code change.
+4. Fake extensions (no new method on a domain interface):
+   - `mockEntryRepo`: `listPageCalls`, `getCalls`, `failUpdateOn(n)` (already exists), `failGetOn(n)`, and `afterListPage func()` called right after `ListPage` builds its result, used to mutate or delete an entry after the scan.
+   - `mockContentTypeRepo`: `listCalls`.
+   - `mockArticleRepo`: failure switches for `UpdateBody` (per id and per call number) and `RepointImage`.
+   - Site and author fakes: failure switches for `RepointImage` and `RepointAvatar`.
+   - `mockImageRepo`: `Delete` with `failDeleteFor`.
+   - Storage fake: records `Delete` calls, with a failure switch per stored name; each record notes `transactor.committed` at call time.
+   - Variant file fake: the same recording and failure switch.
+5. Build `NewMergeMediaUseCase(...)` with the transactor, storage, variant files, recorder, the fakes, an injected `*log.Logger` over a `bytes.Buffer`, and `NewRunPurgeUseCase` / `NewPreviewPurgeUseCase` over the same fakes. The constructors are chosen by the coder. Mount the media handler on a fresh `chi.NewRouter()` mirroring `NewMediaRouter` in api/router.go, with the media_purge.qa.md setup, `jwtauth.New("HS256", []byte("test-secret"), nil)` and an admin 2 cookie. Compare error bodies as `message + "\n"`.
+6. Seed: images 7, 8 and 9 (same sha256 A), 2 and 3 (sha256 B). The type "event" has title and thumbnail (media, optional), plus a second type with two media fields (thumbnail, poster) for the walk and index cases. Seed articles, a site and an author that point at the copies. Inject a fixed clock so entry `updated_at` is known.
+7. Snapshot helper `libraryState()` deep-copying every fake. Assert "unchanged" with `reflect.DeepEqual` of before and after.
+
+## All-or-nothing
+8. Failing step table (the 9 rows of the first outline), one subtest per row. Arm the failure, POST `/v1/media/merge` with keep 7 and merge [9]. Assert 500 and "Internal server error\n". Assert `libraryState()` equals the state before, including bodies, cover ids, site and author references, entries with `updated_at` and `updated_by`, and both image rows. Assert the storage fake and the variant fake recorded no `Delete` call. Assert exactly one failed `media.merge` audit event. Use `failCommit` for the commit row.
+9. `failBegin`: 500, no write call on any fake, no file call.
+10. Two copies (8 and 9), `failDeleteFor(9)` after 8 was deleted: 500, image 8 exists again after rollback, all else unchanged.
+11. Success path: 200 and the unchanged body, entries 41 and 42 hold 7 with `created_by`, `updated_by`, `created_at` and `updated_at` unchanged, image 9 gone, the storage fake got `Delete`, and the audit event is a success with `merged` and `articles_updated`.
+12. Validation, lookup and 409 requests (all existing 400, 404, 409 cases): `runCalls == 0`.
+13. Rollback and `updated_at` / `updated_by`: arm `failUpdateOn(2)` on the entry fake, assert both entries are `DeepEqual` to the seed.
+
+## Files after commit
+14. Success: the storage fake `Delete` record has `committed == true` for every call; the variant fake records likewise. Failure rows of step 8: no records at all.
+15. File failure table (storage `Delete` fails for 9; variant purge fails for 9) with copies 8 and 9: 200 and the same body as the no-failure run, images gone, repoints kept, the other copy's removal still attempted, the log buffer holds one line with the stored name and the error text, the audit event is a success with the usual params.
+16. Purge group with a storage failure after commit: 200, `groups_purged` 2, `failed_groups` 0, merged count includes the copies of group A, the log buffer holds the failure.
+
+## Concurrent entry edit
+17. Edit after the scan: set `afterListPage` to rewrite entry 41 to a new title with new `updated_at` and `updated_by`. After the merge, entry 41 has the new title, thumbnail 7 and the edit's `updated_at` and `updated_by`.
+18. Edit that moves the entry to image 8 after the scan: entry 41 still holds 8 and the entry fake saw no `Update` for 41.
+19. Entry deleted after the scan: 200, image 9 gone, no error.
+20. Re-read data: an entry with thumbnail 9 and poster 9: both become 7, the title is kept, `Update` is called once for that entry.
+
+## Concurrent article edit
+17a. `mockArticleRepo` gains `afterFindByBody func()` called right after `FindByBodyContaining` builds its result. Edit after planning: article 1 gets an extra paragraph; after the merge it holds the paragraph and the kept stored name. Edit removing the copy name: no `UpdateBody` for article 1, not in `articles_updated`. Article deleted after planning: 200, image 9 gone. Suffix collision (`a.jpg` inside `ba.jpg`): `ba.jpg` untouched. Add a property-style table for the replacement helper (identity without the copy name, idempotence, kept name never removed).
+
+## Cancelled purge
+17b. Cancel the context from a fake hook after group A commits: A merged, B and C untouched, `Execute`/`Run` return the context error, `failed_groups` unchanged, no `last_purge_*` saved, no success `media.purge` audit event. Scheduler: `Execute` receives the scheduler context (no more `context.Background()`), cancelled between groups it stops the same way.
+
+## One scan per run and per preview
+21. Preview over 6 groups, 40 entries, 2 media types: `contenttype listCalls == 1`, `entry listPageCalls == 2`. Body equals the body from before the slice (same seed in an existing test).
+22. Run over the same seed: `listCalls == 1`, `listPageCalls == 2`, and the response and stored `last_purge_*` match.
+23. Stale index: an entry with thumbnail in group A and poster in group B ends with both kept ids, `Update` called twice for it.
+24. Media-used-only-by-entry group (existing step 30 of media_field_type.qa.md): the used copy is kept, the preview and the run each scan once.
+
+## Purge per group
+25. Failure table (UpdateBody of the second article, article RepointImage, entry Update, image Delete for a merged copy) armed for group B in plan order A, B, C: 200 and the counters of the feature, group B fully rolled back, A and C merged, `Save` counters equal the response.
+26. All groups failing: 200, `groups_purged` 0, `failed_groups == groups_total`, state unchanged, `last_purge_at` saved.
+27. Three groups: `transactor.runCalls == 3`.
+
+## Amended existing tests (the coder owns them)
+28. media_library test "failure deleting a copy": bodies are now restored and image 3 still exists. media_purge step 30: group A's bodies are restored (no longer switched), failed_groups 1, groups_purged 1, A contributes 0, the next run converges.
+29. Every other existing test passes unchanged. Constructors of `MergeMedia`, `RunPurge` and `PreviewPurge` gain the transactor and logger; update the call sites in tests only.
+
+## Behaviour unchanged (walk refactor)
+30. No new case. Run the full existing suites (`task test`) and require them to pass, as the pure-refactor proof.
+
+## Layering guards
+31. `internal/domain/transaction` imports only the stdlib and `internal/domain/*`. The use cases import no `persistence`, `gorm`, `models` or `net/http`. The GORM adapter asserts `var _ transaction.Transactor = &X{}` and is constructed only in api/dependecyInjection.go. Use the grep-style test of media_purge.qa.md.
+
+## Manual verification (not exercised by the Go tests)
+- Real SQLite, then MySQL/MariaDB and PostgreSQL via the driver tasks: seed duplicates with articles, a site, an author and entries on a copy; make the image delete fail (for example a read-only DB or a trigger); confirm no row changed.
+- Real merge against real uploads: the copy's file and variant files are gone after a 200, and still present after a forced failure.
+- Concurrent merge and entry edit with two shells (curl with the jwt cookie): the edit survives.
+- Time `POST /v1/media/purge` with many groups and 1,000 entries (one scan, not one per group).
+- Check that a SQLite transaction does not deadlock with the connection settings in `persistence/database` (all reads inside the transaction go through the tx-bound repositories).
+
+## Decisions accepted by the user
+Amend the two earlier scenarios; a file failure after commit is 200 plus a log line (injected `*log.Logger`); one transaction per purge group; entries scanned then re-read per candidate inside the transaction; article bodies are re-read and rewritten inside the transaction (the user chose to fix the body race: the plan only decides which articles change); a purge cancelled by its context stops between groups without recording a run; entries created after the scan that point at a copy are not repointed (accepted limitation, shown as missing); adapter verified by hand, Go tests use fakes with snapshot/restore rollback.