fix: stop serving the page shell in place of a missing UI asset - #7482
Open
ibuildthecloud wants to merge 1 commit into
Open
fix: stop serving the page shell in place of a missing UI asset#7482ibuildthecloud wants to merge 1 commit into
ibuildthecloud wants to merge 1 commit into
Conversation
A request for a build asset that is not present was answered with the page shell: 200, text/html, and no cache policy of its own. Browsers refuse that under strict MIME checking, and because it is a 200 every cache in the path stores it -- the CDN edge, and then each visitor's browser. A rolling deploy is enough to produce the miss. For a few seconds one replica serves a shell naming hashes another replica does not have yet. That window is seconds; what it leaves behind is not. Recovering meant purging the CDN and every browser that had loaded the site meanwhile, and the error it produces names MIME types rather than the deploy that caused it. Three things, together: Assets under /_app/ now 404 when absent. A name derived from a file's contents is never a route, so falling back could only ever be wrong there. Everything else still falls back, because a client-side route is what the shell exists to resolve. Obot states its own cache policy. It set none for the UI, so whichever CDN sat in front applied its default -- four hours, in the case this fixes. Content- hashed files are immutable for a year, since a change to one is a change to its name. The shell, version.json and the fallback are no-cache: they keep their names across builds, and name assets belonging to the build they came from. The 404 is no-store, because an edge caches a 404 briefly by default, which is long enough to outlive the deploy that caused it. The embedded build is reached through a variable so a test can supply one. It exists only after the UI is compiled, so tests that read it would otherwise report on whether someone had run make. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Deploying obot with
|
| Latest commit: |
7e8fe72
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://56efd119.obot-dvt.pages.dev |
| Branch Preview URL: | https://fix-ui-asset-cache.obot-dvt.pages.dev |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the UI static file handler so missing built assets under /_app/ return 404 (instead of falling back to the HTML shell), and explicitly sets cache headers to prevent CDN/browser cache poisoning during rolling deploy skew.
Changes:
- Return
404(withCache-Control: no-store) for missing/_app/*assets to avoid serving/caching HTML as JS/CSS. - Add explicit cache policy: long-lived immutable caching for content-hashed assets under
/_app/immutable/, andno-cachefor shell/routes and non-hashed build files. - Add unit tests using an injected
fs.FS(fstest.MapFS) so tests don’t depend on a locally-built UI.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| ui/handler.go | Adds cache headers; distinguishes missing build assets from client-side routes; makes embedded build FS swappable for tests. |
| ui/handler_test.go | Adds coverage for missing-asset 404 behavior and cache policy via an in-memory build FS. |
Suppressed comments (1)
ui/handler.go:118
- This build-asset check should use the same normalized path used for cache policy / FS lookup; otherwise dot-segment paths (e.g. "/_app/../admin") can be misclassified as missing build assets and forced to 404.
} else if strings.HasPrefix(r.URL.Path, buildAssetPrefix) {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+81
to
97
| // Said here rather than left to whatever CDN sits in front. Obot set no | ||
| // policy for the UI at all, so the edge applied its own default -- four | ||
| // hours -- to everything, including a page shell served in place of a | ||
| // missing asset. Stating it keeps that decision where the meaning of each | ||
| // response is known. | ||
| if strings.HasPrefix(r.URL.Path, immutableAssetPrefix) { | ||
| // A change to one of these is a change to its name, so there is nothing | ||
| // a client can hold that will ever be wrong. | ||
| w.Header().Set("Cache-Control", "public, max-age=31536000, immutable") | ||
| } else { | ||
| // Everything else keeps its name across builds -- the shell above all, | ||
| // which names the assets for the build it came from. Cached, it goes on | ||
| // asking for files that a later build no longer has. | ||
| w.Header().Set("Cache-Control", "no-cache") | ||
| } | ||
|
|
||
| userPath := path.Join("user/build/", r.URL.Path) |
| http.ServeFileFS(w, r, embedded, "user/build/admin.html") | ||
| http.ServeFileFS(w, r, assets, "user/build/admin.html") | ||
| } else if r.URL.Path == "/admin/" { | ||
| // we have to redirect to /admin instead of serving the index.html file because ending slash will laod a different route for js files |
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.
Found deploying to a real environment: the site came back from a deploy with a
blank page and a wall of
What was happening
A request for a build asset that is not present fell through to
fallback.html—200,text/html, and no cache policy of its own. Browsersrefuse that under strict MIME checking, and because it is a
200, everycache in the path stores it: the CDN edge, and then each visitor's browser.
A rolling deploy is enough to produce the miss. For a few seconds one replica
serves a shell naming hashes another replica does not have yet. The window is
seconds; what it leaves behind is not — recovering took a full CDN purge and
a browser cache clear, while the origin had been healthy throughout. The MIME
error says nothing about any of that.
Content-hashed names made it slower to read: chunks whose contents had not
changed kept the same name and loaded fine, so only some assets failed, which
looks like corruption rather than absence.
What changed
Missing assets under
/_app/return404. A name derived from a file'scontents is never a route, so falling back could only ever be wrong there.
Everything else still falls back — resolving a client-side route is what the
shell is for.
Obot states its own cache policy. It set none for the UI (
server.gocovers
/api/only), so whichever CDN sat in front applied its default —4 hours, in the case this fixes:
/_app/immutable/…public, max-age=31536000, immutable/_app/version.jsonno-cacheno-storeno-cacheon the shell is the point: it names the assets of the build it camefrom, so a cached copy goes on requesting files a later build no longer has —
the same failure, self-inflicted.
no-storeon the 404 because an edge caches404s briefly by default, which is long enough to outlive the deploy.
Tests
uihad none; it now has 5 tests / 9 subtests. Verified non-vacuous — revertingthe fix fails 9 of them.
The embedded build is reached through a variable so a test can supply one. The
real build exists only after
make ui, so the first version of these testspassed on my machine and failed in a clean checkout — they were reporting on
whether someone had run make. They now run against an
fstest.MapFSand passwith no build present, which is the CI condition.
Verified live
/_app/immutable/entry/app.NOTREAL.js200 text/html404,no-store200, no cache policy200,immutable/some/deep/client/route200 text/htmlno-cacheDraft for review of the cache values themselves — particularly
no-cacherather than
no-storeon the shell. They behave identically today because theembedded FS has no modtime and so emits no validators;
no-cacheleaves roomfor
ETags later.🤖 Generated with Claude Code