Improve streaming, file caching and edge conditions by ganto · Pull Request #120 · ganto/pkgproxy

ganto · 2026-03-24T22:13:20Z

No description provided.

Copilot

Pull request overview

This PR updates pkgproxy’s cache-miss handling to stream upstream responses directly to disk (via temp files) rather than buffering entire bodies in memory, and adds edge-condition handling (client disconnects, truncated upstream responses) to avoid caching incomplete data.

Changes:

Add FileCache.CreateTempWriter / FileCache.CommitTempFile and refactor SaveToDisk to use them.
Rework Cache middleware to tee responses to a temp file using new resilientWriter/safeWriter and validate Content-Length before committing.
Add unit tests for the new cache primitives and streaming behaviors; update Makefile test args and changelog.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
pkg/pkgproxy/writers.go	Introduces `resilientWriter` (cache-side) and `safeWriter` (client-side) for best-effort streaming through `io.MultiWriter`.
pkg/pkgproxy/writers_test.go	Adds unit tests for `resilientWriter` / `safeWriter` behavior.
pkg/pkgproxy/proxy.go	Streams cache-miss bodies to temp files, forwards `Content-Length`, validates bytes before committing, and performs cleanup.
pkg/pkgproxy/bufferwriter.go	Prevents `Flush`/`WriteHeader` passthrough after client-side writer failure to avoid post-disconnect issues.
pkg/pkgproxy/proxy_test.go	Adds tests covering streaming cache-write edge conditions (non-200 cleanup, connection error, CL mismatch, missing CL, disk write failure).
pkg/cache/cache.go	Extends `FileCache` with temp-writer + atomic commit methods; refactors `SaveToDisk` to use them.
pkg/cache/cache_test.go	Adds unit tests for `CreateTempWriter`, `CommitTempFile`, and ensures `SaveToDisk` still works.
openspec/specs/streaming-cache-write/spec.md	Adds spec documenting the new streaming cache-write requirements and behaviors.
openspec/changes/archive/2026-03-24-stream-cache-writes/tasks.md	Captures the implementation checklist for the change set.
openspec/changes/archive/2026-03-24-stream-cache-writes/specs/streaming-cache-write/spec.md	Archives the spec snapshot for this change.
openspec/changes/archive/2026-03-24-stream-cache-writes/proposal.md	Records motivation and scope (memory spike reduction, truncation prevention).
openspec/changes/archive/2026-03-24-stream-cache-writes/design.md	Records design decisions and tradeoffs for streaming + validation approach.
openspec/changes/archive/2026-03-24-stream-cache-writes/.openspec.yaml	Adds archive metadata for the spec-driven change.
Makefile	Forces `go test` to run with `-count=1` to avoid cached results.
CHANGELOG.md	Documents the behavior change (streaming to disk, CL validation, caching despite client disconnect).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

pkg/pkgproxy/proxy_test.go

Copilot · 2026-03-24T22:18:26Z

pkg/pkgproxy/writers_test.go

+	// Use an invalid base path to trigger creation failure
+	fc := cache.New(&cache.CacheConfig{
+		BasePath:     "/nonexistent/path/that/cannot/exist",
+		FileSuffixes: []string{".rpm"},
+	})


Using a hard-coded absolute base path (/nonexistent/...) to force temp file creation failure is not deterministic across environments (e.g., running as root or different OS path semantics), which can make the test flaky. Prefer a deterministic failure mode using t.TempDir() with permissions removed or a base path that is a file (not a directory).

Copilot · 2026-03-24T22:18:26Z

pkg/cache/cache.go

+	// and renaming it to the final path for the given URI. Trusts that
+	// the URI was already validated by CreateTempWriter.


The interface doc says CommitTempFile “trusts that the URI was already validated by CreateTempWriter”, but the implementation currently re-validates via resolvedFilePath(uri). Update the comment to match the actual behavior (or adjust implementation if you truly want a trust/no-revalidation contract).

Suggested change

// and renaming it to the final path for the given URI. Trusts that

// the URI was already validated by CreateTempWriter.

// and renaming it to the final path for the given URI. Resolves and

