Add exception safety to withinSessionLevelLock (#11)

- Preserve original exception when release also fails
- Skip release when lock was not acquired (wasAcquired === false)
- Add ADR-002 documenting the design decision

Author: antonkomarev

adr/adr-002-within-session-lock-exception-safety.md

@@ -0,0 +1,63 @@
+# ADR-002: Exception safety in withinSessionLevelLock
+
+## Status
+
+Accepted
+
+## Context
+
+`withinSessionLevelLock()` acquires a session-level advisory lock, executes a user callback, and releases the lock. The release must happen reliably, but two edge cases require careful handling:
+
+1. **Double failure.** The callback throws, and then the release also throws (e.g. the database connection was lost). In PHP, an exception thrown from a `finally` block replaces the in-flight exception. A naive `try/finally` would expose the caller to a `PDOException` from release instead of the real error.
+
+2. **Release of an unacquired lock.** With a zero timeout the lock may not be acquired (`wasAcquired === false`). Calling `PG_ADVISORY_UNLOCK` on a key that was never held returns `false` in isolation, but if the same key was previously acquired in the same session (reentrant locking), the call decrements the counter and silently releases someone else's lock.
+
+## Decision
+
+Use `try/catch/finally` with two guards:
+
+- **Skip release when the lock was not acquired** — `if ($lockHandle->wasAcquired)`.
+- **Protect the original exception** — catch the callback exception, store it, re-throw; wrap release in its own `try/catch` so a release failure cannot mask the original.
+
+```php
+$exception = null;
+try {
+    return $callback($lockHandle);
+} catch (\Throwable $e) {
+    $exception = $e;
+    throw $e;
+} finally {
+    if ($lockHandle->wasAcquired) {
+        try {
+            $this->releaseSessionLevelLock(...);
+        } catch (\Throwable $releaseException) {
+            if ($exception === null) {
+                throw $releaseException;
+            }
+        }
+    }
+}
+```
+
+### Behavior matrix
+
+| Callback | Release | Result |
+|----------|---------|--------|
+| OK | OK | Return callback result |
+| OK | Throws | Throw release exception |
+| Throws | OK | Throw original exception |
+| Throws | Throws | Throw original exception, suppress release exception |
+
+### Why suppress the release exception (not chain it)
+
+PHP sets `previous` only in the exception constructor — there is no `addPrevious()`. Wrapping the original in a new exception would change its type and break typed `catch` blocks in user code. Suppressing the secondary release error is the lesser evil: the root cause (callback failure) is always preserved.
+
+### Why not log the suppressed exception
+
+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.
+
+## 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.

src/Postgres/PostgresAdvisoryLocker.php

@@ -83,15 +83,26 @@ public function withinSessionLevelLock(
             accessMode: $accessMode,
         );
 
+        $exception = null;
         try {
             return $callback($lockHandle);
-        }
-        finally {
-            $this->releaseSessionLevelLock(
-                dbConnection: $dbConnection,
-                key: $key,
-                accessMode: $accessMode,
-            );
+        } 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;
+                    }
+                }
+            }
         }
     }
 

test/Integration/Postgres/PostgresAdvisoryLockerTest.php

@@ -754,6 +754,66 @@ public static function provideItCanExecuteCodeWithinSessionLockData(): array
         ];
     }
 
+    public function testWithinSessionLevelLockPreservesOriginalExceptionWhenReleaseFails(): 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 throws an exception and the connection is killed before release
+        try {
+            $locker->withinSessionLevelLock(
+                $dbConnection,
+                $lockKey,
+                function () use ($dbConnection): never {
+                    // Kill own connection's backend to make release fail
+                    $pid = $dbConnection->pgsqlGetPid();
+                    $killerConnection = $this->initPostgresPdoConnection();
+                    $killerConnection->exec("SELECT PG_TERMINATE_BACKEND($pid)");
+
+                    throw new \RuntimeException('Original error from callback');
+                },
+                TimeoutDuration::zero(),
+            );
+            $this->fail('Expected RuntimeException was not thrown');
+        } catch (\RuntimeException $e) {
+            // 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());
+        }
+    }
+
+    public function testWithinSessionLevelLockDoesNotReleaseWhenLockWasNotAcquired(): void
+    {
+        // GIVEN: A lock already held by another connection so the second cannot acquire it
+        $locker = $this->initLocker();
+        $dbConnection1 = $this->initPostgresPdoConnection();
+        $dbConnection2 = $this->initPostgresPdoConnection();
+        $lockKey = PostgresLockKey::create('test');
+        $locker->acquireSessionLevelLock(
+            $dbConnection1,
+            $lockKey,
+            TimeoutDuration::zero(),
+        );
+
+        // WHEN: withinSessionLevelLock is called on second connection (lock not acquired)
+        $locker->withinSessionLevelLock(
+            $dbConnection2,
+            $lockKey,
+            function ($lockHandle): void {
+                $this->assertFalse($lockHandle->wasAcquired);
+            },
+            TimeoutDuration::zero(),
+        );
+
+        // THEN: The first connection's lock should still be intact (not decremented by a spurious release)
+        $this->assertPgAdvisoryLocksCount(1);
+        $this->assertPgAdvisoryLockExistsInConnection($dbConnection1, $lockKey);
+    }
+
     public function testItSanitizesCommentToPreventSqlInjection(): void
     {
         // GIVEN: A lock key with newline that could break SQL comment and inject code