Skip to content
Snippets Groups Projects
Verified Commit 41487915 authored by Hordur Freyr Yngvason's avatar Hordur Freyr Yngvason :baby:
Browse files

Fix duration for builds failed via Ci::Build#doom!

The duration of a build is `finished_at - started_at`. This method was
marking a build as failed without setting `finished_at`, resulting in
its duration appearing ever-growing.

Changelog: fixed
parent 059d8e46
No related branches found
No related tags found
1 merge request!136458Fix duration for builds failed via Ci::Build#doom!
......@@ -921,13 +921,25 @@ def report_artifacts
# Consider this object to have a structural integrity problems
def doom!
transaction do
update_columns(status: :failed, failure_reason: :data_integrity_failure)
now = Time.current
attributes = {
status: :failed,
failure_reason: :data_integrity_failure,
updated_at: now
}
attributes[:finished_at] = now unless finished_at.present?
update_columns(attributes)
all_queuing_entries.delete_all
all_runtime_metadata.delete_all
end
deployment&.sync_status_with(self)
::Gitlab::Ci::Pipeline::Metrics
.job_failure_reason_counter
.increment(reason: :data_integrity_failure)
Gitlab::AppLogger.info(
message: 'Build doomed',
class: self.class.name,
......
......@@ -5229,16 +5229,34 @@ def run_job_without_exception
subject { build.doom! }
let(:traits) { [] }
let(:build) { create(:ci_build, *traits, pipeline: pipeline) }
let(:build) do
travel(-1.minute) do
create(:ci_build, *traits, pipeline: pipeline)
end
end
it 'updates status and failure_reason', :aggregate_failures do
subject
it 'updates status, failure_reason, finished_at and updated_at', :aggregate_failures do
old_timestamp = build.updated_at
new_timestamp = \
freeze_time do
Time.current.tap do
subject
end
end
expect(old_timestamp).not_to eq(new_timestamp)
expect(build.updated_at).to eq(new_timestamp)
expect(build.finished_at).to eq(new_timestamp)
expect(build.status).to eq("failed")
expect(build.failure_reason).to eq("data_integrity_failure")
end
it 'logs a message' do
it 'logs a message and increments the job failure counter', :aggregate_failures do
expect(::Gitlab::Ci::Pipeline::Metrics.job_failure_reason_counter)
.to(receive(:increment))
.with(reason: :data_integrity_failure)
expect(Gitlab::AppLogger)
.to receive(:info)
.with(a_hash_including(message: 'Build doomed', class: build.class.name, build_id: build.id))
......@@ -5273,12 +5291,20 @@ def run_job_without_exception
context 'with running builds' do
let(:traits) { [:picked] }
it 'drops associated runtime metadata' do
it 'drops associated runtime metadata', :aggregate_failures do
subject
expect(build.reload.runtime_metadata).not_to be_present
end
end
context 'finished builds' do
let(:traits) { [:finished] }
it 'does not update finished_at' do
expect { subject }.not_to change { build.reload.finished_at }
end
end
end
it 'does not generate cross DB queries when a record is created via FactoryBot' do
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment