## Summary `Geo::Concerns::UploadReplicatorBehavior#calculate_checksum` in `ee/app/replicators/geo/concerns/upload_replicator_behavior.rb` returns `model_record.upload_state.verification_checksum` when `parent_upload_verification_succeeded?` is true. Issue #594341 added this so the primary site does not re-read files the legacy replicator already checksummed. The method has no primary-site guard. A secondary site therefore adopts the primary site's checksum and never reads its own file. ## Problem The steps below follow one upload on a secondary site at Event B (milestone 19.6, when the legacy flag is turned off). 1. The registry consistency worker creates a new pending registry row. 2. `Gitlab::Geo::Replication::BlobDownloader` calls `ok_to_skip_download?`. The file exists on the secondary site, so the row becomes synced with no download. A code comment says this is safe because verification follows. 3. `verify` calls `calculate_checksum`. It adopts the primary site's checksum and does not open the file. The comparison with `primary_checksum` always matches. No actual verification happened. For most rows this is harmless, because the legacy replicator already hashed the secondary site's copy through `Geo::UploadRegistry`. But rows whose legacy verification failed, mismatched, or never ran become verified with no read and no resync. Step 3 repeats at a default of 90-day reverification interval, because `upload_states` stays succeeded after Event B. This means that a file deleted or damaged on a secondary site after Event B stays verified. The primary site also never hashes pre-cutover uploads again. Today, partition replicators are off on secondary sites while `geo_upload_replication` (the legacy replicator flag) is on, so the problem is currently hidden. ## Previous design The first version of this issue added a `::Gitlab::Geo.primary?` guard to `parent_upload_verification_succeeded?`. !256554 implements this guard. With it, every secondary site hashes every file. This works but is costly. Each secondary site will re-hash its full upload table at Event B: at the default `verification_max_capacity` of 10, this will take about one hour per 200,000 uploads. The largest Geo sites hold tens of millions of uploads, so it might take weeks and block verification of other replicables in the meantime. This edit proposes a cheaper design below. ## Proposal This proposal makes two changes in the same concern. **Change 1**: adopt the legacy's checksum only on the first verification. The tracker is the partition state row on the primary site and the registry row on the secondary site. The state machine sets `verified_at` on success and on failure, and never clears it. Reverification only sets the state back to pending. ```ruby def parent_upload_verification_succeeded? return false unless immutable? && model_record.upload_state # Adopt only on the first verification on this site. Reverification must # read the file, or damage after the first checksum is never found. return false unless verification_state_tracker.verified_at.nil? model_record.upload_state.verification_succeeded? end ``` Event A (milestone 19.5) and Event B still do not checksum by default, but each later reverification will checksum the file. The primary site's `Geo::ReverificationBatchWorker` sets the pace, one batch at a time, so the added work spreads over the 90-day cycle on both sites. **Change 2**: skip the download if the legacy primary checksum is not succeeded, or if the legacy secondary verification already succeeded for this upload. ```ruby # We skip the download when: # - the legacy primary checksum is not succeeded; OR # - the legacy registry already verified this secondary's copy. def skip_download_can_rely_on_verification? return false unless super return true unless parent_upload_verification_succeeded? ::Geo::UploadRegistry.verification_succeeded.exists?(file_id: model_record.id) end ``` No site-check is needed because `BlobDownloader` runs only on secondary sites. At Event B, rows the legacy replicator verified skip the download. The replicator downloads the other rows, and `BlobDownloader#checksum_mismatch?` checks the bytes. Both changes are needed together. Change 1 alone trusts unverified files until the next cycle. Change 2 alone never checksums again before #628552 removes the code. ## Trade-off Event B downloads only the uploads the legacy replicator did not verify, so if it never marked any upload as verified, they'll all get download when the switch happen. However this is very unlikely that a customer find themselves in such a situation. ## Implementation plan - [ ] Add the verified_at condition to `parent_upload_verification_succeeded?`. - [ ] Add the private `skip_download_can_rely_on_verification?` override. - [ ] Update the module comment of the concern. - [ ] Add shared example contexts in `ee/spec/support/shared_examples/replicators/upload_replicator_behavior_shared_examples.rb` for `verified_at nil` and set, on the primary site and the secondary site. - [ ] Add shared example contexts for skip-download cases: base false, parent not succeeded, legacy succeeded, legacy failed or absent. - [ ] Run `ee/spec/replicators/geo/project_upload_replicator_spec.rb`. - [ ] Run all `ee/spec/replicators/geo/*_upload_replicator_spec.rb` files. - [ ] Add Changelog: fixed and EE: true.