fix(mcp): the initialize limiter refuses correctly and tells the client nothing #20

New issue

Closed

opened 2026-08-26 10:09:55 +00:00 by jlxq0 · 3 comments

jlxq0 commented 2026-08-26 10:09:55 +00:00

Owner

Copy link

Reported by Clark, who hit it while blocked by it. Filed rather than fixed: development is paused until 19:00 and the DAV server's rate limiting is separately unexplained. This goes ahead of #16 — it has an observed victim.

What happened

Julian's claude.ai session reconnected repeatedly, exhausted the initialize allowance, never reached a tool call, and concluded the window "needs real time to reset". Five failed attempts, and a wrong model of the rule at the end of them.

The limiter behaved correctly. Nothing here proposes changing it.

The numbers, read from this repository rather than relayed

// src/main.rs
let initialize_limiter = Arc::new(InitializeLimiter::new(
    session::SESSION_KEEP_ALIVE,          // 30 * 60
    MAX_INITIALIZES_PER_IDENTITY,         // 8
));

InitializeLimiter::new(replenish_1_per, burst) builds Quota::with_period(30 min).allow_burst(8). So: eight back-to-back, then one slot every 30 minutes, and four hours to refill from empty.

That is worse than "the window resets", and wrong in the direction that costs the user. Someone who waits out what they believe is a 30-minute window gets exactly one session, reconnects twice, and is refused again — which reads as the rule being broken rather than obeyed.

Two buckets, keyed on sha256(bearer)[..16] and the Logto sub. That is deliberate and it is why this caught a client minting a fresh token per session: a new bearer gets a fresh bucket, the sub bucket does not. The header comment already names this case.

The defect

src/main.rs:

return (
    StatusCode::TOO_MANY_REQUESTS,
    "too many MCP initialize requests; try again later\n",
).into_response();

No Retry-After. No machine-readable class. A plain-text body whose entire actionable content is "later".

A client cannot distinguish too many sessions from your request was rejected, and cannot know when to try again. So it retries, and every retry against a correct rule looks to the person behind it like the rule being broken.

The number needed is already computed and then thrown away. governor returns NotUntil, which carries wait_time_from(now); InitializeLimiter::check maps it to a unit struct:

if bearer_bucket.check().is_err() {
    return Err(RateLimited);
}

This is not "we cannot know when to retry". It is knowing and discarding it at the boundary.

Proposed shape, subject to review

• check returns the wait, not a unit — Err(RateLimited { retry_after: Duration }) from NotUntil::wait_time_from.
• The 429 carries Retry-After with that number: seconds until one slot exists, never the window length. The window length is the wrong number and it is precisely the one Julian inferred.
• A JSON body with a stable class, so a client can branch: {"error":"too_many_sessions","retry_after_seconds":N}. The refusal happens in HTTP middleware ahead of rmcp, so this is the layer that can carry it.
• The message says what is exhausted and that it refills one at a time, because "try again later" is what produced the wrong model.

To verify rather than assume, while in there

check consumes the bearer cell before testing the sub bucket:

let bearer_bucket = get_or_insert_with_quota(&self.bearer, bearer_hash, self.quota)?;
if bearer_bucket.check().is_err() { return Err(RateLimited); }
if let Some(s) = sub { ... }

governor's check() consumes a cell on success, so a refusal caused by the sub bucket has already spent one from the bearer bucket. Reading the code that appears to make repeated attempts drain the bearer allowance faster than the rule advertises. Stated as something to test, not as a finding — the test is two attempts against a sub bucket that is already empty, asserting the bearer bucket's remaining capacity.

The general form, for AGENTS.md

A limiter that is right and silent produces a user retrying blind against a correct rule, and every retry looks to them like the rule being broken.

Which is the day's subject pointed at a person rather than at a check: a correct refusal that carries no information is the same object as a green that carries none. Both are outcomes indistinguishable from the outcome they are not.

The entry belongs with the fix, not after it, because the next limiter written in this repo will have the same shape.

