Skip to content

Commit 2bcc762

Browse files
committed
feat(logging): improve file-backed source cleanup and directory recreation logic
- Added `assertFileBodySourceCleaned` helper to streamline cleanup validations in tests. - Introduced handling to recreate missing directories during file source operations. - Enhanced tests to verify behavior after manual directory removal, ensuring robustness. - Fixed edge cases in log file merging when parts are missing.
1 parent d9c01a6 commit 2bcc762

2 files changed

Lines changed: 72 additions & 17 deletions

File tree

internal/logging/request_logger.go

Lines changed: 18 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,9 @@ func (s *FileBodySource) CreatePart(prefix string) (*os.File, error) {
110110
return nil, fmt.Errorf("file body source has been cleaned")
111111
}
112112
prefix = sanitizeTempPrefix(prefix)
113+
if errMkdir := os.MkdirAll(s.dir, 0755); errMkdir != nil {
114+
return nil, errMkdir
115+
}
113116
file, errCreate := os.CreateTemp(s.dir, prefix+"-*.tmp")
114117
if errCreate != nil {
115118
return nil, errCreate
@@ -165,16 +168,23 @@ func (s *FileBodySource) WriteTo(w io.Writer) error {
165168
return nil
166169
}
167170
paths := s.Paths()
168-
for i, path := range paths {
169-
if i > 0 {
170-
if _, errWrite := io.WriteString(w, "\n"); errWrite != nil {
171-
return errWrite
172-
}
173-
}
171+
wrote := false
172+
for _, path := range paths {
174173
file, errOpen := os.Open(path)
175174
if errOpen != nil {
175+
if os.IsNotExist(errOpen) {
176+
continue
177+
}
176178
return errOpen
177179
}
180+
if wrote {
181+
if _, errWrite := io.WriteString(w, "\n"); errWrite != nil {
182+
if errClose := file.Close(); errClose != nil {
183+
log.WithError(errClose).Warn("failed to close log part file")
184+
}
185+
return errWrite
186+
}
187+
}
178188
_, errCopy := io.Copy(w, file)
179189
if errClose := file.Close(); errClose != nil {
180190
log.WithError(errClose).Warn("failed to close log part file")
@@ -185,6 +195,7 @@ func (s *FileBodySource) WriteTo(w io.Writer) error {
185195
if errCopy != nil {
186196
return errCopy
187197
}
198+
wrote = true
188199
}
189200
return nil
190201
}
@@ -222,7 +233,7 @@ func (s *FileBodySource) Cleanup() error {
222233
}
223234
}
224235
if dir != "" {
225-
if errRemove := os.Remove(dir); errRemove != nil && !os.IsNotExist(errRemove) && firstErr == nil {
236+
if errRemove := os.RemoveAll(dir); errRemove != nil && firstErr == nil {
226237
firstErr = errRemove
227238
}
228239
}

internal/logging/request_logger_home_test.go

Lines changed: 54 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"encoding/json"
77
"net/http"
88
"os"
9+
"path/filepath"
910
"strings"
1011
"testing"
1112
"time"
@@ -23,6 +24,57 @@ func (c *stubHomeRequestLogClient) RPushRequestLog(_ context.Context, payload []
2324
return nil
2425
}
2526

27+
func assertFileBodySourceCleaned(t *testing.T, partPaths []string) {
28+
t.Helper()
29+
30+
dirs := make(map[string]struct{}, len(partPaths))
31+
for _, path := range partPaths {
32+
if _, errStat := os.Stat(path); !os.IsNotExist(errStat) {
33+
t.Fatalf("expected part %s to be removed, stat err=%v", path, errStat)
34+
}
35+
dirs[filepath.Dir(path)] = struct{}{}
36+
}
37+
for dir := range dirs {
38+
if _, errStat := os.Stat(dir); !os.IsNotExist(errStat) {
39+
t.Fatalf("expected part dir %s to be removed, stat err=%v", dir, errStat)
40+
}
41+
}
42+
}
43+
44+
func TestFileBodySource_RecreatesPartDirAfterManualCleanup(t *testing.T) {
45+
logsDir := t.TempDir()
46+
source, errSource := NewFileBodySourceInDir(logsDir, "websocket-timeline-test")
47+
if errSource != nil {
48+
t.Fatalf("NewFileBodySourceInDir: %v", errSource)
49+
}
50+
if errAppend := source.AppendPart([]byte("before manual cleanup")); errAppend != nil {
51+
t.Fatalf("AppendPart before cleanup: %v", errAppend)
52+
}
53+
if errRemove := os.RemoveAll(logsDir); errRemove != nil {
54+
t.Fatalf("RemoveAll logs dir: %v", errRemove)
55+
}
56+
if errAppend := source.AppendPart([]byte("after manual cleanup")); errAppend != nil {
57+
t.Fatalf("AppendPart after cleanup: %v", errAppend)
58+
}
59+
60+
raw, errBytes := source.Bytes()
61+
if errBytes != nil {
62+
t.Fatalf("Bytes after cleanup: %v", errBytes)
63+
}
64+
if bytes.Contains(raw, []byte("before manual cleanup")) {
65+
t.Fatalf("expected manually removed part to be skipped, got %q", string(raw))
66+
}
67+
if !bytes.Contains(raw, []byte("after manual cleanup")) {
68+
t.Fatalf("expected recreated part content, got %q", string(raw))
69+
}
70+
71+
partPaths := source.Paths()
72+
if errCleanup := source.Cleanup(); errCleanup != nil {
73+
t.Fatalf("Cleanup: %v", errCleanup)
74+
}
75+
assertFileBodySourceCleaned(t, partPaths)
76+
}
77+
2678
func TestFileRequestLogger_HomeEnabled_ForwardsWhenRequestLogEnabled(t *testing.T) {
2779
original := currentHomeRequestLogClient
2880
defer func() {
@@ -143,11 +195,7 @@ func TestFileRequestLogger_LogRequestWithSourcesWritesLocalLogAndCleansParts(t *
143195
t.Fatalf("LogRequestWithOptionsAndSources error: %v", errLog)
144196
}
145197

146-
for _, path := range partPaths {
147-
if _, errStat := os.Stat(path); !os.IsNotExist(errStat) {
148-
t.Fatalf("expected part %s to be removed, stat err=%v", path, errStat)
149-
}
150-
}
198+
assertFileBodySourceCleaned(t, partPaths)
151199

152200
entries, errRead := os.ReadDir(logsDir)
153201
if errRead != nil {
@@ -245,11 +293,7 @@ func TestFileRequestLogger_HomeEnabled_ForwardsSourceLogAndCleansParts(t *testin
245293
if !strings.Contains(got.RequestLog, "Event: websocket.request") {
246294
t.Fatalf("forwarded request_log missing websocket request: %s", got.RequestLog)
247295
}
248-
for _, path := range partPaths {
249-
if _, errStat := os.Stat(path); !os.IsNotExist(errStat) {
250-
t.Fatalf("expected part %s to be removed, stat err=%v", path, errStat)
251-
}
252-
}
296+
assertFileBodySourceCleaned(t, partPaths)
253297
}
254298

255299
func TestFileRequestLogger_HomeEnabled_ForwardsStreamingRequestID(t *testing.T) {

0 commit comments

Comments
 (0)