vfscache: fix log message growing without bound on repeated write errors

Write() overwrote a successful write's nil error with the stale
lastErr returned by kickWaiters() once the downloader had recorded
too many errors. download() then wrapped that stale error again and
stored it back as the new lastErr, so every subsequent write added
another "vfs reader: failed to write to cache file:" prefix - fixes #4998

vfs/vfscache/downloaders/downloaders.go

@@ -457,15 +457,18 @@ func (dl *downloader) Write(p []byte) (n int, err error) {
 	// defer log.Trace(dl.dls.src, "p_len=%d", len(p))("n=%d, err=%v", &n, &err)
 
 	// Kick the waiters on exit if some characters received
+	//
+	// Only logs kickWaiters failures rather than returning them as this
+	// Write's own error: kickWaiters can return the downloaders' stale
+	// accumulated lastErr, which would otherwise fail this write (even
+	// when it succeeded) and get wrapped again by download(), growing
+	// unboundedly on every subsequent call while lastErr stays set.
 	defer func() {
 		if n <= 0 {
 			return
 		}
 		if waitErr := dl.dls.kickWaiters(); waitErr != nil {
 			fs.Errorf(dl.dls.src, "vfs cache: download write: failed to kick waiters: %v", waitErr)
-			if err == nil {
-				err = waitErr
-			}
 		}
 	}()
 

vfs/vfscache/downloaders/downloaders_test.go

@@ -2,6 +2,8 @@ package downloaders
 
 import (
 	"context"
+	"errors"
+	"fmt"
 	"io"
 	"sync"
 	"testing"
@@ -151,4 +153,43 @@ func TestDownloaders(t *testing.T) {
 			t.Fatal("Download did not return: the waiter was never dispatched")
 		}
 	})
+
+	// A successful write must not be turned into a failure by a stale
+	// error left over from a previous problem (e.g. the cache running
+	// out of space earlier). Regression test: this used to make Write
+	// return dls.lastErr even when the write itself succeeded, which
+	// then got wrapped again by download() and stored back as the new
+	// lastErr - causing the wrapped error message to grow without bound
+	// on every subsequent write.
+	t.Run("WriteDoesNotFailOnStaleError", func(t *testing.T) {
+		item, dls := newTest()
+		defer cancel(dls)
+
+		dls.mu.Lock()
+		dls.errorCount = maxErrorCount + 1
+		dls.lastErr = fmt.Errorf("vfs reader: failed to write to cache file: %w", errors.New("no space left on device"))
+		// A waiter for a range that isn't downloaded yet, so kickWaiters
+		// doesn't dispatch it immediately and reaches the errorCount check.
+		dls.waiters = append(dls.waiters, waiter{
+			r:       ranges.Range{Pos: 10 * 1024 * 1024, Size: 250},
+			errChan: make(chan error, 1),
+		})
+		dls.mu.Unlock()
+
+		dl := &downloader{
+			dls:       dls,
+			quit:      make(chan struct{}),
+			kick:      make(chan struct{}, 1),
+			offset:    0,
+			maxOffset: 1024,
+		}
+
+		p := make([]byte, 16)
+		_, err := io.ReadFull(readers.NewPatternReader(item.size), p)
+		require.NoError(t, err)
+
+		n, err := dl.Write(p)
+		require.NoError(t, err, "a successful write must not fail due to unrelated stale state")
+		assert.Equal(t, len(p), n)
+	})
 }