feat: bound MySQL writes with session lock-wait timeout - #892
Conversation
Sorry, something went wrong.
WalkthroughsetTimeout() in the MySQL adapter now also sets innodb_lock_wait_timeout and lock_wait_timeout as session variables (derived from the millisecond input, capped to whole seconds) in addition to the existing SELECT optimizer hint. clearTimeout() resets those session variables to DEFAULT. processException() maps MySQL error HY000/1205 to TimeoutException. ChangesMySQL lock wait timeout handling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem🚥 Pre-merge checks | ✅ 4 | ❌ 1 ❌ Failed checks (1 warning)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches 📝 Generate docstrings
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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
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 | 🟠 Major
Wrap the exec() call in a try-catch and throw DatabaseException for consistency; reorder to apply session variables before registering the hook.
At line 48, exec() will throw PDOException (due to ATTR_ERRMODE_EXCEPTION configured in SQL.php), but this exception is not caught or wrapped in DatabaseException. Additionally, the hook is registered at line 36 before session variables are applied at line 48; if the SET SESSION call 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🤖 Prompt for AI Agentsif ($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}");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.
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
📥 CommitsReviewing files that changed from the base of the PR and between cfba533 and 21f5ccd.
📒 Files selected for processing (1)- src/Database/Adapter/MySQL.php
Sorry, something went wrong.
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
|
Sorry, something went wrong.
| // 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.
Fix SET syntax
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.
Sorry, something went wrong.
| // 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.
Respect event scope
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.
Sorry, something went wrong.
| // 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.
Pooled sessions leak
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.
Sorry, something went wrong.
| // 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.
Fix SET syntax
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.
Sorry, something went wrong.
| $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.
Reset session timeouts
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.
ArtifactsRepro: 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.
Ran code and verified through T-Rex
Sorry, something went wrong.
| @@ -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.
Preserve event scope
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).
Sorry, something went wrong.
| // 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.
Pool state drifts
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.
ArtifactsRepro: 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.
Ran code and verified through T-Rex
Sorry, something went wrong.
Reviewers
At least 1 approving review is required to merge this pull request.
Problem
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:
A 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)
The bound is a connection-scoped floor that Pool re-applies on each checkout (tracking the latest timeout), so there is no paired clearTimeout reset — avoiding fragile set/teardown of connection state that may never run on exception paths and isn't reachable through the pool anyway.
Scope
🤖 Generated with Claude Code