fix(v3): make asset server flush semantics honest - #5931
Merged
Conversation
Three related problems in the asset server, all found while investigating Go->JS event dispatch and all independent of it. contentTypeSniffer.Flush delegated to the wrapped writer without first completing itself. Until 512 bytes have been seen the sniffer deliberately holds the body back so it can detect a Content-Type, so a flush before that point sent nothing and reported success. Any response that writes a short chunk and then waits - every streaming format - silently delivered nothing. Flush now completes first, sniffing from the short prefix, which is the right reading of an explicit flush: send what has been written so far. The platform response writers did not implement http.Flusher at all, so http.ResponseController(w).Flush() could never reach them. On macOS, iOS and Linux that was only a missing declaration: Write already pushes each chunk straight to the URL scheme task or into the pipe WebKitGTK reads, so a flush has no work to do. Those now declare a documented no-op. Windows deliberately does not, and this is the interesting one. Its Write accumulates into an in-memory bytes.Buffer that is handed to WebView2 only in Finish, so nothing can reach the page mid-response. A no-op Flush there would claim a capability that does not exist. Left unimplemented, ResponseController.Flush keeps returning ErrNotSupported, which is true. It also means streaming cannot work on Windows today regardless of what WebView2 supports - worth knowing before designing around it. dispatchWorkers is declared and read but never assigned, which reads like an oversight and is not: it must stay 0. At 0 each request gets its own goroutine. Any positive value switches to a fixed worker pool, where a long-lived request holds its worker for its whole lifetime and enough of them starve the pool - later requests, including the page's own assets, queue behind them and the app appears to hang at startup with no error anywhere. Documented rather than removed, since the pooled path is still useful for workloads known to be short-lived. Tests cover the sniffer: a short prefix is released on flush, later writes pass through, flushing before any write is a no-op, an explicit Content-Type is untouched, the full-prefix path still completes on its own, and http.ResponseController finds the flusher. Three of them fail without the fix.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe asset server now preserves content-type sniffer errors and unwritten prefixes during flushing. Supported webview writers expose ChangesAsset server response flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant HTTPHandler
participant contentTypeSniffer
participant WrappedWriter
HTTPHandler->>contentTypeSniffer: Flush()
contentTypeSniffer->>contentTypeSniffer: Complete content-type sniffing
contentTypeSniffer->>WrappedWriter: Write buffered prefix
contentTypeSniffer->>WrappedWriter: Flush()
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@v3/internal/assetserver/content_type_sniffer.go`:
- Around line 149-155: Update contentTypeSniffer.Flush and the
contentTypeSniffer state to retain any error returned by complete instead of
discarding it, and have subsequent Write calls and completion attempts return
the stored failure before proceeding. Preserve normal flushing behavior while
adding a writer test whose prefix write fails only during Flush and verifies the
error is later surfaced.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c1a20fa7-e68a-4268-bf0a-b6470f4a8827
📒 Files selected for processing (8)
v3/internal/assetserver/assetserver_webview.gov3/internal/assetserver/content_type_sniffer.gov3/internal/assetserver/content_type_sniffer_test.gov3/internal/assetserver/webview/responsewriter_darwin.gov3/internal/assetserver/webview/responsewriter_ios.gov3/internal/assetserver/webview/responsewriter_linux.gov3/internal/assetserver/webview/responsewriter_linux_gtk3.gov3/internal/assetserver/webview/responsewriter_windows.go
Contributor
There was a problem hiding this comment.
Pull request overview
This PR corrects flushing behavior in the v3 asset server so that Flush() actually releases buffered bytes when Content-Type sniffing is in progress, and so http.ResponseController.Flush() accurately reflects platform capabilities (including explicitly not claiming streaming support on Windows). It also documents why dispatchWorkers must remain 0 to avoid starvation/hangs with long-lived requests.
Changes:
- Fix
contentTypeSniffer.Flush()to complete sniffing and release any buffered prefix before delegating to the wrapped writer’s flush. - Declare no-op
Flush()implementations for non-Windows platform WebView response writers sohttp.ResponseControllercan flush successfully where streaming is already effectively “write-through”. - Document Windows’ intentional lack of
http.Flusherand documentdispatchWorkersstaying0to prevent worker-pool starvation.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| v3/internal/assetserver/webview/responsewriter_windows.go | Documents intentional non-support for http.Flusher to avoid dishonest flush semantics on Windows. |
| v3/internal/assetserver/webview/responsewriter_linux.go | Adds no-op Flush() so http.ResponseController.Flush() can succeed on Linux GTK4/WebKitGTK. |
| v3/internal/assetserver/webview/responsewriter_linux_gtk3.go | Adds no-op Flush() so http.ResponseController.Flush() can succeed on Linux GTK3 legacy path. |
| v3/internal/assetserver/webview/responsewriter_ios.go | Adds no-op Flush() so http.ResponseController.Flush() can succeed on iOS. |
| v3/internal/assetserver/webview/responsewriter_darwin.go | Adds no-op Flush() so http.ResponseController.Flush() can succeed on macOS. |
| v3/internal/assetserver/content_type_sniffer.go | Updates Flush() to resolve and release the sniffing prefix so short streaming writes aren’t silently withheld. |
| v3/internal/assetserver/content_type_sniffer_test.go | Adds unit tests covering flush behavior, buffering semantics, and http.ResponseController integration. |
| v3/internal/assetserver/assetserver_webview.go | Documents dispatchWorkers being intentionally kept at 0 to avoid starvation with long-lived requests. |
Suppressed comments (2)
v3/internal/assetserver/content_type_sniffer_test.go:79
- This test ignores the error return from io.WriteString. Checking it makes the test fail fast if the writer begins returning errors, rather than continuing with potentially invalid assertions.
rw.Header().Set(HeaderContentType, "text/event-stream")
rw.WriteHeader(http.StatusOK)
_, _ = io.WriteString(rw, "data: x\n\n")
rw.Flush()
v3/internal/assetserver/content_type_sniffer_test.go:116
- This test ignores the error return from io.WriteString. If writes start failing, ResponseController.Flush might still return an error and the body assertions become misleading; checking the write error makes the test more robust.
rw.WriteHeader(http.StatusOK)
_, _ = io.WriteString(rw, "chunk")
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Review found a real hole in the previous commit. complete cleared the whole prefix regardless of what the wrapped Write actually accepted, and Flush discarded the error because http.Flusher cannot report one. A writer that failed the flush-triggered write therefore lost the buffered bytes and the failure with them: the request-end complete saw an empty prefix and reported success. complete now advances the prefix only by the number of bytes the writer accepted, so a short or failed write leaves the remainder for a later attempt, and records the error on the sniffer. Write and complete both return that error afterwards, which is how the failure reaches the caller now that Flush cannot. Two tests added, both failing without the change: one fails only the flush-triggered prefix write and asserts the error surfaces from the next Write and from complete, the other uses a short writer and asserts only the unsent remainder is kept. Existing tests also stopped ignoring errors from io.WriteString, so a writer that starts failing can no longer let a test pass while skipping its assertions.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@v3/internal/assetserver/content_type_sniffer_test.go`:
- Around line 193-204: Update shortWriter.Write to return io.ErrShortWrite
whenever it truncates the input because len(b) exceeds w.limit, while preserving
the underlying ResponseWriter error and byte count for non-truncated writes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ed178421-b21e-4b76-927a-bb102f19730a
📒 Files selected for processing (2)
v3/internal/assetserver/content_type_sniffer.gov3/internal/assetserver/content_type_sniffer_test.go
shortWriter returned a short count with a nil error, which io.Writer forbids: an implementation must return a non-nil error whenever it accepts fewer bytes than it was offered. It now reports io.ErrShortWrite, which also makes the test exercise the realistic path - the sniffer records that error and surfaces it from the next Write - rather than a state a conforming writer could never produce. The test now asserts all three: the accepted bytes reach the client, the unsent remainder is retained, and the error reaches the caller.
leaanthony
pushed a commit
that referenced
this pull request
Aug 9, 2026
…erver flush semantics honest
27 tasks
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
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.
Description
Three problems in the asset server, found while investigating Go→JS event dispatch (#5930) and independent of it. Split out so they can be reviewed and land on their own.
1.
contentTypeSniffer.Flush()never completed itselfFlushdelegated straight to the wrapped writer. But until 512 bytes have been seen the sniffer is deliberately holding the body back so it can detect aContent-Type— so a flush before that point sent nothing and reported success.Any response that writes a short chunk and then waits — every streaming format — silently delivered nothing to the page.
Flushnow completes first, sniffing from the short prefix. That's the right reading of an explicit flush: send what has been written so far.2. The platform response writers didn't implement
http.FlusherNone of the five did, so
http.ResponseController(w).Flush()could never reach them.On macOS, iOS and Linux that was only a missing declaration —
Writealready hands each chunk to the URL scheme task (didReceiveData) or into the pipe WebKitGTK consumes, so a flush has no work to do. Those now declare a documented no-op.Windows deliberately does not, and this is the interesting part. Its
Writeaccumulates into an in-memorybytes.Bufferthat is handed to WebView2 only inFinish, so nothing can reach the page mid-response. A no-opFlushthere would claim a capability that doesn't exist and makeResponseController.Flushreport success while nothing is delivered. Left unimplemented, it keeps returningErrNotSupported— which is the truth.That also means streaming cannot work on Windows today regardless of what WebView2 supports. Worth knowing before designing around it: this is host-side, not an engine limitation.
3.
dispatchWorkersis declared and read but never assignedIt reads like an oversight. It isn't — it must stay
0.At
0, every request gets its own goroutine. Any positive value switchesServeWebViewRequestto a fixed worker pool, where a long-lived request (a streaming response, or anything blocking on the frontend) holds its worker for its entire lifetime. Enough of those starve the pool: later requests — including the page's own assets — queue behind them, and the app appears to hang at startup with no error anywhere.Documented rather than removed, since the pooled path is still useful for a request-heavy workload known to be short-lived. It needs a bound on request lifetime before it can be switched on.
Type of change
How Has This Been Tested?
Six new tests in
content_type_sniffer_test.go; three fail without the fix:FlushReleasesShortPrefixWritesPassThroughAfterFlushFlushBeforeWriteFlushWithExplicitContentTypeFullPrefixStillCompletesWithoutFlushSupportsResponseControllerhttp.ResponseControllerfinds the flusherBuilds verified for
linux,windowsandiosin addition to the host, since four of the eight files are platform-specific.Flush)Test Configuration
Checklist:
website/src/pages/changelog.mdxwith details of this PR (v3 changelog entries are added automatically)Summary by CodeRabbit
Bug Fixes
Documentation
Tests