feat(Push): add Appwrite Push (MQTT 5) adapter - #129
Conversation
Greptile SummaryThis PR adds a new Appwrite Push adapter that publishes MQTT 5 notifications to a self-hosted broker, along with a pure-PHP MQTT 5 codec (
Confidence Score: 3/5
Important Files Changed
Prompt To Fix All With AI### Issue 1
src/Utopia/Messaging/Adapter/Push/Appwrite.php:151-156
**Unsent tokens silently absent from response on socket failure**
When `readPacket` throws (broker timeout, EOF, or disconnect) only the currently-inflight tokens are recorded as failures. Any tokens with index `$cursor` through `$total - 1` that have not yet been sent are never added to `$response`, so they are invisible to the caller — they appear neither as successes nor failures. With a 5,000-token fan-out and a broker that closes the socket after the first window, ~4,744 tokens would silently vanish from the returned results array.
The fix is to also record the unsent remainder before returning:
```php
} catch (\Throwable $error) {
foreach ($inflight as $token) {
$response->addResult($token, $error->getMessage());
}
// Mark tokens that were never attempted.
for ($i = $cursor; $i < $total; $i++) {
$response->addResult($tokens[$i], $error->getMessage());
}
return;
}
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (6): Last reviewed commit: "feat(Push): add Appwrite Push (MQTT 5) a..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
|
|
||
| public function getMaxMessagesPerRequest(): int | ||
| { |
There was a problem hiding this comment.
$this->readBuffer is never reset at the start of each connection. If the adapter instance is reused (e.g., send() is called twice), or if the broker sends an extra packet after the last PUBACK (e.g., a PINGREQ that landed in the buffer just before disconnect), that residual data persists into the next call. On the next invocation readPacket() would immediately return the leftover packet as if it were the new connection's CONNACK, causing handshake() to throw "Broker did not respond with CONNACK" even on a healthy connection.
Add $this->readBuffer = ''; at the start of connect() or at the top of process() to isolate each connection's read state.
|
|
||
| private function resolveEndpoint(): string | ||
| { | ||
| $endpoint = \rtrim($this->endpoint); |
There was a problem hiding this comment.
rtrim strips only trailing whitespace, so a leading space in the configured endpoint (e.g., " broker.example.com") would produce a malformed URL like tls:// broker.example.com:8883 that stream_socket_client rejects. Use trim to strip both ends.
| $endpoint = \rtrim($this->endpoint); | |
| $endpoint = \trim($this->endpoint); |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| $packet = MQTT::decodePacket($this->readBuffer); | ||
| if ($packet !== null) { | ||
| return $packet; | ||
| } |
There was a problem hiding this comment.
$this->receiveMaximum is instance state that is only ever updated via min() in handshake(). If the adapter is reused across multiple send() calls and the broker advertises a low receiveMaximum (say 10) on the first call, subsequent connections — even to a different broker endpoint — will be throttled to that minimum permanently for the lifetime of the object. Resetting it to the class-default (or to 65535) at the start of each connect() would make each connection's window independent.
|
Following up - this PR has been open for 1 month. Let me know if any changes are needed or if the implementation approach needs adjustment. |
Based on PR utopia-php#122 by abnegate. Adds Appwrite Push - a self-hosted MQTT 5 based push notification adapter with minimal MQTT 5 control-packet codec.
| } catch (\Throwable $error) { | ||
| foreach ($inflight as $token) { | ||
| $response->addResult($token, $error->getMessage()); | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
When readPacket throws (broker timeout, EOF, or disconnect) only the currently-inflight tokens are recorded as failures. Any tokens with index $cursor through $total - 1 that have not yet been sent are never added to $response, so they are invisible to the caller — they appear neither as successes nor failures. With a 5,000-token fan-out and a broker that closes the socket after the first window, ~4,744 tokens would silently vanish from the returned results array.
The fix is to also record the unsent remainder before returning:
} catch (\Throwable $error) {
foreach ($inflight as $token) {
$response->addResult($token, $error->getMessage());
}
// Mark tokens that were never attempted.
for ($i = $cursor; $i < $total; $i++) {
$response->addResult($tokens[$i], $error->getMessage());
}
return;
}Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Utopia/Messaging/Adapter/Push/Appwrite.php
Line: 151-156
Comment:
**Unsent tokens silently absent from response on socket failure**
When `readPacket` throws (broker timeout, EOF, or disconnect) only the currently-inflight tokens are recorded as failures. Any tokens with index `$cursor` through `$total - 1` that have not yet been sent are never added to `$response`, so they are invisible to the caller — they appear neither as successes nor failures. With a 5,000-token fan-out and a broker that closes the socket after the first window, ~4,744 tokens would silently vanish from the returned results array.
The fix is to also record the unsent remainder before returning:
```php
} catch (\Throwable $error) {
foreach ($inflight as $token) {
$response->addResult($token, $error->getMessage());
}
// Mark tokens that were never attempted.
for ($i = $cursor; $i < $total; $i++) {
$response->addResult($tokens[$i], $error->getMessage());
}
return;
}
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
1 participant
Port of PR #122 by abnegate.
Adds Appwrite Push - a self-hosted, low-power alternative to FCM/APNS that publishes notifications over MQTT 5 to per-device topics.
Changes: