[Server] Log filesystem failures in FileSessionStore (#488)

* [Server] Log filesystem failures in FileSessionStore

* [Server] Warn on failed unlink during session garbage collection

src/Server/Session/FileSessionStore.php

@@ -14,6 +14,8 @@ namespace Mcp\Server\Session;
 use Mcp\Exception\RuntimeException;
 use Mcp\Server\NativeClock;
 use Psr\Clock\ClockInterface;
+use Psr\Log\LoggerInterface;
+use Psr\Log\NullLogger;
 use Symfony\Component\Uid\Uuid;
 
 /**
@@ -26,9 +28,13 @@ class FileSessionStore implements SessionStoreInterface
         private readonly string $directory,
         private readonly int $ttl = 3600,
         private readonly ClockInterface $clock = new NativeClock(),
+        private readonly LoggerInterface $logger = new NullLogger(),
     ) {
-        if (!is_dir($this->directory)) {
+        if (!is_dir($this->directory) && !@mkdir($this->directory, 0775, true) && !is_dir($this->directory)) {
-            @mkdir($this->directory, 0775, true);
+            $this->logger->warning('Failed to create session directory.', [
+                'directory' => $this->directory,
+                'error' => error_get_last()['message'] ?? 'unknown',
+            ]);
         }
 
         if (!is_dir($this->directory) || !is_writable($this->directory)) {
@@ -59,13 +65,23 @@ class FileSessionStore implements SessionStoreInterface
 
         $mtime = @filemtime($path) ?: 0;
         if (($this->clock->now()->getTimestamp() - $mtime) > $this->ttl) {
-            @unlink($path);
+            if (!@unlink($path) && is_file($path)) {
+                $this->logger->warning('Failed to delete expired session file.', [
+                    'path' => $path,
+                    'error' => error_get_last()['message'] ?? 'unknown',
+                ]);
+            }
 
             return false;
         }
 
         $data = @file_get_contents($path);
         if (false === $data) {
+            $this->logger->warning('Failed to read session file.', [
+                'path' => $path,
+                'error' => error_get_last()['message'] ?? 'unknown',
+            ]);
+
             return false;
         }
 
@@ -78,6 +94,11 @@ class FileSessionStore implements SessionStoreInterface
 
         $tmp = $path.'.tmp';
         if (false === @file_put_contents($tmp, $data, \LOCK_EX)) {
+            $this->logger->warning('Failed to write session file.', [
+                'path' => $tmp,
+                'error' => error_get_last()['message'] ?? 'unknown',
+            ]);
+
             return false;
         }
 
@@ -85,6 +106,10 @@ class FileSessionStore implements SessionStoreInterface
         if (!@rename($tmp, $path)) {
             // Fallback if rename fails cross-device
             if (false === @copy($tmp, $path)) {
+                $this->logger->warning('Failed to move session file into place.', [
+                    'path' => $path,
+                    'error' => error_get_last()['message'] ?? 'unknown',
+                ]);
                 @unlink($tmp);
 
                 return false;
@@ -101,8 +126,11 @@ class FileSessionStore implements SessionStoreInterface
     {
         $path = $this->pathFor($id);
 
-        if (is_file($path)) {
+        if (is_file($path) && !@unlink($path) && is_file($path)) {
-            @unlink($path);
+            $this->logger->warning('Failed to delete session file.', [
+                'path' => $path,
+                'error' => error_get_last()['message'] ?? 'unknown',
+            ]);
         }
 
         return true;
@@ -119,6 +147,11 @@ class FileSessionStore implements SessionStoreInterface
 
         $dir = @opendir($this->directory);
         if (false === $dir) {
+            $this->logger->warning('Failed to open session directory for garbage collection.', [
+                'directory' => $this->directory,
+                'error' => error_get_last()['message'] ?? 'unknown',
+            ]);
+
             return $deleted;
         }
 
@@ -140,7 +173,15 @@ class FileSessionStore implements SessionStoreInterface
 
             $mtime = @filemtime($path) ?: 0;
             if (($now - $mtime) > $this->ttl) {
-                @unlink($path);
+                if (!@unlink($path) && is_file($path)) {
+                    $this->logger->warning('Failed to delete expired session file.', [
+                        'path' => $path,
+                        'error' => error_get_last()['message'] ?? 'unknown',
+                    ]);
+
+                    continue;
+                }
+
                 $deleted[] = Uuid::fromString($entry);
             }
         }

tests/Unit/Server/Session/FileSessionStoreTest.php

@@ -16,6 +16,8 @@ use Mcp\Exception\RuntimeException;
 use Mcp\Server\Session\FileSessionStore;
 use PHPUnit\Framework\Attributes\TestDox;
 use PHPUnit\Framework\TestCase;
+use Psr\Log\AbstractLogger;
+use Psr\Log\LogLevel;
 use Symfony\Component\Uid\UuidV4;
 
 class FileSessionStoreTest extends TestCase
@@ -109,4 +111,121 @@ class FileSessionStoreTest extends TestCase
             throw $e;
         }
     }
+
+    #[TestDox('logs a warning when a session file cannot be read')]
+    public function testUnreadableSessionFileLogsWarning(): void
+    {
+        $logger = new WarningCollectingLogger();
+        $store = new FileSessionStore($this->directory, logger: $logger);
+        $id = new UuidV4();
+
+        $store->write($id, 'payload');
+
+        $path = $this->directory.\DIRECTORY_SEPARATOR.$id->toRfc4122();
+        chmod($path, 0000);
+        clearstatcache(true, $path);
+
+        if (is_readable($path)) {
+            $this->markTestSkipped('Permission bits do not restrict reads here (running as root, or a filesystem that ignores them).');
+        }
+
+        $this->assertFalse($store->read($id));
+        $this->assertCount(1, $logger->warnings);
+        $this->assertSame('Failed to read session file.', $logger->warnings[0]['message']);
+        $this->assertSame($path, $logger->warnings[0]['context']['path']);
+    }
+
+    #[TestDox('logs a warning when a session file cannot be written')]
+    public function testUnwritableSessionFileLogsWarning(): void
+    {
+        $logger = new WarningCollectingLogger();
+        $store = new FileSessionStore($this->directory, logger: $logger);
+
+        chmod($this->directory, 0555);
+        clearstatcache(true, $this->directory);
+
+        if (is_writable($this->directory)) {
+            $this->markTestSkipped('Permission bits do not restrict writes here (running as root, or a filesystem that ignores them).');
+        }
+
+        $this->assertFalse($store->write(new UuidV4(), 'payload'));
+        $this->assertCount(1, $logger->warnings);
+        $this->assertSame('Failed to write session file.', $logger->warnings[0]['message']);
+    }
+
+    #[TestDox('logs a warning when the session directory cannot be opened for garbage collection')]
+    public function testGcLogsWarningWhenDirectoryVanished(): void
+    {
+        $logger = new WarningCollectingLogger();
+        $store = new FileSessionStore($this->directory, logger: $logger);
+
+        rmdir($this->directory);
+
+        $this->assertSame([], $store->gc());
+        $this->assertCount(1, $logger->warnings);
+        $this->assertSame('Failed to open session directory for garbage collection.', $logger->warnings[0]['message']);
+        $this->assertSame($this->directory, $logger->warnings[0]['context']['directory']);
+    }
+
+    #[TestDox('gc() warns and does not report a session whose file survived deletion')]
+    public function testGcLogsWarningWhenExpiredFileCannotBeDeleted(): void
+    {
+        $logger = new WarningCollectingLogger();
+        $store = new FileSessionStore($this->directory, ttl: 60, logger: $logger);
+        $id = new UuidV4();
+        $store->write($id, 'payload');
+
+        $path = $this->directory.\DIRECTORY_SEPARATOR.$id->toRfc4122();
+        touch($path, time() - 120);
+
+        chmod($this->directory, 0555);
+        clearstatcache(true, $this->directory);
+
+        if (is_writable($this->directory)) {
+            $this->markTestSkipped('Permission bits do not restrict writes here (running as root, or a filesystem that ignores them).');
+        }
+
+        // The file is still there, so its id must not be reported as deleted.
+        $this->assertSame([], $store->gc());
+        $this->assertFileExists($path);
+        $this->assertCount(1, $logger->warnings);
+        $this->assertSame('Failed to delete expired session file.', $logger->warnings[0]['message']);
+        $this->assertSame($path, $logger->warnings[0]['context']['path']);
+    }
+
+    #[TestDox('stays silent on the happy path')]
+    public function testHappyPathLogsNothing(): void
+    {
+        $logger = new WarningCollectingLogger();
+        $store = new FileSessionStore($this->directory, logger: $logger);
+        $id = new UuidV4();
+
+        $store->write($id, 'payload');
+        $store->read($id);
+        $store->destroy($id);
+        $store->gc();
+
+        $this->assertSame([], $logger->warnings);
+    }
+}
+
+/**
+ * Keeps every warning with its context, so a silent failure can be told apart
+ * from one the operator was told about.
+ */
+final class WarningCollectingLogger extends AbstractLogger
+{
+    /** @var list<array{message: string, context: array<string, mixed>}> */
+    public array $warnings = [];
+
+    /**
+     * @param string|\Stringable   $message
+     * @param array<string, mixed> $context
+     */
+    public function log($level, $message, array $context = []): void
+    {
+        if (LogLevel::WARNING === $level) {
+            $this->warnings[] = ['message' => (string) $message, 'context' => $context];
+        }
+    }
 }