Packages cleanup artifact worker: avoid updating a deleted record
🎞 Context
Follow up of !84073 (merged) as part of #357373 (closed).
Read the Context section there.
After deploying the above fix, we saw the following errors:
Basically, this line gets executed on an Active Record instance that has already been destroyed. This is not allowed
ActiveRecord::ActiveRecordError: cannot update a destroyed record
This MR fixes this issue.
🔮 Further considerations
I think that the error handling is not done properly. Why would we want to update the status
on the target artifact (in this case a package file marked as pending_destruction
)?
The related worker has a very simple task: pick up a pending_destruction
package file and #destroy!
it. If there is any error while accomplishing this task, then the package file record should stay as it is. It will be retried on the next run.
In other words, I don't see the value of the error
and processing
statuses. From my point of view, it only brings more things to do for the worker where it should only select a package file and #destroy!
it.
If those statuses were meant to improve observability then we already have that. The worker logs the package file it selected. Job failures are already logged with a fail
message.
I would even argue that silencing all the errors is not a great idea as we don't know what was the original error. I would rather let the error bubble up and let Sidekiq do its job that is configured to do: log the error and the backtrace.
I don't want to deal with this aspect here, so I opened #358381 to improve the error handling. Here, I just want a quick fix for this situation as this error is making package file cleanup worker executions fail = we are currently piling up package files to destroy on gitlab.com.
🔬 What does this MR do and why?
- In
app/workers/concerns/packages/cleanup_artifact_worker.rb
, do not update thestatus
if the record has been#destroyed?
- Update the package file worker related spec
📺 Screenshots or screen recordings
n / a
🔧 How to set up and validate locally
Reproducing the conditions where a record is deleted but we still throw an error is not easy to do locally.
For a package file worker execution, see !84073 (merged)
🚦 MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.