[BED-5972] Log timeout-wrapped execution time stats, add adaptive timeout logic #222
Conversation
## Walkthrough
This update introduces an adaptive timeout mechanism throughout the codebase by implementing new `AdaptiveTimeout` and `ExecutionTimeSampler` classes. Numerous processors, utilities, and services are refactored to use adaptive timeouts instead of static or hardcoded values. Public method signatures are updated to remove explicit timeout parameters, and corresponding tests are adjusted or commented out to reflect the new adaptive approach.
## Changes
| File(s) | Change Summary |
|---------|----------------|
| `src/CommonLib/AdaptiveTimeout.cs`, `src/CommonLib/ExecutionTimeSampler.cs` | Added new classes for adaptive timeout and execution time sampling, providing dynamic timeout calculation and statistics. |
| `src/CommonLib/IRegistryKey.cs`, `src/CommonLib/LdapUtils.cs`, `src/CommonLib/Ntlm/HttpNtlmAuthenticationService.cs`, `src/CommonLib/Processors/CertAbuseProcessor.cs`, `src/CommonLib/Processors/ComputerSessionProcessor.cs`, `src/CommonLib/Processors/DCLdapProcessor.cs`, `src/CommonLib/Processors/LocalGroupProcessor.cs`, `src/CommonLib/Processors/PortScanner.cs`, `src/CommonLib/Processors/SmbProcessor.cs`, `src/CommonLib/Processors/UserRightsAssignmentProcessor.cs`, `src/CommonLib/SMB/SmbScanner.cs` | Refactored to use `AdaptiveTimeout` instances for operation timeouts; removed or updated timeout parameters in public methods and constructors. |
| `src/CommonLib/Timeout.cs` | Added optional `CancellationToken` parameters to timeout execution wrappers for improved cancellation support. |
| `src/CommonLib/LdapConnectionPool.cs` | Added adaptive timeout handling for LDAP queries with retry logic on timeout exceptions. |
| `src/SharpHoundRPC/PortScanner/IPortScanner.cs` | Updated `CheckPort` interface method to remove the timeout parameter. |
| `test/unit/AdaptiveTimeoutTest.cs` | Added new unit tests for `AdaptiveTimeout` covering adaptive calculation, disabling, and spike handling. |
| `test/unit/ComputerAvailabilityTests.cs`, `test/unit/LocalGroupProcessorTest.cs` | Updated mocks and test calls to match new method signatures without timeout parameters. |
| `test/unit/ComputerSessionProcessorTest.cs`, `test/unit/DCLdapProcessorTest.cs`, `test/unit/HttpNtlmAuthenticationServiceTest.cs`, `test/unit/UserRightsAssignmentProcessorTest.cs` | Commented out or marked as obsolete several timeout-specific tests due to the new adaptive timeout mechanism; adjusted event handlers to match updated signatures. |
| `test/unit/SmbProcessorTest.cs` | Updated event handler and removed explicit timeout argument in test call. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Caller
participant AdaptiveTimeout
participant ExecutionTimeSampler
participant TargetOperation
Caller->>AdaptiveTimeout: ExecuteWithTimeout(Func)
AdaptiveTimeout->>ExecutionTimeSampler: SampleExecutionTime(Func)
ExecutionTimeSampler->>TargetOperation: Invoke operation
TargetOperation-->>ExecutionTimeSampler: Return result
ExecutionTimeSampler-->>AdaptiveTimeout: Record timing sample
AdaptiveTimeout-->>Caller: Return result (with adaptive timeout enforcement)Suggested reviewers
Poem
|
definitelynotagoblin
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🔭 Outside diff range comments (3)
src/CommonLib/Processors/DCLdapProcessor.cs (1)
83-83: Fix copy-paste error in log messageThe log message uses
isSigningRequired.Statusinstead ofisChannelBindingDisabled.Status.-_log.LogTrace("DCLdapScan failed on IsChannelBindingDisabled for {ComputerName}: {Status}", computerName, isSigningRequired.Status); +_log.LogTrace("DCLdapScan failed on IsChannelBindingDisabled for {ComputerName}: {Status}", computerName, isChannelBindingDisabled.Status);src/CommonLib/Processors/LocalGroupProcessor.cs (1)
43-52: Avoid blocking on async code to prevent deadlocksUsing
.GetAwaiter().GetResult()blocks synchronously on async code, which can cause deadlocks in certain contexts (e.g., UI threads, ASP.NET contexts).Consider making this method async:
-public virtual SharpHoundRPC.Result<ISAMServer> OpenSamServer(string computerName) +public virtual async Task<SharpHoundRPC.Result<ISAMServer>> OpenSamServerAsync(string computerName) { - var result = _openSamServerAdaptiveTimeout.ExecuteRPCWithTimeout((_) => SAMServer.OpenServer(computerName)).GetAwaiter().GetResult(); + var result = await _openSamServerAdaptiveTimeout.ExecuteRPCWithTimeout((_) => SAMServer.OpenServer(computerName)); if (result.IsFailed) { return SharpHoundRPC.Result<ISAMServer>.Fail(result.SError); } return SharpHoundRPC.Result<ISAMServer>.Ok(result.Value); }Then update the call site at line 72:
-var openServerResult = OpenSamServer(computerName); +var openServerResult = await OpenSamServerAsync(computerName);src/CommonLib/Ntlm/HttpNtlmAuthenticationService.cs (1)
162-162: Fix copy-paste error in exception messageThe exception message says "CheckIsNtlmSigningRequired failed" but this is in the
CheckIsChannelBindingDisabledmethod.-return SharpHoundRPC.Result<bool>.Fail($"CheckIsNtlmSigningRequired failed: {ex}"); +return SharpHoundRPC.Result<bool>.Fail($"CheckIsChannelBindingDisabled failed: {ex}");
🧹 Nitpick comments (5)
test/unit/HttpNtlmAuthenticationServiceTest.cs (1)
80-89: Consider cleaning up commented test code.While commenting out timeout-specific tests during the adaptive timeout refactoring is understandable, consider either:
- Removing the commented tests entirely to reduce code clutter
- Replacing them with tests that verify the new adaptive timeout behavior
Keeping commented code long-term impacts maintainability and code clarity.
Apply this diff to clean up the commented code:
- // Obsolete by AdaptiveTimeout - // [Fact] - // public void HttpNtlmAuthenticationService_EnsureRequiresAuth_GetSupportedNtlmAuthSchemesAsync_Timeout() { - // var url = new Uri("http://primary.testlab.local/"); - // var service = new HttpNtlmAuthenticationService(new HttpClientFactory(), null); - // var ex = Assert.ThrowsAsync<TimeoutException>(() => - // service.EnsureRequiresAuth(url, true)); - // Assert.Equal($"Timeout getting supported NTLM auth schemes for {url}", ex.Result.Message); - - // } - - // Obsolete by AdaptiveTimeout - // [Fact] - // public void HttpNtlmAuthenticationService_AuthWithBadChannelBindingsAsync_Timeout() { - // var url = new Uri("http://primary.testlab.local/"); - // var authScheme = "NTLM"; - // var service = new HttpNtlmAuthenticationService(new HttpClientFactory(), null); - // var httpResponseMessage = new HttpResponseMessage { - // StatusCode = HttpStatusCode.InternalServerError, - // }; - // var mockAuthenticator = new Mock<NtlmAuthenticationHandler>(It.IsAny<string>(), null); - // mockAuthenticator.Setup(x => - // x.PerformNtlmAuthenticationAsync(It.IsAny<INtlmTransport>(), It.IsAny<CancellationToken>())).Returns(async () => { - // await Task.Delay(1000); - // return httpResponseMessage; - // }); - - // var ex = Assert.ThrowsAsync<TimeoutException>(async () => await TestPrivateMethod.InstanceMethod<Task>(service, - // "AuthWithBadChannelBindingsAsync", - // [ - // url, authScheme, TimeSpan.FromMilliseconds(1), mockAuthenticator.Object - // ])); - // Assert.Equal($"Timeout during NTLM authentication for {url} with {authScheme}", ex.Result.Message); - - // }Also applies to: 91-114
test/unit/ComputerSessionProcessorTest.cs (1)
219-257: Consider adding tests for the new adaptive timeout mechanism.While commenting out the obsolete timeout tests makes sense given the architectural shift to adaptive timeouts, consider whether equivalent tests should be added to verify the adaptive timeout behavior. The current approach leaves untested code paths that were previously covered.
Consider either:
- Adding new tests that verify the adaptive timeout mechanism works as expected
- Removing the commented code entirely to reduce maintenance burden
Additionally, verify that the adaptive timeout mechanism provides adequate coverage for the scenarios these tests previously validated.
test/unit/UserRightsAssignmentProcessorTest.cs (1)
71-92: Test coverage gap for adaptive timeout mechanism.The commented-out timeout test leaves a coverage gap for the new adaptive timeout functionality. Consider adding tests to verify that the
UserRightsAssignmentProcessorcorrectly handles timeout scenarios with the new adaptive timeout mechanism.Consider adding a new test that verifies the adaptive timeout behavior or remove the commented code entirely to reduce maintenance overhead.
src/CommonLib/ExecutionTimeSampler.cs (1)
35-41: Optimize statistical calculations for performance.The
StandardDeviation()method performs O(n) calculations every time it's called, which can be expensive when called frequently (especially during logging). Consider caching the result or using an incremental calculation approach.+private double? _cachedStandardDeviation; +private double? _cachedAverage; +private int _lastCalculationCount; public double StandardDeviation() { + if (_samples.Count == _lastCalculationCount && _cachedStandardDeviation.HasValue) { + return _cachedStandardDeviation.Value; + } + double average = _samples.Average(); double sumOfSquaresOfDifferences = _samples.Select(val => (val - average) * (val - average)).Sum(); double stddiv = Math.Sqrt(sumOfSquaresOfDifferences / _samples.Count); + _cachedStandardDeviation = stddiv; + _cachedAverage = average; + _lastCalculationCount = _samples.Count; + return stddiv; }src/CommonLib/AdaptiveTimeout.cs (1)
165-165: Fix typo in commentThe comment has a typo in "Tchebysheff's Rule".
-// https://modelassist.epixanalytics.com/space/EA/26574957/Tchebysheffs+Rule +// https://modelassist.epixanalytics.com/space/EA/26574957/Chebyshev's+Rule
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
src/CommonLib/AdaptiveTimeout.cs(1 hunks)src/CommonLib/ExecutionTimeSampler.cs(1 hunks)src/CommonLib/IRegistryKey.cs(3 hunks)src/CommonLib/LdapUtils.cs(3 hunks)src/CommonLib/Ntlm/HttpNtlmAuthenticationService.cs(5 hunks)src/CommonLib/Processors/CertAbuseProcessor.cs(3 hunks)src/CommonLib/Processors/ComputerAvailability.cs(3 hunks)src/CommonLib/Processors/ComputerSessionProcessor.cs(4 hunks)src/CommonLib/Processors/DCLdapProcessor.cs(3 hunks)src/CommonLib/Processors/LocalGroupProcessor.cs(9 hunks)src/CommonLib/Processors/PortScanner.cs(3 hunks)src/CommonLib/Processors/SmbProcessor.cs(1 hunks)src/CommonLib/Processors/UserRightsAssignmentProcessor.cs(4 hunks)src/CommonLib/SMB/SmbScanner.cs(2 hunks)src/CommonLib/Timeout.cs(3 hunks)src/SharpHoundRPC/PortScanner/IPortScanner.cs(1 hunks)test/unit/AdaptiveTimeoutTest.cs(1 hunks)test/unit/ComputerAvailabilityTests.cs(1 hunks)test/unit/ComputerSessionProcessorTest.cs(1 hunks)test/unit/DCLdapProcessorTest.cs(3 hunks)test/unit/HttpNtlmAuthenticationServiceTest.cs(1 hunks)test/unit/LocalGroupProcessorTest.cs(12 hunks)test/unit/SmbProcessorTest.cs(1 hunks)test/unit/UserRightsAssignmentProcessorTest.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (16)
test/unit/UserRightsAssignmentProcessorTest.cs (1)
Learnt from: definitelynotagoblin
PR: SpecterOps/SharpHoundCommon#217
File: src/CommonLib/Timeout.cs:17-36
Timestamp: 2025-06-26T16:38:49.677Z
Learning: In SharpHoundCommonLib's Timeout.cs, the team initially had concerns about disposing CancellationTokenSource instances before tasks could check cancellation tokens, but they understand that orphaned tasks (those that exceed timeout) won't be impacted by disposed tokens since their results are already ignored by the timeout handler. They prefer proper resource management with using statements for CancellationTokenSource disposal.
src/CommonLib/LdapUtils.cs (1)
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.
src/SharpHoundRPC/PortScanner/IPortScanner.cs (1)
Learnt from: definitelynotagoblin
PR: SpecterOps/SharpHoundCommon#217
File: src/CommonLib/Timeout.cs:21-21
Timestamp: 2025-06-26T16:37:09.005Z
Learning: In SharpHoundCommonLib's Timeout.cs, the team prefers to use TaskScheduler.Current instead of TaskScheduler.Default in Task.Factory.StartNew calls to maintain flexibility for custom schedulers, even though this requires awareness of potential deadlock risks in certain contexts.
test/unit/HttpNtlmAuthenticationServiceTest.cs (1)
Learnt from: definitelynotagoblin
PR: SpecterOps/SharpHoundCommon#217
File: src/CommonLib/Timeout.cs:17-36
Timestamp: 2025-06-26T16:38:49.677Z
Learning: In SharpHoundCommonLib's Timeout.cs, the team initially had concerns about disposing CancellationTokenSource instances before tasks could check cancellation tokens, but they understand that orphaned tasks (those that exceed timeout) won't be impacted by disposed tokens since their results are already ignored by the timeout handler. They prefer proper resource management with using statements for CancellationTokenSource disposal.
src/CommonLib/Processors/ComputerAvailability.cs (1)
Learnt from: definitelynotagoblin
PR: SpecterOps/SharpHoundCommon#217
File: src/CommonLib/Timeout.cs:21-21
Timestamp: 2025-06-26T16:37:09.005Z
Learning: In SharpHoundCommonLib's Timeout.cs, the team prefers to use TaskScheduler.Current instead of TaskScheduler.Default in Task.Factory.StartNew calls to maintain flexibility for custom schedulers, even though this requires awareness of potential deadlock risks in certain contexts.
src/CommonLib/IRegistryKey.cs (2)
Learnt from: definitelynotagoblin
PR: SpecterOps/SharpHoundCommon#217
File: src/CommonLib/Timeout.cs:21-21
Timestamp: 2025-06-26T16:37:09.005Z
Learning: In SharpHoundCommonLib's Timeout.cs, the team prefers to use TaskScheduler.Current instead of TaskScheduler.Default in Task.Factory.StartNew calls to maintain flexibility for custom schedulers, even though this requires awareness of potential deadlock risks in certain contexts.
Learnt from: definitelynotagoblin
PR: SpecterOps/SharpHoundCommon#217
File: src/CommonLib/Timeout.cs:17-36
Timestamp: 2025-06-26T16:38:49.677Z
Learning: In SharpHoundCommonLib's Timeout.cs, the team initially had concerns about disposing CancellationTokenSource instances before tasks could check cancellation tokens, but they understand that orphaned tasks (those that exceed timeout) won't be impacted by disposed tokens since their results are already ignored by the timeout handler. They prefer proper resource management with using statements for CancellationTokenSource disposal.
test/unit/ComputerSessionProcessorTest.cs (1)
Learnt from: definitelynotagoblin
PR: SpecterOps/SharpHoundCommon#217
File: src/CommonLib/Timeout.cs:17-36
Timestamp: 2025-06-26T16:38:49.677Z
Learning: In SharpHoundCommonLib's Timeout.cs, the team initially had concerns about disposing CancellationTokenSource instances before tasks could check cancellation tokens, but they understand that orphaned tasks (those that exceed timeout) won't be impacted by disposed tokens since their results are already ignored by the timeout handler. They prefer proper resource management with using statements for CancellationTokenSource disposal.
test/unit/DCLdapProcessorTest.cs (1)
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.
src/CommonLib/ExecutionTimeSampler.cs (2)
Learnt from: definitelynotagoblin
PR: SpecterOps/SharpHoundCommon#217
File: src/CommonLib/Timeout.cs:17-36
Timestamp: 2025-06-26T16:38:49.677Z
Learning: In SharpHoundCommonLib's Timeout.cs, the team initially had concerns about disposing CancellationTokenSource instances before tasks could check cancellation tokens, but they understand that orphaned tasks (those that exceed timeout) won't be impacted by disposed tokens since their results are already ignored by the timeout handler. They prefer proper resource management with using statements for CancellationTokenSource disposal.
Learnt from: definitelynotagoblin
PR: SpecterOps/SharpHoundCommon#217
File: src/CommonLib/Timeout.cs:21-21
Timestamp: 2025-06-26T16:37:09.005Z
Learning: In SharpHoundCommonLib's Timeout.cs, the team prefers to use TaskScheduler.Current instead of TaskScheduler.Default in Task.Factory.StartNew calls to maintain flexibility for custom schedulers, even though this requires awareness of potential deadlock risks in certain contexts.
test/unit/AdaptiveTimeoutTest.cs (2)
Learnt from: definitelynotagoblin
PR: SpecterOps/SharpHoundCommon#217
File: src/CommonLib/Timeout.cs:17-36
Timestamp: 2025-06-26T16:38:49.677Z
Learning: In SharpHoundCommonLib's Timeout.cs, the team initially had concerns about disposing CancellationTokenSource instances before tasks could check cancellation tokens, but they understand that orphaned tasks (those that exceed timeout) won't be impacted by disposed tokens since their results are already ignored by the timeout handler. They prefer proper resource management with using statements for CancellationTokenSource disposal.
Learnt from: definitelynotagoblin
PR: SpecterOps/SharpHoundCommon#217
File: src/CommonLib/Timeout.cs:21-21
Timestamp: 2025-06-26T16:37:09.005Z
Learning: In SharpHoundCommonLib's Timeout.cs, the team prefers to use TaskScheduler.Current instead of TaskScheduler.Default in Task.Factory.StartNew calls to maintain flexibility for custom schedulers, even though this requires awareness of potential deadlock risks in certain contexts.
src/CommonLib/Processors/CertAbuseProcessor.cs (1)
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.
src/CommonLib/AdaptiveTimeout.cs (2)
Learnt from: definitelynotagoblin
PR: SpecterOps/SharpHoundCommon#217
File: src/CommonLib/Timeout.cs:17-36
Timestamp: 2025-06-26T16:38:49.677Z
Learning: In SharpHoundCommonLib's Timeout.cs, the team initially had concerns about disposing CancellationTokenSource instances before tasks could check cancellation tokens, but they understand that orphaned tasks (those that exceed timeout) won't be impacted by disposed tokens since their results are already ignored by the timeout handler. They prefer proper resource management with using statements for CancellationTokenSource disposal.
Learnt from: definitelynotagoblin
PR: SpecterOps/SharpHoundCommon#217
File: src/CommonLib/Timeout.cs:21-21
Timestamp: 2025-06-26T16:37:09.005Z
Learning: In SharpHoundCommonLib's Timeout.cs, the team prefers to use TaskScheduler.Current instead of TaskScheduler.Default in Task.Factory.StartNew calls to maintain flexibility for custom schedulers, even though this requires awareness of potential deadlock risks in certain contexts.
src/CommonLib/Processors/DCLdapProcessor.cs (1)
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.
src/CommonLib/Timeout.cs (2)
Learnt from: definitelynotagoblin
PR: SpecterOps/SharpHoundCommon#217
File: src/CommonLib/Timeout.cs:17-36
Timestamp: 2025-06-26T16:38:49.677Z
Learning: In SharpHoundCommonLib's Timeout.cs, the team initially had concerns about disposing CancellationTokenSource instances before tasks could check cancellation tokens, but they understand that orphaned tasks (those that exceed timeout) won't be impacted by disposed tokens since their results are already ignored by the timeout handler. They prefer proper resource management with using statements for CancellationTokenSource disposal.
Learnt from: definitelynotagoblin
PR: SpecterOps/SharpHoundCommon#217
File: src/CommonLib/Timeout.cs:21-21
Timestamp: 2025-06-26T16:37:09.005Z
Learning: In SharpHoundCommonLib's Timeout.cs, the team prefers to use TaskScheduler.Current instead of TaskScheduler.Default in Task.Factory.StartNew calls to maintain flexibility for custom schedulers, even though this requires awareness of potential deadlock risks in certain contexts.
src/CommonLib/Processors/PortScanner.cs (1)
Learnt from: definitelynotagoblin
PR: SpecterOps/SharpHoundCommon#217
File: src/CommonLib/Timeout.cs:21-21
Timestamp: 2025-06-26T16:37:09.005Z
Learning: In SharpHoundCommonLib's Timeout.cs, the team prefers to use TaskScheduler.Current instead of TaskScheduler.Default in Task.Factory.StartNew calls to maintain flexibility for custom schedulers, even though this requires awareness of potential deadlock risks in certain contexts.
src/CommonLib/Ntlm/HttpNtlmAuthenticationService.cs (1)
Learnt from: definitelynotagoblin
PR: SpecterOps/SharpHoundCommon#217
File: src/CommonLib/Timeout.cs:17-36
Timestamp: 2025-06-26T16:38:49.677Z
Learning: In SharpHoundCommonLib's Timeout.cs, the team initially had concerns about disposing CancellationTokenSource instances before tasks could check cancellation tokens, but they understand that orphaned tasks (those that exceed timeout) won't be impacted by disposed tokens since their results are already ignored by the timeout handler. They prefer proper resource management with using statements for CancellationTokenSource disposal.
🧬 Code Graph Analysis (4)
test/unit/ComputerAvailabilityTests.cs (2)
src/CommonLib/Processors/PortScanner.cs (4)
Task(31-69)PortScanner(9-96)PortScanner(14-16)PortScanner(18-21)src/SharpHoundRPC/PortScanner/IPortScanner.cs (1)
Task(5-5)
src/CommonLib/Processors/ComputerAvailability.cs (1)
src/CommonLib/Processors/PortScanner.cs (3)
PortScanner(9-96)PortScanner(14-16)PortScanner(18-21)
src/CommonLib/Processors/ComputerSessionProcessor.cs (3)
src/CommonLib/AdaptiveTimeout.cs (3)
AdaptiveTimeout(9-193)AdaptiveTimeout(19-25)TimeSpan(167-175)src/CommonLib/Logging/Logging.cs (2)
Logging(7-20)LogProvider(22-35)src/CommonLib/OutputTypes/APIResults/SessionAPIResult.cs (1)
SessionAPIResult(5-8)
src/CommonLib/Processors/DCLdapProcessor.cs (4)
src/CommonLib/AdaptiveTimeout.cs (2)
AdaptiveTimeout(9-193)AdaptiveTimeout(19-25)src/CommonLib/Logging/Logging.cs (2)
Logging(7-20)LogProvider(22-35)src/CommonLib/OutputTypes/Computer.cs (1)
LdapService(33-58)src/CommonLib/Processors/LocalGroupProcessor.cs (1)
SharpHoundRPC(43-52)
⏰ 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 (48)
test/unit/SmbProcessorTest.cs (2)
40-43: Event handler signature update looks correct.The change from async lambda to synchronous lambda returning
Task.CompletedTaskmaintains the same functionality while aligning with the updated asynchronous event handling pattern.
44-44: Timeout parameter removal aligns with adaptive timeout refactoring.The removal of the explicit timeout parameter from the
Scanmethod call is consistent with the broader refactoring to use adaptive timeout mechanisms internally.test/unit/ComputerAvailabilityTests.cs (1)
21-21: Mock setup correctly updated for interface change.The mock setup changes properly reflect the updated
IPortScanner.CheckPortmethod signature which removed the timeout parameter in favor of adaptive timeout handling.Also applies to: 26-26
test/unit/LocalGroupProcessorTest.cs (2)
32-32: Mock setups correctly updated for adaptive timeout refactoring.All mock setups for
OpenSamServerhave been properly updated to expect only the string parameter, removing the timeout parameter to align with the new adaptive timeout mechanism.Also applies to: 60-60, 165-165, 187-187, 208-208, 229-229, 250-250, 271-271, 294-294, 317-317, 346-346
175-175: Method call updated to remove explicit timeout parameter.The removal of the timeout parameter from the
GetLocalGroupscall is consistent with the adaptive timeout refactoring approach.src/SharpHoundRPC/PortScanner/IPortScanner.cs (1)
5-5: Interface method signature correctly updated for adaptive timeout.The removal of the timeout parameter from the
CheckPortmethod signature is appropriate as timeout handling is now managed internally by the adaptive timeout mechanism.src/CommonLib/Processors/ComputerAvailability.cs (3)
19-19: LGTM: Improved timeout configuration approach.The change to pass
maxTimeoutto thePortScannerconstructor is cleaner than the previous approach of storing timeout separately. This aligns well with the adaptive timeout mechanism that manages timeouts internally.
29-29: LGTM: Consistent timeout parameter handling.The fallback constructor correctly applies the same timeout configuration pattern as the primary constructor.
101-101: LGTM: Simplified method call aligns with adaptive timeout design.The removal of the timeout parameter from
CheckPortis correct since thePortScannernow manages timeouts internally using the adaptive timeout mechanism.src/CommonLib/IRegistryKey.cs (3)
3-3: LGTM: Required import for logging functionality.The Microsoft.Extensions.Logging import is needed for the AdaptiveTimeout logger configuration.
14-14: LGTM: Well-configured adaptive timeout for registry operations.The AdaptiveTimeout configuration is appropriate for registry operations:
- 10-second max timeout is reasonable for remote registry connections
- Sample count of 100 provides good statistical basis
- Log frequency of 1000 avoids excessive logging
- Minimum 30 samples before adaptive behavior kicks in is conservative
40-40: LGTM: Proper integration with adaptive timeout mechanism.The change from the previous timeout mechanism to the adaptive timeout is correctly implemented and maintains the same error handling pattern.
src/CommonLib/LdapUtils.cs (3)
40-42: LGTM: Well-configured adaptive timeouts for network operations.The AdaptiveTimeout instances are appropriately configured for their respective operations:
_requestNetBiosNameAdaptiveTimeoutwith 1-minute max timeout is suitable for network-based name resolution_callNetWkstaGetInfoAdaptiveTimeoutwith 2-minute max timeout is appropriate for potentially slow network API callsThe sampling parameters (100 samples, 1000 log frequency, 30 min samples) are consistent with other AdaptiveTimeout uses in the codebase.
719-719: LGTM: Proper integration with adaptive timeout for NetWkstaGetInfo.The change from the previous timeout mechanism to the adaptive timeout is correctly implemented and maintains the same error handling pattern.
787-787: LGTM: Consistent adaptive timeout integration for NetBIOS name resolution.The method call correctly uses the adaptive timeout mechanism while preserving the existing error handling and return value patterns.
test/unit/DCLdapProcessorTest.cs (3)
42-45: LGTM: Event handler conversion is correct.The change from async lambda to synchronous lambda returning
Task.CompletedTaskis the correct approach for maintaining the async signature while having synchronous handler logic.Also applies to: 70-73
46-46: LGTM: Method signature simplification aligns with adaptive timeout approach.Removing the explicit timeout parameter from the
Scanmethod calls correctly reflects the internal shift to adaptive timeout management.Also applies to: 74-74
89-117: Adaptive timeout behavior is already covered by existing testsThe
AdaptiveTimeoutTest.cssuite intest/unit/AdaptiveTimeoutTest.csincludes multiple[Fact]methods that verify:
- Default timeout when there aren’t enough samples
- Behavior when adaptive timeout is disabled
- Dynamic adjustment of the timeout based on varying delays
Since these tests comprehensively cover the adaptive timeout mechanism, the commented-out
DCLdapProcessor_CheckScan_Timeoutmethod can safely remain deprecated with no additional timeout tests required.src/CommonLib/Processors/SmbProcessor.cs (2)
19-25: LGTM: Well-implemented adaptive timeout integration.The adaptive timeout implementation is correctly integrated:
- Field properly initialized with reasonable parameters (2-minute max timeout, 100 samples, logging frequency of 1000)
- Constructor correctly sets
MaxTimeoutMson the ISmbScanner instance- Timeout configuration is consistent and appropriate for SMB operations
28-29: LGTM: Clean method signature and adaptive timeout usage.The simplified method signature and use of
ExecuteRPCWithTimeoutcorrectly implements the adaptive timeout pattern while maintaining the same functionality.src/CommonLib/SMB/SmbScanner.cs (3)
30-31: LGTM: Property rename improves clarity.Renaming
TimeoutMstoMaxTimeoutMsbetter reflects its role as the maximum timeout ceiling for the adaptive timeout mechanism.
130-130: LGTM: Correct adaptive timeout implementation.The use of
ExecuteWithTimeoutwith a timeout token parameter correctly implements the adaptive timeout pattern for SMB negotiation operations.
35-38: Fix constructor initialization order issue.The
AdaptiveTimeoutis initialized usingMaxTimeoutMsbefore it might be set by the caller. This creates a potential issue where the adaptive timeout uses the default value (2000ms) instead of the intended maximum timeout.Consider this fix to ensure proper initialization:
public SmbScanner(ILogger log) { _log = log ?? Logging.LogProvider.CreateLogger("SmbScanner"); +} +public void InitializeAdaptiveTimeout() { _adaptiveTimeout = new AdaptiveTimeout(maxTimeout: TimeSpan.FromMilliseconds(MaxTimeoutMs), Logging.LogProvider.CreateLogger(nameof(TrySMBNegotiate)), sampleCount: 100, logFrequency: 1000, minSamplesForAdaptiveTimeout: 30); }Or initialize the adaptive timeout lazily when first used, or accept the max timeout as a constructor parameter.
Likely an incorrect or invalid review comment.
src/CommonLib/ExecutionTimeSampler.cs (1)
13-28: LGTM: Well-designed sampling infrastructure.The overall design is solid:
- Thread-safe queue for concurrent access
- Proper rolling sample size management
- IDisposable implementation for cleanup
- Configurable logging frequency
- Clean API for different delegate types
src/CommonLib/Processors/CertAbuseProcessor.cs (3)
21-22: LGTM: Proper separation of adaptive timeouts for different operations.Using separate
AdaptiveTimeoutinstances forGetMachineSidandOpenSamServeroperations is the correct approach since these are different RPC operations that may have different performance characteristics. The initialization parameters are consistent and appropriate.Also applies to: 30-31
371-371: LGTM: Correct async adaptive timeout usage.The
ExecuteRPCWithTimeoutcall correctly passes the cancellation token to the asyncGetMachineSidmethod, maintaining proper async/await patterns.
451-451: LGTM: Correct sync adaptive timeout usage.The synchronous
OpenSamServeroperation correctly usesExecuteRPCWithTimeoutwithGetAwaiter().GetResult()to maintain the synchronous interface while leveraging the adaptive timeout mechanism.src/CommonLib/Processors/PortScanner.cs (5)
12-12: LGTM - Adaptive timeout field addition.The addition of the
_adaptiveTimeoutfield follows the established pattern for integrating adaptive timeout functionality.
14-16: LGTM - Default constructor delegation.The default constructor properly delegates to the parameterized constructor, maintaining backward compatibility.
18-21: LGTM - Adaptive timeout initialization.The AdaptiveTimeout initialization uses appropriate parameters:
- 10-second max timeout (reasonable for port scanning)
- 100 sample count for good statistical accuracy
- 1000 log frequency for reasonable logging intervals
- 30 minimum samples before adaptive behavior kicks in
The logger name "PortScanner" provides clear context for debugging.
31-32: LGTM - Method signature updated correctly.The
CheckPortmethod signature correctly removes thetimeoutparameter, delegating timeout management to the adaptive timeout instance.
45-47: LGTM - Adaptive timeout execution implementation.The replacement of static
Timeout.ExecuteWithTimeoutwith_adaptiveTimeout.ExecuteWithTimeoutis correct, and the logging now appropriately usesGetAdaptiveTimeout()to show the actual timeout value being used.src/CommonLib/Timeout.cs (3)
140-141: LGTM - Cancellation token support added.The addition of the optional
parentTokenparameter enables proper cancellation token chaining for adaptive timeout scenarios while maintaining backward compatibility.
156-157: LGTM - Consistent cancellation token integration.The RPC timeout wrapper correctly forwards the parent cancellation token to the underlying timeout mechanism.
172-173: LGTM - Async RPC timeout wrapper updated.The async overload of
ExecuteRPCWithTimeoutis consistently updated to support external cancellation tokens.test/unit/AdaptiveTimeoutTest.cs (4)
17-25: LGTM - Test for insufficient samples scenario.The test correctly verifies that when sample count is below the minimum threshold (3), the adaptive timeout returns the maximum timeout value. The test setup and assertion are appropriate.
28-38: LGTM - Test for disabled adaptive timeout.The test correctly verifies that when adaptive timeout is disabled via the constructor parameter, the maximum timeout is always returned regardless of sample count. The multiple executions ensure sufficient samples are collected.
41-51: LGTM - Test for normal adaptive timeout operation.The test correctly verifies that with sufficient samples and adaptive mode enabled, the timeout value adapts to be less than the maximum. The varying delays (40ms, 50ms, 60ms) provide realistic execution time variance.
54-67: LGTM - Test for time spike safety valve.The test correctly simulates the safety valve scenario by first establishing a baseline with fast operations (10ms), then introducing slower operations (100ms) to trigger the safety valve. The assertion verifies that the timeout reverts to the maximum when the safety valve is triggered.
src/CommonLib/Processors/UserRightsAssignmentProcessor.cs (4)
17-19: LGTM - Multiple adaptive timeout instances.Using separate
AdaptiveTimeoutinstances for different RPC operations (OpenLSAPolicy,GetLocalDomainInformation,GetResolvedPrincipalsWithPrivilege) is the correct approach, as each operation has different timing characteristics.
24-26: LGTM - Adaptive timeout initialization.The three adaptive timeout instances are properly initialized with:
- 2-minute max timeout (appropriate for LSA operations)
- Specific logger names for each operation (good for debugging)
- Consistent sampling parameters (100 samples, 1000 log frequency, 30 min samples)
The logger names using
nameof()provide clear operation context.
55-55: LGTM - Method signature updated correctly.The removal of the
timeoutparameter fromGetUserRightsAssignmentsis correct, as timeout management is now handled by the adaptive timeout instances.
56-56: LGTM - RPC calls updated with adaptive timeouts.All three RPC operations correctly use their respective adaptive timeout instances:
- Line 56:
_openLSAPolicyAdaptiveTimeoutfor policy opening- Line 74:
_getLocalDomainInfoAdaptiveTimeoutfor domain info- Line 101:
_getResolvedPrincipalWithPriviledgeAdaptiveTimeoutfor privilege enumerationThe calls properly use
ExecuteRPCWithTimeoutwith the cancellation token parameter.Also applies to: 74-74, 101-101
src/CommonLib/Processors/ComputerSessionProcessor.cs (5)
25-26: LGTM - Dual adaptive timeout instances.Using separate
AdaptiveTimeoutinstances forReadUserSessionsandReadUserSessionsPrivilegedis appropriate since these operations have different timing characteristics and privilege requirements.
39-40: LGTM - Adaptive timeout initialization.Both adaptive timeout instances are properly initialized with:
- 2-minute max timeout (appropriate for network session enumeration)
- Specific logger names using
nameof()for clear operation context- Consistent sampling parameters
The logger names provide clear differentiation between privileged and non-privileged operations.
54-55: LGTM - ReadUserSessions method signature updated.The removal of the
timeoutparameter is correct, delegating timeout management to the adaptive timeout instance.
192-193: LGTM - ReadUserSessionsPrivileged method signature updated.Consistent with the other method, the timeout parameter is correctly removed.
60-84: LGTM - NetAPI calls updated with adaptive timeouts.Both session enumeration methods correctly use their respective adaptive timeout instances:
- Line 60:
_readUserSessionsAdaptiveTimeoutforNetSessionEnum- Line 197:
_readUserSessionsPriviledgedAdaptiveTimeoutforNetWkstaUserEnumThe calls properly use
ExecuteNetAPIWithTimeoutwith the cancellation token parameter, and the timeout token is correctly passed through to the impersonation logic and cancellation checks.Also applies to: 197-222
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
test/unit/AdaptiveTimeoutTest.cs (3)
19-19: Add clarity to constructor parameters with named arguments or constants.The AdaptiveTimeout constructor takes several numeric parameters that aren't immediately clear from the test context. Consider using named arguments or constants to improve readability.
-var adaptiveTimeout = new AdaptiveTimeout(maxTimeout, new TestLogger(_testOutputHelper, Microsoft.Extensions.Logging.LogLevel.Trace), 10, 1000, 3); +var adaptiveTimeout = new AdaptiveTimeout( + maxTimeout: maxTimeout, + logger: new TestLogger(_testOutputHelper, Microsoft.Extensions.Logging.LogLevel.Trace), + sampleSize: 10, + timeoutCalculationMultiplier: 1000, + minimumSamples: 3);Also applies to: 30-30, 43-43, 57-57
17-25: Consider adding more comprehensive edge case testing.The current tests cover the main scenarios well, but consider adding tests for:
- Null or invalid parameters
- Zero or negative timeouts
- Boundary conditions (exactly minimum samples)
- Concurrent execution scenarios
- Exception handling during execution
Example additional test cases:
[Fact] public void AdaptiveTimeout_Constructor_NullLogger_ThrowsException() { Assert.Throws<ArgumentNullException>(() => new AdaptiveTimeout(TimeSpan.FromSeconds(1), null, 10, 1000, 3)); } [Fact] public void AdaptiveTimeout_Constructor_ZeroTimeout_ThrowsException() { Assert.Throws<ArgumentException>(() => new AdaptiveTimeout(TimeSpan.Zero, new TestLogger(_testOutputHelper, LogLevel.Trace), 10, 1000, 3)); }Also applies to: 28-38, 41-51, 54-67
17-17: Consider more descriptive test method names.The current test names are functional but could be more descriptive about the expected behavior.
Consider renaming for better clarity:
-public async Task AdaptiveTimeout_GetAdaptiveTimeout_NotEnoughSamplesAsync() +public async Task AdaptiveTimeout_ReturnsMaxTimeout_WhenInsufficientSamples() -public async Task AdaptiveTimeout_GetAdaptiveTimeout_AdaptiveDisabled() +public async Task AdaptiveTimeout_ReturnsMaxTimeout_WhenAdaptiveDisabled() -public async Task AdaptiveTimeout_GetAdaptiveTimeout() +public async Task AdaptiveTimeout_ReturnsReducedTimeout_WhenSufficientSamples() -public async Task AdaptiveTimeout_GetAdaptiveTimeout_TimeSpikeSafetyValve() +public async Task AdaptiveTimeout_ReturnsMaxTimeout_WhenSafetyValveTriggered()Also applies to: 28-28, 41-41, 54-54
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/unit/AdaptiveTimeoutTest.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: definitelynotagoblin
PR: SpecterOps/SharpHoundCommon#222
File: src/CommonLib/ExecutionTimeSampler.cs:35-43
Timestamp: 2025-07-11T20:15:16.164Z
Learning: In SharpHoundCommon ExecutionTimeSampler statistical methods, the user prefers to keep simple implementations without thread safety snapshots because precise values aren't needed for analysis purposes - they're comfortable with potentially imprecise values from concurrent access.
test/unit/AdaptiveTimeoutTest.cs (3)
Learnt from: definitelynotagoblin
PR: SpecterOps/SharpHoundCommon#217
File: src/CommonLib/Timeout.cs:17-36
Timestamp: 2025-06-26T16:38:49.677Z
Learning: In SharpHoundCommonLib's Timeout.cs, the team initially had concerns about disposing CancellationTokenSource instances before tasks could check cancellation tokens, but they understand that orphaned tasks (those that exceed timeout) won't be impacted by disposed tokens since their results are already ignored by the timeout handler. They prefer proper resource management with using statements for CancellationTokenSource disposal.
Learnt from: definitelynotagoblin
PR: SpecterOps/SharpHoundCommon#217
File: src/CommonLib/Timeout.cs:21-21
Timestamp: 2025-06-26T16:37:09.005Z
Learning: In SharpHoundCommonLib's Timeout.cs, the team prefers to use TaskScheduler.Current instead of TaskScheduler.Default in Task.Factory.StartNew calls to maintain flexibility for custom schedulers, even though this requires awareness of potential deadlock risks in certain contexts.
Learnt from: definitelynotagoblin
PR: SpecterOps/SharpHoundCommon#222
File: src/CommonLib/ExecutionTimeSampler.cs:35-43
Timestamp: 2025-07-11T20:15:16.164Z
Learning: In SharpHoundCommon ExecutionTimeSampler statistical methods, the user prefers to keep simple implementations without thread safety snapshots because precise values aren't needed for analysis purposes - they're comfortable with potentially imprecise values from concurrent access.
⏰ 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 (2)
test/unit/AdaptiveTimeoutTest.cs (2)
9-14: LGTM! Clean test class structure.The test class follows proper xUnit patterns with constructor injection for test output handling.
21-21: Consider timing precision concerns in tests.The tests rely on
Task.Delayoperations which may not be perfectly precise, potentially leading to flaky tests in CI environments or under load. Consider adding tolerance margins or using more deterministic timing mechanisms if tests become unstable.Monitor these tests for flakiness in CI environments. If timing precision becomes an issue, consider:
- Adding tolerance margins to assertions
- Using deterministic timing mechanisms
- Mocking the timing dependencies
Also applies to: 32-34, 45-47, 59-63
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/CommonLib/LdapConnectionPool.cs (2)
61-64: Consider making timeout configurations customizable.The adaptive timeout initialization is correct, but all instances use identical configuration values. Different LDAP operations might have different performance characteristics that could benefit from tailored configurations.
Consider extracting these configuration values to the
LdapConfigclass to allow customization per operation type:-_queryAdaptiveTimeout = new AdaptiveTimeout(maxTimeout: TimeSpan.FromMinutes(2), Logging.LogProvider.CreateLogger("LdapQuery"), sampleCount: 100, logFrequency: 1000, minSamplesForAdaptiveTimeout: 30); +_queryAdaptiveTimeout = new AdaptiveTimeout( + maxTimeout: config.QueryMaxTimeout ?? TimeSpan.FromMinutes(2), + Logging.LogProvider.CreateLogger("LdapQuery"), + sampleCount: config.QuerySampleCount ?? 100, + logFrequency: config.QueryLogFrequency ?? 1000, + minSamplesForAdaptiveTimeout: config.QueryMinSamples ?? 30);
176-185: Good addition of timeout retry logic.Treating timeouts similarly to busy errors with exponential backoff is a sensible approach. However, the log messages could be more precise.
Consider making the log messages more specific about the timeout nature:
-_log.LogDebug("Query - Timeout: Executing busy backoff for query {Info} (Attempt {Count})", +_log.LogDebug("Query - Timeout occurred, executing exponential backoff for query {Info} (Attempt {Count})",Also applies to: 347-356, 508-517
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/CommonLib/LdapConnectionPool.cs(11 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: definitelynotagoblin
PR: SpecterOps/SharpHoundCommon#222
File: src/CommonLib/ExecutionTimeSampler.cs:35-43
Timestamp: 2025-07-11T20:15:16.164Z
Learning: In SharpHoundCommon ExecutionTimeSampler statistical methods, the user prefers to keep simple implementations without thread safety snapshots because precise values aren't needed for analysis purposes - they're comfortable with potentially imprecise values from concurrent access.
src/CommonLib/LdapConnectionPool.cs (2)
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.
Learnt from: definitelynotagoblin
PR: SpecterOps/SharpHoundCommon#217
File: src/CommonLib/Timeout.cs:21-21
Timestamp: 2025-06-26T16:37:09.005Z
Learning: In SharpHoundCommonLib's Timeout.cs, the team prefers to use TaskScheduler.Current instead of TaskScheduler.Default in Task.Factory.StartNew calls to maintain flexibility for custom schedulers, even though this requires awareness of potential deadlock risks in certain contexts.
🧬 Code Graph Analysis (1)
src/CommonLib/LdapConnectionPool.cs (3)
src/CommonLib/Logging/Logging.cs (2)
Logging(7-20)LogProvider(22-35)src/CommonLib/LdapQueryParameters.cs (1)
GetQueryInfo(43-46)src/CommonLib/Ntlm/LdapNative.cs (1)
LdapConnection(29-253)
⏰ 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 (2)
src/CommonLib/LdapConnectionPool.cs (2)
30-33: LGTM! Well-structured adaptive timeout fields.The four distinct adaptive timeout instances appropriately separate different LDAP operation types, allowing each to have its own timeout behavior based on historical performance.
111-111: Consistent migration to adaptive timeouts.All
SendRequestAsynccalls have been properly replaced withSendRequestWithTimeoutusing the appropriate adaptive timeout instance for each operation type.Also applies to: 276-276, 499-499, 982-982
…current timeout on time spike safety valve event
mykeelium
left a comment
mykeelium
left a comment
There was a problem hiding this comment.
This looks like an awesome change and extremely well done for how large it is! This will be great to get in. A few comment and questions. Since some interfaces changed should this also be a minor version bump?
|
Honestly I've been on the fence about that too. |
…apProcessor constructor
…turning the default int value 0 in DCLdapProcessor tests
…, especially with AdaptiveTimeouts in place now
…ear up some assumptions about our lower bound in TimeSpikeSafetyValve comments
Description
A first step towards execution time analytics.
Adding self-configuring timeouts to existing timeout wrappers.
Motivation and Context
https://specterops.atlassian.net/browse/BED-5972
https://specterops.atlassian.net/browse/BED-6198
How Has This Been Tested?
Unit tests, local lab
Screenshots (if appropriate):
Types of changes
Checklist:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests