feat: bound MySQL writes with session lock-wait timeout#892
Conversation
📝 WalkthroughWalkthrough
ChangesMySQL lock wait timeout handling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Database/Adapter/MySQL.php (1)
35-49:⚠️ Potential issue | 🟠 MajorWrap the
exec()call in a try-catch and throwDatabaseExceptionfor consistency; reorder to apply session variables before registering the hook.At line 48,
exec()will throwPDOException(due toATTR_ERRMODE_EXCEPTIONconfigured in SQL.php), but this exception is not caught or wrapped inDatabaseException. Additionally, the hook is registered at line 36 before session variables are applied at line 48; if theSET SESSIONcall fails, the hook remains active while the intended write-timeout bounds were never established.The same issue exists in
clearTimeout()at line 64.Suggested patch
if ($milliseconds <= 0) { throw new DatabaseException('Timeout must be greater than 0'); } $this->timeout = $milliseconds; + // Bound writes: hints are ignored by INSERT/UPDATE/DDL, so cap how long a + // statement waits on row (InnoDB) and metadata locks before failing fast. + $seconds = \max(1, (int) \ceil($milliseconds / 1000)); + try { + $this->getPDO()->exec("SET SESSION innodb_lock_wait_timeout = {$seconds}, SESSION lock_wait_timeout = {$seconds}"); + } catch (\PDOException $e) { + throw new DatabaseException('Failed to set MySQL session lock wait timeout: ' . $e->getMessage(), 0, $e); + } + // Bound reads: the max_execution_time optimizer hint only applies to SELECTs. $this->before($event, 'timeout', function ($sql) use ($milliseconds) { return \preg_replace( pattern: '/SELECT/', replacement: "SELECT /*+ max_execution_time({$milliseconds}) */", subject: $sql, limit: 1 ); }); - - // Bound writes: hints are ignored by INSERT/UPDATE/DDL, so cap how long a - // statement waits on row (InnoDB) and metadata locks before failing fast. - $seconds = \max(1, (int) \ceil($milliseconds / 1000)); - $this->getPDO()->exec("SET SESSION innodb_lock_wait_timeout = {$seconds}, SESSION lock_wait_timeout = {$seconds}");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Database/Adapter/MySQL.php` around lines 35 - 49, The exec() call that sets session variables is not wrapped in exception handling and will throw PDOException without converting it to DatabaseException, and the hook is registered before session variables are applied, so if SET SESSION fails the hook remains active. Move the getPDO().exec() call that sets innodb_lock_wait_timeout and lock_wait_timeout before the before() hook registration, wrap it in a try-catch block that catches PDOException and throws DatabaseException instead. Apply the same fix to the clearTimeout() method at line 64 to ensure consistency across both timeout/clearTimeout methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/Database/Adapter/MySQL.php`:
- Around line 35-49: The exec() call that sets session variables is not wrapped
in exception handling and will throw PDOException without converting it to
DatabaseException, and the hook is registered before session variables are
applied, so if SET SESSION fails the hook remains active. Move the
getPDO().exec() call that sets innodb_lock_wait_timeout and lock_wait_timeout
before the before() hook registration, wrap it in a try-catch block that catches
PDOException and throws DatabaseException instead. Apply the same fix to the
clearTimeout() method at line 64 to ensure consistency across both
timeout/clearTimeout methods.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bfc2f263-ad99-4387-a78a-796c08b266d9
📒 Files selected for processing (1)
src/Database/Adapter/MySQL.php
MySQL::setTimeout only injected a max_execution_time optimizer hint, which MySQL honors for read-only SELECTs only. INSERT/UPDATE/DDL had no app-level bound: row-lock waits sat for the 50s default and metadata-lock waits for lock_wait_timeout's one-year default, holding the connection the whole time. Add a session-level write bound (innodb_lock_wait_timeout + lock_wait_timeout) so blocked writes fail fast with error 1205 and release their connection, and map 1205 to TimeoutException. The bound is a connection-scoped floor that Pool re-applies on each checkout, so there's no paired clearTimeout reset to leak through. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Greptile Summary
Confidence Score: 3/5The change needs attention before merge because timeout state can leak across connection reuse, event scopes, and pooled connections. The touched file is small and the intended behavior is clear, but the connection-level timeout changes introduce several likely runtime correctness issues around state restoration and scope. src/Database/Adapter/MySQL.php
 T-Rex LogsWhat T-Rex did
