Merge pull request #1372 from rumpl/feature/rotate-debug-logs

Add log rotation for debug logs

Author: dgageot

cmd/root/root.go

@@ -15,6 +15,7 @@ import (
 
 	"github.com/docker/cagent/pkg/environment"
 	"github.com/docker/cagent/pkg/feedback"
+	"github.com/docker/cagent/pkg/logging"
 	"github.com/docker/cagent/pkg/paths"
 	"github.com/docker/cagent/pkg/telemetry"
 	"github.com/docker/cagent/pkg/version"
@@ -24,7 +25,7 @@ type rootFlags struct {
 	enableOtel  bool
 	debugMode   bool
 	logFilePath string
-	logFile     *os.File
+	logFile     io.Closer
 }
 
 func NewRootCmd() *cobra.Command {
@@ -169,8 +170,9 @@ We collect anonymous usage data to help improve cagent. To disable:
 }
 
 // setupLogging configures slog logging behavior.
-// When --debug is enabled, logs are written to a single file <dataDir>/cagent.debug.log (append mode),
-// or to the file specified by --log-file.
+// When --debug is enabled, logs are written to a rotating file <dataDir>/cagent.debug.log,
+// or to the file specified by --log-file. Log files are rotated when they exceed 10MB,
+// keeping up to 3 backup files.
 func (f *rootFlags) setupLogging() error {
 	if !f.debugMode {
 		slog.SetDefault(slog.New(slog.DiscardHandler))
@@ -179,11 +181,7 @@ func (f *rootFlags) setupLogging() error {
 
 	path := cmp.Or(strings.TrimSpace(f.logFilePath), filepath.Join(paths.GetDataDir(), "cagent.debug.log"))
 
-	if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil {
-		return err
-	}
-
-	logFile, err := os.OpenFile(path, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o600)
+	logFile, err := logging.NewRotatingFile(path)
 	if err != nil {
 		return err
 	}

pkg/logging/rotate.go

@@ -0,0 +1,128 @@
+package logging
+
+import (
+	"fmt"
+	"os"
+	"path/filepath"
+	"sync"
+)
+
+const (
+	DefaultMaxSize    = 10 * 1024 * 1024 // 10MB
+	DefaultMaxBackups = 3
+)
+
+// RotatingFile is an io.WriteCloser that rotates log files when they exceed a size limit.
+type RotatingFile struct {
+	path       string
+	maxSize    int64
+	maxBackups int
+
+	mu   sync.Mutex
+	file *os.File
+	size int64
+}
+
+type Option func(*RotatingFile)
+
+func WithMaxSize(size int64) Option {
+	return func(r *RotatingFile) {
+		r.maxSize = size
+	}
+}
+
+func WithMaxBackups(count int) Option {
+	return func(r *RotatingFile) {
+		r.maxBackups = count
+	}
+}
+
+// NewRotatingFile creates a new rotating file writer.
+func NewRotatingFile(path string, opts ...Option) (*RotatingFile, error) {
+	r := &RotatingFile{
+		path:       path,
+		maxSize:    DefaultMaxSize,
+		maxBackups: DefaultMaxBackups,
+	}
+
+	for _, opt := range opts {
+		opt(r)
+	}
+
+	if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil {
+		return nil, err
+	}
+
+	if err := r.openFile(); err != nil {
+		return nil, err
+	}
+
+	return r, nil
+}
+
+func (r *RotatingFile) openFile() error {
+	file, err := os.OpenFile(r.path, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o600)
+	if err != nil {
+		return err
+	}
+
+	info, err := file.Stat()
+	if err != nil {
+		file.Close()
+		return err
+	}
+
+	r.file = file
+	r.size = info.Size()
+	return nil
+}
+
+func (r *RotatingFile) Write(p []byte) (int, error) {
+	r.mu.Lock()
+	defer r.mu.Unlock()
+
+	if r.size+int64(len(p)) > r.maxSize {
+		if err := r.rotate(); err != nil {
+			return 0, err
+		}
+	}
+
+	n, err := r.file.Write(p)
+	r.size += int64(n)
+	return n, err
+}
+
+func (r *RotatingFile) Close() error {
+	r.mu.Lock()
+	defer r.mu.Unlock()
+
+	if r.file != nil {
+		return r.file.Close()
+	}
+	return nil
+}
+
+func (r *RotatingFile) rotate() error {
+	if err := r.file.Close(); err != nil {
+		return err
+	}
+
+	// Remove the oldest backup if it exists
+	oldest := fmt.Sprintf("%s.%d", r.path, r.maxBackups)
+	_ = os.Remove(oldest)
+
+	// Shift existing backups: .2 -> .3, .1 -> .2, etc.
+	for i := r.maxBackups - 1; i >= 1; i-- {
+		oldPath := fmt.Sprintf("%s.%d", r.path, i)
+		newPath := fmt.Sprintf("%s.%d", r.path, i+1)
+		_ = os.Rename(oldPath, newPath)
+	}
+
+	// Rename current log to .1
+	if err := os.Rename(r.path, r.path+".1"); err != nil && !os.IsNotExist(err) {
+		return err
+	}
+
+	r.size = 0
+	return r.openFile()
+}

pkg/logging/rotate_test.go

@@ -0,0 +1,133 @@
+package logging
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestRotatingFile_Write(t *testing.T) {
+	dir := t.TempDir()
+	path := filepath.Join(dir, "test.log")
+
+	rf, err := NewRotatingFile(path, WithMaxSize(100), WithMaxBackups(2))
+	require.NoError(t, err)
+	defer rf.Close()
+
+	data := []byte("hello world\n")
+	n, err := rf.Write(data)
+	require.NoError(t, err)
+	assert.Equal(t, len(data), n)
+
+	content, err := os.ReadFile(path)
+	require.NoError(t, err)
+	assert.Equal(t, data, content)
+}
+
+func TestRotatingFile_Rotate(t *testing.T) {
+	dir := t.TempDir()
+	path := filepath.Join(dir, "test.log")
+
+	rf, err := NewRotatingFile(path, WithMaxSize(50), WithMaxBackups(2))
+	require.NoError(t, err)
+	defer rf.Close()
+
+	// Write enough data to trigger rotation
+	data := make([]byte, 30)
+	for i := range data {
+		data[i] = 'a'
+	}
+
+	_, err = rf.Write(data)
+	require.NoError(t, err)
+
+	// This write should trigger rotation
+	_, err = rf.Write(data)
+	require.NoError(t, err)
+
+	// Check that backup file was created
+	_, err = os.Stat(path + ".1")
+	require.NoError(t, err, "backup file should exist")
+
+	// Original file should have new content
+	content, err := os.ReadFile(path)
+	require.NoError(t, err)
+	assert.Equal(t, data, content)
+
+	// Backup should have old content
+	backup, err := os.ReadFile(path + ".1")
+	require.NoError(t, err)
+	assert.Equal(t, data, backup)
+}
+
+func TestRotatingFile_MaxBackups(t *testing.T) {
+	dir := t.TempDir()
+	path := filepath.Join(dir, "test.log")
+
+	rf, err := NewRotatingFile(path, WithMaxSize(20), WithMaxBackups(2))
+	require.NoError(t, err)
+	defer rf.Close()
+
+	data := make([]byte, 15)
+
+	// Write 4 times to trigger multiple rotations
+	for i := range 4 {
+		for j := range data {
+			data[j] = byte('a' + i)
+		}
+		_, err = rf.Write(data)
+		require.NoError(t, err)
+	}
+
+	// Should have current file + 2 backups (maxBackups=2)
+	_, err = os.Stat(path)
+	require.NoError(t, err, "current file should exist")
+
+	_, err = os.Stat(path + ".1")
+	require.NoError(t, err, "backup .1 should exist")
+
+	_, err = os.Stat(path + ".2")
+	require.NoError(t, err, "backup .2 should exist")
+
+	// .3 should NOT exist because maxBackups=2
+	_, err = os.Stat(path + ".3")
+	require.True(t, os.IsNotExist(err), "backup .3 should not exist")
+}
+
+func TestRotatingFile_AppendsToExisting(t *testing.T) {
+	dir := t.TempDir()
+	path := filepath.Join(dir, "test.log")
+
+	// Create a file with existing content
+	err := os.WriteFile(path, []byte("existing\n"), 0o600)
+	require.NoError(t, err)
+
+	rf, err := NewRotatingFile(path, WithMaxSize(1000), WithMaxBackups(2))
+	require.NoError(t, err)
+	defer rf.Close()
+
+	_, err = rf.Write([]byte("new\n"))
+	require.NoError(t, err)
+
+	content, err := os.ReadFile(path)
+	require.NoError(t, err)
+	assert.Equal(t, "existing\nnew\n", string(content))
+}
+
+func TestRotatingFile_CreatesDirectory(t *testing.T) {
+	dir := t.TempDir()
+	path := filepath.Join(dir, "subdir", "nested", "test.log")
+
+	rf, err := NewRotatingFile(path)
+	require.NoError(t, err)
+	defer rf.Close()
+
+	_, err = rf.Write([]byte("test"))
+	require.NoError(t, err)
+
+	_, err = os.Stat(path)
+	require.NoError(t, err)
+}