fix(limits): retry_after ceiling capped at window — deterministic on coarse timers (#346) - #351

Merged
AndresL230 merged 2 commits into
mainfrom
fix/346-retry-after-ceiling
Jul 29, 2026
Merged

fix(limits): retry_after ceiling capped at window — deterministic on coarse timers (#346)#351
AndresL230 merged 2 commits into
mainfrom
fix/346-retry-after-ceiling

Conversation

@Darkest-Teddy

@Darkest-TeddyDarkest-Teddy commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

Problem

check_rate_limit computes the retry hint as int(window - elapsed) + 1 — a broken ceiling. When the rate-limited call shares an identical time.time() value with the first call in the window, elapsed == 0.0 exactly and the hint overshoots to window + 1 (61s for a 60s window). On Windows this happens reliably (the timer ticks every ~15.6 ms, so a tight loop of 6 calls lands on one tick), which:

  • violates the (0, window] contract callers surface as Retry-After, and
  • makes TestRateLimit::test_sixth_call_returns_retry_after fail out-of-the-box for every Windows contributor (assert 61 <= 60). Linux CI passes only by accident — elapsed there is tiny-but-nonzero, so int(59.999…) + 1 == 60.

Fix

True ceiling semantics, clamped at the window, in both copies of the limiter:

retry=min(window_sec, math.ceil(window_sec- (now-bucket[0])))
  • services/request_limits.py (shared limiter behind OCR extraction + Gradescope routes)
  • services/flashcard_import_service.py (older twin the shared module's docstring notes should eventually migrate)

The clamp also guards the pathological elapsed < 0 case (clock steps backward between calls).

Tests (TDD — written first, verified failing)

  • test_retry_capped_at_window_on_coincident_timestamps (both limiters): freezes time.time so elapsed == 0.0 exactly — deterministic reproduction of the Windows failure on every platform; pins retry == 60. Failed (61) before the fix, passes after.
  • test_retry_uses_ceiling_of_remaining_window (both limiters): 0.5s remaining → retry == 1, pinning ceiling semantics against a regression to plain truncation.
  • New tests/test_request_limits.py also covers the shared limiter's base contract (allow-up-to-limit, over-limit hint bounds, key isolation, window reset), which previously had no direct unit tests.

Verification

  • Affected suites (test_flashcard_import_service, test_request_limits, test_extract_auth_bounds, test_gradescope): 72/72 passed — on the Windows machine where the suite was previously red.
  • ruff check clean on all touched files.

Supersedes #347 (same clamp idea, but closed without regression tests; this PR adds the deterministic frozen-time coverage the issue asked for).

Fixes#346

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Improved rate-limit retry guidance so displayed wait times remain within the configured limit window.
    • Corrected rounding for fractional remaining wait times, providing more accurate retry estimates.
    • Ensured edge cases, including coincident timestamps and near-expired windows, return valid retry values.
  • Tests

    • Added coverage for rate-limit boundaries, timing behavior, key isolation, and window resets.

@coderabbitai

coderabbitaiBot commented Jul 17, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@AndresL230, you've reached your PR review limit, so we couldn't start this review.

Next review available in:35 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 0b822bd9-4b81-4c35-85d6-9dd7b4d0d7ea

📥 Commits

Reviewing files that changed from the base of the PR and between b0716af and 6731cd7.

📒 Files selected for processing (3)
  • backend/services/request_limits.py
  • backend/tests/test_flashcard_import_service.py
  • backend/tests/test_request_limits.py
📝 Walkthrough

Walkthrough

The rate limiters now calculate retry delays with ceiling semantics and cap them at the configured window. Tests cover coincident timestamps, near-window expiry, reset behavior, limit enforcement, and per-key isolation.

Changes

Rate-limit retry behavior

Layer / File(s)Summary
Bounded retry calculations
backend/services/flashcard_import_service.py, backend/services/request_limits.py
Retry delays use ceiling-based remaining-window calculations capped at the configured rate-limit window.
Retry boundary coverage
backend/tests/test_flashcard_import_service.py, backend/tests/test_request_limits.py
Tests validate bounded retry hints, coincident timestamps, near-window expiry, window resets, request limits, and per-key isolation.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested reviewers:jose-gael-cruz-lopez

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check nameStatusExplanationResolution
Docstring Coverage⚠️ WarningDocstring 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.
✅ Passed checks (4 passed)
Check nameStatusExplanation
Title check✅ PassedThe title clearly summarizes the retry_after ceiling fix and its deterministic timer behavior.
Description check✅ PassedThe description covers the problem, fix, testing, and issue reference, which is enough despite different headings.
Linked Issues check✅ PassedThe PR implements the off-by-one retry_after cap and adds frozen-time regression tests requested by #346.
Out of Scope Changes check✅ PassedAll changes are directly related to the rate-limit retry fix and its regression coverage.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/346-retry-after-ceiling

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

Comment @coderabbitai help to get the list of available commands.

@cloudflare-workers-and-pages

cloudflare-workers-and-pagesBot commented Jul 17, 2026

Copy link
Copy Markdown

Deploying with Cloudflare Workers Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

StatusNameLatest CommitPreview URLUpdated (UTC)
✅ Deployment successful!
View logs
frontend-staging6731cd7Commit Preview URL

Branch Preview URL
Jul 29 2026, 09:10 AM

…coarse timers (#346)
Rebased onto current main: main had already adopted math.ceil in the
flashcard limiter; this keeps that and adds the min(window, ...) cap to
BOTH sliding-window limiters (services/request_limits.py still had the
old int(...) + 1 overshoot), plus regression tests for coincident
timestamps and sub-second remainders.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@AndresL230
AndresL230force-pushed the fix/346-retry-after-ceiling branch from a0cf1f9 to b0716afCompareJuly 29, 2026 08:43
@AndresL230
AndresL230 marked this pull request as ready for review July 29, 2026 08:44
… align twin comments
Review follow-ups: request_limits.py's comment now names the negative-
elapsed (NTP step) case the cap protects against, matching its twin; a
regression test in both limiter test files freezes time backward so a
future revert of the cap fails loudly.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@AndresL230
AndresL230 merged commit 5d5130b into mainJul 29, 2026
6 checks passed
@AndresL230
AndresL230 deleted the fix/346-retry-after-ceiling branch August 2, 2026 18:30
Sign up for freeto 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.

check_rate_limit retry_after off-by-one makes TestRateLimit flaky on Windows

2 participants

@Darkest-Teddy@AndresL230
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

fix(limits): retry_after ceiling capped at window — deterministic on coarse timers (#346) - #351

Merged
AndresL230 merged 2 commits into
mainfrom
fix/346-retry-after-ceiling
Jul 29, 2026
Merged

fix(limits): retry_after ceiling capped at window — deterministic on coarse timers (#346)#351
AndresL230 merged 2 commits into
mainfrom
fix/346-retry-after-ceiling

Conversation

@Darkest-Teddy

@Darkest-TeddyDarkest-Teddy commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

Problem

check_rate_limit computes the retry hint as int(window - elapsed) + 1 — a broken ceiling. When the rate-limited call shares an identical time.time() value with the first call in the window, elapsed == 0.0 exactly and the hint overshoots to window + 1 (61s for a 60s window). On Windows this happens reliably (the timer ticks every ~15.6 ms, so a tight loop of 6 calls lands on one tick), which:

  • violates the (0, window] contract callers surface as Retry-After, and
  • makes TestRateLimit::test_sixth_call_returns_retry_after fail out-of-the-box for every Windows contributor (assert 61 <= 60). Linux CI passes only by accident — elapsed there is tiny-but-nonzero, so int(59.999…) + 1 == 60.

Fix

True ceiling semantics, clamped at the window, in both copies of the limiter:

retry=min(window_sec, math.ceil(window_sec- (now-bucket[0])))
  • services/request_limits.py (shared limiter behind OCR extraction + Gradescope routes)
  • services/flashcard_import_service.py (older twin the shared module's docstring notes should eventually migrate)

The clamp also guards the pathological elapsed < 0 case (clock steps backward between calls).

Tests (TDD — written first, verified failing)

  • test_retry_capped_at_window_on_coincident_timestamps (both limiters): freezes time.time so elapsed == 0.0 exactly — deterministic reproduction of the Windows failure on every platform; pins retry == 60. Failed (61) before the fix, passes after.
  • test_retry_uses_ceiling_of_remaining_window (both limiters): 0.5s remaining → retry == 1, pinning ceiling semantics against a regression to plain truncation.
  • New tests/test_request_limits.py also covers the shared limiter's base contract (allow-up-to-limit, over-limit hint bounds, key isolation, window reset), which previously had no direct unit tests.

Verification

  • Affected suites (test_flashcard_import_service, test_request_limits, test_extract_auth_bounds, test_gradescope): 72/72 passed — on the Windows machine where the suite was previously red.
  • ruff check clean on all touched files.

Supersedes #347 (same clamp idea, but closed without regression tests; this PR adds the deterministic frozen-time coverage the issue asked for).

Fixes#346

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Improved rate-limit retry guidance so displayed wait times remain within the configured limit window.
    • Corrected rounding for fractional remaining wait times, providing more accurate retry estimates.
    • Ensured edge cases, including coincident timestamps and near-expired windows, return valid retry values.
  • Tests

    • Added coverage for rate-limit boundaries, timing behavior, key isolation, and window resets.

@coderabbitai

coderabbitaiBot commented Jul 17, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@AndresL230, you've reached your PR review limit, so we couldn't start this review.

Next review available in:35 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 0b822bd9-4b81-4c35-85d6-9dd7b4d0d7ea

📥 Commits

Reviewing files that changed from the base of the PR and between b0716af and 6731cd7.

📒 Files selected for processing (3)
  • backend/services/request_limits.py
  • backend/tests/test_flashcard_import_service.py
  • backend/tests/test_request_limits.py
📝 Walkthrough

Walkthrough

The rate limiters now calculate retry delays with ceiling semantics and cap them at the configured window. Tests cover coincident timestamps, near-window expiry, reset behavior, limit enforcement, and per-key isolation.

Changes

Rate-limit retry behavior

Layer / File(s)Summary
Bounded retry calculations
backend/services/flashcard_import_service.py, backend/services/request_limits.py
Retry delays use ceiling-based remaining-window calculations capped at the configured rate-limit window.
Retry boundary coverage
backend/tests/test_flashcard_import_service.py, backend/tests/test_request_limits.py
Tests validate bounded retry hints, coincident timestamps, near-window expiry, window resets, request limits, and per-key isolation.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested reviewers:jose-gael-cruz-lopez

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check nameStatusExplanationResolution
Docstring Coverage⚠️ WarningDocstring 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.
✅ Passed checks (4 passed)
Check nameStatusExplanation
Title check✅ PassedThe title clearly summarizes the retry_after ceiling fix and its deterministic timer behavior.
Description check✅ PassedThe description covers the problem, fix, testing, and issue reference, which is enough despite different headings.
Linked Issues check✅ PassedThe PR implements the off-by-one retry_after cap and adds frozen-time regression tests requested by #346.
Out of Scope Changes check✅ PassedAll changes are directly related to the rate-limit retry fix and its regression coverage.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/346-retry-after-ceiling

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

Comment @coderabbitai help to get the list of available commands.

@cloudflare-workers-and-pages

cloudflare-workers-and-pagesBot commented Jul 17, 2026

Copy link
Copy Markdown

Deploying with Cloudflare Workers Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

StatusNameLatest CommitPreview URLUpdated (UTC)
✅ Deployment successful!
View logs
frontend-staging6731cd7Commit Preview URL

Branch Preview URL
Jul 29 2026, 09:10 AM

…coarse timers (#346)
Rebased onto current main: main had already adopted math.ceil in the
flashcard limiter; this keeps that and adds the min(window, ...) cap to
BOTH sliding-window limiters (services/request_limits.py still had the
old int(...) + 1 overshoot), plus regression tests for coincident
timestamps and sub-second remainders.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@AndresL230
AndresL230force-pushed the fix/346-retry-after-ceiling branch from a0cf1f9 to b0716afCompareJuly 29, 2026 08:43
@AndresL230
AndresL230 marked this pull request as ready for review July 29, 2026 08:44
… align twin comments
Review follow-ups: request_limits.py's comment now names the negative-
elapsed (NTP step) case the cap protects against, matching its twin; a
regression test in both limiter test files freezes time backward so a
future revert of the cap fails loudly.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@AndresL230
AndresL230 merged commit 5d5130b into mainJul 29, 2026
6 checks passed
@AndresL230
AndresL230 deleted the fix/346-retry-after-ceiling branch August 2, 2026 18:30
Sign up for freeto 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.

check_rate_limit retry_after off-by-one makes TestRateLimit flaky on Windows

2 participants

@Darkest-Teddy@AndresL230
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(limits): retry_after ceiling capped at window — deterministic on coarse timers (#346) - #351

Merged
AndresL230 merged 2 commits into
mainfrom
fix/346-retry-after-ceiling
Jul 29, 2026
Merged

fix(limits): retry_after ceiling capped at window — deterministic on coarse timers (#346)#351
AndresL230 merged 2 commits into
mainfrom
fix/346-retry-after-ceiling

Conversation

@Darkest-Teddy

@Darkest-TeddyDarkest-Teddy commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

Problem

check_rate_limit computes the retry hint as int(window - elapsed) + 1 — a broken ceiling. When the rate-limited call shares an identical time.time() value with the first call in the window, elapsed == 0.0 exactly and the hint overshoots to window + 1 (61s for a 60s window). On Windows this happens reliably (the timer ticks every ~15.6 ms, so a tight loop of 6 calls lands on one tick), which:

  • violates the (0, window] contract callers surface as Retry-After, and
  • makes TestRateLimit::test_sixth_call_returns_retry_after fail out-of-the-box for every Windows contributor (assert 61 <= 60). Linux CI passes only by accident — elapsed there is tiny-but-nonzero, so int(59.999…) + 1 == 60.

Fix

True ceiling semantics, clamped at the window, in both copies of the limiter:

retry=min(window_sec, math.ceil(window_sec- (now-bucket[0])))
  • services/request_limits.py (shared limiter behind OCR extraction + Gradescope routes)
  • services/flashcard_import_service.py (older twin the shared module's docstring notes should eventually migrate)

The clamp also guards the pathological elapsed < 0 case (clock steps backward between calls).

Tests (TDD — written first, verified failing)

  • test_retry_capped_at_window_on_coincident_timestamps (both limiters): freezes time.time so elapsed == 0.0 exactly — deterministic reproduction of the Windows failure on every platform; pins retry == 60. Failed (61) before the fix, passes after.
  • test_retry_uses_ceiling_of_remaining_window (both limiters): 0.5s remaining → retry == 1, pinning ceiling semantics against a regression to plain truncation.
  • New tests/test_request_limits.py also covers the shared limiter's base contract (allow-up-to-limit, over-limit hint bounds, key isolation, window reset), which previously had no direct unit tests.

Verification

  • Affected suites (test_flashcard_import_service, test_request_limits, test_extract_auth_bounds, test_gradescope): 72/72 passed — on the Windows machine where the suite was previously red.
  • ruff check clean on all touched files.

Supersedes #347 (same clamp idea, but closed without regression tests; this PR adds the deterministic frozen-time coverage the issue asked for).

Fixes#346

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Improved rate-limit retry guidance so displayed wait times remain within the configured limit window.
    • Corrected rounding for fractional remaining wait times, providing more accurate retry estimates.
    • Ensured edge cases, including coincident timestamps and near-expired windows, return valid retry values.
  • Tests

    • Added coverage for rate-limit boundaries, timing behavior, key isolation, and window resets.

@coderabbitai

coderabbitaiBot commented Jul 17, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@AndresL230, you've reached your PR review limit, so we couldn't start this review.

Next review available in:35 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 0b822bd9-4b81-4c35-85d6-9dd7b4d0d7ea

📥 Commits

Reviewing files that changed from the base of the PR and between b0716af and 6731cd7.

📒 Files selected for processing (3)
  • backend/services/request_limits.py
  • backend/tests/test_flashcard_import_service.py
  • backend/tests/test_request_limits.py
📝 Walkthrough

Walkthrough

The rate limiters now calculate retry delays with ceiling semantics and cap them at the configured window. Tests cover coincident timestamps, near-window expiry, reset behavior, limit enforcement, and per-key isolation.

Changes

Rate-limit retry behavior

Layer / File(s)Summary
Bounded retry calculations
backend/services/flashcard_import_service.py, backend/services/request_limits.py
Retry delays use ceiling-based remaining-window calculations capped at the configured rate-limit window.
Retry boundary coverage
backend/tests/test_flashcard_import_service.py, backend/tests/test_request_limits.py
Tests validate bounded retry hints, coincident timestamps, near-window expiry, window resets, request limits, and per-key isolation.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested reviewers:jose-gael-cruz-lopez

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check nameStatusExplanationResolution
Docstring Coverage⚠️ WarningDocstring 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.
✅ Passed checks (4 passed)
Check nameStatusExplanation
Title check✅ PassedThe title clearly summarizes the retry_after ceiling fix and its deterministic timer behavior.
Description check✅ PassedThe description covers the problem, fix, testing, and issue reference, which is enough despite different headings.
Linked Issues check✅ PassedThe PR implements the off-by-one retry_after cap and adds frozen-time regression tests requested by #346.
Out of Scope Changes check✅ PassedAll changes are directly related to the rate-limit retry fix and its regression coverage.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/346-retry-after-ceiling

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

Comment @coderabbitai help to get the list of available commands.

@cloudflare-workers-and-pages

cloudflare-workers-and-pagesBot commented Jul 17, 2026

Copy link
Copy Markdown

Deploying with Cloudflare Workers Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

StatusNameLatest CommitPreview URLUpdated (UTC)
✅ Deployment successful!
View logs
frontend-staging6731cd7Commit Preview URL

Branch Preview URL
Jul 29 2026, 09:10 AM

…coarse timers (#346)
Rebased onto current main: main had already adopted math.ceil in the
flashcard limiter; this keeps that and adds the min(window, ...) cap to
BOTH sliding-window limiters (services/request_limits.py still had the
old int(...) + 1 overshoot), plus regression tests for coincident
timestamps and sub-second remainders.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@AndresL230
AndresL230force-pushed the fix/346-retry-after-ceiling branch from a0cf1f9 to b0716afCompareJuly 29, 2026 08:43
@AndresL230
AndresL230 marked this pull request as ready for review July 29, 2026 08:44
… align twin comments
Review follow-ups: request_limits.py's comment now names the negative-
elapsed (NTP step) case the cap protects against, matching its twin; a
regression test in both limiter test files freezes time backward so a
future revert of the cap fails loudly.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@AndresL230
AndresL230 merged commit 5d5130b into mainJul 29, 2026
6 checks passed
@AndresL230
AndresL230 deleted the fix/346-retry-after-ceiling branch August 2, 2026 18:30
Sign up for freeto 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.

check_rate_limit retry_after off-by-one makes TestRateLimit flaky on Windows

2 participants

@Darkest-Teddy@AndresL230
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(limits): retry_after ceiling capped at window — deterministic on coarse timers (#346) - #351

Merged
AndresL230 merged 2 commits into
mainfrom
fix/346-retry-after-ceiling
Jul 29, 2026
Merged

fix(limits): retry_after ceiling capped at window — deterministic on coarse timers (#346)#351
AndresL230 merged 2 commits into
mainfrom
fix/346-retry-after-ceiling

Conversation

@Darkest-Teddy

@Darkest-TeddyDarkest-Teddy commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

Problem

check_rate_limit computes the retry hint as int(window - elapsed) + 1 — a broken ceiling. When the rate-limited call shares an identical time.time() value with the first call in the window, elapsed == 0.0 exactly and the hint overshoots to window + 1 (61s for a 60s window). On Windows this happens reliably (the timer ticks every ~15.6 ms, so a tight loop of 6 calls lands on one tick), which:

  • violates the (0, window] contract callers surface as Retry-After, and
  • makes TestRateLimit::test_sixth_call_returns_retry_after fail out-of-the-box for every Windows contributor (assert 61 <= 60). Linux CI passes only by accident — elapsed there is tiny-but-nonzero, so int(59.999…) + 1 == 60.

Fix

True ceiling semantics, clamped at the window, in both copies of the limiter:

retry=min(window_sec, math.ceil(window_sec- (now-bucket[0])))
  • services/request_limits.py (shared limiter behind OCR extraction + Gradescope routes)
  • services/flashcard_import_service.py (older twin the shared module's docstring notes should eventually migrate)

The clamp also guards the pathological elapsed < 0 case (clock steps backward between calls).

Tests (TDD — written first, verified failing)

  • test_retry_capped_at_window_on_coincident_timestamps (both limiters): freezes time.time so elapsed == 0.0 exactly — deterministic reproduction of the Windows failure on every platform; pins retry == 60. Failed (61) before the fix, passes after.
  • test_retry_uses_ceiling_of_remaining_window (both limiters): 0.5s remaining → retry == 1, pinning ceiling semantics against a regression to plain truncation.
  • New tests/test_request_limits.py also covers the shared limiter's base contract (allow-up-to-limit, over-limit hint bounds, key isolation, window reset), which previously had no direct unit tests.

Verification

  • Affected suites (test_flashcard_import_service, test_request_limits, test_extract_auth_bounds, test_gradescope): 72/72 passed — on the Windows machine where the suite was previously red.
  • ruff check clean on all touched files.

Supersedes #347 (same clamp idea, but closed without regression tests; this PR adds the deterministic frozen-time coverage the issue asked for).

Fixes#346

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Improved rate-limit retry guidance so displayed wait times remain within the configured limit window.
    • Corrected rounding for fractional remaining wait times, providing more accurate retry estimates.
    • Ensured edge cases, including coincident timestamps and near-expired windows, return valid retry values.
  • Tests

    • Added coverage for rate-limit boundaries, timing behavior, key isolation, and window resets.

@coderabbitai

coderabbitaiBot commented Jul 17, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@AndresL230, you've reached your PR review limit, so we couldn't start this review.

Next review available in:35 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 0b822bd9-4b81-4c35-85d6-9dd7b4d0d7ea

📥 Commits

Reviewing files that changed from the base of the PR and between b0716af and 6731cd7.

📒 Files selected for processing (3)
  • backend/services/request_limits.py
  • backend/tests/test_flashcard_import_service.py
  • backend/tests/test_request_limits.py
📝 Walkthrough

Walkthrough

The rate limiters now calculate retry delays with ceiling semantics and cap them at the configured window. Tests cover coincident timestamps, near-window expiry, reset behavior, limit enforcement, and per-key isolation.

Changes

Rate-limit retry behavior

Layer / File(s)Summary
Bounded retry calculations
backend/services/flashcard_import_service.py, backend/services/request_limits.py
Retry delays use ceiling-based remaining-window calculations capped at the configured rate-limit window.
Retry boundary coverage
backend/tests/test_flashcard_import_service.py, backend/tests/test_request_limits.py
Tests validate bounded retry hints, coincident timestamps, near-window expiry, window resets, request limits, and per-key isolation.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested reviewers:jose-gael-cruz-lopez

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check nameStatusExplanationResolution
Docstring Coverage⚠️ WarningDocstring 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.
✅ Passed checks (4 passed)
Check nameStatusExplanation
Title check✅ PassedThe title clearly summarizes the retry_after ceiling fix and its deterministic timer behavior.
Description check✅ PassedThe description covers the problem, fix, testing, and issue reference, which is enough despite different headings.
Linked Issues check✅ PassedThe PR implements the off-by-one retry_after cap and adds frozen-time regression tests requested by #346.
Out of Scope Changes check✅ PassedAll changes are directly related to the rate-limit retry fix and its regression coverage.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/346-retry-after-ceiling

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

Comment @coderabbitai help to get the list of available commands.

@cloudflare-workers-and-pages

cloudflare-workers-and-pagesBot commented Jul 17, 2026

Copy link
Copy Markdown

Deploying with Cloudflare Workers Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

StatusNameLatest CommitPreview URLUpdated (UTC)
✅ Deployment successful!
View logs
frontend-staging6731cd7Commit Preview URL

Branch Preview URL
Jul 29 2026, 09:10 AM

…coarse timers (#346)
Rebased onto current main: main had already adopted math.ceil in the
flashcard limiter; this keeps that and adds the min(window, ...) cap to
BOTH sliding-window limiters (services/request_limits.py still had the
old int(...) + 1 overshoot), plus regression tests for coincident
timestamps and sub-second remainders.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@AndresL230
AndresL230force-pushed the fix/346-retry-after-ceiling branch from a0cf1f9 to b0716afCompareJuly 29, 2026 08:43
@AndresL230
AndresL230 marked this pull request as ready for review July 29, 2026 08:44
… align twin comments
Review follow-ups: request_limits.py's comment now names the negative-
elapsed (NTP step) case the cap protects against, matching its twin; a
regression test in both limiter test files freezes time backward so a
future revert of the cap fails loudly.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@AndresL230
AndresL230 merged commit 5d5130b into mainJul 29, 2026
6 checks passed
@AndresL230
AndresL230 deleted the fix/346-retry-after-ceiling branch August 2, 2026 18:30
Sign up for freeto 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.

check_rate_limit retry_after off-by-one makes TestRateLimit flaky on Windows

2 participants

@Darkest-Teddy@AndresL230
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

fix(limits): retry_after ceiling capped at window — deterministic on coarse timers (#346) - #351

Merged
AndresL230 merged 2 commits into
mainfrom
fix/346-retry-after-ceiling
Jul 29, 2026
Merged

fix(limits): retry_after ceiling capped at window — deterministic on coarse timers (#346)#351
AndresL230 merged 2 commits into
mainfrom
fix/346-retry-after-ceiling

Conversation

@Darkest-Teddy

@Darkest-TeddyDarkest-Teddy commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

Problem

check_rate_limit computes the retry hint as int(window - elapsed) + 1 — a broken ceiling. When the rate-limited call shares an identical time.time() value with the first call in the window, elapsed == 0.0 exactly and the hint overshoots to window + 1 (61s for a 60s window). On Windows this happens reliably (the timer ticks every ~15.6 ms, so a tight loop of 6 calls lands on one tick), which:

  • violates the (0, window] contract callers surface as Retry-After, and
  • makes TestRateLimit::test_sixth_call_returns_retry_after fail out-of-the-box for every Windows contributor (assert 61 <= 60). Linux CI passes only by accident — elapsed there is tiny-but-nonzero, so int(59.999…) + 1 == 60.

Fix

True ceiling semantics, clamped at the window, in both copies of the limiter:

retry=min(window_sec, math.ceil(window_sec- (now-bucket[0])))
  • services/request_limits.py (shared limiter behind OCR extraction + Gradescope routes)
  • services/flashcard_import_service.py (older twin the shared module's docstring notes should eventually migrate)

The clamp also guards the pathological elapsed < 0 case (clock steps backward between calls).

Tests (TDD — written first, verified failing)

  • test_retry_capped_at_window_on_coincident_timestamps (both limiters): freezes time.time so elapsed == 0.0 exactly — deterministic reproduction of the Windows failure on every platform; pins retry == 60. Failed (61) before the fix, passes after.
  • test_retry_uses_ceiling_of_remaining_window (both limiters): 0.5s remaining → retry == 1, pinning ceiling semantics against a regression to plain truncation.
  • New tests/test_request_limits.py also covers the shared limiter's base contract (allow-up-to-limit, over-limit hint bounds, key isolation, window reset), which previously had no direct unit tests.

Verification

  • Affected suites (test_flashcard_import_service, test_request_limits, test_extract_auth_bounds, test_gradescope): 72/72 passed — on the Windows machine where the suite was previously red.
  • ruff check clean on all touched files.

Supersedes #347 (same clamp idea, but closed without regression tests; this PR adds the deterministic frozen-time coverage the issue asked for).

Fixes#346

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Improved rate-limit retry guidance so displayed wait times remain within the configured limit window.
    • Corrected rounding for fractional remaining wait times, providing more accurate retry estimates.
    • Ensured edge cases, including coincident timestamps and near-expired windows, return valid retry values.
  • Tests

    • Added coverage for rate-limit boundaries, timing behavior, key isolation, and window resets.

@coderabbitai

coderabbitaiBot commented Jul 17, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@AndresL230, you've reached your PR review limit, so we couldn't start this review.

Next review available in:35 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 0b822bd9-4b81-4c35-85d6-9dd7b4d0d7ea

📥 Commits

Reviewing files that changed from the base of the PR and between b0716af and 6731cd7.

📒 Files selected for processing (3)
  • backend/services/request_limits.py
  • backend/tests/test_flashcard_import_service.py
  • backend/tests/test_request_limits.py
📝 Walkthrough

Walkthrough

The rate limiters now calculate retry delays with ceiling semantics and cap them at the configured window. Tests cover coincident timestamps, near-window expiry, reset behavior, limit enforcement, and per-key isolation.

Changes

Rate-limit retry behavior

Layer / File(s)Summary
Bounded retry calculations
backend/services/flashcard_import_service.py, backend/services/request_limits.py
Retry delays use ceiling-based remaining-window calculations capped at the configured rate-limit window.
Retry boundary coverage
backend/tests/test_flashcard_import_service.py, backend/tests/test_request_limits.py
Tests validate bounded retry hints, coincident timestamps, near-window expiry, window resets, request limits, and per-key isolation.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested reviewers:jose-gael-cruz-lopez

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check nameStatusExplanationResolution
Docstring Coverage⚠️ WarningDocstring 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.
✅ Passed checks (4 passed)
Check nameStatusExplanation
Title check✅ PassedThe title clearly summarizes the retry_after ceiling fix and its deterministic timer behavior.
Description check✅ PassedThe description covers the problem, fix, testing, and issue reference, which is enough despite different headings.
Linked Issues check✅ PassedThe PR implements the off-by-one retry_after cap and adds frozen-time regression tests requested by #346.
Out of Scope Changes check✅ PassedAll changes are directly related to the rate-limit retry fix and its regression coverage.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/346-retry-after-ceiling

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

Comment @coderabbitai help to get the list of available commands.

@cloudflare-workers-and-pages

cloudflare-workers-and-pagesBot commented Jul 17, 2026

Copy link
Copy Markdown

Deploying with Cloudflare Workers Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

StatusNameLatest CommitPreview URLUpdated (UTC)
✅ Deployment successful!
View logs
frontend-staging6731cd7Commit Preview URL

Branch Preview URL
Jul 29 2026, 09:10 AM

…coarse timers (#346)
Rebased onto current main: main had already adopted math.ceil in the
flashcard limiter; this keeps that and adds the min(window, ...) cap to
BOTH sliding-window limiters (services/request_limits.py still had the
old int(...) + 1 overshoot), plus regression tests for coincident
timestamps and sub-second remainders.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@AndresL230
AndresL230force-pushed the fix/346-retry-after-ceiling branch from a0cf1f9 to b0716afCompareJuly 29, 2026 08:43
@AndresL230
AndresL230 marked this pull request as ready for review July 29, 2026 08:44
… align twin comments
Review follow-ups: request_limits.py's comment now names the negative-
elapsed (NTP step) case the cap protects against, matching its twin; a
regression test in both limiter test files freezes time backward so a
future revert of the cap fails loudly.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@AndresL230
AndresL230 merged commit 5d5130b into mainJul 29, 2026
6 checks passed
@AndresL230
AndresL230 deleted the fix/346-retry-after-ceiling branch August 2, 2026 18:30
Sign up for freeto 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.

check_rate_limit retry_after off-by-one makes TestRateLimit flaky on Windows

2 participants

@Darkest-Teddy@AndresL230
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(limits): retry_after ceiling capped at window — deterministic on coarse timers (#346) - #351

Merged
AndresL230 merged 2 commits into
mainfrom
fix/346-retry-after-ceiling
Jul 29, 2026
Merged

fix(limits): retry_after ceiling capped at window — deterministic on coarse timers (#346)#351
AndresL230 merged 2 commits into
mainfrom
fix/346-retry-after-ceiling

Conversation

@Darkest-Teddy

@Darkest-TeddyDarkest-Teddy commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

Problem

check_rate_limit computes the retry hint as int(window - elapsed) + 1 — a broken ceiling. When the rate-limited call shares an identical time.time() value with the first call in the window, elapsed == 0.0 exactly and the hint overshoots to window + 1 (61s for a 60s window). On Windows this happens reliably (the timer ticks every ~15.6 ms, so a tight loop of 6 calls lands on one tick), which:

  • violates the (0, window] contract callers surface as Retry-After, and
  • makes TestRateLimit::test_sixth_call_returns_retry_after fail out-of-the-box for every Windows contributor (assert 61 <= 60). Linux CI passes only by accident — elapsed there is tiny-but-nonzero, so int(59.999…) + 1 == 60.

Fix

True ceiling semantics, clamped at the window, in both copies of the limiter:

retry=min(window_sec, math.ceil(window_sec- (now-bucket[0])))
  • services/request_limits.py (shared limiter behind OCR extraction + Gradescope routes)
  • services/flashcard_import_service.py (older twin the shared module's docstring notes should eventually migrate)

The clamp also guards the pathological elapsed < 0 case (clock steps backward between calls).

Tests (TDD — written first, verified failing)

  • test_retry_capped_at_window_on_coincident_timestamps (both limiters): freezes time.time so elapsed == 0.0 exactly — deterministic reproduction of the Windows failure on every platform; pins retry == 60. Failed (61) before the fix, passes after.
  • test_retry_uses_ceiling_of_remaining_window (both limiters): 0.5s remaining → retry == 1, pinning ceiling semantics against a regression to plain truncation.
  • New tests/test_request_limits.py also covers the shared limiter's base contract (allow-up-to-limit, over-limit hint bounds, key isolation, window reset), which previously had no direct unit tests.

Verification

  • Affected suites (test_flashcard_import_service, test_request_limits, test_extract_auth_bounds, test_gradescope): 72/72 passed — on the Windows machine where the suite was previously red.
  • ruff check clean on all touched files.

Supersedes #347 (same clamp idea, but closed without regression tests; this PR adds the deterministic frozen-time coverage the issue asked for).

Fixes#346

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Improved rate-limit retry guidance so displayed wait times remain within the configured limit window.
    • Corrected rounding for fractional remaining wait times, providing more accurate retry estimates.
    • Ensured edge cases, including coincident timestamps and near-expired windows, return valid retry values.
  • Tests

    • Added coverage for rate-limit boundaries, timing behavior, key isolation, and window resets.

@coderabbitai

coderabbitaiBot commented Jul 17, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@AndresL230, you've reached your PR review limit, so we couldn't start this review.

Next review available in:35 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 0b822bd9-4b81-4c35-85d6-9dd7b4d0d7ea

📥 Commits

Reviewing files that changed from the base of the PR and between b0716af and 6731cd7.

📒 Files selected for processing (3)
  • backend/services/request_limits.py
  • backend/tests/test_flashcard_import_service.py
  • backend/tests/test_request_limits.py
📝 Walkthrough

Walkthrough

The rate limiters now calculate retry delays with ceiling semantics and cap them at the configured window. Tests cover coincident timestamps, near-window expiry, reset behavior, limit enforcement, and per-key isolation.

Changes

Rate-limit retry behavior

Layer / File(s)Summary
Bounded retry calculations
backend/services/flashcard_import_service.py, backend/services/request_limits.py
Retry delays use ceiling-based remaining-window calculations capped at the configured rate-limit window.
Retry boundary coverage
backend/tests/test_flashcard_import_service.py, backend/tests/test_request_limits.py
Tests validate bounded retry hints, coincident timestamps, near-window expiry, window resets, request limits, and per-key isolation.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested reviewers:jose-gael-cruz-lopez

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check nameStatusExplanationResolution
Docstring Coverage⚠️ WarningDocstring 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.
✅ Passed checks (4 passed)
Check nameStatusExplanation
Title check✅ PassedThe title clearly summarizes the retry_after ceiling fix and its deterministic timer behavior.
Description check✅ PassedThe description covers the problem, fix, testing, and issue reference, which is enough despite different headings.
Linked Issues check✅ PassedThe PR implements the off-by-one retry_after cap and adds frozen-time regression tests requested by #346.
Out of Scope Changes check✅ PassedAll changes are directly related to the rate-limit retry fix and its regression coverage.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/346-retry-after-ceiling

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

Comment @coderabbitai help to get the list of available commands.

@cloudflare-workers-and-pages

cloudflare-workers-and-pagesBot commented Jul 17, 2026

Copy link
Copy Markdown

Deploying with Cloudflare Workers Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

StatusNameLatest CommitPreview URLUpdated (UTC)
✅ Deployment successful!
View logs
frontend-staging6731cd7Commit Preview URL

Branch Preview URL
Jul 29 2026, 09:10 AM

…coarse timers (#346)
Rebased onto current main: main had already adopted math.ceil in the
flashcard limiter; this keeps that and adds the min(window, ...) cap to
BOTH sliding-window limiters (services/request_limits.py still had the
old int(...) + 1 overshoot), plus regression tests for coincident
timestamps and sub-second remainders.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@AndresL230
AndresL230force-pushed the fix/346-retry-after-ceiling branch from a0cf1f9 to b0716afCompareJuly 29, 2026 08:43
@AndresL230
AndresL230 marked this pull request as ready for review July 29, 2026 08:44
… align twin comments
Review follow-ups: request_limits.py's comment now names the negative-
elapsed (NTP step) case the cap protects against, matching its twin; a
regression test in both limiter test files freezes time backward so a
future revert of the cap fails loudly.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@AndresL230
AndresL230 merged commit 5d5130b into mainJul 29, 2026
6 checks passed
@AndresL230
AndresL230 deleted the fix/346-retry-after-ceiling branch August 2, 2026 18:30
Sign up for freeto 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.

check_rate_limit retry_after off-by-one makes TestRateLimit flaky on Windows

2 participants

@Darkest-Teddy@AndresL230
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(limits): retry_after ceiling capped at window — deterministic on coarse timers (#346) - #351

Merged
AndresL230 merged 2 commits into
mainfrom
fix/346-retry-after-ceiling
Jul 29, 2026
Merged

fix(limits): retry_after ceiling capped at window — deterministic on coarse timers (#346)#351
AndresL230 merged 2 commits into
mainfrom
fix/346-retry-after-ceiling

Conversation

@Darkest-Teddy

@Darkest-TeddyDarkest-Teddy commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

Problem

check_rate_limit computes the retry hint as int(window - elapsed) + 1 — a broken ceiling. When the rate-limited call shares an identical time.time() value with the first call in the window, elapsed == 0.0 exactly and the hint overshoots to window + 1 (61s for a 60s window). On Windows this happens reliably (the timer ticks every ~15.6 ms, so a tight loop of 6 calls lands on one tick), which:

  • violates the (0, window] contract callers surface as Retry-After, and
  • makes TestRateLimit::test_sixth_call_returns_retry_after fail out-of-the-box for every Windows contributor (assert 61 <= 60). Linux CI passes only by accident — elapsed there is tiny-but-nonzero, so int(59.999…) + 1 == 60.

Fix

True ceiling semantics, clamped at the window, in both copies of the limiter:

retry=min(window_sec, math.ceil(window_sec- (now-bucket[0])))
  • services/request_limits.py (shared limiter behind OCR extraction + Gradescope routes)
  • services/flashcard_import_service.py (older twin the shared module's docstring notes should eventually migrate)

The clamp also guards the pathological elapsed < 0 case (clock steps backward between calls).

Tests (TDD — written first, verified failing)

  • test_retry_capped_at_window_on_coincident_timestamps (both limiters): freezes time.time so elapsed == 0.0 exactly — deterministic reproduction of the Windows failure on every platform; pins retry == 60. Failed (61) before the fix, passes after.
  • test_retry_uses_ceiling_of_remaining_window (both limiters): 0.5s remaining → retry == 1, pinning ceiling semantics against a regression to plain truncation.
  • New tests/test_request_limits.py also covers the shared limiter's base contract (allow-up-to-limit, over-limit hint bounds, key isolation, window reset), which previously had no direct unit tests.

Verification

  • Affected suites (test_flashcard_import_service, test_request_limits, test_extract_auth_bounds, test_gradescope): 72/72 passed — on the Windows machine where the suite was previously red.
  • ruff check clean on all touched files.

Supersedes #347 (same clamp idea, but closed without regression tests; this PR adds the deterministic frozen-time coverage the issue asked for).

Fixes#346

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Improved rate-limit retry guidance so displayed wait times remain within the configured limit window.
    • Corrected rounding for fractional remaining wait times, providing more accurate retry estimates.
    • Ensured edge cases, including coincident timestamps and near-expired windows, return valid retry values.
  • Tests

    • Added coverage for rate-limit boundaries, timing behavior, key isolation, and window resets.

@coderabbitai

coderabbitaiBot commented Jul 17, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@AndresL230, you've reached your PR review limit, so we couldn't start this review.

Next review available in:35 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 0b822bd9-4b81-4c35-85d6-9dd7b4d0d7ea

📥 Commits

Reviewing files that changed from the base of the PR and between b0716af and 6731cd7.

📒 Files selected for processing (3)
  • backend/services/request_limits.py
  • backend/tests/test_flashcard_import_service.py
  • backend/tests/test_request_limits.py
📝 Walkthrough

Walkthrough

The rate limiters now calculate retry delays with ceiling semantics and cap them at the configured window. Tests cover coincident timestamps, near-window expiry, reset behavior, limit enforcement, and per-key isolation.

Changes

Rate-limit retry behavior

Layer / File(s)Summary
Bounded retry calculations
backend/services/flashcard_import_service.py, backend/services/request_limits.py
Retry delays use ceiling-based remaining-window calculations capped at the configured rate-limit window.
Retry boundary coverage
backend/tests/test_flashcard_import_service.py, backend/tests/test_request_limits.py
Tests validate bounded retry hints, coincident timestamps, near-window expiry, window resets, request limits, and per-key isolation.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested reviewers:jose-gael-cruz-lopez

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check nameStatusExplanationResolution
Docstring Coverage⚠️ WarningDocstring 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.
✅ Passed checks (4 passed)
Check nameStatusExplanation
Title check✅ PassedThe title clearly summarizes the retry_after ceiling fix and its deterministic timer behavior.
Description check✅ PassedThe description covers the problem, fix, testing, and issue reference, which is enough despite different headings.
Linked Issues check✅ PassedThe PR implements the off-by-one retry_after cap and adds frozen-time regression tests requested by #346.
Out of Scope Changes check✅ PassedAll changes are directly related to the rate-limit retry fix and its regression coverage.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/346-retry-after-ceiling

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

Comment @coderabbitai help to get the list of available commands.

@cloudflare-workers-and-pages

cloudflare-workers-and-pagesBot commented Jul 17, 2026

Copy link
Copy Markdown

Deploying with Cloudflare Workers Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

StatusNameLatest CommitPreview URLUpdated (UTC)
✅ Deployment successful!
View logs
frontend-staging6731cd7Commit Preview URL

Branch Preview URL
Jul 29 2026, 09:10 AM

…coarse timers (#346)
Rebased onto current main: main had already adopted math.ceil in the
flashcard limiter; this keeps that and adds the min(window, ...) cap to
BOTH sliding-window limiters (services/request_limits.py still had the
old int(...) + 1 overshoot), plus regression tests for coincident
timestamps and sub-second remainders.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@AndresL230
AndresL230force-pushed the fix/346-retry-after-ceiling branch from a0cf1f9 to b0716afCompareJuly 29, 2026 08:43
@AndresL230
AndresL230 marked this pull request as ready for review July 29, 2026 08:44
… align twin comments
Review follow-ups: request_limits.py's comment now names the negative-
elapsed (NTP step) case the cap protects against, matching its twin; a
regression test in both limiter test files freezes time backward so a
future revert of the cap fails loudly.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@AndresL230
AndresL230 merged commit 5d5130b into mainJul 29, 2026
6 checks passed
@AndresL230
AndresL230 deleted the fix/346-retry-after-ceiling branch August 2, 2026 18:30
Sign up for freeto 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.

check_rate_limit retry_after off-by-one makes TestRateLimit flaky on Windows

2 participants

@Darkest-Teddy@AndresL230
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

fix(limits): retry_after ceiling capped at window — deterministic on coarse timers (#346) - #351

Merged
AndresL230 merged 2 commits into
mainfrom
fix/346-retry-after-ceiling
Jul 29, 2026
Merged

fix(limits): retry_after ceiling capped at window — deterministic on coarse timers (#346)#351
AndresL230 merged 2 commits into
mainfrom
fix/346-retry-after-ceiling

Conversation

@Darkest-Teddy

@Darkest-TeddyDarkest-Teddy commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

Problem

check_rate_limit computes the retry hint as int(window - elapsed) + 1 — a broken ceiling. When the rate-limited call shares an identical time.time() value with the first call in the window, elapsed == 0.0 exactly and the hint overshoots to window + 1 (61s for a 60s window). On Windows this happens reliably (the timer ticks every ~15.6 ms, so a tight loop of 6 calls lands on one tick), which:

  • violates the (0, window] contract callers surface as Retry-After, and
  • makes TestRateLimit::test_sixth_call_returns_retry_after fail out-of-the-box for every Windows contributor (assert 61 <= 60). Linux CI passes only by accident — elapsed there is tiny-but-nonzero, so int(59.999…) + 1 == 60.

Fix

True ceiling semantics, clamped at the window, in both copies of the limiter:

retry=min(window_sec, math.ceil(window_sec- (now-bucket[0])))
  • services/request_limits.py (shared limiter behind OCR extraction + Gradescope routes)
  • services/flashcard_import_service.py (older twin the shared module's docstring notes should eventually migrate)

The clamp also guards the pathological elapsed < 0 case (clock steps backward between calls).

Tests (TDD — written first, verified failing)

  • test_retry_capped_at_window_on_coincident_timestamps (both limiters): freezes time.time so elapsed == 0.0 exactly — deterministic reproduction of the Windows failure on every platform; pins retry == 60. Failed (61) before the fix, passes after.
  • test_retry_uses_ceiling_of_remaining_window (both limiters): 0.5s remaining → retry == 1, pinning ceiling semantics against a regression to plain truncation.
  • New tests/test_request_limits.py also covers the shared limiter's base contract (allow-up-to-limit, over-limit hint bounds, key isolation, window reset), which previously had no direct unit tests.

Verification

  • Affected suites (test_flashcard_import_service, test_request_limits, test_extract_auth_bounds, test_gradescope): 72/72 passed — on the Windows machine where the suite was previously red.
  • ruff check clean on all touched files.

Supersedes #347 (same clamp idea, but closed without regression tests; this PR adds the deterministic frozen-time coverage the issue asked for).

Fixes#346

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Improved rate-limit retry guidance so displayed wait times remain within the configured limit window.
    • Corrected rounding for fractional remaining wait times, providing more accurate retry estimates.
    • Ensured edge cases, including coincident timestamps and near-expired windows, return valid retry values.
  • Tests

    • Added coverage for rate-limit boundaries, timing behavior, key isolation, and window resets.

@coderabbitai

coderabbitaiBot commented Jul 17, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@AndresL230, you've reached your PR review limit, so we couldn't start this review.

Next review available in:35 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 0b822bd9-4b81-4c35-85d6-9dd7b4d0d7ea

📥 Commits

Reviewing files that changed from the base of the PR and between b0716af and 6731cd7.

📒 Files selected for processing (3)
  • backend/services/request_limits.py
  • backend/tests/test_flashcard_import_service.py
  • backend/tests/test_request_limits.py
📝 Walkthrough

Walkthrough

The rate limiters now calculate retry delays with ceiling semantics and cap them at the configured window. Tests cover coincident timestamps, near-window expiry, reset behavior, limit enforcement, and per-key isolation.

Changes

Rate-limit retry behavior

Layer / File(s)Summary
Bounded retry calculations
backend/services/flashcard_import_service.py, backend/services/request_limits.py
Retry delays use ceiling-based remaining-window calculations capped at the configured rate-limit window.
Retry boundary coverage
backend/tests/test_flashcard_import_service.py, backend/tests/test_request_limits.py
Tests validate bounded retry hints, coincident timestamps, near-window expiry, window resets, request limits, and per-key isolation.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested reviewers:jose-gael-cruz-lopez

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check nameStatusExplanationResolution
Docstring Coverage⚠️ WarningDocstring 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.
✅ Passed checks (4 passed)
Check nameStatusExplanation
Title check✅ PassedThe title clearly summarizes the retry_after ceiling fix and its deterministic timer behavior.
Description check✅ PassedThe description covers the problem, fix, testing, and issue reference, which is enough despite different headings.
Linked Issues check✅ PassedThe PR implements the off-by-one retry_after cap and adds frozen-time regression tests requested by #346.
Out of Scope Changes check✅ PassedAll changes are directly related to the rate-limit retry fix and its regression coverage.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/346-retry-after-ceiling

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

Comment @coderabbitai help to get the list of available commands.

@cloudflare-workers-and-pages

cloudflare-workers-and-pagesBot commented Jul 17, 2026

Copy link
Copy Markdown

Deploying with Cloudflare Workers Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

StatusNameLatest CommitPreview URLUpdated (UTC)
✅ Deployment successful!
View logs
frontend-staging6731cd7Commit Preview URL

Branch Preview URL
Jul 29 2026, 09:10 AM

…coarse timers (#346)
Rebased onto current main: main had already adopted math.ceil in the
flashcard limiter; this keeps that and adds the min(window, ...) cap to
BOTH sliding-window limiters (services/request_limits.py still had the
old int(...) + 1 overshoot), plus regression tests for coincident
timestamps and sub-second remainders.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@AndresL230
AndresL230force-pushed the fix/346-retry-after-ceiling branch from a0cf1f9 to b0716afCompareJuly 29, 2026 08:43
@AndresL230
AndresL230 marked this pull request as ready for review July 29, 2026 08:44
… align twin comments
Review follow-ups: request_limits.py's comment now names the negative-
elapsed (NTP step) case the cap protects against, matching its twin; a
regression test in both limiter test files freezes time backward so a
future revert of the cap fails loudly.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@AndresL230
AndresL230 merged commit 5d5130b into mainJul 29, 2026
6 checks passed
@AndresL230
AndresL230 deleted the fix/346-retry-after-ceiling branch August 2, 2026 18:30
Sign up for freeto 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.

check_rate_limit retry_after off-by-one makes TestRateLimit flaky on Windows

2 participants

@Darkest-Teddy@AndresL230