feat: retention days for audit log

[yottahmd]

Summary by CodeRabbit

Release Notes

• New Features
  • Added configurable audit log retention with automatic cleanup of expired logs. Set retention days via the AUDIT_RETENTION_DAYS environment variable or configuration. Default is 7 days; use 0 to retain logs indefinitely.

[coderabbitai]

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

• 🔍 Trigger a full review

📝 Walkthrough

Walkthrough

This change introduces audit log retention functionality by extending configuration structures to include RetentionDays, implementing a background cleaner for removing expired audit files, integrating it into the file-based audit store, and wiring the store lifecycle into the server for proper resource cleanup.

Changes

Cohort / File(s)	Summary
Configuration Structures internal/cmn/config/config.go, internal/cmn/config/definition.go, internal/cmn/config/loader.go, internal/cmn/config/loader_test.go	Added RetentionDays field (defaulting to 7) to AuditConfig and AuditDef structs. Extended loader to bind AUDIT_RETENTION_DAYS environment variable and apply retention configuration during server initialization.
Audit File Cleaner internal/persis/fileaudit/cleaner.go, internal/persis/fileaudit/cleaner_test.go	Implements new unexported cleaner type with background goroutine that periodically (every 24 hours) removes audit log files older than retentionDays. Includes purge logic, file parsing, and comprehensive test coverage for boundary conditions and edge cases.
Audit Store Integration internal/persis/fileaudit/store.go, internal/persis/fileaudit/store_test.go	Updated New() constructor to accept retentionDays parameter (signature change from New(baseDir string) to New(baseDir string, retentionDays int)). Added cleaner field to Store and new Close() method for lifecycle management. Cleaner is initialized only when retentionDays > 0.
Server Lifecycle internal/service/frontend/server.go	Added auditStore field to Server struct. Wired store into server initialization and extended graceful shutdown to invoke Close() on the audit store.

Sequence Diagram(s)

sequenceDiagram
    participant Config as Configuration Loader
    participant Store as File Audit Store
    participant Cleaner as Background Cleaner
    participant Server as Frontend Server
    
    Config->>Store: New(baseDir, retentionDays)
    Store->>Cleaner: newCleaner(baseDir, retentionDays)
    Cleaner->>Cleaner: Start 24-hour ticker loop
    Cleaner-->>Store: Running in background
    Store-->>Config: Return Store instance
    Config->>Server: Initialize with audit store
    Note over Server: auditStore holds Store reference
    
    Server->>Server: Graceful shutdown triggered
    Server->>Store: Close()
    Store->>Cleaner: Stop via stopCh
    Cleaner->>Cleaner: Exit loop, cleanup
    Cleaner-->>Store: Stopped
    Store-->>Server: Cleanup complete

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related PRs

• feat: audit logging for security operations #1570: Extends the file-based audit store implementation with retention-based background cleaner and modifies the New() constructor signature alongside server lifecycle integration.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 29.63% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The pull request title 'feat: retention days for audit log' directly and clearly describes the main change: adding a retention days feature for audit logs.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch ralph/audit-log-retention

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 3

🤖 Fix all issues with AI agents

In `@internal/persis/fileaudit/cleaner_test.go`:
- Line 109: Update the misspelled word "unparseable" to the standard spelling
"unparsable" in the test description strings found in
internal/persis/fileaudit/cleaner_test.go (e.g. the string "jsonl file with
unparseable date name should not be deleted" and the two other occurrences
flagged by static analysis). Make the exact string replacements in the test
cases so all three assertions/readable messages use "unparsable" consistently.

In `@internal/persis/fileaudit/cleaner.go`:
- Around line 56-75: The purgeExpiredFiles cutoff currently uses
AddDate(0,0,-c.retentionDays) making retentionDays=7 keep 8 days; update the
cutoff computation in cleaner.purgeExpiredFiles (where cutoff is set) to
AddDate(0,0,-c.retentionDays+1) so files dated on the cutoff day are considered
expired (i.e., keep exactly retentionDays including today), or instead add a
clarifying comment on the inclusive/exclusive boundary if you intend to keep the
current behavior.

In `@internal/service/frontend/server.go`:
- Around line 805-809: The signal handler in setupGracefulShutdown currently
calls srv.httpServer.Shutdown(shutdownCtx) and thus skips coordinated cleanup;
change it to invoke srv.Shutdown(shutdownCtx) so the server's full shutdown path
runs (which calls auditStore.Close, syncService.Stop, sseHub.Shutdown, etc.).
Ensure setupGracefulShutdown uses the same shutdownCtx and error handling when
calling srv.Shutdown and remove or avoid directly calling
srv.httpServer.Shutdown so all cleanup in srv.Shutdown executes on
SIGINT/SIGTERM.

