Fix session reads and saves across processes

src/Session/Session.php

@@ -12,7 +12,10 @@ class Session implements SessionInterface
 
     private array $data;
 
-    private array $original = [];
+    /**
+     * The JSON encoding of the data as last loaded or saved.
+     */
+    private ?string $original = null;
 
     private bool $loaded = false;
 
@@ -29,6 +32,11 @@ public function __construct(string $id = 'default', array $data = [], SessionSto
         $this->storage = $storage;
     }
 
+    public function getId(): string
+    {
+        return $this->id;
+    }
+
     public function setStorage(SessionStorageInterface $storage): void
     {
         $this->storage = $storage;
@@ -62,11 +70,19 @@ public function save(): void
             return;
         }
         $this->lazyLoad();
-        if ($this->data === $this->original) {
+        $encoded = $this->encode();
+        if ($encoded !== null && $encoded === $this->original) {
             return;
         }
 
         $this->storage->save($this->id, $this->data);
+        $this->original = $encoded;
+    }
+
+    public function reload(): void
+    {
+        $this->loaded = false;
+        $this->lazyLoad();
     }
 
     /**
@@ -76,8 +92,18 @@ private function lazyLoad(): void
     {
         if (! $this->loaded && isset($this->storage)) {
             $this->data = $this->storage->load($this->id);
-            $this->original = $this->data;
+            $this->original = $this->encode();
             $this->loaded = true;
         }
     }
+
+    /**
+     * Encode the data as JSON, so that changes within objects are detected.
+     */
+    private function encode(): ?string
+    {
+        $encoded = json_encode($this->data);
+
+        return $encoded === false ? null : $encoded;
+    }
 }

src/Session/SessionInterface.php

