Retry keep-around ref writes that fail

Gitlab::Git::KeepAround swallowed the Gitaly error when write_ref failed, so MergeRequests::KeepAroundRefsWorker reported success and its retry: 20 never engaged — leaving the merge commit unprotected from git gc. This matters more since !248802 (merged) deduplicated the worker: the redundant jobs that used to paper over a failed write are gone.

• KeepAround#execute now returns the SHAs whose ref it could not write; KeepAroundRefsService passes them to the worker as a ServiceResponse error, and the worker raises so Sidekiq retries.
• A lease across the write (in_lock, single attempt) keeps a retrying job from racing an identical newly-enqueued one.
• Nothing is left ungated. KeepAround and KeepAroundRefsService each dispatch on retry_failed_keep_around_ref_writes (gitlab_com_derisk, default off, actor is the project) to an old_execute holding today's code and a new_execute holding the change, so with the flag off the RPCs, logs, error tracking, exceptions and return values are all as on master. Rollout issue: #609449.

Verified with 51 specs covering both flag states, five revert-checks confirming each gate holds, and manual runs on a Praefect-backed GDK — details below.

Detailed context for AI agents

1. Why the ref is checked first, in a method of its own

commit_by returns nil both when the commit is gone and when Gitaly is unreachable, because Gitlab::Git::Commit.find rescues CommandError, NoRepository, ArgumentError → nil (lib/gitlab/git/commit.rb:81). So an outage skipped the SHA before anything was attempted and nothing could be reported.

kept_around? does raise, through Repository#ref_exists? (app/models/repository.rb:371), which rescues only ArgumentError. Checking the ref first is therefore what makes an unreachable Gitaly observable. Both keep-around counters retain their meaning — the skip for an already-kept SHA still happens after the requested counter — and the RPC count per SHA is unchanged (2) except for a SHA whose commit is missing, which goes from 1 to 2.

The two orderings live in old_execute and new_execute rather than in one method branching on the flag, so the disabled path is master's code rather than a reconstruction of it: old_execute's body diffs byte-for-byte against master's execute. It also removes the reading in which kept_around? looks like it is called twice per SHA — the calls are one per path.

This ordering is also what produces the one real rollout risk — see known gaps.

2. Exception taxonomy

Checking the ref first gives up the shield Commit.find provided, so the rescue had to widen. NoRepository is Class.new(::Gitlab::Git::BaseError) (lib/gitlab/git/repository.rb:32) — not a CommandError — so without it in the rescue list a repository that disappears mid-flight would raise straight out of execute and interrupt inline callers. Ci::Pipeline#keep_around_commits runs in after_commit on: :create (app/models/ci/pipeline.rb:204), so that would abort pipeline creation.

Gitaly condition	raised by kept_around?	rescued?
unreachable (UNAVAILABLE)	Gitlab::Git::CommandError	yes
DEADLINE_EXCEEDED	CommandTimedOut < CommandError	yes
repository missing (NOT_FOUND)	Gitlab::Git::Repository::NoRepository	yes, flag on only — tracked and skipped, never reported
overloaded / circuit open	Gitlab::Git::ResourceExhaustedError	no — see known gaps
INVALID_ARGUMENT	ArgumentError	swallowed by Repository#ref_exists?

NoRepository has its own rescue arm in new_execute, which tracks it and skips the SHA without reporting it: the repository is gone, so no retry can write the ref, and reporting it would burn all 20 attempts on a permanent condition. It is still rescued rather than left to raise, because an inline caller such as Ci::Pipeline#keep_around_commits must not be interrupted. old_execute does not rescue it at all, exactly as master, so with the flag off it propagates as it does today.

3. Why a write lease is needed

deduplicate :until_executed takes its idempotency key in check!, reached from duplicate_jobs/strategies/deduplicates_when_scheduling.rb, which runs in client middleware. Sidekiq re-enqueues a retry via JobRetry / the scheduled poller, neither of which runs client middleware. So a retrying job:

1. never runs the deduplication check, so it is never suppressed — it carries the idempotency_key written into its payload at duplicate_job.rb:76, but nothing reads it on the way in, and
2. still runs the ensure duplicate_job.delete! in until_executed.rb:26 — unconditional, because reschedulable? is false without if_deduplicated: :reschedule_once (duplicate_job.rb:180) — deleting cookie_key (duplicate_job.rb:229), which is derived from the job args alone. A newly-enqueued identical job holds that same key, so the retry's cleanup releases it.

