We have several examples of cross-database modifications (eg. executing queries against 2 different databases within the context of a transaction) that are caused by the use of `dependent: :destroy`. One example is: ```rb class Project has_many :builds, class_name: 'Ci::Build', inverse_of: :project, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent end ``` The usual use case for such things is cleaning up data that lives outside of Postgres. One common example being files that live in object storage that relate to builds/artifacts and so on. When you execute `project.destroy` it does: 1. Start a transaction 2. Loop through all builds calling `destroy` on each 3. Destroy the project 4. Commit the transaction This code will all work still when we have builds on a separate DB but we will lose the transactional guarantee that any failure during steps 2/3 will mean the entire transaction is rolled back and nothing is deleted. When we move to 2 separate databases we are just going to have to live with possible data inconsistencies like this. ## Possible solutions 1. We are implementing an alternative to Postgres's cascading deletes in https://gitlab.com/gitlab-org/gitlab/-/issues/338535 and there has been some discussion about extending this mechanism to support the `destroy` case as well (ie. we load the models in our cleanup worker and call `#destroy` on them allowing them to do whatever cleanup they need) but [it was mentioned that this may not work if the parent object no longer exists as data from the parent object may be necessary for the cleanup](https://gitlab.com/gitlab-org/gitlab/-/issues/338535#note_660636049). We should investigate to see if there is any way to handle these cases. We may still want to implement this as an option for cases where it will work and then fan out more complicated cases to the relevant team to come up with a specific solution to their case 2. Schedule the cleanup async in sidekiq. This would give us better guarantee that the work will complete eventually (due to sidekiq retries) than doing it sync in the single request context as short timeouts could cause this to lead to partial cleanup regularly 3. Schedule the cleanup after the first transaction completes ## TODO 1. [x] List all examples that violate cross-db modification 2. [x] For each example describe high level what the purpose of `dependent: :destroy` is 3. [x] Can examples be made async? Do they need a parent object present and if so why? 4. [x] Can the deletes remain sync but be done after the transaction keeping parent objects in memory? Worst case scenario is partial cleanup? Is this bad? Loose FK constraints will enforce DB cleanup later but there may remain partial cleanup of object storage? Can we even guarantee perfect object storage cleanup today? 5. [x] Also look into `FastDestroyAll` and usages and see if it has the same problem 6. [x] What code paths are calling `FastDestroyAll.fast_destroy_all` directly and do they actually cross-db boundaries in a transaction? ## Examples 1. `app/models/project.rb: has_many :builds, class_name: 'Ci::Build', inverse_of: :project, dependent: :destroy` 1. Fanned out to ~"group::pipeline authoring" https://gitlab.com/gitlab-org/gitlab/-/issues/340256 1. ~~`app/models/project.rb: has_one :ci_cd_settings, class_name: 'ProjectCiCdSetting', inverse_of: :project, autosave: true, dependent: :destroy`~~ 1. ~~Seems to be have been added in https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/18745/diffs as some kind of workaround for import/export issues which weirdly related to `project.update(ci_cd_settings: nil)`. This appears to not actually be critically necessary for it to be a `destroy` and operation as there is no destroying needed. I suspect we'll be able to implement a different workaround here.~~ 1. This is actually redundant. Since the table is called `project_ci_cd_settings` (and not `ci_cd_settings` like I thought) then it won't actually be moving to the new CI database. I did actually create an MR to remove this https://gitlab.com/gitlab-org/gitlab/-/merge_requests/69947 but realised it's not necessary. The code is still redundant and we could remove it but it's not a blocker so I'll leave this alone for now. 1. `app/models/user.rb: has_many :triggers, dependent: :destroy, class_name: 'Ci::Trigger', foreign_key: :owner_id` 1. I was able to trace this back to this comment https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/9713#note_24835526 but the MR gives no indication about why the cascading delete is not sufficient. 1. MR to fix it https://gitlab.com/gitlab-org/gitlab/-/merge_requests/69721 1. `User.has_many :builds, dependent: :nullify, class_name: 'Ci::Build'` and `User.has_many :pipelines, dependent: :nullify, class_name: 'Ci::Pipeline'`\ 1. Extracted to https://gitlab.com/gitlab-org/gitlab/-/issues/340260 ## `FastDestroyAll` examples 1. [`app/models/project.rb: use_fast_destroy :build_trace_chunks`](https://gitlab.com/gitlab-org/gitlab/-/blob/5a72461ae48319e513aa1ed2d91efe9a206bab97/app/models/project.rb#L121) 1. If using the [`fast_destroy_all`](https://gitlab.com/gitlab-org/gitlab/-/blob/6ead7b93907a705b3ffc3f2f5aa32e6a47dcf6a2/app/models/concerns/fast_destroy_all.rb#L46) method then this could theoretically have the same problem since it calls `delete_all`. I've yet to determine the code path that uses it but it does not seem to be the same path of deleting a `Project` 1. For the `Project.use_fast_destroy :build_trace_chunks` case it seems it does not actually have this transaction problem as it doesn't ever actually call `delete` or `delete_all` on the `ci_build_trace_chunks`. Checking the output from `Project#destroy` I can't see any deletes happening on `ci_build_trace_chunks` and so I think it seems to be relying on Postgres cascading deletes. The code path is: 1. [`use_fast_destroy`](https://gitlab.com/gitlab-org/gitlab/-/blob/6ead7b93907a705b3ffc3f2f5aa32e6a47dcf6a2/app/models/project.rb#L121) calls => 1. [`set_callback :destroy, :before`](https://gitlab.com/gitlab-org/gitlab/-/blob/6ead7b93907a705b3ffc3f2f5aa32e6a47dcf6a2/app/models/concerns/fast_destroy_all.rb#L80)