Draft: Give the reactive caching CI workers a feature category

Moves ReactiveCachingWorker off feature_category :not_owned - four of its five responsbilities do CI/CD work (MR widget report comparisons, CI config variables, pipeline inputs, security report generation; EE::Group's seat count is the outlier), so it now reports :continuous_integration and is listed under [Verify] in CODEOWNERS. ExternalServiceReactiveCachingWorker keeps :not_owned because its consumers genuinely span several groups. Metadata and ownership only, no behaviour change.

Detailed context for AI agents

Root cause / motivation

ReactiveCacheableWorker declares loggable_arguments 0, and its context_for_arguments builds the Sidekiq application context from argument 0 only:

class_name, *_other_args = arguments
Gitlab::ApplicationContext.new(related_class: class_name.to_s)

For an MR widget report comparison, argument 0 is MergeRequest, so the logged related_class is MergeRequest even though the CPU-bound work is done by a Ci::CompareReportsBaseService subclass passed in argument 2 (for example Ci::CompareTestReportsService). MergeRequest#calculate_reactive_cache raises NameError unless it is handed such a subclass, confirming the MR is only the cache holder, never the workload. Combined with :not_owned on the shared concern, these jobs were attributed to Code Review rather than to the group that owns the computation.

The five consumers of ReactiveCachingWorker (work type :no_dependency)

MergeRequest report comparisons, Ci::ListConfigVariablesService, Ci::PipelineCreation::FindPipelineInputsService, Security::MergeRequestSecurityReportGenerationService, EE::Group#billable_members_count. The group seat count does not fit CI/CD but does not justify its own category either, so it is noted in a code comment rather than given special handling.

Why ExternalServiceReactiveCachingWorker keeps :not_owned

Its consumers are Environment, Clusters::Cluster, Clusters::Platforms::Kubernetes, SshHostKey, PrometheusAdapter, Integrations::ReactivelyCached, ErrorTracking::ProjectErrorTrackingSetting, Observability::ObservabilityPresenter, the Jira ByIdsFinder and the security training URL services. Those span several groups, so for that worker the owner really does depend on the class being cached, and the existing rubocop:todo Gitlab/AvoidFeatureCategoryNotOwned moves here from the shared concern.

Alternatives considered and rejected

  • Give the whole reactive caching framework to Verify (the concern, all three workers, app/models/concerns/reactive_caching.rb). Rejected: it would make Verify the owner of environments, Prometheus, error tracking and Jira reactive caching.
  • Derive the category dynamically in context_for_arguments from the compare service in argument 2. Better long-term fix, and it would also fix the misleading related_class in the logs, but it needs a service-to-category mapping table. Left as a follow-up.

Motivation from local profiling

Measured on a GDK with seeded data, so the absolute numbers are indicative only: an MR report comparison job across two pipelines carrying 50,000 junit test cases each took about 3.1 seconds, and a Ci::ListConfigVariablesService job on a 90 KB CI config with 15 includes and 210 jobs took about 1.1 seconds. Both are CPU-bound work on a worker declared urgency :high, whose documented execution latency requirement is 10 seconds. Whoever owns the category is the group that sees these in their Sidekiq dashboards and error budget.

Verification performed

  • gitlab:sidekiq:all_queues_yml:generate then :check passes. The regenerated file confirms ReactiveCaching::LowUrgencyWorker inherits the category from its superclass and that ExternalServiceReactiveCachingWorker is unchanged.
  • spec/workers/every_sidekiq_worker_spec.rb - 13 examples, 0 failures. This validates every worker's category against config/feature_categories.yml.
  • spec/workers/reactive_caching_worker_spec.rb and spec/models/concerns/reactive_caching_spec.rb - 67 examples, 0 failures.
  • rubocop on the three changed Ruby files - no offences. rake danger_local clean.

Out of scope (follow-ups, no issues filed yet)

Dynamic category derivation in context_for_arguments; performance findings in the CI tree noticed while profiling (the per-artifact decompressed-size validator that shells out to gzip -dc | head -c | wc -c, a streaming rewrite of the junit parser, and routing Gitlab::Ci::Config::Entry::Timeout through the existing Gitlab::Ci::Build::DurationParser request-store cache); and the unused old-value cache read in ReactiveCaching#exclusively_update_reactive_cache!.

Changelog

No trailer added - the change is not user facing.

Edited by Marc Shaw

Merge request reports

Loading
Loading