refactor(etl-uvicorn): bind reserved /invoke fields via a route dependency; no sealed-settings opt-out - #75
Conversation
…endency, drop the sealed-settings opt-in The body-replay middleware is replaced by bind_invocation_envelope, a FastAPI dependency that reads the framework's Starlette-cached body parse, so the /invoke body is buffered and decoded exactly once — no receive replay, no second copy of a large body. The request-size cap stays below the framework as InvokeBodyLimitMiddleware, a streaming byte counter that never buffers. /metadata now advertises invoke_with_sealed_dag_node_settings unconditionally: sealed per-invoke settings are the platform's required settings path, so the wrap_in_fastapi/generate_fast_api parameter and --sealed-dag-node-settings CLI flag are removed.
136431f to
30b0112
Compare
There was a problem hiding this comment.
2 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="unstructured_platform_plugins/invocation_settings.py">
<violation number="1" location="unstructured_platform_plugins/invocation_settings.py:247">
P1: Streaming `/invoke` plugins lose the reserved invocation bindings on the repository's locked FastAPI 0.117.1. This `yield` dependency resets `_INVOCATION` when the path operation returns, but `wrap_in_fastapi` only starts iterating an async-generator plugin after that point while sending its `StreamingResponse`; the plugin therefore sees `current_invocation_settings()` and `current_invocation_context()` as `None`. Keeping the context around the stream (and adding a streaming binding test), or requiring FastAPI >= 0.118.0 and updating the lock, would preserve sealed settings and request identity for streaming plugins.</violation>
<violation number="2" location="unstructured_platform_plugins/invocation_settings.py:357">
P2: A mixed-method `/invoke` route also receives the envelope dependency on its non-POST methods, so `GET /invoke` can unexpectedly resolve settings and fail with a 500 instead of reaching its handler. Restrict installation to POST-only routes or reject mixed-method `/invoke` routes before mutating the shared FastAPI `dependant`.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
| ) from exc | ||
|
|
||
| with invocation_envelope(invocation_settings, invocation_context): | ||
| yield |
There was a problem hiding this comment.
P1: Streaming /invoke plugins lose the reserved invocation bindings on the repository's locked FastAPI 0.117.1. This yield dependency resets _INVOCATION when the path operation returns, but wrap_in_fastapi only starts iterating an async-generator plugin after that point while sending its StreamingResponse; the plugin therefore sees current_invocation_settings() and current_invocation_context() as None. Keeping the context around the stream (and adding a streaming binding test), or requiring FastAPI >= 0.118.0 and updating the lock, would preserve sealed settings and request identity for streaming plugins.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At unstructured_platform_plugins/invocation_settings.py, line 248:
<comment>Streaming `/invoke` plugins lose the reserved invocation bindings on the repository's locked FastAPI 0.117.1. This `yield` dependency resets `_INVOCATION` when the path operation returns, but `wrap_in_fastapi` only starts iterating an async-generator plugin after that point while sending its `StreamingResponse`; the plugin therefore sees `current_invocation_settings()` and `current_invocation_context()` as `None`. Keeping the context around the stream (and adding a streaming binding test), or requiring FastAPI >= 0.118.0 and updating the lock, would preserve sealed settings and request identity for streaming plugins.</comment>
<file context>
@@ -147,6 +156,98 @@ async def plugin_metadata() -> dict:
+ ) from exc
+
+ with invocation_envelope(invocation_settings, invocation_context):
+ yield
+
+
</file context>
| app.state.invocation_envelope_installed = True | ||
| app.add_middleware(InvocationEnvelopeMiddleware) | ||
| for route in routes: | ||
| route.dependant.dependencies.insert( |
There was a problem hiding this comment.
P2: A mixed-method /invoke route also receives the envelope dependency on its non-POST methods, so GET /invoke can unexpectedly resolve settings and fail with a 500 instead of reaching its handler. Restrict installation to POST-only routes or reject mixed-method /invoke routes before mutating the shared FastAPI dependant.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At unstructured_platform_plugins/invocation_settings.py, line 358:
<comment>A mixed-method `/invoke` route also receives the envelope dependency on its non-POST methods, so `GET /invoke` can unexpectedly resolve settings and fail with a 500 instead of reaching its handler. Restrict installation to POST-only routes or reject mixed-method `/invoke` routes before mutating the shared FastAPI `dependant`.</comment>
<file context>
@@ -192,103 +286,83 @@ async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None:
app.state.invocation_envelope_installed = True
- app.add_middleware(InvocationEnvelopeMiddleware)
+ for route in routes:
+ route.dependant.dependencies.insert(
+ 0,
+ get_parameterless_sub_dependant(
</file context>
Summary
Stacked on #74 — the alternative shape for the transport, for side-by-side comparison. Two changes:
1. Extraction moves from body-replay ASGI middleware to a route dependency.
InvocationEnvelopeMiddlewarebuffered the raw/invokebody,json.loads-ed it itself, and replayed the buffered messages so the framework could parse the same bytes again. All of that existed only because ASGI middleware runs before FastAPI does. The replacement,bind_invocation_envelope, runs as a dependency on the/invokeroute, where the framework has already buffered and parsed the body —request.json()is Starlette-cached and shared with FastAPI's own body handling. Net effect:del-based memory managementdetail+ top-levelreason) via a registered exception handler, and the same blame-derived statusesThe one concern that genuinely must sit below the framework — the request-size cap, since neither Starlette nor uvicorn bounds body size — stays as
InvokeBodyLimitMiddleware: a streaming byte counter that buffers nothing and answers 413 over the cap.install_invocation_envelope(app)keeps its name and call sites (the wrapper's, and a hand-rolled app's) but now attaches the dependency to the registered POST/invokeroute(s) — the same insertioninclude_routerperforms for router-level dependencies — plus the cap middleware and the error handler. It must be called after the/invokeroute is registered and raises if none exists, so a mis-ordered install fails loudly instead of leaving the app silently uncovered.2. No opt-out: the sealed capability is always advertised.
Per-invoke sealed settings are the platform's required settings path, so
/metadataadvertisesinvocation_settings,invocation_context, andinvoke_with_sealed_dag_node_settingsunconditionally. Theinvoke_with_sealed_dag_node_settingsparameter onwrap_in_fastapi/generate_fast_apiand the--sealed-dag-node-settingsCLI flag are removed. Note what this changes downstream: the controller will treat every plugin on this version as a sealed-settings consumer, so bumping the wrapper and consumingcurrent_invocation_settings()must land together per plugin — a plugin that bumps without consuming would decrypt and ignore, running on boot-time state.Behavior deltas vs #74 (deliberate)
json_invalid422 answers before the dependency runs (the middleware answered 500 underFF_REQUIRE_…). Every other FF-required edge — absent field, plaintext, bodyless invoke, non-object JSON — still fails closed with the same errors, pinned by tests./metadata: sealed capability always present;add_metadata_routeloses the flag parameter.Verification
utic-invocation-settings0.4.0 build (the one failure,test/test_schema.py::test_file_data, pre-exists on the base branch — environment drift, unrelated).test_invocation_middleware.py→test_invocation_envelope.py: every scenario ported (sealed / composite / plaintext / absent binding, non-dict rejection, context version gate, secret never in logs or response, FF-required matrix, cross-request leak check) plus new coverage: install-before-route raises, ASGI-level cap tests (chunked over-cap → 413 + downstream sees disconnect, client disconnect passes through, non-invoke uncapped).ruff checkclean;ruff formatapplied to the files this PR owns (the pinned formatter can't resolve until 0.4.0 publishes).