Conversation
📝 WalkthroughWalkthroughThe change adds defensive error handling to realtime event transmission in Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can use your project's `biome` configuration to improve the quality of JS/TS/CSS/JSON code reviews.Add a configuration file to your project to customize how CodeRabbit runs |
Greptile SummaryThis PR wraps the
Confidence Score: 3/5
Important Files Changed
Reviews (1): Last reviewed commit: "fix: fail open realtime publishing" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } catch (\Exception $e) { | ||
| Console::error('Realtime send failed: '.$e->getMessage()); | ||
| } |
There was a problem hiding this comment.
\Throwable instead of \Exception for true fail-open
The catch block only handles \Exception but not PHP's \Error hierarchy (e.g., TypeError, ValueError, Error). The getPubSubPool() method accesses a global $register and calls chained methods on it — if $register is null or ->get('pools') returns null, PHP 8 will throw a \TypeError (which extends \Error, not \Exception), bypassing this catch block entirely and propagating the exception upward. This defeats the stated "fail open" intent of the PR.
The standard PHP pattern for a true fail-open is to catch \Throwable, which covers both \Exception and \Error:
| } catch (\Exception $e) { | |
| Console::error('Realtime send failed: '.$e->getMessage()); | |
| } | |
| } catch (\Throwable $e) { | |
| Console::error('Realtime send failed: '.$e->getMessage()); | |
| } |
| ] | ||
| ); | ||
| } catch (\Exception $e) { | ||
| Console::error('Realtime send failed: '.$e->getMessage()); |
There was a problem hiding this comment.
Only getMessage() is captured. Without the stack trace, it can be very difficult to determine the root cause of a failure in production (e.g., which layer of the PubSub/pool code failed). Consider including getTraceAsString() or at least the exception class:
| Console::error('Realtime send failed: '.$e->getMessage()); | |
| Console::error('Realtime send failed: ' . get_class($e) . ': ' . $e->getMessage() . "\n" . $e->getTraceAsString()); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Appwrite/Event/Realtime.php`:
- Around line 112-114: Replace the catch block to catch \Throwable instead of
\Exception so errors like TypeError are handled (locate the try surrounding
$this->realtime->send()), and update the Console::error call to follow PSR-12
spacing for concatenation (the string + $e->getMessage() expression used with
Console::error). Ensure the catch uses catch (\Throwable $e) and the message
concatenation uses spaces around the . operator while still calling
Console::error('Realtime send failed: ' . $e->getMessage()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7211b9c3-08ee-4d34-af43-48827f93edea
📒 Files selected for processing (1)
src/Appwrite/Event/Realtime.php
| } catch (\Exception $e) { | ||
| Console::error('Realtime send failed: '.$e->getMessage()); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In PHP 8.x, does catch (\Exception $e)catch\Error/\TypeError, or is catch (\Throwable $e) required to catch both?
💡 Result:
In PHP 8.x, catch (\Exception $e) does NOT catch \Error or \TypeError. You must use catch (\Throwable $e) to catch both Exceptions and Errors (including TypeError, which extends Error). Throwable is the base interface implemented by both Exception and Error. Error (and its subclasses like TypeError) do not extend Exception, so a catch block specific to Exception will not match them. This behavior has been consistent since PHP 7, and remains the same in PHP 8.x.
Citations:
- 1: https://secure.php.net/manual/language.exceptions.php
- 2: https://www.php.net/manual/en/class.throwable.php
- 3: https://www.php.net/manual/en/language.exceptions.php
- 4: https://www.php.net/manual/en/language.errors.php7.php
🏁 Script executed:
# Check if the file exists and view the context around lines 112-114
if [ -f "src/Appwrite/Event/Realtime.php" ]; then
echo "=== File exists, showing lines 100-125 ==="
sed -n '100,125p' src/Appwrite/Event/Realtime.php | cat -n
else
echo "File not found, searching for similar files..."
find . -name "Realtime.php" -type f
fiRepository: appwrite/appwrite
Length of output: 917
🏁 Script executed:
# Search for other catch (\Exception $e) patterns in the codebase to see if this is a broader issue
rg 'catch\s*\(\s*\\Exception\s*\$' --type php | head -20Repository: appwrite/appwrite
Length of output: 881
Use catch (\Throwable $e) instead of catch (\Exception $e) to handle all error types.
In PHP, \Exception and \Error are separate hierarchies under the common \Throwable interface. The current code only catches \Exception, missing \Error and \TypeError. This means a TypeError or other Error thrown from $this->realtime->send() will bypass the error handler and propagate up, defeating the fail-open intent. The fix also improves PSR-12 compliance with operator spacing.
Proposed fix
- } catch (\Exception $e) {
- Console::error('Realtime send failed: '.$e->getMessage());
+ } catch (\Throwable $e) {
+ Console::error('Realtime send failed: ' . $e->getMessage());
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (\Exception $e) { | |
| Console::error('Realtime send failed: '.$e->getMessage()); | |
| } | |
| } catch (\Throwable $e) { | |
| Console::error('Realtime send failed: ' . $e->getMessage()); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Appwrite/Event/Realtime.php` around lines 112 - 114, Replace the catch
block to catch \Throwable instead of \Exception so errors like TypeError are
handled (locate the try surrounding $this->realtime->send()), and update the
Console::error call to follow PSR-12 spacing for concatenation (the string +
$e->getMessage() expression used with Console::error). Ensure the catch uses
catch (\Throwable $e) and the message concatenation uses spaces around the .
operator while still calling Console::error('Realtime send failed: ' .
$e->getMessage()).
🔄 PHP-Retry SummaryFlaky tests detected across commits: |
✨ Benchmark results
⚡ Benchmark Comparison
|
1 participant
What does this PR do?
(Provide a description of what this PR does and why it's needed.)
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Screenshots may also be helpful.)
Related PRs and Issues
Checklist