From 83c35870d888c586fc58c0750ac1336547c13049 Mon Sep 17 00:00:00 2001 From: Katharina Przybill <30441792+kathap@users.noreply.github.com> Date: Fri, 7 Nov 2025 16:24:11 +0100 Subject: [PATCH 1/5] Fix hanging staging on cc-uploader kill MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit add idempotent droplet creation via build lock + find-or-create: - lock build: SELECT * FROM builds WHERE id = $1 FOR UPDATE LIMIT 1; - if there exists already a droplet, take that one and ensure buildpack_lifecycle_data is attached (but don’t overwrite) --- app/actions/droplet_create.rb | 24 +++++--- .../runtime/stagings_controller.rb | 2 +- spec/unit/actions/droplet_create_spec.rb | 59 +++++++++++++++++-- 3 files changed, 71 insertions(+), 14 deletions(-) diff --git a/app/actions/droplet_create.rb b/app/actions/droplet_create.rb index ff3945615d9..b663342e323 100644 --- a/app/actions/droplet_create.rb +++ b/app/actions/droplet_create.rb @@ -49,22 +49,30 @@ def create_docker_droplet(build) droplet end - def create_buildpack_droplet(build) - droplet = droplet_from_build(build) - + def find_or_create_buildpack_droplet(build) DropletModel.db.transaction do + BuildModel.where(id: build.id).for_update.first or + raise "Build #{build.id} not found for locking" + + existing = DropletModel.where(build_guid: build.guid).first + return existing if existing + + droplet = droplet_from_build(build) droplet.save + if build.cnb_lifecycle? droplet.cnb_lifecycle_data = build.cnb_lifecycle_data else droplet.buildpack_lifecycle_data = build.buildpack_lifecycle_data end - end - droplet.reload - Steno.logger('build_completed').info("droplet created: #{droplet.guid}") - record_audit_event(droplet, build.package, user_audit_info_from_build(build)) - droplet + droplet.reload + Steno.logger('build_completed').info("droplet created: #{droplet.guid}") + record_audit_event(droplet, build.package, user_audit_info_from_build(build)) + droplet + end + rescue Sequel::UniqueConstraintViolation + DropletModel.where(build_guid: build.guid).first end private diff --git a/app/controllers/runtime/stagings_controller.rb b/app/controllers/runtime/stagings_controller.rb index aa5400af27b..303db1b4cef 100644 --- a/app/controllers/runtime/stagings_controller.rb +++ b/app/controllers/runtime/stagings_controller.rb @@ -134,7 +134,7 @@ def droplet_from_build(build, guid) end def create_droplet_from_build(build) - DropletCreate.new.create_buildpack_droplet(build) + DropletCreate.new.find_or_create_buildpack_droplet(build) end def upload_path diff --git a/spec/unit/actions/droplet_create_spec.rb b/spec/unit/actions/droplet_create_spec.rb index 93f5584f553..4331dff6647 100644 --- a/spec/unit/actions/droplet_create_spec.rb +++ b/spec/unit/actions/droplet_create_spec.rb @@ -158,13 +158,13 @@ module VCAP::CloudController end end - describe '#create_buildpack_droplet' do + describe '#find_or_create_buildpack_droplet' do context 'buildpack lifecycle' do let!(:buildpack_lifecycle_data) { BuildpackLifecycleDataModel.make(build:) } it 'sets it on the droplet' do expect do - droplet_create.create_buildpack_droplet(build) + droplet_create.find_or_create_buildpack_droplet(build) end.to change { [DropletModel.count, Event.count] }.by([1, 1]) droplet = DropletModel.last @@ -195,6 +195,55 @@ module VCAP::CloudController }) end + it 'creates the droplet once and attaches lifecycle data' do + expect { + droplet_create.find_or_create_buildpack_droplet(build) + }.to change { [DropletModel.count, Event.count] }.by([1, 1]) + + droplet = DropletModel.last + expect(droplet.state).to eq(DropletModel::STAGING_STATE) + expect(droplet.app).to eq(app) + expect(droplet.package).to eq(package) + expect(droplet.build).to eq(build) + + buildpack_lifecycle_data.reload + expect(buildpack_lifecycle_data.droplet).to eq(droplet) + end + + it 'is idempotent on repeated calls (no duplicate droplet, same GUID, no extra event)' do + first = droplet_create.find_or_create_buildpack_droplet(build) + + expect { + second = droplet_create.find_or_create_buildpack_droplet(build) + expect(second.guid).to eq(first.guid) + }.to change(DropletModel, :count).by(0) + .and change(Event, :count).by(0) + + # Ensure still only one droplet for this build + expect(DropletModel.where(build_guid: build.guid).count).to eq(1) + + buildpack_lifecycle_data.reload + expect(buildpack_lifecycle_data.droplet).to eq(first) + end + + it 'returns the pre-existing droplet when one already exists for the build' do + existing = DropletModel.make( + app: app, + package: package, + build: build, + state: DropletModel::STAGING_STATE + ) + buildpack_lifecycle_data.update(droplet: existing) + + expect { + returned = droplet_create.find_or_create_buildpack_droplet(build) + expect(returned.guid).to eq(existing.guid) + }.to change(DropletModel, :count).by(0) + .and change(Event, :count).by(0) + + expect(DropletModel.where(build_guid: build.guid).first.guid).to eq(existing.guid) + end + context 'when the build does not contain created_by fields' do let(:build) do BuildModel.make( @@ -205,7 +254,7 @@ module VCAP::CloudController it 'sets the actor to UNKNOWN' do expect do - droplet_create.create_buildpack_droplet(build) + droplet_create.find_or_create_buildpack_droplet(build) end.to change { [DropletModel.count, Event.count] }.by([1, 1]) droplet = DropletModel.last @@ -230,7 +279,7 @@ module VCAP::CloudController it 'sets it on the droplet' do expect do - droplet_create.create_buildpack_droplet(build) + droplet_create.find_or_create_buildpack_droplet(build) end.to change { [DropletModel.count, Event.count] }.by([1, 1]) droplet = DropletModel.last @@ -271,7 +320,7 @@ module VCAP::CloudController it 'sets the actor to UNKNOWN' do expect do - droplet_create.create_buildpack_droplet(build) + droplet_create.find_or_create_buildpack_droplet(build) end.to change { [DropletModel.count, Event.count] }.by([1, 1]) droplet = DropletModel.last From 47e66618b3f322598a3466b3efd39226c4e619fe Mon Sep 17 00:00:00 2001 From: Katharina Przybill <30441792+kathap@users.noreply.github.com> Date: Mon, 10 Nov 2025 11:30:33 +0100 Subject: [PATCH 2/5] fix Rubocop, when existing droplet is found, ensure lifecycle data is attached before returning --- app/actions/droplet_create.rb | 10 +++++++++- spec/unit/actions/droplet_create_spec.rb | 14 ++++++-------- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/app/actions/droplet_create.rb b/app/actions/droplet_create.rb index b663342e323..1521f5adae6 100644 --- a/app/actions/droplet_create.rb +++ b/app/actions/droplet_create.rb @@ -55,7 +55,15 @@ def find_or_create_buildpack_droplet(build) raise "Build #{build.id} not found for locking" existing = DropletModel.where(build_guid: build.guid).first - return existing if existing + if existing + if build.cnb_lifecycle? + existing.cnb_lifecycle_data ||= build.cnb_lifecycle_data + else + existing.buildpack_lifecycle_data ||= build.buildpack_lifecycle_data + end + existing.save_changes + return existing + end droplet = droplet_from_build(build) droplet.save diff --git a/spec/unit/actions/droplet_create_spec.rb b/spec/unit/actions/droplet_create_spec.rb index 4331dff6647..ff40470b743 100644 --- a/spec/unit/actions/droplet_create_spec.rb +++ b/spec/unit/actions/droplet_create_spec.rb @@ -196,9 +196,9 @@ module VCAP::CloudController end it 'creates the droplet once and attaches lifecycle data' do - expect { + expect do droplet_create.find_or_create_buildpack_droplet(build) - }.to change { [DropletModel.count, Event.count] }.by([1, 1]) + end.to change { [DropletModel.count, Event.count] }.by([1, 1]) droplet = DropletModel.last expect(droplet.state).to eq(DropletModel::STAGING_STATE) @@ -213,11 +213,10 @@ module VCAP::CloudController it 'is idempotent on repeated calls (no duplicate droplet, same GUID, no extra event)' do first = droplet_create.find_or_create_buildpack_droplet(build) - expect { + expect do second = droplet_create.find_or_create_buildpack_droplet(build) expect(second.guid).to eq(first.guid) - }.to change(DropletModel, :count).by(0) - .and change(Event, :count).by(0) + end.not_to(change { [DropletModel.count, Event.count] }) # Ensure still only one droplet for this build expect(DropletModel.where(build_guid: build.guid).count).to eq(1) @@ -235,11 +234,10 @@ module VCAP::CloudController ) buildpack_lifecycle_data.update(droplet: existing) - expect { + expect do returned = droplet_create.find_or_create_buildpack_droplet(build) expect(returned.guid).to eq(existing.guid) - }.to change(DropletModel, :count).by(0) - .and change(Event, :count).by(0) + end.not_to(change { [DropletModel.count, Event.count] }) expect(DropletModel.where(build_guid: build.guid).first.guid).to eq(existing.guid) end From b2fe6d1110ef8ade16b3c6e2f5ec7dbaeabe2324 Mon Sep 17 00:00:00 2001 From: Katharina Przybill <30441792+kathap@users.noreply.github.com> Date: Thu, 13 Nov 2025 15:13:41 +0100 Subject: [PATCH 3/5] lock on build is not needed and unique constraint violation can not happen --- app/actions/droplet_create.rb | 5 ----- 1 file changed, 5 deletions(-) diff --git a/app/actions/droplet_create.rb b/app/actions/droplet_create.rb index 1521f5adae6..1e8abebcf9f 100644 --- a/app/actions/droplet_create.rb +++ b/app/actions/droplet_create.rb @@ -51,9 +51,6 @@ def create_docker_droplet(build) def find_or_create_buildpack_droplet(build) DropletModel.db.transaction do - BuildModel.where(id: build.id).for_update.first or - raise "Build #{build.id} not found for locking" - existing = DropletModel.where(build_guid: build.guid).first if existing if build.cnb_lifecycle? @@ -79,8 +76,6 @@ def find_or_create_buildpack_droplet(build) record_audit_event(droplet, build.package, user_audit_info_from_build(build)) droplet end - rescue Sequel::UniqueConstraintViolation - DropletModel.where(build_guid: build.guid).first end private From d4fb64e8f6612d623be1c16478133df813479b00 Mon Sep 17 00:00:00 2001 From: Katharina Przybill <30441792+kathap@users.noreply.github.com> Date: Mon, 24 Nov 2025 09:21:53 +0100 Subject: [PATCH 4/5] add log if droplet already exists --- app/actions/droplet_create.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/actions/droplet_create.rb b/app/actions/droplet_create.rb index 1e8abebcf9f..df0d5296d3e 100644 --- a/app/actions/droplet_create.rb +++ b/app/actions/droplet_create.rb @@ -53,6 +53,7 @@ def find_or_create_buildpack_droplet(build) DropletModel.db.transaction do existing = DropletModel.where(build_guid: build.guid).first if existing + Steno.logger('droplet_existing').info("existing droplet found: #{existing.guid}") if build.cnb_lifecycle? existing.cnb_lifecycle_data ||= build.cnb_lifecycle_data else From c96da93c356f493af2192b801c0e4814ff2b44c7 Mon Sep 17 00:00:00 2001 From: Katharina Przybill <30441792+kathap@users.noreply.github.com> Date: Tue, 25 Nov 2025 15:33:45 +0100 Subject: [PATCH 5/5] put droplet reload out of transaction --- app/actions/droplet_create.rb | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/app/actions/droplet_create.rb b/app/actions/droplet_create.rb index df0d5296d3e..603560051ad 100644 --- a/app/actions/droplet_create.rb +++ b/app/actions/droplet_create.rb @@ -50,6 +50,7 @@ def create_docker_droplet(build) end def find_or_create_buildpack_droplet(build) + droplet = nil DropletModel.db.transaction do existing = DropletModel.where(build_guid: build.guid).first if existing @@ -71,12 +72,12 @@ def find_or_create_buildpack_droplet(build) else droplet.buildpack_lifecycle_data = build.buildpack_lifecycle_data end - - droplet.reload - Steno.logger('build_completed').info("droplet created: #{droplet.guid}") - record_audit_event(droplet, build.package, user_audit_info_from_build(build)) - droplet end + + droplet.reload + Steno.logger('build_completed').info("droplet created: #{droplet.guid}") + record_audit_event(droplet, build.package, user_audit_info_from_build(build)) + droplet end private