Skip to content

feat: bound MySQL writes with session lock-wait timeout #892

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
loks0n wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: bound MySQL writes with session lock-wait timeout #892

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/Database/Adapter/MySQL.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ public function setTimeout(int $milliseconds, string $event = Database::EVENT_AL

$this->timeout = $milliseconds;

// 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/',
Expand All @@ -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.

@greptile-apps greptile-apps Bot Jun 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 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.

@greptile-apps greptile-apps Bot Jun 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 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.

@greptile-apps greptile-apps Bot Jun 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 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.

$seconds = \max(1, (int) \ceil($milliseconds / 1000));
$this->getPDO()->exec("SET SESSION innodb_lock_wait_timeout = {$seconds}, SESSION lock_wait_timeout = {$seconds}");

@greptile-apps greptile-apps Bot Jun 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 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.

Comment on lines +49 to +50

@greptile-apps greptile-apps Bot Jun 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 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.

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.

View artifacts

T-Rex Ran code and verified through T-Rex

Comment on lines 36 to +50

@greptile-apps greptile-apps Bot Jun 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 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).

Comment on lines +47 to +50

@greptile-apps greptile-apps Bot Jun 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 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.

Artifacts

Repro: PHP script demonstrating pool timeout state drift with mock adapters

  • Contains supporting evidence from the run (text/x-php; charset=utf-8).

Repro: failing script output showing Pool::getTimeout()==0 after setTimeout(1000) and only one adapter receiving the timeout

  • Keeps the command output available without making the summary code-heavy.

View artifacts

T-Rex Ran code and verified through T-Rex

}

/**
Expand Down Expand Up @@ -162,6 +170,11 @@ protected function processException(PDOException $e): \Exception
return new TimeoutException('Query timed out', $e->getCode(), $e);
}

// Lock wait timeout (blocked write released by innodb_lock_wait_timeout/lock_wait_timeout)
if ($e->getCode() === 'HY000' && isset($e->errorInfo[1]) && $e->errorInfo[1] === 1205) {
return new TimeoutException('Query timed out', $e->getCode(), $e);
}

// Functional index dependency
if ($e->getCode() === 'HY000' && isset($e->errorInfo[1]) && $e->errorInfo[1] === 3837) {
return new DependencyException('Attribute cannot be deleted because it is used in an index', $e->getCode(), $e);
Expand Down
Loading