accounting: fix global error acounting

fs.CountError is called when an error is encountered. The method was
calling GlobalStats().Error(err) which incremented the error at the
global stats level. This led to calls to core/stats with group= filter
returning an error count of 0 even if errors actually occured.

This change requires the context to be provided when calling
fs.CountError. Doing so, we can retrieve the correct StatsInfo to
increment the errors from.

Fixes rclone#5865

Author: benjaminlegrand-seagate

Diff for: cmd/cmd.go

@@ -83,12 +83,13 @@ func ShowVersion() {
 // It returns a string with the file name if points to a file
 // otherwise "".
 func NewFsFile(remote string) (fs.Fs, string) {
+	ctx := context.Background()
 	_, fsPath, err := fspath.SplitFs(remote)
 	if err != nil {
-		err = fs.CountError(err)
+		err = fs.CountError(ctx, err)
 		fs.Fatalf(nil, "Failed to create file system for %q: %v", remote, err)
 	}
-	f, err := cache.Get(context.Background(), remote)
+	f, err := cache.Get(ctx, remote)
 	switch err {
 	case fs.ErrorIsFile:
 		cache.Pin(f) // pin indefinitely since it was on the CLI
@@ -97,7 +98,7 @@ func NewFsFile(remote string) (fs.Fs, string) {
 		cache.Pin(f) // pin indefinitely since it was on the CLI
 		return f, ""
 	default:
-		err = fs.CountError(err)
+		err = fs.CountError(ctx, err)
 		fs.Fatalf(nil, "Failed to create file system for %q: %v", remote, err)
 	}
 	return nil, ""
@@ -108,18 +109,19 @@ func NewFsFile(remote string) (fs.Fs, string) {
 // This works the same as NewFsFile however it adds filters to the Fs
 // to limit it to a single file if the remote pointed to a file.
 func newFsFileAddFilter(remote string) (fs.Fs, string) {
-	fi := filter.GetConfig(context.Background())
+	ctx := context.Background()
+	fi := filter.GetConfig(ctx)
 	f, fileName := NewFsFile(remote)
 	if fileName != "" {
 		if !fi.InActive() {
 			err := fmt.Errorf("can't limit to single files when using filters: %v", remote)
-			err = fs.CountError(err)
+			err = fs.CountError(ctx, err)
 			fs.Fatal(nil, err.Error())
 		}
 		// Limit transfers to this file
 		err := fi.AddFile(fileName)
 		if err != nil {
-			err = fs.CountError(err)
+			err = fs.CountError(ctx, err)
 			fs.Fatalf(nil, "Failed to limit to single file %q: %v", remote, err)
 		}
 	}
@@ -139,9 +141,10 @@ func NewFsSrc(args []string) fs.Fs {
 //
 // This must point to a directory
 func newFsDir(remote string) fs.Fs {
-	f, err := cache.Get(context.Background(), remote)
+	ctx := context.Background()
+	f, err := cache.Get(ctx, remote)
 	if err != nil {
-		err = fs.CountError(err)
+		err = fs.CountError(ctx, err)
 		fs.Fatalf(nil, "Failed to create file system for %q: %v", remote, err)
 	}
 	cache.Pin(f) // pin indefinitely since it was on the CLI
@@ -175,6 +178,7 @@ func NewFsSrcFileDst(args []string) (fsrc fs.Fs, srcFileName string, fdst fs.Fs)
 // NewFsSrcDstFiles creates a new src and dst fs from the arguments
 // If src is a file then srcFileName and dstFileName will be non-empty
 func NewFsSrcDstFiles(args []string) (fsrc fs.Fs, srcFileName string, fdst fs.Fs, dstFileName string) {
+	ctx := context.Background()
 	fsrc, srcFileName = newFsFileAddFilter(args[0])
 	// If copying a file...
 	dstRemote := args[1]
@@ -193,14 +197,14 @@ func NewFsSrcDstFiles(args []string) (fsrc fs.Fs, srcFileName string, fdst fs.Fs
 			fs.Fatalf(nil, "%q is a directory", args[1])
 		}
 	}
-	fdst, err := cache.Get(context.Background(), dstRemote)
+	fdst, err := cache.Get(ctx, dstRemote)
 	switch err {
 	case fs.ErrorIsFile:
-		_ = fs.CountError(err)
+		_ = fs.CountError(ctx, err)
 		fs.Fatalf(nil, "Source doesn't exist or is a directory and destination is a file")
 	case nil:
 	default:
-		_ = fs.CountError(err)
+		_ = fs.CountError(ctx, err)
 		fs.Fatalf(nil, "Failed to create file system for destination %q: %v", dstRemote, err)
 	}
 	cache.Pin(fdst) // pin indefinitely since it was on the CLI
@@ -234,7 +238,8 @@ func ShowStats() bool {
 
 // Run the function with stats and retries if required
 func Run(Retry bool, showStats bool, cmd *cobra.Command, f func() error) {
-	ci := fs.GetConfig(context.Background())
+	ctx := context.Background()
+	ci := fs.GetConfig(ctx)
 	var cmdErr error
 	stopStats := func() {}
 	if !showStats && ShowStats() {
@@ -248,7 +253,7 @@ func Run(Retry bool, showStats bool, cmd *cobra.Command, f func() error) {
 	SigInfoHandler()
 	for try := 1; try <= ci.Retries; try++ {
 		cmdErr = f()
-		cmdErr = fs.CountError(cmdErr)
+		cmdErr = fs.CountError(ctx, cmdErr)
 		lastErr := accounting.GlobalStats().GetLastError()
 		if cmdErr == nil {
 			cmdErr = lastErr
@@ -436,19 +441,19 @@ func initConfig() {
 		fs.Infof(nil, "Creating CPU profile %q\n", *cpuProfile)
 		f, err := os.Create(*cpuProfile)
 		if err != nil {
-			err = fs.CountError(err)
+			err = fs.CountError(ctx, err)
 			fs.Fatal(nil, fmt.Sprint(err))
 		}
 		err = pprof.StartCPUProfile(f)
 		if err != nil {
-			err = fs.CountError(err)
+			err = fs.CountError(ctx, err)
 			fs.Fatal(nil, fmt.Sprint(err))
 		}
 		atexit.Register(func() {
 			pprof.StopCPUProfile()
 			err := f.Close()
 			if err != nil {
-				err = fs.CountError(err)
+				err = fs.CountError(ctx, err)
 				fs.Fatal(nil, fmt.Sprint(err))
 			}
 		})
@@ -460,25 +465,26 @@ func initConfig() {
 			fs.Infof(nil, "Saving Memory profile %q\n", *memProfile)
 			f, err := os.Create(*memProfile)
 			if err != nil {
-				err = fs.CountError(err)
+				err = fs.CountError(ctx, err)
 				fs.Fatal(nil, fmt.Sprint(err))
 			}
 			err = pprof.WriteHeapProfile(f)
 			if err != nil {
-				err = fs.CountError(err)
+				err = fs.CountError(ctx, err)
 				fs.Fatal(nil, fmt.Sprint(err))
 			}
 			err = f.Close()
 			if err != nil {
-				err = fs.CountError(err)
+				err = fs.CountError(ctx, err)
 				fs.Fatal(nil, fmt.Sprint(err))
 			}
 		})
 	}
 }
 
 func resolveExitCode(err error) {
-	ci := fs.GetConfig(context.Background())
+	ctx := context.Background()
+	ci := fs.GetConfig(ctx)
 	atexit.Run()
 	if err == nil {
 		if ci.ErrorOnNoTransfer {

Diff for: cmd/serve/dlna/dlna.go

@@ -190,16 +190,17 @@ func (s *server) ModelNumber() string {
 
 // Renders the root device descriptor.
 func (s *server) rootDescHandler(w http.ResponseWriter, r *http.Request) {
+	ctx := r.Context()
 	tmpl, err := data.GetTemplate()
 	if err != nil {
-		serveError(s, w, "Failed to load root descriptor template", err)
+		serveError(ctx, s, w, "Failed to load root descriptor template", err)
 		return
 	}
 
 	buffer := new(bytes.Buffer)
 	err = tmpl.Execute(buffer, s)
 	if err != nil {
-		serveError(s, w, "Failed to render root descriptor XML", err)
+		serveError(ctx, s, w, "Failed to render root descriptor XML", err)
 		return
 	}
 
@@ -215,15 +216,16 @@ func (s *server) rootDescHandler(w http.ResponseWriter, r *http.Request) {
 
 // Handle a service control HTTP request.
 func (s *server) serviceControlHandler(w http.ResponseWriter, r *http.Request) {
+	ctx := r.Context()
 	soapActionString := r.Header.Get("SOAPACTION")
 	soapAction, err := upnp.ParseActionHTTPHeader(soapActionString)
 	if err != nil {
-		serveError(s, w, "Could not parse SOAPACTION header", err)
+		serveError(ctx, s, w, "Could not parse SOAPACTION header", err)
 		return
 	}
 	var env soap.Envelope
 	if err := xml.NewDecoder(r.Body).Decode(&env); err != nil {
-		serveError(s, w, "Could not parse SOAP request body", err)
+		serveError(ctx, s, w, "Could not parse SOAP request body", err)
 		return
 	}
 
@@ -257,6 +259,7 @@ func (s *server) soapActionResponse(sa upnp.SoapAction, actionRequestXML []byte,
 
 // Serves actual resources (media files).
 func (s *server) resourceHandler(w http.ResponseWriter, r *http.Request) {
+	ctx := r.Context()
 	remotePath := r.URL.Path
 	node, err := s.vfs.Stat(r.URL.Path)
 	if err != nil {
@@ -277,7 +280,7 @@ func (s *server) resourceHandler(w http.ResponseWriter, r *http.Request) {
 	file := node.(*vfs.File)
 	in, err := file.Open(os.O_RDONLY)
 	if err != nil {
-		serveError(node, w, "Could not open resource", err)
+		serveError(ctx, node, w, "Could not open resource", err)
 		return
 	}
 	defer fs.CheckClose(in, &err)

Diff for: cmd/serve/dlna/dlna_util.go

@@ -1,6 +1,7 @@
 package dlna
 
 import (
+	"context"
 	"crypto/md5"
 	"encoding/xml"
 	"fmt"
@@ -142,9 +143,10 @@ func logging(next http.Handler) http.Handler {
 // Error recovery and general request logging are left to logging().
 func traceLogging(next http.Handler) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		ctx := r.Context()
 		dump, err := httputil.DumpRequest(r, true)
 		if err != nil {
-			serveError(nil, w, "error dumping request", err)
+			serveError(ctx, nil, w, "error dumping request", err)
 			return
 		}
 		fs.Debugf(nil, "%s", dump)
@@ -182,8 +184,8 @@ func withHeader(name string, value string, next http.Handler) http.Handler {
 }
 
 // serveError returns an http.StatusInternalServerError and logs the error
-func serveError(what interface{}, w http.ResponseWriter, text string, err error) {
-	err = fs.CountError(err)
+func serveError(ctx context.Context, what interface{}, w http.ResponseWriter, text string, err error) {
+	err = fs.CountError(ctx, err)
 	fs.Errorf(what, "%s: %v", text, err)
 	http.Error(w, text+".", http.StatusInternalServerError)
 }

Diff for: cmd/serve/http/http.go

@@ -186,6 +186,7 @@ func (s *HTTP) handler(w http.ResponseWriter, r *http.Request) {
 
 // serveDir serves a directory index at dirRemote
 func (s *HTTP) serveDir(w http.ResponseWriter, r *http.Request, dirRemote string) {
+	ctx := r.Context()
 	VFS, err := s.getVFS(r.Context())
 	if err != nil {
 		http.Error(w, "Root directory not found", http.StatusNotFound)
@@ -198,7 +199,7 @@ func (s *HTTP) serveDir(w http.ResponseWriter, r *http.Request, dirRemote string
 		http.Error(w, "Directory not found", http.StatusNotFound)
 		return
 	} else if err != nil {
-		serve.Error(dirRemote, w, "Failed to list directory", err)
+		serve.Error(ctx, dirRemote, w, "Failed to list directory", err)
 		return
 	}
 	if !node.IsDir() {
@@ -208,7 +209,7 @@ func (s *HTTP) serveDir(w http.ResponseWriter, r *http.Request, dirRemote string
 	dir := node.(*vfs.Dir)
 	dirEntries, err := dir.ReadDirAll()
 	if err != nil {
-		serve.Error(dirRemote, w, "Failed to list directory", err)
+		serve.Error(ctx, dirRemote, w, "Failed to list directory", err)
 		return
 	}
 
@@ -234,6 +235,7 @@ func (s *HTTP) serveDir(w http.ResponseWriter, r *http.Request, dirRemote string
 
 // serveFile serves a file object at remote
 func (s *HTTP) serveFile(w http.ResponseWriter, r *http.Request, remote string) {
+	ctx := r.Context()
 	VFS, err := s.getVFS(r.Context())
 	if err != nil {
 		http.Error(w, "File not found", http.StatusNotFound)
@@ -247,7 +249,7 @@ func (s *HTTP) serveFile(w http.ResponseWriter, r *http.Request, remote string)
 		http.Error(w, "File not found", http.StatusNotFound)
 		return
 	} else if err != nil {
-		serve.Error(remote, w, "Failed to find file", err)
+		serve.Error(ctx, remote, w, "Failed to find file", err)
 		return
 	}
 	if !node.IsFile() {
@@ -287,7 +289,7 @@ func (s *HTTP) serveFile(w http.ResponseWriter, r *http.Request, remote string)
 	// open the object
 	in, err := file.Open(os.O_RDONLY)
 	if err != nil {
-		serve.Error(remote, w, "Failed to open file", err)
+		serve.Error(ctx, remote, w, "Failed to open file", err)
 		return
 	}
 	defer func() {

Diff for: cmd/serve/webdav/webdav.go

@@ -349,6 +349,7 @@ func (w *WebDAV) ServeHTTP(rw http.ResponseWriter, r *http.Request) {
 // serveDir serves a directory index at dirRemote
 // This is similar to serveDir in serve http.
 func (w *WebDAV) serveDir(rw http.ResponseWriter, r *http.Request, dirRemote string) {
+	ctx := r.Context()
 	VFS, err := w.getVFS(r.Context())
 	if err != nil {
 		http.Error(rw, "Root directory not found", http.StatusNotFound)
@@ -361,7 +362,7 @@ func (w *WebDAV) serveDir(rw http.ResponseWriter, r *http.Request, dirRemote str
 		http.Error(rw, "Directory not found", http.StatusNotFound)
 		return
 	} else if err != nil {
-		serve.Error(dirRemote, rw, "Failed to list directory", err)
+		serve.Error(ctx, dirRemote, rw, "Failed to list directory", err)
 		return
 	}
 	if !node.IsDir() {
@@ -372,7 +373,7 @@ func (w *WebDAV) serveDir(rw http.ResponseWriter, r *http.Request, dirRemote str
 	dirEntries, err := dir.ReadDirAll()
 
 	if err != nil {
-		serve.Error(dirRemote, rw, "Failed to list directory", err)
+		serve.Error(ctx, dirRemote, rw, "Failed to list directory", err)
 		return
 	}
 

Diff for: fs/accounting/accounting.go

@@ -44,7 +44,9 @@ func Start(ctx context.Context) {
 	//
 	// We can't do this in an init() method as it uses fs.Config
 	// and that isn't set up then.
-	fs.CountError = GlobalStats().Error
+	fs.CountError = func(ctx context.Context, err error) error {
+		return Stats(ctx).Error(err)
+	}
 }
 
 // Account limits and accounts for one transfer

Diff for: fs/accounting/stats_groups_test.go

@@ -7,6 +7,8 @@ import (
 	"testing"
 	"time"
 
+	"github.com/rclone/rclone/fs"
+	"github.com/rclone/rclone/fs/fserrors"
 	"github.com/rclone/rclone/fs/rc"
 	"github.com/rclone/rclone/fstest/testy"
 	"github.com/stretchr/testify/assert"
@@ -206,6 +208,34 @@ func TestStatsGroupOperations(t *testing.T) {
 	})
 }
 
+func TestCountError(t *testing.T) {
+	ctx := context.Background()
+	Start(ctx)
+	defer func() {
+		groups = newStatsGroups()
+	}()
+	t.Run("global stats", func(t *testing.T) {
+		GlobalStats().ResetCounters()
+		err := fs.CountError(ctx, fmt.Errorf("global err"))
+		assert.Equal(t, int64(1), GlobalStats().errors)
+
+		assert.True(t, fserrors.IsCounted(err))
+	})
+	t.Run("group stats", func(t *testing.T) {
+		statGroupName := fmt.Sprintf("%s-error_group", t.Name())
+		GlobalStats().ResetCounters()
+		stCtx := WithStatsGroup(ctx, statGroupName)
+		st := StatsGroup(stCtx, statGroupName)
+
+		err := fs.CountError(stCtx, fmt.Errorf("group err"))
+
+		assert.Equal(t, int64(0), GlobalStats().errors)
+		assert.Equal(t, int64(1), st.errors)
+		assert.True(t, fserrors.IsCounted(err))
+	})
+
+}
+
 func percentDiff(start, end uint64) uint64 {
 	return (start - end) * 100 / start
 }

Diff for: fs/cache/cache.go

@@ -29,7 +29,7 @@ func createOnFirstUse() {
 		c.SetExpireInterval(ci.FsCacheExpireInterval)
 		c.SetFinalizer(func(value interface{}) {
 			if s, ok := value.(fs.Shutdowner); ok {
-				_ = fs.CountError(s.Shutdown(context.Background()))
+				_ = fs.CountError(context.Background(), s.Shutdown(context.Background()))
 			}
 		})
 	})

Diff for: fs/config.go

@@ -41,7 +41,7 @@ var (
 	//
 	// This is a function pointer to decouple the config
 	// implementation from the fs
-	CountError = func(err error) error { return err }
+	CountError = func(ctx context.Context, err error) error { return err }
 
 	// ConfigProvider is the config key used for provider options
 	ConfigProvider = "provider"