Stop leaking connection slots and reclaim unused sessions (v1.6.3) - #7
Merged
Merged
Conversation
A client that opened 14 sessions in 51 seconds and made one tool call met
"Connection limit exceeded" on everything after. The 20-slot pool per API
token was correct; the accounting around it was not.
- A POST /mcp without a session ID acquired a slot before the transport
opened. If the request never became a session — malformed body, aborted
client, transport error — `onclose` never fired and the session was never
registered with the idle sweeper, so the slot was unreachable until the
process restarted. Released from `res.on('close')` now, and the
activeSessions gauge no longer decrements for a session that never was.
- A session that initialized and was then abandoned held its slot for the
full 10-minute idle timeout. Sessions that have never made a call are now
reclaimed after 30 s; sessions doing real work keep the old timeout.
- getClientIp read the leftmost X-Forwarded-For entry. Caddy appends the
real peer to whatever the client sent, so that entry was attacker
controlled: a client could pick its own rate-limit bucket, evade the
per-IP limit, or poison the bucket another tenant was counted in. It now
reads the rightmost hop, and Caddy overwrites the header rather than
appending to it.
- "Connection limit exceeded" logged only an IP, which is the proxy's for
everyone. It now carries a token fingerprint plus active/limit, so the
line says whose pool is full and how full.
The limit itself stays at 20.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Tn4FT2sVqoFz1QdpNDNAy7
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Was war los
Aus dem Produktions-Log: ein Client öffnete 14 Sessions in 51 Sekunden und machte dabei genau einen Tool-Call (
list_tables). Danach kam für jeden weiteren VersuchConnection limit exceeded. Das Limit von 20 gleichzeitigen Verbindungen pro API-Token war nicht das Problem — die Buchhaltung drumherum war es.Vier Defekte
1. Slots, die ein fehlgeschlagener Init nie zurückgab (der schwerwiegendste)
POST /mcpohne Session-ID belegt einen Slot, bevor der Transport geöffnet wird. Wird daraus keine Session — fehlerhafter Body, abgebrochener Client, Transport-Fehler — feuerttransport.onclosenie und die Session landet nie im Map des Idle-Sweepers. Der Slot war damit bis zum Prozess-Neustart unerreichbar. Jeder abgebrochene Init fraß dauerhaft 1 von 20.Wird jetzt über
res.on('close')freigegeben. DieactiveSessions-Gauge zählt außerdem nicht mehr für eine Session herunter, die es nie gab.2. Verwaiste Sessions blockierten 10 Minuten
Eine Session, die initialisiert und dann liegen gelassen wird, hielt ihren Slot für den vollen Idle-Timeout. Genau dieses Muster produziert der beobachtete Client. Sessions ohne je einen Call werden jetzt nach 30 s eingesammelt (
unusedSessionTimeoutMs); Sessions, die arbeiten, behalten die 10 Minuten.3.
X-Forwarded-Forwar client-kontrolliert (sicherheitsrelevant)getClientIp()las den linkesten XFF-Eintrag. Caddy hängt den echten Peer an das an, was der Client geschickt hat — der linkeste Eintrag ist also frei wählbar. Damit konnte ein Client sich seinen eigenen Rate-Limit-Bucket aussuchen, das Per-IP-Limit durch Rotieren des Headers umgehen, oder gezielt den Bucket vergiften, in dem ein anderer Mandant gezählt wird. Das erklärt auch die172.18.0.1im Log, wo eine echte Client-IP stehen sollte.Gelesen wird jetzt der rechteste Hop — der, den unser eigener Proxy angehängt hat. Zusätzlich überschreibt Caddy den Header jetzt, statt anzuhängen (
header_up), sodass er genau einen Hop trägt.4. Das Limit-Log war unbrauchbar
Connection limit exceededloggte nur eine IP — und die ist hinter dem Proxy für alle dieselbe. Die Zeile trägt jetzt Token-Fingerprint,activeundlimit, beantwortet also "wessen Pool ist voll und wie voll".Nicht geändert
Das Limit bleibt bei 20. Mit dem 30-Sekunden-Timeout hat es wieder echten Spielraum: der beobachtete Burst von 14 Sessions/51 s belegt damit nur noch einen Bruchteil des Pools.
Tests
tests/connectionSlots.spec.ts, 7 Tests, zuerst rot geschrieben — Test 1 reproduziert das Leak exakt (21 abgelehnte Inits → Pool leer → 429 für den ehrlichen Client danach). Volle Suite: 329/329 grün,typecheckundlintsauber.🤖 Generated with Claude Code
https://claude.ai/code/session_01Tn4FT2sVqoFz1QdpNDNAy7