Skip to content

Commit a43420c

Browse files
DerDreschnerbackportbot[bot]
authored andcommitted
fix(Database): Use real idle-timer to prevent lastInsertId being reset on MariaDB/MySQL
The previous implementation of the idle timer runs on a strict 30 second interval and sends a dummy `SELECT` statement to keep the connection open. This generates issues with the `lastInsertId` on long-running tasks (like our CI pipeline), as the MariaDB documentation clearly states: > If the last query wasn't an INSERT or UPDATE statement or if the modified table does not have a column with the AUTO_INCREMENT attribute and LAST_INSERT_ID was not used, this function will return zero. Source: https://mariadb.com/docs/connectors/mariadb-connector-c/api-functions/mysql_insert_id To mitigate that, this commit now uses a real idle-timer per connection instead. Assisted-by: ClaudeCode:claude-fable-5 Signed-off-by: David Dreschner <david.dreschner@nextcloud.com>
1 parent 8698ea8 commit a43420c

10 files changed

Lines changed: 277 additions & 3 deletions

lib/composer/composer/autoload_classmap.php

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1656,6 +1656,11 @@
16561656
'OC\\DB\\ConnectionFactory' => $baseDir . '/lib/private/DB/ConnectionFactory.php',
16571657
'OC\\DB\\DbDataCollector' => $baseDir . '/lib/private/DB/DbDataCollector.php',
16581658
'OC\\DB\\Exceptions\\DbalException' => $baseDir . '/lib/private/DB/Exceptions/DbalException.php',
1659+
'OC\\DB\\Middleware\\ConnectionActivityConnection' => $baseDir . '/lib/private/DB/Middleware/ConnectionActivityConnection.php',
1660+
'OC\\DB\\Middleware\\ConnectionActivityDriver' => $baseDir . '/lib/private/DB/Middleware/ConnectionActivityDriver.php',
1661+
'OC\\DB\\Middleware\\ConnectionActivityMiddleware' => $baseDir . '/lib/private/DB/Middleware/ConnectionActivityMiddleware.php',
1662+
'OC\\DB\\Middleware\\ConnectionActivityNotifier' => $baseDir . '/lib/private/DB/Middleware/ConnectionActivityNotifier.php',
1663+
'OC\\DB\\Middleware\\ConnectionActivityStatement' => $baseDir . '/lib/private/DB/Middleware/ConnectionActivityStatement.php',
16591664
'OC\\DB\\Middleware\\UtcTimezoneMiddleware' => $baseDir . '/lib/private/DB/Middleware/UtcTimezoneMiddleware.php',
16601665
'OC\\DB\\Middleware\\UtcTimezoneMiddlewareDriver' => $baseDir . '/lib/private/DB/Middleware/UtcTimezoneMiddlewareDriver.php',
16611666
'OC\\DB\\MigrationException' => $baseDir . '/lib/private/DB/MigrationException.php',

lib/composer/composer/autoload_static.php

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1697,6 +1697,11 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
16971697
'OC\\DB\\ConnectionFactory' => __DIR__ . '/../../..' . '/lib/private/DB/ConnectionFactory.php',
16981698
'OC\\DB\\DbDataCollector' => __DIR__ . '/../../..' . '/lib/private/DB/DbDataCollector.php',
16991699
'OC\\DB\\Exceptions\\DbalException' => __DIR__ . '/../../..' . '/lib/private/DB/Exceptions/DbalException.php',
1700+
'OC\\DB\\Middleware\\ConnectionActivityConnection' => __DIR__ . '/../../..' . '/lib/private/DB/Middleware/ConnectionActivityConnection.php',
1701+
'OC\\DB\\Middleware\\ConnectionActivityDriver' => __DIR__ . '/../../..' . '/lib/private/DB/Middleware/ConnectionActivityDriver.php',
1702+
'OC\\DB\\Middleware\\ConnectionActivityMiddleware' => __DIR__ . '/../../..' . '/lib/private/DB/Middleware/ConnectionActivityMiddleware.php',
1703+
'OC\\DB\\Middleware\\ConnectionActivityNotifier' => __DIR__ . '/../../..' . '/lib/private/DB/Middleware/ConnectionActivityNotifier.php',
1704+
'OC\\DB\\Middleware\\ConnectionActivityStatement' => __DIR__ . '/../../..' . '/lib/private/DB/Middleware/ConnectionActivityStatement.php',
17001705
'OC\\DB\\Middleware\\UtcTimezoneMiddleware' => __DIR__ . '/../../..' . '/lib/private/DB/Middleware/UtcTimezoneMiddleware.php',
17011706
'OC\\DB\\Middleware\\UtcTimezoneMiddlewareDriver' => __DIR__ . '/../../..' . '/lib/private/DB/Middleware/UtcTimezoneMiddlewareDriver.php',
17021707
'OC\\DB\\MigrationException' => __DIR__ . '/../../..' . '/lib/private/DB/MigrationException.php',

lib/private/DB/Connection.php

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
use Doctrine\DBAL\Result;
2525
use Doctrine\DBAL\Schema\Schema;
2626
use Doctrine\DBAL\Statement;
27+
use OC\DB\Middleware\ConnectionActivityNotifier;
2728
use OC\DB\QueryBuilder\Partitioned\PartitionedQueryBuilder;
2829
use OC\DB\QueryBuilder\Partitioned\PartitionSplit;
2930
use OC\DB\QueryBuilder\QueryBuilder;
@@ -62,6 +63,8 @@ class Connection extends PrimaryReadReplicaConnection {
6263
protected int $queriesBuilt = 0;
6364
protected int $queriesExecuted = 0;
6465
protected ?DbDataCollector $dbDataCollector = null;
66+
/** Seconds the connection may sit idle before the next use re-verifies connectivity */
67+
private const CONNECTION_CHECK_INTERVAL = 30;
6568
private array $lastConnectionCheck = [];
6669

6770
protected ?float $transactionActiveSince = null;
@@ -121,6 +124,10 @@ public function __construct(
121124
parent::__construct($params, $driver, $config, $eventManager);
122125
$this->adapter = new $params['adapter']($this);
123126
$this->tablePrefix = $params['tablePrefix'];
127+
$activityNotifier = $params['activity_notifier'] ?? null;
128+
if ($activityNotifier instanceof ConnectionActivityNotifier) {
129+
$activityNotifier->setListener($this->refreshLastConnectionCheck(...));
130+
}
124131
$this->isShardingEnabled = isset($this->params['sharding']) && !empty($this->params['sharding']);
125132

126133
if ($this->isShardingEnabled) {
@@ -220,7 +227,7 @@ public function connect($connectionName = null) {
220227
$status = parent::connect();
221228
$eventLogger->end('connect:db');
222229

223-
$this->lastConnectionCheck[$this->getConnectionName()] = time();
230+
$this->refreshLastConnectionCheck();
224231

225232
return $status;
226233
} catch (Exception $e) {
@@ -902,21 +909,32 @@ public function rollBack() {
902909
private function reconnectIfNeeded(): void {
903910
if (
904911
!isset($this->lastConnectionCheck[$this->getConnectionName()])
905-
|| time() <= $this->lastConnectionCheck[$this->getConnectionName()] + 30
912+
|| time() <= $this->lastConnectionCheck[$this->getConnectionName()] + self::CONNECTION_CHECK_INTERVAL
906913
|| $this->isTransactionActive()
907914
) {
908915
return;
909916
}
910917

911918
try {
912919
$this->_conn->query($this->getDriver()->getDatabasePlatform()->getDummySelectSQL());
913-
$this->lastConnectionCheck[$this->getConnectionName()] = time();
920+
$this->refreshLastConnectionCheck();
914921
} catch (ConnectionLost|\Exception $e) {
915922
$this->logger->warning('Exception during connectivity check, closing and reconnecting', ['exception' => $e]);
916923
$this->close();
917924
}
918925
}
919926

927+
/**
928+
* A successful round trip proves the connection is alive: pushing the idle
929+
* timer forward keeps the connectivity probe of reconnectIfNeeded() from
930+
* firing between adjacent operations, where its query would reset the
931+
* driver level last insert id on MySQL. Invoked for every driver level
932+
* execution via the ConnectionActivityMiddleware.
933+
*/
934+
private function refreshLastConnectionCheck(): void {
935+
$this->lastConnectionCheck[$this->getConnectionName()] = time();
936+
}
937+
920938
private function getConnectionName(): string {
921939
return $this->isConnectedToPrimary() ? 'primary' : 'replica';
922940
}

lib/private/DB/ConnectionFactory.php

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
use Doctrine\DBAL\Configuration;
1212
use Doctrine\DBAL\DriverManager;
1313
use Doctrine\DBAL\Event\Listeners\OracleSessionInit;
14+
use OC\DB\Middleware\ConnectionActivityMiddleware;
1415
use OC\DB\Middleware\UtcTimezoneMiddleware;
1516
use OC\DB\QueryBuilder\Sharded\AutoIncrementHandler;
1617
use OC\DB\QueryBuilder\Sharded\ShardConnectionManager;
@@ -145,9 +146,12 @@ public function getConnection(string $type, array $additionalConnectionParams):
145146
break;
146147
}
147148
$configuration = new Configuration();
149+
$activityMiddleware = new ConnectionActivityMiddleware();
148150
$configuration->setMiddlewares([
149151
new UtcTimezoneMiddleware(),
152+
$activityMiddleware,
150153
]);
154+
$connectionParams['activity_notifier'] = $activityMiddleware->getNotifier();
151155
/** @var Connection $connection */
152156
$connection = DriverManager::getConnection(
153157
$connectionParams,
Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
10+
namespace OC\DB\Middleware;
11+
12+
use Doctrine\DBAL\Driver\Connection;
13+
use Doctrine\DBAL\Driver\Middleware\AbstractConnectionMiddleware;
14+
use Doctrine\DBAL\Driver\PDO\Connection as PDOConnection;
15+
use Doctrine\DBAL\Driver\Result;
16+
use Doctrine\DBAL\Driver\Statement;
17+
18+
final class ConnectionActivityConnection extends AbstractConnectionMiddleware {
19+
public function __construct(
20+
private Connection $inner,
21+
private ConnectionActivityNotifier $notifier,
22+
) {
23+
parent::__construct($inner);
24+
}
25+
26+
/**
27+
* Kept working for consumers that reach the native PDO handle through the
28+
* deprecated accessor, like SQLiteSessionInit: forwarding is intentionally
29+
* preferred over migrating the callers, as those code paths get refactored
30+
* with the DBAL 4 upgrade anyway.
31+
*/
32+
public function getWrappedConnection(): \PDO {
33+
if (!$this->inner instanceof PDOConnection) {
34+
throw new \LogicException('The wrapped connection is not a PDO based connection');
35+
}
36+
return $this->inner->getWrappedConnection();
37+
}
38+
39+
#[\Override]
40+
public function prepare(string $sql): Statement {
41+
return new ConnectionActivityStatement(parent::prepare($sql), $this->notifier);
42+
}
43+
44+
#[\Override]
45+
public function query(string $sql): Result {
46+
$result = parent::query($sql);
47+
$this->notifier->notify();
48+
return $result;
49+
}
50+
51+
#[\Override]
52+
public function exec(string $sql): int {
53+
$result = parent::exec($sql);
54+
$this->notifier->notify();
55+
return $result;
56+
}
57+
}
Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
10+
namespace OC\DB\Middleware;
11+
12+
use Doctrine\DBAL\Driver;
13+
use Doctrine\DBAL\Driver\Middleware\AbstractDriverMiddleware;
14+
15+
final class ConnectionActivityDriver extends AbstractDriverMiddleware {
16+
public function __construct(
17+
Driver $driver,
18+
private ConnectionActivityNotifier $notifier,
19+
) {
20+
parent::__construct($driver);
21+
}
22+
23+
#[\Override]
24+
public function connect(array $params) {
25+
return new ConnectionActivityConnection(parent::connect($params), $this->notifier);
26+
}
27+
}
Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
10+
namespace OC\DB\Middleware;
11+
12+
use Doctrine\DBAL\Driver;
13+
use Doctrine\DBAL\Driver\Middleware;
14+
15+
/**
16+
* Doctrine middleware reporting every query and statement execution back to
17+
* the owning connection, so the idle timer of the connectivity check can be
18+
* refreshed (see \OC\DB\Connection::refreshLastConnectionCheck()). Working on
19+
* the driver level covers executions of prepared statements as well, which
20+
* bypass the executeQuery() and executeStatement() methods of the connection.
21+
*/
22+
final class ConnectionActivityMiddleware implements Middleware {
23+
private ConnectionActivityNotifier $notifier;
24+
25+
public function __construct() {
26+
$this->notifier = new ConnectionActivityNotifier();
27+
}
28+
29+
/**
30+
* Hand the notifier to the connection that wants to listen, e.g. via the
31+
* connection parameters.
32+
*/
33+
public function getNotifier(): ConnectionActivityNotifier {
34+
return $this->notifier;
35+
}
36+
37+
#[\Override]
38+
public function wrap(Driver $driver): Driver {
39+
return new ConnectionActivityDriver($driver, $this->notifier);
40+
}
41+
}
Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
10+
namespace OC\DB\Middleware;
11+
12+
/**
13+
* Relays driver level activity to a listener that can only be registered
14+
* after the middleware was created: middlewares are configured before the
15+
* DriverManager constructs the connection wrapper that wants to listen.
16+
*/
17+
final class ConnectionActivityNotifier {
18+
private ?\Closure $listener = null;
19+
20+
/**
21+
* @param \Closure():void $listener
22+
*/
23+
public function setListener(\Closure $listener): void {
24+
$this->listener = $listener;
25+
}
26+
27+
public function notify(): void {
28+
if ($this->listener !== null) {
29+
($this->listener)();
30+
}
31+
}
32+
}
Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
10+
namespace OC\DB\Middleware;
11+
12+
use Doctrine\DBAL\Driver\Middleware\AbstractStatementMiddleware;
13+
use Doctrine\DBAL\Driver\Result;
14+
use Doctrine\DBAL\Driver\Statement;
15+
16+
final class ConnectionActivityStatement extends AbstractStatementMiddleware {
17+
public function __construct(
18+
Statement $statement,
19+
private ConnectionActivityNotifier $notifier,
20+
) {
21+
parent::__construct($statement);
22+
}
23+
24+
#[\Override]
25+
public function execute($params = null): Result {
26+
$result = parent::execute($params);
27+
$this->notifier->notify();
28+
return $result;
29+
}
30+
}

tests/lib/DB/ConnectionTest.php

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,9 @@
1515
use Doctrine\DBAL\Platforms\MySQLPlatform;
1616
use OC\DB\Adapter;
1717
use OC\DB\Connection;
18+
use OC\DB\ConnectionAdapter;
19+
use OCP\IDBConnection;
20+
use OCP\Server;
1821
use Test\TestCase;
1922

2023
#[\PHPUnit\Framework\Attributes\Group('DB')]
@@ -96,4 +99,56 @@ public function testClusterConnectsToPrimaryAndReplica(): void {
9699
$connection->ensureConnectedToReplica();
97100
}
98101

102+
public function testSuccessfulQueryResetsConnectivityCheckTimer(): void {
103+
$inner = $this->getInnerConnection();
104+
105+
// Ensure the connection is established before touching the timer
106+
$qb = $inner->getQueryBuilder();
107+
$qb->select('configvalue')->from('appconfig')->setMaxResults(1);
108+
$qb->executeQuery()->closeCursor();
109+
110+
$property = $this->backdateLastConnectionCheck($inner);
111+
$before = time();
112+
113+
$qb->executeQuery()->closeCursor();
114+
115+
// A connectivity probe firing between adjacent operations would reset
116+
// the driver level last insert id on MySQL
117+
self::assertGreaterThanOrEqual($before, max($property->getValue($inner)));
118+
}
119+
120+
public function testPreparedStatementExecutionResetsConnectivityCheckTimer(): void {
121+
$inner = $this->getInnerConnection();
122+
123+
$statement = $inner->prepare('SELECT `configvalue` FROM `*PREFIX*appconfig`', 1);
124+
125+
$property = $this->backdateLastConnectionCheck($inner);
126+
$before = time();
127+
128+
$statement->executeQuery()->free();
129+
130+
self::assertGreaterThanOrEqual($before, max($property->getValue($inner)));
131+
}
132+
133+
private function getInnerConnection(): Connection {
134+
$connection = Server::get(IDBConnection::class);
135+
if (!$connection instanceof ConnectionAdapter) {
136+
self::markTestSkipped('Test requires the real database connection');
137+
}
138+
139+
return $connection->getInner();
140+
}
141+
142+
/**
143+
* Make the connectivity check timer stale, but by less than the check
144+
* interval: the probe must not fire, so only actual query activity can
145+
* refresh the timer.
146+
*/
147+
private function backdateLastConnectionCheck(Connection $connection): \ReflectionProperty {
148+
$property = new \ReflectionProperty(Connection::class, 'lastConnectionCheck');
149+
$property->setValue($connection, ['primary' => time() - 20, 'replica' => time() - 20]);
150+
151+
return $property;
152+
}
153+
99154
}

0 commit comments

Comments
 (0)