diff --git a/app/actions/buildpack_update.rb b/app/actions/buildpack_update.rb index e668ecd712b..4529b9d00c6 100644 --- a/app/actions/buildpack_update.rb +++ b/app/actions/buildpack_update.rb @@ -13,6 +13,10 @@ def update(buildpack, message) Buildpack.db.transaction do Locking[name: 'buildpacks'].lock! + # The buildpack was loaded before this lock, so its in-memory position may be stale. + # Re-read under the lock so move_to's shift range is correct; removing this reintroduces + # duplicate/missing positions. + buildpack.refresh MetadataUpdate.update(buildpack, message) buildpack.move_to(message.position) if message.requested?(:position) diff --git a/spec/unit/actions/buildpack_update_spec.rb b/spec/unit/actions/buildpack_update_spec.rb index b804a7ae713..40f936cb3ad 100644 --- a/spec/unit/actions/buildpack_update_spec.rb +++ b/spec/unit/actions/buildpack_update_spec.rb @@ -39,6 +39,26 @@ module VCAP::CloudController expect(buildpack1.reload.position).to eq(1) end + + context 'when the in-memory position is stale because of a concurrent reorder' do + let!(:buildpack4) { create(:buildpack, position: 4) } + + it 'recomputes the shift from the committed state and keeps positions contiguous' do + # buildpack2 was loaded (position 2) before the lock. Simulate a concurrent request + # that reorders the list in the meantime: buildpack2 drifts from 2 -> 3 in the DB, + # but the in-memory object still thinks it is at 2. + buildpack4.move_to(1) + expect(buildpack2.position).to eq(2) # stale in-memory value + expect(Buildpack.first(guid: buildpack2.guid).position).to eq(3) # committed DB truth + + message = BuildpackUpdateMessage.new(position: 1) + BuildpackUpdate.new(user_audit_info).update(buildpack2, message) + + positions = Buildpack.order(:position).all.map(&:position) + expect(positions).to eq([1, 2, 3, 4]), "expected contiguous positions, got #{positions.inspect}" + expect(buildpack2.reload.position).to eq(1) + end + end end context 'when enabled is changed' do