fix(reposerver): context-aware revision lock to prevent convoy deadlock

[marionebl]

Summary

• Make Lock() in reposerver/repository/lock.go context-aware to prevent convoy deadlocks under rapid commit bursts
• Replace sync.Cond (blocks indefinitely) with sync.Mutex + chan struct{} broadcast channel and select on ctx.Done()
• Pass ctx through all 7 Lock() call sites in repository.go

Resolves #26866

Test plan

• New convoy deadlock tests pass (TestLock_WaiterForDifferentRevision_CannotBeUnblocked, TestLock_ConvoyFormsUnderSequentialRevisions)
• All existing lock tests pass unchanged
• go build ./reposerver/... succeeds
• go build ./controller/... succeeds

[bunnyshell]

🔴 Preview Environment stopped on Bunnyshell

See: Environment Details | Pipeline Logs

Available commands (reply to this comment):

• 🔵 /bns:start to start the environment
• 🚀 /bns:deploy to redeploy the environment
• ❌ /bns:delete to remove the environment

[codecov]

Codecov Report

❌ Patch coverage is 91.42857% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.20%. Comparing base (6b35246) to head (318ef90).

Files with missing lines	Patch %	Lines
reposerver/repository/lock.go	88.88%	1 Missing and 1 partial ⚠️
reposerver/repository/repository.go	94.11%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #26867      +/-   ##
==========================================
+ Coverage   63.17%   63.20%   +0.02%     
==========================================
  Files         414      414              
  Lines       56461    56468       +7     
==========================================
+ Hits        35669    35688      +19     
+ Misses      17422    17413       -9     
+ Partials     3370     3367       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[dudinea]

@marionebl thank you for working on this! The change to the locking mechanism looks good to me, have not found any problems with the locking change itself.

Please see my comment for some minor change request.

Some questions:

Do you have any data/graphs on performance improvement that this PR gives?

As a result of the PR some Repo Server methods might start returning context Canceled or DeadLine exceeded errors which they never did before. Have you seen any new/unexpected user visible behavior when running a loaded Argo CD instance with this PR: like any unexpected error messages or transient errors in UI, changes in error statuses in Applications or ApplicationSets?

[alexymantha]

LGTM too, thanks for fixing this!

[ppapapetrou76]

I have a couple of concerns/discussion points

1. The current implementation creates a new channel on every broadcast/lock release
   In high-contention scenarios, this could create GC pressure. WDYT?

2. When a lock is released, all waiting goroutines will try to acquire the lock at the same time. Only one succeeds, and the others go back to waiting. This could be optimized using a semaphore or a queue; otherwise, in theory, a goroutine might be delayed too long if it's unlucky to acquire the lock. WDYT?

3. Consider adding UTs for the following cases

• context cancellation during init() callback
• context cancellation after lock acquisition

and ideally a UT with 100+ concurrent goroutines

[ppapapetrou76]

How about changing the code to the following to avoid the following race condition? After unlocking state.mu, another goroutine could acquire the lock again and read the old broadcast channel reference just as it's being closed. This timing window is small but exists.

if notify {
    close(state.broadcast)
    state.mu.Unlock()
    // Create new channel while others are processing
    state.mu.Lock()
    state.broadcast = make(chan struct{})
    state.mu.Unlock()
} else {
    state.mu.Unlock()
}

[dudinea]

@ppapapetrou76

After unlocking state.mu, another goroutine could acquire the lock again and read the old broadcast channel reference just as it's being closed

in the PR code all reads/updates of the state.broadcast field
are between Lock and Unlock calls, so how can another goroutine read an obsolete reference?

[marionebl]

The current implementation creates a new channel on every broadcast/lock release
In high-contention scenarios, this could create GC pressure. WDYT?

To my understanding the change in GC pressure should be negligible - about 112 bytes per allocation. This shouldn't matter compared to the memory consumed for managing the git operations themselves (200kb - 1MB+).

When a lock is released, all waiting goroutines will try to acquire the lock at the same time. Only one succeeds, and the others go back to waiting. This could be optimized using a semaphore or a queue; otherwise, in theory, a goroutine might be delayed too long if it's unlucky to acquire the lock. WDYT?

I opted to fix just the bug at hand for now; maybe we can improve the waiting / queuing logic in a follow up PR?

Consider adding UTs for the following cases

context cancellation during init() callback

This would imply changing init to receive context, which it does not today. Can we split this out into a separate PR?

context cancellation after lock acquisition

I'm not sure how I'd achieve this. Once Lock returns (closer, nil), the caller owns the lock. The Lock function is done - it has no further interaction with the context. Cancellation after that point is the caller's responsibility (they call closer.Close()).

and ideally a UT with 100+ concurrent goroutines

will do, adapting the test

[marionebl]

Do you have any data/graphs on performance improvement that this PR gives?

No - the problem doesn't surface as performance problem, but as a live lock. The affected repo server pod would stop consuming resources and all GenerateManifest calls would time out.

As a result of the PR some Repo Server methods might start returning context Canceled or DeadLine exceeded errors which they never did before. Have you seen any new/unexpected user visible behavior when running a loaded Argo CD instance with this PR: like any unexpected error messages or transient errors in UI, changes in error statuses in Applications or ApplicationSets?

We haven't observed any changes in user visible behaviour in our internal testing.