## Summary Since 18.11, `Geo::UploadReplicator` verification on secondary sites passes whenever the primary site has verified the upload, even if the secondary's file is damaged or missing. The cause is a shared code path that was written for the new upload partition replicators but also runs for the legacy replicator. ## Problem Commit b1c5260d1ba5 (#594291, shipped in 18.11) added a checksum-adoption shortcut to `Geo::Concerns::UploadReplicatorBehavior`. When `parent_upload_verification_succeeded?` is true, `calculate_checksum` returns `model_record.upload_state.verification_checksum` instead of hashing the file. This shortcut was meant for the upload partition replicators (for example `Geo::ProjectUploadReplicator`), where `upload_state` belongs to the parent `Upload` record. `Geo::UploadReplicator` also includes this concern. For the legacy `::Upload` model, `upload_state` is the upload's own verification state, not a parent's. On a secondary site, the main database is a read replica, so that row is the primary site's row. The secondary ends up comparing the primary's checksum with `primary_checksum`, which is the same value. The comparison always matches, and no file is ever read. The partition replicators are behind `geo_*_upload_replication` feature flags, which are off. The legacy replicator, behind `geo_upload_replication` (on), is the one syncing and verifying uploads on every secondary today. So this bug is live on every secondary site running 18.11 or later, not hidden behind a flag. Impact: - Upload verification on secondary sites passes damaged or missing files, both on first verification and on every reverification. - In the worst case, when a new `Geo::UploadRegistry` row finds the file already present (failover re-attach, backup restore, tracking database reset), `ok_to_skip_download?` marks the file synced without downloading it, on the assumption that verification will check it. Verification then copies the checksum instead of hashing the file, so that file gets no check at all. The primary site is not affected. `track_checksum_attempt!` moves the state to `verification_started` before the checksum is calculated, so `upload_state.verification_succeeded?` is false at that moment on the primary, and the adoption never fires there. Issue #628305 says this problem is "currently hidden" because the partition replicators are off on secondaries. That is not correct: the legacy replicator has the same bug today, and it is the one actually running. ## Reproduction steps On a GDK with Geo (primary and secondary site): 1. Create an upload with a file, for example by adding an image to a comment. 2. Let the Upload get verified and synced. You can check this is the Admin UI. 3. Overwrite the file on disk with different bytes. 4. Call `replicator.verify` on the corresponding `Geo::UploadRegistry` row. Results on `master`: - Reverification (registry already has `verified_at`): `verification_succeeded`. With the fix proposed below, verifications now rightly fails. ## Proposal The fix is an override in the legacy replicator, so it never adopts the parent's checksum: ```ruby # An Upload's upload_state is its own state, not a parent's. On a secondary, # copying it would compare the primary's checksum with itself. override :parent_upload_verification_succeeded? def parent_upload_verification_succeeded? false end ``` The override goes in `ee/app/replicators/geo/upload_replicator.rb`. It restores the pre-18.11 behavior on secondaries: verification hashes the local file, and skip-download stays safe because the verification that follows it reads the file. The primary's behavior does not change. The override lives in the legacy class, so it goes away when the legacy code is removed (#628552). Rows that were verified on secondaries between 18.11 and this fix stay marked verified until their next reverification. Admins can close that gap immediately with the "Reverify all" action for uploads on each secondary site, at the cost of rehashing every upload. This is a note for the release note, not a required step for this issue. ## Relationship to #628305 and !256554 !256554 (for #628305) merges first. It adds a first-verification-only guard, which on its own also makes legacy reverifications read the file, and a `skip_download_can_rely_on_verification?` override that checks `Geo::UploadRegistry.verification_succeeded`. For the legacy replicator, that check looks at its own brand-new registry, so it is always false when the primary has already verified the upload. Until this fix lands, the legacy replicator therefore re-downloads instead of skipping. This fix makes `parent_upload_verification_succeeded?` false for the legacy replicator, so its skip-download behaves as it did before !256554. Separately, #628305's Change 2 treats `Geo::UploadRegistry.verification_succeeded` as proof that the secondary's own copy was hashed. That proof only holds for rows verified after this fix lands, so this issue blocks #628305. Both fixes need to land in milestone 19.5. ## Implementation plan - [ ] Add the `parent_upload_verification_succeeded?` override to `Geo::UploadReplicator`. - [ ] In `ee/spec/support/shared_examples/replicators/upload_replicator_behavior_shared_examples.rb`, restrict the adoption examples and the `#skip_download_can_rely_on_verification?` examples to partition replicators. They currently also run for `Geo::UploadReplicator` via `ee/spec/replicators/geo/upload_replicator_spec.rb`. - [ ] Add legacy examples: `calculate_checksum` reads the file even when `upload_state` has succeeded, on the primary and on a secondary. - [ ] Add a regression example: a damaged file on a secondary fails verification, on first verification and on reverification. - [ ] Add `Changelog: fixed` and `EE: true`. - [ ] Backport to every maintained stable branch that includes b1c5260d1ba5 (18.11 and later). The backport spec diff differs from master, because the !256554 examples do not exist on stable branches.