Skip to content

[Platform] Make DeferredResult and RawHttpResult safe to dump()/dd() - #2501

Open
alireza-aminzadeh wants to merge 2 commits into
symfony:mainfrom
alireza-aminzadeh:fix/deferred-result-safe-dump
Open

alireza-aminzadeh wants to merge 2 commits into
symfony:mainfrom
alireza-aminzadeh:fix/deferred-result-safe-dump

Conversation

@alireza-aminzadeh

@alireza-aminzadeh alireza-aminzadeh commented Sep 6, 2026

Copy link
Copy Markdown
Q A
Bug fix? yes
New feature? no
Docs? no
Issues Contributes to #2280
License MIT

Every HTTP-based ModelClient wraps its injected client with
EventSourceHttpClient (needed to decode SSE streaming responses), so the
response held by RawHttpResult is always a Symfony HttpClient
AsyncResponse, itself wrapping a TraceableResponse whenever HTTP client
profiling is enabled, which is the default in a full-stack app's dev
environment. Neither class has a dedicated VarDumper caster, and both keep
their network stream alive until it is explicitly consumed.

dump()'ing or dd()'ing a DeferredResult right after
PlatformInterface::invoke(), before the result is ever read, therefore
reflects into that live, not-yet-consumed response instead of a safe
snapshot: closures bound to the response, its HTTP client and its internal
chunk-accounting state all get expanded, and poking at that state from
outside the client's own bookkeeping is exactly what the
AsyncResponse/TraceableResponse pair is not designed to tolerate.

This PR adds __debugInfo() to RawHttpResult so it never exposes the live
response in a dump, replacing it with a short, static summary of its type
instead of letting the dumper walk into its internals. It also adds
__debugInfo() to DeferredResult so a dump surfaces the conversion state
(pending, converted or failed) at a glance, without ever calling
getResult() itself, so inspecting a pending result cannot trigger a
conversion as a side effect.

Tests assert that __debugInfo() never calls any method on the wrapped
response, that the httpStream property stays visible, and that
DeferredResult's reported state matches pending/converted/failed without
triggering a conversion.

No CHANGELOG.md/UPGRADE.md changes, since this is a bug fix only.

@carsonbot

Copy link
Copy Markdown
Collaborator

Hey!

To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done?

Note that a pull request does not have to be "perfect" or "ready for merge" when you first open it. We just want it to be ready for a first review.

Cheers!

Carsonbot

@alireza-aminzadeh
alireza-aminzadeh marked this pull request as ready for review September 6, 2026 15:08
@carsonbot carsonbot added Bug Something isn't working Platform Issues & PRs about the AI Platform component Status: Needs Review labels Sep 6, 2026

@chr-hertel chr-hertel left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice idea, thanks @alireza-aminzadeh! Haven't tested it yet, but I think for the DeferredResult it would be nice to keep more of the instance's state, e.g. error message, metadata, options

Every HTTP-based ModelClient wraps its injected client with
EventSourceHttpClient (needed to decode SSE streaming responses), so the
response held by RawHttpResult is always a Symfony HttpClient
AsyncResponse, itself wrapping a TraceableResponse whenever HTTP client
profiling is enabled, which is the default in a full-stack app's dev
environment. Neither class has a dedicated VarDumper caster, and both
keep their network stream alive until it is explicitly consumed.

dump()'ing or dd()'ing a DeferredResult right after
PlatformInterface::invoke(), before the result is ever read, therefore
reflects into that live, not-yet-consumed response instead of a safe
snapshot: closures bound to the response, its HTTP client and its
internal chunk-accounting state all get expanded, and poking at that
state from outside the client's own bookkeeping is exactly what the
AsyncResponse/TraceableResponse pair is not designed to tolerate.

Add __debugInfo() to RawHttpResult so it never exposes the live response
in a dump, replacing it with a short, static summary of its type instead
of letting the dumper walk into its internals. Add __debugInfo() to
DeferredResult so a dump also surfaces the conversion state ("pending",
"converted" or "failed") at a glance, without ever calling getResult()
itself, so inspecting a pending result cannot trigger a conversion as a
side effect.

Fix symfony#2280
…dResult::__debugInfo()

733d81e added __debugInfo() to DeferredResult, reporting only the
conversion state ("pending", "converted" or "failed"). Per review, that
is too little to be useful at a glance: knowing a result failed without
seeing why still sends a developer back to calling getResult() (and
therefore catching the exception) just to read the message.

Extend __debugInfo() with three more entries, none of which touch
$rawResult any more than $state already does:

 * options - the array DeferredResult was constructed with, e.g. the
   model options a call site passed to invoke(). It is a plain,
   readonly, always-initialized constructor argument.
 * metadata - $this->getMetadata()->all(), which starts out empty and,
   once conversion succeeds, only ever holds what a ResultConverter or a
   stream listener explicitly added to it (e.g. token usage), never the
   raw response itself.
 * error - $conversionFailure?->getMessage(), null unless the state is
   "failed". Reading the message does not walk the exception's
   (potentially deep) stack trace.

Widen the @return docblock to an array shape accordingly, and add one
test per new entry, covering the pending, converted and failed states.
@alireza-aminzadeh

Copy link
Copy Markdown
Author

Thanks for the feedback! Pushed a commit that extends DeferredResult::__debugInfo() with the three pieces of state you mentioned, none of which touch $rawResult any more than the existing state entry does:

  • options — the array the DeferredResult was constructed with (a plain, readonly, always-initialized constructor argument, e.g. the model options a call site passed to invoke()).
  • metadata$this->getMetadata()->all(); empty until conversion succeeds, and even then only ever holds what a ResultConverter or a stream listener explicitly added to it (e.g. token_usage), never the raw response itself.
  • error$conversionFailure?->getMessage(), null unless the state is failed; reading the message doesn't walk the exception's stack trace.

Also widened the @return docblock to an explicit array shape and added one test per new entry (testDebugInfoIncludesOptionsWithoutConverting, testDebugInfoReportsEmptyMetadataAndNullErrorWhilePending, testDebugInfoIncludesMetadataAfterGetResult, testDebugInfoIncludesErrorMessageAfterConversionFailure), covering the pending/converted/failed states.

Rebased on the latest main while at it. The full platform suite (908 tests) and PHPStan pass locally. Ready for another look whenever you have time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Something isn't working Platform Issues & PRs about the AI Platform component Status: Needs Review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants