Skip to content

fix: prevent GraphQLCollector from accumulating batches across requests - #1250

Open
w0lan wants to merge 1 commit into
overblog:masterfrom
w0lan:fix/graphql-collector-reset-batches
Open

fix: prevent GraphQLCollector from accumulating batches across requests#1250
w0lan wants to merge 1 commit into
overblog:masterfrom
w0lan:fix/graphql-collector-reset-batches

Conversation

@w0lan

@w0lan w0lan commented Aug 21, 2026

Copy link
Copy Markdown
Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes (pre-existing failures on master unchanged — see Testing)
Documented? no
Fixed tickets -
License MIT

Summary

GraphQLCollector appends one entry per executed GraphQL operation to $batches, and each
entry holds a VarDumper clone of the full response. Two independent things keep that array
from being cleared between requests in a long-running process:

  1. reset() clears $data but leaves $batches untouched;
  2. the collector carries no kernel.reset tag, so in an application where the collector is
    registered while Symfony's profiler service is not, reset() is never called at all —
    the profiler service is the only thing that forwards reset() to data collectors, and it
    only exists when framework.profiler is enabled.

This PR fixes both: reset() now clears $batches, and profiler.yaml tags the collector
kernel.reset so that ServicesResetter reaches it directly, exactly like Executor and
TypeResolver already do since #1203. (Unlike the schema state involved in #1242, $batches
is purely per-request runtime state, so clearing it cannot lose anything — details below.)

Where this actually bites

(Symfony line references below are from 7.3.)

Under php-fpm the request teardown frees the whole userland heap after every request, so the
array never survives — regardless of process reuse. (One nuance: FrameworkBundle's
HttpCache calls Kernel::boot() per forwarded ESI fragment, HttpCache.php:65, which
triggers the resetter mid-response.) Under a
long-running runtime (RoadRunner, Swoole, FrankenPHP worker mode) the process serves thousands
of requests, Kernel::boot() runs the resetter between them (Kernel.php:106-109), and every
request leaves another response clone behind. Concretely:

  • framework.profiler.enabled: true + collect: false (on-demand profiling, common on
    staging/production), or only_exceptions: true, or a RequestMatcher: Profiler::collect()
    early-returns (Profiler.php:132-134), so GraphQLCollector::collect() never runs and
    $data stays empty — but onPostExecutor() keeps firing, because it is an ordinary event
    listener. Profiler::reset() is called here, and today it clears nothing. Fixed by change 1.
  • A dev/debug worker (symfony/runtime worker mode with the profiler on): same thing,
    bounded only by how long the worker lives.
  • The collector registered without Symfony's profileroverblog_graphql.profiler.enabled: true
    while framework.profiler is off, and, before fix: make profiler configurable and disable it by default in non-debug mode #1243, any non-debug application, since
    profiler.yaml was loaded unconditionally. Here there is no profiler service, so nothing
    calls reset() on the collector at all, and change 1 alone would be a no-op. Fixed by change 2.

The change

     public function reset(): void
     {
         $this->data = [];
+        $this->batches = [];
     }
             - { name: kernel.event_listener, event: graphql.post_executor, method: onPostExecutor }
+            - { name: kernel.reset, method: reset }

services_resetter is defined unconditionally by FrameworkBundle
(Resources/config/services.php:182) and ResettableServicePass wires every kernel.reset
service into it, using an IGNORE_ON_UNINITIALIZED_REFERENCE reference — so a collector that
was never instantiated during the request is skipped, and one that was gets cleared. When the
Symfony profiler is enabled, reset() simply runs twice (once through Profiler::reset(),
once directly); it is idempotent.

I deliberately did not touch collect() or onPostExecutor(). Capping the number of retained
batches, or storing the response size instead of a full cloneVar($result), would also bound
the worst case within a single request, but that changes what the profiler panel shows and
belongs in a separate discussion.

Why this cannot blind the profiler panel

Two mechanisms, neither of which depends on event ordering:

  • Profiler::collect() stores clone $collector in the Profile (Profiler.php:157-161);
    GraphQLCollector has no __clone(), so the arrays are copied by value and a later reset()
    on the live service cannot reach them.
  • the base DataCollector serializes only $data, so the persisted profile never contained
    $batches in the first place. The panel reads what collect() copied into
    $data['batches'].

Batched requests are also safe: GraphController::processBatchQuery() executes the whole batch
inside one controller call, so all operations of one HTTP request land in $batches before
kernel.response.

Consistency with the rest of the ecosystem

Every Symfony collector that accumulates working state during a request clears it in reset()
SerializerDataCollector is the closest structural twin (it also fills a private array from
outside collect(); SerializerDataCollector.php:40-45), and RequestDataCollector,
RouterDataCollector, DumpDataCollector, DoctrineDataCollector and others do the same for
their own buffers. GraphQLCollector is the outlier; this brings it in line.

This is not a repeat of #1242

Executor::$schemas was populated once at container compile time, which is why resetting it
lost data permanently. $batches is filled purely at runtime, per request, by the
graphql.post_executor listener — there is no compile-time state to lose.

Relation to #1243

#1243 already identified this accumulation ("All this data was accumulated into
$this->batches and never consumed") and fixed the exposure by not registering the collector
outside debug mode. That was the right call and it covers the common production setup. This PR
fixes the accumulator itself, for every configuration where the collector is registered on
purpose.

How big the accumulator is

Measured on a Symfony 7 GraphQL API running v1.7.0 with the collector registered and never
reset (case 3 above): heap dumps under RoadRunner after 901 requests showed $batches holding
70.18 MB out of 77.12 MB (91%) of all traced worker retention; on FrankenPHP worker mode a
search query with a ~66.8 KB response grew the worker heap by ~199 KB per request with no
plateau, and VarDumper\Cloner\Data grew by exactly two objects per executed operation,
with nothing freed during the run.

Those numbers quantify the accumulator, not this patch. The reset() one-liner alone measured
no change in that setup (without the tag, nothing called reset()), and our own production
problem was solved by not registering the collector — the same approach as #1243. With the
complete patch, in the same setup with the collector registered, worker heap growth went from
205.6 KB/req to flat after warm-up, and a heap dump after 301 requests showed $batches
holding a single entry instead of 301.

Testing

  • GraphQLCollectorTest::testResetClearsBatches() — unit level: collect a batch, reset(),
    and verify through the public API that a following collect() reports no leftovers and that
    a batch collected afterwards is the only one reported.
  • Functional\DataCollector\GraphQLCollectorResetTest — wiring level: boot the test
    application (which has framework.profiler.enabled: false, so the collector is registered
    without Symfony's profiler), perform a GraphQL request, then call services_resetter->reset()
    the way Kernel::boot() does between requests, and assert the collector reports nothing.

Both fail on master. The functional one also fails with only the reset() change applied, so
it covers the tag as well.

Verified on PHP 8.1 (Symfony 6.4 resolution) and PHP 8.4/8.5 (Symfony 8.1 resolution).
Symfony 5.4 and 7.x were not run here; everything the functional test relies on is identical
in those branches (ResettableServicePass, the unconditional public services_resetter,
test.service_container). Full suite: 714 tests (712 on master plus the two added here); the
5 failures and 7 errors reproduce identically on an unmodified master checkout (a newer
webonyx/graphql-php resolving without a committed lock file, plus a PHP 8.5 deprecation from
a transitive dependency) — the patch adds two tests and no new failures. check-cs is clean;
static-analysis adds no new findings beyond more instances of a pre-existing symbol-discovery
artifact this environment already reports for the existing tests.

Thanks for the bundle, and for the recent work on worker-mode support.

GraphQLCollector appends one entry per executed GraphQL operation to
$batches, each holding a VarDumper clone of the full response, and
nothing cleared it between requests: reset() cleared only $data, and
the collector carried no kernel.reset tag, so where it is registered
while Symfony's profiler service is not, reset() was never called at
all.

reset() now clears $batches as well, and profiler.yaml tags the
collector kernel.reset so ServicesResetter reaches it directly, the
same way Executor and TypeResolver already do (overblog#1203). Unlike the
compile-time schema state involved in overblog#1242, $batches is per-request
runtime state, so clearing it loses nothing. The base DataCollector
serializes only $data into the persisted profile, so the profiler
panel is unaffected.
@w0lan

w0lan commented Aug 21, 2026

Copy link
Copy Markdown
Author

The red jobs are the pre-existing master failures, not this patch: nightly master CI has been red since Aug 19 (5 × GraphDumpSchemaCommandTest with the newer webonyx/graphql-php, plus the known CustomScalarType phpstan covariance error). See the Testing section — the two tests added here pass in every matrix entry, and all lowest-deps jobs are green.

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.

1 participant