From 116d753f4482bd27a57226d67702bafabb169011 Mon Sep 17 00:00:00 2001 From: Anton Komarev Date: Sun, 8 Feb 2026 20:07:43 +0300 Subject: [PATCH 1/3] Improve withinSessionLevelLock: invoke callback only on successful lock acquisition - Change withinSessionLevelLock() to invoke callback only when lock is acquired - Return WithinSessionLevelLockHandle with wasAcquired flag and callback result - Callback no longer receives SessionLevelLockHandle argument - Update all tests to check wasAcquired outside callback - Add test verifying callback is not invoked when lock is not acquired BREAKING CHANGE: withinSessionLevelLock() return type changed from mixed to WithinSessionLevelLockHandle --- .../WithinSessionLevelLockHandle.php | 28 +++++++++++ src/Postgres/PostgresAdvisoryLocker.php | 50 ++++++++++++------- .../Postgres/PostgresAdvisoryLockerTest.php | 22 +++++--- 3 files changed, 74 insertions(+), 26 deletions(-) create mode 100644 src/Postgres/LockHandle/WithinSessionLevelLockHandle.php diff --git a/src/Postgres/LockHandle/WithinSessionLevelLockHandle.php b/src/Postgres/LockHandle/WithinSessionLevelLockHandle.php new file mode 100644 index 0000000..c668648 --- /dev/null +++ b/src/Postgres/LockHandle/WithinSessionLevelLockHandle.php @@ -0,0 +1,28 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +declare(strict_types=1); + +namespace Cog\DbLocker\Postgres\LockHandle; + +/** + * @template TReturn + */ +final class WithinSessionLevelLockHandle +{ + /** + * @param TReturn $result + */ + public function __construct( + public readonly bool $wasAcquired, + public readonly mixed $result, + ) {} +} diff --git a/src/Postgres/PostgresAdvisoryLocker.php b/src/Postgres/PostgresAdvisoryLocker.php index 54bb017..db4cb1f 100644 --- a/src/Postgres/PostgresAdvisoryLocker.php +++ b/src/Postgres/PostgresAdvisoryLocker.php @@ -19,6 +19,7 @@ use Cog\DbLocker\Postgres\Enum\PostgresLockAccessModeEnum; use Cog\DbLocker\Postgres\Enum\PostgresLockLevelEnum; use Cog\DbLocker\Postgres\LockHandle\SessionLevelLockHandle; +use Cog\DbLocker\Postgres\LockHandle\WithinSessionLevelLockHandle; use Cog\DbLocker\Postgres\LockHandle\TransactionLevelLockHandle; use Cog\DbLocker\TimeoutDuration; @@ -53,21 +54,22 @@ public function acquireTransactionLevelLock( } /** - * Acquires a session-level advisory lock and ensures its release after executing the callback. + * Acquires a session-level advisory lock, executes the callback only if acquired, and ensures release. + * + * The callback is invoked only when the lock is successfully acquired. If the lock + * is not acquired (held by another process), the callback is not called and + * a handle with `wasAcquired = false` is returned. * * This method guarantees that the lock is released even if an exception is thrown during execution. * Useful for safely wrapping critical sections that require locking. * - * If the lock was not acquired (i.e., `wasAcquired` is `false`), it is up to the callback - * to decide how to handle the situation (e.g., retry, throw, log, or silently skip). - * * ⚠️ Transaction-level advisory locks are strongly preferred whenever possible, * as they are automatically released at the end of a transaction and are less error-prone. * Use session-level locks only when transactional context is not available. * - * @param callable(SessionLevelLockHandle): TReturn $callback A callback that receives the lock handle. + * @param callable(): TReturn $callback A callback to execute while the lock is held. * @param TimeoutDuration $timeoutDuration Maximum wait time. Use TimeoutDuration::zero() for an immediate (non-blocking) attempt. - * @return TReturn The return value of the callback. + * @return WithinSessionLevelLockHandle Handle with wasAcquired and result of the callback (null if not acquired). * * @throws LockAcquireException If a database error occurs during lock acquisition. NOT thrown for normal lock contention. * @throws LockReleaseException If a database error occurs during lock release (only thrown if no other exception occurred during callback execution). @@ -82,7 +84,7 @@ public function withinSessionLevelLock( callable $callback, TimeoutDuration $timeoutDuration, PostgresLockAccessModeEnum $accessMode = PostgresLockAccessModeEnum::Exclusive, - ): mixed { + ): WithinSessionLevelLockHandle { $lockHandle = $this->acquireSessionLevelLock( dbConnection: $dbConnection, key: $key, @@ -90,24 +92,34 @@ public function withinSessionLevelLock( accessMode: $accessMode, ); + if (!$lockHandle->wasAcquired) { + return new WithinSessionLevelLockHandle( + wasAcquired: false, + result: null, + ); + } + $exception = null; try { - return $callback($lockHandle); + $result = $callback(); + + return new WithinSessionLevelLockHandle( + wasAcquired: true, + result: $result, + ); } catch (\Throwable $e) { $exception = $e; throw $e; } finally { - if ($lockHandle->wasAcquired) { - try { - $this->releaseSessionLevelLock( - dbConnection: $dbConnection, - key: $key, - accessMode: $accessMode, - ); - } catch (\Throwable $releaseException) { - if ($exception === null) { - throw $releaseException; - } + try { + $this->releaseSessionLevelLock( + dbConnection: $dbConnection, + key: $key, + accessMode: $accessMode, + ); + } catch (\Throwable $releaseException) { + if ($exception === null) { + throw $releaseException; } } } diff --git a/test/Integration/Postgres/PostgresAdvisoryLockerTest.php b/test/Integration/Postgres/PostgresAdvisoryLockerTest.php index 8a3557c..52b32ba 100644 --- a/test/Integration/Postgres/PostgresAdvisoryLockerTest.php +++ b/test/Integration/Postgres/PostgresAdvisoryLockerTest.php @@ -843,7 +843,7 @@ public function testItCanExecuteCodeWithinSessionLock( $y = 3; // WHEN: Executing code within a session-level lock using callback - $result = $locker->withinSessionLevelLock( + $handle = $locker->withinSessionLevelLock( new PdoConnectionAdapter($dbConnection), $lockKey, function () use ($dbConnection, $lockKey, $accessMode, $x, $y): int { @@ -857,7 +857,8 @@ function () use ($dbConnection, $lockKey, $accessMode, $x, $y): int { ); // THEN: Callback should execute successfully and lock should be auto-released - $this->assertSame(5, $result); + $this->assertTrue($handle->wasAcquired); + $this->assertSame(5, $handle->result); $this->assertPgAdvisoryLocksCount(0); $this->assertPgAdvisoryLockMissingInConnection($dbConnection, $lockKey); } @@ -902,11 +903,13 @@ function () use ($dbConnection): never { // THEN: The original exception from callback should be preserved, not masked by release failure $this->assertSame('Original error from callback', $e->getMessage()); } catch (\PDOException $e) { - $this->fail('PDOException from release should not mask the original RuntimeException: ' . $e->getMessage()); + $this->fail( + 'PDOException from release should not mask the original RuntimeException: ' . $e->getMessage(), + ); } } - public function testWithinSessionLevelLockDoesNotReleaseWhenLockWasNotAcquired(): void + public function testWithinSessionLevelLockDoesNotCallCallbackWhenLockWasNotAcquired(): void { // GIVEN: A lock already held by another connection so the second cannot acquire it $locker = $this->initLocker(); @@ -918,17 +921,22 @@ public function testWithinSessionLevelLockDoesNotReleaseWhenLockWasNotAcquired() $lockKey, TimeoutDuration::zero(), ); + $callbackExecuted = false; // WHEN: withinSessionLevelLock is called on second connection (lock not acquired) - $locker->withinSessionLevelLock( + $handle = $locker->withinSessionLevelLock( new PdoConnectionAdapter($dbConnection2), $lockKey, - function ($lockHandle): void { - $this->assertFalse($lockHandle->wasAcquired); + function () use (&$callbackExecuted): void { + $callbackExecuted = true; }, TimeoutDuration::zero(), ); + // THEN: Callback should not be called, handle should indicate lock was not acquired + $this->assertFalse($handle->wasAcquired); + $this->assertNull($handle->result); + $this->assertFalse($callbackExecuted); // THEN: The first connection's lock should still be intact (not decremented by a spurious release) $this->assertPgAdvisoryLocksCount(1); $this->assertPgAdvisoryLockExistsInConnection($dbConnection1, $lockKey); From 74f52a5eeb71d5786656977d2ea248e1b0463df3 Mon Sep 17 00:00:00 2001 From: Anton Komarev Date: Mon, 9 Feb 2026 07:26:08 +0300 Subject: [PATCH 2/3] Document callback result loss when lock release fails in withinSessionLevelLock - Add docblock warning that LockReleaseException guarantees successful callback but loses return value - Update ADR-003 with 'Known limitation' section explaining PHP finally semantics - Suggest WithinSessionLockReleaseException as potential future improvement - Direct users to low-level API for cases where callback result must be preserved --- adr/adr-003-within-session-lock-exception-safety.md | 9 +++++++++ src/Postgres/PostgresAdvisoryLocker.php | 3 +++ 2 files changed, 12 insertions(+) diff --git a/adr/adr-003-within-session-lock-exception-safety.md b/adr/adr-003-within-session-lock-exception-safety.md index 7ab6646..8f7b23c 100644 --- a/adr/adr-003-within-session-lock-exception-safety.md +++ b/adr/adr-003-within-session-lock-exception-safety.md @@ -56,8 +56,17 @@ PHP sets `previous` only in the exception constructor — there is no `addPrevio The library has no logger dependency and adding one for a single edge case is not justified. Users who need visibility into release failures can wrap `withinSessionLevelLock` in their own try/catch or use `acquireSessionLevelLock` / `releaseSessionLevelLock` directly. +## Known limitation: callback result is lost on release failure + +When the callback succeeds but `releaseSessionLevelLock()` throws, `LockReleaseException` is thrown and the callback's return value is discarded. The caller receives no indication of the callback result. + +This is a consequence of PHP's `finally` semantics — a `throw` in `finally` prevents the `try` block's `return` from completing. The `LockReleaseException` itself serves as a reliable signal that the callback completed successfully (it is only thrown when `$exception === null`), but the return value is not preserved. + +A future improvement could introduce a dedicated exception subclass (e.g. `WithinSessionLockReleaseException`) that carries the callback result. For now, users who need the callback result in this scenario should use `acquireSessionLevelLock()` / `releaseSessionLevelLock()` directly. + ## Consequences - Original exceptions from user callbacks are never masked by release failures. - `PG_ADVISORY_UNLOCK` is not called when the lock was never acquired, preventing reentrant counter corruption. - Release failures are silently suppressed when a callback exception is already in flight — acceptable trade-off given no logger. +- Callback return value is lost when release fails — acceptable trade-off; low-level API is available for callers who need it. diff --git a/src/Postgres/PostgresAdvisoryLocker.php b/src/Postgres/PostgresAdvisoryLocker.php index db4cb1f..395c474 100644 --- a/src/Postgres/PostgresAdvisoryLocker.php +++ b/src/Postgres/PostgresAdvisoryLocker.php @@ -73,6 +73,9 @@ public function acquireTransactionLevelLock( * * @throws LockAcquireException If a database error occurs during lock acquisition. NOT thrown for normal lock contention. * @throws LockReleaseException If a database error occurs during lock release (only thrown if no other exception occurred during callback execution). + * ⚠️ A LockReleaseException guarantees that the callback has completed successfully, + * but the callback's return value is lost. If you need access to the callback result + * in this scenario, use acquireSessionLevelLock() / releaseSessionLevelLock() directly. * * @see acquireTransactionLevelLock() for preferred locking strategy. * From 3decd001aad63a69325e5f57e14644c683fa6f56 Mon Sep 17 00:00:00 2001 From: Anton Komarev Date: Mon, 9 Feb 2026 07:36:11 +0300 Subject: [PATCH 3/3] Add test for callback result loss when lock release fails - Test verifies that LockReleaseException is thrown when callback succeeds but release fails - Confirms callback result is lost in this scenario (documented limitation in ADR-003) - Uses PG_TERMINATE_BACKEND to simulate release failure - All 94 tests pass --- .../Postgres/PostgresAdvisoryLockerTest.php | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/test/Integration/Postgres/PostgresAdvisoryLockerTest.php b/test/Integration/Postgres/PostgresAdvisoryLockerTest.php index 52b32ba..e4511b8 100644 --- a/test/Integration/Postgres/PostgresAdvisoryLockerTest.php +++ b/test/Integration/Postgres/PostgresAdvisoryLockerTest.php @@ -942,6 +942,39 @@ function () use (&$callbackExecuted): void { $this->assertPgAdvisoryLockExistsInConnection($dbConnection1, $lockKey); } + public function testWithinSessionLevelLockLosesCallbackResultWhenReleaseFails(): void + { + // GIVEN: A session-level lock acquired via withinSessionLevelLock callback + $locker = $this->initLocker(); + $dbConnection = $this->initPostgresPdoConnection(); + $dbConnection->setAttribute(\PDO::ATTR_ERRMODE, \PDO::ERRMODE_EXCEPTION); + $lockKey = PostgresLockKey::create('test'); + + // WHEN: Callback succeeds with a result but connection is killed before release + try { + $locker->withinSessionLevelLock( + new PdoConnectionAdapter($dbConnection), + $lockKey, + function () use ($dbConnection): string { + // Kill own connection's backend to make release fail + $pid = $dbConnection->pgsqlGetPid(); + $killerConnection = $this->initPostgresPdoConnection(); + $killerConnection->exec("SELECT PG_TERMINATE_BACKEND($pid)"); + + return 'important-result'; + }, + TimeoutDuration::zero(), + ); + $this->fail('Expected LockReleaseException was not thrown'); + } catch (\Cog\DbLocker\Exception\LockReleaseException $e) { + // THEN: LockReleaseException is thrown (guaranteeing callback succeeded) + $this->assertStringContainsString('Failed to release lock', $e->getMessage()); + // THEN: But the callback result is lost - this is a known limitation documented in ADR-003 + } catch (\Throwable $e) { + $this->fail('Expected LockReleaseException but got: ' . get_class($e) . ' - ' . $e->getMessage()); + } + } + public function testItSanitizesCommentToPreventSqlInjection(): void { // GIVEN: A lock key with newline that could break SQL comment and inject code