Move the checking merge status write into the mergeability worker

Marc Shawrequested to merge
628132-mark-as-checking-in-worker into master

Moves the merge_status = checking UPDATE out of the GET request path (widget poll, MR page, REST GET) and into MergeRequestMergeabilityCheckWorker, behind the mark_mergeability_checking_in_worker flag. The GET now only enqueues the worker, and the worker marks the row as checking right before running the check, so the row reads unchecked until the job is picked up. This is the simpler alternative to the Redis marker in !255070 (closed) - only one of the two should merge.

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 the REST GET GET /projects/:id/merge_requests/:merge_request_iid.
  • It calls MergeRequests::MergeabilityCheckService#async_execute, which ran merge_request.mark_as_checking (an UPDATE merge_requests SET merge_status = 'checking') before enqueueing MergeRequestMergeabilityCheckWorker. That is a database write inside a GET request.
  • Log analysis in the linked work item shows roughly 3.6M such 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. The work item is a corrective action from incident INC-12449.
  • A write during a GET also sticks the user's following requests to the primary database via the load balancer.

Change

Small: 2 production files, 2 spec files, 1 feature flag YAML.

  • New feature flag mark_mergeability_checking_in_worker, type gitlab_com_derisk, default disabled, actor is the project.
  • MergeabilityCheckService#async_execute: with the flag on, skips mark_as_checking and only enqueues the worker. Flag off: unchanged.
  • MergeRequestMergeabilityCheckWorker#perform: with the flag on, calls the guarded MergeRequest.batch_mark_as_checking([merge_request.id]) right before running MergeabilityCheckService#execute. It only moves rows still in unchecked / cannot_be_merged_recheck, so a verdict written between find_by_id and this line is left alone (raised in review by Patrick). Flag off: unchanged.
  • Net effect: same number of UPDATEs per check as today (checking, then the verdict), but the first one moves from the web request into Sidekiq.

Behaviour with the flag on

  • Between the GET and the job being picked up, the row reads unchecked instead of checking. public_merge_status, GraphQL detailedMergeStatus (UNCHECKED rather than CHECKING) and webhooks reflect that.
  • No visible widget change. The whole-widget "checking" spinner was removed in !256153 (merged), and CheckConflictStatusService reports checking for both unchecked and checking, so the widget shows "Checking if merge request can be merged..." with the conflict check spinning in both states. Verified on GDK with the flag off (row checking) and on (row unchecked): pixel-identical widgets. Note that the MR page itself usually settles the status synchronously through widget.json (MergeRequestPollWidgetEntity#mergeable), so the unchecked window is mostly visible to the REST GET and cached_widget callers.
  • GET /projects/:id/merge_requests?with_merge_status_recheck=true already works this way: the GET only enqueues MergeabilityCheckBatchWorker, which runs batch_mark_as_checking in Sidekiq.
  • Enqueue dedup: the worker is idempotent!, so Sidekiq already deduplicates with the default until_executing strategy while a job is queued. Re-enqueues during a running job are not deduplicated, same as today, because recheck_merge_status? is true for checking too.
  • The guarded update_all matches nothing when the row is already checking, preparing, or settled, so a job enqueued before the flag flipped, or one racing a synchronous check, is a no-op on that line.
  • Jobs enqueued while the flag was off already had checking written in the GET; the worker's guarded write is a no-op for them.

Alternative approach

  • !255070 (closed) (same author) implements a Redis marker instead: the GET claims a short-lived Redis key and enqueues only if it wins, the worker clears it, and status readers report checking while the key exists. That saves one UPDATE per check and dedupes during the running window, but adds a Redis round trip to status reads and a new lifecycle to reason about.
  • This MR is the simpler alternative raised in that MR's review thread: move the write into the worker and accept that the status reads unchecked until the job starts. The two MRs are alternatives; only one should merge.

Edge cases

  • Race: the worker writes checking outside the mergeability exclusive lease. An earlier revision used merge_request.mark_as_checking, which saves the in-memory row, so a verdict a synchronous check (for example PUT .../merge) wrote between find_by_id and that line was flipped back to checking; if the worker then lost the lease the row stayed checking until the next poll. The guarded batch_mark_as_checking closes that: the in-memory row still reads unchecked, which is a valid source state for every later transition, and write_merge_status already discards stale verdicts. Covered by the worker spec example does not overwrite a merge status written after the merge request was loaded, which fails on the unguarded line.
  • MergeRequests::MergeabilityCheckBatchWorker still uses batch_mark_as_checking; unchanged, because that write already happens in Sidekiq.
  • Geo / read-only database: service_error in async_execute still returns early before enqueueing.

Feature flag and rollout

  • Flag mark_mergeability_checking_in_worker. Rollout issue: #630354.
  • Plan: enable for gitlab-org/gitlab first, watch the detect_writes_on_get logs for the widget endpoint and the worker's duration, then percentage of actors.
  • Once default-on, follow-ups: remove the allowlist entries in prevent_writes_on_get.rb, and consider deduplicate :until_executed, if_deduplicated: :reschedule_once on the worker.

Verification

  • spec/services/merge_requests/mergeability_check_service_spec.rb #async_execute block: flag on leaves the row unchecked, flag off marks checking; 4 examples pass.
  • spec/workers/merge_request_mergeability_check_worker_spec.rb: flag on marks checking before execute, flag off leaves unchecked, and a verdict written after find_by_id is not overwritten (fails on the unguarded line); 9 examples pass including the idempotent worker shared example.
  • spec/models/merge_request_spec.rb #check_mergeability block: 11 examples pass.
  • RuboCop clean on all changed files.

Try-to-break testing (GDK, 2026-09-22)

All with mark_mergeability_checking_in_worker enabled for the project unless stated.

  • Dedup: three REST GETs on an unchecked MR with Sidekiq stopped produced one queued job. The until_executing cookie lives 10 minutes, so even the later page load was absorbed.
  • No GET writes: single GET, project list GET with with_merge_status_recheck=true, and the widget page all left the row unchecked and only enqueued jobs.
  • Flag flipped off between enqueue and run: the worker wrote the verdict straight from unchecked. Flag flipped on between a flag-off enqueue (row already checking) and run: the worker's mark_as_checking was a no-op and the verdict was written.
  • Worker loses the lease to a synchronous check: the row is left at checking and the job logs Failed to obtain a lock. The next poll re-enqueues (the dedup cookie is cleared when a job starts) and settles it once the lease is free. Same behaviour as today.
  • Stale overwrite: reproduced in a Rails runner against the unguarded mark_as_checking revision (verdict flipped back to checking, row stuck until the next poll). Now closed by the guarded batch_mark_as_checking; see the race edge case above.
  • Rows in preparing, already checking, or settled are not matched by the guarded update, and the following mark_as_mergeable still runs from the in-memory state.
  • GraphQL while queued: mergeStatusEnum and detailedMergeStatus are UNCHECKED instead of CHECKING; the CONFLICT mergeability check is CHECKING either way, so the widget renders the same spinner.
  • Webhooks: the merge request open hook payload now carries merge_status: unchecked where it carried checking. spec/services/merge_requests/after_create_service_spec.rb covers both flag states. The predictive rspec pipeline did not select that spec, so this only surfaced locally.
  • Local run of 1217 examples across the mergeability service, worker, batch worker, entities, controller, REST API and prevent_writes_on_get specs: green after the webhook spec update.

Out of scope

  • Removing the checking state from the state machine.
  • The redundant check trigger in Projects::MergeRequestsController#show (the widget poll triggers the same check right after page load).
  • The allowlist entries in prevent_writes_on_get.rb.

References

Edited by Marc Shaw

Merge request reports