Add configurable large payload storage to the Azure Functions provider - #270
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Reference-token collisions can misinterpret valid JSON strings and cause incorrect hydration or download failures.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
What changed in this PR
Adds configurable large-payload storage for Azure Functions Durable apps.
Changes:
- Adds payload-store configuration across clients, workers, entities, and orchestrations.
- Adds serialization, hydration, documentation, changelog, and E2E coverage.
- Adds Blob Storage-backed large-payload workflows.
Critical review finding: Reference-token handling can collide with valid user JSON strings; an unambiguous envelope or explicit collision prevention is required.
| File | Description |
|---|---|
tests/azure-functions-durable/test_worker_compat.py |
Worker configuration and round-trip tests |
tests/azure-functions-durable/test_converters.py |
Converter payload tests |
tests/azure-functions-durable/test_client_compat.py |
Client integration tests |
tests/azure-functions-durable/e2e/test_dtask_large_payloads_e2e.py |
Large-payload E2E coverage |
tests/azure-functions-durable/e2e/apps/dtask_style/requirements.txt |
E2E storage dependencies |
tests/azure-functions-durable/e2e/apps/dtask_style/large_payloads.py |
E2E payload workflows |
tests/azure-functions-durable/e2e/apps/dtask_style/function_app.py |
E2E app configuration |
noxfile.py |
E2E environment setup |
azure-functions-durable/README.md |
Configuration and retention documentation |
azure-functions-durable/CHANGELOG.md |
Unreleased feature entry |
azure-functions-durable/azure/durable_functions/worker.py |
Invocation-time payload-store resolution |
azure-functions-durable/azure/durable_functions/internal/serialization.py |
Payload hydration during deserialization |
azure-functions-durable/azure/durable_functions/internal/payloads.py |
Payload-store and reference handling |
azure-functions-durable/azure/durable_functions/internal/converters.py |
Payload conversion and externalization |
azure-functions-durable/azure/durable_functions/decorators/durable_app.py |
Public configuration API |
azure-functions-durable/azure/durable_functions/client.py |
Sync and async client integration |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| def _unwrap(self, token: str) -> str: | ||
| if self._store.is_known_token(token): | ||
| return token | ||
| try: | ||
| value = json.loads(token) | ||
| except (ValueError, TypeError): | ||
| return token | ||
| return value if isinstance(value, str) else token |
There was a problem hiding this comment.
The ambiguity is real: JSON-quoting references for the Functions host makes an otherwise ordinary JSON string indistinguishable from a reference. One correction: BlobPayloadStore validates the blob:v1:<container>:<blobName> structure with nonempty components, rather than accepting every string with the prefix.
In a1d8ca0, we have made this an explicit documented restriction of opting into payload externalization, rather than changing the protobuf/host contract or introducing an escaping protocol. Whole payload strings recognized by the configured store are reserved references, including JSON-quoted references, regardless of the size threshold. Download failures propagate rather than falling back to literal data.
Applications that need to carry a reference as literal data can wrap it in an object, e.g. {"reference": "blob:v1:container:blob"}, and retain that wrapper across durable payload boundaries. Added tests verify whole-string reference interpretation, raw/quoted missing-reference failures, and preservation of object-wrapped existing and missing references both inline and when the wrapper itself is externalized. The provider Nox unit suite passes: 283 tests.
A more elaborate marker or hash would not distinguish a deliberately round-tripped reference from a transport reference; arbitrary-data support would require escaping or separate metadata. We propose accepting the documented reserved-string contract for this opt-in feature. Leaving this thread open for reviewer agreement; the collision has not been eliminated.
This comment was written by an agent on behalf of andystaples
Bernd Verst (berndverst)
left a comment
There was a problem hiding this comment.
I recommend addressing the correctness and concurrency findings before merging. The inline comments cover activity I/O blocking the worker event loop, storage failures being classified as application failures, incomplete entity-history hydration, and loss of output-size error details. The override and JSON parsing suggestions are non-blocking.
Additional Python 3.13 guidance:
- Keep newer APIs provider-local:
azure-functions-durablerequires 3.13, but the shareddurabletaskpackage still supports 3.10. - If provider-side hydration batches independent downloads,
asyncio.TaskGroupplus bounded concurrency can provide structured cleanup. It cancels and awaits siblings when a child fails, then propagates failures as an exception group. Python 3.13 improves simultaneous cancellation handling. This belongs around storage I/O, not inside replayed user orchestrators, which must use durable scheduling APIs. asyncio.timeout()can express an overall async transfer deadline if that is part of the intended configuration contract. Coordinate it with Blob SDK retries; do not add an arbitrary hard-coded deadline. Cancelling an await onasyncio.to_thread()does not stop an already-running thread.- I do not see a concrete need for
copy.replace(),typing.ReadOnly, or generic-syntax churn in this diff. Avoid simply cachingget_transport_payload_store(): workers and blueprints may be created before configuration, so cachingNonewould break the supported initialization order.
I am treating the documented reserved whole-string token syntax as an intentional opt-in restriction, rather than repeating the existing collision finding.
| if data is None or data == "": | ||
| return None | ||
| return df_loads(data, expected_type=target_type) | ||
| return df_loads(deexternalize_payload(data), expected_type=target_type) |
There was a problem hiding this comment.
[P1] Separate retryable storage failures from application failures
Hydrating legacy entity results inside deserialize() puts Blob I/O inside _OrchestrationExecutor.execute()'s application-error catch. When a stored entity result raises ConnectionError, the worker returns an OrchestratorResponse with ORCHESTRATION_STATUS_FAILED, although the entity operation succeeded. Recovery of storage cannot resume that terminal orchestration.
Please introduce a host-compatible retry/abandon path for retryable storage failures, distinct from permanent reference errors and user-code exceptions. Moving the read before replay alone is not sufficient: the current Functions host also converts ordinary unclassified invocation exceptions into orchestration failures. This needs to be addressed at the provider/host boundary rather than assuming that raising any exception retries the work item.
There was a problem hiding this comment.
The failure mode is real: moving hydration before replay avoids treating storage I/O as user-code failure, but an ordinary Python invocation exception still does not tell the Functions host to abandon and redeliver the work item.
There isn't a directly comparable .NET Functions implementation of this SDK-managed large-payload feature today. The standalone .NET SDK and backend-managed large-message handling are different execution paths, so neither establishes the recovery behavior we should expect here.
A proper retry/abandon mechanism needs more than a Python retry wrapper: it needs transient/permanent error classification and a host-recognized way to retry the work item without committing a failed orchestration. That requires coordination between the Durable WebJobs SDK/host extension and this Python provider.
The Azure Blob SDK we already use provides bounded retries for some transient failures, for both synchronous and asynchronous operations. Those retries provide some protection, but they do not guarantee recovery; a transient failure that escapes them can still terminate an orchestration. The documentation explicitly warns about that limitation.
For this PR, I propose retaining that documented limitation and waiting for user feedback about transient Blob failures terminating orchestrations before building an additional recovery mechanism across the Durable WebJobs SDK and this provider. This is a conscious deferral, not a claim that error propagation solves recovery. Leaving this thread open for agreement.
This comment was written by an agent on behalf of andystaples
Keep orchestration and entity payload I/O asynchronous while restoring native activity calling conventions. Skip unused historical entity input downloads during replay and retain full client history hydration. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Superseded by the concise review with line-linked, resolvable comments.
andystaples
left a comment
There was a problem hiding this comment.
Changes recommended: two compatibility regressions and two performance issues remain: activity source-directory metadata, direct-call return values, unnecessary replay downloads, and event-loop blocking during Blob compression. Each has an inline thread with reproduction details and a suggested fix.
Python 3.13 typing and reference-trust documentation suggestions are non-blocking. The earlier converter-I/O, entity-history, and swallowed-error findings are addressed; the documented token-collision and storage retry/abandon limitations remain intentional deferrals.
Bernd Verst (berndverst)
left a comment
There was a problem hiding this comment.
Follow-up review of 0021822: I did not identify additional actionable defects in this pass. I am not adding duplicate inline comments for findings that have been addressed.
The revised implementation addresses the earlier findings:
- Binding converters no longer perform storage I/O. Synchronous activities retain host-thread execution and synchronous clients; asynchronous activities use async storage. The orchestration/entity entrypoints await payload transfers outside their execution threads.
- Output externalization is outside the core worker's exception-catching completion path, preserving the original storage/size-limit errors rather than replacing them with a missing-response error.
- Both client history paths hydrate correlated entity envelopes, while replay avoids unused historical entity inputs and scheduled activity inputs. Application object wrappers remain literal.
- Direct activity calls retain ordinary Python return values without storage access. Activity/client wrapper source filenames, worker-visible signatures, forward-reference annotations, invocation context, and concurrent request isolation are preserved.
- Async Blob compression/decompression is offloaded separately from native async HTTP. The provider-local
typing.overrideusage and frozen/slotted transport marker are appropriate; the shared core change stays compatible with Python 3.10. I do not see a reason to add further Python 3.13 syntax or concurrency abstractions to this PR merely for modernization.
One existing design concern remains open: transient payload-storage failures can still cause terminal orchestration failure. The README now states that limitation accurately, and the author has explicitly deferred host-level retry/abandon support. That is a documented deferral, not a recovery fix. I am leaving it in its existing thread for an explicit maintainer decision rather than reopening it as a new finding or marking it resolved.
This is a follow-up comment review, not approval of that deferred recovery behavior.

Summary
DFApp.configure_large_payloads(payload_store=...), shared across synchronous and asynchronous clients, workers, activities, entities, and imported blueprints.Validation
E2E used Azure Storage with Azurite; DTS-specific behavior and distributed tracing were not exercised. Existing lint errors in local generated build copies were left untouched.