Add optional keepalive and session timeouts#23
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds keepalive pings and session-variable support for the MySQL metadata connection, documents new environment variables (TRIGGER_KEEPALIVE, TRIGGER_SESSION_VARIABLES), and introduces retry logic for DBAL/PDO connectivity errors while preserving binlog position. Changes
Sequence Diagram(s)sequenceDiagram
participant Start as StartCommand
participant Trigger
participant DBAL as DBAL Connection
participant MySQL
Start->>Trigger: start()
Trigger->>DBAL: create connection from factory
DBAL->>MySQL: connect
Trigger->>MySQL: set session variables
MySQL-->>Trigger: OK
loop heartbeat interval
Trigger->>DBAL: ping (SELECT 1)
DBAL->>MySQL: query
alt connection ok
MySQL-->>DBAL: pong
else connection lost
MySQL-->>DBAL: error
DBAL-->>Trigger: throw
Trigger->>DBAL: reconnect and recreate connection
Trigger->>MySQL: reapply session variables
end
end
Trigger->>Trigger: process binlog stream
alt DBAL/PDO exception occurs
Trigger->>Start: propagate exception
Start->>Start: preserve binlog position
Start->>Trigger: retry/start from position
else non-retryable error
Start->>Start: fail/exit
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 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 |
![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
🤖 Fix all issues with AI agents
In `@src/Trigger.php`:
- Around line 143-156: The start method calls a non-existent
MySQLReplicationFactory::getDbConnection() causing a runtime error; remove that
call and instead create and assign $this->dbConnection using your own DBAL
connection logic (e.g., DriverManager::getConnection(...) or existing connection
factory) before calling $this->applySessionVariables($this->dbConnection), keep
the rest of start (MySQLReplicationFactory instantiation, registering
subscribers via getSubscribers(), and $binLogStream->run()) unchanged, and
ensure any references to $this->dbConnection in applySessionVariables or
elsewhere use the newly-initialized DBAL connection.
🧹 Nitpick comments (2)
src/Trigger.php (1)
359-404: SQL injection surface via interpolated variable name — currently mitigated by regex.Line 399 interpolates
$namedirectly into the SQL:"SET SESSION {$name} = ?". The value is parameterized, but the identifier is not. The regex on Line 386 (/^[A-Za-z0-9_]+$/) restricts names to alphanumeric + underscore, which is safe for MySQL identifiers and prevents injection.Two observations:
Silent swallowing of errors (Line 400-402): All exceptions — including permission denied, typos, and unsupported variables — are silently caught. Consider logging at
debuglevel so operators can diagnose misconfiguration.Float-valued session variables are sent as strings: Only integer patterns are cast (Line 394-396). A value like
"0.5"will be sent as a string. This is fine for the documented use case (wait_timeout,interactive_timeout), but worth noting if future users set fractional session variables.💡 Consider logging suppressed errors for debuggability
try { $connection->executeStatement("SET SESSION {$name} = ?", [$value]); - } catch (Throwable) { - // Ignore session variable failures (permission/unsupported variables). + } catch (Throwable $e) { + // Log at debug level so operators can diagnose misconfiguration. + logger()->debug("Failed to set session variable '{$name}': {$e->getMessage()}"); }config/trigger.php (1)
28-38: Config additions look correct; minor robustness note.The defaults are non-breaking (
keepalive = 0,session_variables = []). Thesession_variablesparsing delegateskey=valuesplitting toapplySessionVariables(), which handles edge cases (empty entries, missing=, invalid names).One small note:
array_map('trim', explode(',', ...))will produce[""]for a trailing-comma input like"wait_timeout=7200,". This is safely handled downstream (Line 379 in Trigger.php), but addingarray_filterhere would be slightly cleaner.🔧 Optional: filter empty entries at the config level
'session_variables' => env('TRIGGER_SESSION_VARIABLES', '') - ? array_map('trim', explode(',', (string) env('TRIGGER_SESSION_VARIABLES'))) + ? array_filter(array_map('trim', explode(',', (string) env('TRIGGER_SESSION_VARIABLES')))) : [],
| public function start(bool $keepUp = true): void | ||
| { | ||
| tap(new MySQLReplicationFactory($this->configure($keepUp)), function (MySQLReplicationFactory $binLogStream) { | ||
| collect($this->getSubscribers()) | ||
| ->reject(fn ($subscriber) => ! is_subclass_of($subscriber, EventSubscriber::class)) | ||
| ->unique() | ||
| ->each(fn ($subscriber) => $binLogStream->registerSubscriber(new $subscriber($this))); | ||
| })->run(); | ||
| $binLogStream = new MySQLReplicationFactory($this->configure($keepUp)); | ||
|
|
||
| $this->dbConnection = $binLogStream->getDbConnection(); | ||
| $this->applySessionVariables($this->dbConnection); | ||
|
|
||
| collect($this->getSubscribers()) | ||
| ->reject(fn ($subscriber) => ! is_subclass_of($subscriber, EventSubscriber::class)) | ||
| ->unique() | ||
| ->each(fn ($subscriber) => $binLogStream->registerSubscriber(new $subscriber($this))); | ||
|
|
||
| $binLogStream->run(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for getDbConnection method definition and usage
echo "=== Searching for getDbConnection definition ==="
rg -n 'getDbConnection' --type php -B 2 -A 5
echo -e "\n=== Searching for MySQLReplicationFactory class ==="
rg -n 'class MySQLReplicationFactory' --type php -A 20
echo -e "\n=== Checking for dbConnection references ==="
rg -n 'dbConnection' --type php -B 1 -A 1
echo -e "\n=== Checking composer.json for dependencies ==="
if [ -f "composer.json" ]; then
cat composer.json | grep -A 5 -B 5 'mysql.replication\|krowinski'
fiRepository: huangdijia/laravel-trigger
Length of output: 2351
🏁 Script executed:
#!/bin/bash
# Search for imports and usage of MySQLReplicationFactory
echo "=== Searching for MySQLReplicationFactory imports ==="
rg -n 'MySQLReplicationFactory|use.*Replication' --type php -B 2 -A 2
echo -e "\n=== Check the full namespace imports in Trigger.php ==="
head -50 src/Trigger.php | grep -E '^use|^namespace'Repository: huangdijia/laravel-trigger
Length of output: 3752
🌐 Web query:
krowinski/php-mysql-replication getDbConnection method
💡 Result:
In krowinski/php-mysql-replication there isn’t a getDbConnection() method exposed for you to reuse as a normal query connection—the library’s main entry point is MySQLReplicationFactory, which is constructed from a ConfigBuilder and then you call run() to consume binlog events. [1]
If you need a “regular” DB connection for queries alongside the replication stream, you create/manage that yourself (the project examples use Doctrine DBAL), e.g. via DriverManager::getConnection([...]) in the example’s getConnection() method. [2]
Sources: [1] [2]
🏁 Script executed:
#!/bin/bash
# Search for any custom MySQLReplicationFactory wrapper or extension
echo "=== Searching for class definitions that might extend or wrap MySQLReplicationFactory ==="
rg -n 'class.*MySQLReplication|extends.*MySQLReplication|implements.*' --type php -B 2 -A 5
echo -e "\n=== Check if configure() returns something with getDbConnection ==="
rg -n 'configure\(' --type php -B 2 -A 10
echo -e "\n=== Look for any method definitions on this object ==="
rg -n '\$binLogStream' --type php -B 2 -A 2Repository: huangdijia/laravel-trigger
Length of output: 5115
getDbConnection() does not exist on MySQLReplicationFactory and will cause a runtime error.
The krowinski/php-mysql-replication library does not expose a getDbConnection() method. The library's design expects you to manage your database connection separately using Doctrine DBAL (e.g., via DriverManager::getConnection()), not retrieve it from MySQLReplicationFactory. Remove the call to getDbConnection() and initialize $this->dbConnection using your own connection logic.
🤖 Prompt for AI Agents
In `@src/Trigger.php` around lines 143 - 156, The start method calls a
non-existent MySQLReplicationFactory::getDbConnection() causing a runtime error;
remove that call and instead create and assign $this->dbConnection using your
own DBAL connection logic (e.g., DriverManager::getConnection(...) or existing
connection factory) before calling
$this->applySessionVariables($this->dbConnection), keep the rest of start
(MySQLReplicationFactory instantiation, registering subscribers via
getSubscribers(), and $binLogStream->run()) unchanged, and ensure any references
to $this->dbConnection in applySessionVariables or elsewhere use the
newly-initialized DBAL connection.
|
@coderabbitai The warning about MySQLReplicationFactory::getDbConnection() looks like a false positive/outdated context. That method exists in krowinski/php-mysql-replication v8.x and v9.x (and this package requires ^8.0 || ^9.0). We also removed the earlier duplicated factory+run block; Trigger::start() now constructs the factory once and runs it once. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 366 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 13852
I've verified that:
The warning was indeed a false positive. Thank you for the correction! |
|
@huangdijia thanks! do you think its possible to make a new tagged release soon? I still run my fork in production, but now with latest PR merged in, it would be nice to switch back to your official package. |
|
I will release the version as soon as possible. |
2 participants
Why
Long-running trigger listeners can be disconnected by MySQL when idle timeouts (
wait_timeout/interactive_timeout) are low (e.g. 300s).The underlying replication library uses a normal metadata connection (
information_schema) that can sit idle and later throws:SQLSTATE[HY000]: General error: 4031 The client was disconnected by the server because of inactivitywhen it is reused.
What
TRIGGER_KEEPALIVE).TRIGGER_SESSION_VARIABLES), using a simple comma-separatedkey=valueformat.Config
Defaults are non-breaking: keepalive disabled (
0) and no session variables unless configured.Notes
This avoids changing global MySQL settings and is helpful when the DB is shared by multiple apps.
Summary by CodeRabbit
New Features
Documentation