Reported by Clark, who hit it while blocked by it. Filed rather than fixed: development is paused until 19:00 and the DAV server's rate limiting is separately unexplained. **This goes ahead of #16** — it has an observed victim. ## What happened Julian's claude.ai session reconnected repeatedly, exhausted the initialize allowance, never reached a tool call, and concluded the window *"needs real time to reset"*. Five failed attempts, and a wrong model of the rule at the end of them. **The limiter behaved correctly.** Nothing here proposes changing it. ## The numbers, read from this repository rather than relayed ```rust // src/main.rs let initialize_limiter = Arc::new(InitializeLimiter::new( session::SESSION_KEEP_ALIVE, // 30 * 60 MAX_INITIALIZES_PER_IDENTITY, // 8 )); ``` `InitializeLimiter::new(replenish_1_per, burst)` builds `Quota::with_period(30 min).allow_burst(8)`. So: eight back-to-back, then **one slot every 30 minutes**, and four hours to refill from empty. That is worse than "the window resets", and wrong in the direction that costs the user. Someone who waits out what they believe is a 30-minute window gets exactly **one** session, reconnects twice, and is refused again — which reads as the rule being broken rather than obeyed. Two buckets, keyed on `sha256(bearer)[..16]` and the Logto `sub`. That is deliberate and it is why this caught a client minting a fresh token per session: a new bearer gets a fresh bucket, the `sub` bucket does not. The header comment already names this case. ## The defect `src/main.rs`: ```rust return ( StatusCode::TOO_MANY_REQUESTS, "too many MCP initialize requests; try again later\n", ).into_response(); ``` No `Retry-After`. No machine-readable class. A plain-text body whose entire actionable content is *"later"*. A client cannot distinguish **too many sessions** from **your request was rejected**, and cannot know when to try again. So it retries, and every retry against a correct rule looks to the person behind it like the rule being broken. **The number needed is already computed and then thrown away.** `governor` returns `NotUntil`, which carries `wait_time_from(now)`; `InitializeLimiter::check` maps it to a unit struct: ```rust if bearer_bucket.check().is_err() { return Err(RateLimited); } ``` This is not "we cannot know when to retry". It is knowing and discarding it at the boundary. ## Proposed shape, subject to review - `check` returns the wait, not a unit — `Err(RateLimited { retry_after: Duration })` from `NotUntil::wait_time_from`. - The 429 carries `Retry-After` with **that** number: seconds until *one* slot exists, never the window length. The window length is the wrong number and it is precisely the one Julian inferred. - A JSON body with a stable class, so a client can branch: `{"error":"too_many_sessions","retry_after_seconds":N}`. The refusal happens in HTTP middleware ahead of rmcp, so this is the layer that can carry it. - The message says what is exhausted and that it refills one at a time, because "try again later" is what produced the wrong model. ## To verify rather than assume, while in there `check` consumes the bearer cell **before** testing the `sub` bucket: ```rust let bearer_bucket = get_or_insert_with_quota(&self.bearer, bearer_hash, self.quota)?; if bearer_bucket.check().is_err() { return Err(RateLimited); } if let Some(s) = sub { ... } ``` `governor`'s `check()` consumes a cell on success, so a refusal caused by the `sub` bucket has already spent one from the bearer bucket. Reading the code that appears to make repeated attempts drain the bearer allowance faster than the rule advertises. **Stated as something to test, not as a finding** — the test is two attempts against a `sub` bucket that is already empty, asserting the bearer bucket's remaining capacity. ## The general form, for `AGENTS.md` > A limiter that is right and silent produces a user retrying blind against a correct rule, and every retry looks to them like the rule being broken. Which is the day's subject pointed at a person rather than at a check: **a correct refusal that carries no information is the same object as a green that carries none.** Both are outcomes indistinguishable from the outcome they are not. The entry belongs with the fix, not after it, because the next limiter written in this repo will have the same shape.

jlxq0 commented 2026-08-26 10:11:40 +00:00

Author

Owner

Copy link

What this limiter protects, since the answer decides the number

Not Stalwart. It protects this server's own session table.

