amadan

carlos / platform Public

edge: static tags cover the content type, hash once per file, and follow the file

Astra round 2 on the rework (accept with fixes), each finding reproduced
as a failing test first:

- The same bytes can be two representations: an extensionless /about is
  text/plain, the same bytes at about/index.html are text/html, and a
  304 carries no Content-Type. The file's extension is hashed in.
- Every concurrent request for a cold file hashed the whole of it. One
  request hashes and the rest wait for it; at most four files hash at a
  time; a request that gives up waiting is served untagged.
- A remembered tag was keyed by path, so a tree replaced on disk under a
  live handler kept its old tag. Entries are checked against the open
  file's identity, size and mtime.
- The memo was per handler, and the edge rebuilds every static handler
  on each route-table reload. It is one bounded cache for the process.
- An unreadable file answered 404. Open errors map as ServeFile mapped
  them: 404, 403, else 500.
- The identity-GET assertion was requesting gzip through the Go client's
  default. It now asks for identity by name.
Agent notes
Each new test was checked against its broken variant: no extension in the hash, no coalescing, no identity check, and every open error a 404. The static tests pass under -race and 20 times over.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude fcaf736edab6cd3b4a79af4f022c45c33ec071e5 parent 08bd871
2 files changed, +280 −46
  • internal/edge/static.go +136 −43
  • internal/edge/static_cache_test.go +144 −3

CI passed — run details

