fix(sites): stream chunked responses - #13314
Open
HarshMN2345 wants to merge 12 commits into
Open
Conversation
Contributor
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.
HarshMN2345
force-pushed
the
fix/sites-chunked-responses
branch
from
August 23, 2026 17:13
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stream Sites responses through the proxy instead of buffering them, so a progressively rendered page starts reaching the visitor before the runtime has finished producing it.
chunk()does not measureRequires open-runtimes/executor#250
This PR does nothing on its own. It requests response format
0.12.0and only streams if the executor echoes that format back with length-prefixed parts. No released executor does. Without open-runtimes/executor#250 the gate never opens and every request silently takes the buffered path.Merge order: executor#250 first, then this. Merging this alone ships an inert code path plus the compression tradeoff below, with no benefit.
Design
The executor emits parts in the order
statusCode, headers, body, logs, errors, duration, startTime. Status and headers are known from the runtime's response headers before its first body byte, so they arrive first and the proxy can commit them before any content goes out;logs/errors/durationtrail the body and are kept for the execution log rather than forwarded.Part content is length prefixed, which is what makes the incremental read tractable: content is never scanned for the boundary, so a body carrying the boundary string cannot split the envelope and no lookahead is retained between socket reads.
Streaming commits on the first body byte. Until then nothing is written, so previews, static-adapter 404s, and empty-bodied error responses can still fall back to the buffered path and get their branded output.
Scope
Sites only. Functions are buffered inside every runtime, so this does not make them stream —
onPartis not installed for them.Supersedes
#11429 and open-runtimes/executor#223, both closed unmerged ("Towards SER-334"). Those forwarded the raw body on
text/event-streamwith no multipart envelope, which discardedlogs,errorsandduration. Keeping the envelope preserves execution metadata.Open questions before merge
Response::send()applies gzip/brotli/zstd fromaccept-encoding;chunk()does not. Streaming therefore ships uncompressed HTML for all non-preview Sites traffic — roughly 5-10x more bytes on the wire, traded for a lower time-to-first-byte. That trade is probably wrong for many sites, which argues for putting this behind a per-project setting rather than defaulting it on.responseBody?Tests
tests/unit/Utopia/Fetch/BodyMultipartStreamTest.php— incremental parsing, including a golden fixture of the exact bytes executor#250 emits, so a wire change on either side of the contract fails heretests/unit/Utopia/ResponseTest.php—chunk()byte accountingVerified against a real socket with real curl driving
Executor::createExecution: status and headers at +2ms, body runs at +255/506/756/1011ms against a peer dribbling output, confirming the body is forwarded progressively rather than reassembled.🤖 Generated with Claude Code