@@ -8,6 +8,11 @@
 
 interface SessionInterface
 {
+    /**
+     * Get the session ID.
+     */
+    public function getId(): string;
+
     /**
      * Set the storage for this session.
      */
@@ -30,6 +35,13 @@ public function get(string $key): mixed;
      */
     public function save();
 
+    /**
+     * Reload the session data from storage, if storage is defined.
+     *
+     * Unsaved changes are discarded.
+     */
+    public function reload();
+
     /**
      * Clear the session data.
      */

src/Session/Storage/File.php

@@ -44,14 +44,17 @@ public function save(string $sessionId, array $data): void
 
     public function load(string $sessionId): array
     {
-        $data = [];
         $filename = $this->getFilename($sessionId);
-        if (is_readable($filename)) {
-            $raw = file_get_contents($filename);
-            if ($raw !== false) {
-                $data = json_decode($raw, true);
-            }
+        if (! is_readable($filename) || ! ($handle = fopen($filename, 'r'))) {
+            return [];
         }
+        // A shared lock avoids reading a file that save() has truncated but not yet written.
+        // If locking is unsupported, read anyway.
+        flock($handle, LOCK_SH);
+        $raw = stream_get_contents($handle);
+        flock($handle, LOCK_UN);
+        fclose($handle);
+        $data = $raw !== false ? json_decode($raw, true) : [];
 
         return is_array($data) ? $data : [];
     }

tests/Session/SessionTest.php

@@ -0,0 +1,121 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Platformsh\Client\Tests\Session;
+
+use PHPUnit\Framework\TestCase;
+use Platformsh\Client\Session\Session;
+use Platformsh\Client\Session\Storage\SessionStorageInterface;
+
+class SessionTest extends TestCase
+{
+    private SessionStorageInterface $storage;
+
+    protected function setUp(): void
+    {
+        $this->storage = new class() implements SessionStorageInterface {
+            public array $sessions = [];
+
+            public int $saveCount = 0;
+
+            public function load(string $sessionId): array
+            {
+                return $this->sessions[$sessionId] ?? [];
+            }
+
+            public function save(string $sessionId, array $data): void
+            {
+                $this->saveCount++;
+                $this->sessions[$sessionId] = $data;
+            }
+        };
+    }
+
+    public function testGetId(): void
+    {
+        $this->assertSame('default', (new Session())->getId());
+        $this->assertSame('foo', (new Session('foo'))->getId());
+    }
+
+    public function testSaveOnlyWritesChanges(): void
+    {
+        $session = new Session('test', [], $this->storage);
+        $session->save();
+        $this->assertSame(0, $this->storage->saveCount);
+
+        $session->set('token', 'foo');
+        $session->save();
+        $this->assertSame(1, $this->storage->saveCount);
+        $this->assertSame([
+            'token' => 'foo',
+        ], $this->storage->sessions['test']);
+
+        $session->save();
+        $session->set('token', 'foo');
+        $session->save();
+        $this->assertSame(1, $this->storage->saveCount);
+
+        $session->set('token', 'bar');
+        $session->save();
+        $this->assertSame(2, $this->storage->saveCount);
+    }
+
+    public function testSaveWritesChangedObject(): void
+    {
+        $value = new class() implements \JsonSerializable {
+            public string $token = 'foo';
+
+            public function jsonSerialize(): mixed
+            {
+                return $this->token;
+            }
+        };
+        $session = new Session('test', [], $this->storage);
+        $session->set('token', $value);
+        $session->save();
+        $this->assertSame(1, $this->storage->saveCount);
+
+        $value->token = 'bar';
+        $session->save();
+        $this->assertSame(2, $this->storage->saveCount);
+    }
+
+    public function testReload(): void
+    {
+        $session = new Session('test', [], $this->storage);
+        $this->storage->sessions['test'] = [
+            'token' => 'old',
+        ];
+        $this->assertSame('old', $session->get('token'));
+
+        // Another process changes the stored data.
+        $this->storage->sessions['test'] = [
+            'token' => 'new',
+        ];
+        $this->assertSame('old', $session->get('token'));
+        $session->reload();
+        $this->assertSame('new', $session->get('token'));
+
+        // Reloaded data is the new baseline for save().
+        $session->save();
+        $this->assertSame(0, $this->storage->saveCount);
+    }
+
+    public function testReloadDiscardsUnsavedChanges(): void
+    {
+        $session = new Session('test', [], $this->storage);
+        $session->set('token', 'unsaved');
+        $session->reload();
+        $this->assertNull($session->get('token'));
+    }
+
+    public function testReloadWithoutStorage(): void
+    {
+        $session = new Session('test', [
+            'token' => 'foo',
+        ]);
+        $session->reload();
+        $this->assertSame('foo', $session->get('token'));
+    }
+}

tests/Session/Storage/FileTest.php

@@ -0,0 +1,81 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Platformsh\Client\Tests\Session\Storage;
+
+use PHPUnit\Framework\TestCase;
+use Platformsh\Client\Session\Storage\File;
+
+class FileTest extends TestCase
+{
+    private string $dir;
+
+    protected function setUp(): void
+    {
+        $this->dir = sys_get_temp_dir() . '/platformsh-client-test-' . bin2hex(random_bytes(8));
+    }
+
+    protected function tearDown(): void
+    {
+        $filename = $this->dir . '/sess-test/sess-test.json';
+        if (file_exists($filename)) {
+            unlink($filename);
+        }
+        foreach ([$this->dir . '/sess-test', $this->dir] as $dir) {
+            if (is_dir($dir)) {
+                rmdir($dir);
+            }
+        }
+    }
+
+    public function testSaveAndLoad(): void
+    {
+        $storage = new File($this->dir);
+        $this->assertSame([], $storage->load('test'));
+        $storage->save('test', [
+            'foo' => 'bar',
+        ]);
+        $this->assertSame([
+            'foo' => 'bar',
+        ], $storage->load('test'));
+        $storage->save('test', []);
+        $this->assertSame([], $storage->load('test'));
+    }
+
+    public function testLoadWaitsForExclusiveLock(): void
+    {
+        $storage = new File($this->dir);
+        $storage->save('test', [
+            'token' => 'old',
+        ]);
+        $filename = $this->dir . '/sess-test/sess-test.json';
+
+        // A child process truncates the file under an exclusive lock, as
+        // file_put_contents() does, and writes the new data after a delay.
+        $script = <<<'PHP'
+            $h = fopen($argv[1], 'c');
+            flock($h, LOCK_EX);
+            ftruncate($h, 0);
+            echo "locked\n";
+            usleep(500000);
+            fwrite($h, '{"token":"new"}');
+            fflush($h);
+            flock($h, LOCK_UN);
+            fclose($h);
+            PHP;
+        $process = proc_open([PHP_BINARY, '-r', $script, $filename], [
+            1 => ['pipe', 'w'],
+        ], $pipes);
+        $this->assertIsResource($process);
+        $this->assertSame("locked\n", fgets($pipes[1]));
+
+        $data = $storage->load('test');
+
+        fclose($pipes[1]);
+        $this->assertSame(0, proc_close($process));
+        $this->assertSame([
+            'token' => 'new',
+        ], $data);
+    }
+}