Remove retry_failed_keep_around_ref_writes feature flag
Removes the retry_failed_keep_around_ref_writes feature flag now that its rollout (#609449 (closed)) has finished at default-on since 19.3, and the new behaviour is being kept permanently. The flag fixed #591291 (closed), where a failed Gitaly write_ref was swallowed and reported as success, so the keep-around worker's retries never engaged and a merge commit could be garbage collected before it was needed. With the flag gone, the corrected guard ordering and the retryable failure path are now unconditional for every project.
Detailed context for AI agents
Flag details:
- Name:
retry_failed_keep_around_ref_writes - Type: beta,
default_enabled: true, milestone 19.3 - Definition file deleted:
config/feature_flags/beta/retry_failed_keep_around_ref_writes.yml - Introduced by !248909 (merged)
- Rollout issue: #609449 (closed)
- Underlying bug it fixed: #591291 (closed)
- Related context on the Praefect ref-lockfile race: #608179 (closed)
The original bug (why any of this exists): Gitlab::Git::KeepAround tracked the Gitaly error and moved on when write_ref failed, so MergeRequests::KeepAroundRefsWorker reported success and its retry: 20 never engaged. The merge commit was left unprotected from git gc. The flag turned a failed write into a reported failure that the worker retries, and swapped the guard ordering so an unreachable Gitaly is recorded rather than silently skipped.
What removing the flag changes in the code:
lib/gitlab/git/keep_around.rb-old_executeand theretry_failed_writes?flag read are deleted.new_executebecomesexecute. So the keep-around ref is now unconditionally checked before the commit lookup. That ordering is the load-bearing part:commit_byrescues an unreachable Gitaly and returns nil, so if it ran first the SHA would be skipped before a write was ever attempted and the outage would never be seen.kept_around?raises instead, which is what makes the failure reportable.executealways returns the array of SHAs whose ref could not be written.- The
retry_failed_writes:keyword argument is gone fromGitlab::Git::KeepAround.execute,Repository#keep_around(app/models/repository.rb) and the EE override (ee/app/models/ee/repository.rb). It existed only so the service could read the flag once and pass its answer down, because apercentage_of_timegate re-rolls on every read. With no flag there is nothing to pass. app/services/merge_requests/keep_around_refs_service.rb- the per-projectpartitionon flag state is gone, along withold_execute. Every project now goes through the lease-protected write path andexecutealways returns aServiceResponse. The empty-SHAs early return changed fromniltoServiceResponse.success, so the method has one return type.app/workers/merge_requests/keep_around_refs_worker.rb- theresponse.is_a?(ServiceResponse)type guard is dropped; the worker just checksresponse.error?. It still raisesKeepAroundRefsError(which inheritsGitlab::SidekiqMiddleware::RetryError, keeping the retry out of Sentry and out of the Sidekiq execution SLI) so Sidekiq retries up to 20 times.
What is deliberately unchanged: the disable_keep_around_refs ops kill switch, the gitlab_keeparound_refs_requested_total / gitlab_keeparound_refs_created_total counters and their meaning, the exclusive lease in the service (LEASE_TTL 5 minutes, retries: 0) that stops a Sidekiq retry overlapping an identical queued job, the worker's deduplicate :until_executed, and the Gitlab::Git::Repository::NoRepository rescue that keeps the inline callers safe. The inline callers - Ci::Pipeline#keep_around_commits (an after_commit on: :create), DiffPositionableNote#keep_around_commits, DraftNotes::PublishService, MergeRequest#keep_around_commit and the importer path - all ignore the return value, so the change from shas.uniq to failed_shas does not affect them, and they are still protected by the rescues inside execute.
Spec changes: all stub_feature_flags(retry_failed_keep_around_ref_writes: ...) contexts are removed from spec/lib/gitlab/git/keep_around_spec.rb, spec/services/merge_requests/keep_around_refs_service_spec.rb and spec/workers/merge_requests/keep_around_refs_worker_spec.rb, including the worker's "service returns no ServiceResponse" context, which only existed to cover a fully flag-disabled job. The service spec's fork context was reworked: it used to cover one project enabled and one disabled, and now covers a genuine partial failure across the two projects of a fork merge request (one repository reports an unwritten SHA, the other writes cleanly), so the per-project reporting and logging are still pinned. The "with empty shas" example now asserts be_success rather than just not raising.
Verification performed locally, all green:
- RuboCop clean across the 8 changed Ruby files.
spec/lib/gitlab/git/keep_around_spec.rb- 11 examplesspec/services/merge_requests/keep_around_refs_service_spec.rb- 16 examplesspec/workers/merge_requests/keep_around_refs_worker_spec.rb- 14 examplesee/spec/models/repository_spec.rb#keep_around- 8 examplesspec/tasks/gitlab/keep_around_rake_spec.rbandspec/events/repositories/keep_around_refs_created_event_spec.rb- 14 examplesspec/services/draft_notes/publish_service_spec.rbandspec/models/diff_note_spec.rb- 153 examplesspec/lib/gitlab/import/merge_request_helpers_spec.rbandspec/services/draft_notes/create_service_spec.rb- 30 examples- targeted
spec/models/merge_request_spec.rbandspec/models/ci/pipeline_spec.rbkeep-around examples - 5 examples
Out of scope: no change to the ops kill switch, no metric or dashboard changes, no docs changes (the flag was not documented on the public feature flags list page).