fix: keep mmap's reader contract on every platform

Three findings from the release review, all in the package's platform surface:

- An open mapping of an empty file was indistinguishable from a closed reader, because both were a
  nil slice, so ReadAt and Slice answered `mmap: closed` to a standard-library io.ReaderAt consumer
  that probed an empty source. The empty mapping is a non-nil empty slice now, which makes it read
  like an empty *os.File or bytes.Reader while a closed reader still reports itself as closed.
- The Linux prefetch passed `MADV_SEQUENTIAL|MADV_WILLNEED` as one advice. An madvise advice is a
  value, not a flag bit, so that expression is simply MADV_WILLNEED and the sequential hint was
  silently dropped. Each hint is its own call now, and the syscall is indirected so a Linux-only
  test can pin the advice sequence and the release-on-failure path that needed syscall injection
  before.
- The descriptor-only fallback installed no finalizer, so an abandoned reader leaked its descriptor
  where the mapping platforms release theirs. It installs the same finalizer, and its Close comment
  now describes what all platforms do.

AGENTS.md

@@ -82,8 +82,10 @@ entry point is `NewStream` with `Submit`/`Close`/`Wait`; `Run` and `BatchSource`
   read-only device, and a disk-usage refresh keeps the reservations of copies still in flight.
 - A file system without the managed signature attribute stores no cache at all, which is a
   no-op rather than a recorded failure.
-- `mmap` returns `io.EOF` at the end of the content, releases a mapping whose `madvise` failed,
-  closes idempotently on every platform, validates a slice range before allocating it, and retains
+- `mmap` returns `io.EOF` at the end of the content, reports an open mapping of an empty file as
+  empty rather than closed, releases a mapping whose `madvise` failed, issues the Linux sequential
+  hint and prefetch as one call each (an madvise advice is a value, not a flag bit), closes
+  idempotently on every platform, validates a slice range before allocating it, and retains
   the descriptor it opened: `Close` removes the mapping and then closes that descriptor.
 - One source descriptor serves a whole item. The stored hash is read through the descriptor that
   reads the content, and the computed hash is published through it, synchronously, after the hash

README.md

@@ -288,7 +288,7 @@ preallocated, what metadata can be restored, and how a mapped read is implemente
 
 | Platform | Source access time | Managed signature attribute | Target preallocation | Metadata restore | Mapped read |
 | --- | --- | --- | --- | --- | --- |
-| Linux | suppressed: a buffered source opens with `O_NOATIME`, falling back to an ordinary open when the flag is refused | `user.acp.signature`, in the `user.` namespace | `fallocate` reserves the whole target size | xattrs, mode, owner as root, times | `syscall.Mmap`, plus `MADV_SEQUENTIAL\|MADV_WILLNEED` for files up to 16 MiB |
+| Linux | suppressed: a buffered source opens with `O_NOATIME`, falling back to an ordinary open when the flag is refused | `user.acp.signature`, in the `user.` namespace | `fallocate` reserves the whole target size | xattrs, mode, owner as root, times | `syscall.Mmap`, plus `MADV_SEQUENTIAL` and `MADV_WILLNEED` (one advice per call) for files up to 16 MiB |
 | Darwin | not suppressed | `acp.signature` | `Truncate` | xattrs, mode, owner as root, times | `syscall.Mmap` |
 | FreeBSD | not suppressed | `acp.signature`, in the user extended-attribute namespace | `Truncate` | mode and times only | no mapping: the descriptor is read with `ReadAt` |
 | Windows | not suppressed | none: the cache is a silent no-op | `Truncate` | mode and times only | `CreateFileMapping` plus `MapViewOfFile` |
@@ -399,7 +399,8 @@ document keep their shape, and the shell fills one terminal row per item.
 ## Divergence from upstream `mmap`
 
 This module started from the Go project's `mmap` package and is maintained here now: reads run
-through the descriptor ACP owns, `Close` is idempotent and removes the mapping before it closes
+through the descriptor ACP owns, the end of the content is `io.EOF` and an open mapping of an empty
+file is empty rather than closed, `Close` is idempotent and removes the mapping before it closes
 that descriptor, a slice range is validated before anything is allocated, and a platform without
 a mapping reads through the descriptor instead. Upstream changes are not merged automatically.
 

mmap/mmap_darwin.go

@@ -128,8 +128,9 @@ func Open(filename string) (*ReaderAt, error) {
 		return nil, fmt.Errorf("mmap: file %q is too large", filename)
 	}
 
-	// An empty file has no mapping, but it still has the descriptor this reader owns.
-	var data []byte
+	// An empty file has no mapping, but it still has the descriptor this reader owns. The mapping
+	// of a zero-length file is an empty, non-nil slice, so only a closed reader reports a nil one.
+	data := make([]byte, 0)
 	if size != 0 {
 		data, err = syscall.Mmap(int(f.Fd()), 0, int(size), syscall.PROT_READ, syscall.MAP_SHARED)
 		if err != nil {

mmap/mmap_linux.go

@@ -21,6 +21,19 @@ const (
 	prefetchMaxSize = 16 * 1024 * 1024
 )
 
+// linuxPrefetchAdvice is the advice a file that fits the prefetch window receives. An madvise call
+// takes exactly one advice: these are values, not flag bits, so `MADV_SEQUENTIAL|MADV_WILLNEED` is
+// simply `MADV_WILLNEED` and the sequential hint would be dropped. The read is sequential and the
+// whole mapping should be faulted in up front, so both are issued, one per call.
+var linuxPrefetchAdvice = []int{syscall.MADV_SEQUENTIAL, syscall.MADV_WILLNEED}
+
+// madvise and munmap are indirected so a test can drive the prefetch failure path, which otherwise
+// needs syscall injection.
+var (
+	madvise = syscall.Madvise
+	munmap  = syscall.Munmap
+)
+
 // debug is whether to print debugging messages for manual testing.
 //
 // The runtime.SetFinalizer documentation says that, "The finalizer for x is
@@ -132,8 +145,9 @@ func Open(filename string) (*ReaderAt, error) {
 		return nil, fmt.Errorf("mmap: file %q is too large", filename)
 	}
 
-	// An empty file has no mapping, but it still has the descriptor this reader owns.
-	var data []byte
+	// An empty file has no mapping, but it still has the descriptor this reader owns. The mapping
+	// of a zero-length file is an empty, non-nil slice, so only a closed reader reports a nil one.
+	data := make([]byte, 0)
 	if size != 0 {
 		data, err = syscall.Mmap(int(f.Fd()), 0, int(size), syscall.PROT_READ, syscall.MAP_SHARED)
 		if err != nil {
@@ -141,12 +155,14 @@ func Open(filename string) (*ReaderAt, error) {
 			return nil, fmt.Errorf("create mmap fail, %q, %w", filename, err)
 		}
 		if size <= prefetchMaxSize {
-			if err := syscall.Madvise(data, syscall.MADV_SEQUENTIAL|syscall.MADV_WILLNEED); err != nil {
-				// The mapping is not reachable through a ReaderAt yet, so nothing else can release
-				// it: the error path owns the unmapping and the descriptor.
-				_ = syscall.Munmap(data)
-				_ = f.Close()
-				return nil, fmt.Errorf("madvise fail, %q, %w", filename, err)
+			for _, advice := range linuxPrefetchAdvice {
+				if err := madvise(data, advice); err != nil {
+					// The mapping is not reachable through a ReaderAt yet, so nothing else can
+					// release it: the error path owns the unmapping and the descriptor.
+					_ = munmap(data)
+					_ = f.Close()
+					return nil, fmt.Errorf("madvise fail, %q, %w", filename, err)
+				}
 			}
 		}
 	}

mmap/mmap_linux_test.go

@@ -0,0 +1,118 @@
+//go:build linux
+
+package mmap
+
+import (
+	"errors"
+	"os"
+	"syscall"
+	"testing"
+)
+
+// recordPrefetch replaces the madvise syscall with a spy that records every advice the prefetch
+// issues, so a test can pin the advice sequence and the error path without syscall injection.
+func recordPrefetch(t *testing.T, fail error) *[]int {
+	t.Helper()
+
+	previous := madvise
+	advices := new([]int)
+	madvise = func(_ []byte, advice int) error {
+		*advices = append(*advices, advice)
+		return fail
+	}
+	t.Cleanup(func() { madvise = previous })
+
+	return advices
+}
+
+// TestPrefetchIssuesEveryAdvice pins the Linux prefetch. An madvise call takes exactly one advice:
+// these are values, not flag bits, so passing `MADV_SEQUENTIAL|MADV_WILLNEED` keeps only the
+// prefetch and silently drops the sequential hint, which is why the two hints are two calls.
+func TestPrefetchIssuesEveryAdvice(t *testing.T) {
+	if syscall.MADV_SEQUENTIAL|syscall.MADV_WILLNEED != syscall.MADV_WILLNEED {
+		t.Fatalf("this test assumes the two constants are not flag bits on this platform")
+	}
+
+	advices := recordPrefetch(t, nil)
+	reader, err := Open(writeFixture(t, "prefetch.bin", []byte("0123456789")))
+	if err != nil {
+		t.Fatalf("Open: %v", err)
+	}
+	t.Cleanup(func() { _ = reader.Close() })
+
+	if len(*advices) != len(linuxPrefetchAdvice) {
+		t.Fatalf("prefetch issued advice %v, want %v", *advices, linuxPrefetchAdvice)
+	}
+	for index, want := range linuxPrefetchAdvice {
+		if (*advices)[index] != want {
+			t.Fatalf("prefetch issued advice %v, want %v", *advices, linuxPrefetchAdvice)
+		}
+	}
+}
+
+// TestPrefetchSkipsAFileAboveTheWindow pins the documented threshold: a mapping above
+// prefetchMaxSize receives no advice at all, so a large copy does not fault in its whole content.
+func TestPrefetchSkipsAFileAboveTheWindow(t *testing.T) {
+	advices := recordPrefetch(t, nil)
+
+	path := writeFixture(t, "large.bin", nil)
+	if err := os.Truncate(path, prefetchMaxSize+1); err != nil {
+		t.Fatalf("grow the fixture: %v", err)
+	}
+	reader, err := Open(path)
+	if err != nil {
+		t.Fatalf("Open: %v", err)
+	}
+	t.Cleanup(func() { _ = reader.Close() })
+
+	if len(*advices) != 0 {
+		t.Fatalf("a file above the prefetch window received advice %v", *advices)
+	}
+}
+
+// TestPrefetchFailureReleasesTheMapping pins the documented error path: a mapping whose madvise
+// failed is released before the error returns, and the descriptor opened for it is closed with it,
+// so a host that refuses the advice cannot leak either.
+func TestPrefetchFailureReleasesTheMapping(t *testing.T) {
+	failure := errors.New("madvise fixture")
+	recordPrefetch(t, failure)
+
+	previousUnmap := munmap
+	unmapped := 0
+	munmap = func(data []byte) error {
+		unmapped++
+		return previousUnmap(data)
+	}
+	t.Cleanup(func() { munmap = previousUnmap })
+
+	path := writeFixture(t, "advice-failure.bin", []byte("0123456789"))
+	before := descriptorCount(t)
+
+	const attempts = 8
+	for index := 0; index < attempts; index++ {
+		reader, err := Open(path)
+		if reader != nil || err == nil {
+			t.Fatalf("Open with a failing madvise = (%v, %v), want a failure", reader, err)
+		}
+		if !errors.Is(err, failure) {
+			t.Fatalf("Open error = %v, want %v", err, failure)
+		}
+	}
+	if unmapped != attempts {
+		t.Fatalf("released %d mappings, want one per failed open", unmapped)
+	}
+	if after := descriptorCount(t); after > before {
+		t.Fatalf("descriptors leaked: %d -> %d", before, after)
+	}
+}
+
+// descriptorCount counts this process's open descriptors.
+func descriptorCount(t *testing.T) int {
+	t.Helper()
+
+	entries, err := os.ReadDir("/proc/self/fd")
+	if err != nil {
+		t.Skipf("cannot count descriptors: %v", err)
+	}
+	return len(entries)
+}

mmap/mmap_other.go

@@ -12,6 +12,7 @@ import (
 	"errors"
 	"fmt"
 	"os"
+	"runtime"
 )
 
 // ReaderAt reads a memory-mapped file.
@@ -24,8 +25,8 @@ type ReaderAt struct {
 }
 
 // Close closes the reader. It is idempotent: a reader that is already closed reports success,
-// because the file it owned is already gone. A closed reader reports an empty mapping, exactly
-// like the mapping-backed platforms, so a reader that reads after Close sees io.EOF.
+// because the file it owned is already gone. A closed reader reports an empty mapping on every
+// platform, so Len() is zero and a stream over it ends with io.EOF instead of erroring.
 func (r *ReaderAt) Close() error {
 	if r.file == nil {
 		return nil
@@ -114,8 +115,13 @@ func Open(filename string) (*ReaderAt, error) {
 		return nil, fmt.Errorf("mmap: file %q is too large", filename)
 	}
 
-	return &ReaderAt{
+	r := &ReaderAt{
 		file: f,
 		len:  int(fi.Size()),
-	}, nil
+	}
+
+	// A reader that is abandoned without Close releases its descriptor on collection, exactly like
+	// the mapping-backed platforms, so this fallback cannot leak one either.
+	runtime.SetFinalizer(r, (*ReaderAt).Close)
+	return r, nil
 }

mmap/mmap_test.go

@@ -94,6 +94,41 @@ func TestReaderAtContract(t *testing.T) {
 	}
 }
 
+// TestOpenEmptyMappingIsNotEmpty pins the empty mapping against the closed-reader sentinel: an open
+// reader over a zero-length file is empty, not closed, so it behaves like an empty *os.File or
+// bytes.Reader. Reporting it as closed breaks every io.ReaderAt consumer that probes an empty
+// source, such as archive/zip.
+func TestOpenEmptyMappingIsNotClosed(t *testing.T) {
+	reader, err := Open(writeFixture(t, "empty-open.bin", nil))
+	if err != nil {
+		t.Fatalf("Open: %v", err)
+	}
+	if reader.Len() != 0 {
+		t.Fatalf("Len() = %d, want 0", reader.Len())
+	}
+
+	if n, err := reader.ReadAt(make([]byte, 4), 0); n != 0 || err != io.EOF {
+		t.Fatalf("ReadAt(4, 0) = (%d, %v), want (0, %v)", n, err, io.EOF)
+	}
+	if n, err := reader.ReadAt(nil, 0); n != 0 || err != nil {
+		t.Fatalf("ReadAt(nil, 0) = (%d, %v), want (0, nil)", n, err)
+	}
+	if window, err := reader.Slice(0, 4); err != nil || len(window) != 0 {
+		t.Fatalf("Slice(0, 4) = (%d bytes, %v), want an empty window", len(window), err)
+	}
+
+	// Only a closed reader reports itself as closed.
+	if err := reader.Close(); err != nil {
+		t.Fatalf("Close: %v", err)
+	}
+	if _, err := reader.ReadAt(make([]byte, 1), 0); err == nil || errors.Is(err, io.EOF) {
+		t.Fatalf("ReadAt after Close = %v, want a closed-reader error", err)
+	}
+	if _, err := reader.Slice(0, 1); err == nil || errors.Is(err, io.EOF) {
+		t.Fatalf("Slice after Close = %v, want a closed-reader error", err)
+	}
+}
+
 // TestOpenEmptyFile pins the empty mapping: it has no content, it closes cleanly, and a reader
 // over it reports the end of the content instead of a broken reader.
 func TestOpenEmptyFile(t *testing.T) {

mmap/mmap_windows.go

@@ -126,8 +126,9 @@ func Open(filename string) (*ReaderAt, error) {
 		return nil, fmt.Errorf("mmap: file %q is too large", filename)
 	}
 
-	// An empty file has no mapping, but it still has the descriptor this reader owns.
-	var data []byte
+	// An empty file has no mapping, but it still has the descriptor this reader owns. The mapping
+	// of a zero-length file is an empty, non-nil slice, so only a closed reader reports a nil one.
+	data := make([]byte, 0)
 	if size != 0 {
 		low, high := uint32(size), uint32(size>>32)
 		fmap, err := syscall.CreateFileMapping(syscall.Handle(f.Fd()), nil, syscall.PAGE_READONLY, high, low, nil)