[loevgaard]

Closes #10.

Found by the functional tests in #13.

The bug

CachedClientContext memoizes the client and CachedCookieProvider memoizes the cookie of each request, but neither is reset between requests. Under PHP-FPM every request gets a new container, so this doesn't matter.

Long-running runtimes, like FrankenPHP's worker mode, RoadRunner and Swoole, reuse the container and only reset the services tagged kernel.reset. There, every visitor without a cookie gets the client of the first visitor the worker handled:

• they get that visitor's client id in their cookie
• they can read that visitor's metadata
• metadata they set is stored on that visitor's row

Apps that store tracking data like a gclid in the metadata would mix up visitors.

The fix

CachedClientContext implements Symfony\Contracts\Service\ResetInterface and is tagged kernel.reset.

CachedCookieProvider no longer caches under spl_object_hash($request). PHP reuses the handle of a freed object for the next one, so a new request could get the hash of a freed request and receive its cached cookie, i.e. another visitor's client id. As #10 suggests, the cache is now a WeakMap keyed by the request, so an entry goes away with its request. That's also why the cookie provider needs no reset. The cookie is wrapped in an array, because a WeakMap treats an entry holding null as missing.

symfony/service-contracts is now a direct dependency (^2.5 || ^3.0). It was already installed through symfony/dependency-injection.

The README now says to decorate setono_client.client_context.default instead of setono_client.client_context.cached. Symfony's services resetter references the cached service by its id, so it would reset a decorator of that id instead of the cache.

Tests

• LongRunningProcessTest handles several requests without rebooting the kernel, as a worker does:

  • each new visitor gets their own client
  • a visitor doesn't see or overwrite the previous visitor's metadata

  Both fail without the fix.

• Unit tests check that the client context resolves again after reset(), and that a new request doesn't get the cookie of a freed request with the same object handle. That test checks that the handle is reused, and fails with the old spl_object_hash() cache. The test kernel keeps its requests alive, so a functional test can't reproduce the reuse. The extension test checks the kernel.reset tag.

Testing

• PHPUnit: 101 tests pass on PHP 8.4 with Symfony 7.4, rebased on master with Fix configuring a metadata class that extends the bundle's Metadata #14. Merged with Reject unusable client ids and look up the entity manager lazily #20, they also pass with the lowest dependencies installed like CI does, on PHP 8.1 with Symfony 6.4 and on PHP 8.2 with Symfony 7.0
• PHPStan, ECS and composer-dependency-analyser pass
• Infection: 83 of 83 mutants killed
• Reject unusable client ids and look up the entity manager lazily #20 and this PR merge without conflicts, in either order

Stack

PRs that add tests to the bundle and fix what the tests and their review found. #12, #13 and #14 are merged. #15 and #20 are independent of each other and both build on master.

1. Move the tests into tests/Unit and add missing unit tests #12 (merged): move the existing tests into tests/Unit and add the unit tests that were missing
2. Add functional tests that run the bundle with a database #13 (merged): functional tests that run the bundle in a Symfony application with a database
3. Fix configuring a metadata class that extends the bundle's Metadata #14 (merged): fix a custom metadata_class that extends the bundle's Metadata, found by the functional tests
4. Reset the cached client between requests and key the cookie cache by request #15: reset the cached client between requests in long-running runtimes, and key the cookie cache by request, found by the functional tests
5. Reject unusable client ids and look up the entity manager lazily #20: reject unusable client ids and look up the entity manager lazily, found in the review of Move the tests into tests/Unit and add missing unit tests #12 and Add functional tests that run the bundle with a database #13

[loevgaard]

The kernel.reset reference is bound to the ids setono_client.client_context.cached / setono_client.cookie_provider.cached (line 44) before decoration is resolved, so if an app decorates either id directly, services_resetter calls reset() on the app's decorator instead of the bundle's cache.

App registers #[AsDecorator('setono_client.client_context.cached')] final class TracingContext implements ClientContextInterface. ResettableServicePass (TYPE_BEFORE_OPTIMIZATION) stores Reference('setono_client.client_context.cached'); DecoratorServicePass then turns that id into an alias to TracingContext. On the 2nd request in FrankenPHP/RoadRunner (or after the 1st message in messenger:consume) services_resetter throws Error: Call to undefined method TracingContext::reset(). If the decorator does implement ResetInterface but doesn't forward, the bundle's CachedClientContext is never reset and visitor A's client is handed to visitor B again. Reproduced with a ContainerBuilder + ResettableServicePass + the bundle's services.xml: decorating .default works, decorating .cached fatals.

