Skip to content

fix: address code review feedback for maxUtilizationPercent#2

Merged
gwyntel-git merged 1 commit intomainfrom
review/max-utilization-percent-review-fixes
Apr 19, 2026
Merged

fix: address code review feedback for maxUtilizationPercent#2
gwyntel-git merged 1 commit intomainfrom
review/max-utilization-percent-review-fixes

Conversation

@gwyntel-git
Copy link
Copy Markdown

Summary

Addresses code review feedback from PR #1 for the maxUtilizationPercent feature.

Fixes

  1. CRITICAL: Multi-checker cooldown clearing bug — A lenient checker (threshold=99) could clear a cooldown set by a strict checker (threshold=30). Fixed by only allowing the strictest checker for a provider to clear the provider-wide cooldown. Others are blocked with a debug log.

  2. HIGH: min(0)min(1) in Zod schemamaxUtilizationPercent=0 was a footgun (always triggers permanent cooldown). Use enabled: false to fully disable a provider instead.

  3. HIGH: Refactor exhaustionThreshold from unsafe cast to getter — Added exhaustionThreshold getter to QuotaChecker base class (default 99). SyntheticQuotaChecker overrides it via getOption('maxUtilizationPercent', 99). Scheduler reads checker.exhaustionThreshold — type-safe, no as number | undefined casts.

  4. MEDIUM: Update stale ≥99% comment in cooldown-manager.ts to reflect configurable threshold.

  5. MEDIUM: Add 3 new test cases:

    • Cooldown clearing when utilization drops below threshold
    • Multiple windows where only one exceeds threshold
    • Lenient checker cannot clear strict checker's cooldown (the bug fix)

Test Results

  • All 1866 backend tests pass (6 new tests added)
  • Pre-commit hooks pass (Biome format + full suite)

Based on subagent code review, fixes:

1. CRITICAL: Multi-checker cooldown clearing bug — a lenient checker
   (threshold=99) could clear a cooldown set by a strict checker
   (threshold=30). Fixed by only allowing the strictest checker for
   a provider to clear the provider-wide cooldown. Others are blocked.

2. HIGH: min(0) → min(1) in Zod schema. maxUtilizationPercent=0 was
   a footgun (always triggers cooldown). Use enabled:false instead.

3. HIGH: Refactor exhaustionThreshold from unsafe options cast to a
   proper getter on QuotaChecker base class. SyntheticQuotaChecker
   overrides it via getOption('maxUtilizationPercent', 99). Scheduler
   reads checker.exhaustionThreshold — type-safe, no casts.

4. MEDIUM: Update stale '≥99%' comment in cooldown-manager.ts.

5. MEDIUM: Add 3 new test cases:
   - Cooldown clearing when utilization drops below threshold
   - Multiple windows where only one exceeds threshold
   - Lenient checker cannot clear strict checker's cooldown
@gwyntel-git gwyntel-git merged commit 9d65cd6 into main Apr 19, 2026
@gwyntel-git gwyntel-git deleted the review/max-utilization-percent-review-fixes branch April 19, 2026 04:47
gwyntel-git added a commit that referenced this pull request Apr 19, 2026
Based on subagent code review, fixes:

1. CRITICAL: Multi-checker cooldown clearing bug — a lenient checker
   (threshold=99) could clear a cooldown set by a strict checker
   (threshold=30). Fixed by only allowing the strictest checker for
   a provider to clear the provider-wide cooldown. Others are blocked.

2. HIGH: min(0) → min(1) in Zod schema. maxUtilizationPercent=0 was
   a footgun (always triggers cooldown). Use enabled:false instead.

3. HIGH: Refactor exhaustionThreshold from unsafe options cast to a
   proper getter on QuotaChecker base class. SyntheticQuotaChecker
   overrides it via getOption('maxUtilizationPercent', 99). Scheduler
   reads checker.exhaustionThreshold — type-safe, no casts.

4. MEDIUM: Update stale '≥99%' comment in cooldown-manager.ts.

5. MEDIUM: Add 3 new test cases:
   - Cooldown clearing when utilization drops below threshold
   - Multiple windows where only one exceeds threshold
   - Lenient checker cannot clear strict checker's cooldown

Co-authored-by: Gwyn <gwyn@gwyn.tel>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant