Repository navigation
fix(sites): stream chunked responses - #13314
Conversation
Greptile SummaryThe PR adds opt-in streaming for non-preview Sites responses while preserving buffered handling for unsupported executors and responses requiring branded output.
Confidence Score: 4/5The PR does not yet appear safe to merge because a malformed or truncated trailing executor envelope can be discovered only after the visitor response has already been ended successfully. The body part's completion flag is still forwarded as the HTTP response-ending flag, while full multipart-envelope validation occurs afterward; the subsequent socket close therefore cannot reliably turn that already-completed response into an observable truncation. Files Needing Attention: app/controllers/general.php Important Files Changed
Reviews (13): Last reviewed commit: "test: cover an envelope abandoned mid ru..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
✨ Benchmark resultsComparing
Per-scenario breakdown & investigation detailsMetrics below reflect the current branch (after). Δ P95 compares against the base.
Top API waits (after)
|
A callback suppresses decoding in call(), so createExecution reads the buffered body itself. It assumed multipart, but the executor answers its own failures with JSON: deriving a boundary from "application/json" yields an empty string, the parse returns nothing, and the ExecutorException ends up with an empty message and an unknown type. This affected every non-preview site request, since that is where the callback is installed, and it applied whether or not streaming engaged. Switch on the response's own content type instead, and fail loudly if the JSON will not parse rather than reporting an empty error. Also pin the executor's streamed envelope as a fixture, so a wire change on either side of the contract fails here, and cover part ordering, trailing data after the closing delimiter, and the malformed-input paths.
Replace the anonymous state class and its four booleans with a tri-state local: null while undecided, false once the response is buffered, true once content is on the wire. Streaming is decided on the first body run, where the status code and headers are guaranteed to have arrived. Any status at or above 400 now takes the buffered path, which subsumes the separate static-404 case and the deferred empty-body check, and keeps branded pages working. Terminating the response no longer needs its own flag, since chunk() is a no-op once the response has been sent. Collapse the executor's redundant streaming flag into the reader it tracked, and trim comments to the density of the surrounding files.
1e43064 to
e1b739f
Compare
A stream that stopped before the closing delimiter was reported as a complete execution, and the proxy wrote the terminating chunk regardless, so a visitor received a partial page under a successful status with nothing to indicate the content was cut short. Fail the execution when the envelope did not finish, and drop the connection instead of terminating it cleanly, since content is already on the wire by then and a broken transfer is the only remaining way to signal truncation. Validate the CRLF after a content run and after a part terminator as well: both were skipped over unread, so a malformed envelope could still parse.
The close on truncation sat at the end of the handler, which an executor timeout never reached: it is rethrown as an AppwriteException from the catch, so the error hook appended the branded page to a visitor's partial body and terminated it as a complete response. Close where the failure is caught instead, and mark the response sent so no later writer touches it. Decode the headers part before committing to streaming, and fail the request when it cannot be decoded rather than serving the page with none of its headers. Guard the buffered path's decode the same way, since it stored null. Drop the guard around the branded overrides: each already requires a status that the streamed path cannot have, so it only indented them.
Matches Schedule\Source\Functions, which holds a promoted readonly \Closure with a typed docblock. Every caller already passes a closure, and mixed only existed because a property cannot be declared callable.
The terminating chunk in the streamed branch could never run: the body part's final callback ends the response inside the part handler, some 250 lines earlier, and every other route there marks the response sent as well. The part buffer kept every trailing part, though only the status code and headers are ever read from it and only before the first body byte. Logs and errors are capped at 5MB each and are already held by the executor client that the rest of the handler reads them from, so a streamed request was holding them twice. Keep only what is still readable. An empty boundary could not occur: the executor sets the content type and the format echo in one statement, and the branch could not have recovered anyway. Emitting part of a run before the rest arrives is the reader's whole purpose and nothing covered it: turning it into a run-buffering reader left the suite green. Pin the malformed size message too, since deleting that guard tripped a later one and passed. Both mutants now fail. The executor writes seven parts, so the fixture carries seven.
Streaming had no switch: once an executor advertised the format, every non-preview site request streamed, and the only way back was to roll the executor image. Default it off. A streamed response also skips compression, since that lives in send() and a chunked body has no length to compress against, so an operator should be choosing the trade rather than inheriting it on upgrade.
The existing truncation test stops after a whole part. A runtime dying mid body leaves a different state: content already forwarded, the run never finished. Letting isComplete() tolerate that state now fails.
🔵 Tier A · Mergeable after minor fixes
This PR adds gated incremental streaming for non-preview Sites requests by negotiating the executor’s multipart format, parsing response parts as they arrive, and forwarding body chunks while retaining buffered fallbacks. It also adds streamed-byte accounting and configuration plumbing, with tests for multipart parsing, byte accounting, response schema serialization, and content-disposition filenames. Latest changes: The newest commits extend ResponseTest with collection/table schema-type serialization cases and content-disposition filename edge cases.
Fix with agent prompt### Issue 1
app/controllers/general.php:667-668
**Cover the enabled streaming path through the proxy**
The new tests exercise the parser and byte counter but never run the executor-to-proxy integration, so they would still pass if Sites responses were buffered or lost their status/headers. Please add HTTP-level coverage with streaming enabled for progressive delivery and a buffered error/preview fallback; AGENTS.md requires e2e coverage for HTTP-visible behavior, and existing Sites/Proxy suites provide the comparable test surface.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.📂 Walkthrough · 9
⏳ Still open from earlier reviews · 1
Reviewed the commits since |
![hansi-codes[bot]](https://avatars.githubusercontent.com/in/5046327?s=60&v=4){kind=small}
| responseFormat: $canStreamResponse ? Executor::RESPONSE_FORMAT_STREAM : Executor::RESPONSE_FORMAT_ARRAY_HEADERS, | ||
| onPart: $onExecutionPart |
There was a problem hiding this comment.
Cover the enabled streaming path through the proxy
The new tests exercise the parser and byte counter but never run the executor-to-proxy integration, so they would still pass if Sites responses were buffered or lost their status/headers. Please add HTTP-level coverage with streaming enabled for progressive delivery and a buffered error/preview fallback; AGENTS.md requires e2e coverage for HTTP-visible behavior, and existing Sites/Proxy suites provide the comparable test surface.
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/controllers/general.php
Line: 667-668
Comment:
**Cover the enabled streaming path through the proxy**
The new tests exercise the parser and byte counter but never run the executor-to-proxy integration, so they would still pass if Sites responses were buffered or lost their status/headers. Please add HTTP-level coverage with streaming enabled for progressive delivery and a buffered error/preview fallback; AGENTS.md requires e2e coverage for HTTP-visible behavior, and existing Sites/Proxy suites provide the comparable test surface.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.🟡 Minor · testing · Reply if this doesn't apply.
…ponses # Conflicts: # tests/unit/Utopia/ResponseTest.php
Security rulesNo new WARNING or ERROR findings from security rules. 6 existing findings tracked in
|
…n the compose snapshot
🟢 Tier S · Ready to merge
Adds opt-in streaming for eligible Sites responses using the executor’s multipart response format, forwarding body chunks as they arrive while retaining buffered handling for previews and error pages. It adds the streaming configuration and executor version updates, an incremental multipart parser, and unit and end-to-end coverage for progressive delivery. The latest test adjustment uses Latest changes: The newest commits rename the Astro test page’s response-status query parameter from
📂 Walkthrough · 10
Reviewed the commits since |
commit: |
![appwrite[bot]](https://avatars.githubusercontent.com/in/325556?s=60&v=4){kind=small}
Streams Sites responses through the proxy instead of buffering them, so a progressively rendered page or an SSE endpoint reaches the visitor as the runtime produces it.
_APP_COMPUTE_STREAMING(defaultdisabled). When enabled, the executor client requests response format0.12.0for non-preview site requests and reads the length-prefixed multipart envelope as it arrives, forwarding body chunks to the visitor.openruntimes/executorto1.3.0(feat: add streamed response format 0.12.0 open-runtimes/executor#250), the first release that emits0.12.0, indocker-compose.ymland the Console compose snapshot.Response::abort()instead of appending an error page, and the execution is still recorded with its logs.chunk(), so bandwidth metering is unchanged.Tests:
testSiteStreaming(Sites e2e): an Astro page rendering three parts two seconds apart arrives part by part through the proxy..envenables the flag for CI.BodyMultipartStreamTestcovers incremental parsing, the executor wire format, malformed separators and incomplete envelopes.