diff --git a/docs/internal/sources.md b/docs/internal/sources.md index a551341..27acd37 100644 --- a/docs/internal/sources.md +++ b/docs/internal/sources.md @@ -131,9 +131,10 @@ fallback. 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 +leaves unused capacity greater than the actual body length, the completed body +is copied to release the excess backing storage, including when the body is +empty. 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. diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index 5891b00..12ec2a1 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -1,7 +1,6 @@ # Implementation Review Remediation -Status: One computational-efficiency finding awaits remediation. Earlier -findings below remain resolved. +Status: All recorded findings have been resolved in separate remediation units. ## Computational Efficiency Review @@ -10,7 +9,16 @@ Review baseline: `8a5c0de`, compared with planning commit `3fde1f6`, the ### Finding 1 — Release A Stale File Reservation For An Empty Body (P3) -**Status: Open.** +**Status: Resolved.** Completed empty bodies now release stale file-size +reservations using the existing compaction path. The stale-size table covers +empty content, metadata, and hashes; the retained-artifact benchmark includes +an empty-body fixture without enforcing capacity or allocation thresholds. +Three offline, one-iteration samples for four retained empty artifacts changed +from 4,194,928–4,194,976 retained bytes to 624–640 bytes. Initial reservation +allocations still occur; this fix reduces retained storage, not cumulative +allocation. The nonempty benchmark remains near 1 MiB for four 256 KiB bodies. +Artifact package race tests and vet passed, and the internal documentation +now accurately describes the compaction condition. Location: [file reader](../../internal/artifact/reader.go), lines 137–166. diff --git a/internal/artifact/reader.go b/internal/artifact/reader.go index 7545cf5..d6c0acf 100644 --- a/internal/artifact/reader.go +++ b/internal/artifact/reader.go @@ -158,7 +158,7 @@ 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) { + if 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)) diff --git a/internal/artifact/reader_benchmark_test.go b/internal/artifact/reader_benchmark_test.go index 39fa7cd..b4190b4 100644 --- a/internal/artifact/reader_benchmark_test.go +++ b/internal/artifact/reader_benchmark_test.go @@ -42,7 +42,20 @@ func BenchmarkArtifactFileRead(b *testing.B) { // BenchmarkRetainedArtifactFileRead measures live artifact bodies when an // opened file reports a size larger than the content that reaches EOF. func BenchmarkRetainedArtifactFileRead(b *testing.B) { - body := bytes.Repeat([]byte("x"), 256<<10) + for _, fixture := range []struct { + name string + body []byte + }{ + {name: "small", body: bytes.Repeat([]byte("x"), 256<<10)}, + {name: "empty", body: []byte{}}, + } { + b.Run(fixture.name, func(b *testing.B) { + benchmarkRetainedArtifactFileRead(b, fixture.body) + }) + } +} + +func benchmarkRetainedArtifactFileRead(b *testing.B, body []byte) { path := filepath.Join(b.TempDir(), "overreported.txt") if err := os.WriteFile(path, body, 0o600); err != nil { b.Fatal(err) diff --git a/internal/artifact/reader_test.go b/internal/artifact/reader_test.go index 612ca11..a201bd6 100644 --- a/internal/artifact/reader_test.go +++ b/internal/artifact/reader_test.go @@ -241,6 +241,7 @@ func TestFileReaderUsesOpenedSizeOnlyAsACapacityHint(t *testing.T) { reportedSize int64 }{ {name: "overreported size", body: []byte("actual file content"), reportedSize: 128}, + {name: "overreported size with empty content", body: []byte{}, reportedSize: 16 << 20}, {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)}, } @@ -280,6 +281,9 @@ func TestFileReaderUsesOpenedSizeOnlyAsACapacityHint(t *testing.T) { if artifact.Size != int64(len(tc.body)) { t.Fatalf("artifact size=%d, want %d", artifact.Size, len(tc.body)) } + if artifact.Name != filepath.Base(filePath) || artifact.URI != filePath || artifact.ContentType == "" { + t.Fatalf("unexpected artifact metadata: name=%q URI=%q content type=%q", artifact.Name, artifact.URI, artifact.ContentType) + } wantHash := fmt.Sprintf("%x", sha256.Sum256(tc.body)) if artifact.Hash != wantHash { t.Fatalf("artifact hash=%q, want %q", artifact.Hash, wantHash)