// src/session.rs
pub const MAX_SESSIONS: usize = 256;

CappedSessionManager rejects create_session once 256 sessions are live, and the initialize limiter exists so one authenticated identity cannot reach that cap alone. Stalwart is shielded by the read and write limiters at 60 and 30 per minute, which are a different resource and unaffected by any change here. So the initialize limiter is not redundant — but its correct value is a function of MAX_SESSIONS, and that has not been part of the discussion.

The arithmetic the proposed raise implies

	identities needed to exhaust the 256-session pool at full burst
burst 8, today	32
burst 32, proposed	8

Raising the burst fourfold divides the flood threshold fourfold. With refill at one per minute, one identity also sustains 60 initializes an hour indefinitely, and since sessions idle out after 30 minutes its steady-state holding is roughly 30 concurrent sessions — about an eighth of the pool, per identity, without bursting at all.

That may be entirely fine: this deployment has a handful of users, not eight hundred. But it should be decided rather than arrived at, and the decision is "how many identities do we need to be safe against" rather than "what number unblocks Julian". If eight is too few, MAX_SESSIONS moves with the burst, and it is the same one-line environment change.

One thing worth noticing while here

The refusal at the deeper guard is more useful than the one at the shallower:

// src/session.rs
"session cap reached ({MAX_SESSIONS} sessions active); try again later"

That names the resource and its size. The initialize limiter, which fires first and is therefore the one a user actually meets, says only "too many MCP initialize requests; try again later". The guard nobody reached reports better than the guard everybody hits.

Ordering for the change

Per Alan, and recorded here so the sequence is not re-litigated tomorrow:

1. tracing::warn! on the refusal branch, first and alone. No 429 from this limiter has been directly observed — the diagnosis is inference from a logged absence. It is a good chain, and a good chain is not an observation. If the warn lands and Julian is not hitting this branch, the raise is treating the wrong thing.
2. Retry-After from NotUntil::wait_time_from, never the window length.
3. A JSON-RPC error object with a distinguishable code.
4. The numbers, as environment variables rather than constants, with the MAX_SESSIONS interaction above settled in the same PR.

Shipping 4 without 1–3 is the same afternoon at a higher number.

## What this limiter protects, since the answer decides the number **Not Stalwart.** It protects this server's own session table. ```rust // src/session.rs pub const MAX_SESSIONS: usize = 256; ``` `CappedSessionManager` rejects `create_session` once 256 sessions are live, and the initialize limiter exists so one authenticated identity cannot reach that cap alone. Stalwart is shielded by the read and write limiters at 60 and 30 per minute, which are a different resource and unaffected by any change here. So the initialize limiter is **not redundant** — but its correct value is a function of `MAX_SESSIONS`, and that has not been part of the discussion. ## The arithmetic the proposed raise implies | | identities needed to exhaust the 256-session pool at full burst | |---|---| | burst 8, today | 32 | | burst 32, proposed | **8** | Raising the burst fourfold divides the flood threshold fourfold. With refill at one per minute, one identity also sustains 60 initializes an hour indefinitely, and since sessions idle out after 30 minutes its steady-state holding is roughly 30 concurrent sessions — about an eighth of the pool, per identity, without bursting at all. That may be entirely fine: this deployment has a handful of users, not eight hundred. But it should be **decided** rather than arrived at, and the decision is "how many identities do we need to be safe against" rather than "what number unblocks Julian". If eight is too few, `MAX_SESSIONS` moves with the burst, and it is the same one-line environment change. ## One thing worth noticing while here The refusal at the deeper guard is more useful than the one at the shallower: ```rust // src/session.rs "session cap reached ({MAX_SESSIONS} sessions active); try again later" ``` That names the resource and its size. The initialize limiter, which fires *first* and is therefore the one a user actually meets, says only "too many MCP initialize requests; try again later". The guard nobody reached reports better than the guard everybody hits. ## Ordering for the change Per Alan, and recorded here so the sequence is not re-litigated tomorrow: 1. **`tracing::warn!` on the refusal branch, first and alone.** No 429 from this limiter has been directly observed — the diagnosis is inference from a logged absence. It is a good chain, and a good chain is not an observation. If the warn lands and Julian is not hitting this branch, the raise is treating the wrong thing. 2. `Retry-After` from `NotUntil::wait_time_from`, never the window length. 3. A JSON-RPC error object with a distinguishable code. 4. The numbers, as environment variables rather than constants, with the `MAX_SESSIONS` interaction above settled in the same PR. Shipping 4 without 1–3 is the same afternoon at a higher number.

jlxq0 commented 2026-08-26 10:12:14 +00:00

Author

Owner

Copy link

Four additions from Clark, recorded here rather than left in a channel

1. The disposal branch is already closed. Clark's rule is right — a redundant limiter that has cost a user an afternoon should be removed rather than retuned — but it does not apply: this limiter does not shield Stalwart. It bounds session::MAX_SESSIONS = 256, this server's own session table, and Stalwart is shielded separately by the read and write limiters at 60 and 30 per minute. Recorded so tomorrow does not re-open a settled question.

The live branch is the other one: 32 concurrent is only cheap if a session is small, and nobody has measured one. "8 → 32 is fine" is a claim about a number that does not exist yet. Measure the per-session footprint before accepting the burst, alongside the MAX_SESSIONS arithmetic above — eight identities at full burst fill the pool.

2. Retry-After comes from the bucket, never from the constants. A value computed from SESSION_KEEP_ALIVE and the burst drifts from the quota the first time the quota changes, and the quota changes in this same PR. NotUntil::wait_time_from is the source. This is the same fault as the original defect pointed at its own fix: a number that is available being re-derived from something that only agrees today.

3. Provoke the warn line on beta before the raise ships anywhere. caldav-mcp-beta has logged two lines in total since 08:21:50Z — verified:

caldav-mcp listening (public)
caldav-mcp metrics listening (internal)

A 429 against that background is unmissable and costs nobody anything. This is the only cheap chance to observe the refusal rather than infer it, and the whole diagnosis currently rests on inference from a logged absence.

4. The connector's session reuse is the actual bug and survives the raise. Filed separately as #21. Twenty-four events costing twenty-four initializes means a fresh session per call, and raising the ceiling does not fix that — it moves where it bites. The issue exists so the next person meeting a limit at 32 finds the reason rather than raising it to 64.

Attribution for the PR body

The sentence, verbatim, is Clark's Infrastructure lead's, from the investigation, and is an observation rather than editorial:

it measured a limiter that was not firing while the one that was firing recorded nothing

That is why rate_limited 0 was true and useless across twelve hours, and it is the reason src/metrics.rs gains an HTTP status counter in this change: the metric that would have shown this was the one nobody had.

Order

1. tracing::warn! on the refusal branch, alone
2. provoke it on beta and record the line
3. the raise, with Retry-After from the bucket, a distinguishable JSON-RPC error code, the numbers as environment variables, and the HTTP status metric

Report the resulting pod digest to Clark as well as to Alan; Clark verifies it against the registry.

## Four additions from Clark, recorded here rather than left in a channel **1. The disposal branch is already closed.** Clark's rule is right — a redundant limiter that has cost a user an afternoon should be removed rather than retuned — but it does not apply: this limiter does not shield Stalwart. It bounds `session::MAX_SESSIONS = 256`, this server's own session table, and Stalwart is shielded separately by the read and write limiters at 60 and 30 per minute. Recorded so tomorrow does not re-open a settled question. The live branch is the other one: **32 concurrent is only cheap if a session is small, and nobody has measured one.** "8 → 32 is fine" is a claim about a number that does not exist yet. Measure the per-session footprint before accepting the burst, alongside the `MAX_SESSIONS` arithmetic above — eight identities at full burst fill the pool. **2. `Retry-After` comes from the bucket, never from the constants.** A value computed from `SESSION_KEEP_ALIVE` and the burst drifts from the quota the first time the quota changes, **and the quota changes in this same PR**. `NotUntil::wait_time_from` is the source. This is the same fault as the original defect pointed at its own fix: a number that is available being re-derived from something that only agrees today. **3. Provoke the warn line on beta before the raise ships anywhere.** `caldav-mcp-beta` has logged **two lines in total** since `08:21:50Z` — verified: ```text caldav-mcp listening (public) caldav-mcp metrics listening (internal) ``` A 429 against that background is unmissable and costs nobody anything. This is the only cheap chance to *observe* the refusal rather than infer it, and the whole diagnosis currently rests on inference from a logged absence. **4. The connector's session reuse is the actual bug and survives the raise.** Filed separately as #21. Twenty-four events costing twenty-four initializes means a fresh session per call, and raising the ceiling does not fix that — it moves where it bites. The issue exists so the next person meeting a limit at 32 finds the reason rather than raising it to 64. ## Attribution for the PR body The sentence, verbatim, is **Clark's Infrastructure lead's**, from the investigation, and is an observation rather than editorial: > it measured a limiter that was not firing while the one that was firing recorded nothing That is why `rate_limited 0` was true and useless across twelve hours, and it is the reason `src/metrics.rs` gains an HTTP status counter in this change: the metric that would have shown this was the one nobody had. ## Order 1. `tracing::warn!` on the refusal branch, alone 2. provoke it on beta and record the line 3. the raise, with `Retry-After` from the bucket, a distinguishable JSON-RPC error code, the numbers as environment variables, and the HTTP status metric Report the resulting pod digest to Clark as well as to Alan; Clark verifies it against the registry.

jlxq0 referenced this issue 2026-08-26 10:12:33 +00:00

upstream: the MCP connector appears to open a fresh session per tool call #21

jlxq0 commented 2026-08-26 10:14:09 +00:00

Author

Owner

Copy link

Decided, so tomorrow starts here

The raise as originally specified was a regression and would have been invisible in the diff:

burst  8  ->  32 identities fill MAX_SESSIONS = 256
burst 32  ->   8 identities fill it

It trades a limit that refuses one churning user for one that lets eight of them lock everybody out — a per-user refusal converted into global exhaustion, in the direction nobody notices, because the person it breaks is not the person who triggered it.

Rule: the burst and the pool move together or not at all.

Decision (Alan): MAX_SESSIONS to 1024 with burst at 32, preserving today's 32-identities-to-fill ratio. Keeping the ratio the current design chose rather than inventing a new number.

Conditional on the measurement below, which is now gating rather than a nicety. If a session is large enough that 1024 is not free, keep the ratio and cap on memory instead. Either way the PR states the number, because a capacity number defended by a feeling is the same object as a threshold nobody derived.

Fallback if measuring proves expensive: burst 24, MAX_SESSIONS unchanged. Covers the 24 events exactly, takes 10 identities to fill rather than 8, and the one-per-minute refill still does the work. No pool change, no measurement.

How to measure a session's footprint, cheaply

Beta is the right instrument for the same reason it is the right place for the warn line: it has logged two lines since 08:21:50Z and serves nothing, so any delta is attributable.

1. Record container_memory_working_set_bytes for the app container on caldav-mcp-beta, with no sessions live.
2. Open N sessions — N = 32 is enough to be above noise — and leave them idle, because idle is the state a session spends almost all of its life in and the only one 1024 of them would ever be in simultaneously.
3. Record the metric again. Divide the delta by N.

Measuring the pod rather than summing struct sizes is deliberate: a session retains rmcp's own session state and our SessionIdentityBindings entry, and the pod metric captures both without me having to enumerate what I think a session holds. Enumerating is how the estimate ends up defending the number I already wanted.

The result multiplies to a memory figure for 1024 that goes in the PR next to the pod's actual limit, and the decision is then arithmetic rather than judgement.

Order tomorrow

1. tracing::warn! on the refusal branch, alone
2. provoke it on beta and record the line — the diagnosis currently rests on inference from a logged absence
3. measure a session's footprint as above
4. burst and pool together, per the decision or the fallback
5. Retry-After from wait_time_from, a distinguishable JSON-RPC error code, the numbers as environment variables, and the HTTP status counter in src/metrics.rs

Then #16.

## Decided, so tomorrow starts here The raise as originally specified was a regression and would have been invisible in the diff: ```text burst 8 -> 32 identities fill MAX_SESSIONS = 256 burst 32 -> 8 identities fill it ``` It trades a limit that refuses one churning user for one that lets eight of them lock everybody out — a per-user refusal converted into global exhaustion, in the direction nobody notices, because the person it breaks is not the person who triggered it. **Rule: the burst and the pool move together or not at all.** **Decision (Alan):** `MAX_SESSIONS` to **1024** with burst at **32**, preserving today's 32-identities-to-fill ratio. Keeping the ratio the current design chose rather than inventing a new number. **Conditional on the measurement below**, which is now gating rather than a nicety. If a session is large enough that 1024 is not free, keep the ratio and cap on memory instead. Either way the PR states the number, because a capacity number defended by a feeling is the same object as a threshold nobody derived. **Fallback if measuring proves expensive:** burst **24**, `MAX_SESSIONS` unchanged. Covers the 24 events exactly, takes 10 identities to fill rather than 8, and the one-per-minute refill still does the work. No pool change, no measurement. ## How to measure a session's footprint, cheaply Beta is the right instrument for the same reason it is the right place for the warn line: it has logged two lines since `08:21:50Z` and serves nothing, so any delta is attributable. 1. Record `container_memory_working_set_bytes` for the `app` container on `caldav-mcp-beta`, with no sessions live. 2. Open N sessions — N = 32 is enough to be above noise — and **leave them idle**, because idle is the state a session spends almost all of its life in and the only one 1024 of them would ever be in simultaneously. 3. Record the metric again. Divide the delta by N. Measuring the pod rather than summing struct sizes is deliberate: a session retains rmcp's own session state *and* our `SessionIdentityBindings` entry, and the pod metric captures both without me having to enumerate what I think a session holds. Enumerating is how the estimate ends up defending the number I already wanted. The result multiplies to a memory figure for 1024 that goes in the PR next to the pod's actual limit, and the decision is then arithmetic rather than judgement. ## Order tomorrow 1. `tracing::warn!` on the refusal branch, alone 2. provoke it on beta and record the line — the diagnosis currently rests on inference from a logged absence 3. measure a session's footprint as above 4. burst and pool together, per the decision or the fallback 5. `Retry-After` from `wait_time_from`, a distinguishable JSON-RPC error code, the numbers as environment variables, and the HTTP status counter in `src/metrics.rs` Then #16.

jlxq0 referenced this issue from a commit 2026-08-26 14:02:18 +00:00

feat(mcp): log initialize refusals, and say which bucket refused

jlxq0 referenced this issue 2026-08-26 14:02:36 +00:00

feat(mcp): log initialize refusals, and say which bucket refused #22

jlxq0 referenced this issue from a commit 2026-08-26 14:20:57 +00:00

feat(mcp): make the initialize limiter observable, and refill in minutes

jlxq0 referenced this issue from a pull request that will close it, 2026-08-26 14:21:26 +00:00

feat(mcp): make the initialize limiter observable, and refill in minutes #23

jlxq0 closed this issue 2026-08-26 14:25:04 +00:00

jlxq0 referenced this issue 2026-08-26 14:25:52 +00:00

chore(release): prepare v0.2.0 #24

jlxq0 referenced this issue from a commit 2026-08-26 16:00:54 +00:00

feat(auth): log the X-Forwarded-For entry count, never the entries

Sign in to join this conversation.

No Branch/Tag specified

Branches Tags

main

beta

v0.3.0

v0.2.2

v0.2.1

v0.2.0

v0.1.2-beta.20260826052605

v0.1.2

v0.1.1

v0.1.0

Labels

Clear labels   

blocked

Held on something external and named in the body: an issue, a credential, a measurement. Not the same as unreviewed.

  

waiting-on-julian

Blocked on Julian. The ask is in the issue.

No labels

blocked

waiting-on-julian

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

jlxq0/caldav-mcp#20

No description provided.