Skip to content

Commit 83c3587

Browse files
committed
Fix hanging staging on cc-uploader kill
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)
1 parent 3e7bba2 commit 83c3587

File tree

3 files changed

+71
-14
lines changed

3 files changed

+71
-14
lines changed

app/actions/droplet_create.rb

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -49,22 +49,30 @@ def create_docker_droplet(build)
4949
droplet
5050
end
5151

52-
def create_buildpack_droplet(build)
53-
droplet = droplet_from_build(build)
54-
52+
def find_or_create_buildpack_droplet(build)
5553
DropletModel.db.transaction do
54+
BuildModel.where(id: build.id).for_update.first or
55+
raise "Build #{build.id} not found for locking"
56+
57+
existing = DropletModel.where(build_guid: build.guid).first
58+
return existing if existing
59+
60+
droplet = droplet_from_build(build)
5661
droplet.save
62+
5763
if build.cnb_lifecycle?
5864
droplet.cnb_lifecycle_data = build.cnb_lifecycle_data
5965
else
6066
droplet.buildpack_lifecycle_data = build.buildpack_lifecycle_data
6167
end
62-
end
6368

64-
droplet.reload
65-
Steno.logger('build_completed').info("droplet created: #{droplet.guid}")
66-
record_audit_event(droplet, build.package, user_audit_info_from_build(build))
67-
droplet
69+
droplet.reload
70+
Steno.logger('build_completed').info("droplet created: #{droplet.guid}")
71+
record_audit_event(droplet, build.package, user_audit_info_from_build(build))
72+
droplet
73+
end
74+
rescue Sequel::UniqueConstraintViolation
75+
DropletModel.where(build_guid: build.guid).first
6876
end
6977

7078
private

app/controllers/runtime/stagings_controller.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,7 @@ def droplet_from_build(build, guid)
134134
end
135135

136136
def create_droplet_from_build(build)
137-
DropletCreate.new.create_buildpack_droplet(build)
137+
DropletCreate.new.find_or_create_buildpack_droplet(build)
138138
end
139139

140140
def upload_path

spec/unit/actions/droplet_create_spec.rb

Lines changed: 54 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -158,13 +158,13 @@ module VCAP::CloudController
158158
end
159159
end
160160

161-
describe '#create_buildpack_droplet' do
161+
describe '#find_or_create_buildpack_droplet' do
162162
context 'buildpack lifecycle' do
163163
let!(:buildpack_lifecycle_data) { BuildpackLifecycleDataModel.make(build:) }
164164

165165
it 'sets it on the droplet' do
166166
expect do
167-
droplet_create.create_buildpack_droplet(build)
167+
droplet_create.find_or_create_buildpack_droplet(build)
168168
end.to change { [DropletModel.count, Event.count] }.by([1, 1])
169169

170170
droplet = DropletModel.last
@@ -195,6 +195,55 @@ module VCAP::CloudController
195195
})
196196
end
197197

198+
it 'creates the droplet once and attaches lifecycle data' do
199+
expect {
200+
droplet_create.find_or_create_buildpack_droplet(build)
201+
}.to change { [DropletModel.count, Event.count] }.by([1, 1])
202+
203+
droplet = DropletModel.last
204+
expect(droplet.state).to eq(DropletModel::STAGING_STATE)
205+
expect(droplet.app).to eq(app)
206+
expect(droplet.package).to eq(package)
207+
expect(droplet.build).to eq(build)
208+
209+
buildpack_lifecycle_data.reload
210+
expect(buildpack_lifecycle_data.droplet).to eq(droplet)
211+
end
212+
213+
it 'is idempotent on repeated calls (no duplicate droplet, same GUID, no extra event)' do
214+
first = droplet_create.find_or_create_buildpack_droplet(build)
215+
216+
expect {
217+
second = droplet_create.find_or_create_buildpack_droplet(build)
218+
expect(second.guid).to eq(first.guid)
219+
}.to change(DropletModel, :count).by(0)
220+
.and change(Event, :count).by(0)
221+
222+
# Ensure still only one droplet for this build
223+
expect(DropletModel.where(build_guid: build.guid).count).to eq(1)
224+
225+
buildpack_lifecycle_data.reload
226+
expect(buildpack_lifecycle_data.droplet).to eq(first)
227+
end
228+
229+
it 'returns the pre-existing droplet when one already exists for the build' do
230+
existing = DropletModel.make(
231+
app: app,
232+
package: package,
233+
build: build,
234+
state: DropletModel::STAGING_STATE
235+
)
236+
buildpack_lifecycle_data.update(droplet: existing)
237+
238+
expect {
239+
returned = droplet_create.find_or_create_buildpack_droplet(build)
240+
expect(returned.guid).to eq(existing.guid)
241+
}.to change(DropletModel, :count).by(0)
242+
.and change(Event, :count).by(0)
243+
244+
expect(DropletModel.where(build_guid: build.guid).first.guid).to eq(existing.guid)
245+
end
246+
198247
context 'when the build does not contain created_by fields' do
199248
let(:build) do
200249
BuildModel.make(
@@ -205,7 +254,7 @@ module VCAP::CloudController
205254

206255
it 'sets the actor to UNKNOWN' do
207256
expect do
208-
droplet_create.create_buildpack_droplet(build)
257+
droplet_create.find_or_create_buildpack_droplet(build)
209258
end.to change { [DropletModel.count, Event.count] }.by([1, 1])
210259

211260
droplet = DropletModel.last
@@ -230,7 +279,7 @@ module VCAP::CloudController
230279

231280
it 'sets it on the droplet' do
232281
expect do
233-
droplet_create.create_buildpack_droplet(build)
282+
droplet_create.find_or_create_buildpack_droplet(build)
234283
end.to change { [DropletModel.count, Event.count] }.by([1, 1])
235284

236285
droplet = DropletModel.last
@@ -271,7 +320,7 @@ module VCAP::CloudController
271320

272321
it 'sets the actor to UNKNOWN' do
273322
expect do
274-
droplet_create.create_buildpack_droplet(build)
323+
droplet_create.find_or_create_buildpack_droplet(build)
275324
end.to change { [DropletModel.count, Event.count] }.by([1, 1])
276325

277326
droplet = DropletModel.last

0 commit comments

Comments
 (0)