Skip to content

[BUG] with_max_clients bounds subscriptions per connection, not the client count #216

Description

@lxsaah

Summary

WebSocketConnectorBuilder::with_max_clients(max) does not limit clients. It
sets max_subs_per_connection — the per-connection subscription ceiling. The
connector's own doc example uses it in a position where it reads as a connection
cap, so an operator reaching for the obvious DoS control gets something else and
leaves connection count unbounded.

The mechanism

Paths below are relative to aimdb-websocket-connector/src/.

// server/builder.rs:352
max_subs_per_connection: self.max_clients.max(1),

The rustdoc is candid about it (server/builder.rs:162-166):

Set the per-connection subscription ceiling (default: 1 024).
Despite the name, this bounds live subscriptions per connection
(max_subs_per_connection), not the client count — connection count is the
axum accept loop's concern, not enforced here.

But the builder's own doc example (server/builder.rs:60-65) presents it
alongside bind/path/late-join, where it reads unambiguously as a client cap:

let connector = connector
    .bind("0.0.0.0:8080")
    .path("/ws")
    .with_late_join(true)
    .with_max_clients(500);

Nothing else caps connections on this transport. SessionLimits::max_connections
exists and defaults to 16, enforced in the generic accept loop
(aimdb-core/src/session/server.rs:424), but the ws path opts out:

// server/http.rs:199
max_connections: usize::MAX, // axum owns the accept loop

The live connection count is tracked — ClientManager.connections
(server/client_manager.rs:49), incremented by connection_guard() (:77-82)
and decremented on drop (:162-166), with a public client_count() (:72-74).
It is simply never acted on: the only consumer is the /health endpoint
(server/http.rs:220).

So the concept exists in the session engine, the counter exists in the connector,
and the one builder method that sounds like it connects them does something
else.

Impact

An operator configuring .with_max_clients(500) believes connections are capped
at 500. They are unlimited; what is capped is how many subscriptions each of them
may hold. The failure mode is silent — no error, no log — and visible only by
reading a rustdoc that contradicts both the method name and the example directly
above it.

The two limits differ in every respect that matters:

max_subs_per_connection (what the method sets) a client cap (what it sounds like)
Bounds live subscriptions within one session concurrent connections to the server
Counted by cancels.len() in the AimX session loop ClientManager.connections
Enforced at aimdb-core/src/session/server.rs:203-206 the HTTP upgrade
On violation Denied reply to that sub; connection stays up HTTP rejection; no WebSocket established
Scope per connection server-wide
Default 1024 (server/builder.rs:95) none

Fix

Keep the name and make it true, rather than deprecating it — with_max_clients
is exactly the name a connection cap wants, so retiring it would mean asking for
it back.

Give with_max_clients its literal meaning. The counter is already
maintained and public; enforcing it needs a check at the upgrade handler, before
ws.on_upgrade(…) (server/http.rs:206), rejecting with 503 or 429 when over.

Name the current behaviour separately. Add
with_max_subs_per_connection for the subscription ceiling, which today has no
correctly-named setter at all, and correct the doc example at
server/builder.rs:60-65.

This splits one field into two, so each needs a default. The subscription ceiling
should keep 1024 (server/builder.rs:95), leaving today's behaviour untouched
for anyone who never called the method. The connection cap's default is the real
decision: unlimited preserves current behaviour and makes the cap opt-in, while
any finite default would silently cap deployments that legitimately run more.
Core's SessionLimits::max_connections default of 16
(aimdb-core/src/session/mod.rs:214) is not a useful precedent for a browser-facing
server.

The change is silent for callers, so it needs a loud CHANGELOG entry.
.with_max_clients(50) today means "≤50 subscriptions per connection, unlimited
connections"; afterwards it means "≤50 connections, ≤1024 subscriptions per
connection". That tightens the bound that was unbounded and loosens the one that
was set — in opposite safety directions, with nothing failing to compile. The
affected set is small and nameable: only callers who passed a non-default value.
The entry should say to write .with_max_subs_per_connection(50) to keep the old
meaning.

One ordering detail: the guard is currently acquired at session construction
(server/dispatch.rs:85), which runs inside the on_upgrade closure — after
the upgrade is accepted. A plain if client_count() >= max at the handler
therefore reads a count that has not yet incremented for in-flight upgrades, and
a burst can overshoot. The clean form is a
try_connection_guard(max) -> Option<ConnectionGuard> doing check-and-increment
in one fetch_update, called at the handler and passed into the session. The
tree already uses that idiom for the same shape of problem — fetch_update +
checked_add in ClientHandle::next_id
(aimdb-core/src/session/client.rs:243).

Test

With with_max_clients(n) configured, open n connections, assert the next is
refused at the HTTP layer without a WebSocket being established, then drop one
and assert a new one is accepted — that last step pins the Drop decrement
(server/client_manager.rs:162-166), which nothing exercises today. Separately,
assert with_max_subs_per_connection(n) refuses the n + 1-th subscription on a
single connection with Denied while leaving the connection usable, and that the
two limits are independent — setting one does not move the other, which is the
defect this issue is about.

Environment

  • Present on the current main branch — not a regression introduced by
    PR Feat/retire aimdb ws protocol #201. Verified at merge-base 625736c: the mapping to
    max_subs_per_connection, the "Despite the name" rustdoc and the misleading
    doc example are all already there.
  • Found by inspection while following up the Feat/retire aimdb ws protocol #201 review; not observed in the
    wild.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working🔌 bridgesProtocol bridges

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions