Commit 275bcfb6 authored by Kamil Trzciński's avatar Kamil Trzciński

Merge branch 'fix/build-erase-race-condition' into 'master'

Avoid race condition when expiring artifacts

## What does this MR do?

It may happen that job which purpose is to remove expired artifacts will be executed asynchronously when, in the meantime, project associated with given build gets removed by another async job. In that case we should not remove artifacts because such build will be eventually removed anyway along with artifacts, when project removal is complete.

## Does this MR meet the acceptance criteria?

- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- Tests
  - [x] Added for this feature/bug
  - [x] All builds are passing

## What are the relevant issue numbers?

Closes #22296

See merge request !6881
parents 4e6af0c3 b63b13f9
...@@ -2,6 +2,7 @@ Please view this file on the master branch, on stable branches it's out of date. ...@@ -2,6 +2,7 @@ Please view this file on the master branch, on stable branches it's out of date.
## 8.13.0 (2016-10-22) ## 8.13.0 (2016-10-22)
- Avoid race condition when asynchronously removing expired artifacts. (!6881)
- Improve Merge When Build Succeeds triggers and execute on pipeline success. (!6675) - Improve Merge When Build Succeeds triggers and execute on pipeline success. (!6675)
- Respond with 404 Not Found for non-existent tags (Linus Thiel) - Respond with 404 Not Found for non-existent tags (Linus Thiel)
- Truncate long labels with ellipsis in labels page - Truncate long labels with ellipsis in labels page
......
...@@ -2,10 +2,14 @@ class ExpireBuildInstanceArtifactsWorker ...@@ -2,10 +2,14 @@ class ExpireBuildInstanceArtifactsWorker
include Sidekiq::Worker include Sidekiq::Worker
def perform(build_id) def perform(build_id)
build = Ci::Build.with_expired_artifacts.reorder(nil).find_by(id: build_id) build = Ci::Build
return unless build .with_expired_artifacts
.reorder(nil)
.find_by(id: build_id)
Rails.logger.info "Removing artifacts build #{build.id}..." return unless build.try(:project)
Rails.logger.info "Removing artifacts for build #{build.id}..."
build.erase_artifacts! build.erase_artifacts!
end end
end end
...@@ -6,12 +6,17 @@ describe ExpireBuildInstanceArtifactsWorker do ...@@ -6,12 +6,17 @@ describe ExpireBuildInstanceArtifactsWorker do
let(:worker) { described_class.new } let(:worker) { described_class.new }
describe '#perform' do describe '#perform' do
before { build } before do
worker.perform(build.id)
subject! { worker.perform(build.id) } end
context 'with expired artifacts' do context 'with expired artifacts' do
let(:build) { create(:ci_build, :artifacts, artifacts_expire_at: Time.now - 7.days) } let(:artifacts_expiry) { { artifacts_expire_at: Time.now - 7.days } }
context 'when associated project is valid' do
let(:build) do
create(:ci_build, :artifacts, artifacts_expiry)
end
it 'does expire' do it 'does expire' do
expect(build.reload.artifacts_expired?).to be_truthy expect(build.reload.artifacts_expired?).to be_truthy
...@@ -26,8 +31,23 @@ describe ExpireBuildInstanceArtifactsWorker do ...@@ -26,8 +31,23 @@ describe ExpireBuildInstanceArtifactsWorker do
end end
end end
context 'when associated project was removed' do
let(:build) do
create(:ci_build, :artifacts, artifacts_expiry) do |build|
build.project.delete
end
end
it 'does not remove artifacts' do
expect(build.reload.artifacts_file.exists?).to be_truthy
end
end
end
context 'with not yet expired artifacts' do context 'with not yet expired artifacts' do
let(:build) { create(:ci_build, :artifacts, artifacts_expire_at: Time.now + 7.days) } let(:build) do
create(:ci_build, :artifacts, artifacts_expire_at: Time.now + 7.days)
end
it 'does not expire' do it 'does not expire' do
expect(build.reload.artifacts_expired?).to be_falsey expect(build.reload.artifacts_expired?).to be_falsey
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment