amadan
edge: a failed static hash hands the file back at its start
Astra round 4 (accept with fixes, one defect): a hash that failed on a read error returned without rewinding, and the handler then served the file untagged through ServeContent, which sniffs a Content-Type from wherever the file is positioned. An extensionless image with a bad block would go out as text/plain. The hash now rewinds on every path out, and the handler answers 500 if even the rewind fails. Also from that round: the last assertion of the busy-hasher test raced its own 20ms budget; the budget is lifted before it. Noted, not changed: a failed If-Match now leaves Content-Type on its bodyless 412, where ServeContent alone would not have set one.
Agent notes
The rewind test uses a reader that fails mid-file, inside and after the sniffed prefix. With the rewind removed it is the only test that fails: on a successful hash ServeContent seeks for itself, so nothing else notices.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2 files changed,
+79
−10
internal/edge/static.go+26 −10internal/edge/static_cache_test.go+53 −0
CI passed — run details
diff --git a/internal/edge/static.go b/internal/edge/static.go| index 0cf6cf776..247ca200d 100644 |
| --- a/internal/edge/static.go |
| +++ b/internal/edge/static.go |
| @@ -448,10 +448,20 @@ func (s *staticSite) serveTree(dir string, w http.ResponseWriter, r *http.Reques |
| // ServeContent leaves it alone. |
| w.Header().Set("ETag", tag) |
| w.Header().Set("Content-Type", ctype) |
| - } else if !errors.Is(err, errTagBusy) && 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) |
| + } else { |
| + if !errors.Is(err, errTagBusy) && 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) |
| + } |
| + // A failed hash rewinds, but if even that failed the file's |
| + // position is unknown and ServeContent would sniff its type |
| + // from the middle of it. |
| + if _, serr := f.Seek(0, io.SeekStart); serr != nil { |
| + log.Printf("edge: static %s/%s: rewind %s: %v", s.app, s.channel, p, serr) |
| + http.Error(w, "500 Internal Server Error", http.StatusInternalServerError) |
| + return |
| + } |
| } |
| } |
| // ServeContent brings Content-Type, ranges and the conditional-request |
| @@ -562,7 +572,7 @@ func (e tagEntry) current(fi os.FileInfo) bool { |
| // hashing it if no current entry exists. f is left positioned at its start. |
| // ctx and the wait budget bound the WAITING — on another request's hash, or |
| // on a free hasher — never a hash in progress. |
| -func (c *tagCache) tag(ctx context.Context, fp string, f *os.File, fi os.FileInfo) (tag, ctype string, err error) { |
| +func (c *tagCache) tag(ctx context.Context, fp string, f io.ReadSeeker, fi os.FileInfo) (tag, ctype string, err error) { |
| var budget <-chan time.Time |
| for { |
| c.mu.Lock() |
| @@ -609,7 +619,7 @@ func (c *tagCache) tag(ctx context.Context, fp string, f *os.File, fi os.FileInf |
| } |
| } |
| -func (c *tagCache) hash(ctx context.Context, budget <-chan time.Time, f *os.File, fi os.FileInfo) (string, string, error) { |
| +func (c *tagCache) hash(ctx context.Context, budget <-chan time.Time, f io.ReadSeeker, fi os.FileInfo) (tag, ctype string, err error) { |
| if fi.Size() > tagInline { |
| select { |
| case c.sem <- struct{}{}: |
| @@ -624,6 +634,15 @@ func (c *tagCache) hash(ctx context.Context, budget <-chan time.Time, f *os.File |
| if c.hold != nil { |
| c.hold() |
| } |
| + // From here the file is being read, and it goes back to its start on |
| + // EVERY path out — a failed hash included. The caller serves the file |
| + // either way, and net/http sniffs a Content-Type from wherever the file |
| + // is positioned. |
| + defer func() { |
| + if _, serr := f.Seek(0, io.SeekStart); serr != nil && err == nil { |
| + tag, ctype, err = "", "", serr |
| + } |
| + }() |
| // net/http's own rule (serveContent): the extension's type, else a sniff |
| // of the first 512 bytes. |
| var head [512]byte |
| @@ -631,7 +650,7 @@ func (c *tagCache) hash(ctx context.Context, budget <-chan time.Time, f *os.File |
| if err != nil && err != io.EOF && err != io.ErrUnexpectedEOF { |
| return "", "", err |
| } |
| - ctype := mime.TypeByExtension(filepath.Ext(fi.Name())) |
| + ctype = mime.TypeByExtension(filepath.Ext(fi.Name())) |
| if ctype == "" { |
| ctype = http.DetectContentType(head[:n]) |
| } |
| @@ -642,9 +661,6 @@ func (c *tagCache) hash(ctx context.Context, budget <-chan time.Time, f *os.File |
| if _, err := io.Copy(h, f); err != nil { |
| return "", "", err |
| } |
| - if _, err := f.Seek(0, io.SeekStart); err != nil { |
| - return "", "", err |
| - } |
| return `W/"` + hex.EncodeToString(h.Sum(nil)[:16]) + `"`, ctype, nil |
| } |
diff --git a/internal/edge/static_cache_test.go b/internal/edge/static_cache_test.go| index 8a51154a6..ff7d9178f 100644 |
| --- a/internal/edge/static_cache_test.go |
| +++ b/internal/edge/static_cache_test.go |
| @@ -446,7 +446,10 @@ func TestTagCacheDoesNotQueueBehindLargeHashes(t *testing.T) { |
| } |
| // Once a hasher frees up the large file is tagged, and stays tagged. |
| + // The budget is lifted first: at 20ms a stalled scheduler could find the |
| + // timer and the free hasher ready together, and pick the timer. |
| <-c.sem |
| + c.wait = time.Minute |
| fp, f, fi = openTagged(t, dir, "big.bin", strings.Repeat("x", tagInline+1)) |
| if tag, _, err := c.tag(context.Background(), fp, f, fi); err != nil || tag == "" { |
| t.Fatalf("a large file with a free hasher = %q, %v", tag, err) |
| @@ -541,3 +544,53 @@ func TestStaticUnreadableFileIsNotA404(t *testing.T) { |
| t.Fatalf("GET of an unreadable file = %d, want 403", resp.StatusCode) |
| } |
| } |
| + |
| +// failingFile reads like a file whose later blocks are bad: the first good |
| +// bytes come back, then every read fails. Seeks work. |
| +type failingFile struct { |
| + data []byte |
| + good int |
| + pos int64 |
| +} |
| + |
| +func (f *failingFile) Read(p []byte) (int, error) { |
| + if f.pos >= int64(f.good) { |
| + return 0, errors.New("input/output error") |
| + } |
| + n := copy(p, f.data[f.pos:f.good]) |
| + f.pos += int64(n) |
| + return n, nil |
| +} |
| + |
| +func (f *failingFile) Seek(off int64, whence int) (int64, error) { |
| + switch whence { |
| + case io.SeekStart: |
| + f.pos = off |
| + case io.SeekCurrent: |
| + f.pos += off |
| + case io.SeekEnd: |
| + f.pos = int64(len(f.data)) + off |
| + } |
| + return f.pos, nil |
| +} |
| + |
| +// A hash that fails part-way must hand the file back at its start. The |
| +// handler then serves it untagged, and net/http sniffs the Content-Type from |
| +// wherever the file is positioned: left mid-file, an extensionless image |
| +// would go out as text/plain. |
| +func TestTagCacheRewindsTheFileWhenHashingFails(t *testing.T) { |
| + c := newTagCache() |
| + _, _, fi := openTagged(t, t.TempDir(), "picture", "placeholder") |
| + for _, good := range []int{100, 2000} { // fails inside the sniffed prefix, and after it |
| + f := &failingFile{data: make([]byte, 4096), good: good} |
| + if _, _, err := c.tag(context.Background(), fmt.Sprintf("/nowhere/picture-%d", good), f, fi); err == nil { |
| + t.Fatalf("hashing a file that fails after %d bytes reported no error", good) |
| + } |
| + if f.pos != 0 { |
| + t.Fatalf("after a failed hash (%d good bytes) the file is at offset %d, want 0", good, f.pos) |
| + } |
| + } |
| + if n := len(c.entries); n != 0 { |
| + t.Fatalf("%d tags remembered from hashes that failed", n) |
| + } |
| +} |