From e85a196531207ac93c376dc7af350e62b80eee2f Mon Sep 17 00:00:00 2001 From: Luther Monson Date: Thu, 22 Oct 2026 02:51:23 -0700 Subject: [PATCH] feat: owner-verified session-lock release via ephpm_kv_del_if_eq The opt-in session lock was released with a plain del, so a request that overran lockTtlSeconds deleted the lock a later request had since taken. Release it with the atomic compare-and-delete ephpm_kv_del_if_eq (ePHPm v0.11.x+) instead, feature-detected with function_exists(); older runtimes keep the best-effort del. The capability lives in a new optional CompareAndDeleteKvOpsInterface (implemented by SapiKvOps and InMemoryKvOps) rather than on KvOpsInterface, so custom KvOpsInterface backends keep working unchanged and fall back to the plain del. --- README.md | 24 +++++--- src/CompareAndDeleteKvOpsInterface.php | 33 +++++++++++ src/InMemoryKvOps.php | 16 +++++- src/KvSessionHandler.php | 42 ++++++++++---- src/SapiKvOps.php | 16 +++++- tests/InMemoryKvOpsTest.php | 17 ++++++ tests/KvSessionHandlerTest.php | 50 +++++++++++++++- tests/Support/NoCompareAndDeleteKvOps.php | 69 +++++++++++++++++++++++ 8 files changed, 244 insertions(+), 23 deletions(-) create mode 100644 src/CompareAndDeleteKvOpsInterface.php create mode 100644 tests/Support/NoCompareAndDeleteKvOps.php diff --git a/README.md b/README.md index 4c07c52..1850242 100644 --- a/README.md +++ b/README.md @@ -301,10 +301,17 @@ safety valve that frees a session whose owner crashed before `close()`), Two caveats worth knowing: -- **Best-effort unlock.** The SAPI has no compare-and-delete, so the - release deletes the lock key without verifying the owner token. The - lock TTL, not the unlock, is the real backstop against a stuck lock — - the same trade the Files handler's advisory `flock()` makes. +- **Owner-verified unlock on ePHPm v0.11.x+.** The release uses the + atomic compare-and-delete `ephpm_kv_del_if_eq()`: the lock is deleted + only while it still holds this request's token, so a request that + overran `lockTtlSeconds` cannot free the lock a later request has since + taken. The function is feature-detected; on older runtimes (or a custom + `KvOpsInterface` backend that does not also implement + `CompareAndDeleteKvOpsInterface`) the release falls back to a + best-effort delete without token verification. Either way the lock TTL + is the backstop against a stuck lock. Size `lockTtlSeconds` above your + slowest request: once it lapses, another request can take the lock while + the first is still running. - **Requires a runtime providing `ephpm_kv_setnx`.** Enabling locking on an older runtime that lacks it surfaces a `Call to undefined function` the first time a session is read. The default (disabled) path never @@ -441,10 +448,11 @@ ePHPm runs PHP inside the same OS process as the KV store via the embed SAPI. The store is a Rust [`DashMap`](https://docs.rs/dashmap/) plus TTL management. ePHPm registers a small set of host functions (`ephpm_kv_get`, `ephpm_kv_set`, `ephpm_kv_setnx`, `ephpm_kv_del`, -`ephpm_kv_exists`, `ephpm_kv_expire`, `ephpm_kv_ttl`, `ephpm_kv_pttl`, -`ephpm_kv_incr_by`) into PHP's global function table. Calling one is a -direct C call into Rust — no socket, no protocol parser. (`setnx` backs -the opt-in per-session lock; the rest cover the core session lifecycle.) +`ephpm_kv_del_if_eq`, `ephpm_kv_exists`, `ephpm_kv_expire`, +`ephpm_kv_ttl`, `ephpm_kv_pttl`, `ephpm_kv_incr_by`) into PHP's global +function table. Calling one is a direct C call into Rust — no socket, no +protocol parser. (`setnx` and `del_if_eq` back the opt-in per-session +lock; the rest cover the core session lifecycle.) This package wraps those functions in a `SessionHandlerInterface` + `SessionUpdateTimestampHandlerInterface` diff --git a/src/CompareAndDeleteKvOpsInterface.php b/src/CompareAndDeleteKvOpsInterface.php new file mode 100644 index 0000000..fe35db5 --- /dev/null +++ b/src/CompareAndDeleteKvOpsInterface.php @@ -0,0 +1,33 @@ + */ private array $values = []; @@ -62,6 +62,20 @@ public function del(string $key): int return 1; } + public function supportsDelIfEq(): bool + { + return true; + } + + public function delIfEq(string $key, string $expected): bool + { + if ($this->liveValue($key) !== $expected) { + return false; + } + unset($this->values[$key], $this->deadlines[$key]); + return true; + } + public function exists(string $key): bool { return $this->liveValue($key) !== null; diff --git a/src/KvSessionHandler.php b/src/KvSessionHandler.php index 70d5a5f..2d7bb2e 100644 --- a/src/KvSessionHandler.php +++ b/src/KvSessionHandler.php @@ -56,6 +56,7 @@ final class KvSessionHandler implements */ private bool $lockHeld = false; private ?string $lockKey = null; + private ?string $lockToken = null; /** * @param string $prefix Key prefix written before each session @@ -87,10 +88,13 @@ final class KvSessionHandler implements * request's lifetime. This handler does not do that by default. When * `$lockSessions` is true, `read()` acquires a per-session lock at * `lock:` with a bounded {@see \ephpm_kv_setnx} spin and - * `close()` releases it. Because the SAPI has no compare-and-delete, the - * release is *best-effort* (`del` without owner-token verification) — - * Files-handler parity, pending a future CAS primitive. The lock TTL caps - * the blast radius of a crashed owner. + * `close()` releases it. The release is owner-verified when the backend + * supports compare-and-delete ({@see CompareAndDeleteKvOpsInterface}; + * `ephpm_kv_del_if_eq()`, ePHPm v0.11.x+): only a lock still holding this + * request's token is deleted, so a request that overran the lock TTL + * cannot free the lock a later request has since taken. On older runtimes + * the release falls back to a best-effort `del` without token + * verification. The lock TTL caps the blast radius of a crashed owner. */ public function __construct( string $prefix = 'php_session:', @@ -124,9 +128,9 @@ public function open(string $path, string $name): bool } /** - * Releases the per-session lock if this handler holds one. Best-effort: - * with no compare-and-delete primitive we cannot verify the owner token - * before deleting, so the TTL is the real safety net (see the constructor). + * Releases the per-session lock if this handler holds one — owner-verified + * via compare-and-delete where the runtime supports it, best-effort `del` + * otherwise (see the constructor). */ public function close(): bool { @@ -272,6 +276,7 @@ private function acquireLock(string $id): bool if ($this->ops->setnx($key, $token, $this->lockTtlSeconds)) { $this->lockHeld = true; $this->lockKey = $key; + $this->lockToken = $token; return true; } if ($this->nowMs() >= $deadlineMs) { @@ -282,18 +287,31 @@ private function acquireLock(string $id): bool } /** - * Best-effort release of a lock this handler acquired. No-op if we never - * took one (e.g. locking disabled, or the acquire timed out). + * Release a lock this handler acquired. No-op if we never took one (e.g. + * locking disabled, or the acquire timed out). + * + * With compare-and-delete available the lock is deleted only while it + * still holds our token; if our TTL already lapsed and another request + * took the lock, it is left alone. Without it (a custom backend, or an + * ePHPm runtime older than v0.11.x) we cannot prove ownership, so the + * release is a best-effort `del` — the lock TTL bounds the damage. */ private function releaseLock(): void { if ($this->lockHeld && $this->lockKey !== null) { - // No CAS: we cannot prove we still own the token before deleting. - // The lock TTL bounds the damage; this matches Files-handler parity. - $this->ops->del($this->lockKey); + if ( + $this->lockToken !== null + && $this->ops instanceof CompareAndDeleteKvOpsInterface + && $this->ops->supportsDelIfEq() + ) { + $this->ops->delIfEq($this->lockKey, $this->lockToken); + } else { + $this->ops->del($this->lockKey); + } } $this->lockHeld = false; $this->lockKey = null; + $this->lockToken = null; } private function nowMs(): int diff --git a/src/SapiKvOps.php b/src/SapiKvOps.php index 7fe2aee..f7d7b05 100644 --- a/src/SapiKvOps.php +++ b/src/SapiKvOps.php @@ -10,7 +10,7 @@ * so we fail fast outside the runtime instead of producing * "Call to undefined function" errors at request time. */ -final class SapiKvOps implements KvOpsInterface +final class SapiKvOps implements KvOpsInterface, CompareAndDeleteKvOpsInterface { public function __construct() { @@ -44,6 +44,20 @@ public function del(string $key): int return (int) \ephpm_kv_del($key); } + /** + * `ephpm_kv_del_if_eq()` ships in ePHPm v0.11.x and later; older runtimes + * lack it, so it is feature-detected rather than assumed. + */ + public function supportsDelIfEq(): bool + { + return \function_exists('ephpm_kv_del_if_eq'); + } + + public function delIfEq(string $key, string $expected): bool + { + return (bool) \ephpm_kv_del_if_eq($key, $expected); + } + public function exists(string $key): bool { return (bool) \ephpm_kv_exists($key); diff --git a/tests/InMemoryKvOpsTest.php b/tests/InMemoryKvOpsTest.php index d99da2c..0c127e8 100644 --- a/tests/InMemoryKvOpsTest.php +++ b/tests/InMemoryKvOpsTest.php @@ -159,4 +159,21 @@ public function test_flush_clears_values_and_deadlines(): void self::assertNull($ops->get('b')); self::assertSame(-2, $ops->pttl('b')); } + + public function test_del_if_eq_deletes_only_on_matching_value(): void + { + $ops = new InMemoryKvOps(); + self::assertTrue($ops->supportsDelIfEq()); + + $ops->set('k', 'mine', 60); + self::assertFalse($ops->delIfEq('k', 'theirs')); + self::assertSame('mine', $ops->get('k')); + + self::assertTrue($ops->delIfEq('k', 'mine')); + self::assertNull($ops->get('k')); + self::assertSame(-2, $ops->pttl('k')); + + // Missing key: nothing to delete. + self::assertFalse($ops->delIfEq('k', 'mine')); + } } diff --git a/tests/KvSessionHandlerTest.php b/tests/KvSessionHandlerTest.php index 42ca046..fa54d90 100644 --- a/tests/KvSessionHandlerTest.php +++ b/tests/KvSessionHandlerTest.php @@ -5,7 +5,9 @@ namespace Ephpm\SessionHandler\Tests; use Ephpm\SessionHandler\InMemoryKvOps; +use Ephpm\SessionHandler\KvOpsInterface; use Ephpm\SessionHandler\KvSessionHandler; +use Ephpm\SessionHandler\Tests\Support\NoCompareAndDeleteKvOps; use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\Attributes\PreserveGlobalState; use PHPUnit\Framework\Attributes\RunInSeparateProcess; @@ -224,7 +226,7 @@ public function test_create_sid_does_not_recurse_when_handler_is_active(): void // ── opt-in session locking ─────────────────────────────────────────────── private function lockingHandler( - InMemoryKvOps $ops, + KvOpsInterface $ops, int $maxWaitMs = 5_000, int $spinUs = 20_000, int $lockTtl = 30, @@ -322,4 +324,50 @@ public function test_lock_becomes_available_after_ttl_expiry(): void self::assertNotNull($tokenB); self::assertNotSame($tokenA, $tokenB); } + + public function test_release_leaves_a_lock_another_request_has_since_taken(): void + { + $ops = new InMemoryKvOps(); + + // A takes the lock, then (simulated) overruns its TTL: the lock lapses + // and B takes it, so the key now holds B's token. + $a = $this->lockingHandler($ops); + $a->read('sid'); + $ops->set('php_session:lock:sid', 'token-of-B', 30); + + // A's release is compare-and-delete: B's lock must survive it. + $a->close(); + self::assertSame('token-of-B', $ops->get('php_session:lock:sid')); + } + + public function test_release_without_compare_and_delete_falls_back_to_plain_del(): void + { + // A backend without CompareAndDeleteKvOpsInterface (a custom + // KvOpsInterface, or SapiKvOps on ePHPm older than v0.11.x) keeps the + // previous best-effort release: the lock key is deleted unverified. + $ops = new NoCompareAndDeleteKvOps(new InMemoryKvOps()); + + $a = $this->lockingHandler($ops); + $a->read('sid'); + self::assertTrue($ops->exists('php_session:lock:sid')); + $ops->set('php_session:lock:sid', 'token-of-B', 30); + + $a->close(); + self::assertFalse($ops->exists('php_session:lock:sid')); + } + + public function test_release_with_compare_and_delete_frees_our_own_lock(): void + { + $ops = new InMemoryKvOps(); + $a = $this->lockingHandler($ops); + $a->read('sid'); + + $a->close(); + self::assertFalse($ops->exists('php_session:lock:sid')); + + // A second close() is a no-op (nothing held any more). + $ops->set('php_session:lock:sid', 'someone-else', 30); + $a->close(); + self::assertSame('someone-else', $ops->get('php_session:lock:sid')); + } } diff --git a/tests/Support/NoCompareAndDeleteKvOps.php b/tests/Support/NoCompareAndDeleteKvOps.php new file mode 100644 index 0000000..add1231 --- /dev/null +++ b/tests/Support/NoCompareAndDeleteKvOps.php @@ -0,0 +1,69 @@ +inner->get($key); + } + + public function set(string $key, string $value, int $ttlSeconds = 0): bool + { + return $this->inner->set($key, $value, $ttlSeconds); + } + + public function setnx(string $key, string $value, int $ttlSeconds = 0): bool + { + return $this->inner->setnx($key, $value, $ttlSeconds); + } + + public function del(string $key): int + { + return $this->inner->del($key); + } + + public function exists(string $key): bool + { + return $this->inner->exists($key); + } + + public function incrBy(string $key, int $delta): int + { + return $this->inner->incrBy($key, $delta); + } + + public function expire(string $key, int $ttlSeconds): bool + { + return $this->inner->expire($key, $ttlSeconds); + } + + public function ttl(string $key): int + { + return $this->inner->ttl($key); + } + + public function pttl(string $key): int + { + return $this->inner->pttl($key); + } + + public function flush(): bool + { + return $this->inner->flush(); + } +}