The issue https://gitlab.com/gitlab-org/gitlab/-/issues/323918 reported crashes for `InProductMarketingEmailsWorker`. After an [investigation](https://gitlab.com/gitlab-org/gitlab/-/issues/323918#note_526495553) we found multiple layers of issues that lead to the crash. The crash itself can be solved with an immediate temporary fix, but we need to invest more time for the underlaying issues. ## What were the issues 1. [`OnboardingProgress`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/models/onboarding_progress.rb) should only contain root namespaces. However, it's possible to move root groups to be a sub-group. This is not yet the problem why the workers are crashing, but requires a fix as well. 1. Because we have sub-groups in the `onboarding_progresses` table in production, we called [`self_and_ancestors`](https://gitlab.com/gitlab-org/gitlab/blob/master/app/models/namespaces/traversal/recursive.rb#L39) to resolve the root group of the given sub-group. However, this query compilation failed. <details> <summary>Explanation</summary> The query that gets produced when `onboarding_progresses` is joined for groups and we call the ancestor, looks like the following: ```sql WITH RECURSIVE "base_and_ancestors" AS ((SELECT "namespaces".* FROM "namespaces" INNER JOIN "onboarding_progresses" ON "onboarding_progresses"."namespace_id" = "namespaces"."id" WHERE "namespaces"."type" = 'Group' AND "namespaces"."id" = 118) UNION (SELECT "namespaces".* FROM "namespaces", "base_and_ancestors" INNER JOIN "onboarding_progresses" ON "onboarding_progresses"."namespace_id" = "namespaces"."id" WHERE "namespaces"."type" = 'Group' AND "namespaces"."id" = "base_and_ancestors"."parent_id")) SELECT "namespaces".* FROM "base_and_ancestors" AS "namespaces" INNER JOIN "onboarding_progresses" ON "onboarding_progresses"."namespace_id" = "namespaces"."id" ``` And gives us the following error: ``` ActiveRecord::StatementInvalid: PG::UndefinedTable: ERROR: invalid reference to FROM-clause entry for table "namespaces" LINE 3: ...sses" ON "onboarding_progresses"."namespace_id" = "namespace... ^ HINT: There is an entry for table "namespaces", but it cannot be referenced from this part of the query. ``` The reason is the indirect JOIN that we do: `SELECT "namespaces".* FROM "namespaces", "base_and_ancestors"`. If we'd change this to a direct `JOIN`, it would work: ```sql WITH RECURSIVE "base_and_ancestors" AS ( (SELECT "namespaces".* FROM "namespaces" INNER JOIN "onboarding_progresses" ON "onboarding_progresses"."namespace_id" = "namespaces"."id" WHERE "namespaces"."type" = 'Group' AND "namespaces"."id" = 118) UNION (SELECT "namespaces".* FROM "namespaces" JOIN "base_and_ancestors" ON "base_and_ancestors"."parent_id" = "namespaces"."id" INNER JOIN "onboarding_progresses" ON "onboarding_progresses"."namespace_id" = "namespaces"."id" WHERE "namespaces"."type" = 'Group') ) SELECT "namespaces".* FROM "base_and_ancestors" AS "namespaces" INNER JOIN "onboarding_progresses" ON "onboarding_progresses"."namespace_id" = "namespaces"."id"; ``` For this we'd need to change [`Gitlab::ObjectHierarchy#base_and_descendants_cte`](https://gitlab.com/gitlab-org/gitlab/blob/master/lib/gitlab/object_hierarchy.rb#L170) to use a `JOIN` instead. However, this is more work and could have impact to other code as well. </details> 1. The query issue was brought to the light by [`.each_batch`](https://gitlab.com/gitlab-org/gitlab/blob/master/app/models/concerns/each_batch.rb#L1) which is GitLab's implementation of `in_batches`. It doesn't work well together with the `ancestor` call since it keeps `INNER JOIN ON onboarding_progresses` which results into the conflict. <details> <summary>Explanation</summary> This is a minimal test case that will fail with `.each_batch` but doesn't fail with `.in_batches`: ```ruby it 'fails using each_batch with a JOIN' do root_group = create(:group) nested_group = create(:group) onboarding_progress = create(:onboarding_progress, namespace: nested_group) # Modify group to not be a root group anymore, so we can call root_ancestor nested_group.parent = root_group nested_group.save Group.joins(:onboarding_progress).each_batch do |batch| batch.each do |group| group.root_ancestor end end end ``` The `root_ancestor` is different when using `in_batches`. Here are the two different SQL queries: **each_batch** ```sql WITH RECURSIVE "base_and_ancestors" AS ( (SELECT "namespaces".* FROM "namespaces" INNER JOIN "onboarding_progresses" ON "onboarding_progresses"."namespace_id" = "namespaces"."id" WHERE "namespaces"."type" = 'Group' AND "namespaces"."id" = 118) UNION (SELECT "namespaces".* FROM "namespaces" JOIN "base_and_ancestors" ON "base_and_ancestors"."parent_id" = "namespaces"."id" INNER JOIN "onboarding_progresses" ON "onboarding_progresses"."namespace_id" = "namespaces"."id" WHERE "namespaces"."type" = 'Group') ) SELECT "namespaces".* FROM "base_and_ancestors" AS "namespaces" INNER JOIN "onboarding_progresses" ON "onboarding_progresses"."namespace_id" = "namespaces"."id"; ``` **in_batches** ```sql WITH RECURSIVE base_and_ancestors AS ( (SELECT namespaces.* FROM namespaces WHERE namespaces.type = 'Group' AND namespaces.id = 118) UNION (SELECT namespaces.* FROM namespaces, base_and_ancestors WHERE namespaces.type = 'Group' AND namespaces.id = base_and_ancestors.parent_id ) ) SELECT namespaces.* FROM base_and_ancestors AS namespaces" ``` As you can see, it is resolving the JOIN on `onboarding_progresses`, since it queries each group individually. It's maybe not a particular issue with `each_batch`, but in combination with `root_ancestor` and a JOIN it can lead to issues. </details> ## Next steps 1. Immediate Action: Prevent calling `user.can?(:create_projects, group)` when it is not a root group, so we don't resolve to the ancestor. This will fix the crashing. 1. Mid-term action: Remove groups from `onboarding_progresses` when they become a sub-group 1. Mid-term action: Remove all existing sub-groups from `onboarding_progresses` 1. Mid-term action: Fix race condition for `experiment_subjects` (we might not need to fix it at all, when we remove the experiment recording though) 1. Long-term action: Fix the issue with calling `root_ancestor` in combination with `.each_batch`