fix: side-effect-free idempotency lookup and session-scoped lease use (#82 #110)

- EventLog::lookupIdempotent() no longer deletes expired rows; it returns null
  and defers eviction to a new purgeExpiredIdempotent() sweep. rememberIdempotent
  upserts so a stale expired row is refreshed in place.
- LeaseManager::ensureUsable() accepts an optional SessionId and routes through
  getForSession() so a lease id from another session cannot pass scope checks.

Co-authored-by: Cursor <cursoragent@cursor.com>

Author: nficano

src/Runtime/LeaseManager.php

@@ -79,9 +79,20 @@ public function getForSession(LeaseId $id, SessionId $sessionId): LeaseGranted
         return $lease;
     }
 
-    public function ensureUsable(LeaseId $id, LeaseScope $scope): LeaseGranted
+    /**
+     * Resolve a lease and assert it matches the requested scope. When
+     * `$sessionId` is supplied the lookup is session-scoped (leases are
+     * session-scoped per RFC §15.5), so a caller holding a lease id from a
+     * different session cannot pass scope checks.
+     *
+     * @throws PermissionDeniedException when the lease belongs to another
+     *                                   session or the scope does not match.
+     */
+    public function ensureUsable(LeaseId $id, LeaseScope $scope, ?SessionId $sessionId = null): LeaseGranted
     {
-        $lease = $this->get($id);
+        $lease = $sessionId instanceof SessionId
+            ? $this->getForSession($id, $sessionId)
+            : $this->get($id);
         if (
             $lease->permission !== $scope->permission
             || $lease->resource !== $scope->resource

src/Store/EventLog.php

@@ -305,10 +305,16 @@ public function rememberIdempotent(IdempotencyRecord $record): ?string
         if ($existing !== null) {
             return $existing;
         }
+        // Upsert: lookupIdempotent() returns null for an expired row without
+        // deleting it (#110), so refresh any stale entry in place rather than
+        // colliding with the (principal, idempotency_key) primary key.
         $stmt = $this->pdo->prepare(<<<'SQL'
             INSERT INTO idempotency_cache
                 (principal, idempotency_key, outcome_message_id, expires_at)
             VALUES (:principal, :key, :outcome, :expires)
+            ON CONFLICT(principal, idempotency_key) DO UPDATE SET
+                outcome_message_id = excluded.outcome_message_id,
+                expires_at = excluded.expires_at
             SQL);
         $stmt->execute([
             ':principal' => $record->principal,
@@ -319,6 +325,23 @@ public function rememberIdempotent(IdempotencyRecord $record): ?string
         return null;
     }
 
+    /**
+     * Delete idempotency-cache rows whose retention horizon has passed.
+     * Intended to be driven by a periodic sweep rather than read paths.
+     *
+     * @return int number of rows purged
+     */
+    public function purgeExpiredIdempotent(): int
+    {
+        $stmt = $this->pdo->prepare(
+            'DELETE FROM idempotency_cache WHERE expires_at <= :now',
+        );
+        $stmt->execute([
+            ':now' => $this->clock->now()->format(\DateTimeInterface::RFC3339_EXTENDED),
+        ]);
+        return $stmt->rowCount();
+    }
+
     public function lookupIdempotent(string $principal, string $idempotencyKey): ?string
     {
         $stmt = $this->pdo->prepare(<<<'SQL'
@@ -342,12 +365,8 @@ public function lookupIdempotent(string $principal, string $idempotencyKey): ?st
         }
         $expires = new \DateTimeImmutable($expiresStr);
         if ($expires <= $this->clock->now()) {
-            // Lazy GC of an expired entry.
-            $del = $this->pdo->prepare(<<<'SQL'
-                DELETE FROM idempotency_cache
-                WHERE principal = :principal AND idempotency_key = :key
-                SQL);
-            $del->execute([':principal' => $principal, ':key' => $idempotencyKey]);
+            // Treat an expired entry as absent. Deletion is deferred to
+            // purgeExpiredIdempotent() so this lookup stays side-effect free.
             return null;
         }
         return $outcome;

tests/Unit/EventLogTest.php

@@ -156,7 +156,8 @@ public function testIdempotencyCacheExpiresLazily(): void
             $this->log->lookupIdempotent('alice', 'refund-1'),
             'expired entry should not be returned',
         );
-        // After lazy GC, a fresh remember succeeds with the new outcome.
+        // An expired entry is overwritten in place, so a fresh remember
+        // succeeds with the new outcome.
         $newExpires = $this->clock->now()->modify('+1 hour');
         self::assertNull($this->log->rememberIdempotent(
             new IdempotencyRecord('alice', 'refund-1', 'msg_y', $newExpires),
@@ -177,4 +178,31 @@ public function testIdempotencyCachePartitionsByPrincipal(): void
         self::assertSame('msg_a', $this->log->lookupIdempotent('alice', 'refund-1'));
         self::assertSame('msg_b', $this->log->lookupIdempotent('bob', 'refund-1'));
     }
+
+    public function testLookupIdempotentDoesNotDeleteExpiredRows(): void
+    {
+        $this->log->rememberIdempotent(
+            new IdempotencyRecord('alice', 'refund-1', 'msg_x', $this->clock->now()->modify('+5 seconds')),
+        );
+        $this->clock->advance(10);
+
+        // A read must not mutate the store: the expired row survives lookup
+        // and is only removed by an explicit purge.
+        self::assertNull($this->log->lookupIdempotent('alice', 'refund-1'));
+        self::assertSame(1, $this->log->purgeExpiredIdempotent());
+    }
+
+    public function testPurgeExpiredIdempotentRemovesOnlyExpiredRows(): void
+    {
+        $this->log->rememberIdempotent(
+            new IdempotencyRecord('alice', 'expired', 'msg_old', $this->clock->now()->modify('+5 seconds')),
+        );
+        $this->log->rememberIdempotent(
+            new IdempotencyRecord('alice', 'live', 'msg_new', $this->clock->now()->modify('+1 hour')),
+        );
+        $this->clock->advance(10);
+
+        self::assertSame(1, $this->log->purgeExpiredIdempotent());
+        self::assertSame('msg_new', $this->log->lookupIdempotent('alice', 'live'));
+    }
 }

tests/Unit/LeaseManagerTest.php

@@ -81,6 +81,23 @@ public function testEnsureUsableValidatesScope(): void
         );
     }
 
+    public function testEnsureUsableRejectsForeignSession(): void
+    {
+        $clock = new FakeClock();
+        $mgr = new LeaseManager($clock);
+        $lease = $this->makeLease($clock);
+        $owner = \Arcp\Ids\SessionId::random();
+        $other = \Arcp\Ids\SessionId::random();
+        $mgr->register($lease, $owner);
+
+        $this->expectException(PermissionDeniedException::class);
+        $mgr->ensureUsable(
+            $lease->leaseId,
+            new LeaseScope($lease->permission, $lease->resource, $lease->operation),
+            $other,
+        );
+    }
+
     public function testExtendUpdatesExpiry(): void
     {
         $clock = new FakeClock();