feat: option to keep deleted recordings in backup for certain period #645
Conversation
📝 WalkthroughWalkthroughThis pull request introduces an enhanced recording backup management system. The changes add new configuration options to control the deletion and backup of recording files. A new feature allows users to enable backup of deleted recordings, specifying a backup path and retention duration. The implementation includes modifications to the configuration handling, scheduler, and recording deletion processes to support this new functionality, providing more robust file management for recording files. Changes
Sequence DiagramsequenceDiagram
participant Config as Configuration
participant Scheduler as Backup Scheduler
participant FileSystem as File System
Config->>Scheduler: Enable Backup
Scheduler->>Scheduler: Start Hourly Check
Scheduler->>FileSystem: Check Backup Directory
FileSystem-->>Scheduler: Return File List
Scheduler->>Scheduler: Calculate Cutoff Time
Scheduler->>FileSystem: Remove Expired Backup Files
Possibly related PRs
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
pkg/config/config.go (1)
90-94: Consider adding validation for backup configuration values.The new fields look good, but consider adding validation to ensure:
DelRecordingBackupPathis in a safe locationDelRecordingBackupDurationis within reasonable bounds (e.g., minimum 1 hour, maximum 30 days)pkg/models/recording_delete.go (1)
Line range hint
82-91: Consider preserving directory structure in backup.The current implementation flattens the directory structure in backup. Consider preserving it to maintain organization and prevent filename collisions.
config_sample.yaml (1)
146-150: Consider making the backup path more configurable.While the implementation works, the hardcoded "/app" prefix in the backup path might not be suitable for all deployment scenarios. Consider:
- Making it relative to
recording_files_path- Using environment variables for path configuration
- Adding validation to ensure both paths are on the same disk/volume
Example configuration using relative path:
- del_recording_backup_path: "/app/recording_files/del_backup" + del_recording_backup_path: "${recording_files_path}/del_backup"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
config_sample.yaml(1 hunks)pkg/config/config.go(3 hunks)pkg/models/recording_delete.go(3 hunks)pkg/models/scheduler.go(2 hunks)pkg/models/scheduler_recording.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (3)
pkg/models/scheduler.go (1)
54-55: LGTM! Clean implementation of the hourly backup cleanup.The hourly ticker is properly initialized and cleaned up, following the same pattern as other tickers in the scheduler.
Also applies to: 67-68
config_sample.yaml (2)
142-145: LGTM! Clear documentation of the backup feature.The configuration and its documentation clearly explain the purpose and behavior of the deleted recording backup feature.
151-152: LGTM! Clear duration configuration.The backup duration is well-documented and uses Go's standard duration format.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pkg/models/recording_delete.go (3)
72-74: Improve error handling in cleanup operations.The current implementation silently ignores errors when removing compressed and JSON files. Consider logging these errors for better debugging capabilities.
Apply this diff:
-_ = os.Remove(filePath + ".fiber.gz") -_ = os.Remove(filePath + ".json") +if err := os.Remove(filePath + ".fiber.gz"); err != nil && !os.IsNotExist(err) { + log.Debugf("Failed to remove compressed file: %v", err) +} +if err := os.Remove(filePath + ".json"); err != nil && !os.IsNotExist(err) { + log.Debugf("Failed to remove JSON file: %v", err) +}
Line range hint
79-87: Enhance directory cleanup robustness.The directory cleanup logic could be improved in several ways:
- Use
filepath.Cleanfor path comparison- Add debug logging for directory removal attempts
- Consider using
filepath.Dirinstead of string replacementApply this diff:
-dir := strings.Replace(filePath, f.Name(), "", 1) +dir := filepath.Dir(filePath) +cleanRecordingPath := filepath.Clean(config.GetConfig().RecorderInfo.RecordingFilesPath) -if dir != config.GetConfig().RecorderInfo.RecordingFilesPath { +if filepath.Clean(dir) != cleanRecordingPath { empty, err := m.isDirEmpty(dir) if err == nil && empty { + log.Debugf("Attempting to remove empty directory: %s", dir) err = os.Remove(dir) if err != nil { - log.Error(err) + log.Errorf("Failed to remove directory %s: %v", dir, err) } } }
16-16: Add documentation for the backup functionality.Consider adding documentation to explain:
- The purpose of the backup feature
- The behavior when backup is enabled/disabled
- How the retention period works
- Any limitations or requirements for the backup path
Add this documentation above the
DeleteRecordingmethod:// DeleteRecording removes a recording identified by RecordId. If backup is enabled // via RecorderInfo.EnableDelRecordingBackup, the recording will be moved to the // backup location specified by DelRecordingBackupPath instead of being deleted. // The backup will be retained according to DelRecordingBackupDuration setting. // Returns an error if the operation fails.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
config_sample.yaml(1 hunks)pkg/models/recording_delete.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- config_sample.yaml
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (1)
pkg/models/recording_delete.go (1)
39-61: Improve reliability of backup operations.The current backup implementation has several reliability concerns:
- No space check before backup operation
- No handling of cross-device moves (could fail on different filesystems)
- Setting modification time to current time might affect retention calculations
- JSON file backup error handling could be improved
Consider implementing the improvements suggested in the previous review comment, which addresses these concerns comprehensively.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/models/recording_delete.go (1)
41-69:⚠️ Potential issueImprove reliability of backup operations.
The backup implementation needs several reliability improvements:
- No handling of cross-device moves (os.Rename may fail)
- No size checks for backup directory
- No validation of backup directory existence and permissions
Please refer to the previous review comment for the detailed implementation suggestions, including:
- Adding space checks before backup
- Handling cross-device moves with fallback to copy
- Using Go's standard library for file modification time updates
🧹 Nitpick comments (2)
pkg/models/recording_delete.go (2)
26-36: Enhance error handling robustness.While the switch to
errors.Asis good, the error handling could be improved:
- The error message splitting using
strings.SplitNis fragile and may not work consistently across different error messages.- The error handling could be more informative for debugging purposes.
Consider this improvement:
f, err := os.Stat(filePath) if err != nil { var pathError *fs.PathError if errors.As(err, &pathError) { - log.Errorln(filePath + " does not exist, so deleting from DB without stopping") + log.WithFields(log.Fields{ + "path": filePath, + "error": err, + }).Info("File not found, proceeding with DB deletion") fileExist = false } else { - ms := strings.SplitN(err.Error(), "/", -1) - return errors.New(ms[len(ms)-1]) + return fmt.Errorf("failed to check file status: %w", err) } }
Line range hint
74-89: Improve cleanup operations reliability.The cleanup operations have potential issues:
- Errors from cleanup operations are silently ignored
- There's a potential race condition in the directory emptiness check
Consider these improvements:
- _ = os.Remove(filePath + ".fiber.gz") - _ = os.Remove(filePath + ".json") + if err := os.Remove(filePath + ".fiber.gz"); err != nil && !errors.Is(err, fs.ErrNotExist) { + log.WithError(err).Warn("Failed to remove compressed file") + } + if err := os.Remove(filePath + ".json"); err != nil && !errors.Is(err, fs.ErrNotExist) { + log.WithError(err).Warn("Failed to remove JSON file") + } // we will check if the directory is empty or not // if empty then better to delete that directory if fileExist { dir := strings.Replace(filePath, f.Name(), "", 1) if dir != config.GetConfig().RecorderInfo.RecordingFilesPath { - empty, err := m.isDirEmpty(dir) - if err == nil && empty { - err = os.Remove(dir) - if err != nil { - log.Error(err) - } + // Use RemoveAll only if directory is empty to handle race conditions + entries, err := os.ReadDir(dir) + if err == nil && len(entries) == 0 { + if err := os.RemoveAll(dir); err != nil { + log.WithError(err).Error("Failed to remove empty directory") + } } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/models/recording_delete.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (2)
pkg/models/recording_delete.go (2)
10-14: LGTM! Appropriate imports added for new functionality.The new imports support the enhanced file operations and time-based features.
Line range hint
1-89: Verify implementation completeness across the codebase.Let's verify the implementation for potential issues:
- Check for consistent error handling patterns
- Look for potential backup path issues
- Verify scheduler integration for backup cleanup
✅ Verification successful
Implementation is complete and well-integrated
The codebase shows:
- Consistent error handling patterns
- Proper backup path validation and creation
- Complete scheduler integration for backup cleanup
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify implementation completeness and consistency # Check for consistent error handling patterns echo "Checking error handling patterns..." rg -A 2 "os\.(Rename|Remove)" --type go # Look for backup path validation echo "Checking backup path validation..." rg "DelRecordingBackupPath" --type go # Verify scheduler integration echo "Checking scheduler integration..." rg "checkDelRecordingBackupPath" --type goLength of output: 4433
Summary by CodeRabbit
Release Notes
New Features
Improvements
Configuration