Scheduler fixes #27
Scheduler fixes #27
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR addresses scheduler reliability issues by improving logging, preventing duplicate job schedules, and enhancing error handling for backup job scheduling.
- Enhanced logging throughout the scheduler initialization and job scheduling processes
- Added guards to prevent duplicate scheduler initialization using a global flag
- Improved job cleanup and verification with better error messages
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| from datetime import datetime # Import to ensure availability | ||
|
|
There was a problem hiding this comment.
Redundant import - datetime is already imported at line 2. This duplicate import should be removed.
| from datetime import datetime # Import to ensure availability | |
| def backup_with_context(): | ||
| from datetime import datetime, timedelta # Import inside function for closure scope | ||
|
|
||
| with app.app_context(): |
There was a problem hiding this comment.
Redundant import - both datetime and timedelta are already imported at line 2. This duplicate import should be removed.
| def backup_with_context(): | |
| from datetime import datetime, timedelta # Import inside function for closure scope | |
| with app.app_context(): | |
| def backup_with_context(): | |
| with app.app_context(): |
| logger.info("Scheduler initialization at startup completed") | ||
| except Exception as e: | ||
| logger.error(f"Failed to initialize scheduler at startup: {e}") | ||
| if not globals().get('_scheduler_startup_completed', False): |
There was a problem hiding this comment.
Using globals() for state management is an anti-pattern. Consider using a module-level variable instead, similar to how _scheduler_initialized is handled elsewhere in the code.
| for job in backup_jobs: | ||
| logger.info(f"Scheduled job: {job.id} -> next run: {job.next_run_time}") | ||
|
|
||
| globals()['_scheduler_startup_completed'] = True |
There was a problem hiding this comment.
Using globals() for state management is an anti-pattern. Consider using a module-level variable instead, similar to how _scheduler_initialized is handled elsewhere in the code.
| import traceback | ||
| logger.error(f"Traceback: {traceback.format_exc()}") | ||
| else: | ||
| logger.info("Scheduler startup initialization skipped - already completed") |
There was a problem hiding this comment.
Using globals() for state management is an anti-pattern. Consider using a module-level variable instead, similar to how _scheduler_initialized is handled elsewhere in the code.
No description provided.