fix: mark project file as failed when export worker errors#4601
fix: mark project file as failed when export worker errors#4601officialasishkumar wants to merge 3 commits intoOpenFn:mainfrom
Conversation
ExportWorker.perform/1 set the ProjectFile status to :in_progress at the start of the export but never updated it to :failed when the with chain encountered an error. With max_attempts: 1, this left the record permanently orphaned with status :in_progress and path nil. The data retention cron then crashed when iterating over these orphaned records because it passed nil to Lightning.Storage.delete/1, generating ~56,000 Sentry events. Fix both problems: - Restructure the with/else block in perform/1 to call a new mark_project_file_failed/1 helper on any error, setting the ProjectFile status to :failed before returning the error to Oban. - Add a nil-path guard clause in remove_expired_files_for/1 so that orphaned files are logged and deleted from the database without attempting storage deletion. Closes OpenFn#4454 Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
There was a problem hiding this comment.
Pull request overview
Fixes ExportWorker and data retention behavior to prevent project_files records from being left orphaned (:in_progress with path: nil) after export failures, and avoids cron crashes when cleaning up expired files.
Changes:
- Restructures
ExportWorker.perform/1error handling to mark the ProjectFile as:failedon export failure. - Adds a nil-path branch in data retention cleanup to delete orphaned ProjectFile records without calling storage deletion.
- Adds tests covering both the ExportWorker failure case and nil-path retention cleanup, plus a changelog entry.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
lib/lightning/workorders/export_worker.ex |
Ensures failed exports mark ProjectFiles as failed via a helper and refactors success/error flow. |
lib/lightning/projects.ex |
Guards against nil paths during retention cleanup and deletes orphaned records safely. |
test/lightning/export_worker_test.exs |
Adds test asserting ProjectFile is marked :failed when export fails. |
test/lightning/projects_test.exs |
Adds test asserting nil-path orphaned ProjectFiles are deleted by retention job. |
CHANGELOG.md |
Documents the fix for export failure orphaning and retention cron crashes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {:ok, project_file} = | ||
| update_project_file(project_file, %{ | ||
| status: :completed, | ||
| path: storage_path | ||
| }) |
There was a problem hiding this comment.
update_project_file/2 is pattern-matched with {:ok, project_file} = .... If the update returns {:error, changeset} (or any non-{:ok, _} tuple), this will raise a MatchError, bypass the with ... else handling, and can leave the ProjectFile stuck :in_progress again. Consider keeping this update inside the with chain (or handling its {:error, _} result explicitly) so failures are routed through the else branch and mark_project_file_failed/1 runs.
There was a problem hiding this comment.
Good catch. Moved the update_project_file call into the with chain so a failed update now flows into the else branch and triggers mark_project_file_failed instead of raising a MatchError.
| defp mark_project_file_failed(project_file_id) do | ||
| case Repo.get(Projects.File, project_file_id) do | ||
| nil -> | ||
| :ok | ||
|
|
||
| project_file -> | ||
| project_file | ||
| |> Projects.File.mark_failed() | ||
| |> Repo.update() | ||
| end |
There was a problem hiding this comment.
mark_project_file_failed/1 ignores the result of Repo.update/1. If this update fails, the worker will still return {:error, reason} but the ProjectFile may remain :in_progress. Consider handling {:error, changeset} here (e.g., log it) so failures to mark the record as failed are observable.
There was a problem hiding this comment.
Agreed. Added a case clause that logs the changeset errors via Logger.error when Repo.update fails, so the failure is visible in logs rather than silently swallowed.
Collapse single-line expressions to satisfy mix format --check-formatted. Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
Moved the project file update into the with chain to prevent MatchError when the update fails. Added error logging when mark_project_file_failed encounters a repo update failure. Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
Description
This PR fixes two related bugs where the ExportWorker leaves ProjectFile records stuck as
:in_progresswith a nil path on failure, and the data retention cron crashes when encountering these orphaned records.Problem 1:
ExportWorker.perform/1sets the ProjectFile status to:in_progressearly in itswithchain but never updates it to:failedwhen a subsequent step errors. Withmax_attempts: 1, Oban does not retry, leaving the record permanently orphaned.Problem 2:
remove_expired_files_for/1inProjectspassespathtoLightning.Storage.delete/1without checking for nil, causing crashes on orphaned records. This generated ~56,000 Sentry events.Fix 1: Restructure the
with/elseblock to call a newmark_project_file_failed/1helper on any error path, setting the status to:failedbefore returning the error to Oban.Fix 2: Add a nil-path guard clause in
remove_expired_files_for/1that logs a warning and deletes the orphaned database record without attempting storage deletion.Closes #4454
Validation steps
mix test test/lightning/export_worker_test.exsmix test test/lightning/projects_test.exs:failed(not left as:in_progress).Additional notes for the reviewer
mark_project_file_failed/1helper re-fetches the ProjectFile by ID rather than relying on the variable from thewithchain, because the binding may not exist if the failure occurred beforeget_project_filesucceeded.AI Usage
Pre-submission checklist
/reviewwith Claude Code)
(e.g.,
:owner,:admin,:editor,:viewer)