Skip to content

[Server] Log filesystem failures in FileSessionStore - #488

Open
chr-hertel wants to merge 2 commits into
modelcontextprotocol:mainfrom
chr-hertel:fix/issue-23-filesession-logging
Open

[Server] Log filesystem failures in FileSessionStore#488
chr-hertel wants to merge 2 commits into
modelcontextprotocol:mainfrom
chr-hertel:fix/issue-23-filesession-logging

Conversation

@chr-hertel

Copy link
Copy Markdown
Member

FileSessionStore now takes an optional PSR-3 logger (defaulting to NullLogger, matching the SDK convention) and warns when a filesystem operation fails in a way that loses data: failed mkdir, session file read/write/move failures, failed unlinks, and opendir failure during GC.

Benign race suppressions (e.g. filemtime on a concurrently deleted file) keep their @.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

GC still reports failed session-file deletions as successful without logging them.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds PSR-3 warning logs for filesystem failures in FileSessionStore.

Changes:

  • Injects an optional logger with NullLogger fallback.
  • Logs directory, read, write, move, deletion, and GC-open failures.
  • Adds warning and happy-path tests.
File summaries
File Description
src/Server/Session/FileSessionStore.php Adds filesystem failure logging.
tests/Unit/Server/Session/FileSessionStoreTest.php Tests selected warning paths.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.


$dir = @opendir($this->directory);
if (false === $dir) {
$this->logger->warning('Failed to open session directory for garbage collection.', [
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants