Develop #26
Develop #26
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the backup service with improved reliability, error handling, and concurrency management. The changes focus on preventing duplicate backup jobs, better handling of temporary directory conflicts, and more robust cleanup procedures.
- Adds duplicate backup prevention through running job checks and recent backup detection
- Implements more robust temporary directory management with retry logic and force cleanup
- Enhances database transaction handling with immediate commits and proper error recovery
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 11 comments.
| File | Description |
|---|---|
| backup_service.py | Enhanced backup process with duplicate prevention, improved temp directory handling, better error handling, and more robust cleanup procedures |
| app.py | Added thread-safe scheduler initialization, stuck job cleanup on startup, and improved job scheduling with duplicate prevention |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if retry_count >= max_retries: | ||
| raise Exception(f"Unable to clean temp directory after {max_retries} attempts: {e}") | ||
| # Add a small delay and try with a new timestamp | ||
| import time |
There was a problem hiding this comment.
Import statements should be placed at the top of the file, not inside functions. Move this import to the module level.
| logger.error(f"Failed to cleanup temp directory {temp_clone_dir}: {cleanup_error}") | ||
| # Try force cleanup | ||
| try: | ||
| import stat |
There was a problem hiding this comment.
Import statements should be placed at the top of the file, not inside functions. Move this import to the module level.
| else: | ||
| raise e | ||
| # Use git command directly for better error handling | ||
| import subprocess |
There was a problem hiding this comment.
Import statements should be placed at the top of the file, not inside functions. Move this import to the module level.
| logger.warning(f"Failed to remove temp directory {temp_dir}: {e}") | ||
| # Try to force remove if it's a permission issue | ||
| try: | ||
| import stat |
There was a problem hiding this comment.
Import statements should be placed at the top of the file, not inside functions. Move this import to the module level to avoid duplicate imports and follow Python conventions.
| try: | ||
| import stat | ||
| def handle_remove_readonly(func, path, exc): | ||
| if exc[1].errno == 13: # Permission denied |
There was a problem hiding this comment.
The magic number 13 for permission denied errno is duplicated. Consider defining a constant like PERMISSION_DENIED_ERRNO = 13 at the module level for better maintainability.
| backup_name = f"{repository.name}_{timestamp}" | ||
| # Generate timestamp for this backup with microseconds for uniqueness | ||
| timestamp = datetime.utcnow().strftime('%Y%m%d_%H%M%S_%f') | ||
| backup_name = f"{repository.name}_{timestamp[:19]}" # Keep readable format for backup name |
There was a problem hiding this comment.
The magic number 19 for string slicing is unclear. The comment mentions 'readable format' but it's not obvious why 19 characters. Consider using a more explicit approach or explaining the format in a constant.
|
|
||
| if repository.is_active: | ||
| # Wait a moment to ensure job removal is complete | ||
| import time |
There was a problem hiding this comment.
Import statements should be placed at the top of the file, not inside functions. Move this import to the module level.
|
|
||
| # Flag to ensure we only initialize once | ||
| # Thread-safe flag to ensure we only initialize once | ||
| import threading |
There was a problem hiding this comment.
Import statements should be placed at the top of the file, not in the middle of the module. Move this import to the module level imports section.
| try: | ||
| db.session.rollback() | ||
| except: |
There was a problem hiding this comment.
Using bare except: clause is discouraged as it catches all exceptions including system-exiting ones. Use except Exception: instead to catch only standard exceptions.
| if clone_dir.exists(): | ||
| try: | ||
| shutil.rmtree(clone_dir) | ||
| except: |
There was a problem hiding this comment.
Using bare except: clause is discouraged as it catches all exceptions including system-exiting ones. Use except Exception: instead to catch only standard exceptions.
No description provided.