Skip to content

Fix cross database transaction in Projects::OverwriteProjectService#execute

Related to #340256 (closed).

Problem

There is at least one case where we do Project.transaction { Projects::DestroyService(...).execute }. This is done in Projects::OverwriteProjectService#execute which does Project.transaction and then #destroy_old_project and then mentioned service. This service appears to be used by import-export feature. So, maybe they are the best team to solve this. This might also indicate that maybe we should keep this closed and create a new one. Do it at your discretion :)

We need to somehow fix Projects::OverwriteProjectService#execute to do it differently. To explicitly indicate this problem and place that needs to be considered the MR was created: !78079 (merged). By removing allow_cross_database_modification_within_transaction the buggy behavior will be triggered.

For now, I re-open this issue since this the most comprehensive description of the solution.

Note: This was discovered now since the ./spec/services/projects/overwrite_project_service_spec.rb was part of spec/support/database/cross-database-modification-allowlist.yml and was not removed once the original issue was merged.

Analysis

Based on #340256 (comment 820203511)

@lmcandrew This is a tricky topic that is likely to involve product. First of all, project import with overwrite basically replaces existing project with a completely new one, while preserving the same name & path (and a few other things like user stars). One might achieve the same outcome by manually deleting old project and importing a new one under the same name. What is the expected behaviour of such failed import?

I am guessing the main concern for using outermost Project.transaction during 'overwriting' process is to prevent potential data loss of the project that is being overridden, if something goes wrong. This service was introduced 4 years ago and didn't change much since. Does Projects::DestroyService guarantee against data loss if it fails to execute? I see there is Projects::DestroyService#attempt_rollback but it sounds like rollback is not guaranteed? Can there be a situation, where user initiated project import with overwrite fails because of destroy service failing, resulting in original project not being in it's original condition? Is this an acceptable potential outcome of a failed import attempt?

If destroy rollback preserves data then I think it should be possible to update it and not utilize the outer most Project.transaction, which should solve the issue of cross-DB transaction. If it doesn't - is partial data loss of the old project acceptable? Since project overwrite deletes the old project, does it matter which state the project is in if project destroy fails?

Currently the 'overwriting' process is:

  1. Update a set of relations' project_id from old to new (stars, members, group links, etc)
  2. Destroy old project
  3. Rename new project to old project's name/path

I am proposing we update this process to start copying mentioned relation instead of updating/moving existing ones:

  • If something goes wrong during the copying process, the old project is still intact and we do not need to update/move it's relations back. This does mean updating all of the services listed in Projects::OverwriteProjectService#move_before_destroy_relationships
  • If something goes wrong during old project removal, old project is also intact, we just need to clean up/remove new project

side note: I don't fully understand right now by just looking at the code what happens with new project when old one is failed to be deleted/renamed (it might be removed elsewhere). We attempt to restore the repository, but what about all other data? Can data loss potentially happen already?

Additionally, we log any exceptions of this service but I haven't seen any occurrences of this in the past 7 days on production (that's how far our logs go).

Solution

In terms of the solution, I think we should:

  1. Update all the relation 'move' services to perform a copy instead
  2. Update Projects::OverwriteProjectService#destroy_old_project to not be in Project.transaction
  3. If something goes wrong during old project destroy - remove previously copied relations, to match existing behaviour
  4. If something goes wrong during new project rename - remove previously copied relations, to match existing behaviour

5️⃣ for the estimation.