fix: keep media references when a file cannot be deleted

[itsmiso-ai]

Closes #138

What's new?

N/A

What's changed?

• Media.delete_media_files/2 and Media.delete_media_item/2 now attempt every attached file and, when one cannot be removed, return {:error, {:file_removal_failed, [{filepath, reason}]}} instead of unconditionally returning {:ok, media_item}. On failure the record, its filepath attributes and addl_attrs are left untouched, and the media_deleted user script does not fire. The failed paths are logged once as a warning at the detection point.
• {:error, :enoent} is treated as success: an already-missing file is the state deletion wanted, so retrying a partially-completed deletion converges.
• Media.delete_media_item/2 gained an ignore_file_removal_errors option, used by Sources.delete_source/2: media_items.source_id is references(:sources, on_delete: :restrict), so a surviving media row would make the source cascade raise instead of deleting. When set, the record is deleted anyway with a warning, and the media_deleted hook is suppressed (documented on the option).
• The media delete controller action flashes :error naming the failed files instead of "Files deleted successfully.", and no longer hard-matches {:ok, _}.
• Regression tests: context tests for failed removal (references preserved, other files still removed, warning logged, hook not fired, retry convergence, already-gone files), a delete_source/2 test proving the cascade deletes despite an undeletable media file, and a controller test asserting the error flash and retained reference.

What's fixed?

An I/O, permission or read-only-filesystem failure during file deletion used to leave the file on disk while the stored filepaths were cleared and the media_deleted hook fired, so the record stopped pointing at the retained file, the deletion could not be retried through it, and the UI claimed success.

Invariants this change must keep (and every other path enforcing the same rule)

1. A failed file removal never clears filepath references or deletes the record. Consumers: MediaItemController.delete/2 (handles {:error, {:file_removal_failed, _}} and a generic error tuple with an error flash + redirect; regression test added); MediaRetentionWorker (media_retention_worker.ex:45 and :70 — discards the result, so the new error tuple cannot crash it, and because references are retained the next daily run re-selects and retries the same items). No other call sites exist in lib/ or test/.
2. media_deleted fires only on complete success (removed or already-gone). Single gate in media.ex (delete_attached_files_if_required / the with in delete_media_files/2); the ignore_file_removal_errors suppression is documented and pinned by tests both ways (fires on convergence retry, does not fire on failure).
3. Cascading deletion never raises on an undeletable media file. The on_delete: :restrict FK is real (priv/repo/migrations/20240125025325_create_media_items.exs:9); Sources.delete_source/2 passes ignore_file_removal_errors: true (test added in sources_test.exs); Profiles.delete_media_profile/2 → Sources.delete_source/2 and MediaProfileDeletionWorker inherit this through the same chain, so they never see the new tuple. tasks.media_item_id (:restrict) is covered by the pre-existing Tasks.delete_tasks_for/1 call that still runs first.
4. Retry converges. :enoent maps to success in delete_attached_file/1, so files removed by a partially-failed first attempt never re-block the retry (pinned by the retry test).
5. Empty-parent-directory cleanup keeps its warning-only contract. FilesystemUtils.delete_file_and_remove_empty_directories/1 is unchanged except its @doc caller list; the remaining discarding callers (Media.delete_internal_metadata_files/1, Sources.delete_source_files/1, Sources.delete_internal_metadata_files/1, FileSyncing.handle_file_deletion/2) are untouched, matching the issue's explicit scope exclusion.

Any other comments?

Opened by the Courier autonomous loop for audit issue #138.

Validation status: the lane environment has no Elixir/OTP toolchain and no container runtime (verified: mix/elixir/docker absent on both coordinator and worker machines), so mix test, mix check, credo and the formatter could not be executed locally — CI ("Lint and Test") is the first executable gate and is explicitly pending. What was done locally: the full diff was read line-by-line, the on_delete: :restrict FK claim and the complete caller inventory (MediaItemController, MediaRetentionWorker, Sources.delete_source → Profiles.delete_media_profile) were verified against the code, and two independent static review passes over the final diff (including the test additions) reported no blockers.

• I am the original author of this code, and I am giving it freely to the community and Pinchflat project maintainers
• I confirm this is a critical issue that needs to be addressed in this fork

[itsmiso-ai]

Fix-up addressing the Lint and Test blocker on the previous head (0e66446).

The single failing test — delete_media_files/2 when a file cannot be removed logs a warning naming the file it could not remove (test/pinchflat/media_test.exs) — was relying on ExUnit.CaptureLog.capture_log([level: :warning], …). Per the ExUnit.CaptureLog.capture_log/2 Elixir 1.17 docs, the :level option does not override the primary Logger.level/0, and config/test.exs:37 pins that to :critical. The warning was filtered out before the capture handler saw it, so capture_log returned "".