Before this MR the worker never raised, so the window did not exist. Adding the raise would reopen the concurrent write_ref race that #608179 (closed) added the deduplication to prevent. KeepAroundRefsService#write_refs closes it by holding the write inside Gitlab::ExclusiveLeaseHelpers#in_lock with retries: 0: a single obtain attempt, no sleep (SleepingLock#obtain never sleeps before the first attempt and there is no second), raising FailedToObtainLockError on contention. in_lock's ensure lease&.cancel is a compare-and-delete on its own uuid, so a failed attempt cannot release the holder's lease.

Contention reports the SHAs as unwritten rather than returning success, so the worker retries. Returning success would permanently drop the write whenever the lease holder was interrupted before finishing — the exact failure this MR exists to fix.

But contention is not a failure: nothing was attempted and no Gitaly error was tracked. The distinction lives in the logs rather than the return value: the error's payload[:unwritten_shas] is one flat array, and the service logs each cause under its own message (Keep-around reference write failed / Keep-around reference write skipped, lease already held). Collapsing the messages would make a contention retry indistinguishable from an unreachable Gitaly in Kibana, which is the one signal this rollout has — RetryError is deliberately absent from Sentry (section 4). The per-project log lines live in the service because the worker only sees the job's full project_ids and cannot attribute a SHA to a repository; that matters for a fork merge request, where the flag can be on for one of the two projects. The worker's own warn line is just the job-level summary.

The lease lives in the service rather than the worker because that is where per-project flag state is known, which is what keeps it gated. It is keyed on project + sorted SHAs rather than on the job arguments, since it is the ref write that must not overlap. Partial SHA overlap between two jobs still produces different keys and is not covered — unchanged from what deduplication gave.

LEASE_TTL is 5 minutes, which is long for a worker at urgency :high. That is deliberate: in_lock releases the lease on every path including a raise, so the timeout is only reached when the process dies mid-write. A TTL that can expire while a write is still running would let a second job overlap it and reintroduce the Praefect reference-transaction race the lease exists to prevent, whereas a lease still held after a crash only costs a few of the worker's 20 retries.

4. Why KeepAroundRefsError inherits RetryError

Gitlab::SidekiqMiddleware::RetryError is excluded from Sentry (lib/gitlab/error_tracking.rb:125) and from the Sidekiq execution SLI (lib/gitlab/sidekiq_middleware/server_metrics.rb:181). The retry is the expected outcome and KeepAround has already tracked the underlying Gitaly error with object_id: sha, so a plain StandardError would duplicate it in Sentry once per attempt and burn the code_review_workflow error budget on an urgency :high worker. Prior art in the same namespace: MergeRequests::CreatePipelineWorker::PipelineCreationRetryError.

Consequence: after 20 attempts the job dies into the Sidekiq dead set with no Sentry event. There is no sidekiq_retries_exhausted handler, so the dead set is the signal — called out in the rollout issue.

5. Gating audit

Change	Gated	How
Ref checked before commit_by	yes	only in KeepAround#new_execute, chosen by retry_failed_writes?, actor @repository.project
NoRepository rescued, tracked and skipped	yes	only in new_execute; old_execute lets it propagate
Failure reporting to the worker	yes	only from the service's new_execute, per project
Write lease	yes	write_refs is only reached from the service's new_execute
Per-project Gitlab::AppLogger lines	yes	only emitted from the service's new_execute
Worker raise + RetryError	yes	only reachable on a ServiceResponse error
Return value of KeepAround#execute	yes	old_execute returns master's value
Return value of KeepAroundRefsService#execute	yes	execute returns old_execute's value when no project is enabled

Nothing is unconditional. KeepAround#old_execute is master's execute renamed: its body diffs identical, down to the shas.uniq.each return value. KeepAroundRefsService#old_execute is master's loop with the repositories helper inlined (projects.map(&:repository).each), returning the same array of repositories, and execute returns that value when the partition finds no enabled project. The Project.id_in query is unchanged; the only extra work on the disabled path is the flag check itself.

That exactness is why MergeRequests::KeepAroundRefsWorker tests the type rather than calling error? on whatever comes back — response.is_a?(ServiceResponse) && response.error?. The old service path returns an Array, so a bare response&.error? would raise NoMethodError on the disabled path and burn 20 retries there. The type check goes away with the flag. For completeness: no caller reads KeepAround#execute's value either — Ci::Pipeline, DiffPositionableNote, DraftNotes::PublishService and MergeRequest all discard it, and the EE Repository#keep_around override (ee/app/models/ee/repository.rb:56) is super + ensure, which preserves the body's return value.

