Repository navigation
Conversation
WalkthroughRefactors IRegistryKey.Connect to remove an explicit else branch and adds AdaptiveTimeout usage (2-minute max) to RegistryProcessor and WebClientServiceProcessor, replacing direct calls with ExecuteWithTimeout and handling timeout/failure results. Changes
Sequence Diagram(s)sequenceDiagram
participant Proc as Processor
participant AT as AdaptiveTimeout
participant Op as Operation
Proc->>AT: ExecuteWithTimeout(Op, max=2m)
AT->>Op: Run operation (bounded)
alt completes within 2 minutes
Op-->>AT: APIResult.Success(value)
AT-->>Proc: APIResult.Success(value)
Proc->>Proc: Use result.Value
else times out or fails
Op--xAT: Timeout / failure
AT-->>Proc: APIResult.Failure (timeout or error)
Proc->>Proc: Return Failure / throw TimeoutException
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-06-26T16:38:49.677ZApplied to files:
🧬 Code graph analysis (1)src/CommonLib/Processors/WebClientServiceProcessor.cs (2)
⏰ 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)
🔇 Additional comments (2)
Tip 📝 Customizable high-level summaries are now available!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting. 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 (2)
src/CommonLib/Processors/WebClientServiceProcessor.cs (1)
82-96: Timeout handling in IsWebClientRunning is correct; consider surfacing error detailsThe timeout wrapper around
TestPathExistscorrectly bounds the pipe check and returns the originalAPIResult<bool>on success, throwing on timeout. To aid diagnosis and align withSHRegistryKey.Connect, you could include the underlying timeout error in the exception message:- throw new TimeoutException($"Failed to check pipe on {computerName}: {pipePath}"); + throw new TimeoutException( + $"Failed to check pipe on {computerName}: {pipePath}. Error: {result.Error}");This keeps behavior the same while exposing more context when a timeout occurs.
src/CommonLib/Processors/RegistryProcessor.cs (1)
58-67: Registry collection timeout behavior is good; optionally include underlying errorWrapping
StrategyExecutor.CollectAsyncin_registryAdaptiveTimeout.ExecuteWithTimeoutand returning a failedAPIResult<RegistryData>on!result.IsSuccesscorrectly prevents hangs and surfaces a timeout condition to callers.If
ExecuteWithTimeoutprovides a descriptiveError, you might optionally add it to the failure message for easier troubleshooting:- if (!result.IsSuccess) { - return APIResult<RegistryData>.Failure($"Timeout when grabbing registry data from {targetMachine}"); - } + if (!result.IsSuccess) { + return APIResult<RegistryData>.Failure( + $"Timeout when grabbing registry data from {targetMachine}: {result.Error}"); + }This keeps the contract intact while exposing more detail about what went wrong.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/CommonLib/IRegistryKey.cs(1 hunks)src/CommonLib/Processors/RegistryProcessor.cs(2 hunks)src/CommonLib/Processors/WebClientServiceProcessor.cs(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: definitelynotagoblin
Repo: SpecterOps/SharpHoundCommon PR: 222
File: src/CommonLib/Processors/LocalGroupProcessor.cs:19-27
Timestamp: 2025-07-15T17:45:25.688Z
Learning: In SharpHoundCommon, the team prefers to keep code simple rather than implement perfect resource management when the resources being managed are non-critical. Specifically, they accept not implementing IDisposable for AdaptiveTimeout instances when the Dispose method is primarily for flushing analytics logs from ExecutionTimeSampler, viewing it as a courtesy rather than a safety requirement.
🧬 Code graph analysis (2)
src/CommonLib/Processors/RegistryProcessor.cs (1)
src/CommonLib/Logging/Logging.cs (2)
Logging(7-20)LogProvider(22-35)
src/CommonLib/Processors/WebClientServiceProcessor.cs (2)
src/CommonLib/Logging/Logging.cs (2)
Logging(7-20)LogProvider(22-35)src/CommonLib/OutputTypes/APIResult.cs (4)
APIResult(2-5)APIResult(7-28)APIResult(10-15)APIResult(17-22)
⏰ 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 (3)
src/CommonLib/IRegistryKey.cs (1)
39-43: Connect control flow simplification is fineSwitching to an early return plus a trailing
TimeoutExceptionkeeps behavior unchanged and improves readability. No issues with the timeout handling here.src/CommonLib/Processors/WebClientServiceProcessor.cs (1)
18-20: AdaptiveTimeout wiring for CreateFile looks consistentUsing a per-instance
AdaptiveTimeoutwith a 2‑minute cap and a dedicated logger forIsWebClientRunningmatches the pattern elsewhere in the codebase and is fine as a long‑lived field. Based on learnings.src/CommonLib/Processors/RegistryProcessor.cs (1)
18-18: Registry AdaptiveTimeout initialization matches established patternIntroducing
_registryAdaptiveTimeoutwith a 2‑minute max timeout and a dedicated logger forReadRegistrySettingsaligns with the new timeout strategy and looks good. No extra disposal complexity needed here. Based on learnings.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/CommonLib/Processors/WebClientServiceProcessor.cs(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: definitelynotagoblin
Repo: SpecterOps/SharpHoundCommon PR: 222
File: src/CommonLib/Processors/LocalGroupProcessor.cs:19-27
Timestamp: 2025-07-15T17:45:25.688Z
Learning: In SharpHoundCommon, the team prefers to keep code simple rather than implement perfect resource management when the resources being managed are non-critical. Specifically, they accept not implementing IDisposable for AdaptiveTimeout instances when the Dispose method is primarily for flushing analytics logs from ExecutionTimeSampler, viewing it as a courtesy rather than a safety requirement.
🧬 Code graph analysis (1)
src/CommonLib/Processors/WebClientServiceProcessor.cs (1)
src/CommonLib/OutputTypes/APIResult.cs (4)
APIResult(2-5)APIResult(7-28)APIResult(10-15)APIResult(17-22)
⏰ 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/Processors/WebClientServiceProcessor.cs (1)
19-19: LGTM! Field initialization is appropriate.The 2-minute timeout is reasonable for network-based operations. Based on learnings, the team accepts not implementing disposal for AdaptiveTimeout instances.
2 participants
Description
The check for IsWebClientRunning was not wrapped in a timeout. Additionally, our registry strategy executor was not wrapped in timeouts either. Both of these are used during SHE local group collection and could potentially lead to major hangs.
Motivation and Context
https://specterops.atlassian.net/browse/BED-6829
How Has This Been Tested?
Unable to reproduce, so adding these as a precaution.
Screenshots (if appropriate):
Types of changes
Checklist:
Summary by CodeRabbit
Refactor
Bug Fixes