GODT-2149: Sort logs by timestamp when clearing

internal/logging/clear.go

@@ -18,67 +18,52 @@
 package logging
 
 import (
+	"fmt"
+	"io/fs"
 	"os"
 	"path/filepath"
-	"sort"
 
+	"github.com/bradenaw/juniper/xslices"
 	"github.com/sirupsen/logrus"
+	"golang.org/x/exp/slices"
 )
 
 func clearLogs(logDir string, maxLogs int, maxCrashes int) error {
 	files, err := os.ReadDir(logDir)
 	if err != nil {
-		return err
+		return fmt.Errorf("failed to read log directory: %w", err)
 	}
 
-	var logsWithPrefix []string
+	names := xslices.Map(files, func(file fs.DirEntry) string {
-	var crashesWithPrefix []string
+		return file.Name()
+	})
 
-	for _, file := range files {
+	// Remove old logs.
-		if MatchLogName(file.Name()) {
+	removeOldLogs(logDir, xslices.Filter(names, func(name string) bool {
-			if MatchStackTraceName(file.Name()) {
+		return MatchLogName(name) && !MatchStackTraceName(name)
-				crashesWithPrefix = append(crashesWithPrefix, file.Name())
+	}), maxLogs)
-			} else {
-				logsWithPrefix = append(logsWithPrefix, file.Name())
-			}
-		} else {
-			// Older versions of Bridge stored logs in subfolders for each version.
-			// That also has to be cleared and the functionality can be removed after some time.
-			if file.IsDir() {
-				if err := clearLogs(filepath.Join(logDir, file.Name()), maxLogs, maxCrashes); err != nil {
-					return err
-				}
-			} else {
-				removeLog(logDir, file.Name())
-			}
-		}
-	}
 
-	removeOldLogs(logDir, logsWithPrefix, maxLogs)
+	// Remove old stack traces.
-	removeOldLogs(logDir, crashesWithPrefix, maxCrashes)
+	removeOldLogs(logDir, xslices.Filter(names, func(name string) bool {
+		return MatchLogName(name) && MatchStackTraceName(name)
+	}), maxCrashes)
 
 	return nil
 }
 
