Skip to content

fix: persist config hash to disk so hasChanges works correctly across PHP-FPM processes#56

Merged
ankurv02 merged 6 commits into
masterfrom
CBP-51066
Jun 26, 2026
Merged

fix: persist config hash to disk so hasChanges works correctly across PHP-FPM processes#56
ankurv02 merged 6 commits into
masterfrom
CBP-51066

Conversation

@ankurv02

Copy link
Copy Markdown
Contributor
  • hasChanges in ConfigurationFetchedArgs was always true in PHP-FPM environments. Because each HTTP request gets a fresh PHP process, $_lastConfigurations was always null at the start of every request — meaning the SDK reported a config change on every single fetch, even when nothing changed.
  • Fixed by replacing the in-memory $lastConfigurations with a disk-based hash. On each fetch, the SDK computes md5(json_encode(config)), compares it against the last hash written to sys_get_temp_dir()/rollout/cache/config_hash{apiKey}.txt, and persists the new hash if it changed. This survives across processes and across workers.
  • This is the fix suggested by @AsafRollout in the PR fix(CBP-51066): PHP SDK cache status and distinctId fixes #54 review.

@snyk-prodsec-rw

snyk-prodsec-rw commented Jun 24, 2026

Copy link
Copy Markdown
Collaborator

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@ankurv02 ankurv02 requested review from a team, AsafRollout and markawm June 24, 2026 13:58
Comment thread src/Rox/Core/Core.php Outdated
sys_get_temp_dir(),
'rollout',
'cache',
'config_hash_' . md5($this->_sdkSettings->getApiKey()) . '.txt'

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

api key is short already; any benefit to hashing it, rather than just using as-is?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed. The API key is now used directly as part of the cache key string

Comment thread src/Rox/Core/Core.php Outdated
if (!is_dir($dir)) {
@mkdir($dir, 0755, true);
}
@file_put_contents($hashFile, $currentHash);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Might just be me not understanding what the php is doing, but iiuc this is writing a file containing an md5 hash of the client data; is that right? Presumably elsewhere there is some code that stores the actual client data, so that it can be loaded? Why not just load & md5() the actual client data file that already exists? Is it to have a smaller file to keep reading here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed. We now use FilesystemCache->fetch() and ->save() — the same Doctrine cache abstraction used everywhere else.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

See this has changed a bit, but unless I'm misreading the php, isn't it still saving a separate file containing the hash of the client data? (in my comment I was asking about not storing a separate file, and instead just getting md5('/tmp/client-data-aaa-111-bbb-222.json)` for e.g.)
Another thought to managing a separate file: if these are all separate processes, there is no synchronisation between the access of the two different files (client data & this hash metadata file) - could they get out of sync? Or at least, processes get false results if a different process were in the middle of updating two separate files?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

better approach 👍 now removing the separate hash file entirely — hasChanges is now derived directly from !$result->isFromCache(). Guzzle's cache middleware already sets isFromCache=true when the response is a byte-identical cache HIT, so that single in-memory flag replaces both the hash computation and the separate persisted file. No second file, no two-file race condition.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎉 that sounds much cleaner and simpler.

@ankurv02 ankurv02 requested a review from markawm June 25, 2026 05:50

@markawm markawm left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

lgtm as someone who doesn't know php; would be good if @AsafRollout can double check.

Comment thread src/Rox/Core/Core.php Outdated

$hasChanges = ($this->_lastConfigurations == null || !$this->_lastConfigurations->equals($result));
$this->_lastConfigurations = $result;
$hasChanges = !$result->isFromCache();

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.

ah?
isFromCache doesn't mean if it has changes
we fetch every time the cache expire from the network, but the files can be the same... 🤷

@AsafRollout AsafRollout Jun 25, 2026

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.

i see the other conversation about it, asked Gemini:

The X-Kevinrob-Cache header tells you exactly how the middleware handled the lifecycle of that specific request:
HIT: The data was fresh in your cache. Guzzle intercepted the request and returned it instantly without ever talking to the remote server.
MISS: The data was not in your cache (or had completely expired). Guzzle went to the network, downloaded a full [2](https://ditt.to/caching-requests-with-guzzle-2.html)00 OK response, and saved it to the cache.
REVALIDATED: The data had expired, so Guzzle went to the network with validation headers (ETag/Last-Modified). The server responded with a 304 Not Modified. The middleware updated the cache expiration and handed you the data.

from what i understand, on each page load, we will do setup, which will fetch
first time will be a MISS, isFromCache should be false, and hasChanges=true (can be false if we have storage cache, but we can ignore it)
all other times until expiration we will get a HIT, isFromCache=true, hasChanges=false ✔️
on expiration: every 30 or 60 secs, or whenever the user set the expiration to be, we should get REVALIDATED or a MISS
the MISS in this case, should be like the first time MISS ✔️
but when we get REVALIDATED (most of the updates probably) it will return isFromCache=false, but also hasChanges=true, i think it should be hasChanges=isFromCache=false

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Got it 👍 . so before this, isFromCache() only returned true for a cache HIT, so a REVALIDATED response (server said "nothing changed, use what you have") was treated the same as a full network fetch — meaning hasChanges would fire true even though the config was identical.

What we have now: a CacheStatus class that owns the three states (HIT, MISS, REVALIDATED) and the logic around them. isFromCache() still means HIT only — which drives whether we show AppliedFromLocalStorage or AppliedFromNetwork. But hasChanges now uses a separate isContentUnchanged() which returns true for both HIT and REVALIDATED. So a 304 from the server no longer triggers a false "config changed" callback.

Also added a test that covers exactly the case you described — REVALIDATED should be isFromCache=false but isContentUnchanged=true.

@ankurv02 ankurv02 requested a review from AsafRollout June 26, 2026 06:23
@ankurv02 ankurv02 merged commit 9d4c9b0 into master Jun 26, 2026
13 checks passed
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.

4 participants