Don't apply adaptive timeouts to LDAP queries #252
Conversation
WalkthroughConstructor in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App as Application
participant Pool as LdapConnectionPool
participant AT as AdaptiveTimeout
participant LDAP as LDAP Server
App->>Pool: new LdapConnectionPool(...)
note right of Pool: Constructor initializes 3 AdaptiveTimeout instances
rect rgba(200,230,255,0.25)
Pool->>AT: Initialize(useAdaptiveTimeout: false)
note right of AT: Adaptive logic disabled → fixed timeout semantics
end
App->>Pool: Execute LDAP query
Pool->>AT: Request current timeout (fixed)
Pool->>LDAP: Send request with timeout
LDAP-->>Pool: Response or timeout
Pool-->>App: Return result or error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/CommonLib/LdapConnectionPool.cs (1)
61-63: LGTM with optional documentation suggestion.The explicit disabling of adaptive timeouts for LDAP query operations aligns with the PR objective. The code correctly passes
useAdaptiveTimeout: falseto ensure queries always use the 2-minute max timeout instead of adjusting based on execution time statistics.Consider adding inline comments to explain the rationale for disabling adaptive timeouts on these specific operations but not on
_testConnectionAdaptiveTimeout(line 64). This would help future maintainers understand the intentional distinction:+// Adaptive timeouts disabled for LDAP queries due to runtime volatility. +// Connection tests (line 64) retain adaptive behavior since they gate query success. _queryAdaptiveTimeout = new AdaptiveTimeout(maxTimeout: TimeSpan.FromMinutes(2), Logging.LogProvider.CreateLogger("LdapQuery"), useAdaptiveTimeout: false); _pagedQueryAdaptiveTimeout = new AdaptiveTimeout(maxTimeout: TimeSpan.FromMinutes(2), Logging.LogProvider.CreateLogger("LdapPagedQuery"), useAdaptiveTimeout: false); _rangedRetrievalAdaptiveTimeout = new AdaptiveTimeout(maxTimeout: TimeSpan.FromMinutes(2), Logging.LogProvider.CreateLogger("LdapRangedRetrieval"), useAdaptiveTimeout: false);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/CommonLib/LdapConnectionPool.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: definitelynotagoblin
PR: SpecterOps/SharpHoundCommon#217
File: src/CommonLib/LdapConnectionPool.cs:677-677
Timestamp: 2025-06-26T16:59:06.972Z
Learning: In SharpHoundCommonLib's LdapConnectionPool.cs, the team acknowledges that method `GetConnectionForSpecificServerAsync` has misleading naming - it contains "Async" in the name but is actually synchronous and blocks on async code, which they recognize as confusing but may need to live with due to backwards compatibility constraints.
📚 Learning: 2025-06-26T16:59:06.972Z
Learnt from: definitelynotagoblin
PR: SpecterOps/SharpHoundCommon#217
File: src/CommonLib/LdapConnectionPool.cs:677-677
Timestamp: 2025-06-26T16:59:06.972Z
Learning: In SharpHoundCommonLib's LdapConnectionPool.cs, the team acknowledges that method `GetConnectionForSpecificServerAsync` has misleading naming - it contains "Async" in the name but is actually synchronous and blocks on async code, which they recognize as confusing but may need to live with due to backwards compatibility constraints.
Applied to files:
src/CommonLib/LdapConnectionPool.cs
🧬 Code graph analysis (1)
src/CommonLib/LdapConnectionPool.cs (2)
src/CommonLib/AdaptiveTimeout.cs (3)
AdaptiveTimeout(12-347)AdaptiveTimeout(28-47)TimeSpan(226-240)src/CommonLib/Logging/Logging.cs (2)
Logging(7-20)LogProvider(22-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
src/CommonLib/LdapConnectionPool.cs (1)
64-64: Verify the intentional distinction for test connection timeouts.The
_testConnectionAdaptiveTimeoutstill uses adaptive timeouts (defaultuseAdaptiveTimeout: true) while the query timeouts on lines 61-63 explicitly disable it. Based on the PR rationale that connection checks already gate query success, this distinction appears intentional—test connections can benefit from adaptive behavior to efficiently validate connections, while queries should use fixed timeouts.Please confirm this intentional design: connection tests retain adaptive timeout behavior because they're more stable/predictable than queries and benefit from the adaptive mechanism, while queries themselves are too volatile to benefit from adaptation. If this is correct, consider documenting this distinction (see comment on lines 61-63).
definitelynotagoblin
force-pushed
the
non-adaptive-timeout-ldaps
branch
from
f67773c to
a6366b5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/CommonLib/AdaptiveTimeout.cs (1)
217-225: Align the Chebyshev comment with the new 7σ boundThe explanatory comment still cites 5σ coverage even though we now use a 7σ multiplier. Please update the math so future readers aren’t misled.
- // Within 5 standard deviations will have a conservative lower bound of catching 96% of executions (1 - 1/5^2), + // Within 7 standard deviations will have a conservative lower bound of catching ~98% of executions (1 - 1/7^2),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/CommonLib/AdaptiveTimeout.cs(1 hunks)src/CommonLib/LdapConnectionPool.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: definitelynotagoblin
PR: SpecterOps/SharpHoundCommon#217
File: src/CommonLib/LdapConnectionPool.cs:677-677
Timestamp: 2025-06-26T16:59:06.972Z
Learning: In SharpHoundCommonLib's LdapConnectionPool.cs, the team acknowledges that method `GetConnectionForSpecificServerAsync` has misleading naming - it contains "Async" in the name but is actually synchronous and blocks on async code, which they recognize as confusing but may need to live with due to backwards compatibility constraints.
📚 Learning: 2025-06-26T16:59:06.972Z
Learnt from: definitelynotagoblin
PR: SpecterOps/SharpHoundCommon#217
File: src/CommonLib/LdapConnectionPool.cs:677-677
Timestamp: 2025-06-26T16:59:06.972Z
Learning: In SharpHoundCommonLib's LdapConnectionPool.cs, the team acknowledges that method `GetConnectionForSpecificServerAsync` has misleading naming - it contains "Async" in the name but is actually synchronous and blocks on async code, which they recognize as confusing but may need to live with due to backwards compatibility constraints.
Applied to files:
src/CommonLib/LdapConnectionPool.cs
🧬 Code graph analysis (1)
src/CommonLib/LdapConnectionPool.cs (2)
src/CommonLib/AdaptiveTimeout.cs (3)
AdaptiveTimeout(12-347)AdaptiveTimeout(28-47)TimeSpan(226-240)src/CommonLib/Logging/Logging.cs (2)
Logging(7-20)LogProvider(22-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
Description
LDAP queries are too volatile I suspect in runtime for adaptive timeouts. By the time backoff logic triggers, retries are already exhausted.
These queries should already be gated by connection checks, so we should be pretty confident that they respond successfully, and so we'll disable adaptive timeouts on them.
Motivation and Context
SpecterOps/SharpHound#177
Closes BED-6603
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist:
Summary by CodeRabbit
Bug Fixes
Performance
Chores