fix: guard LDAP connection reconnect with a mutex to remove data race - #323
fix: guard LDAP connection reconnect with a mutex to remove data race#323mvanhorn wants to merge 2 commits into
Conversation
Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a data race in the LDAP client's connection management. By introducing a mutex to guard the connection retrieval process, it ensures that reconnection logic is executed atomically, preventing multiple redundant connections and potential race conditions during concurrent access. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. A mutex stands guard at the gate, / To keep the connection state straight. / No races shall run, / With locks, we are done, / And concurrency meets its own fate. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces thread safety to the LDAP client connection retrieval by adding a mutex to the LDAPConn struct and refactoring connection creation into a testable dialLDAP function, supported by new concurrent tests. The review feedback recommends optimizing this concurrency model by switching from sync.Mutex to sync.RWMutex. This allows a fast-path read lock check in getConn() when the connection is healthy, preventing unnecessary exclusive lock contention on every LDAP operation.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| } | ||
|
|
||
| type LDAPConn struct { | ||
| mu sync.Mutex |
There was a problem hiding this comment.
Using a simple sync.Mutex means that every call to getConn() will acquire an exclusive lock, even when the connection is perfectly healthy. Since getConn() is called on every LDAP operation, this introduces unnecessary lock contention and serializes all LDAP queries across the operator.
Using a sync.RWMutex allows us to use a fast-path read lock (RLock) to check if the connection is healthy, which completely eliminates contention on the hot path.
| mu sync.Mutex | |
| mu sync.RWMutex |
References
- The repository style guide recommends using sync.RWMutex and utilizing .RLock() for read-only operations as a future enhancement to improve concurrency. (link)
| func (l *LDAPConn) getConn() LDAPConnClient { | ||
| l.mu.Lock() | ||
| defer l.mu.Unlock() | ||
|
|
||
| if l.conn != nil && l.conn.IsClosing() { |
There was a problem hiding this comment.
Implement a fast-path check using a read lock (RLock) to avoid exclusive lock contention when the connection is healthy. If the connection is closing, release the read lock and acquire the write lock (Lock) to perform the reconnect safely with double-checking.
| func (l *LDAPConn) getConn() LDAPConnClient { | |
| l.mu.Lock() | |
| defer l.mu.Unlock() | |
| if l.conn != nil && l.conn.IsClosing() { | |
| func (l *LDAPConn) getConn() LDAPConnClient { | |
| l.mu.RLock() | |
| if l.conn != nil && !l.conn.IsClosing() { | |
| conn := l.conn | |
| l.mu.RUnlock() | |
| return conn | |
| } | |
| l.mu.RUnlock() | |
| l.mu.Lock() | |
| defer l.mu.Unlock() | |
| if l.conn != nil && l.conn.IsClosing() { |
References
- The repository style guide recommends using sync.RWMutex and utilizing .RLock() for read-only operations as a future enhancement to improve concurrency. (link)
|
Warning Review limit reached
Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughChangesLDAP reconnect handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
pkg/clients/ldap/client.go (2)
85-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider deferring the read-lock release for consistency.
l.mu.RUnlock()is called manually in two branches (Line 89 and Line 92). The write-lock section below usesdefer l.mu.Unlock()(Line 95). Use the same defer pattern for the read lock, immediately afterl.mu.RLock(), so a future code path added between the lock and return cannot skip the unlock and cause a permanent read-lock hold on this shared connection guard.♻️ Proposed refactor
l.mu.RLock() + defer l.mu.RUnlock() conn := l.conn connGeneration := l.connGeneration if conn != nil && !conn.IsClosing() { - l.mu.RUnlock() return conn } - l.mu.RUnlock()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/clients/ldap/client.go` around lines 85 - 92, Update the read-lock handling in the connection retrieval block to defer l.mu.RUnlock() immediately after l.mu.RLock(), and remove both manual unlock calls while preserving the existing return behavior for valid connections and the subsequent write-lock flow.
94-117: 🩺 Stability & Availability | 🔵 TrivialNote the retry behavior under sustained dial failures.
The reconnect (dial + bind) runs while holding the exclusive write lock, which is expected since there's a single shared connection. However, a failed dial does not advance
connGeneration(only the success path at Line 116 does). So if the LDAP server stays unreachable, each caller that arrives after a prior failure re-attempts its own full dial+bind serially under the lock rather than reusing the just-failed result. With many concurrent callers during an outage, this can serialize toN × 5s(the dial timeout) before all callers give up. Consider a short-lived failure cache or backoff so concurrent callers arriving during an active outage window don't each pay the full dial timeout.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/clients/ldap/client.go` around lines 94 - 117, Update the reconnect logic around the connection-generation check and dialLDAP/UnauthenticatedBind flow to cache failed reconnect attempts or apply a short backoff window. Record the failed attempt and its expiry while holding the existing lock, and have callers arriving during that active outage window return the current connection without retrying dial and bind; preserve the existing successful reconnect behavior and generation increment.pkg/clients/ldap/client_test.go (1)
104-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend coverage for two related reconnect paths.
This test covers a dial failure. Two related paths in
getConn()remain untested:
- A bind failure after a successful dial (
client.goLines 108-114), which should callnewConn.Close()and incrementcloseCounton the mock.- Concurrent callers hitting a failing dial at the same time, to confirm
connGenerationstays unchanged and no caller observes a partially-updated connection.Add these two cases to increase confidence in the new locking and generation logic.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/clients/ldap/client_test.go` around lines 104 - 124, Extend TestGetLdapConnection_ReconnectFailureKeepsExistingConnection coverage with a bind-failure case that uses a successfully dialed new connection, verifies the bind error, confirms newConn.Close() increments the mock closeCount, and preserves the existing connection. Add a concurrent failing-dial case using synchronized callers, asserting all callers receive failure, connGeneration remains unchanged, and ldapConn.conn is never partially replaced.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/clients/ldap/client_test.go`:
- Around line 104-124: Extend
TestGetLdapConnection_ReconnectFailureKeepsExistingConnection coverage with a
bind-failure case that uses a successfully dialed new connection, verifies the
bind error, confirms newConn.Close() increments the mock closeCount, and
preserves the existing connection. Add a concurrent failing-dial case using
synchronized callers, asserting all callers receive failure, connGeneration
remains unchanged, and ldapConn.conn is never partially replaced.
In `@pkg/clients/ldap/client.go`:
- Around line 85-92: Update the read-lock handling in the connection retrieval
block to defer l.mu.RUnlock() immediately after l.mu.RLock(), and remove both
manual unlock calls while preserving the existing return behavior for valid
connections and the subsequent write-lock flow.
- Around line 94-117: Update the reconnect logic around the
connection-generation check and dialLDAP/UnauthenticatedBind flow to cache
failed reconnect attempts or apply a short backoff window. Record the failed
attempt and its expiry while holding the existing lock, and have callers
arriving during that active outage window return the current connection without
retrying dial and bind; preserve the existing successful reconnect behavior and
generation increment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8a64102b-64d0-4fb7-a821-0578da0d105a
📒 Files selected for processing (2)
pkg/clients/ldap/client.gopkg/clients/ldap/client_test.go
getConn runs on every LDAP operation, so taking an exclusive lock even when the connection is healthy serialized all queries. Use an RWMutex: check the healthy case under RLock, and only escalate to the write lock when a reconnect is actually needed. A generation counter makes the escalation safe -- if another goroutine reconnected while we were upgrading, we return its connection instead of dialing a second time. Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
c07e20c to
3d6a5e3
Compare
1 participant
Changes
📝 Description
What changed?
getConn()inpkg/clients/ldap/client.gonow takes async.Mutexbefore it checksIsClosing(), reconnects, and writesl.conn. Previously these steps ran without synchronization, so two goroutines could both reconnect and writel.connat the same time. A newmu sync.Mutexfield guards every read and write ofl.conn.Why is this change needed?
When the reconciler, the offboarding job, and LDAP query resolution call
getConn()concurrently while the connection is closing, each goroutine seesIsClosing()true and dials a new connection. That leaks all but one of the new connections and races on the unsynchronizedl.conn = newConnwrite. This fixes #277.Dependencies
🧪 Testing
Test Coverage
Added two tests in
pkg/clients/ldap/client_test.go:TestGetLdapConnection_ConcurrentReconnect: 50 goroutines callgetConn()at once on a closing connection and assert exactly one reconnect, one bind, and a single sharedl.conn.TestGetLdapConnection_ReconnectFailureKeepsExistingConnection: when the dial fails,getConn()returns nil and leaves the existingl.connuntouched.Commands run:
go build ./...passed.go test -race ./pkg/clients/ldap/...passed.The reconnect dial is now reached through a
dialLDAPpackage variable so the concurrency test can inject a stub without a live LDAP server. Real behavior is unchanged: it dials the same URL with the same 5s timeout.Performance Impact
🚀 Deployment
Deploy Steps
Prerequisites
Post-Deployment Monitoring
Rollback Plan
Details:
⚙️ Configuration Changes
✅ Developer Checklist
Fixes #277
Summary by CodeRabbit
Bug Fixes
Tests