From f1e1a45a05a14bcc7889827c1615a7036a2ebcfc Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Fri, 9 Oct 2026 00:23:27 +0000 Subject: [PATCH] Reserve capacity for regular file artifacts --- docs/internal/sources.md | 12 ++++-- docs/roadmap/implementation.md | 63 +++++++++++++++++++++++++++++++- internal/artifact/reader.go | 14 ++++++- internal/artifact/reader_test.go | 63 ++++++++++++++++++++++++++++++++ 4 files changed, 146 insertions(+), 6 deletions(-) diff --git a/docs/internal/sources.md b/docs/internal/sources.md index 69204cc..56e32ae 100644 --- a/docs/internal/sources.md +++ b/docs/internal/sources.md @@ -116,10 +116,14 @@ and after opening the file. It copies content into an artifact, records metadata and an opaque content-equality value, and applies a content-type fallback. -Regular files are read synchronously in bounded chunks. Cancellation is -checked before opening, before and after every read, and before publishing the -artifact, so a canceled read never publishes partial content. The ordinary -reader does not detach file reads into background goroutines. +Regular files are read synchronously in bounded chunks. The opened file's size +is only an initial-capacity hint, capped at 1 MiB; reads continue to EOF, and +the returned body determines artifact size and hash. If an overreported hint +leaves more than twice the actual body length unused, the completed body is +copied to release the excess backing storage. Cancellation is checked before +opening, before and after every read, and before publishing the artifact, so a +canceled read never publishes partial content. The ordinary reader does not +detach file reads into background goroutines. This ordinary reader does not implement an inbound HTTP security boundary. In particular, it does not constrain files to an application root or impose an diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index f3ef4aa..0788461 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -2,7 +2,7 @@ Implement the remaining local work in the [performance roadmap](performance.md) in order. Each sequential stage is scoped for one Luna coding-agent prompt. -Stages 1–3 are complete; Stages 4–14 remain pending. Update status and evidence +Stages 1–4 are complete; Stages 5–14 remain pending. Update status and evidence as each work unit lands. The completed inspection plan is preserved at `481e2d4:docs/roadmap/implementation.md` in Git history. @@ -178,6 +178,12 @@ without `-race`. Different request inputs must remain independent. ## Stage 4 — Reserve A Useful Initial File-Read Capacity +**Status: Complete (2026-10-09).** File reads now use the opened regular +file's size only as a capped initial-capacity hint, continue reading to EOF, +and compact a completed body when a stale hint leaves disproportionate unused +capacity. The exact-size and overreported-size benchmark comparisons are +recorded below. + **Outcome:** Ordinary file reads avoid early geometric buffer growth without trusting metadata as the amount of content to return. @@ -683,6 +689,61 @@ including exact message/session output, cancellation, artifact-text lifecycle, and a new concurrent-render case that verifies independent request variables, artifact text, and session IDs against shared compiled templates. +### Artifact File-Read Capacity Comparison + +The benchmark-only fixture was committed as `996295d` (`Add artifact file-read +benchmarks`) before the reader change. Before measurements use that revision; +after measurements use the same fixture after the bounded reservation and +post-read compaction were implemented. The real-file cases contain 4 KiB, +1 MiB, and 4 MiB of repeated `x` bytes. The retained case reads a real 256 KiB +file whose opened-file seam reports 16 MiB, retaining four results. Fixture +creation and MIME lookup are outside timing. Environment: Linux/amd64, Intel +Core i5-7260U @ 2.20 GHz, benchmark parallelism 4, Go 1.26.8, ConfigKit +`v0.5.0`, JSON Schema `v6.0.2`, and backend modules `v3.20260930.0`. + +Command for both runs (five 500 ms samples per benchmark, without the race +detector): + +```sh +GOTOOLCHAIN=local GOWORK=off GOPROXY=off GONOPROXY=none GOSUMDB=off \ + go test -mod=readonly -run '^$' \ + -bench '^(BenchmarkArtifactFileRead|BenchmarkRetainedArtifactFileRead)$' \ + -benchmem -benchtime=500ms -count=5 ./internal/artifact +``` + +Times list all five `ns/op` samples in command order, followed by median and +range. Allocation figures are `B/op; allocs/op`. + +| File-read case | Before `ns/op` (median; range) | Before allocations | After `ns/op` (median; range) | After allocations | +| --- | --- | --- | --- | --- | +| 4 KiB | 37904, 38759, 37073, 36437, 36930 (37073; 36437–38759) | 70513–70514; 12 | 37222, 37414, 36521, 37867, 39988 (37414; 36521–39988) | 70513; 12 | +| 1 MiB | 3576064, 4010278, 3919714, 3918643, 3844238 (3918643; 3576064–4010278) | 4752589–4752677; 23–24 | 3105775, 3055909, 3160450, 3268202, 3079892 (3105775; 3055909–3268202) | 1115066–1115090; 12 | +| 4 MiB | 16818098, 15977605, 16465923, 15753739, 15589001 (15977605; 15589001–16818098) | 20244106–20244130; 33 | 17060165, 16462547, 15995322, 17206316, 16180077 (16462547; 15995322–17206316) | 21177936–21178008; 24–25 | + +The 1 MiB case reduced cumulative allocation from about 4.75 MB to 1.12 MB +and from 23–24 to 12 allocations, with a lower median time. For 4 MiB, the +reservation reduced allocation count from 33 to 24–25 but increased cumulative +allocated bytes by about 0.93 MB; timing ranges overlap. The small-file case +was effectively unchanged. These results support the specified capped hint as +a way to avoid repeated growth for ordinary 1 MiB reads, while showing that +the 1 MiB initial reserve is not a cumulative-byte win for every larger file. + +| Overreported-size retained workload | Before | After | +| --- | --- | --- | +| `ns/op` (median; range) | 1144696, 1073206, 1060135, 1059339, 1073194 (1073194; 1059339–1144696) | 1188822, 1201670, 1199592, 1175553, 1173590 (1188822; 1173590–1201670) | +| `B/op; allocs/op` | 779427–779463; 18 | 1377557–1377574; 17 | +| Retained heap (`retained-B`, median; range) | 1082272, 1082496, 1082944, 1082384, 1082496 (1082496; 1082272–1082944) | 1049728, 1049392, 1049280, 1049504, 1049168 (1049392; 1049168–1049728) | + +For the stale 16 MiB hint and 256 KiB actual body, the bounded reserve adds +about 0.6 MB of cumulative allocation and roughly 0.1 ms to the read, but the +post-read compaction prevents it from increasing live retained storage; the +four-result retained-heap median is about 33 KiB lower than the baseline. This +is intentionally a stale-metadata path, not a normal exact-size read. The +ordinary reader still derives returned bytes, size, and hash from content read +through EOF. Focused verification passed: `go test -mod=readonly +./internal/artifact`; the existing cancellation and nonregular-file checks +remain in the passing suite. + ## Open Questions No consumer-policy decisions remain open. File reservation and encoding error diff --git a/internal/artifact/reader.go b/internal/artifact/reader.go index 2486b9e..7545cf5 100644 --- a/internal/artifact/reader.go +++ b/internal/artifact/reader.go @@ -21,6 +21,7 @@ var ( ) const fileReadChunkSize = 64 * 1024 +const fileReadInitialCapacityLimit = 1 << 20 // Reader resolves artifact references into actual artifacts. type Reader interface { @@ -133,7 +134,11 @@ func readFileArtifact(ctx context.Context, path string, open func(string) (artif return nil, fmt.Errorf("%w: %s", ErrUnsupportedFile, path) } - data := make([]byte, 0) + capacity := 0 + if size := openedInfo.Size(); size > 0 { + capacity = int(min(size, int64(fileReadInitialCapacityLimit))) + } + data := make([]byte, 0, capacity) chunk := make([]byte, fileReadChunkSize) for { if err := ctx.Err(); err != nil { @@ -153,6 +158,13 @@ func readFileArtifact(ctx context.Context, path string, open func(string) (artif return nil, fmt.Errorf("failed to read file %s: %w", path, readErr) } } + if len(data) > 0 && cap(data)-len(data) > len(data) { + // A stale size hint should not make a small completed artifact retain a + // disproportionately large backing array. + compact := make([]byte, len(data)) + copy(compact, data) + data = compact + } contentType := mime.TypeByExtension(filepath.Ext(path)) if contentType == "" { diff --git a/internal/artifact/reader_test.go b/internal/artifact/reader_test.go index f47ba28..612ca11 100644 --- a/internal/artifact/reader_test.go +++ b/internal/artifact/reader_test.go @@ -3,7 +3,9 @@ package artifact import ( "bytes" "context" + "crypto/sha256" "errors" + "fmt" "os" "path/filepath" "strings" @@ -232,6 +234,60 @@ func TestFileReaderFailuresAndMetadata(t *testing.T) { }) } +func TestFileReaderUsesOpenedSizeOnlyAsACapacityHint(t *testing.T) { + tests := []struct { + name string + body []byte + reportedSize int64 + }{ + {name: "overreported size", body: []byte("actual file content"), reportedSize: 128}, + {name: "underreported size", body: bytes.Repeat([]byte("x"), fileReadChunkSize+17), reportedSize: 1}, + {name: "very large hint with small content", body: []byte("small actual content"), reportedSize: int64(^uint64(0) >> 1)}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + filePath := filepath.Join(t.TempDir(), "artifact.txt") + if err := os.WriteFile(filePath, tc.body, 0o600); err != nil { + t.Fatal(err) + } + reader := &fileReader{open: func(path string) (artifactFile, error) { + file, err := os.Open(path) + if err != nil { + return nil, err + } + info, err := file.Stat() + if err != nil { + file.Close() + return nil, err + } + return &reportedInfoFile{ + artifactFile: file, + info: reportedFileSizeInfo{FileInfo: info, size: tc.reportedSize}, + }, nil + }} + + artifact, err := reader.Read(context.Background(), domain.ArtifactRef{ + Type: domain.ArtifactRefFile, + URI: filePath, + }) + if err != nil { + t.Fatalf("read artifact: %v", err) + } + if !bytes.Equal(artifact.Body, tc.body) { + t.Fatalf("artifact body length=%d, want %d", len(artifact.Body), len(tc.body)) + } + if artifact.Size != int64(len(tc.body)) { + t.Fatalf("artifact size=%d, want %d", artifact.Size, len(tc.body)) + } + wantHash := fmt.Sprintf("%x", sha256.Sum256(tc.body)) + if artifact.Hash != wantHash { + t.Fatalf("artifact hash=%q, want %q", artifact.Hash, wantHash) + } + }) + } +} + func TestFileReaderCancelsAfterReadProgress(t *testing.T) { filePath := filepath.Join(t.TempDir(), "artifact.bin") content := bytes.Repeat([]byte("x"), fileReadChunkSize*2) @@ -268,6 +324,13 @@ func (f *reportedInfoFile) Stat() (os.FileInfo, error) { return f.info, nil } +type reportedFileSizeInfo struct { + os.FileInfo + size int64 +} + +func (info reportedFileSizeInfo) Size() int64 { return info.size } + type cancelAfterProgressFile struct { artifactFile cancel context.CancelFunc