MT-2107 Add persistent session support with automatic token renewal and retry#3
MT-2107 Add persistent session support with automatic token renewal and retry#3filiptorphage-mjuk wants to merge 40 commits into
Conversation
…ersistent sessions and token renewal.
…try point for calling REST API
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds persistent FileMaker session-token caching with TTL renewal and automatic single-retry on token invalidation. The feature introduces a session-cache abstraction layer with APCu backing, integrates cache-aware token lifecycle into the communication provider, and exposes configuration methods through the public API. ChangesSession Cache and Token Reuse
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR adds opt-in persistent session token caching to
Confidence Score: 5/5Safe to merge — all previously identified issues (cache key security, strict comparison cast, null guards, scope-flag bleed) are confirmed fixed in this revision. The session caching logic, retry path, and scope-flag lifecycle are all consistent and correct. The one remaining note is a defensive gap in extractErrorCode() where an empty messages array from the FileMaker server would cause a PHP 8 TypeError, but this is a theoretical edge case given the API spec always includes at least one message element. src/Supporting/CommunicationProvider.php — new extractErrorCode() helper could use an extra null guard on messages[0]. Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant FMDataAPI
participant CommunicationProvider
participant SessionCache
participant FileMakerServer
Caller->>FMDataAPI: startCommunication()
FMDataAPI->>CommunicationProvider: startCommunication()
CommunicationProvider->>CommunicationProvider: login()
CommunicationProvider->>SessionCache: get()
alt Cache hit
SessionCache-->>CommunicationProvider: token
CommunicationProvider->>CommunicationProvider: "accessToken = cached token"
else Cache miss
SessionCache-->>CommunicationProvider: null
CommunicationProvider->>FileMakerServer: POST /sessions (Basic auth)
FileMakerServer-->>CommunicationProvider: 200 + token
CommunicationProvider->>SessionCache: set(token, ttl)
end
CommunicationProvider-->>FMDataAPI: "keepAuth = true"
Caller->>FMDataAPI: layout('X').query()
FMDataAPI->>CommunicationProvider: callRestAPI(...)
CommunicationProvider->>CommunicationProvider: callRestAPIWithoutRetry(...)
CommunicationProvider->>FileMakerServer: GET /layouts/X/records
alt 952 or 112 error and retry enabled
FileMakerServer-->>CommunicationProvider: 952 Invalid token
CommunicationProvider->>SessionCache: delete()
CommunicationProvider->>CommunicationProvider: login() fresh
CommunicationProvider->>FileMakerServer: POST /sessions
FileMakerServer-->>CommunicationProvider: 200 + new token
CommunicationProvider->>SessionCache: set(new token, ttl)
CommunicationProvider->>FileMakerServer: GET /layouts/X/records retry
FileMakerServer-->>CommunicationProvider: 200 + records
else Success
FileMakerServer-->>CommunicationProvider: 200 + records
end
Caller->>FMDataAPI: endCommunication()
FMDataAPI->>CommunicationProvider: endCommunication()
CommunicationProvider->>SessionCache: get()
alt Token matches cache
SessionCache-->>CommunicationProvider: same token
CommunicationProvider->>SessionCache: set(token, ttl) renew TTL
CommunicationProvider->>CommunicationProvider: "accessToken = null, return"
else Token replaced by another worker
SessionCache-->>CommunicationProvider: different token
CommunicationProvider->>FileMakerServer: DELETE /sessions/stale-token
end
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
src/Supporting/CommunicationProvider.php:1040-1048
If the FileMaker server ever returns a response where `messages` is an empty array, `$this->responseBody->messages[0]` evaluates to `null`, and the subsequent `property_exists(null, 'code')` call throws a `TypeError` in PHP 8 (`Argument #1 must be of type object|string, null given`). Adding an `is_object` guard keeps the method safe against malformed responses.
```suggestion
private function extractErrorCode(): int
{
$errorCode = -1;
if (is_object($this->responseBody) && property_exists($this->responseBody, 'messages')) {
$result = $this->responseBody->messages[0] ?? null;
$errorCode = is_object($result) && property_exists($result, 'code') ? intval($result->code) : -1;
}
return $errorCode;
}
```
Reviews (21): Last reviewed commit: "Enforced strict types declaration in Apc..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Supporting/CommunicationProvider.php`:
- Around line 305-309: The current TTL-renewal block in CommunicationProvider
(checking $this->sessionStore and $this->accessToken) unconditionally clears
$this->accessToken after calling PersistentSessionStore::set(), which can orphan
a server session if set() returns false; change the logic to check the boolean
return of $this->sessionStore->set($this->accessToken) and only null out
$this->accessToken and return when set() returns true, and if set() returns
false perform a fallback logout (call $this->logout() or the existing
server-side session invalidation path) instead of dropping the token so a
transient cache failure does not leak an active session.
- Around line 998-1003: The extracted error code from
responseBody->messages[0]->code is currently left as-is (string) which prevents
strict numeric comparisons in shouldRetryOnTokenError(); update
extractErrorCode() to cast the value to an int (e.g., (int) $result->code or
intval($result->code)) before assigning/returning so that extractErrorCode()
returns a true int (still defaulting to -1 when absent) and the strict checks in
shouldRetryOnTokenError() (=== 952 / === 112) work correctly.
- Around line 985-991: The current retry decision returns true for error codes
952 and 112 which causes unsafe automatic retries for non-idempotent verbs;
change the logic so only 952 triggers automatic retry (replace "return
$errorCode === 952 || $errorCode === 112;" with a check that returns true only
for 952), and if you want to support 112 make it opt-in: add a configuration
flag (e.g., $this->allowRetry112) and only consider 112 when that flag is true
AND the request is idempotent (use or add an isIdempotentRequest($method) helper
called from callRestAPI()/retry decision). Ensure you reference
extractErrorCode() where the error is read and the callRestAPI() path that
triggers retries so the gating logic is applied consistently.
- Around line 570-576: The calls to callRestAPIWithoutRetry() from login() and
logout() are invoking a private method which prevents TestProvider from
overriding it to mock /sessions; change the visibility of the
callRestAPIWithoutRetry() method from private to protected so TestProvider can
override it, then update any subclass overrides (e.g., in TestProvider) to
provide mocked responses for authentication endpoints; ensure references in
login(), logout(), and any other callers (lines near the calls to
callRestAPIWithoutRetry()) remain unchanged but now dispatch to the protected
method.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6f7ca6e4-c153-4104-9e91-2db99ee1e347
📒 Files selected for processing (7)
src/FMDataAPI.phpsrc/PersistentSession/ApcuSessionCache.phpsrc/PersistentSession/PersistentSessionStore.phpsrc/PersistentSession/SessionCacheInterface.phpsrc/Supporting/CommunicationProvider.phpsrc/Supporting/FileMakerLayout.phptest/TestProvider.php
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/Supporting/CommunicationProvider.php (3)
570-576:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
callRestAPIWithoutRetry()isprivate, blocking test mocking of/sessionsendpoints.
login()(line 570) andlogout()(line 610) both callcallRestAPIWithoutRetry()directly. Because the method isprivate,TestProvidercannot override it, so test mode hits real authentication endpoints while other API calls are mocked, breaking test isolation.This is still unresolved from the previous review cycle.
Also applies to: 608-613
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Supporting/CommunicationProvider.php` around lines 570 - 576, The private visibility of callRestAPIWithoutRetry prevents TestProvider from overriding it and causes real /sessions calls during tests; change the method declaration of callRestAPIWithoutRetry from private to protected so subclasses (e.g., TestProvider) can mock/override it, then keep login() and logout() calling callRestAPIWithoutRetry as before (no call-site changes needed) and run tests to confirm session endpoints are now intercepted by the test subclass.
305-309:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
endCommunication(): TTL-renewal path silently orphans the server session on cache failure.If
sessionStore->set()returnsfalse(ornull) the token is nulled and the method returns, leaving a live server-side session with no cache entry pointing to it.
This is still unresolved from the previous review cycle.🛡️ Proposed fix
if ($this->sessionStore !== null && $this->accessToken !== null) { if ($this->sessionStore->get() === $this->accessToken) { - $this->sessionStore->set($this->accessToken); // renew TTL - $this->accessToken = null; - return; + $renewed = $this->sessionStore->set($this->accessToken); // renew TTL + if ($renewed !== false) { + $this->accessToken = null; + return; + } + // Cache write failed; fall through and logout to avoid orphaning the server session. }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Supporting/CommunicationProvider.php` around lines 305 - 309, In endCommunication() the TTL-renewal branch calls $this->sessionStore->set($this->accessToken) but then unconditionally nulls $this->accessToken and returns, which orphaned server sessions if set() fails; change this to check the return value of $this->sessionStore->set(...): if it returns truthy proceed to null $this->accessToken and return, otherwise do not null the token, log or surface the failure (via process logger or throw) and leave $this->accessToken intact so future cleanup can retry; reference the endCommunication() method and the calls to $this->sessionStore->get() and $this->sessionStore->set() when making the change.
985-991:⚠️ Potential issue | 🟠 Major | ⚡ Quick winError 112 retry still fires on non-idempotent verbs.
callRestAPI()wraps POST, PATCH, and DELETE as well as GET. Error 112 ("Window is missing") is not a Data API auth-invalidation signal like 952, so retrying it for mutating verbs is unsafe — the server-side outcome is already ambiguous.This is still unresolved from the previous review cycle.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Supporting/CommunicationProvider.php` around lines 985 - 991, The current auth-retry check always treats error code 112 as an auth-invalid signal, causing unsafe retries for mutating verbs; update the logic so error 952 still always returns true but error 112 only triggers a retry for safe/idempotent HTTP methods (e.g., GET, HEAD, OPTIONS) used by callRestAPI(), by passing the HTTP method (or reading the request method available in the current context) into the check (the function using extractErrorCode() / the code that decides retries) and returning true for 112 only when the method is idempotent; ensure POST/PATCH/DELETE do not retry on 112.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Supporting/CommunicationProvider.php`:
- Around line 716-726: Initialize a $reauthed flag to false before the try, then
after the existing try/catch add a check that if $reauthed is false you rethrow
the original $firstAttempt exception so the original failure is not lost; keep
the existing logic that sets $reauthed = $this->login() and $this->keepAuth =
true when $resumeScope, and do not throw inside the try (to avoid circular
chaining) — use the post-try check to throw $firstAttempt when !$reauthed so
callers see the original error.
---
Duplicate comments:
In `@src/Supporting/CommunicationProvider.php`:
- Around line 570-576: The private visibility of callRestAPIWithoutRetry
prevents TestProvider from overriding it and causes real /sessions calls during
tests; change the method declaration of callRestAPIWithoutRetry from private to
protected so subclasses (e.g., TestProvider) can mock/override it, then keep
login() and logout() calling callRestAPIWithoutRetry as before (no call-site
changes needed) and run tests to confirm session endpoints are now intercepted
by the test subclass.
- Around line 305-309: In endCommunication() the TTL-renewal branch calls
$this->sessionStore->set($this->accessToken) but then unconditionally nulls
$this->accessToken and returns, which orphaned server sessions if set() fails;
change this to check the return value of $this->sessionStore->set(...): if it
returns truthy proceed to null $this->accessToken and return, otherwise do not
null the token, log or surface the failure (via process logger or throw) and
leave $this->accessToken intact so future cleanup can retry; reference the
endCommunication() method and the calls to $this->sessionStore->get() and
$this->sessionStore->set() when making the change.
- Around line 985-991: The current auth-retry check always treats error code 112
as an auth-invalid signal, causing unsafe retries for mutating verbs; update the
logic so error 952 still always returns true but error 112 only triggers a retry
for safe/idempotent HTTP methods (e.g., GET, HEAD, OPTIONS) used by
callRestAPI(), by passing the HTTP method (or reading the request method
available in the current context) into the check (the function using
extractErrorCode() / the code that decides retries) and returning true for 112
only when the method is idempotent; ensure POST/PATCH/DELETE do not retry on
112.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: dbbce53c-9f59-47df-b5b7-5d1f0376f892
📒 Files selected for processing (1)
src/Supporting/CommunicationProvider.php
filiptorphage-mjuk
changed the title
1 similar comment
|
Tip For best results, initiate chat on the files or code changes.
Let me know how I can help! 🐇 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/TestProvider.php (1)
95-103:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
validResponse()silentnullreturn will cause aTypeErrorin retry test scenarios.
validResponse()returnsnullimplicitly when no hash matches. The new retry path inCommunicationProvider::callRestAPI()callscallRestAPIWithoutRetry()a second time after re-login with a freshly issued token. That new token changes theAuthorizationheader, producing a different SHA-1 hash that won't match any entry in$this->responses. The resultingnullreaches line 72 ($this->curlInfo = $response['curlinfo']), throwing aTypeErrorin PHP 8.No current test exercises the retry path (no 952/112 mock exists in
buildResponses()), so this is latent — but it will break as soon as retry tests are added.🛡️ Proposed fix: add a safe fallback return
private function validResponse($input) { $hash = sha1(json_encode($input)); foreach ($this->responses as $key => $value) { if ($hash === $key) { return $value; } } + // No registered mock for this request. Return a neutral response to + // avoid a TypeError on array access in callRestAPIWithoutRetry(). + return [ + 'response' => '{"response":{},"messages":[{"code":"0","message":"OK"}]}', + 'curlerror' => 0, + 'curlerrormessage'=> '', + 'curlinfo' => ['http_code' => 200], + ]; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/TestProvider.php` around lines 95 - 103, validResponse() can return null when no hash matches causing a TypeError downstream; update validResponse (in TestProvider::validResponse) to always return a safe fallback instead of implicit null—either an array with the expected keys (e.g., 'curlinfo' and other keys used by callRestAPI()/callRestAPIWithoutRetry) or throw a clear exception. Ensure the fallback shape matches what callRestAPI reads (so $response['curlinfo'] exists) or adjust tests in buildResponses to include retry hashes (952/112) if you prefer explicit mocks; update only TestProvider::validResponse to return that default response when no $hash matches.
♻️ Duplicate comments (1)
src/Supporting/CommunicationProvider.php (1)
988-994:⚠️ Potential issue | 🟠 Major | ⚡ Quick winError 112 retry safety — ongoing discussion from previous review.
This was raised in the prior round (lines 988-994) and is explicitly unresolved: the maintainer deferred the decision to the library author. The core concern remains — error 112 ("Window is missing") is not a Data API token-invalidation code like 952, and retrying it unconditionally on non-idempotent verbs (POST/PATCH/DELETE) could cause duplicate writes if the server processed the request before returning the error.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Supporting/CommunicationProvider.php` around lines 988 - 994, The current boolean return treats FileMaker error 112 the same as token-invalid (952), which can trigger unsafe automatic retries for non-idempotent verbs; update the method that calls extractErrorCode() so that 952 remains always considered an auth/refresh condition but 112 is only treated as retry-safe when the original HTTP method is idempotent (e.g., GET, HEAD, OPTIONS, PUT) — if you can't determine the request method from this class, remove 112 from the automatic-retry path and surface/log it for manual handling; reference extractErrorCode() and the enclosing method that returns the boolean to implement this conditional check (or the alternative of dropping 112).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Supporting/CommunicationProvider.php`:
- Around line 719-729: The current retry catch path can leave $this->keepAuth
stuck as false when $this->login() returns false (no exception) causing scope
state corruption; update the retry logic in CommunicationProvider::callRestAPI
(the block that calls $this->login(), checks $reauthed and then calls
callRestAPIWithoutRetry) to explicitly restore $this->keepAuth to the original
$resumeScope value whenever reauth fails (i.e., when $reauthed is false and no
exception was thrown) so a failed re-login does not silently drop the caller's
startCommunication()/endCommunication() scope; ensure you reference the existing
$resumeScope, $reauthed, $this->keepAuth and continue to call or skip
callRestAPIWithoutRetry exactly as before.
---
Outside diff comments:
In `@test/TestProvider.php`:
- Around line 95-103: validResponse() can return null when no hash matches
causing a TypeError downstream; update validResponse (in
TestProvider::validResponse) to always return a safe fallback instead of
implicit null—either an array with the expected keys (e.g., 'curlinfo' and other
keys used by callRestAPI()/callRestAPIWithoutRetry) or throw a clear exception.
Ensure the fallback shape matches what callRestAPI reads (so
$response['curlinfo'] exists) or adjust tests in buildResponses to include retry
hashes (952/112) if you prefer explicit mocks; update only
TestProvider::validResponse to return that default response when no $hash
matches.
---
Duplicate comments:
In `@src/Supporting/CommunicationProvider.php`:
- Around line 988-994: The current boolean return treats FileMaker error 112 the
same as token-invalid (952), which can trigger unsafe automatic retries for
non-idempotent verbs; update the method that calls extractErrorCode() so that
952 remains always considered an auth/refresh condition but 112 is only treated
as retry-safe when the original HTTP method is idempotent (e.g., GET, HEAD,
OPTIONS, PUT) — if you can't determine the request method from this class,
remove 112 from the automatic-retry path and surface/log it for manual handling;
reference extractErrorCode() and the enclosing method that returns the boolean
to implement this conditional check (or the alternative of dropping 112).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 65626517-062c-4a1b-80c1-3e6c9c52d370
📒 Files selected for processing (2)
src/Supporting/CommunicationProvider.phptest/TestProvider.php
… not when the actual command fails (since it's only in the login method that `keepAuth` is being set.
|
@greptileai Would adding this block to the case where if ($this->resumeScopeAfterReauth) {
$this->keepAuth = true;
$this->resumeScopeAfterReauth = false;
} |
|
Yes, that fix is correct and necessary. Here's the exact scenario that exposes the gap:
The fix belongs at the cache-hit return: if ($cached !== null) {
$this->accessToken = $cached;
if ($this->resumeScopeAfterReauth) {
$this->keepAuth = true;
$this->resumeScopeAfterReauth = false;
}
return true;
}The same block already exists in the HTTP-login success path — it just needs to be mirrored here. Without it, |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@composer.json`:
- Around line 21-23: Reword the suggest message for ext-apcu to make it clear
the extension is optional and only needed when using ApcuSessionCache: update
the value under "suggest" (key "ext-apcu") in composer.json to a phrasing such
as "Optional: required only if you use ApcuSessionCache" or similar, referencing
ext-apcu and ApcuSessionCache so users understand it is not mandatory.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 02a7b43d-4638-4492-8b09-074e03ca76b2
📒 Files selected for processing (4)
composer.jsonsrc/FMDataAPI.phpsrc/SessionCache/ApcuSessionCache.phpsrc/Supporting/CommunicationProvider.php
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@greptileai What do I have to do to get the confidence score up to 5 of 5? |
|
Here are the concrete issues keeping the confidence score below 5/5: 1.
|
| Priority | Issue |
|---|---|
| High | Add if ($firstAttempt === null) return; early in callRestAPI() |
| High | Set $this->resumeScopeAfterReauth = false (not $resumeScope) on retry failure |
| High | Add unit tests covering the new retry and cache paths |
| Low | Add declare(strict_types=1) to ApcuSessionCache.php |
|
|
You're right on all three points — I apologize for the incorrect comments.
|
2 participants
Overview
By default the library logs in and out on every single database operation, which means a full authentication round-trip each time. This PR adds opt-in support for sharing a session token across PHP requests, so only the first request pays the cost of logging in — every subsequent request on the same server reuses the cached token instead.
Persistent sessions are entirely opt-in via a new
$sessionCacheconstructor parameter. The retry mechanism is also opt-in — without a session cache it requires explicitly callingsetRetryOnAccessTokenInvalidation(true), and with one it activates automatically. Omitting both keeps all existing behaviour intact.How to use it
Pass a cache backend to the constructor and the library handles the rest — storing the token after login, reusing it on subsequent requests, and automatically recovering if the token goes stale.
startCommunication()andendCommunication()are optional — token reuse works without them. They become useful when batching multiple operations in a single request, since within a scopelogin()andlogout()short-circuit without touching the cache on every call.endCommunication()also resets the 840-second cache TTL, which matters for longer-running scripts where you want the token to stay warm.Custom backends just need to extend
AbstractSessionCache(which itself implementsSessionCacheInterface):New files
AbstractSessionCache.php— public API. An abstract class with internal properties for managing the session cache used by FMDataAPI. This is what custom implementations will extend if custom cache implementations wants to be used.ApcuSessionCache.php— public API. Ready-made APCu implementation. Throws if APCu isn't available.SessionCacheInterface.php— public API. Three methods:get,set(with TTL),delete. These will need to be implemented if you want to extendAbstractSessionCache.Changes to existing files
Review carefully
src/Supporting/CommunicationProvider.php— the core of this PR. Most of the new logic lives here.$retryOnAccessTokenInvalidationand$sessionStoreproperties. Constructor acceptsPersistentSessionStore|null $sessionStore = null.startCommunication()(new) — setskeepAuthto the return value oflogin(), so it's only ever true when login actually succeeded.endCommunication()(new) — if the cached token matches ours, renews the TTL and skips the server logout. If tokens differ, another worker has replaced the cached token; ours is an orphan and getsDELETEd. Falls through tologout()when no cache is configured.login()— checks the cache before attempting an HTTP login. After a fresh login, stores the token in the cache. Changed to callcallRestAPIWithoutRetryinternally.logout()— three new guards: returns early ifaccessTokenis null; returns early if the token matches the cache; wraps theDELETEintry/finally.callRestAPI()/callRestAPIWithoutRetry()— the originalcallRestAPIis renamed tocallRestAPIWithoutRetryand made private. The newcallRestAPIwraps it with retry logic on 952/112, clearing the cache, re-logging in, and retrying once. The original exception is available viagetPrevious().Mechanical changes — safe to skim
FMDataAPI.php— new$sessionCacheconstructor parameter;startCommunication()andendCommunication()delegate to the provider; newsetRetryOnAccessTokenInvalidation()method.FileMakerLayout.php—startCommunication()andendCommunication()delegate to the provider.TestProvider.php— constructor updated to accept and forwardPersistentSessionStore|nullto the parent.Behavioural changes for existing users
These apply regardless of whether a session cache is configured.
Please verify all of these.
logout()null guard — previously, callinglogout()with no active session would fire aDELETE /sessions/request with a null token in the URL. Before it always threw an exception (ifthrowExceptionInErrorwas set). Now it immediately returns. This should never occur in the first place, and ifaccessTokenis already null, then we should already be logged out.logout()try/finally —accessTokenis now always nulled after aDELETEattempt, even if the request throws. Previously a thrown exception left it set.startCommunication()login-failure fix — This one were divided into two files. InFMDataAPI.php, thenkeepAuthwere never set tofalsein non-throwing mode. InFileMakerLayout.phpthenkeepAuthwere never set to false in case of a failed login at all. Now, in case of any error, thenkeepAuthis set to false.endCommunication()— When a session cache is enabled, this method now only logs out sessions where the localaccessTokendoes not match the cached session token. This is to ensure that one, and only one, session token is stored in the cache.setRetryOnAccessTokenInvalidation(true)to enable it. With a session cache it activates automatically, since a persistent session that can't recover from an expired token is effectively broken. The retry runs in a fresh session — session-scoped state like global fields set before the failure won't carry over.Known limitations
Currently, when a session cache is used, the value stored in the key-value pair only has its TTL renewed upon calling
endCommunication(). This is a deliberate choice as APCu is not atomic and this minimizes the risk of race conditions. This means that if an application does not callendCommunication()within 14 minutes from opening the session, then the session will be invalidated in the cache and a new session will be started on the next FileMaker Data API request.Race conditions and stale tokens
If multiple workers miss the cache simultaneously (on cold start or after a 952 clears it), each logs in independently and gets its own token. Last writer wins the cache; the rest become orphans and idle out within FM Server's 15-minute timeout.
endCommunication()mitigates this by explicitlyDELETEing a token when it finds the cache has already moved on, but workers that crash mid-scope won't get that cleanup. If your FM Server has a low per-user session limit and you anticipate frequent simultaneous cold starts, consider warming the cache from a single entry point before traffic arrives.Under sustained cache failure in
endCommunication()with high concurrency, orphaned tokens could approach FileMaker's session cap (error 953).Regarding tests
I have modified
TestProviderto take in$sessionCacheand replaced thecallRestAPI()override to now overridecallRestAPIWithoutRetry()(as I renamed that function). However, I'm not sure how the tests in this repositories are used, and I'm not sure how to even run them, so I'll leave the testing to you (msyk). If you feel the need to add any tests to this, feel free to do so.Summary by CodeRabbit