Skip to content

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:

Screenshot_2022-04-08_at_10.10.12

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 the status 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.

Edited by David Fernandez

Merge request reports