Repository navigation
fix(realtime): limit subscriptions per connection - #14527
HarshMN2345 wants to merge 9 commits into
Conversation
One connection could keep adding subscriptions until the realtime worker ran out of memory and dropped every other client with it. Connections now stop at 100 subscriptions and 100 channels (both configurable), a subscription's queries can be at most 4096 chars, and the subscription count is tracked as we go instead of rebuilding the whole metadata array on every message. Fixes #14430
…on-limits fix(realtime): limit subscriptions per connection
…tion-limit # Conflicts: # tests/e2e/Services/Realtime/RealtimeCustomClientTest.php # tests/unit/Messaging/MessagingTest.php
A 4096-character total per subscription is stricter than REST, which allows 4096 per query. Subscription queries stay bounded by the 64 KB message size.
Count a batch's distinct subscription IDs and channels against what the connection already holds, instead of keeping a second per-connection subscription index that every subscribe, unsubscribe and re-auth path has to maintain. Both counts are bounded by the caps, so reading the tree stays cheap.
Other realtime limits (message size, message and connect rate) are fixed values, so the subscription and channel caps follow them instead of adding two environment variables.
🟡 Tier B · Needs changes before merging
This change adds fixed per-connection limits for realtime subscriptions and distinct channels, enforcing them during URL handshakes and
Fix with agent prompt### Issue 1
src/Appwrite/Realtime/Message/Handlers/Subscribe.php:88-95
**Make the cap check safe across concurrent tail subscriptions**
Two concurrent `subscribe` messages can both pass these checks against the same pre-update state, then yield while the handler authorizes `console.tail.*` channels through the database before either batch mutates the tree. For example, with 99 existing subscriptions, two one-subscription tail batches can each pass at 100 and then leave 101 subscriptions, bypassing the limit.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.📂 Walkthrough · 5
Reviewed |
![hansi-codes[bot]](https://avatars.githubusercontent.com/in/5046327?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 Tier B · 1 blocking finding to address. Summary
| if (\count(\array_unique($subscriptionIds)) > APP_LIMIT_REALTIME_SUBSCRIPTIONS) { | ||
| throw new Exception( | ||
| Exception::REALTIME_POLICY_VIOLATION, | ||
| 'Too many subscriptions. A connection can have at most ' . APP_LIMIT_REALTIME_SUBSCRIPTIONS . ' subscriptions.' | ||
| ); | ||
| } | ||
|
|
||
| if (\count(\array_unique($channels)) > APP_LIMIT_REALTIME_CHANNELS) { |
There was a problem hiding this comment.
Make the cap check safe across concurrent tail subscriptions
Two concurrent subscribe messages can both pass these checks against the same pre-update state, then yield while the handler authorizes console.tail.* channels through the database before either batch mutates the tree. For example, with 99 existing subscriptions, two one-subscription tail batches can each pass at 100 and then leave 101 subscriptions, bypassing the limit.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Appwrite/Realtime/Message/Handlers/Subscribe.php
Line: 88-95
Comment:
**Make the cap check safe across concurrent tail subscriptions**
Two concurrent `subscribe` messages can both pass these checks against the same pre-update state, then yield while the handler authorizes `console.tail.*` channels through the database before either batch mutates the tree. For example, with 99 existing subscriptions, two one-subscription tail batches can each pass at 100 and then leave 101 subscriptions, bypassing the limit.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.🟠 Major · concurrency · Reply if this doesn't apply.
![tenki-reviewer[bot]](https://avatars.githubusercontent.com/in/2900641?s=60&v=4){kind=small}
There was a problem hiding this comment.
Adds subscription/channel limits to realtime connections, but the batch limit check races concurrent subscribe frames under Swoole and can be exceeded.
Key findings
- 🟠 Batch limit check races concurrent subscribe frames — Subscribe.php:81
| $subscriptionIds = \array_keys($existing); | ||
| $channels = $realtime->connections[$connectionId]['channels'] ?? []; | ||
| foreach ($parsedPayloads as $parsedPayload) { | ||
| $subscriptionIds[] = $parsedPayload['subscriptionId']; | ||
| $channels = [...$channels, ...$parsedPayload['convertedChannels']]; | ||
| } | ||
|
|
||
| if (\count(\array_unique($subscriptionIds)) > APP_LIMIT_REALTIME_SUBSCRIPTIONS) { | ||
| throw new Exception( | ||
| Exception::REALTIME_POLICY_VIOLATION, | ||
| 'Too many subscriptions. A connection can have at most ' . APP_LIMIT_REALTIME_SUBSCRIPTIONS . ' subscriptions.' | ||
| ); | ||
| } | ||
|
|
||
| if (\count(\array_unique($channels)) > APP_LIMIT_REALTIME_CHANNELS) { | ||
| throw new Exception( | ||
| Exception::REALTIME_POLICY_VIOLATION, | ||
| 'Too many channels. A connection can subscribe to at most ' . APP_LIMIT_REALTIME_CHANNELS . ' channels.' | ||
| ); | ||
| } |
There was a problem hiding this comment.
🟠 bug · high
Batch limit check races concurrent subscribe frames
The whole-batch limit check snapshots existing subscriptions and connection channels (src/Appwrite/Realtime/Message/Handlers/Subscribe.php:81), but the state-mutating $realtime->subscribe() calls happen later at src/Appwrite/Realtime/Message/Handlers/Subscribe.php:160 with coroutine yields (the console-DB tail authorization reads) in between. Swoole runs each websocket frame in its own coroutine and concurrent messages on one connection interleave, as documented at app/realtime.php:1233, so two back-to-back subscribe frames both validate against the same pre-batch counts and both apply. A client can thereby exceed APP_LIMIT_REALTIME_SUBSCRIPTIONS and APP_LIMIT_REALTIME_CHANNELS on a single connection, defeating the memory-bound limit this change introduces.
📋 Prompt for AI Agents
In src/Appwrite/Realtime/Message/Handlers/Subscribe.php, the limit checks at lines 81-100 validate against a snapshot taken before the handler's DB awaits (tail authorization reads around line 134), while the apply loop runs at lines 160-169 after those yields. Because Swoole runs each websocket frame in its own coroutine and concurrent frames on one connection interleave (documented at app/realtime.php), two concurrent subscribe batches can both pass the check and both apply, exceeding APP_LIMIT_REALTIME_SUBSCRIPTIONS and APP_LIMIT_REALTIME_CHANNELS. Add per-connection mutual exclusion: acquire a per-connectionId lock (e.g. a Swoole channel/mutex keyed by connectionId, registered in the message container) at the top of the handler and release it in a finally block after the subscribe loop and tail registration, so snapshot, yields, and apply are serialized per connection. Alternatively enforce the caps inside Realtime::subscribe (src/Appwrite/Messaging/Adapter/Realtime.php) against live tree state so the invariant holds regardless of caller interleaving.
✨ Benchmark resultsComparing
Per-scenario breakdown & investigation detailsMetrics below reflect the current branch (after). Δ P95 compares against the base.
Top API waits (after)
|
This branch has not been deployed
2 participants
A realtime connection can now hold at most 100 subscriptions and 100 distinct channels (
APP_LIMIT_REALTIME_SUBSCRIPTIONS,APP_LIMIT_REALTIME_CHANNELS). Both caps are checked on the URL handshake and onsubscribemessages, before any state changes. Going over sends a1008policy-violation error and closes the connection, the same as other subscribe policy violations. The SDKs don't reconnect after a1008.The channel cap is needed too: a client can reuse one subscription ID with new channels, which grows the subscription tree without adding to the subscription count. The caps are constants, like the other realtime limits (message size, message and connect rates).
This builds on #14440. I removed the per-connection subscription index, the 4096-character query limit (stricter than REST; queries are already bounded by the 64 KB message size), the two env vars, and its tests.
Verified:
pint --teston the touched filesSubscribe.phpSubscribehandler directly. A 101st subscription is rejected. Updating an existing ID at the cap is still allowed. A 101-item batch is rejected and leaves nothing behind. Reusing an ID on a 101st channel is rejected.Closes #14430
Related: #14440 (merged into this branch)