[loevgaard]

Symfony does this with every service tagged kernel.reset: the services resetter references the service id, and decorating that id points the reference to the decorator. I compiled the bundle's services with Symfony's decorator and reset passes: a decorator of setono_client.client_context.default works at any priority (0 puts it in front of the cache, 40 between the cache and the cookie based context), and only a decorator of setono_client.client_context.cached fails. The failure is a loud error, not a silent leak, so I documented it instead of working around it (dbd13f9). The README says to decorate the .default service of a chain, and that a decorator of the cached service has to implement ResetInterface and pass reset() on. The cookie provider no longer has the tag (see the comment on CachedCookieProvider).

[loevgaard]

Per-request isolation of the cached client now depends entirely on services_resetter being run, while the same PR made CachedCookieProvider correct by construction by keying it on the Request; the client cache is still not tied to the request it was resolved for.

Kernel::boot() only runs services_resetter once a previous handle() has set resetServices, and only when the app goes through Kernel::handle(). A client resolved before the worker's first request, a loop that drives the http_kernel service directly, messenger:consume --no-reset, or the decorator-by-id case above all still hand one visitor's Client (id + metadata) to the next. Remembering the main Request the client was resolved for (WeakMap or a weak reference, falling back to the memoized client when there is no main request so kernel.terminate listeners keep working) would make kernel.reset a memory optimisation rather than the only guard.

[loevgaard]

I'm keeping kernel.reset here. It's how Symfony resets services that hold per-request state in long-running runtimes, and the Symfony runtimes of FrankenPHP and RoadRunner call Kernel::handle(), which runs the reset. The cookie provider can key its cache by the request, because the request is its argument. getClient() has no request, and is also called when there's none, e.g. by kernel.terminate listeners, after the request stack is empty. Falling back to the last client there would bring back the problem this PR fixes. messenger:consume --no-reset turns the reset off on purpose, and a message handler has no visitor.

[loevgaard]

CLAUDE.md still says CachedCookieProvider 'memoizes per Request via spl_object_hash', but this PR replaced the spl_object_hash array with a WeakMap keyed by the Request object.

The PR edits the bullet two lines above but leaves this one stale; the doc now describes the exact mechanism the PR removed because of handle reuse (the new test comment explains spl_object_hash collisions), so a future contributor or agent following CLAUDE.md may reintroduce it.

[loevgaard]

Done in dbd13f9.

[loevgaard]

CachedCookieProvider now has two invalidation mechanisms for the same problem: the WeakMap already drops an entry when its Request is freed (as the docblock says), so reset() plus the kernel.reset tag adds no correctness.

A retained Request can't collide with a new one, and a freed one is evicted automatically, so reset() only frees memory early. Meanwhile the extra kernel.reset tag on line 44 of services.xml exposes the cookie provider to the decorate-by-id crash described on services.xml:25. Keep the WeakMap and drop reset()/the tag (or keep reset and say so in the docblock); either is simpler than both.

[loevgaard]

Done in b24f58b: CachedCookieProvider no longer has reset(), ResetInterface or the kernel.reset tag, and its docblock says why it needs none.

[loevgaard]

The freed-request regression test relies on the engine reusing the freed object's handle but never asserts that it did, so it can silently stop guarding the bug.

If anything keeps $firstRequest (or one of its sub-objects allocated first) alive, or a different allocation order/engine version hands out a different handle, the new Request gets a different spl_object_id and the test passes even against the old spl_object_hash implementation. Capture spl_object_id($firstRequest) before unset() and assert the new request has the same id, so the precondition the comment describes is checked.

[loevgaard]

Done in b24f58b: before it checks the cookie, the test asserts that the new request gets the spl_object_id() of the freed one. It still fails with the spl_object_hash() cache.

[loevgaard]

The long-running functional tests only exercise the CachedClientContext reset; they cannot catch a regression in CachedCookieProvider's per-request caching.

AbstractBrowser keeps the previous HttpFoundation Request in $this->request until the next one has been built, so consecutive requests never share an object handle; and the cookie provider is reset anyway. Reverting CachedCookieProvider to the spl_object_hash array (with or without reset()) would very likely leave both tests green, so the CLAUDE.md claim that this file covers the long-running behaviour only holds for the client context; the cookie provider relies solely on the unit tests.

[loevgaard]

Right, and a functional test can't easily catch it. I tried one that hands requests to Kernel::handle() like a worker loop and frees each one. But the test kernel keeps every request alive, so no handle is reused, and the old cache passed too. The unit test is the regression test for this, and CLAUDE.md now says so (dbd13f9).