-func removeOldLogs(logDir string, filenames []string, maxLogs int) {
+func removeOldLogs(dir string, names []string, max int) {
-	count := len(filenames)
+	if count := len(names); count <= max {
-	if count <= maxLogs {
 		return
 	}
 
-	sort.Strings(filenames) // Sorted by timestamp: oldest first.
+	// Sort by timestamp, oldest first.
-	for _, filename := range filenames[:count-maxLogs] {
+	slices.SortFunc(names, func(a, b string) bool {
-		removeLog(logDir, filename)
+		return getLogTime(a) < getLogTime(b)
-	}
+	})
-}
 
-func removeLog(logDir, filename string) {
+	for _, path := range xslices.Map(names[:len(names)-max], func(name string) string { return filepath.Join(dir, name) }) {
-	// We need to be sure to delete only log files.
+		if err := os.Remove(path); err != nil {
-	// Directory with logs can also contain other files.
+			logrus.WithError(err).WithField("path", path).Warn("Failed to remove old log file")
-	if !MatchLogName(filename) {
+		}
-		return
-	}
-	if err := os.Remove(filepath.Join(logDir, filename)); err != nil {
-		logrus.WithError(err).Error("Failed to remove", filepath.Join(logDir, filename))
 	}
 }

internal/logging/logging.go

@@ -24,6 +24,7 @@ import (
 	"os"
 	"path/filepath"
 	"regexp"
+	"strconv"
 	"time"
 
 	"github.com/ProtonMail/proton-bridge/v3/internal/constants"
@@ -141,6 +142,17 @@ func getLogName(version, revision string) string {
 	return fmt.Sprintf("v%v_%v_%v.log", version, revision, time.Now().Unix())
 }
 
+func getLogTime(name string) int {
+	re := regexp.MustCompile(`^v.*_.*_(?P<timestamp>\d+).log$`)
+
+	timestamp, err := strconv.Atoi(re.FindStringSubmatch(name)[re.SubexpIndex("timestamp")])
+	if err != nil {
+		return 0
+	}
+
+	return timestamp
+}
+
 func MatchLogName(name string) bool {
 	return regexp.MustCompile(`^v.*\.log$`).MatchString(name)
 }

internal/logging/logging_test.go

@@ -29,24 +29,39 @@ import (
 func TestClearLogs(t *testing.T) {
 	dir := t.TempDir()
 
+	// Create some old log files.
 	require.NoError(t, os.WriteFile(filepath.Join(dir, "other.log"), []byte("Hello"), 0o755))
-	require.NoError(t, os.WriteFile(filepath.Join(dir, "v1_10.log"), []byte("Hello"), 0o755))
+	require.NoError(t, os.WriteFile(filepath.Join(dir, "v2.4.7_debe87f2f5_0000000001.log"), []byte("Hello"), 0o755))
-	require.NoError(t, os.WriteFile(filepath.Join(dir, "v1_11.log"), []byte("Hello"), 0o755))
+	require.NoError(t, os.WriteFile(filepath.Join(dir, "v2.4.8_debe87f2f5_0000000002.log"), []byte("Hello"), 0o755))
-	require.NoError(t, os.WriteFile(filepath.Join(dir, "v2_12.log"), []byte("Hello"), 0o755))
+	require.NoError(t, os.WriteFile(filepath.Join(dir, "v2.4.9_debe87f2f5_0000000003.log"), []byte("Hello"), 0o755))
-	require.NoError(t, os.WriteFile(filepath.Join(dir, "v2_13.log"), []byte("Hello"), 0o755))
+	require.NoError(t, os.WriteFile(filepath.Join(dir, "v2.5.0_debe87f2f5_0000000004.log"), []byte("Hello"), 0o755))
+	require.NoError(t, os.WriteFile(filepath.Join(dir, "v2.5.1_debe87f2f5_0000000005.log"), []byte("Hello"), 0o755))
+	require.NoError(t, os.WriteFile(filepath.Join(dir, "v2.5.2_debe87f2f5_0000000006.log"), []byte("Hello"), 0o755))
+	require.NoError(t, os.WriteFile(filepath.Join(dir, "v2.5.3_debe87f2f5_0000000007.log"), []byte("Hello"), 0o755))
+	require.NoError(t, os.WriteFile(filepath.Join(dir, "v2.5.4_debe87f2f5_0000000008.log"), []byte("Hello"), 0o755))
+	require.NoError(t, os.WriteFile(filepath.Join(dir, "v2.5.5_debe87f2f5_0000000009.log"), []byte("Hello"), 0o755))
+	require.NoError(t, os.WriteFile(filepath.Join(dir, "v2.5.6_debe87f2f5_0000000010.log"), []byte("Hello"), 0o755))
+	require.NoError(t, os.WriteFile(filepath.Join(dir, "v2.5.7_debe87f2f5_0000000011.log"), []byte("Hello"), 0o755))
+	require.NoError(t, os.WriteFile(filepath.Join(dir, "v2.5.8_debe87f2f5_0000000012.log"), []byte("Hello"), 0o755))
+	require.NoError(t, os.WriteFile(filepath.Join(dir, "v2.5.12_debe87f2f5_0000000013.log"), []byte("Hello"), 0o755))
+	require.NoError(t, os.WriteFile(filepath.Join(dir, "v2.5.9_debe87f2f5_0000000014.log"), []byte("Hello"), 0o755))
+	require.NoError(t, os.WriteFile(filepath.Join(dir, "v2.5.10_debe87f2f5_0000000015.log"), []byte("Hello"), 0o755))
+	require.NoError(t, os.WriteFile(filepath.Join(dir, "v2.5.11_debe87f2f5_0000000016.log"), []byte("Hello"), 0o755))
 
+	// Clear the logs.
 	require.NoError(t, clearLogs(dir, 3, 0))
+
+	// We should only clear matching files, and keep the 3 most recent ones.
 	checkFileNames(t, dir, []string{
 		"other.log",
-		"v1_11.log",
+		"v2.5.9_debe87f2f5_0000000014.log",
-		"v2_12.log",
+		"v2.5.10_debe87f2f5_0000000015.log",
-		"v2_13.log",
+		"v2.5.11_debe87f2f5_0000000016.log",
 	})
 }
 
 func checkFileNames(t *testing.T, dir string, expectedFileNames []string) {
-	fileNames := getFileNames(t, dir)
+	require.ElementsMatch(t, expectedFileNames, getFileNames(t, dir))
-	require.Equal(t, expectedFileNames, fileNames)
 }
 
 func getFileNames(t *testing.T, dir string) []string {