fix: GetLocalGroups RPC response time too variant for adaptive timeouts#277
fix: GetLocalGroups RPC response time too variant for adaptive timeouts#277definitelynotagoblin wants to merge 2 commits intov4from
Conversation
…se time to extract much value from adaptive timeout logic, and is causing data loss besides. Going to switch to static timeouts
WalkthroughA timeout mechanism in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/CommonLib/Processors/LocalGroupProcessor.cs (1)
215-232:⚠️ Potential issue | 🟠 Major
IsTimeoutcheck is now dead code with staticTimeout.ExecuteRPCWithTimeout.The
Timeout.ExecuteRPCWithTimeoutmethod (per context snippet 2) returnsSharpHoundRPC.Result<T>.Fail(result.Error)on timeout, which only sets theErrorstring property. TheIsTimeoutproperty is never set totrueby this code path—Result<T>.Fail(string)only initializesError.This means the check at lines 229-231 will never trigger, making it dead code:
if (getMembersResult.IsTimeout) { yield break; }Either:
- Remove the dead
IsTimeoutcheck since early termination on this specific timeout may not be desired (given the PR's goal to avoid data loss from premature timeouts), or- Update
Timeout.ExecuteRPCWithTimeoutto setIsTimeout = truewhen the failure is due to timeout.Option 1: Remove the dead code
ret.Collected = false; ret.FailureReason = $"SamGetMembersInAlias failed with status {getMembersResult.SError}"; yield return ret; - if (getMembersResult.IsTimeout) { - yield break; - } continue;Option 2: Fix Timeout.ExecuteRPCWithTimeout to set IsTimeout
In
src/CommonLib/Timeout.cs, around line 156-162:public static async Task<SharpHoundRPC.Result<T>> ExecuteRPCWithTimeout<T>(TimeSpan timeout, Func<CancellationToken, SharpHoundRPC.Result<T>> func, CancellationToken parentToken = default) { var result = await ExecuteWithTimeout(timeout, func, parentToken); if (result.IsSuccess) return result.Value; - else - return SharpHoundRPC.Result<T>.Fail(result.Error); + else { + var failResult = SharpHoundRPC.Result<T>.Fail(result.Error); + failResult.IsTimeout = result.Error?.Contains("Timeout after") == true; + return failResult; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CommonLib/Processors/LocalGroupProcessor.cs` around lines 215 - 232, The IsTimeout branch is dead because Timeout.ExecuteRPCWithTimeout returns a failed Result without setting IsTimeout; remove the unreachable check and its yield break to avoid misleading code: delete the `if (getMembersResult.IsTimeout) { yield break; }` block in LocalGroupProcessor where `getMembersResult` is handled (inside the GetMembers RPC handling after Timeout.ExecuteRPCWithTimeout) and ensure the surrounding logic still continues/returns as intended (ret.Collected=false, FailureReason set, yield return ret, continue).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/CommonLib/Processors/LocalGroupProcessor.cs`:
- Around line 215-232: The IsTimeout branch is dead because
Timeout.ExecuteRPCWithTimeout returns a failed Result without setting IsTimeout;
remove the unreachable check and its yield break to avoid misleading code:
delete the `if (getMembersResult.IsTimeout) { yield break; }` block in
LocalGroupProcessor where `getMembersResult` is handled (inside the GetMembers
RPC handling after Timeout.ExecuteRPCWithTimeout) and ensure the surrounding
logic still continues/returns as intended (ret.Collected=false, FailureReason
set, yield return ret, continue).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0faced32-2219-43c0-9e97-5b8ddfe88005
📒 Files selected for processing (1)
src/CommonLib/Processors/LocalGroupProcessor.cs
1 participant
Description
GetLocalGroups for RPC is too inconsistent in payload and response time to extract much value from adaptive timeout logic, and is causing data loss besides. Switching to static timeouts.
Motivation and Context
https://specterops.atlassian.net/browse/BED-7153
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist:
Summary by CodeRabbit