From 8525385f0a4040370168fb1288eb4679ab9bcee1 Mon Sep 17 00:00:00 2001 From: Hashem Aldhaheri <158606+aenawi@users.noreply.github.com> Date: Sat, 12 Sep 2026 14:26:46 +0400 Subject: [PATCH] Tag the UI assets so a rebuilt server changes what the reader sees MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit style.css and app.js went out with no Cache-Control, no ETag and no Last-Modified. embed.FS reports a zero modtime, so net/http had nothing to put in Last-Modified either, and a browser handed a response with no freshness information at all is free to invent its own. That breaks the loop this lab is built around: edit, rebuild control, look at the page. A rebuilt server that does not change the page reads as a broken fix, and it cost a session on #68 debugging code that was correct the whole time. It lands on readers too, not only editors — anyone who opened the lab before pulling can be looking at a new app.js against an old style.css with nothing indicating it. Every embedded asset now carries an ETag over its own bytes and Cache-Control: no-cache, which does not mean "do not store", it means store it and ask first. The hash is computed once at startup because the bytes are in the binary and cannot change while it runs. The header is set before handing off to http.FileServer, which is not a trick: http.ServeContent reads the ETag already on the ResponseWriter when it evaluates If-None-Match, so 304 handling, ranges and content sniffing all stay where they were. No build step, no dependency, no version in index.html. Measured against the running lab on 2026-09-12. Before, style.css answered with no cache headers of any kind. After, it answers with Cache-Control: no-cache and an ETag; a matching If-None-Match returns 304 with no body, a stale one returns the full 37646 bytes. Driving a browser: with style.css already cached, editing it and rebuilding control made a plain reload pick up the change, and a second reload with nothing changed moved 300 bytes on the wire for a 37692-byte stylesheet. AGENTS.md and CLAUDE.md both told agents a hard reload was part of the loop. It no longer is, so they say so. Closes #70 --- AGENTS.md | 10 +- CLAUDE.md | 7 +- lab/control/assets.go | 89 +++++++++++++++++ lab/control/assets_test.go | 192 +++++++++++++++++++++++++++++++++++++ lab/control/main.go | 6 +- 5 files changed, 298 insertions(+), 6 deletions(-) create mode 100644 lab/control/assets.go create mode 100644 lab/control/assets_test.go diff --git a/AGENTS.md b/AGENTS.md index 9835452..15dae22 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -80,12 +80,18 @@ docker compose up -d --build --no-deps control ``` Then confirm the server is actually serving your change — `curl` the asset and -grep it — before concluding anything about whether the change worked. The -browser caches these aggressively too, so a hard reload is part of the loop. +grep it — before concluding anything about whether the change worked. This has already sent one session off to debug a fix that was correct the whole time. +A hard reload used to be part of that loop as well, because the assets went out +with no `ETag` and no `Cache-Control` and the browser was free to keep whatever +it had. It no longer is: every asset is tagged with a hash of its own bytes and +marked `no-cache`, so an ordinary reload revalidates and picks up the rebuild. A +304 keeps that cheap. If you find yourself reaching for Cmd+Shift+R to see a +change, that is a bug worth reporting rather than a step worth remembering. + ## Verify in the lab, not only in the tests `make check` runs without Docker and finishes in about a second. It is necessary diff --git a/CLAUDE.md b/CLAUDE.md index 387504c..f31902f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -31,9 +31,10 @@ You can run the lab, drive a browser and call the API, which means "I believe this works" is almost never the best sentence available to you. Run it, and report what came back. -Two traps that make a correct change look broken, both covered in AGENTS.md: -the UI is `//go:embed`-ed and needs the control container rebuilt, and the -browser caches the assets hard. +One trap that makes a correct change look broken, covered in AGENTS.md: the UI +is `//go:embed`-ed and needs the control container rebuilt. The browser no +longer needs a hard reload on top of that — the assets carry an `ETag` and +revalidate — but the rebuild is still yours to run. ## The lab on this machine is probably in use diff --git a/lab/control/assets.go b/lab/control/assets.go new file mode 100644 index 0000000..c99101c --- /dev/null +++ b/lab/control/assets.go @@ -0,0 +1,89 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +// Copyright (C) 2026 Hashem Aldhaheri + +package main + +import ( + "crypto/sha256" + "encoding/hex" + "io/fs" + "net/http" + "path" + "strings" +) + +// The UI is //go:embed-ed, so the server knows the exact bytes of every asset +// at build time and can say so. Without that, style.css and app.js went out +// with no Cache-Control, no ETag and no Last-Modified — embed.FS reports a zero +// modtime, so net/http had nothing to put there — and a browser handed a +// response with no freshness information at all is free to invent one. The loop +// this lab is built around is edit, `docker compose up -d --build --no-deps +// control`, look at the page; a rebuilt server that does not change the page +// reads as a broken fix, and that cost a session on #68. +// +// So every asset carries an ETag over its own bytes and Cache-Control: +// no-cache, which does not mean "do not store" — it means store it and ask +// before using it. The browser revalidates on every load and gets a 304 with no +// body back when nothing moved. One round trip against a server on loopback, +// and the page can never be a new app.js against yesterday's style.css. +// +// The hash is computed once at startup rather than per request: the bytes are +// in the binary and cannot change while it runs. + +// staticUI serves fsys with a per-file ETag and Cache-Control: no-cache. +// +// It sets the header and then hands off to http.FileServer, which is not a +// trick: http.ServeContent reads the ETag already on the ResponseWriter when it +// evaluates If-None-Match, so the 304 handling, range requests and content +// sniffing all stay where they were. +func staticUI(fsys fs.FS) (http.Handler, error) { + tags, err := assetETags(fsys) + if err != nil { + return nil, err + } + files := http.FileServer(http.FS(fsys)) + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if tag, ok := tags[assetName(r.URL.Path)]; ok { + w.Header().Set("ETag", tag) + w.Header().Set("Cache-Control", "no-cache") + } + files.ServeHTTP(w, r) + }), nil +} + +// assetETags hashes every file in fsys, keyed by its path within fsys. +func assetETags(fsys fs.FS) (map[string]string, error) { + tags := map[string]string{} + err := fs.WalkDir(fsys, ".", func(p string, d fs.DirEntry, err error) error { + if err != nil || d.IsDir() { + return err + } + b, err := fs.ReadFile(fsys, p) + if err != nil { + return err + } + sum := sha256.Sum256(b) + // Half of SHA-256 is far more than enough to tell two versions of one + // stylesheet apart, and a shorter header is easier to read in curl -I. + tags[p] = `"` + hex.EncodeToString(sum[:16]) + `"` + return nil + }) + if err != nil { + return nil, err + } + return tags, nil +} + +// assetName maps a request path to the name of the embedded file +// http.FileServer will serve for it, so the two agree on what is being tagged. +// A directory gets its index.html, which is how / becomes index.html. +func assetName(p string) string { + if !strings.HasPrefix(p, "/") { + p = "/" + p + } + // Before Clean, which strips the trailing slash that marks a directory. + if strings.HasSuffix(p, "/") { + p += "index.html" + } + return strings.TrimPrefix(path.Clean(p), "/") +} diff --git a/lab/control/assets_test.go b/lab/control/assets_test.go new file mode 100644 index 0000000..8902515 --- /dev/null +++ b/lab/control/assets_test.go @@ -0,0 +1,192 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +// Copyright (C) 2026 Hashem Aldhaheri + +package main + +import ( + "io/fs" + "net/http" + "net/http/httptest" + "testing" + "testing/fstest" +) + +func testUI() fs.FS { + return fstest.MapFS{ + "index.html": {Data: []byte("")}, + "style.css": {Data: []byte(".panel { color: red }")}, + "app.js": {Data: []byte("export const x = 1\n")}, + "hud/scene.js": {Data: []byte("export function scene() {}\n")}, + } +} + +func serveUI(t *testing.T, fsys fs.FS) http.Handler { + t.Helper() + h, err := staticUI(fsys) + if err != nil { + t.Fatalf("staticUI: %v", err) + } + return h +} + +func get(t *testing.T, h http.Handler, path string, headers map[string]string) *http.Response { + t.Helper() + r := httptest.NewRequest(http.MethodGet, path, nil) + for k, v := range headers { + r.Header.Set(k, v) + } + w := httptest.NewRecorder() + h.ServeHTTP(w, r) + return w.Result() +} + +// Every asset answers with something the browser can revalidate against. This +// is the whole bug in #70: with no ETag and no Cache-Control, a rebuilt server +// does not change what the reader sees. +func TestAssetsCarryAnETagAndRevalidate(t *testing.T) { + h := serveUI(t, testUI()) + + // Not /index.html: http.FileServer redirects that to / on its own, and + // that predates this change. + for _, path := range []string{"/", "/style.css", "/app.js", "/hud/scene.js"} { + res := get(t, h, path, nil) + if res.StatusCode != http.StatusOK { + t.Fatalf("%s: status %d, want 200", path, res.StatusCode) + } + if got := res.Header.Get("ETag"); got == "" { + t.Errorf("%s: no ETag", path) + } + if got := res.Header.Get("Cache-Control"); got != "no-cache" { + t.Errorf("%s: Cache-Control %q, want %q", path, got, "no-cache") + } + } +} + +// The cheap half: an unchanged asset comes back as a bodiless 304, so +// revalidating on every load costs a round trip and not a stylesheet. +func TestUnchangedAssetIsNotModified(t *testing.T) { + h := serveUI(t, testUI()) + + first := get(t, h, "/style.css", nil) + tag := first.Header.Get("ETag") + if tag == "" { + t.Fatal("no ETag on the first response") + } + + second := get(t, h, "/style.css", map[string]string{"If-None-Match": tag}) + if second.StatusCode != http.StatusNotModified { + t.Fatalf("status %d, want 304", second.StatusCode) + } + if n := second.ContentLength; n > 0 { + t.Errorf("304 carried %d bytes of body", n) + } + if got := second.Header.Get("ETag"); got != tag { + t.Errorf("304 ETag %q, want %q", got, tag) + } +} + +// The point of hashing the bytes rather than stamping a build: edit the file, +// and the tag the browser is holding stops matching. +func TestEditedAssetGetsANewETag(t *testing.T) { + before := get(t, serveUI(t, testUI()), "/style.css", nil).Header.Get("ETag") + + edited := testUI().(fstest.MapFS) + edited["style.css"] = &fstest.MapFile{Data: []byte(".panel { color: blue }")} + after := get(t, serveUI(t, edited), "/style.css", nil).Header.Get("ETag") + + if before == "" || after == "" { + t.Fatalf("missing ETag: before %q, after %q", before, after) + } + if before == after { + t.Errorf("ETag %q survived an edit to the file", before) + } + + // And the stale tag no longer satisfies the request, so the browser is + // handed the new bytes rather than told to keep the old ones. + res := get(t, serveUI(t, edited), "/style.css", map[string]string{"If-None-Match": before}) + if res.StatusCode != http.StatusOK { + t.Errorf("status %d for a stale ETag, want 200", res.StatusCode) + } +} + +// Two assets that differ must not share a tag, or a page can be a new app.js +// against an old style.css with nothing indicating it. +func TestETagsAreNotSharedBetweenAssets(t *testing.T) { + h := serveUI(t, testUI()) + + seen := map[string]string{} + for _, path := range []string{"/", "/style.css", "/app.js", "/hud/scene.js"} { + tag := get(t, h, path, nil).Header.Get("ETag") + if other, clash := seen[tag]; clash { + t.Errorf("%s and %s share the ETag %s", other, path, tag) + } + seen[tag] = path + } +} + +// The tag on / has to be the tag of index.html, the file / actually serves, or +// the root document is the one page that never revalidates correctly. +func TestRootIsTaggedWithIndexHTML(t *testing.T) { + fsys := testUI() + tags, err := assetETags(fsys) + if err != nil { + t.Fatalf("assetETags: %v", err) + } + + root := get(t, serveUI(t, fsys), "/", nil).Header.Get("ETag") + if root != tags["index.html"] { + t.Errorf("/ tagged %q, index.html hashes to %q", root, tags["index.html"]) + } +} + +func TestAssetName(t *testing.T) { + cases := map[string]string{ + "/": "index.html", + "/index.html": "index.html", + "/style.css": "style.css", + "/hud/": "hud/index.html", + "/hud/scene.js": "hud/scene.js", + "/./style.css": "style.css", + "/hud/../app.js": "app.js", + "/../../etc/pass": "etc/pass", + "style.css": "style.css", + } + for in, want := range cases { + if got := assetName(in); got != want { + t.Errorf("assetName(%q) = %q, want %q", in, got, want) + } + } +} + +// A path with no file behind it is served exactly as it was before: no tag +// invented for something that does not exist, and net/http still 404s. +func TestMissingAssetIsUntagged(t *testing.T) { + res := get(t, serveUI(t, testUI()), "/nope.css", nil) + if res.StatusCode != http.StatusNotFound { + t.Fatalf("status %d, want 404", res.StatusCode) + } + if got := res.Header.Get("ETag"); got != "" { + t.Errorf("404 carried ETag %q", got) + } +} + +// The real embedded UI, not a fixture: the files this lab actually ships are +// the ones that were going out unrevalidatable. +func TestEmbeddedUIIsTagged(t *testing.T) { + ui, err := fs.Sub(uiFS, "ui") + if err != nil { + t.Fatalf("embedded UI: %v", err) + } + h := serveUI(t, ui) + + for _, path := range []string{"/", "/style.css", "/app.js"} { + res := get(t, h, path, nil) + if res.StatusCode != http.StatusOK { + t.Errorf("%s: status %d, want 200", path, res.StatusCode) + } + if res.Header.Get("ETag") == "" || res.Header.Get("Cache-Control") != "no-cache" { + t.Errorf("%s: ETag %q, Cache-Control %q", + path, res.Header.Get("ETag"), res.Header.Get("Cache-Control")) + } + } +} diff --git a/lab/control/main.go b/lab/control/main.go index 9915b21..d93fc39 100644 --- a/lab/control/main.go +++ b/lab/control/main.go @@ -500,7 +500,11 @@ func routes(c *Controller) http.Handler { if err != nil { log.Fatalf("embedded UI: %v", err) } - mux.Handle("/", http.FileServer(http.FS(ui))) + assets, err := staticUI(ui) + if err != nil { + log.Fatalf("embedded UI: %v", err) + } + mux.Handle("/", assets) writeJSON := func(w http.ResponseWriter, v any) { w.Header().Set("Content-Type", "application/json")