diff --git a/internal/edge/static.go b/internal/edge/static.go
index 89aedc5f9..e816af52b 100644
--- a/internal/edge/static.go
+++ b/internal/edge/static.go
@@ -17,6 +17,7 @@ import (
"crypto/ed25519"
"crypto/sha256"
"encoding/hex"
+ "errors"
"fmt"
"io"
"io/fs"
@@ -28,6 +29,7 @@ import (
"sort"
"strings"
"sync"
+ "sync/atomic"
"time"
"github.com/carlosframework/platform/internal/apperr"
@@ -122,12 +124,6 @@ type staticSite struct {
mu sync.Mutex
version string // currently served version; "" until first resolve
checkedAt time.Time // when the pointer was last read
-
- // tags memoizes fileTag by extracted file path. Its own lock, not mu:
- // requests hash and look up concurrently, and mu is held across a
- // pointer read.
- tagMu sync.Mutex
- tags map[string]string
}
// newStaticHandler builds the serving handler for one static route. The
@@ -229,9 +225,6 @@ func (s *staticSite) currentDir(ctx context.Context) (dir, version string, err e
return s.stale(fmt.Errorf("swap current -> %s: %w", cp.Version, err))
}
s.version = cp.Version
- s.tagMu.Lock()
- s.tags = nil // the memo's paths are the previous tree's
- s.tagMu.Unlock()
}
s.checkedAt = time.Now()
return s.dir(s.version), s.version, nil
@@ -395,7 +388,7 @@ func extractSiteTar(data []byte, dst string) error {
// holds, so "modified since?" answers no and the browser keeps the build that
// was withdrawn. No date is offered, so there is none to compare.
//
-// The validator is the file's content (fileTag). What ServeFile did besides
+// The validator is the file's content (tagCache). What ServeFile did besides
// is kept by hand: the refusal of a ".." element, and the redirect off a
// trailing /index.html onto its directory.
func (s *staticSite) serveTree(dir string, w http.ResponseWriter, r *http.Request) {
@@ -430,23 +423,41 @@ func (s *staticSite) serveTree(dir string, w http.ResponseWriter, r *http.Reques
}
f, err := os.Open(fp)
if err != nil {
- http.NotFound(w, r)
+ // ServeFile's mapping, kept: a file that vanished since the Stat is
+ // a 404, but one that is there and will not open is not a missing
+ // page, and must not read as one in an access log.
+ switch {
+ case errors.Is(err, fs.ErrNotExist):
+ http.NotFound(w, r)
+ case errors.Is(err, fs.ErrPermission):
+ http.Error(w, "403 Forbidden", http.StatusForbidden)
+ default:
+ log.Printf("edge: static %s/%s: open %s: %v", s.app, s.channel, p, err)
+ http.Error(w, "500 Internal Server Error", http.StatusInternalServerError)
+ }
return
}
defer f.Close()
- if tag, err := s.fileTag(fp, f); err == nil {
- w.Header().Set("ETag", tag)
- } else {
- // No tag is the honest answer: the response then carries no
- // validator and no-cache makes the browser refetch it whole.
- log.Printf("edge: static %s/%s: tag %s: %v", s.app, s.channel, p, err)
+ // Stat the OPEN file: its identity is what a remembered tag is checked
+ // against, and it is the file whose bytes are about to be sent.
+ if ofi, err := f.Stat(); err == nil && !ofi.IsDir() {
+ fi = ofi
+ if tag, err := staticTags.tag(r.Context(), fp, f, fi); err == nil {
+ w.Header().Set("ETag", tag)
+ } else if r.Context().Err() == nil {
+ // No tag is the honest answer: the response then carries no
+ // validator and no-cache makes the browser refetch it whole.
+ log.Printf("edge: static %s/%s: tag %s: %v", s.app, s.channel, p, err)
+ }
}
// ServeContent brings Content-Type, ranges and the conditional-request
// handling; the zero time is "no Last-Modified, and no date to test".
http.ServeContent(w, r, fi.Name(), time.Time{}, f)
}
-// fileTag is the entity tag for one extracted file: a hash of its bytes.
+// tagCache computes and remembers the entity tag of each extracted file: a
+// hash of its bytes and of the one thing besides them that decides what the
+// client receives, the file name's extension.
//
// The content, and nothing that merely names it. A version name is reused —
// a pruned version re-shipped under its old name is a new release
@@ -457,40 +468,122 @@ func (s *staticSite) serveTree(dir string, w http.ResponseWriter, r *http.Reques
// release that changed one page does not make every browser refetch every
// asset.
//
-// WEAK, although a hash of the identity bytes is as strong as a validator
-// gets. The edge gzips compressible responses above this handler and weakens
-// a strong tag when it does (compress.go, weakenETag) — but only on the 200.
-// A 304 and a HEAD pass through as they are, so a strong tag would come back
-// weak on the page and strong on its own revalidation, and a cache may not
-// update the one from the other. Weak from the start, it reads the same on
-// all three. The cost is that If-Range falls back to the whole file.
+// The extension is hashed in because the same bytes can be two
+// representations. net/http types a file by its extension and sniffs only
+// when there is none, so /about served from an extensionless file is
+// text/plain and the same bytes at about/index.html are text/html. A 304
+// carries no Content-Type; bytes alone would leave the browser rendering the
+// old one.
+//
+// WEAK, although such a hash is as strong as a validator gets. The edge gzips
+// compressible responses above this handler and weakens a strong tag when it
+// does (compress.go, weakenETag) — but only on the 200. A 304 and a HEAD pass
+// through as they are, so a strong tag would come back weak on the page and
+// strong on its own revalidation, and a cache may not update the one from the
+// other. Weak from the start, it reads the same on all three. The cost is
+// that If-Range falls back to the whole file.
+//
+// One cache for the process rather than one per handler: the edge rebuilds
+// every static handler on each route-table reload, and a per-handler memo
+// would re-hash every site each time a route anywhere on the box changed.
+// Three properties make a shared, long-lived memo safe:
//
-// Memoized per file: extracted trees are immutable, so a path hashes once for
-// as long as this handler serves its version. The memo is dropped when the
-// version changes (the paths are the old tree's) and with the handler itself,
-// so it never outgrows one tree's worth of the files actually requested.
-func (s *staticSite) fileTag(fp string, f *os.File) (string, error) {
- s.tagMu.Lock()
- tag, ok := s.tags[fp]
- s.tagMu.Unlock()
- if ok {
- return tag, nil
+// - An entry is checked against the file it was computed FROM, not the path
+// it was computed at (os.SameFile, size, mtime). A tree replaced on disk
+// under a live handler — cache maintenance, then a re-shipped version of
+// the same name extracted again — is new files, and hashes again.
+// - A cold file is hashed once however many requests want it: the rest
+// wait for the one in flight. And at most tagHashers files hash at a
+// time, so a sweep of a site's large assets cannot occupy every core and
+// the disk at once. A request that gives up waiting is served untagged.
+// - It is bounded: past tagCacheMax entries it is emptied and refills on
+// demand, which costs hashing and never correctness.
+type tagCache struct {
+ mu sync.Mutex
+ entries map[string]tagEntry
+ flights map[string]chan struct{} // closed when the hash for that path is done
+ sem chan struct{}
+ hashed atomic.Int64 // files hashed since start; the tests' evidence of coalescing
+}
+
+type tagEntry struct {
+ fi os.FileInfo
+ tag string
+}
+
+const (
+ tagHashers = 4
+ tagCacheMax = 1 << 16
+)
+
+var staticTags = &tagCache{
+ entries: map[string]tagEntry{},
+ flights: map[string]chan struct{}{},
+ sem: make(chan struct{}, tagHashers),
+}
+
+// current reports whether e still describes the file fi is.
+func (e tagEntry) current(fi os.FileInfo) bool {
+ return e.fi != nil && os.SameFile(e.fi, fi) && e.fi.Size() == fi.Size() && e.fi.ModTime().Equal(fi.ModTime())
+}
+
+// tag returns the entity tag for the open file f at fp, hashing it if no
+// current entry exists. f is left positioned at its start. ctx bounds the
+// WAITING — on another request's hash, or on a free hasher — never a hash in
+// progress, whose result every later request needs anyway.
+func (c *tagCache) tag(ctx context.Context, fp string, f *os.File, fi os.FileInfo) (string, error) {
+ for {
+ c.mu.Lock()
+ if e, ok := c.entries[fp]; ok && e.current(fi) {
+ c.mu.Unlock()
+ return e.tag, nil
+ }
+ if wait, ok := c.flights[fp]; ok {
+ c.mu.Unlock()
+ select {
+ case <-wait:
+ continue // look again: usually a hit, a miss if the file changed
+ case <-ctx.Done():
+ return "", ctx.Err()
+ }
+ }
+ done := make(chan struct{})
+ c.flights[fp] = done
+ c.mu.Unlock()
+
+ tag, err := c.hash(ctx, f, fi)
+ c.mu.Lock()
+ if err == nil {
+ if len(c.entries) >= tagCacheMax {
+ c.entries = map[string]tagEntry{}
+ }
+ c.entries[fp] = tagEntry{fi: fi, tag: tag}
+ }
+ delete(c.flights, fp)
+ c.mu.Unlock()
+ close(done)
+ return tag, err
}
+}
+
+func (c *tagCache) hash(ctx context.Context, f *os.File, fi os.FileInfo) (string, error) {
+ select {
+ case c.sem <- struct{}{}:
+ defer func() { <-c.sem }()
+ case <-ctx.Done():
+ return "", ctx.Err()
+ }
+ c.hashed.Add(1)
h := sha256.New()
+ io.WriteString(h, strings.ToLower(filepath.Ext(fi.Name())))
+ h.Write([]byte{0})
if _, err := io.Copy(h, f); err != nil {
return "", err
}
if _, err := f.Seek(0, io.SeekStart); err != nil {
return "", err
}
- tag = `W/"` + hex.EncodeToString(h.Sum(nil)[:16]) + `"`
- s.tagMu.Lock()
- if s.tags == nil {
- s.tags = map[string]string{}
- }
- s.tags[fp] = tag
- s.tagMu.Unlock()
- return tag, nil
+ return `W/"` + hex.EncodeToString(h.Sum(nil)[:16]) + `"`, nil
}
// containsDotDot reports whether a URL path has a ".." element — net/http's
diff --git a/internal/edge/static_cache_test.go b/internal/edge/static_cache_test.go
index 6ff1ef5d1..0aa64924e 100644
--- a/internal/edge/static_cache_test.go
+++ b/internal/edge/static_cache_test.go
@@ -5,8 +5,10 @@ import (
"io"
"net/http"
"net/http/httptest"
+ "os"
"path/filepath"
"strings"
+ "sync"
"testing"
"github.com/carlosframework/platform/internal/registry"
@@ -32,13 +34,24 @@ func reqStatic(t *testing.T, method, base, path string, hdr map[string]string) (
t.Fatalf("%s %s%s: %v", method, base, path, err)
}
defer resp.Body.Close()
- b, _ := io.ReadAll(resp.Body)
+ b, err := io.ReadAll(resp.Body)
+ if err != nil {
+ t.Fatalf("%s %s%s: read body: %v", method, base, path, err)
+ }
return resp, string(b)
}
// siteServer ships files as version of app into its own store and serves it
// from a fresh handler, behind the same compression wrapper the edge applies.
func siteServer(t *testing.T, app, version string, files map[string]string) *httptest.Server {
+ t.Helper()
+ srv, _ := siteServerTree(t, app, version, files)
+ return srv
+}
+
+// siteServerTree is siteServer that also says where version's tree is
+// extracted, for the tests that reach under the handler.
+func siteServerTree(t *testing.T, app, version string, files map[string]string) (*httptest.Server, string) {
t.Helper()
store := release.NewDirStore(filepath.Join(t.TempDir(), "store"))
priv, pub := testKeys(t)
@@ -57,7 +70,7 @@ func siteServer(t *testing.T, app, version string, files map[string]string) *htt
h.ServeHTTP(w, r)
}))
t.Cleanup(srv.Close)
- return srv
+ return srv, h.(*staticSite).dir(version)
}
// A static response used to carry Last-Modified and nothing else, and a
@@ -240,7 +253,12 @@ func TestStaticTagIsTheSameCompressedRevalidatedAndOnHead(t *testing.T) {
if got := resp.Header.Get("ETag"); got != tag {
t.Errorf("HEAD ETag = %q, want the GET's %q", got, tag)
}
- resp, _ = reqStatic(t, "GET", srv.URL, "/", nil) // identity
+ // Asked for by name: with no Accept-Encoding of its own the Go client
+ // requests gzip and hides that it did.
+ resp, body := reqStatic(t, "GET", srv.URL, "/", map[string]string{"Accept-Encoding": "identity"})
+ if resp.StatusCode != http.StatusOK || body != page || resp.Header.Get("Content-Encoding") != "" {
+ t.Fatalf("identity GET = %d, Content-Encoding %q, %d bytes; want the plain page", resp.StatusCode, resp.Header.Get("Content-Encoding"), len(body))
+ }
if got := resp.Header.Get("ETag"); got != tag {
t.Errorf("identity GET ETag = %q, want the gzipped GET's %q", got, tag)
}
@@ -284,3 +302,126 @@ func TestStaticKeepsServeFileBehaviour(t *testing.T) {
t.Fatalf("a path with a .. element = %d, want 400", resp2.StatusCode)
}
}
+
+// The same bytes can be two representations. /about as an extensionless file
+// is sniffed as text/plain; redeployed as about/index.html it is text/html,
+// byte for byte the same. A tag made of the bytes alone would answer 304 and
+// leave the browser rendering markup as text.
+func TestStaticTagCoversTheContentType(t *testing.T) {
+ store, _, priv, srv := staticFixture(t)
+ const same = "Hello <em>world</em>"
+ shipSite(t, store, "site", "v2", map[string]string{"index.html": "home", "about": same})
+ if err := release.Promote(context.Background(), store, priv, release.OpsAccount, "site", "canary/x", "v2"); err != nil {
+ t.Fatalf("Promote v2: %v", err)
+ }
+ resp, _ := reqStatic(t, "GET", srv.URL, "/about", nil)
+ plainTag, plainType := resp.Header.Get("ETag"), resp.Header.Get("Content-Type")
+ if resp.StatusCode != http.StatusOK || !strings.HasPrefix(plainType, "text/plain") {
+ t.Fatalf("GET /about = %d %q, want a text/plain 200", resp.StatusCode, plainType)
+ }
+
+ shipSite(t, store, "site", "v3", map[string]string{"index.html": "home", "about/index.html": same})
+ if err := release.Promote(context.Background(), store, priv, release.OpsAccount, "site", "canary/x", "v3"); err != nil {
+ t.Fatalf("Promote v3: %v", err)
+ }
+ resp, body := reqStatic(t, "GET", srv.URL, "/about", map[string]string{"If-None-Match": plainTag})
+ if resp.StatusCode != http.StatusOK || body != same || !strings.HasPrefix(resp.Header.Get("Content-Type"), "text/html") {
+ t.Fatalf("revalidating /about after it became HTML = %d %q; want a text/html 200", resp.StatusCode, resp.Header.Get("Content-Type"))
+ }
+}
+
+// A cold file is hashed ONCE however many requests arrive for it together.
+// Without that, a burst of requests against one large uncached asset — HEADs
+// and one-byte ranges included — would each read and hash the whole file
+// before sending a byte.
+func TestStaticColdFileIsHashedOnceUnderABurst(t *testing.T) {
+ big := strings.Repeat("0123456789abcdef", 64*1024) // 1 MiB
+ srv := siteServer(t, "site", "v1", map[string]string{"index.html": "home", "big.bin": big})
+ before := staticTags.hashed.Load()
+
+ var wg sync.WaitGroup
+ tags := make([]string, 24)
+ for i := range tags {
+ wg.Add(1)
+ go func(i int) {
+ defer wg.Done()
+ method := "GET"
+ if i%2 == 1 {
+ method = "HEAD"
+ }
+ req, err := http.NewRequest(method, srv.URL+"/big.bin", nil)
+ if err != nil {
+ return
+ }
+ req.Host = "site.example.com"
+ req.Header.Set("Range", "bytes=0-0")
+ resp, err := http.DefaultClient.Do(req)
+ if err != nil {
+ return
+ }
+ io.Copy(io.Discard, resp.Body)
+ resp.Body.Close()
+ tags[i] = resp.Header.Get("ETag")
+ }(i)
+ }
+ wg.Wait()
+ for i, tag := range tags {
+ if tag == "" || tag != tags[0] {
+ t.Fatalf("request %d ETag = %q, want every request to carry %q", i, tag, tags[0])
+ }
+ }
+ if got := staticTags.hashed.Load() - before; got != 1 {
+ t.Fatalf("the file was hashed %d times under a burst of %d requests, want once", got, len(tags))
+ }
+}
+
+// A remembered tag describes the file it was computed from, not the path. If
+// the tree is replaced on disk under a live handler — cache maintenance, then
+// another handler unpacking a re-shipped version of the same name — the old
+// tag must not be handed out over the new bytes, nor a 304 against it.
+func TestStaticTagDoesNotOutliveTheFileItDescribes(t *testing.T) {
+ srv, tree := siteServerTree(t, "site", "v1", map[string]string{"index.html": "home", "style.css": "body{}"})
+ resp, _ := reqStatic(t, "GET", srv.URL, "/style.css", nil)
+ oldTag := resp.Header.Get("ETag")
+ if oldTag == "" {
+ t.Fatal("no ETag for /style.css")
+ }
+
+ // Replace the file the way a fresh extraction would: new bytes under a
+ // new inode, renamed into place.
+ fp := filepath.Join(tree, "style.css")
+ if err := os.WriteFile(fp+".new", []byte("body{color:red}"), 0o644); err != nil {
+ t.Fatal(err)
+ }
+ if err := os.Rename(fp+".new", fp); err != nil {
+ t.Fatal(err)
+ }
+
+ resp, body := reqStatic(t, "GET", srv.URL, "/style.css", map[string]string{"If-None-Match": oldTag})
+ if resp.StatusCode != http.StatusOK || body != "body{color:red}" {
+ t.Fatalf("revalidating against a replaced file = %d %q, want 200 with the new bytes", resp.StatusCode, body)
+ }
+ if got := resp.Header.Get("ETag"); got == "" || got == oldTag {
+ t.Fatalf("ETag over the replaced file = %q, want a new tag (old %q)", got, oldTag)
+ }
+}
+
+// A file that is there and cannot be read is not a missing page. ServeFile
+// said 403, and an operator reading the access log must still be able to tell
+// a broken cache from a mistyped URL.
+func TestStaticUnreadableFileIsNotA404(t *testing.T) {
+ if os.Geteuid() == 0 {
+ t.Skip("root reads through file modes")
+ }
+ srv, tree := siteServerTree(t, "site", "v1", map[string]string{"index.html": "home", "style.css": "body{}"})
+ reqStatic(t, "GET", srv.URL, "/", nil) // extract the tree
+ fp := filepath.Join(tree, "style.css")
+ if err := os.Chmod(fp, 0); err != nil {
+ t.Fatal(err)
+ }
+ t.Cleanup(func() { os.Chmod(fp, 0o644) })
+ resp, _ := reqStatic(t, "GET", srv.URL, "/style.css", nil)
+ if resp.StatusCode != http.StatusForbidden {
+ t.Fatalf("GET of an unreadable file = %d, want 403", resp.StatusCode)
+ }
+}