## Summary This issue records the Sec requirements and the GitLab.com measurements for unified Sidekiq flow control. It does not propose a new architecture. Two issues owned by group::tenant services already propose the change: - gitlab-com/gl-infra/tenant-scale/tenant-services/team#137, "Use concurrency limiter for job deferring" - gitlab-com/gl-infra/tenant-scale/tenant-services/team#130, "Evaluate Deferring Sidekiq workers with new circuit breaker/throttling" Both sit under gitlab-com/gl-infra/tenant-scale/tenant-services&58, which states as expected impact that "the flow-control layer for Sidekiq is unified under the concurrency limiter, replacing the separate skip/defer middleware". Scope of this issue is GitLab.com and the `sec` database. Dedicated and Cells validation stays with tenant services. >>> This issue replaces an earlier version that proposed building the mechanism from scratch. That version was written before the existing `Throttling` subsystem and the two issues above were found. ## The user-facing problem `defer_on_database_health_signal` reschedules each job independently with `perform_in`. Sidekiq orders the scheduled set by run time, not by enqueue time, so each job re-checks health on a phase offset from its own arrival. When the signal clears, the order in which jobs run bears little relation to the order in which they arrived. A user whose pipeline finishes has no deterministic expectation of when security ingestion completes. Two pipelines finishing seconds apart can have their ingestion complete tens of minutes apart, decided by nothing but arrival phase. ### Measured behaviour Simulation of `SkipJobs#deferral_delay` over 20,000 jobs with uniform arrivals during a sustained red signal. The constants match `lib/gitlab/sidekiq_middleware/skip_jobs.rb`. Ordering, measured as the share of arrival-ordered job pairs where the later job executed first: | red period | backoff off | backoff on | |---|---|---| | 30 min | 29.3% | 32.2% | | 1 hour | 43.8% | 32.1% | | 3 hours | 47.4% | 29.3% | | 6 hours | 48.7% | 30.7% | At six hours the ordering is indistinguishable from random. No configuration of `delay_by` or the backoff flag improves this, because the cause is per-job independent rescheduling rather than the delay values. Latency, for a 3 hour autovacuum: | delay_by | `incremental_database_health_defer_delay` | wait p99 | executes after recovery, p99 | average deferrals per job | |---|---|---|---|---| | 1 min | off (current default) | 179 min | 1.0 min | 90.9 | | 5 min | off | 180 min | 4.9 min | 18.6 | | 1 min | on | 202 min | 28.8 min | 7.5 | | 5 min | on | 204 min | 28.7 min | 5.4 | Two conclusions follow: 1. `delay_by: 5.minutes` reduces re-enqueue volume by a factor of 5 and costs about 4 minutes of additional tail. This is a good trade. 2. Enabling `incremental_database_health_defer_delay` adds roughly 23 minutes to the p99 wait and lets work arrive up to 30 minutes after the database recovered. The post-recovery tail is bounded by `DATABASE_HEALTH_MAX_DELAY`, not by the incident length. This is a poor trade for a user-facing ingestion path. ## Mechanism findings These were read from master and verified against a production-sec clone where measurable. 1. **Deferral is unbounded.** `SkipJobs#defer_job!` increments `deferred_count` and re-enqueues. There is no maximum defer count, no drop path and no dead letter. `ApplicationWorker.deferred` only sets job-hash flags. The `retry` budget covers exceptions, not deferrals. 2. **Health is evaluated once per job execution, with no cache.** `HealthStatus.evaluate` maps over all four indicators with no early exit. Three of them subclass `PrometheusAlertIndicator` and issue a live Mimir query per evaluation, because `strong_memoize_attr` is per instance and a new instance is built each time. `AutovacuumActiveOnTable` adds two catalog queries forced onto the primary, measured at about 4 ms on a production-sec clone. The cost does not vary with the number of watched tables. 3. **Deduplication does not preserve position.** `DuplicateJobs::Server` runs before `SkipJobs` in the server chain, so the idempotency key is released before the deferral happens. The re-enqueued job takes a fresh key. ## Defect to report when both mechanisms are active A job resumed from the concurrency limiter buffer carries `concurrency_limit_resume: true`, which makes `ConcurrencyLimit::Middleware#resumed?` skip re-buffering it. `SkipJobs#defer_job!` re-enqueues with only `deferred`, `deferred_count`, `deferred_by` and `jid`. The resume marker is dropped. A resumed job that is then deferred on health therefore re-enters the buffer at the back and loses its FIFO position. This defeats the ordering property that #137 is intended to provide. It matters during any migration period where both mechanisms are enabled on the same worker. ## Preconditions for the sec workers The concurrency limiter is inert for these workers today: - They declare no `concurrency_limit`, so `get_concurrency_limit` returns 0. - `over_the_limit?` returns false when the limit is 0. - `Throttling::Middleware#throttle!` returns early when the current limit is 0. - `sidekiq_concurrency_limit_default_calculation`, which would derive a limit from fleet percentage, is an ops flag and is default off. - `sidekiq_throttling_middleware` is a worker flag and is default off. `LimitManager#current_limit` falls back to `worker_klass.get_concurrency_limit` when Redis holds no override, and overrides expire after 30 minutes. The worker attribute is therefore the natural place to express a health-derived ceiling. ## Sec input to the tenant services design 1. **Per-worker contended-table knowledge already exists.** `database_health_check_attrs[:tables]` holds the curated list per worker. !255632 and !255634 corrected those lists for the two heaviest sec ingestion workers. A health-aware decision can read them to answer whether a given worker contributes to the table under contention, rather than treating database pressure as a single global condition. 2. **The `sec` database is a live driver.** This work comes out of WAL saturation alerts on `patroni-sec`, tracked in &23596. 3. **Migration order matters.** A health-driven concurrency limit and `defer_on_database_health_signal` must not be active on the same worker for the same signal. `Throttling::Server` runs before `SkipJobs` in the server chain, and `Throttling::Middleware#perform` never stops a job, it only rewrites the limit and yields. `SkipJobs` therefore gets the last word with a binary stop and masks any graded response. ## Proposed sequence 1. Agree the approach with group::tenant services on #130 and #137. 2. Cache the database health signal and add an early exit to `HealthStatus.evaluate` on the first Stop. This is required before any per-enqueue or per-perform evaluation of a health-derived limit, and it reduces cost under the current mechanism too. 3. Add a health-derived input to `Throttling::Decider`, scoped to workers that declare `gitlab_sec` tables, opt-in per worker. 4. Migrate the sec ingestion workers to the concurrency limiter for flow control. 5. Remove `defer_on_database_health_signal` from the migrated workers, which answers #130 for these workers. ## Independent of the above `delay_by` on the two sec ingestion workers should move from 1 minute to 5 minutes, matching every other multi-table watcher in the tree. The table above shows the trade. `incremental_database_health_defer_delay` should stay off for these workers. ## References - Parent epic: https://gitlab.com/groups/gitlab-org/-/epics/23596 - Tenant services epic: https://gitlab.com/groups/gitlab-com/gl-infra/tenant-scale/tenant-services/-/epics/58 - !255632 and !255634, the table list corrections - #629386, the remaining uncovered writer