Commit fa4e7efb authored by Stan Hu's avatar Stan Hu

Avoid idling in transaction while saving project export object

When large exports were generated, we saw in production that these files
often were uploaded to object storage but were missing the relevant
`import_export_uploads` database entry. This was happening because the
idle in transaction timeout killed the database transaction.

CarrierWave v1, which we run now, will idle in transaction while saving
a file to object storage. CarrierWave v1 saves the file in the
`after_save` callback. From the database perspective, this begins a
transaction, inserts an entry into `import_export_uploads`, and then
attempts to upload the file to object storage. However, this quickly
runs into our 60-second idle-in-transaction timeout.

CarrierWave v2 tried to fix this in
https://github.com/carrierwaveuploader/carrierwave/pull/2209 by moving
the object storage save in the `after_commit` callback. This means the
object would only be uploaded after the database COMMIT, but this
presents other problems. For example, the file may appear to be
available even though it hasn't finished uploading to object storage.

CarrierWave v3, which has not yet released, will revert back to the v1
behavior via
https://github.com/carrierwaveuploader/carrierwave/pull/2546 with a
twist: if the transaction is rolled back for reason, CarrierWave will
delete the file in object storage.

For project exports, the v2 issues aren't a big deal since we don't need
to read the value after save, but this does mean the actual file may not
appear until some time after the database entry has been committed. This
means that the UI might display that a project export is available even
though it hasn't finished uploading, resulting in a 404. To avoid that,
we might want to call `file.exist?`, but this would require an HTTP HEAD
request.

Relates to:

* https://gitlab.com/gitlab-com/gl-infra/production/-/issues/4814
* https://gitlab.com/gitlab-org/gitlab/-/issues/195981

Changelog: fixed
parent bcbf3f72
......@@ -11,6 +11,12 @@ class ImportExportUpload < ApplicationRecord
mount_uploader :import_file, ImportExportUploader
mount_uploader :export_file, ImportExportUploader
# This causes CarrierWave v1 and v3 (but not v2) to upload the file to
# object storage *after* the database entry has been committed to the
# database. This avoids idling in a transaction.
skip_callback :save, :after, :store_export_file!
set_callback :commit, :after, :store_export_file!
scope :updated_before, ->(date) { where('updated_at < ?', date) }
scope :with_export_file, -> { where.not(export_file: nil) }
......
......@@ -43,4 +43,23 @@ RSpec.describe ImportExportUpload do
end
end
end
context 'ActiveRecord callbacks' do
let(:after_save_callbacks) { described_class._save_callbacks.select { |cb| cb.kind == :after } }
let(:after_commit_callbacks) { described_class._commit_callbacks.select { |cb| cb.kind == :after } }
def find_callback(callbacks, key)
callbacks.find { |cb| cb.instance_variable_get(:@key) == key }
end
it 'export file is stored in after_commit callback' do
expect(find_callback(after_commit_callbacks, :store_export_file!)).to be_present
expect(find_callback(after_save_callbacks, :store_export_file!)).to be_nil
end
it 'import file is stored in after_save callback' do
expect(find_callback(after_save_callbacks, :store_import_file!)).to be_present
expect(find_callback(after_commit_callbacks, :store_import_file!)).to be_nil
end
end
end
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