Draft: Track pending mergeability checks in Redis instead of the database

Marc Shawrequested to merge
628132-mergeability-check-redis-marker into master

Merge request GETs (widget poll, MR page, REST GET) ran an UPDATE merge_requests SET merge_status to mark the MR as checking before enqueueing the mergeability worker. Behind the mergeability_check_redis_marker flag, the GET now claims a short-lived Redis marker and enqueues only if it wins, the worker clears it when done, and the status readers report checking while the marker exists. No database write in the GET, and one UPDATE per check instead of two. The widget-side change (removing the whole-widget checking override so pending merge requests use the merge checks section) is split out to !256153 (merged).

Detailed context for AI agents

Problem

  • MergeRequest#check_mergeability(async: true) is called from GET paths: the widget poll (Projects::MergeRequests::ContentController#cached_widget), Projects::MergeRequestsController#show and friends, and the REST API GET .../merge_requests/:iid.
  • That calls MergeabilityCheckService#async_execute, which called merge_request.mark_as_checking (an UPDATE merge_requests SET merge_status) before enqueueing MergeRequestMergeabilityCheckWorker. The write happened inside the GET request.
  • Log analysis shows this is roughly 3.6M writes per week, making the widget the top GET endpoint writing to the database. These endpoints are currently allowlisted in lib/gitlab/database/query_analyzers/prevent_writes_on_get.rb (detect_writes_on_get). Follow-up to #628132, itself a corrective action from INC-12449.
  • A write during a GET also sticks the user's following requests to the primary via the database load balancer, so this pushed read traffic onto the primary too.
  • The write only happened on the first GET after a merge request flipped to unchecked (no checking -> checking transition in the state machine, so later polls were no-ops). Volume comes from target-branch pushes flipping many MRs to unchecked in batch, each then getting viewed once.

Change

  • New MergeRequests::MergeabilityCheckMarker (app/models/merge_requests/mergeability_check_marker.rb). Backed by a Redis key merge_request:mergeability_check:<mr_id> in Gitlab::Redis::SharedState.
    • claim: SET NX EX with QUEUED_TTL (5 minutes), returns true only if this call set it.
    • refresh: SET XX EX with RUNNING_TTL (2 minutes).
    • clear: DEL.
    • pending?: EXISTS.
  • MergeabilityCheckService#async_execute: flag on, claims the marker and only enqueues the worker if the claim won (no database write). Flag off, unchanged (mark_as_checking still runs).
  • MergeRequestMergeabilityCheckWorker#perform: calls refresh before running the check (shrinks the TTL from the queued window to the running window) and clear in an ensure, so a finished or failed job frees the merge request for the next check immediately. A job that lost the lease to a check already running (MergeabilityCheckService::FAILED_TO_OBTAIN_LOCK reason) leaves the marker to that check. Both calls are gated on the flag, so with it off the worker is unchanged.
  • AutoMerge::BaseService#available_for? and availability_details memoize per merge request and per check_mergeability_async mode, so one service instance cannot answer a synchronous caller with a result computed for the widget.
  • Webhook payloads read the raw merge_status column, so the merge request open hook now carries unchecked where it used to carry checking; detailed_merge_status in the same payload reads the marker and still says checking. spec/services/merge_requests/after_create_service_spec.rb covers both flag states.
  • ContentController#widget ran the check synchronously twice per GET: MergeRequestPollWidgetEntity#mergeable calls MergeRequest#mergeable?, and available_auto_merge_strategies reaches mergeable? through AutoMerge::MergeWhenChecksPassService#availability_details (and the EE add-to-train variant). mergeable? gains a check_mergeability_async: option, threaded through AutoMergeService#available_strategies and AutoMerge::BaseService#available_for?/availability_details. Only the two read callers set it, gated on the flag: the widget entity and Types::MergeRequestType#available_auto_merge_strategies. PUT .../merge, MergeOrchestrationService and the auto-merge process paths keep the synchronous check, so strategy selection on a merge action is unchanged. With the flag on and the row unchecked, mergeable is false and merge-when-checks-pass is listed as available until the worker writes the verdict; the widget shows the checking state from detailed_merge_status in the meantime and re-polls.
  • Status readers report checking while a marker is present through one override: MergeRequest#checking? returns super || mergeability_check_pending?, so MergeRequest#public_merge_status (REST entities, the widget) and MergeRequests::Mergeability::DetailedMergeStatusService#checking? (GraphQL detailedMergeStatus, webhooks) pick it up without their own Redis calls. mergeability_check_pending? does the Redis read, but only when the row is unchecked or cannot_be_merged_recheck and the flag is on, so settled merge requests never touch Redis. While the marker exists, checking? and unchecked? are both true; nothing in app or lib branches on that combination.
  • Net effect with the flag on: one UPDATE per check (the verdict) instead of two (checking, then verdict).

