Repository navigation
Fix session reads and saves across processes - #108
Conversation
File::save() writes with file_put_contents(..., LOCK_EX), which truncates the file after taking the lock. File::load() read without a lock, so a concurrent read could see an empty or partial file and return no data. load() now takes a shared lock (flock LOCK_SH) before reading, so it waits for any in-progress write to finish. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A Session loaded its storage once, so a caller could not see changes saved by another process sharing the same session, such as a rotated refresh token. reload() discards the in-memory data and loads it from storage again. getId() returns the session ID. Both methods are added to SessionInterface, since callers get the session through ConnectorInterface::getSession(). Other implementations of the interface must add them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
save() skips writing when the data matches the snapshot taken at load time, but the snapshot was not updated after a write. So once the session had changed, every later save() wrote again, even with no further changes. This is costly for storage backends that start a process per write, such as credential helpers. The snapshot is now the JSON encoding of the data, and it is updated after each save. Comparing JSON also detects changes inside JsonSerializable objects, which would share a reference with a plain array copy. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Empty session saves bypass locking, allowing clears to race with concurrent reads or writes.
Review effort: Balanced
Findings: 1
What changed in this PR
Adds cross-process session synchronization and explicit session reloading while avoiding redundant storage writes.
Changes:
- Adds locked file reads and concurrency tests.
- Adds session ID/reload APIs.
- Tracks JSON snapshots after loads and saves.
| File | Description |
|---|---|
src/Session/Storage/File.php |
Reads session files under shared locks. |
src/Session/Session.php |
Adds reload/ID methods and JSON snapshots. |
src/Session/SessionInterface.php |
Exposes the new session APIs. |
tests/Session/Storage/FileTest.php |
Tests file storage and locking. |
tests/Session/SessionTest.php |
Tests reload and snapshot behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@upsun-dispatch review |
![upsun-dispatch[bot]](https://avatars.githubusercontent.com/in/4153575?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
Changes suggested — 🟡 1 warning · 🔵 1 minor point
🔍 Full review · 5 files reviewed
Verification
file_put_contents(..., LOCK_EX)opens in 'c' mode and truncates only after taking the lock, so the reader's LOCK_SH does keep it from seeing a truncated file.save()updates$originalonly afterstorage->save()returns, so a save that throws leaves the snapshot dirty and the next save retries.- If
encode()fails and returns null, the$encoded !== nullguard makessave()always write instead of wrongly skipping. reload()without storage leaves the constructor data alone, becauselazyLoad()checksisset($this->storage).Sessionis the only class in the repo that implementsSessionInterface, so the new interface methods break nothing inside the repo.
The PR adds tests/Session/SessionTest.php, which covers snapshot and skip-save behaviour, detection of changes inside JsonSerializable objects, and reload semantics. It also adds tests/Session/Storage/FileTest.php, which uses a child process to check that load() waits for the exclusive lock. Both run in the CI job lint-test (PHPUnit on PHP 8.2, ubuntu-latest). No test covers the path where flock() fails.
Review details
- Commit: 2e2b0cb
- Model: claude-opus-5-5
flock() can fail on filesystems that don't support locking. load() then returned no data, making a stored login look empty. It now reads without the lock in that case, as it did before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Note
Reviewed — No new issues found · 1 still open
🔁 Incremental · 1 file reviewed
Outstanding from earlier reviews:
- 🔵 #4150300197 —
src/Session/SessionInterface.php:43: Existing SessionInterface implementations break with a fatal error on upgrade. — SessionInterface still declares abstract getId() (line 14) and reload() (line 43), so existing implementations still break on upgrade.
Verification
load()now callsstream_get_contents()even whenflock(LOCK_SH)fails, so a readable session file is never reported as empty because of locking.- The
flock($handle, LOCK_UN)andfclose()calls after the read still run on every path that opened the handle. - An invalid or non-array JSON body still returns
[]via theis_array($data)check, so a failed lock has no effect on it.
tests/Session/Storage/FileTest.php covers the save/load round trip and waiting on an exclusive lock; no test covers the new unlocked read when flock fails. PHPUnit runs in the ci.yml workflow.
Review 2 of 10 for this pull request · View the full run
Fixes three issues in
src/Session/that affect processes sharing one session, where a stale refresh token can be re-sent after the auth server has rotated it:Storage\File::load()now reads under a shared lock.save()truncates the file after takingLOCK_EX, so an unlocked read could see an empty or partial file and return[].reload()andgetId()methods onSessionandSessionInterface, so callers can re-read storage to see another process's changes. They are on the interface because callers get the session fromConnectorInterface::getSession(); otherSessionInterfaceimplementations must add them.Session::save()now updates its snapshot after writing, so later saves with no changes don't write again (each write starts a process with credential-helper storage). The snapshot is JSON-encoded so that changes insideJsonSerializableobjects are still detected.Once released, the Upsun CLI (upsun/cli#194) could drop its workarounds: the
Filesubclass inlegacy/src/Session/FileStorage.php, and building a newSessionand tracking the session ID inApi::loadStoredToken().🤖 Generated with Claude Code