Reindex group level work items when their author is banned

John Masonrequested to merge
jmason-w2119-reindex-banned-author into master

🤖 AI-authored change.

What does this MR do and why?

Group level non-epic work items now reindex when their author is banned or unbanned, so a banned author's group level work items stop being returned by Advanced Search. Project level behaviour is unchanged, and epic work items are still excluded.

Issue.searchable, which the ban fan-out scopes the author's issues with, was where.not(project_id: nil) — a proxy for "not a group level epic" that also dropped every other group level work item. It now excludes the epic work item type directly.

Reviewer focus: the marginal cost of the widened predicate, and whether excluding epics here is still right now that epics are indexed in work_items rather than in their own index.

References

Closes #628642

Query plans — Database Lab gitlab-production-main, author_id 1614863:

The first two are the comparison that matters, and they differ by 14 buffers: widening the scope costs approximately nothing.

Mechanism, plan analysis and verification

Mechanism

Postgres hides a banned author's work items immediately through the Issue.without_hidden anti-join, but Advanced Search reads hidden from the index, where it is written at index time. EE::Users::BannedUser is what keeps those documents current, which is why the scope it walks decides what gets hidden in search.

The project_id exclusion was added in !159144 (merged) to keep epic work items out of the fan-out, and group level epics are the rows with no project_id.

Epics no longer have their own index — the standalone epics index was removed in 18.6 by !209992 (merged), and EE::Epic#elastic_reference now returns a Search::Elastic::References::WorkItem, the same reference class and the same work_items document this scope's consumer writes. So the reason to exclude them here is no longer a separate index: it is that Epic tracks that document itself, and including the row would double-track it. The comment on the scope says that rather than naming the deleted index.

That raised a separate question, which this MR does not widen to cover: the ban fan-out enqueues %i[issues merge_requests notes], and User#epics is not in that list, so banning an author reindexes no epics at all even though Search::Elastic::Filters.by_not_hidden is registered for them in WorkItemQueryBuilder. That is the same defect class as #628642 against a much larger population. It is tracked in its own issue — a separate association, so widening the scope here would not have fixed it.

The scope's only consumer is Elastic::ProcessBookkeepingService#each_indexed_association, so the blast radius is that one call path. The fan-out itself is untouched: still one ElasticAssociationIndexerWorker job per ban, walking the association with find_in_batches and tracking each batch through Elastic::ProcessBookkeepingService, whose shard limit is 1,000. The job count does not change; what changes is that the walk now yields the author's group level non-epic work items too, so each ban tracks that many more references and writes them to the Redis sorted set.

Plan analysis

SELECT "issues".* FROM "issues"
WHERE "issues"."author_id" = 1614863
  AND ("issues"."project_id" IS NOT NULL OR "issues"."work_item_type_id" != 8)
ORDER BY "issues"."id" ASC LIMIT 1000

The master plan and this MR's plan are the same Parallel Index Scan on index_issues_on_author_id over the same 104,655 rows. Their wall-clock times differ 100x, which is clone cache state rather than the predicate, so buffers are the comparable figure. The withdrawn candidate's subplan was uncorrelated, reading every group level epic row whatever the author it was chained onto.

Verification

Exercised in a local GDK against a live Elasticsearch. Before: banning an author flipped the project level document to hidden: true and left the group level one at hidden: false. After: both flip on ban and clear on unban.

With the old scope restored, four new examples fail across ee/spec/models/issue_spec.rb, ee/spec/services/elastic/process_bookkeeping_service_spec.rb and ee/spec/elastic_integration/work_item_banned_author_spec.rb, and nothing else does. With the change in place all four pass.

What I did not verify

The plans above are Database Lab clones of production, not production itself, and were captured for one author. The full ee/spec/models/issue_spec.rb file was not run green end to end locally, only its .searchable examples plus the two other spec files; CI covers the rest. Backfilling documents for users banned before this ships is out of scope and tracked separately. The epic gap named under Mechanism has its own issue, which is confidential, so it is not linked here.

How to set up and validate locally

bundle exec rspec ee/spec/models/issue_spec.rb -e '.searchable'
bundle exec rspec ee/spec/services/elastic/process_bookkeeping_service_spec.rb
ELASTIC_URL=http://localhost:9200 bundle exec rspec \
  ee/spec/elastic_integration/work_item_banned_author_spec.rb

The integration spec needs a running Elasticsearch; the other two do not.

🤖 Automated change — mention me to get a response. Feedback welcome. Ask to speak to John directly anytime.

Edited by John Mason

Merge request reports