feat: Apply blanket minimum timeout bound to 1s - BED-7153

[definitelynotagoblin]

Description

Add minimum timeout floor for adaptive timeouts, apply a 1 second blanket floor to all current uses, with the option being configurable.

Motivation and Context

https://specterops.atlassian.net/browse/BED-7153

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

• Chore (a change that does not modify the application functionality)
• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• Documentation updates are needed, and have been made accordingly.
• I have added and/or updated tests to cover my changes.
• All new and existing tests passed.
• My changes include a database migration.

Summary by CodeRabbit

• New Features

  • Added configurable minimum timeout enforcement to prevent excessively short timeout values (default: 1 second).
  • Enhanced timeout management with new configuration options for custom minimum timeout thresholds.

• Improvements

  • Improved reliability by enforcing lower bounds on adaptive timeout calculations, ensuring more predictable system behavior.

[coderabbitai]

Walkthrough

The changes introduce a minimum timeout feature to the AdaptiveTimeout class by adding a configurable lower bound parameter. A new constructor overload enables callers to specify a minimum timeout, and the GetAdaptiveTimeout method now clamps computed values to respect this minimum. One usage site explicitly disables adaptive timeouts for the GetMembers operation.

Changes

Cohort / File(s)	Summary
AdaptiveTimeout core implementation src/CommonLib/AdaptiveTimeout.cs	Added _minTimeout field with default of 1 second. New constructor overload accepts minTimeout parameter with validation (non-negative, strictly less than maxTimeout). Modified GetAdaptiveTimeout() to clamp result to minimum using Math.Max(). Minor comment text updates (96% → 98%, 98+% → 99+%).
LocalGroupProcessor timeout configuration src/CommonLib/Processors/LocalGroupProcessor.cs	Changed GetMembers adaptive timeout initialization to explicitly disable adaptive timeouts with useAdaptiveTimeout: false and added clarifying comment.
AdaptiveTimeout test updates test/unit/AdaptiveTimeoutTest.cs	Updated all AdaptiveTimeout constructor calls to include new minTimeout parameter as second argument. Added new test AdaptiveTimeout_GetAdaptiveTimeout_MinTimeout() to validate minimum timeout enforcement.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 A timeout so brave, with bounds now defined,
No shorter than one second—a floor we shall find!
Math.Max keeps us honest, no negative creep,
The rabbit hops faster with guarantees deep! ⏱️✨

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check	❓ Inconclusive	PR description covers motivation with issue link, but testing details are missing and tests checkbox is unchecked despite author claiming tests passed.	Provide details under 'How Has This Been Tested?' section describing test scenarios, including the new minimum timeout test case. Clarify whether the 'tests' checkbox should be checked.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The PR title clearly and specifically summarizes the main change: introducing a 1-second minimum timeout bound to adaptive timeouts, which directly corresponds to the core modifications across AdaptiveTimeout.cs, LocalGroupProcessor.cs, and the accompanying tests.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch anemeth/min-timeout

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (2)

test/unit/AdaptiveTimeoutTest.cs (2)

145-156: New minimum timeout test validates the clamping behavior.

The test correctly verifies that when the computed adaptive timeout (based on a fast 50ms sample) falls below the configured minTimeout, the result is clamped to minTimeout.

One minor observation: the assertion on line 155 (Assert.True(adaptiveTimeoutResult < maxTimeout)) is redundant given line 154 already asserts equality with minTimeout (which is less than maxTimeout by constructor constraint). Consider removing it or keeping it as documentation of intent.

🔧 Optional: Remove redundant assertion

         var adaptiveTimeoutResult = adaptiveTimeout.GetAdaptiveTimeout();
         Assert.Equal(minTimeout, adaptiveTimeoutResult);
-        Assert.True(adaptiveTimeoutResult < maxTimeout);
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/unit/AdaptiveTimeoutTest.cs` around lines 145 - 156, Remove the
redundant assertion in the AdaptiveTimeout_GetAdaptiveTimeout_MinTimeout test:
delete the Assert.True(adaptiveTimeoutResult < maxTimeout) check (it duplicates
the intent already proven by Assert.Equal(minTimeout, adaptiveTimeoutResult)).
Locate the test method named AdaptiveTimeout_GetAdaptiveTimeout_MinTimeout and
remove the extra Assert.True call so the test only asserts equality to
minTimeout.

---

145-156: Consider adding validation tests for constructor edge cases.

The new feature includes constructor validation that throws ArgumentException for invalid minTimeout values. Consider adding tests to verify these guard clauses:

• minTimeout < TimeSpan.Zero → should throw
• minTimeout >= maxTimeout → should throw

[Fact]
public void AdaptiveTimeout_Constructor_ThrowsOnNegativeMinTimeout() {
    Assert.Throws<ArgumentException>(() => 
        new AdaptiveTimeout(TimeSpan.FromSeconds(1), TimeSpan.FromSeconds(-1), 
            new TestLogger(_testOutputHelper, Microsoft.Extensions.Logging.LogLevel.Trace), 10, 1000, 3));
}

[Fact]
public void AdaptiveTimeout_Constructor_ThrowsWhenMinTimeoutExceedsMax() {
    Assert.Throws<ArgumentException>(() => 
        new AdaptiveTimeout(TimeSpan.FromSeconds(1), TimeSpan.FromSeconds(2), 
            new TestLogger(_testOutputHelper, Microsoft.Extensions.Logging.LogLevel.Trace), 10, 1000, 3));
}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/unit/AdaptiveTimeoutTest.cs` around lines 145 - 156, Add unit tests that
validate the AdaptiveTimeout constructor guard clauses: create tests (e.g.,
AdaptiveTimeout_Constructor_ThrowsOnNegativeMinTimeout and
AdaptiveTimeout_Constructor_ThrowsWhenMinTimeoutExceedsMax) that call new
AdaptiveTimeout(...) and assert ArgumentException is thrown when minTimeout <
TimeSpan.Zero and when minTimeout >= maxTimeout respectively; use the same
TestLogger and similar constructor args as other tests to locate the
AdaptiveTimeout constructor validation code.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@test/unit/AdaptiveTimeoutTest.cs`:
- Around line 145-156: Remove the redundant assertion in the
AdaptiveTimeout_GetAdaptiveTimeout_MinTimeout test: delete the
Assert.True(adaptiveTimeoutResult < maxTimeout) check (it duplicates the intent
already proven by Assert.Equal(minTimeout, adaptiveTimeoutResult)). Locate the
test method named AdaptiveTimeout_GetAdaptiveTimeout_MinTimeout and remove the
extra Assert.True call so the test only asserts equality to minTimeout.
- Around line 145-156: Add unit tests that validate the AdaptiveTimeout
constructor guard clauses: create tests (e.g.,
AdaptiveTimeout_Constructor_ThrowsOnNegativeMinTimeout and
AdaptiveTimeout_Constructor_ThrowsWhenMinTimeoutExceedsMax) that call new
AdaptiveTimeout(...) and assert ArgumentException is thrown when minTimeout <
TimeSpan.Zero and when minTimeout >= maxTimeout respectively; use the same
TestLogger and similar constructor args as other tests to locate the
AdaptiveTimeout constructor validation code.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 537ed8ea-bc7a-4643-8614-fc160efcdb24

📥 Commits

Reviewing files that changed from the base of the PR and between f086525 and f16dc45.

📒 Files selected for processing (3)

• src/CommonLib/AdaptiveTimeout.cs
• src/CommonLib/Processors/LocalGroupProcessor.cs
• test/unit/AdaptiveTimeoutTest.cs