write_refs returns failed.to_a, so a nil from a stubbed or future keep_around cannot poison the array union in execute. spec/spec_helper.rb's global stub returns [] to match the contract; with that guard in place it is accuracy rather than a requirement.

6. Test inventory

spec/lib/gitlab/git/keep_around_spec.rb (15) — flag on: refs written, write failure reported, unreachable Gitaly reported, missing repository tracked without being reported or raising, missing commit skipped. Flag off: commit looked up first, the ref never checked when Gitaly is unreachable, missing repository still raises, failed write tracked and swallowed. No flag-off example asserts a return value, since that path returns master's. Plus the disable_keep_around_refs kill switch and the multi-SHA cases.

spec/services/merge_requests/keep_around_refs_service_spec.rb (19) — per-project flag gating including a fork merge request with the flag on for only one of its two projects; lease key stability across SHA order, per-project key separation, contention reporting the SHAs as unwritten with the write skipped, release after success and after a raise, and no lease taken at all with the flag off. Each cause logs its own message with the owning project_id, only the enabled project of a fork is logged, and nothing is logged with the flag off or with the kill switch on.

spec/workers/merge_requests/keep_around_refs_worker_spec.rb (17) — deduplication strategy, KeepAroundRefsError being a RetryError, raise-and-log on a ServiceResponse error, silence on a success, silence on each shape the disabled path can return (nil, [], an array of repositories), the missing-parameter guards, and the idempotent-worker shared example.

7. Verification

51 specs, 0 failures: 15 lib, 19 service, 17 worker.

Each gate was checked by reverting it and confirming the right specs fail:

Reverted	Failures
NoRepository arm dropped from new_execute	1 — tracks a missing repository without reporting the SHAs or raising
new_execute forced back to master's ordering	2 — both flag-on cases that depend on the ref being checked first
Flag dispatch removed from KeepAround#execute	3 — every flag-off case
Per-project gate removed from the service	5 — flag-off return value and logging, both fork cases, and the flag-off lease case
Worker's ServiceResponse type check removed	2 — the [] and repositories-array cases

Manual verification on a Praefect-backed GDK: happy path writes the ref; a held lease makes the service report the SHAs and skip the write; the worker raises KeepAroundRefsError under contention; with the flag off the write proceeds despite a held lease; the lease is released after a successful write. Both log messages observed in application_json.log with project_id, shas and source. The unreachable-Gitaly behaviour (gdk stop praefect-gitaly-0) was verified the same way. Those runs predate the ServiceResponse and old_execute/new_execute refactor, so the return shapes observed were the earlier flat arrays; the RPCs, logs and raises are what the specs now pin.

8. Known gaps

• The guard ordering costs Sentry volume on every inline caller, not just the worker. With the flag on, an unreachable Gitaly is tracked once per SHA from every caller of Repository#keep_around; on master commit_by returned nil and the SHA was skipped silently with no event. Ci::Pipeline#keep_around_commits (2 SHAs, on every pipeline create), DiffPositionableNote#keep_around_commits and DraftNotes::PublishService#keep_around_commits all discard the return value, so they take the Sentry volume without gaining the retry. Gitlab::ErrorTracking.track_exception is not rate limited, so during a Gitaly incident on an enabled project this is unbounded. It is the main thing to watch on rollout, and the reason the actor is the project rather than the instance.
• The NoRepository rescue also covers write_ref, not only kept_around?. NOT_FOUND maps to NoRepository (lib/gitlab/git/wraps_gitaly_errors.rb:44), so with the flag on a repository that disappears mid-write is tracked and skipped instead of raising out of an inline caller. That is the intended direction, and it no longer costs retries now that NoRepository is never reported, but the write_ref path itself is untested — the specs reach the arm through kept_around?.
• Gitlab::Git::ResourceExhaustedError (Gitaly overload via Gitaly::LimitError, gRPC RESOURCE_EXHAUSTED, or an open circuit breaker — lib/gitlab/git/wraps_gitaly_errors.rb:35,50 and gitaly_client/circuit_breaker.rb:83) is not a CommandError and already escapes commit_by on both flag states. Those failures are still not reported, so this does not cover an overloaded Gitaly.
• write_ref failing on a corrupt ref file is permanent, and burns all 20 attempts before reaching the dead set.
• On EE, Repositories::KeepAroundRefsCreatedEvent is published from an ensure regardless of whether the writes succeeded, so Geo is notified even for a failed write. Pre-existing; the return value now makes fixing it possible.

References

• #591291