🧹 Nitpick comments (6)

internal/cmn/config/config.go (1)

131-134: Consider validating RetentionDays is non-negative.

A negative RetentionDays would silently behave like 0 (keep forever) since the store only creates a cleaner when retentionDays > 0. This is safe but could mask a configuration mistake. Consider adding validation in Validate() or clamping in the loader.

internal/cmn/config/loader_test.go (1)

1189-1234: Missing test coverage for RetentionDays configuration.

The TestLoad_Audit suite only tests the Enabled toggle. Consider adding sub-tests that verify RetentionDays can be set via YAML (audit.retentionDays: 30) and overridden via the DAGU_AUDIT_RETENTION_DAYS environment variable, similar to how other config fields are tested in this file.

internal/service/frontend/server.go (1)

70-70: Holding a concrete *fileaudit.Store couples the server to the file-based implementation.

The auditStore field is typed as *fileaudit.Store rather than an interface (e.g., io.Closer). Since the server only needs to call Close() on it, consider using io.Closer to keep the server decoupled from the persistence layer. This is a minor point given the current architecture.

internal/persis/fileaudit/store.go (1)

42-61: Cleaner goroutine starts eagerly in constructor — consider implications.

newCleaner (line 57) spawns a goroutine immediately. Two things to note:

1. The cleaner's purgeExpiredFiles does os.Remove on files that Query might be reading concurrently. This is safe only because readEntriesFromFile already handles os.ErrNotExist gracefully (line 146). Worth a brief comment near the cleaner field or construction to document this intentional design.
2. If Close() is never called (e.g., error paths in the caller), the goroutine leaks. Consider whether the caller (server wiring) covers all error/shutdown paths.

Neither is a blocker — just flagging for awareness.

internal/persis/fileaudit/cleaner.go (2)

20-30: No panic recovery on the background goroutine.

If purgeExpiredFiles ever panics (e.g., an unexpected runtime error), it would crash the entire process. Consider wrapping the goroutine body with a deferred recover, or at minimum logging the panic before re-raising.

This is low-risk since the current implementation only does I/O and parsing, but it's a defensive practice for long-lived background goroutines.

---

49-54: stop() doesn't wait for the goroutine to exit.

Close() → stop() signals the goroutine but returns immediately. If shutdown code subsequently removes or inspects the audit directory, the goroutine may still be mid-purge. Consider adding a sync.WaitGroup or a done channel if you need deterministic shutdown ordering.

Not a blocker for the current use case, but worth noting.

Example with WaitGroup for deterministic shutdown

 type cleaner struct {
 	baseDir       string
 	retentionDays int
 	stopCh        chan struct{}
 	stopOnce      sync.Once
+	wg            sync.WaitGroup
 }

 func newCleaner(baseDir string, retentionDays int) *cleaner {
 	c := &cleaner{
 		baseDir:       baseDir,
 		retentionDays: retentionDays,
 		stopCh:        make(chan struct{}),
 	}
+	c.wg.Add(1)
 	go c.run()
 	return c
 }

 func (c *cleaner) run() {
+	defer c.wg.Done()
 	c.purgeExpiredFiles()
 	// ...
 }

 func (c *cleaner) stop() {
 	c.stopOnce.Do(func() {
 		close(c.stopCh)
 	})
+	c.wg.Wait()
 }

[codecov]

Codecov Report

❌ Patch coverage is 70.58824% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.87%. Comparing base (fd7fabe) to head (c240eb3).
⚠️ Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/persis/fileaudit/cleaner.go	64.91%	10 Missing and 10 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1640      +/-   ##
==========================================
+ Coverage   69.79%   69.87%   +0.07%     
==========================================
  Files         331      332       +1     
  Lines       37248    37322      +74     
==========================================
+ Hits        25999    26080      +81     
+ Misses       9185     9173      -12     
- Partials     2064     2069       +5     

Files with missing lines	Coverage Δ
internal/cmn/config/config.go	74.15% <ø> (ø)
internal/cmn/config/loader.go	83.21% <100.00%> (+0.04%)	⬆️
internal/persis/fileaudit/store.go	72.89% <100.00%> (+1.89%)	⬆️
internal/persis/fileaudit/cleaner.go	64.91% <64.91%> (ø)

... and 13 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e4cdf60...c240eb3. Read the comment docs.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.