Fix: switched to the same Logger.configure(level: :warning) + on_exit restore pattern already used in test/pinchflat/utils/filesystem_utils_test.exs:213-228 and test/pinchflat/slow_indexing/slow_indexing_helpers_test.exs:709-716. The capture handler is now invoked at the configured level and sees the warning.

Info-level review finding addressed — Media.delete_media_item/2's docstring now states that internal metadata files (NFO, thumbnail) also linger on a failed file removal, alongside the existing "tasks already gone" note. The old code removed them unconditionally; the new code leaves them with the record so a retry can finish, which the previous docstring was silent on.

Local validation in this lane

• yarn run lint:check (Prettier 3.9.6 via package.json): ✅ all matched files use Prettier code style.
• mix format --check-formatted, mix credo, mix check --no-fix --no-retry, mix test: could not run — the lane has no Elixir/OTP toolchain and no container runtime (mix/elixir/docker all absent, verified). The Lint and Test CI job is the first executable gate; it is re-running on the new head ecc24a0 and remains pending at handoff.
• Static checks: full diff read end-to-end; the test change matches the established capture pattern verbatim (only the level-bump and on_exit restore differ across the two existing uses); line lengths ≤ 120; no new warnings introduced; no behaviour change to production code beyond the docstring.

Head: ecc24a0 (push only, no merge). PR left open.

[github-actions]

🐳 Image for commit ecc24a0 published: ghcr.io/misospace/pinchflat-dev:pr-149-ecc24a0

Superseded by a newer automated review for this pull request.

[its-saffron]

AI Automated Review

Full PR review.

Analysis engine: glm-5.3-flash (openai) — routed smart (risk match: file_serving_changes) · pr-reviewer-action v3.2.0

Partial coverage: required-check coverage is incomplete — this review did not resolve every required check and must not be read as a complete pass.

Recommendation: Approve. The PR correctly implements issue PR 138: file-removal failures now propagate as {:error, {:file_removal_failed, [{filepath, reason}]}}, filepath references and the record are preserved, the media_deleted hook fires only on complete success, and the controller reports the failed files instead of claiming success. CI ("Lint and Test", plus both image builds) is green on the head commit ecc24a0. All five PR claims were checked against the code and held.

Change-by-change findings

• lib/pinchflat/media/media.ex — delete_media_item/2 now gates record deletion behind delete_attached_files_if_required/3 via a with, and delete_media_files/2 gates the hook + filepath-attribute reset behind do_delete_media_files/1. Both return the new error tuple on failure. The :enoent-as-success mapping in delete_attached_file/1 is what makes retries converge, and it is pinned by tests both ways ("a retry converges once the blocker is cleared", "resets the filepath attributes when the only missing file was already gone"). The ignore_file_removal_errors option suppresses the hook and deletes the record anyway with a warning — exactly as documented in the new @doc. The intentional behavior change (internal metadata files now linger with the record on a failed deletion instead of being removed "no matter what") is explicitly documented in the docstring, which is the correct call: deleting metadata that describes a record you are keeping would recreate the reference-loss bug.
• lib/pinchflat_web/controllers/media_items/media_item_controller.ex — the hard {:ok, _} = match is gone; the action now handles {:error, {:file_removal_failed, failures}} (error flash naming the files) and a generic {:error, _} fallback. Flash content is HTML-escaped by Phoenix on render, so interpolating stored filepaths is safe.
• lib/pinchflat/sources/sources.ex — delete_source/2 passes ignore_file_removal_errors: true, which is required because media_items.source_id is references(:sources, on_delete: :restrict) (confirmed in priv/repo/migrations/20240125025325_create_media_items.exs:9); without it a surviving media row would raise on the cascade. The chain Profiles.delete_media_profile/2 → Sources.delete_source/2 → Media.delete_media_item/2 and MediaProfileDeletionWorker → Profiles.delete_media_profile/2 (verified in lib/pinchflat/profiles/profiles.ex:56-64 and media_profile_deletion_worker.ex:34) inherit the suppression, so no cascade caller sees the new tuple.
• lib/pinchflat/utils/filesystem_utils.ex — doc-only change; the :ok-on-partial-cleanup-failure contract and the error propagation from File.rm/1 are unchanged, and the updated caller list matches the actual call sites found by grep (media.ex:267, media.ex:281, sources.ex:184, sources.ex:194, file_syncing.ex:67).
• lib/pinchflat/downloading/media_retention_worker.ex (unchanged, verified) — both call sites (:45, :70) discard the result inside Enum.each, so the new error tuple cannot crash the worker; because a failed cull leaves the record and its media_filepath intact and does not set prevent_download/culled_at, the item remains matched by MediaQuery.cullable/0 (downloaded + past_retention_period + not culling_prevented) and is re-selected on the next daily run. The claim's retry story holds.
• Tests — 202 added lines covering: error return with references preserved, other files still removed, warning logged (with the Logger.configure level-bump pattern matching the existing filesystem_utils_test.exs idiom), hook not fired on failure / fired on convergence retry and on already-gone files, ignore_file_removal_errors deleting anyway, source cascade surviving an undeletable media file, and the controller error flash with retained reference. All new test paths are derived from fixture locations via Path.dirname/Path.join — no absolute checkout paths.