Why a Redis marker rather than just moving mark_as_checking into the worker

  • Moving the transition into the worker alone would remove the GET write but still costs two UPDATEs per check, and would show unchecked instead of checking until the job actually starts.
  • The marker also dedupes enqueues from the 10-second widget poll while a check is already queued or running.
  • A later follow-up can drop the checking and cannot_be_merged_rechecking states from the state machine (and the MergeRequests::MergeData copy) entirely.

TTL reasoning and edge cases

  • Queued TTL (5 minutes) covers time waiting in Sidekiq, which is exactly what grows during an incident. Running TTL (2 minutes) starts once the worker picks the job up.
  • The marker is a liveness hint, not a correctness boundary. Correctness still comes from the existing exclusive lease in the service and the stale-inputs check (discard_stale_mergeability_verdicts). If the marker expires mid-check, the next poll enqueues a duplicate job that either fails the lease or no-ops because the verdict is already written. The UI shows unchecked instead of checking for one poll; both render the merge checks section with the conflict check spinning once !256153 (merged) lands.
  • A SIGKILLed worker leaves the marker until TTL expiry, so a lost job can delay a re-check by up to 2 minutes. Today a lost job costs nothing, since the next poll just re-enqueues, so this is a small regression for a rare case. TTL values are a tuning knob and should be checked against the worker duration distribution in Kibana before wider rollout.
  • Geo / read-only database: service_error already returns early before the marker is touched.
  • MergeRequests::MergeabilityCheckBatchWorker still uses batch_mark_as_checking in Sidekiq. Left unchanged, since that write is not happening inside a GET.

Feature flag and rollout

  • Flag: mergeability_check_redis_marker, type gitlab_com_derisk, default disabled, actor is the project.
  • Rollout issue: #629171 (closed). Plan: enable for gitlab-org/gitlab first, watch the detect_writes_on_get logs for the widget endpoint and the worker duration, then roll out by percentage of actors.

Split-out frontend change

  • !256153 (merged) removes the whole-widget checking state from the MR widget, so CHECKING and UNCHECKED both fall through to the merge checks section, whose conflict check already shows "Checking for merge conflicts.". Independent of this MR and not flag-gated, so it can merge in either order, but the mergeability_check_redis_marker rollout should wait for it; otherwise EE users see "Set to auto-merge" on a mergeable MR while the verdict is pending.

Out of scope, suggested follow-ups

  • Projects::MergeRequestsController#show_merge_request triggers the async check redundantly with the widget poll. Dropping it would take five endpoints off the allowlist.
  • Remove the merge request endpoints from ALLOWED_ENDPOINTS in prevent_writes_on_get.rb once the flag is enabled by default, so specs fail if the write ever returns.
  • Drop the checking states from the state machine.

Verification

  • Live run on GDK against a real merge request, Sidekiq held to observe the pending state. Flag off: the GET wrote checking, no marker. Flag on: two GETs left the column unchecked, set one marker (TTL 299 s, no second enqueue), REST and GraphQL reported checking, the widget serializer returned mergeable: false with zero merge_requests writes; one worker job then wrote can_be_merged and cleared the marker, and GraphQL availableAutoMergeStrategies went from ["merge_when_checks_pass"] while pending to [].

  • New spec: spec/models/merge_requests/mergeability_check_marker_spec.rb.

  • Widget path specs: spec/serializers/merge_request_poll_widget_entity_spec.rb (flag on: worker enqueued, mergeable false, status unchanged; flag off: synchronous check), spec/models/merge_request_spec.rb (mergeable? async option), spec/services/auto_merge/merge_when_checks_pass_service_spec.rb and ee/spec/services/auto_merge/add_to_merge_train_when_checks_pass_service_spec.rb (available_for? with check_mergeability_async: true does not run the synchronous check). Also green: spec/services/auto_merge/base_service_spec.rb, spec/services/auto_merge_service_spec.rb, ee/spec/services/auto_merge/merge_train_service_spec.rb, spec/graphql/types/merge_request_type_spec.rb.

  • Manual GDK check, MR reset to unchecked before each GET: flag on, cached_widget, show, REST GET/list/changes and widget.json all leave merge_status untouched with 0 merge request writes and the marker set; flag off, the same GETs write checking (or can_be_merged for widget.json).

  • Updated specs: spec/services/merge_requests/mergeability_check_service_spec.rb (flag on: marker set, no merge_status change, no enqueue when already pending, no marker on read-only DB; flag off: still marks as checking), spec/workers/merge_request_mergeability_check_worker_spec.rb (refresh before run, clear after, clear on raise), spec/models/merge_request_spec.rb (public_merge_status with pending marker), spec/services/merge_requests/mergeability/detailed_merge_status_service_spec.rb.

  • Also ran green: spec/serializers/merge_request_poll_cached_widget_entity_spec.rb, spec/serializers/merge_request_poll_widget_entity_spec.rb, the API merge_status spec, and the controller "checks mergeability asynchronously" spec.

  • Rubocop clean.

Related to #628132

Edited by Marc Shaw

Merge request reports