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")