Claims to falsify — results

• Claim 1 (error tuple from delete_media_files/2 / delete_media_item/2) — held (12/12 listed items + sibling scan). Verified in media.ex (do_delete_media_files → log_file_removal_failures returns the tuple; both public functions gate on :ok). A repo-wide grep for delete_media_files|delete_media_item found no production call site beyond the retention worker (discarding), sources.ex:148, and the controller — none missed by the list.
• Claim 2 (controller error flash, no hard {:ok, _} match) — held (12/12). The controller diff replaces the match with a three-way case; the error flash names the failed files via file_removal_failure_message/1.
• Claim 3 (old behavior lost references and fired the hook on failure) — held (6/6). Consistent with the issue's line-level evidence; the new single gate (run_user_script(:media_deleted, ...) appears only at media.ex:223 and media.ex:289, both inside success branches) is confirmed, and repo-wide grep shows no other production emitter of the :media_deleted event.
• Claim 4 (failed removal never clears references or deletes the record; consumers safe) — held (11/11). Controller handles both error shapes; MediaRetentionWorker discards results at both sites; the :restrict FK is real at the cited migration line; Sources.delete_source/2 suppression and the Profiles/MediaProfileDeletionWorker inheritance chain verified.
• Claim 5 (no other call sites; hook single-gated; cascade never raises) — held (12/12). Grep over lib/ and test/ found no additional callers of the two functions or of delete_file_and_remove_empty_directories beyond those accounted for; the discarding callers named in the updated @doc all match real, unchanged call sites.

Standards Compliance

• Alias nested modules (Credo AliasUsage) — followed: all new FilesystemUtils calls in media.ex use the aliased form, consistent with the file's pre-existing usage; the head compiles and CI is green, confirming the alias is in place.
• Module name matches filename — no new modules introduced; all touched files keep their existing name correspondence.
• Queue names — no new workers or queues; the touched worker already uses the existing local_data queue.
• No assumed checkout paths — new tests derive every path from fixture locations; no absolute paths added.
• Docs — the behavior change is documented where it belongs (the @doc on delete_media_item/2 and delete_source/2, plus the filesystem_utils.ex caller contract). AGENTS.md/CODEBASE.md/DEVELOPMENT.md do not document media-deletion error behavior, so no companion-doc update is required.

Linked Issue Fit

Issue PR 138's acceptance criteria are all met: (1) deterministic removal failures (:eacces/:erofs class) return an actionable error identifying the file without clearing its filepath or deleting the record — tested; (2) no success hook or success flash on failure — hook suppression tested with expect(..., 0, ...) and the controller test asserts the error flash and absence of info; (3) partial deletion handled deliberately — other files are still removed, :enoent converges retries, and the retry test pins completion; (4) context, controller, and source-cascade regression tests added, successful deletions and the parent-directory warning contract intact; (5) targeted tests and mix check --no-fix --no-retry ran green in CI on the head commit. The scope exclusion (empty-parent-directory cleanup) is respected — filesystem_utils.ex changed doc-only.

Tool Harness Findings

28 read-only calls: repo-wide greps for delete_media_files|delete_media_item, media_deleted, and delete_file_and_remove_empty_directories (call-site census above); full reads of media.ex (160–309), the controller delete action, sources.ex delete path, profiles.ex, media_retention_worker.ex, filesystem_utils.ex (120–179), the create_media_items migration, media_query.ex cullable/cutoff dynamics, media_item.ex filepath attributes, and the fixture/helper files the new tests rely on. Nothing contradicted the PR's claims; no additional unaccounted callers exist.

Must-check dispositions summary

• File path sanitization / directory traversal (×2) / path traversal review — not applicable to this change: no new path construction from external input; deletion targets only filepath attributes already stored on the record, and the flash output is HTML-escaped.
• Audit findings addressed — satisfied: every acceptance criterion in issue PR 138 is implemented and tested as detailed above.
• Edge-case paths (null bytes, symlinks) — satisfied by design: any File.rm/1 error (including exotic reasons) now fails closed as a surfaced, logged error instead of being silently swallowed, and the new tests exercise the non-regular-file edge case (a directory where a file is expected); symlink semantics via File.rm/1 are unchanged. No traversal surface is introduced.

Requirement trace

4 of 5 requirement(s) not fully traced to enforcement and a test:

• req-21433a4da967 — unverifiable (no valid enforcement location)
• req-f082fd28d0b2 — unverifiable (no valid enforcement location)
• req-259d4903f81b — unverifiable (no valid enforcement location)
• req-cec06a117f01 — unverifiable (no valid enforcement location)

Approval withheld: this review's coverage is incomplete — required-check coverage or the tool-loop investigation did not finish, so it is publishing as an advisory comment rather than an approval.