Repository navigation
fix(realtime): prevent concurrent presence writes on one socket from leaking a hold - #14724
ArnabChatterjee20k wants to merge 2 commits into
Conversation
…king a hold Two overlapping presence messages on the same connection both read no hold before either writes it back, so both increment the row's connections count while onClose releases the hold only once -- the socket stays online after it disconnects. Stake a transient per-connection claim (presenceClaim) synchronously before the counting write yields. Swoole coroutines switch only on I/O, so a concurrent presence message on the socket observes the claim and refreshes instead of counting the socket twice. The claim is promoted to the real hold on success, dropped on failure so a retry can still count, and preserved across a re-subscribe. onClose is untouched (it only ever sees a completed hold). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
🔵 Tier A · Mergeable after minor fixes
This pull request adds a per-connection presence lock so overlapping presence writes on one socket serialize, allowing later writes to refresh the existing hold rather than incrementing the connection count again. The lock is retained when the connection subscribes again, and the handler still reconciles a write that finishes after the socket closes. Latest changes: The newest commits replace the earlier skip-ack claim with a channel-based mutex, so overlapping frames now run their full presence upsert in sequence; the lock is also added to the connection state retained by subscribe.
Note Not approving while a bug finding is open: Authentication can drop the in-flight presence lock
Fix with agent prompt### Issue 1
src/Appwrite/Messaging/Adapter/Realtime.php:202
**Authentication can drop the in-flight presence lock**
`Authentication::action()` calls `unsubscribe()` before resubscribing, so this whitelist cannot preserve the lock through that path; it only snapshots/restores `presence`. If authentication interleaves with a presence upsert, it can recreate the connection without this lock, letting another presence frame create a second mutex and count the same socket concurrently. Carry the lock through the authentication rebind as well.
### Issue 2
src/Appwrite/Realtime/Message/Handlers/Presence.php:82-88
**Add an overlapping-write regression test**
The existing realtime presence tests cover sequential frames, but none exercises two overlapping presence writes on one socket and then checks the close/offline outcome. That leaves the new mutex behavior—and the authentication rebind case—without regression coverage; an E2E test can use the real WebSocket path rather than a mocked boundary.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.📂 Walkthrough · 2
✅ Fixed since the last review · 1
Reviewed the commits since |
![hansi-codes[bot]](https://avatars.githubusercontent.com/in/5046327?s=60&v=4){kind=small}
Security rulesNo new WARNING or ERROR findings from security rules. 19 existing findings tracked in
|
![tenki-reviewer[bot]](https://avatars.githubusercontent.com/in/2900641?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR adds a presenceClaim marker to the realtime presence handler so overlapping presence messages on the same socket refresh instead of double-counting; the fix works but the fast-path acknowledgement drops the second message's payload and can report success for a write that fails.
Key findings
- 🟠 Claim stake resurrects closed connection, bypassing close guard — Presence.php:127
- 🟡 Claim fast-path acks presence without persisting it — Presence.php:108
- 🟡 Claim-ack response shape diverges from post-write path — Presence.php:108
| if (!$holdsUser && ($realtime->connections[$connectionId]['presenceClaim'] ?? null) === $userId) { | ||
| // A sibling presence write on this socket is already establishing the hold; acknowledge | ||
| // the submitted presence without a second counting write. | ||
| foreach (['$collection', '$tenant', 'hostname', 'permissionsHash', 'userInternalId', 'connections'] as $attribute) { | ||
| $presenceDocument->removeAttribute($attribute); | ||
| } | ||
|
|
||
| return [ | ||
| 'type' => 'response', | ||
| 'data' => [ | ||
| 'to' => 'presence', | ||
| 'presence' => $presenceDocument->getArrayCopy(), | ||
| ], | ||
| ]; | ||
| } |
There was a problem hiding this comment.
🟡 bug · medium
Claim fast-path acks presence without persisting it
When a sibling presence write on the socket holds presenceClaim, the fast-path returns a success response built from the just-submitted document without writing it (src/Appwrite/Realtime/Message/Handlers/Presence.php:108-122), so the second message's status/metadata is silently dropped and no presences.[presenceId].upsert event fires for it.
The acknowledgement is also not tied to the sibling write's outcome: if that write throws (lines 131-139), the row is never written even though this message was already acknowledged as present. Clients sending rapid presence updates (heartbeat + explicit status change is the normal pattern) observe a success for state the server never recorded.
📋 Prompt for AI Agents
In src/Appwrite/Realtime/Message/Handlers/Presence.php lines 108-122, the presenceClaim fast-path returns a success response built from the submitted document without writing it, so the second overlapping presence message's status/metadata is never persisted and is acked even if the sibling write later throws (lines 131-139). Fix by deferring the response until the sibling's upsertForUserWithConnections resolves: queue the dropped payload (e.g. re-dispatch the message once the hold is stashed, or persist the second message's status/metadata with a non-counting refresh write against the held sequence), and surface the sibling's error instead of a success when the write fails.
| if (!$holdsUser && ($realtime->connections[$connectionId]['presenceClaim'] ?? null) === $userId) { | ||
| // A sibling presence write on this socket is already establishing the hold; acknowledge | ||
| // the submitted presence without a second counting write. | ||
| foreach (['$collection', '$tenant', 'hostname', 'permissionsHash', 'userInternalId', 'connections'] as $attribute) { | ||
| $presenceDocument->removeAttribute($attribute); | ||
| } | ||
|
|
||
| return [ | ||
| 'type' => 'response', | ||
| 'data' => [ | ||
| 'to' => 'presence', | ||
| 'presence' => $presenceDocument->getArrayCopy(), | ||
| ], | ||
| ]; | ||
| } |
There was a problem hiding this comment.
🟡 bug · medium
Claim-ack response shape diverges from post-write path
The claim fast-path returns the client-built $presenceDocument, which lacks the server-assigned attributes the post-write path returns from the database: $id (when presenceId is unique()), $createdAt, $updatedAt, and $sequence (src/Appwrite/Realtime/Message/Handlers/Presence.php:115-121).
A client whose message lands while a sibling presence write on the same socket is in flight receives an acknowledged presence with a structurally different payload than one received a moment later — same message type, different shape, on the same SDK surface.
📋 Prompt for AI Agents
In src/Appwrite/Realtime/Message/Handlers/Presence.php lines 108-122, the claim-ack early return responds with the raw client-built $presenceDocument, missing the server-assigned $id, $createdAt, $updatedAt and $sequence that the post-write path returns from the database. Make the acknowledged shape match the persisted response — e.g. defer the response until the sibling write resolves and echo its persisted document — keeping the existing attribute-stripping list unchanged.
| if (!$holdsUser) { | ||
| $realtime->connections[$connectionId]['presenceClaim'] = $userId; | ||
| } |
There was a problem hiding this comment.
🟠 bug · high
Claim stake resurrects closed connection, bypassing close guard
The claim stake $realtime->connections[$connectionId]['presenceClaim'] = $userId auto-vivifies a new entry if the socket closed and unsubscribe() already removed connections[$connectionId] during the getDocument I/O yield at line 74 (src/Appwrite/Realtime/Message/Handlers/Presence.php:127-129).
The mid-write-close guard !isset($realtime->connections[$connectionId]) at line 146 then observes the resurrected zombie entry and skips the releaseConnectionForUser undo path, so the counted presence row is stashed at line 174 onto an entry with no live socket and is never released. The user stays visible online until the row's 30-day expiresAt.
📋 Prompt for AI Agents
In src/Appwrite/Realtime/Message/Handlers/Presence.php at lines 126-129, wrap the claim stake in an existence check: if (!$holdsUser && isset($realtime->connections[$connectionId])) { $realtime->connections[$connectionId]['presenceClaim'] = $userId; }. Without this, a socket that closes during the getDocument I/O yield at line 74 has its connection entry removed by onClose/unsubscribe(), and the stake auto-vivifies a zombie entry that defeats the !isset(...) mid-write-close guard at line 146 — the counted presence row is then never released and the user leaks as online until the 30-day expiresAt.
✨ Benchmark resultsComparing
Per-scenario breakdown & investigation detailsMetrics below reflect the current branch (after). Δ P95 compares against the base.
Top API waits (after)
|
…cking Addresses review of the claim fast-path: skipping the second overlapping write dropped its status/metadata, fired no upsert event, returned a client-built document with no server attributes, and could falsely ack when the first write failed; staking the claim also auto-vivified a connection onClose had already removed, so the mid-write-close guard saw a zombie entry and leaked the row. Replace the claim with a per-connection Swoole\Coroutine\Channel mutex taken before the first I/O yield (while the connection is known to exist, so it never resurrects a closed entry). Overlapping presence messages on one socket now run one at a time: the second reads the first's stashed hold and takes the normal refresh path -- persisting its data, emitting its event, and returning the real database document -- instead of counting the socket twice. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| // Recorded once by the connection handler or a presence write; every later (re)subscribe | ||
| // keeps it, so a `subscribe` message after a presence message never drops the hold stash. | ||
| foreach (['authorization', 'sessionId', 'impersonatedUserId', 'impersonator', 'expire', 'jwtExpire', 'presence'] as $key) { | ||
| foreach (['authorization', 'sessionId', 'impersonatedUserId', 'impersonator', 'expire', 'jwtExpire', 'presence', 'presenceLock'] as $key) { |
There was a problem hiding this comment.
Authentication can drop the in-flight presence lock
Authentication::action() calls unsubscribe() before resubscribing, so this whitelist cannot preserve the lock through that path; it only snapshots/restores presence. If authentication interleaves with a presence upsert, it can recreate the connection without this lock, letting another presence frame create a second mutex and count the same socket concurrently. Carry the lock through the authentication rebind as well.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Appwrite/Messaging/Adapter/Realtime.php
Line: 202
Comment:
**Authentication can drop the in-flight presence lock**
`Authentication::action()` calls `unsubscribe()` before resubscribing, so this whitelist cannot preserve the lock through that path; it only snapshots/restores `presence`. If authentication interleaves with a presence upsert, it can recreate the connection without this lock, letting another presence frame create a second mutex and count the same socket concurrently. Carry the lock through the authentication rebind as well.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.🟡 Minor · concurrency · Reply if this doesn't apply.
There was a problem hiding this comment.
Add an overlapping-write regression test
The existing realtime presence tests cover sequential frames, but none exercises two overlapping presence writes on one socket and then checks the close/offline outcome. That leaves the new mutex behavior—and the authentication rebind case—without regression coverage; an E2E test can use the real WebSocket path rather than a mocked boundary.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Appwrite/Realtime/Message/Handlers/Presence.php
Line: 82-88
Comment:
**Add an overlapping-write regression test**
The existing realtime presence tests cover sequential frames, but none exercises two overlapping presence writes on one socket and then checks the close/offline outcome. That leaves the new mutex behavior—and the authentication rebind case—without regression coverage; an E2E test can use the real WebSocket path rather than a mocked boundary.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.🟡 Minor · testing · Reply if this doesn't apply.
This branch has not been deployed
1 participant
Issue
hansi flagged (on #14606): two overlapping presence messages on the same connection both read no stash before either writes it back, so both increment the row's
connectionscount. onClose then releases the hold only once, leaving the socket online after it disconnects.This is real because
Runtime::enableCoroutineis on and Swoole does not serializeonMessageper connection — the DB write insideupsertForUserWithConnectionsyields, so a second presence message on the same socket interleaves before the first stashes its hold. Trigger: a client sending a second presence write (e.g. a status change) while the first is still in flight. A single presence write per connection is unaffected.Solution
Stake a transient per-connection claim (
presenceClaim) synchronously before the counting write yields. Swoole coroutines switch only on I/O, so a concurrent presence message on the socket observes the claim and refreshes instead of counting the socket twice.presence) on success, so onClose releases exactly one count.onCloseand the close-race path are untouched — they only ever see a completed hold.Testing
Handler-level coroutine logic, so it isn't a unit-testable
Statechange. Verified by reasoning; recommend an e2e that sends two presence frames on one socket, closes it, and asserts the presence goes offline (no leak) — runnable once this is on staging.🤖 Generated with Claude Code