diff --git a/vfs/vfscache/downloaders/downloaders.go b/vfs/vfscache/downloaders/downloaders.go index 39cd4e883..6e37adfcb 100644 --- a/vfs/vfscache/downloaders/downloaders.go +++ b/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 - } } }() diff --git a/vfs/vfscache/downloaders/downloaders_test.go b/vfs/vfscache/downloaders/downloaders_test.go index 592c3f470..7ad626927 100644 --- a/vfs/vfscache/downloaders/downloaders_test.go +++ b/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) + }) }