accounting: fix memory leak on long-running rcd

Before this change, Transfer.Done closed the account of a completed transfer but
never released it, because it assigned nil to only a local copy of the pointer.
As a result, every completed transfer continued to reference its account.

An Account holds the transfer context and the source reader, and the stats keep
completed transfers around up to `MaxCompletedTransfers` per group, with groups
discarded only at --max-stats-groups. On a long-running `rclone rcd`, this adds up.

It was noticeable when running bisync repeatedly via the rc, where the transfer
context carries `b.WriteResults` (bisync's `LoggerFn`). A `*bisyncRun` holds
Path1 and Path2 listings, which can be quite large, and are not supposed to be
retained between runs. (Naturally they aren't, when running bisync on the
command line -- which is probably why we didn't notice this issue sooner.)

This change fixes the issue by releasing `tr.acc` in Done, instead of the local
copy. `tr.Snapshot` reads the byte counts off the account, so the progress is
recorded on the transfer first. That read happens before taking `tr.mu`, because
`acc.progress()` locks `acc.values.mu`, `checkReadBefore` holds that lock while
calling `StatsInfo.GetBytes`, and `StatsInfo` locks back into `Transfer` in
`Transferred` and `_removeTransfer`.

fs/accounting/transfer.go

@@ -66,6 +66,7 @@ type Transfer struct {
 	acc         *Account
 	err         error
 	completedAt time.Time
+	doneBytes   int64
 }
 
 // newCheckingTransfer instantiates new checking of the object.
@@ -116,12 +117,20 @@ func (tr *Transfer) Done(ctx context.Context, err error) {
 		}
 		// Signal done with accounting
 		acc.Done()
-		// free the account since we may keep the transfer
+	}
-		acc = nil
+
+	var doneBytes int64
+	if acc != nil {
+		doneBytes, _ = acc.progress()
 	}
 
 	tr.mu.Lock()
 	tr.completedAt = time.Now()
+	if acc != nil {
+		tr.doneBytes = doneBytes
+	}
+	// free the account since we may keep the transfer
+	tr.acc = nil
 	tr.mu.Unlock()
 
 	if tr.checking {
@@ -181,7 +190,7 @@ func (tr *Transfer) Snapshot() TransferSnapshot {
 	tr.mu.RLock()
 	defer tr.mu.RUnlock()
 
-	var s, b int64 = tr.size, 0
+	b, s := tr.doneBytes, tr.size
 	if tr.acc != nil {
 		b, s = tr.acc.progress()
 	}

fs/accounting/transfer_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"io"
+	"strings"
 	"testing"
 
 	"github.com/rclone/rclone/fs/rc"
@@ -56,6 +57,26 @@ func TestTransfer(t *testing.T) {
 		assert.Equal(t, "dstFs:dstFs", snap.DstFs)
 	})
 
+	t.Run("DoneReleasesAccount", func(t *testing.T) {
+		content := "hello world"
+		o := mockobject.New("obj").WithContent([]byte(content), mockobject.SeekModeNone)
+		tr := newTransfer(s, o, srcFs, dstFs)
+		in := tr.Account(ctx, io.NopCloser(strings.NewReader(content)))
+		_, err := io.Copy(io.Discard, in)
+		require.NoError(t, err)
+
+		tr.Done(ctx, nil)
+
+		tr.mu.RLock()
+		acc := tr.acc
+		tr.mu.RUnlock()
+		assert.Nil(t, acc)
+
+		snap := tr.Snapshot()
+		assert.Equal(t, int64(len(content)), snap.Bytes)
+		assert.Equal(t, int64(len(content)), snap.Size)
+	})
+
 	t.Run("rcStats", func(t *testing.T) {
 		out := tr.rcStats()
 		assert.Equal(t, rc.Params{