fix: stop leaking one orphaned session file per request in the Pixel buffer - #609
Open
tiacocstoe wants to merge 1 commit into
Open
tiacocstoe wants to merge 1 commit into
tiacocstoe wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue: PrestaShop/PrestaShop#42389
What this PR does
TemplateBuffer::init()always builds aMockFileSessionStorage, even when it is handed an empty$userId. In that caseMockArraySessionStorage::start()generates a random session id:The file written at shutdown is therefore keyed by an id that no later request will ever produce
again. It can never be read back, it is never reused, and
MockFileSessionStorageimplements nogarbage collection, so it stays on disk permanently.
This PR keeps file backed storage for the case it was designed for, a stable identifier, and falls
back to in memory storage otherwise.
Why
$userIdis empty for most trafficPixelHandler::findIdentifierFromContext():Customer::$id_guestis a public property that is not declared inCustomer::$definition['fields'], soObjectModelnever hydrates it. PrestaShop keeps the guestid on the cookie (
Guest::setNewGuest()writes$cookie->id_guest), not on the customer object.For an anonymous visitor the first branch can never match, and the second only matches visitors who
already have a cart.
Filename distribution measured on a production shop confirms it:
Impact
Measured on a medium traffic PrestaShop 1.7.6.8 shop running ps_facebook 1.38.17 with an active
Pixel. The directory was counted three times in one morning:
272 files in 14 minutes, roughly 28 000 files per day. The shop reached its hosting limit of
5 000 000 inodes; clearing
var/cachereleased about 3 460 000 inodes, some four months ofaccumulation.
The failure mode is quiet: the files are around 200 bytes each, so 18 000 of them take 3.5 MB and no
disk usage alert ever fires. Only inode counting reveals it. And because the directory sits inside
var/cache, any routine cache clear resets the counter, which is presumably why this has goneunnoticed since v1.24.0.
Behaviour change
None that is observable. Requests without a stable identifier already could not restore their buffer
on a subsequent request, because the id was random every time. This PR stops writing a file that
nobody could ever read. Requests carrying
cart_<id>keep their current cross page behaviour.Not covered by this PR
findIdentifierFromContext()reading$context->customer->id_guestis dead code and shouldprobably read
$context->cookie->id_guestinstead. I deliberately left it out: it would notfix this leak, because
Cookie::makeNewLog()callsGuest::setNewGuest()on every cookielessrequest, so crawlers would simply generate
guest_<id>files at the same rate. It deserves itsown change.
cart_<id>files are also never cleaned up once the cart is gone. Agc()pass or a TTL onthe directory would bound that too.
TemplateBuffer::clean()discards the return value ofgetFlashBag()->get()and is never calledanywhere in the module.
Regression origin
Introduced by
0f3a36d,2023-01-12, "Improve Pixel buffer to keep events between different pages", which replaced the
previous in memory string buffer with file backed session storage.
First release containing it: v1.24.0-beta.1 (2023-01-31), stable v1.24.0 (2023-02-06). Neither
TemplateBuffer.phpnorPixelHandler.phphas been touched since, so every release up to thecurrent v1.38.19 is affected.
How to test
ls var/cache/prod/ps_facebook_sessions | wc -lfor i in $(seq 1 20); do curl -s https://shop.example/ -o /dev/null; doneBefore this PR the count grows by one per request. After it, the count stays flat while
cart_<id>files continue to be written and reused for visitors with a cart.