Reviews (2): Last reviewed commit: "feat: bound MySQL writes with session lo..." | Re-trigger Greptile |
loks0n
force-pushed
the
feat/mysql-insert-timeouts
branch
from
21f5ccd to
adbcd12
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Bound writes: hints are ignored by INSERT/UPDATE/DDL, so cap how long a | ||
| // statement waits on row (InnoDB) and metadata locks before failing fast. | ||
| // This is a connection-scoped floor: Pool re-applies it on each checkout, | ||
| // so it tracks the latest timeout without a paired reset to leak through. |
There was a problem hiding this comment.
This statement repeats SESSION in the second assignment. MySQL scopes following unqualified assignments from the latest scope keyword, so this can fail before the timeout is installed. When setTimeout() is called, callers can receive a SQL syntax error instead of getting a bounded query timeout.
| // Bound writes: hints are ignored by INSERT/UPDATE/DDL, so cap how long a | ||
| // statement waits on row (InnoDB) and metadata locks before failing fast. | ||
| // This is a connection-scoped floor: Pool re-applies it on each checkout, | ||
| // so it tracks the latest timeout without a paired reset to leak through. |
There was a problem hiding this comment.
setTimeout($ms, $event) still scopes the read timeout through the before($event, ...) callback, but this new session change applies to every statement on the connection. A caller that sets a timeout only for a narrow event such as EVENT_DOCUMENT_FIND can now make a later unrelated write or DDL fail after the same low lock-wait timeout.
| // Bound writes: hints are ignored by INSERT/UPDATE/DDL, so cap how long a | ||
| // statement waits on row (InnoDB) and metadata locks before failing fast. | ||
| // This is a connection-scoped floor: Pool re-applies it on each checkout, | ||
| // so it tracks the latest timeout without a paired reset to leak through. |
There was a problem hiding this comment.
This line now mutates connection session state, but the pooled adapter does not delegate clearTimeout() back to the borrowed MySQL connection. As a result, Database::clearTimeout() on a Pool can remove the Pool-level transformation while leaving innodb_lock_wait_timeout / lock_wait_timeout low on the pooled PDO session. Later unrelated writes that reuse that connection can then fail fast unexpectedly. Ensure pooled cleanup reaches the concrete MySQL adapter that had its session variables changed.
| // This is a connection-scoped floor: Pool re-applies it on each checkout, | ||
| // so it tracks the latest timeout without a paired reset to leak through. | ||
| $seconds = \max(1, (int) \ceil($milliseconds / 1000)); | ||
| $this->getPDO()->exec("SET SESSION innodb_lock_wait_timeout = {$seconds}, SESSION lock_wait_timeout = {$seconds}"); |
There was a problem hiding this comment.
This SET statement repeats SESSION after the comma, which MySQL rejects as invalid syntax. With the repository PDO settings using exception mode, any call to setTimeout() on MySQL can throw before the timed query runs, so the existing timeout test path and any caller enabling timeouts can fail immediately. Use one session modifier for the assignment list, or qualify each variable with @@SESSION.
| $seconds = \max(1, (int) \ceil($milliseconds / 1000)); | ||
| $this->getPDO()->exec("SET SESSION innodb_lock_wait_timeout = {$seconds}, SESSION lock_wait_timeout = {$seconds}"); |
There was a problem hiding this comment.
These session variables are changed on the connection, but MySQL still inherits Adapter::clearTimeout(), which only removes the SQL transformation callback. After a caller does setTimeout(1000), then clearTimeout(), the same PDO connection keeps innodb_lock_wait_timeout and lock_wait_timeout at 1 second, so later unrelated writes or DDL can fail fast instead of using the server defaults.
Artifacts
Repro: PHP script emulating setTimeout + clearTimeout PDO flow
- Contains supporting evidence from the run (text/x-php; charset=utf-8).
Repro: failing test output showing session variables stuck at 1s after clearTimeout()
- Keeps the command output available without making the summary code-heavy.
| @@ -40,6 +41,13 @@ public function setTimeout(int $milliseconds, string $event = Database::EVENT_AL | |||
| limit: 1 | |||
| ); | |||
| }); | |||
|
|
|||
| // Bound writes: hints are ignored by INSERT/UPDATE/DDL, so cap how long a | |||
| // statement waits on row (InnoDB) and metadata locks before failing fast. | |||
| // This is a connection-scoped floor: Pool re-applies it on each checkout, | |||
| // so it tracks the latest timeout without a paired reset to leak through. | |||
| $seconds = \max(1, (int) \ceil($milliseconds / 1000)); | |||
| $this->getPDO()->exec("SET SESSION innodb_lock_wait_timeout = {$seconds}, SESSION lock_wait_timeout = {$seconds}"); | |||
There was a problem hiding this comment.
The $event argument only scopes the before() callback above, but these session variables are changed for the whole connection regardless of which event was requested. For example, setTimeout(1, Database::EVENT_DOCUMENT_FIND) should limit find queries, but this also lowers lock waits for unrelated updates and schema changes on the same connection, making writes fail after 1 second and breaking the event-specific contract of setTimeout($milliseconds, $event).
| // This is a connection-scoped floor: Pool re-applies it on each checkout, | ||
| // so it tracks the latest timeout without a paired reset to leak through. | ||
| $seconds = \max(1, (int) \ceil($milliseconds / 1000)); | ||
| $this->getPDO()->exec("SET SESSION innodb_lock_wait_timeout = {$seconds}, SESSION lock_wait_timeout = {$seconds}"); |
There was a problem hiding this comment.
This comment relies on the pool reapplying the timeout on checkout, but Pool::setTimeout() delegates to one checked-out adapter and does not store the timeout on the pool adapter itself. In pooled MySQL usage, only the connection that handled setTimeout() gets these session variables; a later write can run on a different pooled connection with the server defaults, so the new write bound becomes nondeterministic.
Artifacts
Repro: PHP script demonstrating pool timeout state drift with mock adapters
- Contains supporting evidence from the run (text/x-php; charset=utf-8).
- Keeps the command output available without making the summary code-heavy.
1 participant
Problem
MySQL::setTimeout()only injected amax_execution_timeoptimizer hint, which MySQL honors for read-only SELECTs only. INSERT/UPDATE/DDL had no app-level bound:lock_wait_timeout's default of one yearA blocked write held its connection the entire time instead of failing fast. (Unlike MariaDB, MySQL has no
SET STATEMENT … FOR <stmt>syntax, so a connection-scoped lock-wait timeout is the real mechanism here.)Change (
src/Database/Adapter/MySQL.php)setTimeout()— keeps the SELECT hint for reads, and adds a connection-scoped write bound:SET SESSION innodb_lock_wait_timeout = N, lock_wait_timeout = N(seconds, floored at 1). Blocked INSERT/UPDATE/DDL now fail fast with error 1205 and release the connection.processException()— maps error 1205 (ER_LOCK_WAIT_TIMEOUT) to the existingTimeoutException, consistent with how SELECT timeouts surface.The bound is a connection-scoped floor that
Poolre-applies on each checkout (tracking the latest timeout), so there is no pairedclearTimeoutreset — avoiding fragile set/teardown of connection state that may never run on exception paths and isn't reachable through the pool anyway.Scope
SET STATEMENT max_statement_time FORalready bounds the statement types it wraps.testQueryTimeoutstill passes.🤖 Generated with Claude Code