diff --git a/src/Server/Session/FileSessionStore.php b/src/Server/Session/FileSessionStore.php index e218e75a..8d08bae0 100644 --- a/src/Server/Session/FileSessionStore.php +++ b/src/Server/Session/FileSessionStore.php @@ -14,6 +14,8 @@ 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 @@ public function __construct( 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)) { - @mkdir($this->directory, 0775, true); + if (!is_dir($this->directory) && !@mkdir($this->directory, 0775, true) && !is_dir($this->directory)) { + $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 @@ public function read(Uuid $id): string|false $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 @@ public function write(Uuid $id, string $data): bool $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 @@ public function write(Uuid $id, string $data): bool 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 @@ public function destroy(Uuid $id): bool { $path = $this->pathFor($id); - if (is_file($path)) { - @unlink($path); + if (is_file($path) && !@unlink($path) && is_file($path)) { + $this->logger->warning('Failed to delete session file.', [ + 'path' => $path, + 'error' => error_get_last()['message'] ?? 'unknown', + ]); } return true; @@ -119,6 +147,11 @@ public function gc(): array $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 @@ public function gc(): array $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); } } diff --git a/tests/Unit/Server/Session/FileSessionStoreTest.php b/tests/Unit/Server/Session/FileSessionStoreTest.php index a2b8729c..ed22e868 100644 --- a/tests/Unit/Server/Session/FileSessionStoreTest.php +++ b/tests/Unit/Server/Session/FileSessionStoreTest.php @@ -16,6 +16,8 @@ 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 @@ public function testUnwritableDirectoryThrowsPackageException(): void 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}> */ + public array $warnings = []; + + /** + * @param string|\Stringable $message + * @param array $context + */ + public function log($level, $message, array $context = []): void + { + if (LogLevel::WARNING === $level) { + $this->warnings[] = ['message' => (string) $message, 'context' => $context]; + } + } }