Skip to content

Tighten too-long heuristic for design-document issues#3620

Merged
jlowin merged 1 commit intomainfrom
tune-too-long-heuristic
Mar 25, 2026
Merged

Tighten too-long heuristic for design-document issues#3620
jlowin merged 1 commit intomainfrom
tune-too-long-heuristic

Conversation

@jlowin
Copy link
Copy Markdown
Member

@jlowin jlowin commented Mar 25, 2026

Enhancement requests that propose multiple solutions, include numbered lists of approaches, or spec out suggested API shapes are doing design work that belongs to maintainers. The too-long heuristic now explicitly flags these patterns alongside the existing LLM failure modes.

@jlowin
Tighten too-long heuristic for design-document issues
0a696fd 
Issues that propose solutions, list numbered approaches, or include
suggested API shapes are doing design work that belongs to maintainers.
@marvin-context-protocol marvin-context-protocol Bot added the enhancement Improvement to existing functionality. For issues and smaller PR improvements. label Mar 25, 2026
@jlowin jlowin merged commit 00df243 into main Mar 25, 2026
7 of 8 checks passed
@jlowin jlowin deleted the tune-too-long-heuristic branch March 25, 2026 14:57
@marvin-context-protocol
Copy link
Copy Markdown
Contributor

marvin-context-protocol Bot commented Mar 25, 2026

Test Failure Analysis

Summary: A flaky rate limiting test failed on Python 3.13 — test_rate_limiting_recovery_over_time — which is unrelated to the changes in this PR.

Root Cause: The test sets up RateLimitingMiddleware(max_requests_per_second=10.0, burst_capacity=4). The underlying TokenBucketRateLimiter initializes with 4 tokens (full burst capacity). The test makes only one call to "use up capacity," but that leaves 3 tokens remaining — so the immediately subsequent call succeeds instead of raising ToolError. The test incorrectly assumes a single call exhausts the burst budget.

Suggested Solution: Fix the test in tests/server/middleware/test_rate_limiting.py around line 439-461:

Use burst_capacity=1 so that the first call exhausts the bucket:

RateLimitingMiddleware(
    max_requests_per_second=10.0,
    burst_capacity=1,  # was 4 — one call exhausts the bucket
)

Or, exhaust all burst tokens before the assertion (similar to how test_global_rate_limiting_blocks_requests at line ~430 does it by manually setting middleware.global_limiter.tokens = 0).

This failure is not caused by this PR, which only modifies .github/workflows/marvin-label-triage.yml.

Detailed Analysis

Failing test: TestRateLimitingMiddlewareIntegration.test_rate_limiting_recovery_over_time
File: tests/server/middleware/test_rate_limiting.py:453
Python version: 3.13 on ubuntu-latest (passes on 3.10)

Log excerpt:

E           Failed: DID NOT RAISE <class 'fastmcp.exceptions.ToolError'>
tests/server/middleware/test_rate_limiting.py:453: Failed

Token bucket behavior (src/fastmcp/server/middleware/rate_limiting.py:34):

self.tokens = capacity  # starts FULL at burst_capacity (4)

With burst_capacity=4, after one call there are still 3 tokens available. The test expects the second immediate call to be rate-limited, but it isn't — the bucket isn't empty yet.

Comparison: The test at line ~430 (test_global_rate_limiting_blocks_requests) works correctly because it explicitly sets middleware.global_limiter.tokens = 0 before asserting rate limiting, rather than relying on a specific burst capacity.

The Python 3.10 run appears to pass, which may indicate a timing difference in test execution speed (parallel workers, GIL behavior, etc.) causing nondeterministic results.

Related Files
  • tests/server/middleware/test_rate_limiting.py:439-461 — the failing test
  • src/fastmcp/server/middleware/rate_limiting.py:22-58 — TokenBucketRateLimiter implementation (tokens start at full capacity)
  • src/fastmcp/server/middleware/rate_limiting.py:129-143 — RateLimitingMiddleware that creates the token bucket with burst_capacity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

enhancement Improvement to existing functionality. For issues and smaller PR improvements.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@jlowin