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 (closed) 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

Loading