harden cache publication across platforms

Keep Windows rename retries bounded and platform-specific, simplify response serialization and cleanup, and cover cache publication through public-client and Windows filesystem scenarios.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 6e0a2bb3-4485-45bd-bcbc-f426f9edbb7c

Author: williammartin

pkg/api/cache.go

@@ -123,6 +123,9 @@ func (crt cacheRoundTripper) RoundTrip(req *http.Request) (*http.Response, error
 // Allow an individual request to override cache options.
 func requestCacheOptions(req *http.Request) (string, time.Duration) {
 	var dur time.Duration
+	// Added alongside the TTL header in https://github.com/cli/go-gh/pull/49.
+	// No production consumer of the directory override is known; retain it for
+	// compatibility.
 	dir := req.Header.Get("X-GH-CACHE-DIR")
 	ttl := req.Header.Get("X-GH-CACHE-TTL")
 	if ttl != "" {
@@ -170,62 +173,91 @@ func (fs *fileStorage) read(key string) (*http.Response, error) {
 	return res, err
 }
 
-func (fs *fileStorage) store(key string, res *http.Response) (storeErr error) {
-	cacheFile := fs.filePath(key)
-
+func (fs *fileStorage) store(key string, res *http.Response) error {
 	fs.mu.Lock()
 	defer fs.mu.Unlock()
 
-	dir := filepath.Dir(cacheFile)
-	if storeErr = os.MkdirAll(dir, 0755); storeErr != nil {
-		return
+	cacheFilePath := fs.filePath(key)
+	dir := filepath.Dir(cacheFilePath)
+	if err := os.MkdirAll(dir, 0755); err != nil {
+		return err
 	}
 
-	// Write to a temporary file in the same directory, then rename into place
-	// so concurrent processes and partial writes cannot corrupt the cache entry.
-	var f *os.File
-	if f, storeErr = os.CreateTemp(dir, ".gh-cache-*"); storeErr != nil {
-		return
+	// Finish writing before publishing the entry. Same-directory rename gives
+	// atomic replacement on Unix; it is best-effort on other platforms.
+	tmpCacheFile, err := os.CreateTemp(dir, ".gh-cache-*")
+	if err != nil {
+		return err
 	}
-	tmpName := f.Name()
+	tmpCacheFileName := tmpCacheFile.Name()
+	// Clean up on errors and panics too. After a successful rename, the
+	// temporary path no longer exists, so removing it is harmless.
 	defer func() {
-		if storeErr != nil {
-			_ = os.Remove(tmpName)
-		}
+		_ = tmpCacheFile.Close()
+		_ = os.Remove(tmpCacheFileName)
 	}()
 
-	var origBody io.ReadCloser
-	if res.Body != nil {
-		origBody, res.Body = copyStream(res.Body)
-		defer res.Body.Close()
+	if err := writeCacheResponse(tmpCacheFile, res); err != nil {
+		return err
 	}
-
-	storeErr = res.Write(f)
-	if origBody != nil {
-		res.Body = origBody
+	if err := tmpCacheFile.Close(); err != nil {
+		return err
 	}
 
-	if cerr := f.Close(); storeErr == nil && cerr != nil {
-		storeErr = cerr
-	}
-	if storeErr != nil {
-		return
+	return renameCacheFile(tmpCacheFileName, cacheFilePath)
+}
+
+func writeCacheResponse(w io.Writer, res *http.Response) error {
+	if res.Body == nil {
+		// Serialize the HTTP response headers only, since there is no body.
+		return res.Write(w)
 	}
 
-	if err := os.Chmod(tmpName, 0600); err != nil {
-		storeErr = err
-		return
+	// Buffer the bytes consumed during serialization so the caller can replay
+	// them. Restore the replay reader even if writing fails or panics.
+	buffer := &bytes.Buffer{}
+	recorder := &errorRecordingReader{Reader: io.TeeReader(res.Body, buffer)}
+	source := &readCloser{Reader: recorder, Closer: res.Body}
+	res.Body = source
+	defer source.Close()
+	defer func() {
+		res.Body = io.NopCloser(&errorReplayingReader{Reader: buffer, err: recorder.err})
+	}()
+
+	return res.Write(w)
+}
+
+type errorRecordingReader struct {
+	io.Reader
+	err error
+}
+
+func (r *errorRecordingReader) Read(p []byte) (int, error) {
+	n, err := r.Reader.Read(p)
+	if err != nil && err != io.EOF {
+		r.err = err
 	}
+	return n, err
+}
 
-	storeErr = os.Rename(tmpName, cacheFile)
-	return
+type errorReplayingReader struct {
+	io.Reader
+	err error
+}
+
+func (r *errorReplayingReader) Read(p []byte) (int, error) {
+	n, err := r.Reader.Read(p)
+	if err == io.EOF && r.err != nil {
+		err = r.err
+		r.err = nil
+	}
+	return n, err
 }
 
-func copyStream(r io.ReadCloser) (io.ReadCloser, io.ReadCloser) {
-	b := &bytes.Buffer{}
-	nr := io.TeeReader(r, b)
-	return io.NopCloser(b), &readCloser{
-		Reader: nr,
-		Closer: r,
+func copyStream(body io.ReadCloser) (replay, source io.ReadCloser) {
+	buffer := &bytes.Buffer{}
+	return io.NopCloser(buffer), &readCloser{
+		Reader: io.TeeReader(body, buffer),
+		Closer: body,
 	}
 }

pkg/api/cache_rename_other.go

@@ -0,0 +1,9 @@
+//go:build !windows
+
+package api
+
+import "os"
+
+func renameCacheFile(oldPath, newPath string) error {
+	return os.Rename(oldPath, newPath)
+}

pkg/api/cache_rename_windows.go

@@ -0,0 +1,45 @@
+package api
+
+import (
+	"errors"
+	"os"
+	"time"
+
+	"golang.org/x/sys/windows"
+)
+
+func renameCacheFile(oldPath string, newPath string) error {
+	return retryCacheRename(func() (error, bool) {
+		err := os.Rename(oldPath, newPath)
+		return err, isRetryableCacheRenameError(err)
+	})
+}
+
+type retryableFn func() (err error, retryable bool)
+
+func retryCacheRename(rename retryableFn) error {
+	// Caching is best-effort: allow brief Windows sharing conflicts to clear
+	// without holding up an API response indefinitely.
+	deadline := time.Now().Add(100 * time.Millisecond)
+	for delay := time.Millisecond; ; delay *= 2 {
+		err, retryable := rename()
+		if err == nil {
+			return nil
+		}
+
+		if !retryable {
+			return err
+		}
+
+		remaining := time.Until(deadline)
+		if remaining <= 0 {
+			return err
+		}
+		time.Sleep(min(delay, remaining))
+	}
+}
+
+func isRetryableCacheRenameError(err error) bool {
+	return errors.Is(err, windows.ERROR_SHARING_VIOLATION) ||
+		errors.Is(err, windows.ERROR_ACCESS_DENIED)
+}

pkg/api/cache_rename_windows_test.go

@@ -0,0 +1,94 @@
+package api
+
+import (
+	"errors"
+	"os"
+	"path/filepath"
+	"testing"
+	"testing/synctest"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"golang.org/x/sys/windows"
+)
+
+func TestCacheRenameStopsRetryingAfterBudget(t *testing.T) {
+	t.Parallel()
+
+	// Given a destination whose reader prevents replacement for the entire retry budget.
+	dir := t.TempDir()
+	oldPath, newPath := filepath.Join(dir, "temp"), filepath.Join(dir, "cache")
+	require.NoError(t, os.WriteFile(oldPath, []byte("replacement"), 0600))
+	require.NoError(t, os.WriteFile(newPath, []byte("previous"), 0600))
+	reader, err := os.Open(newPath)
+	require.NoError(t, err)
+	defer reader.Close()
+
+	synctest.Test(t, func(t *testing.T) {
+		start := time.Now()
+
+		// When publication is attempted while the reader remains open.
+		err := renameCacheFile(oldPath, newPath)
+
+		// Then it returns the sharing error within budget
+		require.Error(t, err)
+		assert.True(t, errors.Is(err, windows.ERROR_SHARING_VIOLATION) || errors.Is(err, windows.ERROR_ACCESS_DENIED),
+			"expected a Windows sharing conflict, got %v", err)
+		elapsed := time.Since(start)
+		assert.Greater(t, elapsed, time.Duration(0), "expected retries before giving up")
+		assert.LessOrEqual(t, elapsed, 100*time.Millisecond, "cache retries must stay within the latency budget")
+
+		// And the contents of both files are unchanged.
+		previous, err := os.ReadFile(newPath)
+		require.NoError(t, err)
+		assert.Equal(t, "previous", string(previous))
+		replacement, err := os.ReadFile(oldPath)
+		require.NoError(t, err)
+		assert.Equal(t, "replacement", string(replacement))
+	})
+}
+
+func TestCacheRenameWaitsForWindowsReader(t *testing.T) {
+	t.Parallel()
+
+	// Given a destination held open without delete sharing by another reader.
+	dir := t.TempDir()
+	oldPath, newPath := filepath.Join(dir, "temp"), filepath.Join(dir, "cache")
+	require.NoError(t, os.WriteFile(oldPath, []byte("replacement"), 0600))
+	require.NoError(t, os.WriteFile(newPath, []byte("previous"), 0600))
+	reader, err := os.Open(newPath)
+	require.NoError(t, err)
+	defer reader.Close()
+
+	synctest.Test(t, func(t *testing.T) {
+		// When the reader closes while the rename is waiting to retry.
+		done := make(chan error, 1)
+		go func() { done <- renameCacheFile(oldPath, newPath) }()
+		synctest.Wait()
+		require.NoError(t, reader.Close())
+
+		// Then the replacement succeeds.
+		require.NoError(t, <-done)
+		data, err := os.ReadFile(newPath)
+		require.NoError(t, err)
+		assert.Equal(t, "replacement", string(data))
+	})
+}
+
+func TestCacheRenameReturnsMissingSourceImmediately(t *testing.T) {
+	t.Parallel()
+
+	// Given a source file that does not exist.
+	dir := t.TempDir()
+	synctest.Test(t, func(t *testing.T) {
+		start := time.Now()
+
+		// When it is renamed.
+		err := renameCacheFile(filepath.Join(dir, "missing"), filepath.Join(dir, "cache"))
+
+		// Then the non-transient Windows error is returned without retrying.
+		require.ErrorIs(t, err, os.ErrNotExist)
+		assert.Zero(t, time.Since(start))
+	})
+}