// validates the URI/path before committing.

Copilot · 2026-03-24T22:18:26Z

pkg/cache/cache.go

+// given URI and sets the file modification time. It trusts that the URI was
+// already validated by CreateTempWriter.


This comment says CommitTempFile trusts that the URI was already validated, but CommitTempFile calls resolvedFilePath(uri) (which validates traversal) anyway. Align the comment with the implementation to avoid misleading future callers.

Suggested change

// given URI and sets the file modification time. It trusts that the URI was

// already validated by CreateTempWriter.

// given URI and sets the file modification time. It resolves the URI again via

// resolvedFilePath, so it can be called independently of CreateTempWriter.

Copilot · 2026-03-24T22:18:26Z

pkg/pkgproxy/writers_test.go

+	// Use an invalid base path to trigger failure on first write
+	fc := cache.New(&cache.CacheConfig{
+		BasePath:     "/nonexistent/path",
+		FileSuffixes: []string{".rpm"},
+	})


Same concern here: /nonexistent/path may be writable in some environments, making this failure-mode test non-deterministic. Consider constructing a guaranteed-unwritable cache base path under t.TempDir() by chmodding it read-only/no-access, or using a base path that points to a file.

pkg/pkgproxy/proxy.go

Replace the in-memory bytes.Buffer with a temp file writer that tees upstream responses directly to disk via io.MultiWriter. This eliminates memory spikes for large packages (kernel, firmware >100 MB). Key changes: - Add CreateTempWriter/CommitTempFile to FileCache interface for streaming writes with atomic commit - Refactor SaveToDisk to use the new primitives internally - Add resilientWriter (cache side) that lazily creates temp files and absorbs disk errors without affecting client responses - Add safeWriter (client side) that absorbs write errors from client disconnects, ensuring upstream body is fully consumed and cached - Validate Content-Length before committing to reject truncated responses - Client disconnects no longer prevent caching Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Assert the error explicitly: accept IsNotExist (directory never created) as success, otherwise require no error before checking entries. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Close the resilientWriter's temp file in the deferred cleanup before attempting removal. Previously, if next(c) returned an error after the temp file was created, the file descriptor would leak and removal could fail on some platforms. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

…t files A close error can indicate the temp file wasn't fully flushed, so committing after a failed close risks caching an incomplete file. Log the error and mark the writer as failed to skip the commit. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ganto requested a review from Copilot March 24, 2026 22:13

ganto added bug Something isn't working enhancement New feature or request go Pull requests that update Go code labels Mar 24, 2026

Copilot started reviewing on behalf of ganto March 24, 2026 22:14 View session

Copilot AI reviewed Mar 24, 2026

View reviewed changes

ganto and others added 7 commits March 24, 2026 23:51

openspec: Add stream-cache-writes change spec

dce4654

Makefile: Disable Go test caching by adding -count=1 to GO_TEST_ARGS

c18b2d8

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

openspec: Archive stream-cache-writes change spec

f2f80a7

test: Fix ignored os.ReadDir error in TestCacheConnectionErrorNoTempFile

fbd07ea

Assert the error explicitly: accept IsNotExist (directory never created) as success, otherwise require no error before checking entries. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ganto force-pushed the feature/stream-cache-writes branch from 1f16420 to c80ff0f Compare March 24, 2026 22:51

ganto merged commit cc4751b into main Mar 24, 2026
20 checks passed

ganto deleted the feature/stream-cache-writes branch March 24, 2026 22:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve streaming, file caching and edge conditions#120

Improve streaming, file caching and edge conditions#120
ganto merged 7 commits intomainfrom
feature/stream-cache-writes

ganto commented Mar 24, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI Mar 24, 2026

Uh oh!

Copilot AI Mar 24, 2026

Uh oh!

Copilot AI Mar 24, 2026

Uh oh!

Copilot AI Mar 24, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		// and renaming it to the final path for the given URI. Trusts that
		// the URI was already validated by CreateTempWriter.

		// given URI and sets the file modification time. It trusts that the URI was
		// already validated by CreateTempWriter.

Conversation

ganto commented Mar 24, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants